[ovs-dev] ovn-nbctl: fix validation logic for acl-del command

Message ID 1504896980-37224-1-git-send-email-zhouhan@gmail.com
State Accepted
Headers show
Series
  • [ovs-dev] ovn-nbctl: fix validation logic for acl-del command
Related show

Commit Message

Han Zhou Sept. 8, 2017, 6:56 p.m.
The command error message is misleading, e.g.:

$ ovn-nbctl acl-del ls1 100 'outport=="lsp1" && ip4'
ovn-nbctl: cannot specify priority without match

$ ovn-nbctl acl-del ls1 to-lport 'outport=="lsp1" && ip4'
ovn-nbctl: cannot specify priority without match

This patch fixes the problem.
$
$ ovn-nbctl acl-del ls1 100 'outport=="lsp1" && ip4'

Signed-off-by: Han Zhou <zhouhan@gmail.com>
---
 ovn/utilities/ovn-nbctl.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Mark Michelson Sept. 11, 2017, 3:56 p.m. | #1
On Fri, Sep 8, 2017 at 1:57 PM Han Zhou <zhouhan@gmail.com> wrote:

> The command error message is misleading, e.g.:
>
> $ ovn-nbctl acl-del ls1 100 'outport=="lsp1" && ip4'
> ovn-nbctl: cannot specify priority without match
>
> $ ovn-nbctl acl-del ls1 to-lport 'outport=="lsp1" && ip4'
> ovn-nbctl: cannot specify priority without match
>
> This patch fixes the problem.
> $
> $ ovn-nbctl acl-del ls1 100 'outport=="lsp1" && ip4'


> Signed-off-by: Han Zhou <zhouhan@gmail.com>
>
Acked-by: Mark Michelson <mmichels@redhat.com>

> ---
>  ovn/utilities/ovn-nbctl.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
> index 46ede4e..8e5c1a4 100644
> --- a/ovn/utilities/ovn-nbctl.c
> +++ b/ovn/utilities/ovn-nbctl.c
> @@ -1469,10 +1469,6 @@ nbctl_acl_del(struct ctl_context *ctx)
>      const struct nbrec_logical_switch *ls;
>      ls = ls_by_name_or_uuid(ctx, ctx->argv[1], true);
>
> -    if (ctx->argc != 2 && ctx->argc != 3 && ctx->argc != 5) {
> -        ctl_fatal("cannot specify priority without match");
> -    }
> -
>      if (ctx->argc == 2) {
>          /* If direction, priority, and match are not specified, delete
>           * all ACLs. */
> @@ -1503,6 +1499,10 @@ nbctl_acl_del(struct ctl_context *ctx)
>
>      int64_t priority = parse_priority(ctx->argv[3]);
>
> +    if (ctx->argc == 4) {
> +        ctl_fatal("cannot specify priority without match");
> +    }
> +
>      /* Remove the matching rule. */
>      for (size_t i = 0; i < ls->n_acls; i++) {
>          struct nbrec_acl *acl = ls->acls[i];
> --
> 2.1.0
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Justin Pettit Sept. 11, 2017, 5:45 p.m. | #2
> On Sep 8, 2017, at 11:56 AM, Han Zhou <zhouhan@gmail.com> wrote:
> 
> The command error message is misleading, e.g.:
> 
> $ ovn-nbctl acl-del ls1 100 'outport=="lsp1" && ip4'
> ovn-nbctl: cannot specify priority without match
> 
> $ ovn-nbctl acl-del ls1 to-lport 'outport=="lsp1" && ip4'
> ovn-nbctl: cannot specify priority without match
> 
> This patch fixes the problem.
> $
> $ ovn-nbctl acl-del ls1 100 'outport=="lsp1" && ip4'
> 
> Signed-off-by: Han Zhou <zhouhan@gmail.com>

Thanks.  I pushed this to master.

--Justin
Justin Pettit Sept. 11, 2017, 5:57 p.m. | #3
> On Sep 11, 2017, at 8:56 AM, Mark Michelson <mmichels@redhat.com> wrote:
> 
> On Fri, Sep 8, 2017 at 1:57 PM Han Zhou <zhouhan@gmail.com> wrote:
> 
>> The command error message is misleading, e.g.:
>> 
>> $ ovn-nbctl acl-del ls1 100 'outport=="lsp1" && ip4'
>> ovn-nbctl: cannot specify priority without match
>> 
>> $ ovn-nbctl acl-del ls1 to-lport 'outport=="lsp1" && ip4'
>> ovn-nbctl: cannot specify priority without match
>> 
>> This patch fixes the problem.
>> $
>> $ ovn-nbctl acl-del ls1 100 'outport=="lsp1" && ip4'
> 
> 
>> Signed-off-by: Han Zhou <zhouhan@gmail.com>
>> 
> Acked-by: Mark Michelson <mmichels@redhat.com>

I missed your acked-by before I pushed it.  Sorry about that, Mark!

--Justin

Patch

diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
index 46ede4e..8e5c1a4 100644
--- a/ovn/utilities/ovn-nbctl.c
+++ b/ovn/utilities/ovn-nbctl.c
@@ -1469,10 +1469,6 @@  nbctl_acl_del(struct ctl_context *ctx)
     const struct nbrec_logical_switch *ls;
     ls = ls_by_name_or_uuid(ctx, ctx->argv[1], true);
 
-    if (ctx->argc != 2 && ctx->argc != 3 && ctx->argc != 5) {
-        ctl_fatal("cannot specify priority without match");
-    }
-
     if (ctx->argc == 2) {
         /* If direction, priority, and match are not specified, delete
          * all ACLs. */
@@ -1503,6 +1499,10 @@  nbctl_acl_del(struct ctl_context *ctx)
 
     int64_t priority = parse_priority(ctx->argv[3]);
 
+    if (ctx->argc == 4) {
+        ctl_fatal("cannot specify priority without match");
+    }
+
     /* Remove the matching rule. */
     for (size_t i = 0; i < ls->n_acls; i++) {
         struct nbrec_acl *acl = ls->acls[i];