netdev-dpdk: fix mbuf leaks
[cascardo/ovs.git] / lib / dpctl.c
index 8eec944..d58df0d 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2014 Nicira, Inc.
+ * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2014, 2015 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -28,6 +28,7 @@
 
 #include "command-line.h"
 #include "compiler.h"
+#include "ct-dpif.h"
 #include "dirs.h"
 #include "dpctl.h"
 #include "dpif.h"
 #include "flow.h"
 #include "match.h"
 #include "netdev.h"
+#include "netdev-dpdk.h"
 #include "netlink.h"
 #include "odp-util.h"
 #include "ofp-parse.h"
 #include "ofpbuf.h"
+#include "ovs-numa.h"
 #include "packets.h"
 #include "shash.h"
 #include "simap.h"
@@ -58,6 +61,11 @@ struct dpctl_command {
     dpctl_command_handler *handler;
 };
 static const struct dpctl_command *get_all_dpctl_commands(void);
+static void dpctl_print(struct dpctl_params *dpctl_p, const char *fmt, ...)
+    OVS_PRINTF_FORMAT(2, 3);
+static void dpctl_error(struct dpctl_params* dpctl_p, int err_no,
+                        const char *fmt, ...)
+    OVS_PRINTF_FORMAT(3, 4);
 
 static void
 dpctl_puts(struct dpctl_params *dpctl_p, bool error, const char *string)
@@ -380,12 +388,14 @@ dpctl_set_if(int argc, const char *argv[], struct dpctl_params *dpctl_p)
                                 "%s: can't change type from %s to %s",
                                  name, type, value);
                     error = EINVAL;
+                    goto next_destroy_args;
                 }
             } else if (!strcmp(key, "port_no")) {
                 if (port_no != u32_to_odp(atoi(value))) {
                     dpctl_error(dpctl_p, 0, "%s: can't change port number from"
                               " %"PRIu32" to %d", name, port_no, atoi(value));
                     error = EINVAL;
+                    goto next_destroy_args;
                 }
             } else if (value[0] == '\0') {
                 smap_remove(&args, key);
@@ -395,7 +405,13 @@ dpctl_set_if(int argc, const char *argv[], struct dpctl_params *dpctl_p)
         }
 
         /* Update configuration. */
-        error = netdev_set_config(netdev, &args, NULL);
+        char *err_s = NULL;
+        error = netdev_set_config(netdev, &args, &err_s);
+        if (err_s || error) {
+            dpctl_error(dpctl_p, error, "%s",
+                        err_s ? err_s : "Error updating configuration");
+            free(err_s);
+        }
         if (error) {
             goto next_destroy_args;
         }
@@ -597,7 +613,57 @@ show_dpif(struct dpif *dpif, struct dpctl_params *dpctl_p)
             }
         }
     }
-    dpif_close(dpif);
+}
+
+typedef void (*dps_for_each_cb)(struct dpif *, struct dpctl_params *);
+
+static int
+dps_for_each(struct dpctl_params *dpctl_p, dps_for_each_cb cb)
+{
+    struct sset dpif_names = SSET_INITIALIZER(&dpif_names),
+                dpif_types = SSET_INITIALIZER(&dpif_types);
+    int error, openerror = 0, enumerror = 0;
+    const char *type, *name;
+    bool at_least_one = false;
+
+    dp_enumerate_types(&dpif_types);
+
+    SSET_FOR_EACH (type, &dpif_types) {
+        error = dp_enumerate_names(type, &dpif_names);
+        if (error) {
+            enumerror = error;
+        }
+
+        SSET_FOR_EACH (name, &dpif_names) {
+            struct dpif *dpif;
+
+            at_least_one = true;
+            error = dpif_open(name, type, &dpif);
+            if (!error) {
+                cb(dpif, dpctl_p);
+                dpif_close(dpif);
+            } else {
+                openerror = error;
+                dpctl_error(dpctl_p, error, "opening datapath %s failed",
+                            name);
+            }
+        }
+    }
+
+    sset_destroy(&dpif_names);
+    sset_destroy(&dpif_types);
+
+    /* If there has been an error while opening a datapath it should be
+     * reported.  Otherwise, we want to ignore the errors generated by
+     * dp_enumerate_names() if at least one datapath has been discovered,
+     * because they're not interesting for the user.  This happens, for
+     * example, if OVS is using a userspace datapath and the kernel module
+     * is not loaded. */
+    if (openerror) {
+        return openerror;
+    } else {
+        return at_least_one ? 0 : enumerror;
+    }
 }
 
 static int
@@ -613,6 +679,7 @@ dpctl_show(int argc, const char *argv[], struct dpctl_params *dpctl_p)
             error = parsed_dpif_open(name, false, &dpif);
             if (!error) {
                 show_dpif(dpif, dpctl_p);
+                dpif_close(dpif);
             } else {
                 dpctl_error(dpctl_p, error, "opening datapath %s failed",
                             name);
@@ -620,86 +687,32 @@ dpctl_show(int argc, const char *argv[], struct dpctl_params *dpctl_p)
             }
         }
     } else {
-        struct sset types;
-        const char *type;
-
-        sset_init(&types);
-        dp_enumerate_types(&types);
-        SSET_FOR_EACH (type, &types) {
-            struct sset names;
-            const char *name;
-
-            sset_init(&names);
-            error = dp_enumerate_names(type, &names);
-            if (error) {
-                lasterror = error;
-                goto next;
-            }
-            SSET_FOR_EACH (name, &names) {
-                struct dpif *dpif;
-
-                error = dpif_open(name, type, &dpif);
-                if (!error) {
-                    show_dpif(dpif, dpctl_p);
-                } else {
-                    dpctl_error(dpctl_p, error, "opening datapath %s failed",
-                                name);
-                    lasterror = error;
-                }
-            }
-next:
-            sset_destroy(&names);
-        }
-        sset_destroy(&types);
+        lasterror = dps_for_each(dpctl_p, show_dpif);
     }
+
     return lasterror;
 }
 
+static void
+dump_cb(struct dpif *dpif, struct dpctl_params *dpctl_p)
+{
+    dpctl_print(dpctl_p, "%s\n", dpif_name(dpif));
+}
+
 static int
 dpctl_dump_dps(int argc OVS_UNUSED, const char *argv[] OVS_UNUSED,
                struct dpctl_params *dpctl_p)
 {
-    struct sset dpif_names, dpif_types;
-    const char *type;
-    int error, lasterror = 0;
-
-    sset_init(&dpif_names);
-    sset_init(&dpif_types);
-    dp_enumerate_types(&dpif_types);
-
-    SSET_FOR_EACH (type, &dpif_types) {
-        const char *name;
-
-        error = dp_enumerate_names(type, &dpif_names);
-        if (error) {
-            lasterror = error;
-        }
-
-        SSET_FOR_EACH (name, &dpif_names) {
-            struct dpif *dpif;
-            if (!dpif_open(name, type, &dpif)) {
-                dpctl_print(dpctl_p, "%s\n", dpif_name(dpif));
-                dpif_close(dpif);
-            }
-        }
-    }
-
-    sset_destroy(&dpif_names);
-    sset_destroy(&dpif_types);
-    return lasterror;
+    return dps_for_each(dpctl_p, dump_cb);
 }
 
 static void
 format_dpif_flow(struct ds *ds, const struct dpif_flow *f, struct hmap *ports,
                  struct dpctl_params *dpctl_p)
 {
-    if (dpctl_p->verbosity) {
-        if (f->ufid_present) {
-            odp_format_ufid(&f->ufid, ds);
-            ds_put_cstr(ds, ", ");
-        } else {
-            ds_put_cstr(ds, "ufid:<empty>, ");
-        }
+    if (dpctl_p->verbosity && f->ufid_present) {
+        odp_format_ufid(&f->ufid, ds);
+        ds_put_cstr(ds, ", ");
     }
     odp_flow_format(f->key, f->key_len, f->mask, f->mask_len, ports, ds,
                     dpctl_p->verbosity);
@@ -729,7 +742,7 @@ dpctl_dump_flows(int argc, const char *argv[], struct dpctl_params *dpctl_p)
     struct dpif_flow_dump_thread *flow_dump_thread;
     struct dpif_flow_dump *flow_dump;
     struct dpif_flow f;
-
+    int pmd_id = PMD_ID_NULL;
     int error;
 
     if (argc > 1 && !strncmp(argv[argc - 1], "filter=", 7)) {
@@ -767,6 +780,12 @@ dpctl_dump_flows(int argc, const char *argv[], struct dpctl_params *dpctl_p)
         }
     }
 
+    /* Make sure that these values are different. PMD_ID_NULL means that the
+     * pmd is unspecified (e.g. because the datapath doesn't have different
+     * pmd threads), while NON_PMD_CORE_ID refers to every non pmd threads
+     * in the userspace datapath */
+    BUILD_ASSERT(PMD_ID_NULL != NON_PMD_CORE_ID);
+
     ds_init(&ds);
     flow_dump = dpif_flow_dump_create(dpif, false);
     flow_dump_thread = dpif_flow_dump_thread_create(flow_dump);
@@ -778,7 +797,8 @@ dpctl_dump_flows(int argc, const char *argv[], struct dpctl_params *dpctl_p)
             struct minimatch minimatch;
 
             odp_flow_key_to_flow(f.key, f.key_len, &flow);
-            odp_flow_key_to_mask(f.mask, f.mask_len, &wc.masks, &flow);
+            odp_flow_key_to_mask(f.mask, f.mask_len, f.key, f.key_len,
+                                 &wc, &flow);
             match_init(&match, &flow, &wc);
 
             match_init(&match_filter, &flow_filter, &wc);
@@ -792,6 +812,18 @@ dpctl_dump_flows(int argc, const char *argv[], struct dpctl_params *dpctl_p)
             minimatch_destroy(&minimatch);
         }
         ds_clear(&ds);
+        /* If 'pmd_id' is specified, overlapping flows could be dumped from
+         * different pmd threads.  So, separates dumps from different pmds
+         * by printing a title line. */
+        if (pmd_id != f.pmd_id) {
+            if (f.pmd_id == NON_PMD_CORE_ID) {
+                ds_put_format(&ds, "flow-dump from non-dpdk interfaces:\n");
+            } else {
+                ds_put_format(&ds, "flow-dump from pmd on cpu core: %d\n",
+                              f.pmd_id);
+            }
+            pmd_id = f.pmd_id;
+        }
         format_dpif_flow(&ds, &f, &portno_names, dpctl_p);
         dpctl_print(dpctl_p, "%s\n", ds_cstr(&ds));
     }
@@ -813,12 +845,43 @@ out_freefilter:
     return error;
 }
 
+/* Extracts the in_port from the parsed keys, and returns the reference
+ * to the 'struct netdev *' of the dpif port.  On error, returns NULL.
+ * Users must call 'netdev_close()' after finish using the returned
+ * reference. */
+static struct netdev *
+get_in_port_netdev_from_key(struct dpif *dpif, const struct ofpbuf *key)
+{
+    const struct nlattr *in_port_nla;
+    struct netdev *dev = NULL;
+
+    in_port_nla = nl_attr_find(key, 0, OVS_KEY_ATTR_IN_PORT);
+    if (in_port_nla) {
+        struct dpif_port dpif_port;
+        odp_port_t port_no;
+        int error;
+
+        port_no = ODP_PORT_C(nl_attr_get_u32(in_port_nla));
+        error = dpif_port_query_by_number(dpif, port_no, &dpif_port);
+        if (error) {
+            goto out;
+        }
+
+        netdev_open(dpif_port.name, dpif_port.type, &dev);
+        dpif_port_destroy(&dpif_port);
+    }
+
+out:
+    return dev;
+}
+
 static int
 dpctl_put_flow(int argc, const char *argv[], enum dpif_flow_put_flags flags,
                struct dpctl_params *dpctl_p)
 {
     const char *key_s = argv[argc - 2];
     const char *actions_s = argv[argc - 1];
+    struct netdev *in_port_netdev = NULL;
     struct dpif_flow_stats stats;
     struct dpif_port dpif_port;
     struct dpif_port_dump port_dump;
@@ -873,13 +936,40 @@ dpctl_put_flow(int argc, const char *argv[], enum dpif_flow_put_flags flags,
         dpctl_error(dpctl_p, error, "parsing actions");
         goto out_freeactions;
     }
-    error = dpif_flow_put(dpif, flags,
-                          ofpbuf_data(&key), ofpbuf_size(&key),
-                          ofpbuf_size(&mask) == 0 ? NULL : ofpbuf_data(&mask),
-                          ofpbuf_size(&mask),
-                          ofpbuf_data(&actions), ofpbuf_size(&actions),
-                          ufid_present ? &ufid : NULL,
-                          dpctl_p->print_statistics ? &stats : NULL);
+
+    /* For DPDK interface, applies the operation to all pmd threads
+     * on the same numa node. */
+    in_port_netdev = get_in_port_netdev_from_key(dpif, &key);
+    if (in_port_netdev && netdev_is_pmd(in_port_netdev)) {
+        int numa_id;
+
+        numa_id = netdev_get_numa_id(in_port_netdev);
+        if (ovs_numa_numa_id_is_valid(numa_id)) {
+            struct ovs_numa_dump *dump = ovs_numa_dump_cores_on_numa(numa_id);
+            struct ovs_numa_info *iter;
+
+            FOR_EACH_CORE_ON_NUMA (iter, dump) {
+                if (ovs_numa_core_is_pinned(iter->core_id)) {
+                    error = dpif_flow_put(dpif, flags,
+                                          key.data, key.size,
+                                          mask.size == 0 ? NULL : mask.data,
+                                          mask.size, actions.data,
+                                          actions.size, ufid_present ? &ufid : NULL,
+                                          iter->core_id, dpctl_p->print_statistics ? &stats : NULL);
+                }
+            }
+            ovs_numa_dump_destroy(dump);
+        } else {
+            error = EINVAL;
+        }
+    } else {
+        error = dpif_flow_put(dpif, flags,
+                              key.data, key.size,
+                              mask.size == 0 ? NULL : mask.data,
+                              mask.size, actions.data,
+                              actions.size, ufid_present ? &ufid : NULL,
+                              PMD_ID_NULL, dpctl_p->print_statistics ? &stats : NULL);
+    }
     if (error) {
         dpctl_error(dpctl_p, error, "updating flow table");
         goto out_freeactions;
@@ -900,6 +990,7 @@ out_freekeymask:
     ofpbuf_uninit(&mask);
     ofpbuf_uninit(&key);
     dpif_close(dpif);
+    netdev_close(in_port_netdev);
     return error;
 }
 
@@ -964,7 +1055,9 @@ dpctl_get_flow(int argc, const char *argv[], struct dpctl_params *dpctl_p)
         goto out;
     }
 
-    error = dpif_flow_get(dpif, NULL, 0, &ufid, &buf, &flow);
+    /* Does not work for DPDK, since do not know which 'pmd' to apply the
+     * operation.  So, just uses PMD_ID_NULL. */
+    error = dpif_flow_get(dpif, NULL, 0, &ufid, PMD_ID_NULL, &buf, &flow);
     if (error) {
         dpctl_error(dpctl_p, error, "getting flow");
         goto out;
@@ -987,6 +1080,7 @@ static int
 dpctl_del_flow(int argc, const char *argv[], struct dpctl_params *dpctl_p)
 {
     const char *key_s = argv[argc - 1];
+    struct netdev *in_port_netdev = NULL;
     struct dpif_flow_stats stats;
     struct dpif_port dpif_port;
     struct dpif_port_dump port_dump;
@@ -1034,10 +1128,33 @@ dpctl_del_flow(int argc, const char *argv[], struct dpctl_params *dpctl_p)
         goto out;
     }
 
-    error = dpif_flow_del(dpif,
-                          ofpbuf_data(&key), ofpbuf_size(&key),
-                          ufid_present ? &ufid : NULL,
-                          dpctl_p->print_statistics ? &stats : NULL);
+    /* For DPDK interface, applies the operation to all pmd threads
+     * on the same numa node. */
+    in_port_netdev = get_in_port_netdev_from_key(dpif, &key);
+    if (in_port_netdev && netdev_is_pmd(in_port_netdev)) {
+        int numa_id;
+
+        numa_id = netdev_get_numa_id(in_port_netdev);
+        if (ovs_numa_numa_id_is_valid(numa_id)) {
+            struct ovs_numa_dump *dump = ovs_numa_dump_cores_on_numa(numa_id);
+            struct ovs_numa_info *iter;
+
+            FOR_EACH_CORE_ON_NUMA (iter, dump) {
+                if (ovs_numa_core_is_pinned(iter->core_id)) {
+                    error = dpif_flow_del(dpif, key.data,
+                                          key.size, ufid_present ? &ufid : NULL,
+                                          iter->core_id, dpctl_p->print_statistics ? &stats : NULL);
+                }
+            }
+            ovs_numa_dump_destroy(dump);
+        } else {
+            error = EINVAL;
+        }
+    } else {
+        error = dpif_flow_del(dpif, key.data, key.size,
+                              ufid_present ? &ufid : NULL, PMD_ID_NULL,
+                              dpctl_p->print_statistics ? &stats : NULL);
+    }
     if (error) {
         dpctl_error(dpctl_p, error, "deleting flow");
         if (error == ENOENT && !ufid_present) {
@@ -1065,6 +1182,7 @@ out:
     ofpbuf_uninit(&key);
     simap_destroy(&port_names);
     dpif_close(dpif);
+    netdev_close(in_port_netdev);
     return error;
 }
 
@@ -1124,6 +1242,84 @@ dpctl_list_commands(int argc OVS_UNUSED, const char *argv[] OVS_UNUSED,
 
     return 0;
 }
+
+static int
+dpctl_dump_conntrack(int argc, const char *argv[],
+                     struct dpctl_params *dpctl_p)
+{
+    struct ct_dpif_dump_state *dump;
+    struct ct_dpif_entry cte;
+    uint16_t zone, *pzone = NULL;
+    struct dpif *dpif;
+    char *name;
+    int error;
+
+    if (argc > 1 && ovs_scan(argv[argc - 1], "zone=%"SCNu16, &zone)) {
+        pzone = &zone;
+        argc--;
+    }
+    name = (argc == 2) ? xstrdup(argv[1]) : get_one_dp(dpctl_p);
+    if (!name) {
+        return EINVAL;
+    }
+    error = parsed_dpif_open(name, false, &dpif);
+    free(name);
+    if (error) {
+        dpctl_error(dpctl_p, error, "opening datapath");
+        return error;
+    }
+
+    error = ct_dpif_dump_start(dpif, &dump, pzone);
+    if (error) {
+        dpctl_error(dpctl_p, error, "starting conntrack dump");
+        dpif_close(dpif);
+        return error;
+    }
+
+    while (!ct_dpif_dump_next(dump, &cte)) {
+        struct ds s = DS_EMPTY_INITIALIZER;
+
+        ct_dpif_format_entry(&cte, &s, dpctl_p->verbosity,
+                             dpctl_p->print_statistics);
+        ct_dpif_entry_uninit(&cte);
+
+        dpctl_print(dpctl_p, "%s\n", ds_cstr(&s));
+        ds_destroy(&s);
+    }
+    ct_dpif_dump_done(dump);
+    dpif_close(dpif);
+    return error;
+}
+
+static int
+dpctl_flush_conntrack(int argc, const char *argv[],
+                      struct dpctl_params *dpctl_p)
+{
+    struct dpif *dpif;
+    uint16_t zone, *pzone = NULL;
+    char *name;
+    int error;
+
+    if (argc > 1 && ovs_scan(argv[argc - 1], "zone=%"SCNu16, &zone)) {
+        pzone = &zone;
+        argc--;
+    }
+    name = (argc == 2) ? xstrdup(argv[1]) : get_one_dp(dpctl_p);
+    if (!name) {
+        return EINVAL;
+    }
+    error = parsed_dpif_open(name, false, &dpif);
+    free(name);
+    if (error) {
+        dpctl_error(dpctl_p, error, "opening datapath");
+        return error;
+    }
+
+    error = ct_dpif_flush(dpif, pzone);
+
+    dpif_close(dpif);
+    return error;
+}
 \f
 /* Undocumented commands for unit testing. */
 
@@ -1146,7 +1342,7 @@ dpctl_parse_actions(int argc, const char *argv[], struct dpctl_params* dpctl_p)
         }
 
         ds_init(&s);
-        format_odp_actions(&s, ofpbuf_data(&actions), ofpbuf_size(&actions));
+        format_odp_actions(&s, actions.data, actions.size);
         dpctl_print(dpctl_p, "%s\n", ds_cstr(&s));
         ds_destroy(&s);
 
@@ -1290,12 +1486,11 @@ dpctl_normalize_actions(int argc, const char *argv[],
     }
 
     ds_clear(&s);
-    odp_flow_format(ofpbuf_data(&keybuf), ofpbuf_size(&keybuf), NULL, 0, NULL,
+    odp_flow_format(keybuf.data, keybuf.size, NULL, 0, NULL,
                     &s, dpctl_p->verbosity);
     dpctl_print(dpctl_p, "input flow: %s\n", ds_cstr(&s));
 
-    error = odp_flow_key_to_flow(ofpbuf_data(&keybuf), ofpbuf_size(&keybuf),
-                                 &flow);
+    error = odp_flow_key_to_flow(keybuf.data, keybuf.size, &flow);
     if (error) {
         dpctl_error(dpctl_p, error, "odp_flow_key_to_flow");
         goto out_freekeybuf;
@@ -1311,14 +1506,12 @@ dpctl_normalize_actions(int argc, const char *argv[],
 
     if (dpctl_p->verbosity) {
         ds_clear(&s);
-        format_odp_actions(&s, ofpbuf_data(&odp_actions),
-                           ofpbuf_size(&odp_actions));
+        format_odp_actions(&s, odp_actions.data, odp_actions.size);
         dpctl_print(dpctl_p, "input actions: %s\n", ds_cstr(&s));
     }
 
     hmap_init(&actions_per_flow);
-    NL_ATTR_FOR_EACH (a, left, ofpbuf_data(&odp_actions),
-                      ofpbuf_size(&odp_actions)) {
+    NL_ATTR_FOR_EACH (a, left, odp_actions.data, odp_actions.size) {
         const struct ovs_action_push_vlan *push;
         switch(nl_attr_type(a)) {
         case OVS_ACTION_ATTR_POP_VLAN:
@@ -1351,8 +1544,7 @@ dpctl_normalize_actions(int argc, const char *argv[],
     for (i = 0; i < n_afs; i++) {
         struct actions_for_flow *af = afs[i];
 
-        sort_output_actions(ofpbuf_data(&af->actions),
-                            ofpbuf_size(&af->actions));
+        sort_output_actions(af->actions.data, af->actions.size);
 
         if (af->flow.vlan_tci != htons(0)) {
             dpctl_print(dpctl_p, "vlan(vid=%"PRIu16",pcp=%d): ",
@@ -1372,9 +1564,8 @@ dpctl_normalize_actions(int argc, const char *argv[],
         }
 
         ds_clear(&s);
-        format_odp_actions(&s, ofpbuf_data(&af->actions),
-                           ofpbuf_size(&af->actions));
-        dpctl_print(dpctl_p, ds_cstr(&s));
+        format_odp_actions(&s, af->actions.data, af->actions.size);
+        dpctl_puts(dpctl_p, false, ds_cstr(&s));
 
         ofpbuf_uninit(&af->actions);
         free(af);
@@ -1407,6 +1598,8 @@ static const struct dpctl_command all_commands[] = {
     { "get-flow", "get-flow [dp] ufid", 1, 2, dpctl_get_flow },
     { "del-flow", "del-flow [dp] flow", 1, 2, dpctl_del_flow },
     { "del-flows", "[dp]", 0, 1, dpctl_del_flows },
+    { "dump-conntrack", "[dp] [zone=N]", 0, 2, dpctl_dump_conntrack },
+    { "flush-conntrack", "[dp] [zone=N]", 0, 2, dpctl_flush_conntrack },
     { "help", "", 0, INT_MAX, dpctl_help },
     { "list-commands", "", 0, INT_MAX, dpctl_list_commands },
 
@@ -1471,20 +1664,18 @@ dpctl_unixctl_handler(struct unixctl_conn *conn, int argc, const char *argv[],
                       void *aux)
 {
     struct ds ds = DS_EMPTY_INITIALIZER;
-    struct dpctl_params dpctl_p;
-    bool opt_parse_err = false;
-
-    dpctl_command_handler *handler = (dpctl_command_handler *) aux;
+    bool error = false;
 
-    dpctl_p.print_statistics = false;
-    dpctl_p.zero_statistics = false;
-    dpctl_p.may_create = false;
-    dpctl_p.verbosity = 0;
+    struct dpctl_params dpctl_p = {
+        .is_appctl = true,
+        .output = dpctl_unixctl_print,
+        .aux = &ds,
+    };
 
     /* Parse options (like getopt). Unfortunately it does
      * not seem a good idea to call getopt_long() here, since it uses global
      * variables */
-    while (argc > 1 && !opt_parse_err) {
+    while (argc > 1 && !error) {
         const char *arg = argv[1];
         if (!strncmp(arg, "--", 2)) {
             /* Long option */
@@ -1498,13 +1689,13 @@ dpctl_unixctl_handler(struct unixctl_conn *conn, int argc, const char *argv[],
                 dpctl_p.verbosity++;
             } else {
                 ds_put_format(&ds, "Unrecognized option %s", argv[1]);
-                opt_parse_err = true;
+                error = true;
             }
         } else if (arg[0] == '-' && arg[1] != '\0') {
             /* Short option[s] */
             const char *opt = &arg[1];
 
-            while (*opt && !opt_parse_err) {
+            while (*opt && !error) {
                 switch (*opt) {
                 case 'm':
                     dpctl_p.verbosity++;
@@ -1514,7 +1705,7 @@ dpctl_unixctl_handler(struct unixctl_conn *conn, int argc, const char *argv[],
                     break;
                 default:
                     ds_put_format(&ds, "Unrecognized option -%c", *opt);
-                    opt_parse_err = true;
+                    error = true;
                     break;
                 }
                 opt++;
@@ -1524,22 +1715,23 @@ dpctl_unixctl_handler(struct unixctl_conn *conn, int argc, const char *argv[],
             break;
         }
 
-        if (opt_parse_err) {
+        if (error) {
             break;
         }
         argv++;
         argc--;
     }
 
-    if (!opt_parse_err) {
-        dpctl_p.is_appctl = true;
-        dpctl_p.output = dpctl_unixctl_print;
-        dpctl_p.aux = &ds;
-
-        handler(argc, argv, &dpctl_p);
+    if (!error) {
+        dpctl_command_handler *handler = (dpctl_command_handler *) aux;
+        error = handler(argc, argv, &dpctl_p) != 0;
     }
 
-    unixctl_command_reply(conn, ds_cstr(&ds));
+    if (error) {
+        unixctl_command_reply_error(conn, ds_cstr(&ds));
+    } else {
+        unixctl_command_reply(conn, ds_cstr(&ds));
+    }
 
     ds_destroy(&ds);
 }
@@ -1550,9 +1742,11 @@ dpctl_unixctl_register(void)
     const struct dpctl_command *p;
 
     for (p = all_commands; p->name != NULL; p++) {
-        char *cmd_name = xasprintf("dpctl/%s", p->name);
-        unixctl_command_register(cmd_name, "", p->min_args, p->max_args,
-                                 dpctl_unixctl_handler, p->handler);
-        free(cmd_name);
+        if (strcmp(p->name, "help")) {
+            char *cmd_name = xasprintf("dpctl/%s", p->name);
+            unixctl_command_register(cmd_name, "", p->min_args, p->max_args,
+                                     dpctl_unixctl_handler, p->handler);
+            free(cmd_name);
+        }
     }
 }