diff mbox

[ovs-dev,2/2] ovn-nbctl: Add ACL commands.

Message ID 20150905235325.GB23703@nicira.com
State Not Applicable
Headers show

Commit Message

Ben Pfaff Sept. 5, 2015, 11:53 p.m. UTC 
On Fri, Sep 04, 2015 at 05:39:02PM -0700, Justin Pettit wrote:
> Signed-off-by: Justin Pettit <jpettit@nicira.com>

I came really close to duplicating your work here on Thursday when I was
writing tests.  I'm glad that didn't happen.

The printf() specifier is wrong here:


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

Comments

Justin Pettit Sept. 10, 2015, 9:07 p.m. UTC | #1 
> On Sep 5, 2015, at 4:53 PM, Ben Pfaff <blp@nicira.com> wrote:
> 
> -        printf("%10s %5ld (%s) %s%s\n", acl->direction, acl->priority,
> +        printf("%10s %5"PRId64" (%s) %s%s\n", acl->direction, acl->priority,

Fixed.

> I'd prefer to avoid casts in acl_cmp(), also the macro there doesn't
> seem to help much:

This is better.  Thanks.

> +    if (dir1 != dir2) {
> +        return dir1 > dir2 ? -1 : 1;

I changed this to "<", so that it would sort from smallest to largest, which means "from-lport" comes before "to-lport".

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

Thanks.  Assuming you agree with the above change, I'll keep this ack.

--Justin
Ben Pfaff Sept. 10, 2015, 9:31 p.m. UTC | #2 
On Thu, Sep 10, 2015 at 02:07:54PM -0700, Justin Pettit wrote:
> 
> > On Sep 5, 2015, at 4:53 PM, Ben Pfaff <blp@nicira.com> wrote:
> > 
> > -        printf("%10s %5ld (%s) %s%s\n", acl->direction, acl->priority,
> > +        printf("%10s %5"PRId64" (%s) %s%s\n", acl->direction, acl->priority,
> 
> Fixed.
> 
> > I'd prefer to avoid casts in acl_cmp(), also the macro there doesn't
> > seem to help much:
> 
> This is better.  Thanks.
> 
> > +    if (dir1 != dir2) {
> > +        return dir1 > dir2 ? -1 : 1;
> 
> I changed this to "<", so that it would sort from smallest to largest, which means "from-lport" comes before "to-lport".
> 
> > Acked-by: Ben Pfaff <blp@nicira.com>
> 
> Thanks.  Assuming you agree with the above change, I'll keep this ack.

Looks good to me, thanks.
diff mbox

Patch

diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
index 0b19521..26a4735 100644
--- a/ovn/utilities/ovn-nbctl.c
+++ b/ovn/utilities/ovn-nbctl.c
@@ -824,7 +824,7 @@  do_acl_list(struct ovs_cmdl_context *ctx)
 
     for (i = 0; i < lswitch->n_acls; i++) {
         const struct nbrec_acl *acl = acls[i];
-        printf("%10s %5ld (%s) %s%s\n", acl->direction, acl->priority,
+        printf("%10s %5"PRId64" (%s) %s%s\n", acl->direction, acl->priority,
                 acl->match, acl->action, acl->log ? " log" : "");
     }
 
I'd prefer to avoid casts in acl_cmp(), also the macro there doesn't
seem to help much:

diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
index 0b19521..384bc7b 100644
--- a/ovn/utilities/ovn-nbctl.c
+++ b/ovn/utilities/ovn-nbctl.c
@@ -775,31 +775,20 @@  dir_encode(const char *dir)
 static int
 acl_cmp(const void *acl1_, const void *acl2_)
 {
-    const struct nbrec_acl *acl1, *acl2;
-
-    acl1 = *((struct nbrec_acl **) acl1_);
-    acl2 = *((struct nbrec_acl **) acl2_);
+    const struct nbrec_acl *const *acl1p = acl1_;
+    const struct nbrec_acl *const *acl2p = acl2_;
+    const struct nbrec_acl *acl1 = *acl1p;
+    const struct nbrec_acl *acl2 = *acl2p;
 
     int dir1 = dir_encode(acl1->direction);
     int dir2 = dir_encode(acl2->direction);
-
-#define CMP(expr) \
-    do { \
-        int res; \
-        res = (expr); \
-        if (res) { \
-            return res; \
-        } \
-    } while (0)
-
-    CMP(dir1 - dir2);
-    CMP(acl1->priority > acl2->priority ? -1 :
-            (acl1->priority < acl2->priority ? 1 : 0));
-    CMP(strcmp(acl1->match, acl2->match));
-
-#undef CMP
-
-    return 0;
+    if (dir1 != dir2) {
+        return dir1 > dir2 ? -1 : 1;
+    } else if (acl1->priority != acl2->priority) {
+        return acl1->priority > acl2->priority ? -1 : 1;
+    } else {
+        return strcmp(acl1->match, acl2->match);
+    }
 }
 
 static void