Re: ftp(1) User-Agent

[Alexander Hall]

On 07/11/14 17:35, Lawrence Teo wrote:
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.

Mainly because that's how I would expect any option to work. -o, just to 
give one example.

hmmm.. use case:

getfile() {
        ftp -U 'firefox' "$@"
}

getfile http://foo.bar/baz1
getfile http://foo.bar/baz2

getfile -U 'chrome' http://foo.bar/baz3

/Alexander

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 */