[ovs-dev] [PATCH v6 1/3] ovn: Assign zone-id consistently

[Ben Pfaff]

On Tue, Mar 22, 2016 at 04:03:57PM -0400, Ramu Ramamurthy wrote:
> Currently, ovn-controller does not record the
> zoneid assigned to logical port. Tests indicate that
> zoneids can be different on the same logical port
> after ovn-controller restart.
> 
> The fix sets the zone-id as an external-id of the interface record,
> and recovers the zone-id from that record.
> 
> Reported-by: Russell Bryant <russell at ovn.org>
> Reported-at: https://bugs.launchpad.net/networking-ovn/+bug/1538696
> Signed-off-by: Ramu Ramamurthy <ramu.ramamurthy at us.ibm.com>

Thanks for the patch.

I'm concerned about some ordering of operations here.  In the main loop,
binding_run() gets called before patch_run().  The former looks for
localnet-related patch ports, but the latter is what actually creates
the localnet-related patch ports.  That means that it takes two trips
through the main loop for this to converge; that's usually undesirable.

When a variable can be declared in an inner scope or an outer one,
please declare it in the outer one, because this makes it easier for the
reader to see what is going on.  For example, here in
get_zones_on_ifaces(), the 'zone_id' declaration can be moved inside the
loop:

        int zone_id;
        SMAP_FOR_EACH(zone, &iface_rec->external_ids) {
            zone_id = smap_get_int(&iface_rec->external_ids, zone->key, 0);
            if (!strncmp(zone->key, OVN_ZONE_ID, strlen(OVN_ZONE_ID)) &&
                zone_id) {
                simap_put(iface_zones, zone->key+strlen(OVN_ZONE_ID), zone_id);
                shash_add(parent_ports, zone->key+strlen(OVN_ZONE_ID), iface_rec);
            }
        }

e.g. like this:

        SMAP_FOR_EACH(zone, &iface_rec->external_ids) {
            int zone_id = smap_get_int(&iface_rec->external_ids, zone->key, 0);
            if (!strncmp(zone->key, OVN_ZONE_ID, strlen(OVN_ZONE_ID)) &&
                zone_id) {
                simap_put(iface_zones, zone->key+strlen(OVN_ZONE_ID), zone_id);
                shash_add(parent_ports, zone->key+strlen(OVN_ZONE_ID), iface_rec);
            }
        }

Also in get_zones_on_ifaces(), it looks like 'already_added' could be an
sset instead of an shash, because the values in the shash are never
used.

There's a lot of minor style issues here, I'll quote a few things from
CodingStyle.md to give you ideas:

      Comments should be written as full sentences that start with a
    capital letter and end with a period.  Put two spaces between
    sentences.

      Put one space on each side of infix binary and ternary operators:

        * / %
        + -
        << >>
        < <= > >=
        == !=
        &
        ^
        |
        &&
        ||
        ?:
        = += -= *= /= %= &= ^= |= <<= >>=

      Do not put any white space around postfix, prefix, or grouping
    operators:

        () [] -> .
        ! ~ ++ -- + - * &

      Put the return type, function name, and the braces that surround the
    function's code on separate lines, all starting in column 0.

I think I will have other comments later, but in this version of the
patch I'm distracted by lots of superficial things described above.

Thanks,

Ben.