Re: [Bug-wget] [PATCH v3] wget: Add --use-askpass=COMMAND support

[Liam R. Howlett]

* Tim Ruehsen <address@hidden> [160729 11:11]:
> On Thursday, July 28, 2016 3:53:28 PM CEST Liam R. Howlett wrote:
> > This adds the --use-askpass option which is disabled by default.
> > 
> > --use-askpass=COMMAND will request the username and password for a given
> > URL by executing the external program COMMAND.  If COMMAND is left
> > blank, then the external program in the environment variable
> > WGET_ASKPASS will be used.  If WGET_ASKPASS is not set then the
> > environment variable SSH_ASKPASS is used.  If there is no value set, an
> > error is returned.  If an error occurs requesting the username or
> > password, wget will exit.
> > 
> > Signed-off-by: Liam R. Howlett <address@hidden>
> > 
> > Liam R. Howlett (1):
> >   wget: Add --use-askpass=COMMAND support
> > 
> >  ChangeLog     |  16 +++++++++
> >  doc/wget.texi |   6 ++++
> >  src/init.c    |  22 ++++++++++++
> >  src/main.c    | 109
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ src/options.h | 
> >  1 +
> >  src/url.c     |   6 ++++
> >  src/url.h     |   1 +
> >  7 files changed, 161 insertions(+)
> 
> Hi Liam,
> 
> I just have a few comments...
> 
> 1. Don't edit ChangeLog. It is empty by purpose and will be generated from 
> ChangeLog-2014-12-10 and 'git log'. So please add the text from your 
> ChangeLog 
> as top part of the commit message. So that the commit message will look like
> 
> line with short description
> <empty line>
> what you have in ChangeLog
> <empty line>
> long description (what you have currently as commit message)
> 
> Just have a look with 'git log' for examples.

Thank you.  I had issues locating the git repo and was not sure how the
doc updates were done.  I have reverted this change in v4 and started
using the official git repo instead of the repo I created from the
archive version.

> 
> 2. In cmd_use_askpass(): If both WGET_ASKPASS and SSH_ASKPASS do not exist, 
> wouldn't it be good to warn the user (or even error & exit) ? Maybe the same 
> for empty env variables ?
> Also, feel free to move the variable declaration into the block where it is 
> used (good programming style - keep the scope of variables as small as 
> possible).

Sounds good.  I have made this change for v4.  I have also restructured
the code to avoid nesting if statements when implementing the error &
exit on no command.  The variable declaration will now have to remain at
the top to adhere to C99, but the code is easier to read.  This works
quite well with the .wgetrc support.  I have left the null check in
src/main.c for defensive programming.

> 
> 3. Please add the gnulib module 'posix_spawn' to bootstrap.conf. That 
> emulates 
> the posix spawn stuff in environments where it wouldn't be available normally.
> If you are interested, see https://www.gnu.org/software/gnulib/MODULES.html

Okay, I have this included in v4 as well.

> 
> 4. In the --help text, try to break your very long line into several shorter 
> lines.

Done in v4 as well.  I followed the formatting of having two space
indent for extra lines.

> 
> 5. In run_use_askpass():
>   bytes = read (com[0], tmp, sizeof (tmp));
> should be changed into
>   bytes = read (com[0], tmp, sizeof (tmp) - 1);
> because you need one byte left in tmp for your \0 termination byte.
> 

Thank you.  I did notice the bytes issues as well and I have addressed
this in v4.


> Your loop might crash. Rewrite it so that nbytes is the first thing to be 
> checked before you use it as array index. Also the first loop run is not 
> needed 
> - you already know that tmp[bytes] is \0.

I had not noticed the possible crash - I fixed it in v4.
I've optimized the first loop out as well.

> 
> Instead of strndup, please use xmemdup() - at that time you already know the 
> length of the string. strndup will redundantly determine the length of the 
> string again.

I've also changed strndup to xmemdup.

> 
> 6. In use_askpass():
> Please get rid of max_size. Use sizeof(question) instead.
> Also please use xstrdup() - it will handle out-of-memory situations 
> gracefully.

Done for v4.

Thank you,
Liam

> 
> 
> Regards, Tim