[ovs-dev] [PATCH v5 06/13] lib/odp-util: Skip ignored fields when parsing and formatting.

Ben Pfaff blp at nicira.com
Tue Sep 9 00:06:05 UTC 2014


On Fri, Sep 05, 2014 at 04:05:13PM -0700, Jarno Rajahalme wrote:
> When a whole field of a key value is ignored, skip it when formatting
> the key, and allow it to be left out when parsing the key from a
> string.  However, when the unmasked bits have non-zero values (as in
> keys received from a datapath), or when the 'verbose' formatting is
> requested those are still formatted, as it may help in debugging.
> 
> Now the named key fields can also be given in arbitrary order.
> Duplicate field values are not checked for, so the last one will
> remain in effect.
> 
> Signed-off-by: Jarno Rajahalme <jrajahalme at nicira.com>

This makes the formatting and parsing code less disastery.  Thank you.

"sparse" says:

    ../lib/odp-util.c:2379:9: warning: incorrect type in argument 2 (different base types)
    ../lib/odp-util.c:2379:9:    expected restricted ovs_be32 [usertype] ( *key )[4]
    ../lib/odp-util.c:2379:9:    got unsigned int [usertype] ( *<noident> )[4]
    ../lib/odp-util.c:2379:9: warning: incorrect type in argument 3 (different base types)
    ../lib/odp-util.c:2379:9:    expected restricted ovs_be32 [usertype] ( *mask )[4]
    ../lib/odp-util.c:2379:9:    got unsigned int [usertype] ( * )[4]
    ../lib/odp-util.c:1635:35: warning: incorrect type in argument 3 (different base types)
    ../lib/odp-util.c:1635:35:    expected restricted ovs_be32 const [usertype] *key_
    ../lib/odp-util.c:1635:35:    got unsigned int const *<noident>
    ../lib/odp-util.c:1635:51: warning: incorrect type in argument 4 (different base types)
    ../lib/odp-util.c:1635:51:    expected restricted ovs_be32 const [usertype] ( *mask_ )[4]
    ../lib/odp-util.c:1635:51:    got unsigned int const [usertype] ( * )[4]


There's still some nastiness around the difference between an
all-one-bits mask and a null mask.  It takes some real care to read
the code to see that it is correct, and I'm sure that it took at least
as much care to write it.  Can we do something about that?  One way
would be to always supply a mask, one that is all-one-bits if there
would otherwise be no mask.  That could be done externally to the
format_*() functions, or it could be internally.

Here's one approach for the internal version:

    static void
    format_eth(struct ds *ds, const char *name, const uint8_t key[ETH_ADDR_LEN],
               const uint8_t (*mask_)[ETH_ADDR_LEN], bool verbose)
    {
        const uint8_t *mask = mask_ ? *mask_ : eth_addr_broadcast;
        if (verbose || !eth_addr_is_zero(key) || !eth_addr_is_zero(mask)) {
            if (!eth_mask_is_exact(mask)) { /* Partially masked. */
                ds_put_format(ds, "%s=", name);
                eth_format_masked(key, mask, ds);
                ds_put_char(ds, ',');
            } else { /* Fully masked. */
                ds_put_format(ds, "%s="ETH_ADDR_FMT",", name, ETH_ADDR_ARGS(key));
            }
        }
    }

The external version would be more like:

    const uint8_t big_all_one_bits[] = { [ 0...255 ] = 0xff };
    #define MASK(PTR, FIELD) \
            (OVS_TYPEOF(&PTR->FIELD)) (PTR ? &PTR->FIELD : (const void *) big_all_one_bits)

though I see some defects in that already.

In format_ipv6_label(), I personally find that
    if (mask && (~*mask & htonl(IPV6_LABEL_MASK))) { /* Partially masked. */
is easier to understand written as:
    if (mask && (*mask & htonl(~IPV6_LABEL_MASK))) { /* Partially masked. */
or
    if (mask && (*mask & ~htonl(IPV6_LABEL_MASK))) { /* Partially masked. */
Similarly format_tun_flags().

In format_u8x(), I personally find that:
        if (mask && ~*mask & UINT8_MAX) { /* Partially masked. */
is easier to understand written as:
        if (mask && *mask != UINT8_MAX) { /* Partially masked. */
Similarly in format_u8u(), format_be16(), format_frag().

format_frag() is kind of funny because masking doesn't really make
sense there; individual bits aren't too meaningful.  I guess what
you've done is about as good as we can get.

There may be some common ground with mf_format() and mf_parse() in
lib/meta-flow.c.

On scan_eth(), the 's' parameter is marked OVS_UNUSED, though it is
used.

I don't recommend marking scan_tcp_flags() inline, because that will
suppress a "function unused" warning if it ever becomes unused.

scan_tcp_flags() should probably pass NULL as the mask to
parse_flags() if its own 'mask' argument is NULL.

In *_bf(), what does "bf" stand for?

I only made it as far as the SCAN_PUT_ATTR macro.  I'll continue the
review from there tomorrow.

Thanks,

Ben.



More information about the dev mailing list