On Fri, Jul 11, 2014 at 12:20:00PM +0200, Alexander Hall wrote: > On 07/10/14 06:30, Lawrence Teo wrote: > > About a month ago, I sent a diff that allows ftp(1) to set its > > User-Agent. > > > > Based on feedback from halex@ and deraadt@, I have changed it so that > > the User-Agent can be set via a -U command-line option instead of an > > environment variable. > > > > I have also fixed a conflict with guenther@'s recent fetch.c commit. > > > > Would anyone like to ok this latest version? > > I was reviewing this and I couldn't help finding it unnecessarily > cumbersome. > > I propose this diff (ontop on the already proposed and committed diff). > Apart from making the code simpler, this diff will change two things:
Thanks for simplifying this. The original diff used an environment variable and for consistency with the existing code that deals with environment variables, I implemented it within auto_fetch(). When I changed it to use a command-line option, I continued implementing it within auto_fetch() because that was where my original code was. But as your diff shows, that's unnecessary, so I appreciate your work in making it less cumbersome. I agree with your diff except for this part: > 1. You may specify -U as many times as you please, using only the last > one. This is the behavious I'd expect. What is the use case for specifying multiple -U instances and only choosing the last one? To me that sounds like something I would accidentally do as opposed to something I would intentionally do, so that's why my code tried to prevent it. > 2. If you compile with -DSMALL, using -U will produce an error. This > does not follow the common, IMO questionable, practice of just > ignoring the switches. However I find it a totally reasonable > for most unavailable switches (possibly -C aside). > > OK? > > /Alexander > > > Index: fetch.c > =================================================================== > RCS file: /cvs/src/usr.bin/ftp/fetch.c,v > retrieving revision 1.124 > diff -u -p -r1.124 fetch.c > --- fetch.c 11 Jul 2014 03:31:52 -0000 1.124 > +++ fetch.c 11 Jul 2014 10:18:25 -0000 > @@ -1284,9 +1284,6 @@ auto_fetch(int argc, char *argv[], char > char *cp, *url, *host, *dir, *file, *portnum; > char *username, *pass, *pathstart; > char *ftpproxy, *httpproxy; > -#ifndef SMALL > - char *uagent = NULL; > -#endif /* !SMALL */ > int rval, xargc; > volatile int argpos; > int dirhasglob, filehasglob, oautologin; > @@ -1307,13 +1304,6 @@ auto_fetch(int argc, char *argv[], char > if ((httpproxy = getenv(HTTP_PROXY)) != NULL && *httpproxy == '\0') > httpproxy = NULL; > > - if (httpuseragent == NULL) > - httpuseragent = HTTP_USER_AGENT; > -#ifndef SMALL > - else > - uagent = httpuseragent; > -#endif /* !SMALL */ > - > /* > * Loop through as long as there's files to fetch. > */ > @@ -1590,9 +1580,6 @@ bad_ftp_url: > } > if (connected && rval != -1) > disconnect(0, NULL); > -#ifndef SMALL > - free(uagent); > -#endif /* !SMALL */ > return (rval); > } > > Index: main.c > =================================================================== > RCS file: /cvs/src/usr.bin/ftp/main.c,v > retrieving revision 1.88 > diff -u -p -r1.88 main.c > --- main.c 11 Jul 2014 03:31:52 -0000 1.88 > +++ main.c 11 Jul 2014 10:18:25 -0000 > @@ -362,19 +362,17 @@ main(volatile int argc, char *argv[]) > trace = 1; > break; > > - case 'U': > #ifndef SMALL > - if (httpuseragent) > - errx(1, "User-Agent was already defined"); > - /* Ensure that User-Agent value is in a single line. */ > + case 'U': > + free (httpuseragent); > if (strcspn(optarg, "\r\n") != strlen(optarg)) > errx(1, "Invalid User-Agent: %s.", optarg); > if (asprintf(&httpuseragent, "User-Agent: %s", > optarg) == -1) > errx(1, "Can't allocate memory for HTTP(S) " > "User-Agent"); > -#endif /* !SMALL */ > break; > +#endif /* !SMALL */ > > case 'v': > verbose = 1; > @@ -394,6 +392,8 @@ main(volatile int argc, char *argv[]) > #ifndef SMALL > cookie_load(); > #endif /* !SMALL */ > + if (httpuseragent == NULL) > + httpuseragent = HTTP_USER_AGENT; > > cpend = 0; /* no pending replies */ > proxy = 0; /* proxy not active */ >
