Re: [Bug-wget] [PATCH v4] (resend) Add --use-askpass=COMMAND support

[Tim Rühsen]

On Mittwoch, 7. September 2016 12:15:26 CEST Liam R. Howlett wrote:
> * Tim Ruehsen <address@hidden> [160907 04:49]:
> > Hi Liam,
> > 
> > I made a mistake and accidentally pushed your v4 patch to master a few
> > days
> > ago.
> > 
> > Here is a fix for several issues (warnings, C89 incompatibility, using
> > strpbrk to find end-of-line, append newline to error messages).
> > 
> > I removed your extra checks, so that a user can just work with executables
> > in> 
> > the PATH (posix_spawnp searches the path):
> >   wget --use-askpass=ssh-askpass <URL>
> > 
> > If the executable does not exists (or something goes wrong when
> > executing),
> > wget will error & stop at that point anyways.
> > 
> > Here is the fix, WDYT ?
> 
> I like the change to use strpbrk.  This patch works with my testing.
> 
> Just a few comments:
> 
> 
> 1. Since you are using xmemdup0, can't you also drop these lines?
> (1101-1102):
>       /* Make sure there is a trailing 0 */
>             tmp[bytes] = '\0';

This is needed to have a trailing 0, else string functions - in this case 
strpbrk - would crash (not knowing where to stop).

> I think you can change the read line to remove the -1 from the size of
> the read (line 1092) if you drop the above:
> 
> - bytes = read (com[0], tmp, sizeof (tmp) - 1);
> + bytes = read (com[0], tmp, sizeof (tmp));

Need that -1 for the trailing 0 :-)

> 2. Is there a whitespace issue here?
> +  if ((p = strpbrk(tmp, "\r\n")))
>                    ^- Whitespace missing?

Yes, thank you !

Regards, Tim

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