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

[Tim Ruehsen]

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.

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).

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

4. In the --help text, try to break your very long line into several shorter 
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.

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.

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.

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.


Regards, Tim

signature.asc
Description: This is a digitally signed message part.