Drop support for the oldstyle protocol
authorWouter Verhelst <w@uter.be>
Sun, 8 Mar 2015 23:03:52 +0000 (00:03 +0100)
committerWouter Verhelst <w@uter.be>
Sun, 8 Mar 2015 23:18:08 +0000 (00:18 +0100)
The newstyle protocol was originally written for NBD 2.9.17, released in
2009. We are 2015 now. People have had over five years to migrate their
setups now.

Additionally, due to the fact that I originally wrote support for the
newstyle protocol in a somewhat confusing way, maintaining that part of
the code has become harder than it needs be. A bug did now appear
because I couldn't read my own code anymore.

While it should, in theory, be possible to just fix the damn code while
still retaining support for the oldstyle protocol, it's easier (and not
too unreasonable) to just drop that instead.

Also fix the test suite so it doesn't try to run tests using the oldstyle
protocol anymore

nbd-client.c
nbd-server.c
tests/run/Makefile.am
tests/run/simple_test

index 3146e8c..f367e05 100644 (file)
@@ -260,10 +260,13 @@ void ask_list(int sock) {
                err("Failed writing length");
 }
 
-void negotiate(int sock, u64 *rsize64, u32 *flags, char* name, uint32_t needed_flags, uint32_t client_flags, uint32_t do_opts) {
+void negotiate(int sock, u64 *rsize64, uint16_t *flags, char* name, uint32_t needed_flags, uint32_t client_flags, uint32_t do_opts) {
        u64 magic, size64;
        uint16_t tmp;
+       uint16_t global_flags;
        char buf[256] = "\0\0\0\0\0\0\0\0\0";
+       uint32_t opt;
+       uint32_t namesize;
 
        printf("Negotiation: ");
        if (read(sock, buf, 8) < 0)
@@ -276,64 +279,51 @@ void negotiate(int sock, u64 *rsize64, u32 *flags, char* name, uint32_t needed_f
        if (read(sock, &magic, sizeof(magic)) < 0)
                err("Failed/2: %m");
        magic = ntohll(magic);
-       if(name) {
-               uint32_t opt;
-               uint32_t namesize;
-
-               if (magic != opts_magic) {
-                       if(magic == cliserv_magic) {
-                               err("It looks like you're trying to connect to an oldstyle server with a named export. This won't work.");
-                       }
-               }
-               printf(".");
-               if(read(sock, &tmp, sizeof(uint16_t)) < 0) {
-                       err("Failed reading flags: %m");
+       if (magic != opts_magic) {
+               if(magic == cliserv_magic) {
+                       err("It looks like you're trying to connect to an oldstyle server. This is no longer supported since nbd 3.10.");
                }
-               *flags = ((u32)ntohs(tmp));
-               if((needed_flags & *flags) != needed_flags) {
-                       /* There's currently really only one reason why this
-                        * check could possibly fail, but we may need to change
-                        * this error message in the future... */
-                       fprintf(stderr, "\nE: Server does not support listing exports\n");
-                       exit(EXIT_FAILURE);
-               }
-
-               if (*flags & NBD_FLAG_NO_ZEROES) {
-                       client_flags |= NBD_FLAG_C_NO_ZEROES;
-               }
-               client_flags = htonl(client_flags);
-               if (write(sock, &client_flags, sizeof(client_flags)) < 0)
-                       err("Failed/2.1: %m");
+       }
+       printf(".");
+       if(read(sock, &tmp, sizeof(uint16_t)) < 0) {
+               err("Failed reading flags: %m");
+       }
+       global_flags = ntohs(tmp);
+       if((needed_flags & *flags) != needed_flags) {
+               /* There's currently really only one reason why this
+                * check could possibly fail, but we may need to change
+                * this error message in the future... */
+               fprintf(stderr, "\nE: Server does not support listing exports\n");
+               exit(EXIT_FAILURE);
+       }
 
-               if(do_opts & NBDC_DO_LIST) {
-                       ask_list(sock);
-                       exit(EXIT_SUCCESS);
-               }
+       if (global_flags & NBD_FLAG_NO_ZEROES) {
+               client_flags |= NBD_FLAG_C_NO_ZEROES;
+       }
+       client_flags = htonl(client_flags);
+       if (write(sock, &client_flags, sizeof(client_flags)) < 0)
+               err("Failed/2.1: %m");
 
-               /* Write the export name that we're after */
-               magic = htonll(opts_magic);
-               if (write(sock, &magic, sizeof(magic)) < 0)
-                       err("Failed/2.2: %m");
-
-               opt = ntohl(NBD_OPT_EXPORT_NAME);
-               if (write(sock, &opt, sizeof(opt)) < 0)
-                       err("Failed/2.3: %m");
-               namesize = (u32)strlen(name);
-               namesize = ntohl(namesize);
-               if (write(sock, &namesize, sizeof(namesize)) < 0)
-                       err("Failed/2.4: %m");
-               if (write(sock, name, strlen(name)) < 0)
-                       err("Failed/2.4: %m");
-       } else {
-               if (magic != cliserv_magic) {
-                       if(magic != opts_magic)
-                               err("Not enough cliserv_magic");
-                       else
-                               err("It looks like you're trying to connect to a newstyle server with the oldstyle protocol. Try the -N option.");
-               }
-               printf(".");
+       if(do_opts & NBDC_DO_LIST) {
+               ask_list(sock);
+               exit(EXIT_SUCCESS);
        }
 
+       /* Write the export name that we're after */
+       magic = htonll(opts_magic);
+       if (write(sock, &magic, sizeof(magic)) < 0)
+               err("Failed/2.2: %m");
+
+       opt = ntohl(NBD_OPT_EXPORT_NAME);
+       if (write(sock, &opt, sizeof(opt)) < 0)
+               err("Failed/2.3: %m");
+       namesize = (u32)strlen(name);
+       namesize = ntohl(namesize);
+       if (write(sock, &namesize, sizeof(namesize)) < 0)
+               err("Failed/2.4: %m");
+       if (write(sock, name, strlen(name)) < 0)
+               err("Failed/2.4: %m");
+
        if (read(sock, &size64, sizeof(size64)) <= 0) {
                if (!errno)
                        err("Server closed connection");
@@ -347,15 +337,9 @@ void negotiate(int sock, u64 *rsize64, u32 *flags, char* name, uint32_t needed_f
        } else
                printf("size = %luMB", (unsigned long)(size64>>20));
 
-       if(!name) {
-               if (read(sock, flags, sizeof(*flags)) < 0)
-                       err("Failed/4: %m\n");
-               *flags = ntohl(*flags);
-       } else {
-               if(read(sock, &tmp, sizeof(tmp)) < 0)
-                       err("Failed/4: %m\n");
-               *flags |= (uint32_t)ntohs(tmp);
-       }
+       if(read(sock, &tmp, sizeof(tmp)) < 0)
+               err("Failed/4: %m\n");
+       *flags = (uint32_t)ntohs(tmp);
 
        if (!(*flags & NBD_FLAG_NO_ZEROES)) {
                if (read(sock, &buf, 124) < 0)
@@ -436,8 +420,7 @@ void usage(char* errmsg, ...) {
        } else {
                fprintf(stderr, "nbd-client version %s\n", PACKAGE_VERSION);
        }
-       fprintf(stderr, "Usage: nbd-client host port nbd_device [-block-size|-b block size] [-timeout|-t timeout] [-swap|-s] [-sdp|-S] [-persist|-p] [-nofork|-n] [-systemd-mark|-m]\n");
-       fprintf(stderr, "Or   : nbd-client -name|-N name host [port] nbd_device [-block-size|-b block size] [-timeout|-t timeout] [-swap|-s] [-sdp|-S] [-persist|-p] [-nofork|-n]\n");
+       fprintf(stderr, "Usage: nbd-client -name|-N name host [port] nbd_device [-block-size|-b block size] [-timeout|-t timeout] [-swap|-s] [-sdp|-S] [-persist|-p] [-nofork|-n] [-systemd-mark|-m]\n");
        fprintf(stderr, "Or   : nbd-client -d nbd_device\n");
        fprintf(stderr, "Or   : nbd-client -c nbd_device\n");
        fprintf(stderr, "Or   : nbd-client -h|--help\n");
@@ -475,12 +458,12 @@ int main(int argc, char *argv[]) {
        int sdp=0;
        int G_GNUC_UNUSED nofork=0; // if -dNOFORK
        u64 size64;
-       u32 flags;
+       uint16_t flags;
        int c;
        int nonspecial=0;
        int b_unix=0;
        char* name=NULL;
-       uint32_t needed_flags=0;
+       uint16_t needed_flags=0;
        uint32_t cflags=NBD_FLAG_C_FIXED_NEWSTYLE;
        uint32_t opts=0;
        sigset_t block, old;
@@ -531,7 +514,7 @@ int main(int argc, char *argv[]) {
                                case 1:
                                        // port
                                        if(!strtol(optarg, NULL, 0)) {
-                                               // not parseable as a number, assume it's the device and we have a name
+                                               // not parseable as a number, assume it's the device
                                                nbddev = optarg;
                                                nonspecial++;
                                        } else {
@@ -605,7 +588,7 @@ int main(int argc, char *argv[]) {
     err("swap option unsupported on Android because mlockall is unsupported.");
 #endif
 
-       if((!port && !name) || !hostname || !nbddev) {
+       if(!name || !hostname || !nbddev || !(opts & NBDC_DO_LIST)) {
                usage("not enough information specified");
                exit(EXIT_FAILURE);
        }
@@ -685,7 +668,7 @@ int main(int argc, char *argv[]) {
                        } else {
                                if(cont) {
                                        u64 new_size;
-                                       u32 new_flags;
+                                       uint16_t new_flags;
 
                                        close(sock); close(nbd);
                                        for (;;) {
index 99cf183..0a92f5c 100644 (file)
@@ -679,18 +679,6 @@ GArray* do_cfile_dir(gchar* dir, struct generic_conf *const genconf, GError** e)
        return retval;
 }
 
-static bool want_oldstyle(struct generic_conf gct, struct generic_conf* gc) {
-       if(gct.flags & F_OLDSTYLE) {
-               return true;
-       }
-       if(gc == NULL) {
-               return false;
-       }
-       if(gc->flags & F_OLDSTYLE) {
-               return true;
-       }
-       return false;
-}
 /**
  * Parse the config file.
  *
@@ -720,7 +708,6 @@ GArray* parse_cfile(gchar* f, struct generic_conf *const genconf, bool expect_ge
        gchar *virtstyle=NULL;
        PARAM lp[] = {
                { "exportname", TRUE,   PARAM_STRING,   &(s.exportname),        0 },
-               { "port",       TRUE,   PARAM_INT,      &(s.port),              0 },
                { "authfile",   FALSE,  PARAM_STRING,   &(s.authname),          0 },
                { "filesize",   FALSE,  PARAM_OFFT,     &(s.expected_size),     0 },
                { "virtstyle",  FALSE,  PARAM_STRING,   &(virtstyle),           0 },
@@ -748,7 +735,7 @@ GArray* parse_cfile(gchar* f, struct generic_conf *const genconf, bool expect_ge
        PARAM gp[] = {
                { "user",       FALSE, PARAM_STRING,    &(genconftmp.user),       0 },
                { "group",      FALSE, PARAM_STRING,    &(genconftmp.group),      0 },
-               { "oldstyle",   FALSE, PARAM_BOOL,      &(genconftmp.flags),      F_OLDSTYLE },
+               { "oldstyle",   FALSE, PARAM_BOOL,      &(genconftmp.flags),      F_OLDSTYLE }, // only left here so we can issue an appropriate error message when the option is used
                { "listenaddr", FALSE, PARAM_STRING,    &(genconftmp.modernaddr), 0 },
                { "port",       FALSE, PARAM_STRING,    &(genconftmp.modernport), 0 },
                { "includedir", FALSE, PARAM_STRING,    &cfdir,                   0 },
@@ -803,9 +790,6 @@ GArray* parse_cfile(gchar* f, struct generic_conf *const genconf, bool expect_ge
                if(i==1 || !expect_generic) {
                        p=lp;
                        p_size=lp_size;
-                       if(!want_oldstyle(genconftmp, genconf)) {
-                               lp[1].required = FALSE;
-                       }
                } 
                for(j=0;j<p_size;j++) {
                        assert(p[j].target != NULL);
@@ -895,9 +879,10 @@ GArray* parse_cfile(gchar* f, struct generic_conf *const genconf, bool expect_ge
                } else {
                        s.virtstyle=VIRT_IPLIT;
                }
-               if(s.port && !want_oldstyle(genconftmp, genconf)) {
-                       g_warning("A port was specified, but oldstyle exports were not requested. This may not do what you expect.");
-                       g_warning("Please read 'man 5 nbd-server' and search for oldstyle for more info");
+               if(genconftmp.flags & F_OLDSTYLE) {
+                       g_message("Since 3.10, the oldstyle protocol is no longer supported. Please migrate to the newstyle protocol.");
+                       g_message("Exiting.");
+                       return NULL;
                }
                /* Don't need to free this, it's not our string */
                virtstyle=NULL;
@@ -1484,91 +1469,69 @@ static void handle_list(uint32_t opt, int net, GArray* servers, uint32_t cflags)
  *
  * @param client The client we're negotiating with.
  **/
-CLIENT* negotiate(int net, CLIENT *client, GArray* servers, int phase) {
-       char zeros[128];
-       uint64_t size_host;
+CLIENT* negotiate(int net, GArray* servers) {
        uint32_t flags = NBD_FLAG_HAS_FLAGS;
-       uint16_t smallflags = 0;
+       uint16_t smallflags = NBD_FLAG_FIXED_NEWSTYLE | NBD_FLAG_NO_ZEROES;
        uint64_t magic;
        uint32_t cflags = 0;
-
-       memset(zeros, '\0', sizeof(zeros));
-       assert(((phase & NEG_INIT) && (phase & NEG_MODERN)) || client);
-       if(phase & NEG_MODERN) {
-               smallflags |= NBD_FLAG_FIXED_NEWSTYLE | NBD_FLAG_NO_ZEROES;
-       }
-       if(phase & NEG_INIT) {
-               /* common */
-               if (write(net, INIT_PASSWD, 8) < 0) {
-                       err_nonfatal("Negotiation failed/1: %m");
-                       if(client)
-                               exit(EXIT_FAILURE);
-               }
-               if(phase & NEG_MODERN) {
-                       /* modern */
-                       magic = htonll(opts_magic);
-               } else {
-                       /* oldstyle */
-                       magic = htonll(cliserv_magic);
-               }
-               if (write(net, &magic, sizeof(magic)) < 0) {
-                       err_nonfatal("Negotiation failed/2: %m");
-                       if(phase & NEG_OLD)
-                               exit(EXIT_FAILURE);
-               }
+       uint32_t opt;
+
+       assert(servers != NULL);
+       if (write(net, INIT_PASSWD, 8) < 0)
+               err_nonfatal("Negotiation failed/1: %m");
+       magic = htonll(opts_magic);
+       if (write(net, &magic, sizeof(magic)) < 0)
+               err_nonfatal("Negotiation failed/2: %m");
+
+       smallflags = htons(smallflags);
+       if (write(net, &smallflags, sizeof(uint16_t)) < 0)
+               err_nonfatal("Negotiation failed/3: %m");
+       if (read(net, &cflags, sizeof(cflags)) < 0)
+               err_nonfatal("Negotiation failed/4: %m");
+       cflags = htonl(cflags);
+       if (cflags & NBD_FLAG_C_NO_ZEROES) {
+               glob_flags |= F_NO_ZEROES;
        }
-       if ((phase & NEG_MODERN) && (phase & NEG_INIT)) {
-               /* modern */
-               uint32_t opt;
-
-               if(!servers)
-                       err("programmer error");
-               smallflags = htons(smallflags);
-               if (write(net, &smallflags, sizeof(uint16_t)) < 0)
-                       err_nonfatal("Negotiation failed/3: %m");
-               if (read(net, &cflags, sizeof(cflags)) < 0)
-                       err_nonfatal("Negotiation failed/4: %m");
-               cflags = htonl(cflags);
-               if (cflags & NBD_FLAG_C_NO_ZEROES) {
-                       glob_flags |= F_NO_ZEROES;
-               }
-               do {
-                       if (read(net, &magic, sizeof(magic)) < 0)
-                               err_nonfatal("Negotiation failed/5: %m");
-                       magic = ntohll(magic);
-                       if(magic != opts_magic) {
-                               err_nonfatal("Negotiation failed/5a: magic mismatch");
-                               return NULL;
-                       }
-                       if (read(net, &opt, sizeof(opt)) < 0)
-                               err_nonfatal("Negotiation failed/6: %m");
-                       opt = ntohl(opt);
-                       switch(opt) {
-                       case NBD_OPT_EXPORT_NAME:
-                               // NBD_OPT_EXPORT_NAME must be the last
-                               // selected option, so return from here
-                               // if that is chosen.
-                               return handle_export_name(opt, net, servers, cflags);
-                               break;
-                       case NBD_OPT_LIST:
-                               handle_list(opt, net, servers, cflags);
-                               break;
-                       case NBD_OPT_ABORT:
-                               // handled below
-                               break;
-                       default:
-                               send_reply(opt, net, NBD_REP_ERR_UNSUP, 0, NULL);
-                               break;
-                       }
-               } while((opt != NBD_OPT_EXPORT_NAME) && (opt != NBD_OPT_ABORT));
-               if(opt == NBD_OPT_ABORT) {
-                       err_nonfatal("Session terminated by client");
+       do {
+               if (read(net, &magic, sizeof(magic)) < 0)
+                       err_nonfatal("Negotiation failed/5: %m");
+               magic = ntohll(magic);
+               if(magic != opts_magic) {
+                       err_nonfatal("Negotiation failed/5a: magic mismatch");
                        return NULL;
                }
+               if (read(net, &opt, sizeof(opt)) < 0)
+                       err_nonfatal("Negotiation failed/6: %m");
+               opt = ntohl(opt);
+               switch(opt) {
+               case NBD_OPT_EXPORT_NAME:
+                       // NBD_OPT_EXPORT_NAME must be the last
+                       // selected option, so return from here
+                       // if that is chosen.
+                       return handle_export_name(opt, net, servers, cflags);
+                       break;
+               case NBD_OPT_LIST:
+                       handle_list(opt, net, servers, cflags);
+                       break;
+               case NBD_OPT_ABORT:
+                       // handled below
+                       break;
+               default:
+                       send_reply(opt, net, NBD_REP_ERR_UNSUP, 0, NULL);
+                       break;
+               }
+       } while((opt != NBD_OPT_EXPORT_NAME) && (opt != NBD_OPT_ABORT));
+       if(opt == NBD_OPT_ABORT) {
+               err_nonfatal("Session terminated by client");
+               return NULL;
        }
-       /* common */
+}
+
+void send_export_info(CLIENT* client) {
+       uint64_t size_host;
+       uint16_t flags;
        size_host = htonll((u64)(client->exportsize));
-       if (write(net, &size_host, 8) < 0)
+       if (write(client->net, &size_host, 8) < 0)
                err("Negotiation failed/9: %m");
        if (client->server->flags & F_READONLY)
                flags |= NBD_FLAG_READ_ONLY;
@@ -1580,24 +1543,14 @@ CLIENT* negotiate(int net, CLIENT *client, GArray* servers, int phase) {
                flags |= NBD_FLAG_ROTATIONAL;
        if (client->server->flags & F_TRIM)
                flags |= NBD_FLAG_SEND_TRIM;
-       if (phase & NEG_OLD) {
-               /* oldstyle */
-               flags = htonl(flags);
-               if (write(client->net, &flags, 4) < 0)
-                       err("Negotiation failed/10: %m");
-       } else {
-               /* modern */
-               smallflags = (uint16_t)(flags & ~((uint16_t)0));
-               smallflags = htons(smallflags);
-               if (write(client->net, &smallflags, sizeof(smallflags)) < 0) {
-                       err("Negotiation failed/11: %m");
-               }
-       }
-       /* common */
+       flags = htons(flags);
+       if (write(client->net, &flags, sizeof(flags)) < 0)
+               err("Negotiation failed/11: %m");
        if (!(glob_flags & F_NO_ZEROES)) {
+               char zeros[128];
+               memset(zeros, '\0', sizeof(zeros));
                if (write(client->net, zeros, 124) < 0)
                        err("Negotiation failed/12: %m");
-               return NULL;
        }
 }
 
@@ -1623,7 +1576,7 @@ int mainloop(CLIENT *client) {
 #ifdef DODBG
        int i = 0;
 #endif
-       negotiate(client->net, client, NULL, client->modern ? NEG_MODERN : (NEG_OLD | NEG_INIT));
+       send_export_info(client);
        DEBUG("Entering request loop!\n");
        reply.magic = htonl(NBD_REPLY_MAGIC);
        reply.error = 0;
@@ -2155,7 +2108,7 @@ handle_modern_connection(GArray *const servers, const int sock)
                 /* Child just continues. */
         }
 
-        client = negotiate(net, NULL, servers, NEG_INIT | NEG_MODERN);
+        client = negotiate(net, servers);
         if (!client) {
                 msg(LOG_ERR, "Modern initial negotiation failed");
                 goto handler_err;
index 2a54d4f..5677bcf 100644 (file)
@@ -1,12 +1,11 @@
 TESTS_ENVIRONMENT=$(srcdir)/simple_test
-TESTS = cmd cfg1 cfgmulti cfgnew cfgsize write flush integrity dirconfig list rowrite tree rotree unix #integrityhuge
+TESTS = cfg1 cfgmulti cfgnew cfgsize write flush integrity dirconfig list rowrite tree rotree unix #integrityhuge
 check_PROGRAMS = nbd-tester-client
 nbd_tester_client_SOURCES = nbd-tester-client.c $(top_srcdir)/cliserv.h $(top_srcdir)/netdb-compat.h $(top_srcdir)/cliserv.c
 nbd_tester_client_CFLAGS = @CFLAGS@ @GLIB_CFLAGS@
 nbd_tester_client_CPPFLAGS = -I$(top_srcdir)
 nbd_tester_client_LDADD = @GLIB_LIBS@
 EXTRA_DIST = integrity-test.tr integrityhuge-test.tr simple_test
-cmd:
 cfg1:
 cfgmulti:
 cfgnew:
index 23daf97..e7e4c41 100755 (executable)
@@ -41,39 +41,32 @@ dd if=/dev/zero of=$tmpnam bs=1024 count=4096 >/dev/null 2>&1
 echo $1
 
 case $1 in
-       */cmd)
-               # Test with export specified on command line
-               ../../nbd-server -C /dev/null -p ${pidfile} 11111 $tmpnam &
-               # -p only works if nbd-server wasn't compiled with -DNOFORK or
-               # -DNODAEMON, which I sometimes do for testing and debugging.
-               PID=$!
-               sleep 1
-               ./nbd-tester-client 127.0.0.1 11111
-               retval=$?
-       ;;
        */cfgsize)
                # Test oversized requests
-               ../../nbd-server -C /dev/null -p ${pidfile} 11112 $tmpnam &
+               cat > ${conffile} <<EOF
+[generic]
+[export]
+       exportname = $tmpnam
+EOF
+               ../../nbd-server -C ${conffile} -p ${pidfile} &
                # -p only works if nbd-server wasn't compiled with -DNOFORK or
                # -DNODAEMON, which I sometimes do for testing and debugging.
                PID=$!
                sleep 1
-               ./nbd-tester-client -o 127.0.0.1 11112
+               ./nbd-tester-client -o 127.0.0.1 -N export
                retval=$?
        ;;
        */cfg1)
                # Test with export specified in config file
                cat > ${conffile} <<EOF
 [generic]
-       oldstyle = true
 [export]
        exportname = $tmpnam
-       port = 11113
 EOF
                ../../nbd-server -C ${conffile} -p ${pidfile} &
                PID=$!
                sleep 1
-               ./nbd-tester-client 127.0.0.1 11113
+               ./nbd-tester-client 127.0.0.1 -N export
                retval=$?
        ;;
        */cfgmulti)
@@ -81,22 +74,19 @@ EOF
                # testing more options too
                cat >${conffile} <<EOF
 [generic]
-       oldstyle = true
 [export1]
        exportname = $tmpnam
-       port = 11114
        copyonwrite = true
        listenaddr = 127.0.0.1
 [export2]
        exportname = $tmpnam
-       port = 11115
        readonly = true
        listenaddr = 127.0.0.1
 EOF
                ../../nbd-server -C ${conffile} -p ${pidfile} &
                PID=$!
                sleep 1
-               ./nbd-tester-client localhost 11114
+               ./nbd-tester-client localhost -N export1
                retval=$?
                if [ $retval -ne 0 ]
                then
@@ -112,7 +102,7 @@ EOF
                        fi
                        exit $retval
                fi
-               ./nbd-tester-client localhost 11115
+               ./nbd-tester-client localhost -N export2
                retval=$?
        ;;
        */cfgnew)