From 0e15264f96e3caff662c7998cc739a3dd5c1c0f2 Mon Sep 17 00:00:00 2001
From: Ben Pfaff <blp@nicira.com>
Date: Fri, 2 Dec 2011 15:29:19 -0800
Subject: [PATCH] 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>
---
 lib/bond.c                | 96 ++++++++++++++-------------------------
 lib/cfm.c                 | 13 +++---
 lib/coverage.c            |  9 ++--
 lib/lacp.c                | 14 +++---
 lib/netdev-linux.c        |  7 +--
 lib/stress.c              | 63 ++++++++++++-------------
 lib/unixctl.c             | 80 ++++++++++++++++++++++----------
 lib/unixctl.h             |  7 +--
 lib/vlog.c                | 37 +++++++++------
 ofproto/ofproto-dpif.c    | 90 ++++++++++++++++--------------------
 ofproto/ofproto.c         |  7 +--
 ovsdb/ovsdb-server.c      | 17 +++----
 utilities/ovs-appctl.8.in |  6 ---
 utilities/ovs-appctl.c    | 18 ++------
 vswitchd/bridge.c         | 27 +++++------
 vswitchd/ovs-vswitchd.c   |  6 +--
 16 files changed, 242 insertions(+), 255 deletions(-)

diff --git a/lib/bond.c b/lib/bond.c
index bb5d4996..84a1117a 100644
--- 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);
 }
 
diff --git a/lib/cfm.c b/lib/cfm.c
index 8acbd093..d2995a58 100644
--- 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 105cd37d..c8d8b706 100644
--- 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 edf7f679..244b86e6 100644
--- 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 27a123cd..19a80fbe 100644
--- 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 412df4fe..0fdc79aa 100644
--- 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)
 }
 
 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 d75166fe..2a2bcf21 100644
--- a/lib/unixctl.c
+++ b/lib/unixctl.c
@@ -48,7 +48,8 @@ COVERAGE_DEFINE(unixctl_received);
 COVERAGE_DEFINE(unixctl_replied);
 
 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 d93e5e44..8eb190b0 100644
--- 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 11b2f7cc..0d7f4d15 100644
--- 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 84ddc9dc..fd0cc673 100644
--- 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);
 }
 
 /* Linux VLAN device support (e.g. "eth0.10" for VLAN 10.)
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index b0a1a66f..521533b2 100644
--- 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);
 }
 
 /* Linux VLAN device support (e.g. "eth0.10" for VLAN 10.)
diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c
index 530568ae..41238fbb 100644
--- 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 3540693d..fc5c885c 100644
--- 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 d3c701b4..e528af34 100644
--- 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 e89855e8..396c7206 100644
--- 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 301d0739..2a869726 100644
--- 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;
-- 
2.30.2