ovn-nbctl: Make error handling consistent with ovs-vsctl.
authorBen Pfaff <blp@ovn.org>
Sun, 8 May 2016 16:21:41 +0000 (09:21 -0700)
committerBen Pfaff <blp@ovn.org>
Sun, 8 May 2016 16:26:06 +0000 (09:26 -0700)
ovs-vsctl distinguishes between internal database inconsistencies, which
it logs, and errors in commands specified by the user, which cause fatal
exits.  ovn-nbctl wasn't as careful about this and tended to just log
everything.  This commit brings it up to the same standard as ovs-vsctl.

This commit also adds --if-exists and --may-exist options in the same kinds
of places as ovs-vsctl, to allow for scripting in cases where it's OK if
an operation has already occurred.

Signed-off-by: Ben Pfaff <blp@ovn.org>
Acked-by: Justin Pettit <jpettit@ovn.org>
ovn/utilities/ovn-nbctl.8.xml
ovn/utilities/ovn-nbctl.c
tests/ovn-nbctl.at

index f6ef803..9228301 100644 (file)
         Initially the switch will have no ports.
       </dd>
 
-      <dt><code>lswitch-del</code> <var>lswitch</var></dt>
+      <dt>[<code>--if-exists</code>] <code>lswitch-del</code> <var>lswitch</var></dt>
       <dd>
-        Deletes <var>lswitch</var>.
+        Deletes <var>lswitch</var>.  It is an error if <var>lswitch</var> does
+        not exist, unless <code>--if-exists</code> is specified.
       </dd>
 
       <dt><code>lswitch-list</code></dt>
 
     <h1>Logical Port Commands</h1>
     <dl>
-      <dt><code>lport-add</code> <var>lswitch</var> <var>lport</var></dt>
+      <dt>[<code>--may-exist</code>] <code>lport-add</code> <var>lswitch</var> <var>lport</var></dt>
       <dd>
-        Creates on <var>lswitch</var> a new logical port named
-        <var>lport</var>.
+        <p>
+          Creates on <var>lswitch</var> a new logical port named
+          <var>lport</var>.
+        </p>
+
+        <p>
+          It is an error if a logical port named <var>lport</var> already
+          exists, unless <code>--may-exist</code> is specified.  Regardless of
+          <code>--may-exist</code>, it is an error if the existing port is in
+          some logical switch other than <var>lswitch</var> or if it has a
+          parent port.
+        </p>
       </dd>
 
-      <dt><code>lport-add</code> <var>lswitch</var> <var>lport</var> <var>parent</var> <var>tag</var></dt>
+      <dt>[<code>--may-exist</code>] <code>lport-add</code> <var>lswitch</var> <var>lport</var> <var>parent</var> <var>tag</var></dt>
       <dd>
-        Creates on <var>lswitch</var> a logical port named <var>lport</var>
-        that is a child of <var>parent</var> that is identifed with VLAN ID
-        <var>tag</var>.  This is useful in cases such as virtualized
-        container environments where Open vSwitch does not have a direct
-        connection to the container's port and it must be shared with
-        the virtual machine's port.
+        <p>
+          Creates on <var>lswitch</var> a logical port named <var>lport</var>
+          that is a child of <var>parent</var> that is identifed with VLAN ID
+          <var>tag</var>.  This is useful in cases such as virtualized
+          container environments where Open vSwitch does not have a direct
+          connection to the container's port and it must be shared with
+          the virtual machine's port.
+        </p>
+
+        <p>
+          It is an error if a logical port named <var>lport</var> already
+          exists, unless <code>--may-exist</code> is specified.  Regardless of
+          <code>--may-exist</code>, it is an error if the existing port is not
+          in <var>lswitch</var> or if it does not have the specified
+          <var>parent</var> and <var>tag</var>.
+        </p>
       </dd>
 
-      <dt><code>lport-del</code> <var>lport</var></dt>
+      <dt>[<code>--if-exists</code>] <code>lport-del</code> <var>lport</var></dt>
       <dd>
-        Deletes <var>lport</var>.
+        Deletes <var>lport</var>.  It is an error if <var>lport</var> does
+        not exist, unless <code>--if-exists</code> is specified.
       </dd>
 
       <dt><code>lport-list</code> <var>lswitch</var></dt>
index f18fe22..5a5c66c 100644 (file)
@@ -350,17 +350,15 @@ Other options:\n\
 }
 \f
 static const struct nbrec_logical_switch *
-lswitch_by_name_or_uuid(struct ctl_context *ctx, const char *id)
+lswitch_by_name_or_uuid(struct ctl_context *ctx, const char *id,
+                        bool must_exist)
 {
     const struct nbrec_logical_switch *lswitch = NULL;
-    bool is_uuid = false;
-    bool duplicate = false;
-    struct uuid lswitch_uuid;
 
-    if (uuid_from_string(&lswitch_uuid, id)) {
-        is_uuid = true;
-        lswitch = nbrec_logical_switch_get_for_uuid(ctx->idl,
-                                                    &lswitch_uuid);
+    struct uuid lswitch_uuid;
+    bool is_uuid = uuid_from_string(&lswitch_uuid, id);
+    if (is_uuid) {
+        lswitch = nbrec_logical_switch_get_for_uuid(ctx->idl, &lswitch_uuid);
     }
 
     if (!lswitch) {
@@ -371,19 +369,15 @@ lswitch_by_name_or_uuid(struct ctl_context *ctx, const char *id)
                 continue;
             }
             if (lswitch) {
-                VLOG_WARN("There is more than one logical switch named '%s'. "
-                        "Use a UUID.", id);
-                lswitch = NULL;
-                duplicate = true;
-                break;
+                ctl_fatal("Multiple logical switches named '%s'.  "
+                          "Use a UUID.", id);
             }
             lswitch = iter;
         }
     }
 
-    if (!lswitch && !duplicate) {
-        VLOG_WARN("lswitch not found for %s: '%s'",
-                is_uuid ? "UUID" : "name", id);
+    if (!lswitch && must_exist) {
+        ctl_fatal("%s: lswitch %s not found", id, is_uuid ? "UUID" : "name");
     }
 
     return lswitch;
@@ -423,7 +417,7 @@ nbctl_show(struct ctl_context *ctx)
     const struct nbrec_logical_switch *lswitch;
 
     if (ctx->argc == 2) {
-        lswitch = lswitch_by_name_or_uuid(ctx, ctx->argv[1]);
+        lswitch = lswitch_by_name_or_uuid(ctx, ctx->argv[1], false);
         if (lswitch) {
             print_lswitch(lswitch, &ctx->output);
         }
@@ -438,7 +432,6 @@ static void
 nbctl_lswitch_add(struct ctl_context *ctx)
 {
     struct nbrec_logical_switch *lswitch;
-
     lswitch = nbrec_logical_switch_insert(ctx->txn);
     if (ctx->argc == 2) {
         nbrec_logical_switch_set_name(lswitch, ctx->argv[1]);
@@ -448,10 +441,11 @@ nbctl_lswitch_add(struct ctl_context *ctx)
 static void
 nbctl_lswitch_del(struct ctl_context *ctx)
 {
+    bool must_exist = !shash_find(&ctx->options, "--if-exists");
     const char *id = ctx->argv[1];
     const struct nbrec_logical_switch *lswitch;
 
-    lswitch = lswitch_by_name_or_uuid(ctx, id);
+    lswitch = lswitch_by_name_or_uuid(ctx, id, must_exist);
     if (!lswitch) {
         return;
     }
@@ -480,14 +474,14 @@ nbctl_lswitch_list(struct ctl_context *ctx)
 }
 \f
 static const struct nbrec_logical_port *
-lport_by_name_or_uuid(struct ctl_context *ctx, const char *id)
+lport_by_name_or_uuid(struct ctl_context *ctx, const char *id,
+                      bool must_exist)
 {
     const struct nbrec_logical_port *lport = NULL;
-    bool is_uuid = false;
-    struct uuid lport_uuid;
 
-    if (uuid_from_string(&lport_uuid, id)) {
-        is_uuid = true;
+    struct uuid lport_uuid;
+    bool is_uuid = uuid_from_string(&lport_uuid, id);
+    if (is_uuid) {
         lport = nbrec_logical_port_get_for_uuid(ctx->idl, &lport_uuid);
     }
 
@@ -499,45 +493,115 @@ lport_by_name_or_uuid(struct ctl_context *ctx, const char *id)
         }
     }
 
-    if (!lport) {
-        VLOG_WARN("lport not found for %s: '%s'",
-                is_uuid ? "UUID" : "name", id);
+    if (!lport && must_exist) {
+        ctl_fatal("%s: lport %s not found", id, is_uuid ? "UUID" : "name");
     }
 
     return lport;
 }
 
+/* Returns the lswitch that contains 'lport'. */
+static const struct nbrec_logical_switch *
+lport_to_lswitch(const struct ovsdb_idl *idl,
+                 const struct nbrec_logical_port *lport)
+{
+    const struct nbrec_logical_switch *lswitch;
+    NBREC_LOGICAL_SWITCH_FOR_EACH (lswitch, idl) {
+        for (size_t i = 0; i < lswitch->n_ports; i++) {
+            if (lswitch->ports[i] == lport) {
+                return lswitch;
+            }
+        }
+    }
+
+    /* Can't happen because of the database schema */
+    ctl_fatal("logical port %s is not part of any logical switch",
+              lport->name);
+}
+
+static const char *
+lswitch_get_name(const struct nbrec_logical_switch *lswitch,
+                 char uuid_s[UUID_LEN + 1], size_t uuid_s_size)
+{
+    if (lswitch->name[0]) {
+        return lswitch->name;
+    }
+    snprintf(uuid_s, uuid_s_size, UUID_FMT, UUID_ARGS(&lswitch->header_.uuid));
+    return uuid_s;
+}
+
 static void
 nbctl_lport_add(struct ctl_context *ctx)
 {
-    struct nbrec_logical_port *lport;
+    bool may_exist = shash_find(&ctx->options, "--may-exist") != NULL;
+
     const struct nbrec_logical_switch *lswitch;
-    int64_t tag;
+    lswitch = lswitch_by_name_or_uuid(ctx, ctx->argv[1], true);
 
-    lswitch = lswitch_by_name_or_uuid(ctx, ctx->argv[1]);
-    if (!lswitch) {
-        return;
+    const char *parent_name;
+    int64_t tag;
+    if (ctx->argc == 3) {
+        parent_name = NULL;
+        tag = -1;
+    } else if (ctx->argc == 5) {
+        /* Validate tag. */
+        parent_name = ctx->argv[3];
+        if (!ovs_scan(ctx->argv[4], "%"SCNd64, &tag)
+            || tag < 0 || tag > 4095) {
+            ctl_fatal("%s: invalid tag", ctx->argv[4]);
+        }
+    } else {
+        ctl_fatal("lport-add with parent must also specify a tag");
     }
 
-    if (ctx->argc != 3 && ctx->argc != 5) {
-        /* If a parent_name is specified, a tag must be specified as well. */
-        VLOG_WARN("Invalid arguments to lport-add.");
-        return;
-    }
+    const char *lport_name = ctx->argv[2];
+    const struct nbrec_logical_port *lport;
+    lport = lport_by_name_or_uuid(ctx, lport_name, false);
+    if (lport) {
+        if (!may_exist) {
+            ctl_fatal("%s: an lport with this name already exists",
+                      lport_name);
+        }
 
-    if (ctx->argc == 5) {
-        /* Validate tag. */
-        if (!ovs_scan(ctx->argv[4], "%"SCNd64, &tag) || tag < 0 || tag > 4095) {
-            VLOG_WARN("Invalid tag '%s'", ctx->argv[4]);
-            return;
+        const struct nbrec_logical_switch *lsw;
+        lsw = lport_to_lswitch(ctx->idl, lport);
+        if (lsw != lswitch) {
+            char uuid_s[UUID_LEN + 1];
+            ctl_fatal("%s: lport already exists but in lswitch %s", lport_name,
+                      lswitch_get_name(lsw, uuid_s, sizeof uuid_s));
+        }
+
+        if (parent_name) {
+            if (!lport->parent_name) {
+                ctl_fatal("%s: lport already exists but has no parent",
+                          lport_name);
+            } else if (strcmp(parent_name, lport->parent_name)) {
+                ctl_fatal("%s: lport already exists with different parent %s",
+                          lport_name, lport->parent_name);
+            }
+
+            if (!lport->n_tag) {
+                ctl_fatal("%s: lport already exists but has no tag",
+                          lport_name);
+            } else if (lport->tag[0] != tag) {
+                ctl_fatal("%s: lport already exists with different "
+                          "tag %"PRId64, lport_name, lport->tag[0]);
+            }
+        } else {
+            if (lport->parent_name) {
+                ctl_fatal("%s: lport already exists but has parent %s",
+                          lport_name, lport->parent_name);
+            }
         }
+
+        return;
     }
 
     /* Create the logical port. */
     lport = nbrec_logical_port_insert(ctx->txn);
-    nbrec_logical_port_set_name(lport, ctx->argv[2]);
-    if (ctx->argc == 5) {
-        nbrec_logical_port_set_parent_name(lport, ctx->argv[3]);
+    nbrec_logical_port_set_name(lport, lport_name);
+    if (tag >= 0) {
+        nbrec_logical_port_set_parent_name(lport, parent_name);
         nbrec_logical_port_set_tag(lport, &tag, 1);
     }
 
@@ -546,7 +610,8 @@ nbctl_lport_add(struct ctl_context *ctx)
     struct nbrec_logical_port **new_ports = xmalloc(sizeof *new_ports *
                                                     (lswitch->n_ports + 1));
     memcpy(new_ports, lswitch->ports, sizeof *new_ports * lswitch->n_ports);
-    new_ports[lswitch->n_ports] = lport;
+    new_ports[lswitch->n_ports] = CONST_CAST(struct nbrec_logical_port *,
+                                             lport);
     nbrec_logical_switch_set_ports(lswitch, new_ports, lswitch->n_ports + 1);
     free(new_ports);
 }
@@ -576,9 +641,10 @@ remove_lport(const struct nbrec_logical_switch *lswitch, size_t idx)
 static void
 nbctl_lport_del(struct ctl_context *ctx)
 {
+    bool must_exist = !shash_find(&ctx->options, "--if-exists");
     const struct nbrec_logical_port *lport;
 
-    lport = lport_by_name_or_uuid(ctx, ctx->argv[1]);
+    lport = lport_by_name_or_uuid(ctx, ctx->argv[1], must_exist);
     if (!lport) {
         return;
     }
@@ -594,7 +660,8 @@ nbctl_lport_del(struct ctl_context *ctx)
         }
     }
 
-    VLOG_WARN("logical port %s is not part of any logical switch",
+    /* Can't happen because of the database schema. */
+    ctl_fatal("logical port %s is not part of any logical switch",
               ctx->argv[1]);
 }
 
@@ -606,10 +673,7 @@ nbctl_lport_list(struct ctl_context *ctx)
     struct smap lports;
     size_t i;
 
-    lswitch = lswitch_by_name_or_uuid(ctx, id);
-    if (!lswitch) {
-        return;
-    }
+    lswitch = lswitch_by_name_or_uuid(ctx, id, true);
 
     smap_init(&lports);
     for (i = 0; i < lswitch->n_ports; i++) {
@@ -631,11 +695,7 @@ nbctl_lport_get_parent(struct ctl_context *ctx)
 {
     const struct nbrec_logical_port *lport;
 
-    lport = lport_by_name_or_uuid(ctx, ctx->argv[1]);
-    if (!lport) {
-        return;
-    }
-
+    lport = lport_by_name_or_uuid(ctx, ctx->argv[1], true);
     if (lport->parent_name) {
         ds_put_format(&ctx->output, "%s\n", lport->parent_name);
     }
@@ -646,11 +706,7 @@ nbctl_lport_get_tag(struct ctl_context *ctx)
 {
     const struct nbrec_logical_port *lport;
 
-    lport = lport_by_name_or_uuid(ctx, ctx->argv[1]);
-    if (!lport) {
-        return;
-    }
-
+    lport = lport_by_name_or_uuid(ctx, ctx->argv[1], true);
     if (lport->n_tag > 0) {
         ds_put_format(&ctx->output, "%"PRId64"\n", lport->tag[0]);
     }
@@ -662,10 +718,7 @@ nbctl_lport_set_addresses(struct ctl_context *ctx)
     const char *id = ctx->argv[1];
     const struct nbrec_logical_port *lport;
 
-    lport = lport_by_name_or_uuid(ctx, id);
-    if (!lport) {
-        return;
-    }
+    lport = lport_by_name_or_uuid(ctx, id, true);
 
     int i;
     for (i = 2; i < ctx->argc; i++) {
@@ -674,11 +727,10 @@ nbctl_lport_set_addresses(struct ctl_context *ctx)
         if (strcmp(ctx->argv[i], "unknown")
             && !ovs_scan(ctx->argv[i], ETH_ADDR_SCAN_FMT,
                          ETH_ADDR_SCAN_ARGS(ea))) {
-            VLOG_ERR("Invalid address format (%s). See ovn-nb(5). "
-                     "Hint: An Ethernet address must be "
-                     "listed before an IP address, together as a single "
-                     "argument.", ctx->argv[i]);
-            return;
+            ctl_fatal("%s: Invalid address format. See ovn-nb(5). "
+                      "Hint: An Ethernet address must be "
+                      "listed before an IP address, together as a single "
+                      "argument.", ctx->argv[i]);
         }
     }
 
@@ -695,10 +747,7 @@ nbctl_lport_get_addresses(struct ctl_context *ctx)
     const char *mac;
     size_t i;
 
-    lport = lport_by_name_or_uuid(ctx, id);
-    if (!lport) {
-        return;
-    }
+    lport = lport_by_name_or_uuid(ctx, id, true);
 
     svec_init(&addresses);
     for (i = 0; i < lport->n_addresses; i++) {
@@ -717,11 +766,7 @@ nbctl_lport_set_port_security(struct ctl_context *ctx)
     const char *id = ctx->argv[1];
     const struct nbrec_logical_port *lport;
 
-    lport = lport_by_name_or_uuid(ctx, id);
-    if (!lport) {
-        return;
-    }
-
+    lport = lport_by_name_or_uuid(ctx, id, true);
     nbrec_logical_port_set_port_security(lport,
             (const char **) ctx->argv + 2, ctx->argc - 2);
 }
@@ -735,11 +780,7 @@ nbctl_lport_get_port_security(struct ctl_context *ctx)
     const char *addr;
     size_t i;
 
-    lport = lport_by_name_or_uuid(ctx, id);
-    if (!lport) {
-        return;
-    }
-
+    lport = lport_by_name_or_uuid(ctx, id, true);
     svec_init(&addrs);
     for (i = 0; i < lport->n_port_security; i++) {
         svec_add(&addrs, lport->port_security[i]);
@@ -757,11 +798,7 @@ nbctl_lport_get_up(struct ctl_context *ctx)
     const char *id = ctx->argv[1];
     const struct nbrec_logical_port *lport;
 
-    lport = lport_by_name_or_uuid(ctx, id);
-    if (!lport) {
-        return;
-    }
-
+    lport = lport_by_name_or_uuid(ctx, id, true);
     ds_put_format(&ctx->output,
                   "%s\n", (lport->up && *lport->up) ? "up" : "down");
 }
@@ -773,11 +810,7 @@ nbctl_lport_set_enabled(struct ctl_context *ctx)
     const char *state = ctx->argv[2];
     const struct nbrec_logical_port *lport;
 
-    lport = lport_by_name_or_uuid(ctx, id);
-    if (!lport) {
-        return;
-    }
-
+    lport = lport_by_name_or_uuid(ctx, id, true);
     if (!strcasecmp(state, "enabled")) {
         bool enabled = true;
         nbrec_logical_port_set_enabled(lport, &enabled, 1);
@@ -785,7 +818,8 @@ nbctl_lport_set_enabled(struct ctl_context *ctx)
         bool enabled = false;
         nbrec_logical_port_set_enabled(lport, &enabled, 1);
     } else {
-        VLOG_ERR("Invalid state '%s' provided to lport-set-enabled", state);
+        ctl_fatal("%s: state must be \"enabled\" or \"disabled\"",
+                  state);
     }
 }
 
@@ -795,11 +829,7 @@ nbctl_lport_get_enabled(struct ctl_context *ctx)
     const char *id = ctx->argv[1];
     const struct nbrec_logical_port *lport;
 
-    lport = lport_by_name_or_uuid(ctx, id);
-    if (!lport) {
-        return;
-    }
-
+    lport = lport_by_name_or_uuid(ctx, id, true);
     ds_put_format(&ctx->output, "%s\n",
                   !lport->enabled || *lport->enabled ? "enabled" : "disabled");
 }
@@ -811,11 +841,7 @@ nbctl_lport_set_type(struct ctl_context *ctx)
     const char *type = ctx->argv[2];
     const struct nbrec_logical_port *lport;
 
-    lport = lport_by_name_or_uuid(ctx, id);
-    if (!lport) {
-        return;
-    }
-
+    lport = lport_by_name_or_uuid(ctx, id, true);
     nbrec_logical_port_set_type(lport, type);
 }
 
@@ -825,11 +851,7 @@ nbctl_lport_get_type(struct ctl_context *ctx)
     const char *id = ctx->argv[1];
     const struct nbrec_logical_port *lport;
 
-    lport = lport_by_name_or_uuid(ctx, id);
-    if (!lport) {
-        return;
-    }
-
+    lport = lport_by_name_or_uuid(ctx, id, true);
     ds_put_format(&ctx->output, "%s\n", lport->type);
 }
 
@@ -841,11 +863,7 @@ nbctl_lport_set_options(struct ctl_context *ctx)
     size_t i;
     struct smap options = SMAP_INITIALIZER(&options);
 
-    lport = lport_by_name_or_uuid(ctx, id);
-    if (!lport) {
-        return;
-    }
-
+    lport = lport_by_name_or_uuid(ctx, id, true);
     for (i = 2; i < ctx->argc; i++) {
         char *key, *value;
         value = xstrdup(ctx->argv[i]);
@@ -868,11 +886,7 @@ nbctl_lport_get_options(struct ctl_context *ctx)
     const struct nbrec_logical_port *lport;
     struct smap_node *node;
 
-    lport = lport_by_name_or_uuid(ctx, id);
-    if (!lport) {
-        return;
-    }
-
+    lport = lport_by_name_or_uuid(ctx, id, true);
     SMAP_FOR_EACH(node, &lport->options) {
         ds_put_format(&ctx->output, "%s=%s\n", node->key, node->value);
     }
@@ -922,10 +936,7 @@ nbctl_acl_list(struct ctl_context *ctx)
     const struct nbrec_acl **acls;
     size_t i;
 
-    lswitch = lswitch_by_name_or_uuid(ctx, ctx->argv[1]);
-    if (!lswitch) {
-        return;
-    }
+    lswitch = lswitch_by_name_or_uuid(ctx, ctx->argv[1], true);
 
     acls = xmalloc(sizeof *acls * lswitch->n_acls);
     for (i = 0; i < lswitch->n_acls; i++) {
@@ -943,40 +954,47 @@ nbctl_acl_list(struct ctl_context *ctx)
     free(acls);
 }
 
-static void
-nbctl_acl_add(struct ctl_context *ctx)
+static const char *
+parse_direction(const char *arg)
 {
-    const struct nbrec_logical_switch *lswitch;
-    const char *action = ctx->argv[5];
-    const char *direction;
-    int64_t priority;
-
-    lswitch = lswitch_by_name_or_uuid(ctx, ctx->argv[1]);
-    if (!lswitch) {
-        return;
-    }
-
     /* Validate direction.  Only require the first letter. */
-    if (ctx->argv[2][0] == 't') {
-        direction = "to-lport";
-    } else if (ctx->argv[2][0] == 'f') {
-        direction = "from-lport";
+    if (arg[0] == 't') {
+        return "to-lport";
+    } else if (arg[0] == 'f') {
+        return "from-lport";
     } else {
-        VLOG_WARN("Invalid direction '%s'", ctx->argv[2]);
-        return;
+        ctl_fatal("%s: direction must be \"to-lport\" or \"from-lport\"", arg);
     }
+}
 
+static int
+parse_priority(const char *arg)
+{
     /* Validate priority. */
-    if (!ovs_scan(ctx->argv[3], "%"SCNd64, &priority) || priority < 0
-        || priority > 32767) {
-        VLOG_WARN("Invalid priority '%s'", ctx->argv[3]);
-        return;
+    int64_t priority;
+    if (!ovs_scan(arg, "%"SCNd64, &priority)
+        || priority < 0 || priority > 32767) {
+        ctl_fatal("%s: priority must in range 0...32767", arg);
     }
+    return priority;
+}
+
+static void
+nbctl_acl_add(struct ctl_context *ctx)
+{
+    const struct nbrec_logical_switch *lswitch;
+    const char *action = ctx->argv[5];
+
+    lswitch = lswitch_by_name_or_uuid(ctx, ctx->argv[1], true);
+
+    const char *direction = parse_direction(ctx->argv[2]);
+    int64_t priority = parse_priority(ctx->argv[3]);
 
     /* Validate action. */
     if (strcmp(action, "allow") && strcmp(action, "allow-related")
         && strcmp(action, "drop") && strcmp(action, "reject")) {
-        VLOG_WARN("Invalid action '%s'", action);
+        ctl_fatal("%s: action must be one of \"allow\", \"allow-related\", "
+                  "\"drop\", and \"reject\"", action);
         return;
     }
 
@@ -1004,17 +1022,10 @@ static void
 nbctl_acl_del(struct ctl_context *ctx)
 {
     const struct nbrec_logical_switch *lswitch;
-    const char *direction;
-    int64_t priority = 0;
-
-    lswitch = lswitch_by_name_or_uuid(ctx, ctx->argv[1]);
-    if (!lswitch) {
-        return;
-    }
+    lswitch = lswitch_by_name_or_uuid(ctx, ctx->argv[1], true);
 
     if (ctx->argc != 2 && ctx->argc != 3 && ctx->argc != 5) {
-        VLOG_WARN("Invalid number of arguments");
-        return;
+        ctl_fatal("cannot specify priority without match");
     }
 
     if (ctx->argc == 2) {
@@ -1025,15 +1036,7 @@ nbctl_acl_del(struct ctl_context *ctx)
         return;
     }
 
-    /* Validate direction.  Only require first letter. */
-    if (ctx->argv[2][0] == 't') {
-        direction = "to-lport";
-    } else if (ctx->argv[2][0] == 'f') {
-        direction = "from-lport";
-    } else {
-        VLOG_WARN("Invalid direction '%s'", ctx->argv[2]);
-        return;
-    }
+    const char *direction = parse_direction(ctx->argv[2]);
 
     /* If priority and match are not specified, delete all ACLs with the
      * specified direction. */
@@ -1054,12 +1057,7 @@ nbctl_acl_del(struct ctl_context *ctx)
         return;
     }
 
-    /* Validate priority. */
-    if (!ovs_scan(ctx->argv[3], "%"SCNd64, &priority) || priority < 0
-        || priority > 32767) {
-        VLOG_WARN("Invalid priority '%s'", ctx->argv[3]);
-        return;
-    }
+    int64_t priority = parse_priority(ctx->argv[3]);
 
     /* Remove the matching rule. */
     for (size_t i = 0; i < lswitch->n_acls; i++) {
@@ -1314,7 +1312,7 @@ static const struct ctl_command_syntax nbctl_commands[] = {
     { "lswitch-add", 0, 1, "[LSWITCH]", NULL, nbctl_lswitch_add,
       NULL, "", RW },
     { "lswitch-del", 1, 1, "LSWITCH", NULL, nbctl_lswitch_del,
-      NULL, "", RW },
+      NULL, "--if-exists", RW },
     { "lswitch-list", 0, 0, "", NULL, nbctl_lswitch_list, NULL, "", RO },
 
     /* acl commands. */
@@ -1326,8 +1324,9 @@ static const struct ctl_command_syntax nbctl_commands[] = {
 
     /* lport commands. */
     { "lport-add", 2, 4, "LSWITCH LPORT [PARENT] [TAG]", NULL, nbctl_lport_add,
-      NULL, "", RW },
-    { "lport-del", 1, 1, "LPORT", NULL, nbctl_lport_del, NULL, "", RW },
+      NULL, "--may-exist", RW },
+    { "lport-del", 1, 1, "LPORT", NULL, nbctl_lport_del, NULL, "--if-exists",
+      RW },
     { "lport-list", 1, 1, "LSWITCH", NULL, nbctl_lport_list, NULL, "", RO },
     { "lport-get-parent", 1, 1, "LPORT", NULL, nbctl_lport_get_parent, NULL,
       "", RO },
index 85f8974..a8ed41e 100644 (file)
@@ -48,6 +48,21 @@ AT_CHECK([ovn-nbctl lswitch-list | ${PERL} $srcdir/uuidfilt.pl], [0], [dnl
 <0> (ls1)
 ])
 
+AT_CHECK([ovn-nbctl show ls0])
+AT_CHECK([ovn-nbctl lswitch-add ls0])
+AT_CHECK([ovn-nbctl show ls0 | ${PERL} $srcdir/uuidfilt.pl], [0],
+  [    lswitch <0> (ls0)
+])
+AT_CHECK([ovn-nbctl lswitch-add ls0])
+AT_CHECK([ovn-nbctl lswitch-del ls0], [1], [],
+  [ovn-nbctl: Multiple logical switches named 'ls0'.  Use a UUID.
+])
+
+AT_CHECK([ovn-nbctl lswitch-del ls2], [1], [],
+  [ovn-nbctl: ls2: lswitch name not found
+])
+AT_CHECK([ovn-nbctl --if-exists lswitch-del ls2])
+
 OVN_NBCTL_TEST_STOP
 AT_CLEANUP
 
@@ -58,6 +73,10 @@ OVN_NBCTL_TEST_START
 
 AT_CHECK([ovn-nbctl lswitch-add ls0])
 AT_CHECK([ovn-nbctl lport-add ls0 lp0])
+AT_CHECK([ovn-nbctl lport-add ls0 lp0], [1], [],
+  [ovn-nbctl: lp0: an lport with this name already exists
+])
+AT_CHECK([ovn-nbctl --may-exist lport-add ls0 lp0])
 AT_CHECK([ovn-nbctl lport-list ls0 | ${PERL} $srcdir/uuidfilt.pl], [0], [dnl
 <0> (lp0)
 ])
@@ -68,11 +87,34 @@ AT_CHECK([ovn-nbctl lport-list ls0 | ${PERL} $srcdir/uuidfilt.pl], [0], [dnl
 <1> (lp1)
 ])
 
+AT_CHECK([ovn-nbctl lswitch-add ls1])
+AT_CHECK([ovn-nbctl lport-add ls0 lp1], [1], [],
+  [ovn-nbctl: lp1: an lport with this name already exists
+])
+AT_CHECK([ovn-nbctl --may-exist lport-add ls1 lp1], [1], [],
+  [ovn-nbctl: lp1: lport already exists but in lswitch ls0
+])
+AT_CHECK([ovn-nbctl --may-exist lport-add ls0 lp1 lp0 5], [1], [],
+  [ovn-nbctl: lp1: lport already exists but has no parent
+])
+
 AT_CHECK([ovn-nbctl lport-del lp1])
 AT_CHECK([ovn-nbctl lport-list ls0 | ${PERL} $srcdir/uuidfilt.pl], [0], [dnl
 <0> (lp0)
 ])
 
+AT_CHECK([ovn-nbctl lport-add ls0 lp2 lp3 5])
+AT_CHECK([ovn-nbctl --may-exist lport-add ls0 lp2 lp4 5], [1], [],
+  [ovn-nbctl: lp2: lport already exists with different parent lp3
+])
+AT_CHECK([ovn-nbctl --may-exist lport-add ls0 lp2 lp3 10], [1], [],
+  [ovn-nbctl: lp2: lport already exists with different tag 5
+])
+AT_CHECK([ovn-nbctl clear Logical_Port lp2 tag])
+AT_CHECK([ovn-nbctl --may-exist lport-add ls0 lp2 lp3 5], [1], [],
+  [ovn-nbctl: lp2: lport already exists but has no tag
+])
+
 OVN_NBCTL_TEST_STOP
 AT_CLEANUP