unixctl: Implement quoting.

The protocol used by ovs-appctl has a long-standing bug that there
is no way to distinguish "ovs-appctl a b c" from "ovs-appctl 'a b c'".
This isn't a big deal because none of the current commands really
want to accept arguments that include spaces, but it's kind of a silly
limitation.

At the same time, the internal API is awkward because every user is
stuck doing its own argument parsing, which is no fun.

This commit fixes both problems, by adding shell-like quoting to the
protocol and modifying the internal API from one that passes a string
to one that passes in an array of pre-parsed strings.  Command
implementations may now specify how many arguments they expect.  This
simplifies some command implementations significantly.

Signed-off-by: Ben Pfaff <blp@nicira.com>

diff --git a/lib/bond.c b/lib/bond.c
index bb5d49968b70186e389ff4b4cacff4c7aa0d9a21..84a1117a636380546b44c8bf97964ab336dfacad 100644 (file)
--- a/lib/bond.c
+++ b/lib/bond.c
@@ -906,7 +906,8 @@ bond_lookup_slave(struct bond *bond, const char *slave_name)
 
 static void
 bond_unixctl_list(struct unixctl_conn *conn,
-                  const char *args OVS_UNUSED, void *aux OVS_UNUSED)
+                  int argc OVS_UNUSED, const char *argv[] OVS_UNUSED,
+                  void *aux OVS_UNUSED)
 {
     struct ds ds = DS_EMPTY_INITIALIZER;
     const struct bond *bond;
@@ -935,13 +936,14 @@ bond_unixctl_list(struct unixctl_conn *conn,
 
 static void
 bond_unixctl_show(struct unixctl_conn *conn,
-                  const char *args, void *aux OVS_UNUSED)
+                  int argc OVS_UNUSED, const char *argv[],
+                  void *aux OVS_UNUSED)
 {
     struct ds ds = DS_EMPTY_INITIALIZER;
     const struct bond_slave *slave;
     const struct bond *bond;
 
-    bond = bond_find(args);
+    bond = bond_find(argv[1]);
     if (!bond) {
         unixctl_command_reply(conn, 501, "no such bond");
         return;
@@ -1009,26 +1011,18 @@ bond_unixctl_show(struct unixctl_conn *conn,
 }
 
 static void
-bond_unixctl_migrate(struct unixctl_conn *conn, const char *args_,
+bond_unixctl_migrate(struct unixctl_conn *conn,
+                     int argc OVS_UNUSED, const char *argv[],
                      void *aux OVS_UNUSED)
 {
-    char *args = (char *) args_;
-    char *save_ptr = NULL;
-    char *bond_s, *hash_s, *slave_s;
+    const char *bond_s = argv[1];
+    const char *hash_s = argv[2];
+    const char *slave_s = argv[3];
     struct bond *bond;
     struct bond_slave *slave;
     struct bond_entry *entry;
     int hash;
 
-    bond_s = strtok_r(args, " ", &save_ptr);
-    hash_s = strtok_r(NULL, " ", &save_ptr);
-    slave_s = strtok_r(NULL, " ", &save_ptr);
-    if (!slave_s) {
-        unixctl_command_reply(conn, 501,
-                              "usage: bond/migrate BOND HASH SLAVE");
-        return;
-    }
-
     bond = bond_find(bond_s);
     if (!bond) {
         unixctl_command_reply(conn, 501, "no such bond");
@@ -1066,23 +1060,15 @@ bond_unixctl_migrate(struct unixctl_conn *conn, const char *args_,
 }
 
 static void
-bond_unixctl_set_active_slave(struct unixctl_conn *conn, const char *args_,
+bond_unixctl_set_active_slave(struct unixctl_conn *conn,
+                              int argc OVS_UNUSED, const char *argv[],
                               void *aux OVS_UNUSED)
 {
-    char *args = (char *) args_;
-    char *save_ptr = NULL;
-    char *bond_s, *slave_s;
+    const char *bond_s = argv[1];
+    const char *slave_s = argv[2];
     struct bond *bond;
     struct bond_slave *slave;
 
-    bond_s = strtok_r(args, " ", &save_ptr);
-    slave_s = strtok_r(NULL, " ", &save_ptr);
-    if (!slave_s) {
-        unixctl_command_reply(conn, 501,
-                              "usage: bond/set-active-slave BOND SLAVE");
-        return;
-    }
-
     bond = bond_find(bond_s);
     if (!bond) {
         unixctl_command_reply(conn, 501, "no such bond");
@@ -1114,24 +1100,13 @@ bond_unixctl_set_active_slave(struct unixctl_conn *conn, const char *args_,
 }
 
 static void
-enable_slave(struct unixctl_conn *conn, const char *args_, bool enable)
+enable_slave(struct unixctl_conn *conn, const char *argv[], bool enable)
 {
-    char *args = (char *) args_;
-    char *save_ptr = NULL;
-    char *bond_s, *slave_s;
+    const char *bond_s = argv[1];
+    const char *slave_s = argv[2];
     struct bond *bond;
     struct bond_slave *slave;
 
-    bond_s = strtok_r(args, " ", &save_ptr);
-    slave_s = strtok_r(NULL, " ", &save_ptr);
-    if (!slave_s) {
-        char *usage = xasprintf("usage: bond/%s-slave BOND SLAVE",
-                                enable ? "enable" : "disable");
-        unixctl_command_reply(conn, 501, usage);
-        free(usage);
-        return;
-    }
-
     bond = bond_find(bond_s);
     if (!bond) {
         unixctl_command_reply(conn, 501, "no such bond");
@@ -1149,35 +1124,33 @@ enable_slave(struct unixctl_conn *conn, const char *args_, bool enable)
 }
 
 static void
-bond_unixctl_enable_slave(struct unixctl_conn *conn, const char *args,
+bond_unixctl_enable_slave(struct unixctl_conn *conn,
+                          int argc OVS_UNUSED, const char *argv[],
                           void *aux OVS_UNUSED)
 {
-    enable_slave(conn, args, true);
+    enable_slave(conn, argv, true);
 }
 
 static void
-bond_unixctl_disable_slave(struct unixctl_conn *conn, const char *args,
+bond_unixctl_disable_slave(struct unixctl_conn *conn,
+                           int argc OVS_UNUSED, const char *argv[],
                            void *aux OVS_UNUSED)
 {
-    enable_slave(conn, args, false);
+    enable_slave(conn, argv, false);
 }
 
 static void
-bond_unixctl_hash(struct unixctl_conn *conn, const char *args_,
+bond_unixctl_hash(struct unixctl_conn *conn, int argc, const char *argv[],
                   void *aux OVS_UNUSED)
 {
-    char *args = (char *) args_;
+    const char *mac_s = argv[1];
+    const char *vlan_s = argc > 2 ? argv[2] : NULL;
+    const char *basis_s = argc > 3 ? argv[3] : NULL;
     uint8_t mac[ETH_ADDR_LEN];
     uint8_t hash;
     char *hash_cstr;
     unsigned int vlan;
     uint32_t basis;
-    char *mac_s, *vlan_s, *basis_s;
-    char *save_ptr = NULL;
-
-    mac_s  = strtok_r(args, " ", &save_ptr);
-    vlan_s = strtok_r(NULL, " ", &save_ptr);
-    basis_s = strtok_r(NULL, " ", &save_ptr);
 
     if (vlan_s) {
         if (sscanf(vlan_s, "%u", &vlan) != 1) {
@@ -1212,17 +1185,18 @@ bond_unixctl_hash(struct unixctl_conn *conn, const char *args_,
 void
 bond_init(void)
 {
-    unixctl_command_register("bond/list", "", bond_unixctl_list, NULL);
-    unixctl_command_register("bond/show", "port", bond_unixctl_show, NULL);
-    unixctl_command_register("bond/migrate", "port hash slave",
+    unixctl_command_register("bond/list", "", 0, 0, bond_unixctl_list, NULL);
+    unixctl_command_register("bond/show", "port", 1, 1,
+                             bond_unixctl_show, NULL);
+    unixctl_command_register("bond/migrate", "port hash slave", 3, 3,
                              bond_unixctl_migrate, NULL);
-    unixctl_command_register("bond/set-active-slave", "port slave",
+    unixctl_command_register("bond/set-active-slave", "port slave", 2, 2,
                              bond_unixctl_set_active_slave, NULL);
-    unixctl_command_register("bond/enable-slave", "port slave",
+    unixctl_command_register("bond/enable-slave", "port slave", 2, 2,
                              bond_unixctl_enable_slave, NULL);
-    unixctl_command_register("bond/disable-slave", "port slave",
+    unixctl_command_register("bond/disable-slave", "port slave", 2, 2,
                              bond_unixctl_disable_slave, NULL);
-    unixctl_command_register("bond/hash", "mac [vlan] [basis]",
+    unixctl_command_register("bond/hash", "mac [vlan] [basis]", 1, 3,
                              bond_unixctl_hash, NULL);
 }
 \f

diff --git a/lib/cfm.c b/lib/cfm.c
index 8acbd0939aa77692b0428d1d5142e133e396a93e..d2995a58cd8840d6b0dcd7e215dbd7d2a2e09c44 100644 (file)
--- a/lib/cfm.c
+++ b/lib/cfm.c
@@ -122,8 +122,7 @@ struct remote_mp {
 static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
 static struct hmap all_cfms = HMAP_INITIALIZER(&all_cfms);
 
-static void cfm_unixctl_show(struct unixctl_conn *, const char *args,
-                             void *aux);
+static unixctl_cb_func cfm_unixctl_show;
 
 static const uint8_t *
 cfm_ccm_addr(const struct cfm *cfm)
@@ -233,7 +232,7 @@ lookup_remote_mp(const struct cfm *cfm, uint64_t mpid)
 void
 cfm_init(void)
 {
-    unixctl_command_register("cfm/show", "[interface]", cfm_unixctl_show,
+    unixctl_command_register("cfm/show", "[interface]", 0, 1, cfm_unixctl_show,
                              NULL);
 }
 
@@ -598,14 +597,14 @@ cfm_print_details(struct ds *ds, const struct cfm *cfm)
 }
 
 static void
-cfm_unixctl_show(struct unixctl_conn *conn,
-                 const char *args, void *aux OVS_UNUSED)
+cfm_unixctl_show(struct unixctl_conn *conn, int argc, const char *argv[],
+                 void *aux OVS_UNUSED)
 {
     struct ds ds = DS_EMPTY_INITIALIZER;
     const struct cfm *cfm;
 
-    if (strlen(args)) {
-        cfm = cfm_find(args);
+    if (argc > 1) {
+        cfm = cfm_find(argv[1]);
         if (!cfm) {
             unixctl_command_reply(conn, 501, "no such CFM object");
             return;

diff --git a/lib/coverage.c b/lib/coverage.c
index 105cd37db3ae3cd675164f224819b7adf0b64ee5..c8d8b70632e6b332c464c33e735d24479245080f 100644 (file)
--- a/lib/coverage.c
+++ b/lib/coverage.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2009, 2010 Nicira Networks.
+ * Copyright (c) 2009, 2010, 2011 Nicira Networks.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -48,8 +48,8 @@ struct coverage_counter *coverage_counters[] = {
 static unsigned int epoch;
 
 static void
-coverage_unixctl_log(struct unixctl_conn *conn, const char *args OVS_UNUSED,
-                     void *aux OVS_UNUSED)
+coverage_unixctl_log(struct unixctl_conn *conn, int argc OVS_UNUSED,
+                     const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
 {
     coverage_log(VLL_WARN, false);
     unixctl_command_reply(conn, 200, NULL);
@@ -58,7 +58,8 @@ coverage_unixctl_log(struct unixctl_conn *conn, const char *args OVS_UNUSED,
 void
 coverage_init(void)
 {
-    unixctl_command_register("coverage/log", "", coverage_unixctl_log, NULL);
+    unixctl_command_register("coverage/log", "", 0, 0,
+                             coverage_unixctl_log, NULL);
 }
 
 /* Sorts coverage counters in descending order by count, within equal counts

diff --git a/lib/lacp.c b/lib/lacp.c
index edf7f6792043a01b1e313ee2c07306052d889955..244b86e687e77081c6bc8e9e91b8a4739f83e9eb 100644 (file)
--- a/lib/lacp.c
+++ b/lib/lacp.c
@@ -136,8 +136,7 @@ static bool slave_may_tx(const struct slave *);
 static struct slave *slave_lookup(const struct lacp *, const void *slave);
 static bool info_tx_equal(struct lacp_info *, struct lacp_info *);
 
-static void lacp_unixctl_show(struct unixctl_conn *, const char *args,
-                              void *aux);
+static unixctl_cb_func lacp_unixctl_show;
 
 /* Populates 'pdu' with a LACP PDU comprised of 'actor' and 'partner'. */
 static void
@@ -188,7 +187,8 @@ parse_lacp_packet(const struct ofpbuf *b)
 void
 lacp_init(void)
 {
-    unixctl_command_register("lacp/show", "[port]", lacp_unixctl_show, NULL);
+    unixctl_command_register("lacp/show", "[port]", 0, 1,
+                             lacp_unixctl_show, NULL);
 }
 
 /* Creates a LACP object. */
@@ -869,14 +869,14 @@ lacp_print_details(struct ds *ds, struct lacp *lacp)
 }
 
 static void
-lacp_unixctl_show(struct unixctl_conn *conn,
-                  const char *args, void *aux OVS_UNUSED)
+lacp_unixctl_show(struct unixctl_conn *conn, int argc, const char *argv[],
+                  void *aux OVS_UNUSED)
 {
     struct ds ds = DS_EMPTY_INITIALIZER;
     struct lacp *lacp;
 
-    if (strlen(args)) {
-        lacp = lacp_find(args);
+    if (argc > 1) {
+        lacp = lacp_find(argv[1]);
         if (!lacp) {
             unixctl_command_reply(conn, 501, "no such lacp object");
             return;

diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
index 27a123cd4ea9c87642d04f7d7dbb3110898e5205..19a80fbe14e1f4587c7aa615bd167a37b2b1faa2 100644 (file)
--- a/lib/netdev-linux.c
+++ b/lib/netdev-linux.c
@@ -803,11 +803,8 @@ netdev_linux_recv(struct netdev *netdev_, void *data, size_t size)
 
     for (;;) {
         ssize_t retval = recv(netdev->fd, data, size, MSG_TRUNC);
-        if (retval > size) {
-            /* Received packet was longer than supplied buffer. */
-            return -EMSGSIZE;
-        } else if (retval >= 0) {
-            return retval;
+        if (retval >= 0) {
+            return retval <= size ? retval : -EMSGSIZE;
         } else if (errno != EINTR) {
             if (errno != EAGAIN) {
                 VLOG_WARN_RL(&rl, "error receiving Ethernet packet on %s: %s",

diff --git a/lib/stress.c b/lib/stress.c
index 412df4fed1d01e033e87e4afa1ad8734c4179130..0fdc79aa0a6e77cf0788852e4b1554b828e4223c 100644 (file)
--- a/lib/stress.c
+++ b/lib/stress.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2010 Nicira Networks.
+ * Copyright (c) 2010, 2011 Nicira Networks.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -112,8 +112,8 @@ stress_set(struct stress_option *option, unsigned int period, bool random)
 }
 \f
 static void
-stress_unixctl_list(struct unixctl_conn *conn, const char *args,
-                    void *aux OVS_UNUSED)
+stress_unixctl_list(struct unixctl_conn *conn, int argc OVS_UNUSED,
+                    const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
 {
     int i, found = 0;
     struct ds results;
@@ -126,7 +126,7 @@ stress_unixctl_list(struct unixctl_conn *conn, const char *args,
                   "RECOMMENDED", "MINIMUM", "MAXIMUM", "DEFAULT");
     for (i = 0; i < n_stress_options; i++) {
         struct stress_option *option = stress_options[i];
-        if (!*args || strstr(option->name, args)) {
+        if (!argv[1] || strstr(option->name, argv[1])) {
             ds_put_format(&results, "\n%s (%s)\n",
                           option->name, option->description);
             if (option->period) {
@@ -162,49 +162,42 @@ stress_unixctl_list(struct unixctl_conn *conn, const char *args,
 }
 
 static void
-stress_unixctl_enable(struct unixctl_conn *conn, const char *args OVS_UNUSED,
-                      void *aux OVS_UNUSED)
+stress_unixctl_enable(struct unixctl_conn *conn, int argc OVS_UNUSED,
+                      const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
 {
     stress_enable(true);
     unixctl_command_reply(conn, 200, "");
 }
 
 static void
-stress_unixctl_disable(struct unixctl_conn *conn, const char *args OVS_UNUSED,
-                       void *aux OVS_UNUSED)
+stress_unixctl_disable(struct unixctl_conn *conn, int argc OVS_UNUSED,
+                       const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
 {
     stress_enable(false);
     unixctl_command_reply(conn, 200, "");
 }
 
 static void
-stress_unixctl_set(struct unixctl_conn *conn, const char *args_,
-                   void *aux OVS_UNUSED)
+stress_unixctl_set(struct unixctl_conn *conn, int argc OVS_UNUSED,
+                   const char *argv[], void *aux OVS_UNUSED)
 {
     int code = 404;
-    char *args = xstrdup(args_);
-    char *save_ptr = NULL;
-    char *option_name;
-    char *option_val;
-
-    option_name = strtok_r(args, " ", &save_ptr);
-    option_val = strtok_r(NULL, " ", &save_ptr);
-    if (option_val) {
-        int i;
-        for (i = 0; i < n_stress_options; i++) {
-            struct stress_option *option = stress_options[i];
-            if (!strcmp(option_name, option->name)) {
-                unsigned int period = strtoul(option_val, NULL, 0);
-                bool random = strstr(args_, "random");
-
-                stress_set(option, period, random);
-                code = 200;
-                break;
-            }
+    const char *option_name = argv[1];
+    const char *option_val = argv[2];
+    int i;
+
+    for (i = 0; i < n_stress_options; i++) {
+        struct stress_option *option = stress_options[i];
+        if (!strcmp(option_name, option->name)) {
+            unsigned int period = strtoul(option_val, NULL, 0);
+            bool random = !strcmp(argv[3], "random");
+
+            stress_set(option, period, random);
+            code = 200;
+            break;
         }
     }
     unixctl_command_reply(conn, code, "");
-    free(args);
 }
 
 /* Exposes ovs-appctl access to the stress options.
@@ -215,10 +208,12 @@ stress_unixctl_set(struct unixctl_conn *conn, const char *args_,
 void
 stress_init_command(void)
 {
-    unixctl_command_register("stress/list", "", stress_unixctl_list, NULL);
+    unixctl_command_register("stress/list", "", 0, 1,
+                             stress_unixctl_list, NULL);
     unixctl_command_register("stress/set", "option period [random | periodic]",
-                             stress_unixctl_set, NULL);
-    unixctl_command_register("stress/enable", "", stress_unixctl_enable, NULL);
-    unixctl_command_register("stress/disable", "",
+                             2, 3, stress_unixctl_set, NULL);
+    unixctl_command_register("stress/enable", "", 0, 0,
+                             stress_unixctl_enable, NULL);
+    unixctl_command_register("stress/disable", "", 0, 0,
                              stress_unixctl_disable, NULL);
 }

diff --git a/lib/unixctl.c b/lib/unixctl.c
index d75166fe424117c0a5d3703c6223a0bc1cfb3343..2a2bcf215479cb1b0133da9fe9f8a1168ded6e98 100644 (file)
--- a/lib/unixctl.c
+++ b/lib/unixctl.c
@@ -48,7 +48,8 @@ COVERAGE_DEFINE(unixctl_received);
 COVERAGE_DEFINE(unixctl_replied);
 \f
 struct unixctl_command {
-    const char *args;
+    const char *usage;
+    int min_args, max_args;
     unixctl_cb_func *cb;
     void *aux;
 };
@@ -82,8 +83,8 @@ static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
 static struct shash commands = SHASH_INITIALIZER(&commands);
 
 static void
-unixctl_help(struct unixctl_conn *conn, const char *args OVS_UNUSED,
-             void *aux OVS_UNUSED)
+unixctl_help(struct unixctl_conn *conn, int argc OVS_UNUSED,
+             const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
 {
     struct ds ds = DS_EMPTY_INITIALIZER;
     const struct shash_node **nodes = shash_sort(&commands);
@@ -95,7 +96,7 @@ unixctl_help(struct unixctl_conn *conn, const char *args OVS_UNUSED,
         const struct shash_node *node = nodes[i];
         const struct unixctl_command *command = node->data;
         
-        ds_put_format(&ds, "  %-23s%s\n", node->name, command->args);
+        ds_put_format(&ds, "  %-23s%s\n", node->name, command->usage);
     }
     free(nodes);
 
@@ -104,15 +105,27 @@ unixctl_help(struct unixctl_conn *conn, const char *args OVS_UNUSED,
 }
 
 static void
-unixctl_version(struct unixctl_conn *conn, const char *args OVS_UNUSED,
-                void *aux OVS_UNUSED)
+unixctl_version(struct unixctl_conn *conn, int argc OVS_UNUSED,
+                const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
 {
     unixctl_command_reply(conn, 200, get_program_version());
 }
 
+/* Registers a unixctl command with the given 'name'.  'usage' describes the
+ * arguments to the command; it is used only for presentation to the user in
+ * "help" output.
+ *
+ * 'cb' is called when the command is received.  It is passed the actual set of
+ * arguments, as a text string, plus a copy of 'aux'.  Normally 'cb' should
+ * call unixctl_command_reply() before it returns, but if the command cannot be
+ * handled immediately then it can defer the reply until later.  A given
+ * connection can only process a single request at a time, so
+ * unixctl_command_reply() must be called eventually to avoid blocking that
+ * connection. */
 void
-unixctl_command_register(const char *name, const char *args,
-        unixctl_cb_func *cb, void *aux)
+unixctl_command_register(const char *name, const char *usage,
+                         int min_args, int max_args,
+                         unixctl_cb_func *cb, void *aux)
 {
     struct unixctl_command *command;
     struct unixctl_command *lookup = shash_find_data(&commands, name);
@@ -124,7 +137,9 @@ unixctl_command_register(const char *name, const char *args,
     }
 
     command = xmalloc(sizeof *command);
-    command->args = args;
+    command->usage = usage;
+    command->min_args = min_args;
+    command->max_args = max_args;
     command->cb = cb;
     command->aux = aux;
     shash_add(&commands, name, command);
@@ -215,8 +230,8 @@ unixctl_server_create(const char *path, struct unixctl_server **serverp)
         return 0;
     }
 
-    unixctl_command_register("help", "", unixctl_help, NULL);
-    unixctl_command_register("version", "", unixctl_version, NULL);
+    unixctl_command_register("help", "", 0, 0, unixctl_help, NULL);
+    unixctl_command_register("version", "", 0, 0, unixctl_version, NULL);
 
     server = xmalloc(sizeof *server);
     list_init(&server->conns);
@@ -301,26 +316,43 @@ static void
 process_command(struct unixctl_conn *conn, char *s)
 {
     struct unixctl_command *command;
-    size_t name_len;
-    char *name, *args;
+    struct svec argv;
 
     COVERAGE_INC(unixctl_received);
     conn->state = S_PROCESS;
 
-    name = s;
-    name_len = strcspn(name, " ");
-    args = name + name_len;
-    args += strspn(args, " ");
-    name[name_len] = '\0';
+    svec_init(&argv);
+    svec_parse_words(&argv, s);
+    svec_terminate(&argv);
 
-    command = shash_find_data(&commands, name);
-    if (command) {
-        command->cb(conn, args, command->aux);
+    if (argv.n == 0) {
+        unixctl_command_reply(conn, 400, "missing command name in request");
     } else {
-        char *msg = xasprintf("\"%s\" is not a valid command", name);
-        unixctl_command_reply(conn, 400, msg);
-        free(msg);
+        const char *name = argv.names[0];
+        char *error;
+
+        command = shash_find_data(&commands, name);
+        if (!command) {
+            error = xasprintf("\"%s\" is not a valid command", name);
+        } else if (argv.n - 1 < command->min_args) {
+            error = xasprintf("\"%s\" command requires at least %d arguments",
+                              name, command->min_args);
+        } else if (argv.n - 1 > command->max_args) {
+            error = xasprintf("\"%s\" command takes at most %d arguments",
+                              name, command->max_args);
+        } else {
+            error = NULL;
+            command->cb(conn, argv.n, (const char **) argv.names,
+                        command->aux);
+        }
+
+        if (error) {
+            unixctl_command_reply(conn, 400, error);
+            free(error);
+        }
     }
+
+    svec_destroy(&argv);
 }
 
 static int

diff --git a/lib/unixctl.h b/lib/unixctl.h
index d93e5e440b6ee0647d3d1c489a3f1a4097d0199b..8eb190b0b01377ab1753a58c7eec35adff5b1eae 100644 (file)
--- a/lib/unixctl.h
+++ b/lib/unixctl.h
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2008, 2009 Nicira Networks.
+ * Copyright (c) 2008, 2009, 2011 Nicira Networks.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -40,8 +40,9 @@ const char *unixctl_client_target(const struct unixctl_client *);
 /* Command registration. */
 struct unixctl_conn;
 typedef void unixctl_cb_func(struct unixctl_conn *,
-                             const char *args, void *aux);
-void unixctl_command_register(const char *name, const char *args,
+                             int argc, const char *argv[], void *aux);
+void unixctl_command_register(const char *name, const char *usage,
+                              int min_args, int max_args,
                               unixctl_cb_func *cb, void *aux);
 void unixctl_command_reply(struct unixctl_conn *, int code,
                            const char *body);

diff --git a/lib/vlog.c b/lib/vlog.c
index 11b2f7cc4ef48be324b4f90412ff1fef40995432..0d7f4d159ab63c540f429b10756bffa0a2bfae8e 100644 (file)
--- a/lib/vlog.c
+++ b/lib/vlog.c
@@ -425,17 +425,25 @@ vlog_set_verbosity(const char *arg)
 }
 
 static void
-vlog_unixctl_set(struct unixctl_conn *conn,
-                 const char *args, void *aux OVS_UNUSED)
+vlog_unixctl_set(struct unixctl_conn *conn, int argc, const char *argv[],
+                 void *aux OVS_UNUSED)
 {
-    char *msg = vlog_set_levels_from_string(args);
-    unixctl_command_reply(conn, msg ? 501 : 202, msg);
-    free(msg);
+    int i;
+
+    for (i = 1; i < argc; i++) {
+        char *msg = vlog_set_levels_from_string(argv[i]);
+        if (msg) {
+            unixctl_command_reply(conn, 501, msg);
+            free(msg);
+            return;
+        }
+    }
+    unixctl_command_reply(conn, 202, NULL);
 }
 
 static void
-vlog_unixctl_list(struct unixctl_conn *conn,
-                  const char *args OVS_UNUSED, void *aux OVS_UNUSED)
+vlog_unixctl_list(struct unixctl_conn *conn, int argc OVS_UNUSED,
+                  const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
 {
     char *msg = vlog_get_levels();
     unixctl_command_reply(conn, 200, msg);
@@ -443,8 +451,8 @@ vlog_unixctl_list(struct unixctl_conn *conn,
 }
 
 static void
-vlog_unixctl_reopen(struct unixctl_conn *conn,
-                    const char *args OVS_UNUSED, void *aux OVS_UNUSED)
+vlog_unixctl_reopen(struct unixctl_conn *conn, int argc OVS_UNUSED,
+                    const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
 {
     if (log_file_name) {
         int error = vlog_reopen_log_file();
@@ -483,11 +491,12 @@ vlog_init(void)
         VLOG_ERR("current time is negative: %s (%ld)", s, (long int) now);
     }
 
-    unixctl_command_register("vlog/set",
-                   "{module[:facility[:level]] | PATTERN:facility:pattern}",
-                   vlog_unixctl_set, NULL);
-    unixctl_command_register("vlog/list", "", vlog_unixctl_list, NULL);
-    unixctl_command_register("vlog/reopen", "", vlog_unixctl_reopen, NULL);
+    unixctl_command_register(
+        "vlog/set", "{module[:facility[:level]] | PATTERN:facility:pattern}",
+        1, INT_MAX, vlog_unixctl_set, NULL);
+    unixctl_command_register("vlog/list", "", 0, 0, vlog_unixctl_list, NULL);
+    unixctl_command_register("vlog/reopen", "", 0, 0,
+                             vlog_unixctl_reopen, NULL);
 }
 
 /* Closes the logging subsystem. */

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 84ddc9dc06db3f568552153d81cb413f53bb04e4..fd0cc67332fe116a3a5e8204039c91fb121ced63 100644 (file)
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -5569,12 +5569,12 @@ ofproto_dpif_lookup(const char *name)
 }
 
 static void
-ofproto_unixctl_fdb_flush(struct unixctl_conn *conn,
-                         const char *args, void *aux OVS_UNUSED)
+ofproto_unixctl_fdb_flush(struct unixctl_conn *conn, int argc OVS_UNUSED,
+                          const char *argv[], void *aux OVS_UNUSED)
 {
     const struct ofproto_dpif *ofproto;
 
-    ofproto = ofproto_dpif_lookup(args);
+    ofproto = ofproto_dpif_lookup(argv[1]);
     if (!ofproto) {
         unixctl_command_reply(conn, 501, "no such bridge");
         return;
@@ -5585,14 +5585,14 @@ ofproto_unixctl_fdb_flush(struct unixctl_conn *conn,
 }
 
 static void
-ofproto_unixctl_fdb_show(struct unixctl_conn *conn,
-                         const char *args, void *aux OVS_UNUSED)
+ofproto_unixctl_fdb_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
+                         const char *argv[], void *aux OVS_UNUSED)
 {
     struct ds ds = DS_EMPTY_INITIALIZER;
     const struct ofproto_dpif *ofproto;
     const struct mac_entry *e;
 
-    ofproto = ofproto_dpif_lookup(args);
+    ofproto = ofproto_dpif_lookup(argv[1]);
     if (!ofproto) {
         unixctl_command_reply(conn, 501, "no such bridge");
         return;
@@ -5678,12 +5678,10 @@ trace_resubmit(struct action_xlate_ctx *ctx, struct rule_dpif *rule)
 }
 
 static void
-ofproto_unixctl_trace(struct unixctl_conn *conn, const char *args_,
+ofproto_unixctl_trace(struct unixctl_conn *conn, int argc, const char *argv[],
                       void *aux OVS_UNUSED)
 {
-    char *dpname, *arg1, *arg2, *arg3, *arg4;
-    char *args = xstrdup(args_);
-    char *save_ptr = NULL;
+    const char *dpname = argv[1];
     struct ofproto_dpif *ofproto;
     struct ofpbuf odp_key;
     struct ofpbuf *packet;
@@ -5697,29 +5695,21 @@ ofproto_unixctl_trace(struct unixctl_conn *conn, const char *args_,
     ofpbuf_init(&odp_key, 0);
     ds_init(&result);
 
-    dpname = strtok_r(args, " ", &save_ptr);
-    if (!dpname) {
-        unixctl_command_reply(conn, 501, "Bad command syntax");
-        goto exit;
-    }
-
     ofproto = ofproto_dpif_lookup(dpname);
     if (!ofproto) {
         unixctl_command_reply(conn, 501, "Unknown ofproto (use ofproto/list "
                               "for help)");
         goto exit;
     }
-    arg1 = strtok_r(NULL, " ", &save_ptr);
-    arg2 = strtok_r(NULL, " ", &save_ptr);
-    arg3 = strtok_r(NULL, " ", &save_ptr);
-    arg4 = strtok_r(NULL, "", &save_ptr); /* Get entire rest of line. */
-    if (dpname && arg1 && (!arg2 || !strcmp(arg2, "-generate")) && !arg3) {
+    if (argc == 3 || (argc == 4 && !strcmp(argv[3], "-generate"))) {
         /* ofproto/trace dpname flow [-generate] */
+        const char *flow_s = argv[2];
+        const char *generate_s = argv[3];
         int error;
 
         /* Convert string to datapath key. */
         ofpbuf_init(&odp_key, 0);
-        error = odp_flow_key_from_string(arg1, NULL, &odp_key);
+        error = odp_flow_key_from_string(flow_s, NULL, &odp_key);
         if (error) {
             unixctl_command_reply(conn, 501, "Bad flow syntax");
             goto exit;
@@ -5735,24 +5725,22 @@ ofproto_unixctl_trace(struct unixctl_conn *conn, const char *args_,
         }
 
         /* Generate a packet, if requested. */
-        if (arg2) {
+        if (generate_s) {
             packet = ofpbuf_new(0);
             flow_compose(packet, &flow);
         }
-    } else if (dpname && arg1 && arg2 && arg3 && arg4) {
+    } else if (argc == 6) {
         /* ofproto/trace dpname priority tun_id in_port packet */
-        uint16_t in_port;
-        ovs_be64 tun_id;
-        uint32_t priority;
-
-        priority = atoi(arg1);
-        tun_id = htonll(strtoull(arg2, NULL, 0));
-        in_port = ofp_port_to_odp_port(atoi(arg3));
-
-        packet = ofpbuf_new(strlen(args) / 2);
-        arg4 = ofpbuf_put_hex(packet, arg4, NULL);
-        arg4 += strspn(arg4, " ");
-        if (*arg4 != '\0') {
+        const char *priority_s = argv[2];
+        const char *tun_id_s = argv[3];
+        const char *in_port_s = argv[4];
+        const char *packet_s = argv[5];
+        uint16_t in_port = ofp_port_to_odp_port(atoi(in_port_s));
+        ovs_be64 tun_id = htonll(strtoull(tun_id_s, NULL, 0));
+        uint32_t priority = atoi(priority_s);
+
+        packet = ofpbuf_new(strlen(packet_s) / 2);
+        if (ofpbuf_put_hex(packet, packet_s, NULL)[0] != '\0') {
             unixctl_command_reply(conn, 501, "Trailing garbage in command");
             goto exit;
         }
@@ -5813,20 +5801,19 @@ exit:
     ds_destroy(&result);
     ofpbuf_delete(packet);
     ofpbuf_uninit(&odp_key);
-    free(args);
 }
 
 static void
-ofproto_dpif_clog(struct unixctl_conn *conn OVS_UNUSED,
-                  const char *args_ OVS_UNUSED, void *aux OVS_UNUSED)
+ofproto_dpif_clog(struct unixctl_conn *conn OVS_UNUSED, int argc OVS_UNUSED,
+                  const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
 {
     clogged = true;
     unixctl_command_reply(conn, 200, NULL);
 }
 
 static void
-ofproto_dpif_unclog(struct unixctl_conn *conn OVS_UNUSED,
-                    const char *args_ OVS_UNUSED, void *aux OVS_UNUSED)
+ofproto_dpif_unclog(struct unixctl_conn *conn OVS_UNUSED, int argc OVS_UNUSED,
+                    const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
 {
     clogged = false;
     unixctl_command_reply(conn, 200, NULL);
@@ -5841,15 +5828,18 @@ ofproto_dpif_unixctl_init(void)
     }
     registered = true;
 
-    unixctl_command_register("ofproto/trace",
-                      "bridge {tun_id in_port packet | odp_flow [-generate]}",
-                      ofproto_unixctl_trace, NULL);
-    unixctl_command_register("fdb/flush", "bridge", ofproto_unixctl_fdb_flush,
-                             NULL);
-    unixctl_command_register("fdb/show", "bridge", ofproto_unixctl_fdb_show,
-                             NULL);
-    unixctl_command_register("ofproto/clog", "", ofproto_dpif_clog, NULL);
-    unixctl_command_register("ofproto/unclog", "", ofproto_dpif_unclog, NULL);
+    unixctl_command_register(
+        "ofproto/trace",
+        "bridge {tun_id in_port packet | odp_flow [-generate]}",
+        2, 4, ofproto_unixctl_trace, NULL);
+    unixctl_command_register("fdb/flush", "bridge", 1, 1,
+                             ofproto_unixctl_fdb_flush, NULL);
+    unixctl_command_register("fdb/show", "bridge", 1, 1,
+                             ofproto_unixctl_fdb_show, NULL);
+    unixctl_command_register("ofproto/clog", "", 0, 0,
+                             ofproto_dpif_clog, NULL);
+    unixctl_command_register("ofproto/unclog", "", 0, 0,
+                             ofproto_dpif_unclog, NULL);
 }
 \f
 /* Linux VLAN device support (e.g. "eth0.10" for VLAN 10.)

diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index b0a1a66f591bd4fad8e2254e614dd9c08d45c14f..521533b2c07033f8d914b16026af4d3cecec8841 100644 (file)
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -3277,8 +3277,8 @@ ofproto_lookup(const char *name)
 }
 
 static void
-ofproto_unixctl_list(struct unixctl_conn *conn, const char *arg OVS_UNUSED,
-                     void *aux OVS_UNUSED)
+ofproto_unixctl_list(struct unixctl_conn *conn, int argc OVS_UNUSED,
+                     const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
 {
     struct ofproto *ofproto;
     struct ds results;
@@ -3300,7 +3300,8 @@ ofproto_unixctl_init(void)
     }
     registered = true;
 
-    unixctl_command_register("ofproto/list", "", ofproto_unixctl_list, NULL);
+    unixctl_command_register("ofproto/list", "", 0, 0,
+                             ofproto_unixctl_list, NULL);
 }
 \f
 /* Linux VLAN device support (e.g. "eth0.10" for VLAN 10.)

diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c
index 530568aec37d343cd24bcb55cbac8ab3810fe184..41238fbb80d47e48006b9062a7850f7ddfc43e2c 100644 (file)
--- a/ovsdb/ovsdb-server.c
+++ b/ovsdb/ovsdb-server.c
@@ -135,10 +135,10 @@ main(int argc, char *argv[])
 
     daemonize_complete();
 
-    unixctl_command_register("exit", "", ovsdb_server_exit, &exiting);
-    unixctl_command_register("ovsdb-server/compact", "",
+    unixctl_command_register("exit", "", 0, 0, ovsdb_server_exit, &exiting);
+    unixctl_command_register("ovsdb-server/compact", "", 0, 0,
                              ovsdb_server_compact, file);
-    unixctl_command_register("ovsdb-server/reconnect", "",
+    unixctl_command_register("ovsdb-server/reconnect", "", 0, 0,
                              ovsdb_server_reconnect, jsonrpc);
 
     exiting = false;
@@ -612,7 +612,8 @@ reconfigure_from_db(struct ovsdb_jsonrpc_server *jsonrpc,
 }
 
 static void
-ovsdb_server_exit(struct unixctl_conn *conn, const char *args OVS_UNUSED,
+ovsdb_server_exit(struct unixctl_conn *conn, int argc OVS_UNUSED,
+                  const char *argv[] OVS_UNUSED,
                   void *exiting_)
 {
     bool *exiting = exiting_;
@@ -621,8 +622,8 @@ ovsdb_server_exit(struct unixctl_conn *conn, const char *args OVS_UNUSED,
 }
 
 static void
-ovsdb_server_compact(struct unixctl_conn *conn, const char *args OVS_UNUSED,
-                     void *file_)
+ovsdb_server_compact(struct unixctl_conn *conn, int argc OVS_UNUSED,
+                     const char *argv[] OVS_UNUSED, void *file_)
 {
     struct ovsdb_file *file = file_;
     struct ovsdb_error *error;
@@ -642,8 +643,8 @@ ovsdb_server_compact(struct unixctl_conn *conn, const char *args OVS_UNUSED,
 /* "ovsdb-server/reconnect": makes ovsdb-server drop all of its JSON-RPC
  * connections and reconnect. */
 static void
-ovsdb_server_reconnect(struct unixctl_conn *conn, const char *args OVS_UNUSED,
-                       void *jsonrpc_)
+ovsdb_server_reconnect(struct unixctl_conn *conn, int argc OVS_UNUSED,
+                       const char *argv[] OVS_UNUSED, void *jsonrpc_)
 {
     struct ovsdb_jsonrpc_server *jsonrpc = jsonrpc_;
 

diff --git a/utilities/ovs-appctl.8.in b/utilities/ovs-appctl.8.in
index 3540693d2db7eabbb3b172eedef00480a2961f7d..fc5c885c5e4de04951747c8b88c917cb31059e4b 100644 (file)
--- a/utilities/ovs-appctl.8.in
+++ b/utilities/ovs-appctl.8.in
@@ -198,12 +198,6 @@ This has no effect if the target application was not invoked with the
 .
 .so lib/common.man
 .
-.SH BUGS
-.
-The protocol used to speak to Open vSwitch daemons does not contain a
-quoting mechanism, so command arguments should not generally contain
-white space.
-.
 .SH "SEE ALSO"
 .
 \fBovs\-appctl\fR can control the following daemons:

diff --git a/utilities/ovs-appctl.c b/utilities/ovs-appctl.c
index d3c701b480206d283546724c31b3383b5b1b39e2..e528af344b9a6623456158f889d5b282fed6f813 100644 (file)
--- a/utilities/ovs-appctl.c
+++ b/utilities/ovs-appctl.c
@@ -26,6 +26,7 @@
 #include "daemon.h"
 #include "dirs.h"
 #include "dynamic-string.h"
+#include "process.h"
 #include "timeval.h"
 #include "unixctl.h"
 #include "util.h"
@@ -39,10 +40,9 @@ main(int argc, char *argv[])
 {
     struct unixctl_client *client;
     const char *target;
-    struct ds request;
     int code, error;
+    char *request;
     char *reply;
-    int i;
 
     set_program_name(argv[0]);
 
@@ -50,17 +50,10 @@ main(int argc, char *argv[])
     target = parse_command_line(argc, argv);
     client = connect_to_target(target);
 
-    /* Compose request. */
-    ds_init(&request);
-    for (i = optind; i < argc; i++) {
-        if (i != optind) {
-            ds_put_char(&request, ' ');
-        }
-        ds_put_cstr(&request, argv[i]);
-    }
-
     /* Transact request and process reply. */
-    error = unixctl_client_transact(client, ds_cstr(&request), &code, &reply);
+    request = process_escape_args(argv + optind);
+    error = unixctl_client_transact(client, request, &code, &reply);
+    free(request);
     if (error) {
         ovs_fatal(error, "%s: transaction error", target);
     }
@@ -73,7 +66,6 @@ main(int argc, char *argv[])
 
     unixctl_client_destroy(client);
     free(reply);
-    ds_destroy(&request);
 
     return 0;
 }

diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index e89855e8bc1dfc52e8a19650b5955b2a0e14f718..396c7206f319877c644604386168c9f88b86681c 100644 (file)
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -309,10 +309,11 @@ bridge_init(const char *remote)
     ovsdb_idl_omit(idl, &ovsrec_ssl_col_external_ids);
 
     /* Register unixctl commands. */
-    unixctl_command_register("qos/show", "interface", qos_unixctl_show, NULL);
-    unixctl_command_register("bridge/dump-flows", "bridge",
+    unixctl_command_register("qos/show", "interface", 1, 1,
+                             qos_unixctl_show, NULL);
+    unixctl_command_register("bridge/dump-flows", "bridge", 1, 1,
                              bridge_unixctl_dump_flows, NULL);
-    unixctl_command_register("bridge/reconnect", "[bridge]",
+    unixctl_command_register("bridge/reconnect", "[bridge]", 0, 1,
                              bridge_unixctl_reconnect, NULL);
     lacp_init();
     bond_init();
@@ -2034,8 +2035,8 @@ qos_unixctl_show_cb(unsigned int queue_id,
 }
 
 static void
-qos_unixctl_show(struct unixctl_conn *conn,
-                 const char *args, void *aux OVS_UNUSED)
+qos_unixctl_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
+                 const char *argv[], void *aux OVS_UNUSED)
 {
     struct ds ds = DS_EMPTY_INITIALIZER;
     struct shash sh = SHASH_INITIALIZER(&sh);
@@ -2045,7 +2046,7 @@ qos_unixctl_show(struct unixctl_conn *conn,
     struct qos_unixctl_show_cbdata data;
     int error;
 
-    iface = iface_find(args);
+    iface = iface_find(argv[1]);
     if (!iface) {
         unixctl_command_reply(conn, 501, "no such interface");
         return;
@@ -2144,13 +2145,13 @@ bridge_lookup(const char *name)
 /* Handle requests for a listing of all flows known by the OpenFlow
  * stack, including those normally hidden. */
 static void
-bridge_unixctl_dump_flows(struct unixctl_conn *conn,
-                          const char *args, void *aux OVS_UNUSED)
+bridge_unixctl_dump_flows(struct unixctl_conn *conn, int argc OVS_UNUSED,
+                          const char *argv[], void *aux OVS_UNUSED)
 {
     struct bridge *br;
     struct ds results;
 
-    br = bridge_lookup(args);
+    br = bridge_lookup(argv[1]);
     if (!br) {
         unixctl_command_reply(conn, 501, "Unknown bridge");
         return;
@@ -2167,12 +2168,12 @@ bridge_unixctl_dump_flows(struct unixctl_conn *conn,
  * connections and reconnect.  If BRIDGE is not specified, then all bridges
  * drop their controller connections and reconnect. */
 static void
-bridge_unixctl_reconnect(struct unixctl_conn *conn,
-                         const char *args, void *aux OVS_UNUSED)
+bridge_unixctl_reconnect(struct unixctl_conn *conn, int argc,
+                         const char *argv[], void *aux OVS_UNUSED)
 {
     struct bridge *br;
-    if (args[0] != '\0') {
-        br = bridge_lookup(args);
+    if (argc > 1) {
+        br = bridge_lookup(argv[1]);
         if (!br) {
             unixctl_command_reply(conn, 501, "Unknown bridge");
             return;

diff --git a/vswitchd/ovs-vswitchd.c b/vswitchd/ovs-vswitchd.c
index 301d073905fd1f613ec9c395969dea2f4965580b..2a86972657af4636dd53757a5ca0a2c1ebd6344d 100644 (file)
--- a/vswitchd/ovs-vswitchd.c
+++ b/vswitchd/ovs-vswitchd.c
@@ -82,7 +82,7 @@ main(int argc, char *argv[])
     if (retval) {
         exit(EXIT_FAILURE);
     }
-    unixctl_command_register("exit", "", ovs_vswitchd_exit, &exiting);
+    unixctl_command_register("exit", "", 0, 0, ovs_vswitchd_exit, &exiting);
 
     bridge_init(remote);
     free(remote);
@@ -234,8 +234,8 @@ usage(void)
 }
 
 static void
-ovs_vswitchd_exit(struct unixctl_conn *conn, const char *args OVS_UNUSED,
-                  void *exiting_)
+ovs_vswitchd_exit(struct unixctl_conn *conn, int argc OVS_UNUSED,
+                  const char *argv[] OVS_UNUSED, void *exiting_)
 {
     bool *exiting = exiting_;
     *exiting = true;