On Sep 8, 2014, at 5:06 PM, Ben Pfaff <[email protected]> wrote:
> 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 <[email protected]>
>
> This makes the formatting and parsing code less disastery. Thank you.
>
(snip)
> 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.
>
How about this variation (I realized that ‘verbose’ covers the case when the
key includes non-masked bits):
diff --git a/lib/odp-util.c b/lib/odp-util.c
index b5c8c2b..ea724da 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -1251,14 +1251,17 @@ 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)
{
- if (verbose || !eth_addr_is_zero(key)
- || !mask || !eth_addr_is_zero(*mask)) {
- if (mask && !eth_mask_is_exact(*mask)) { /* Partially masked. */
+ bool mask_empty = mask && eth_addr_is_zero(*mask);
+
+ if (verbose || !mask_empty) {
+ bool mask_full = !mask || eth_mask_is_exact(*mask);
+
+ if (mask_full) {
+ ds_put_format(ds, "%s="ETH_ADDR_FMT",", name, ETH_ADDR_ARGS(key));
+ } else {
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));
}
}
}
(I’ll wait for your response before changing them all…)
> 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().
>
Will do.
> 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.
>
I learned about that later after I had done that. Since an enum value is not
partially maskable we should always print it without a mask (or not at all when
the mask is zero).
> There may be some common ground with mf_format() and mf_parse() in
> lib/meta-flow.c.
>
I’ll have a look.
> 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.
>
These are remnants from debugging. I actually used the inline keyword to
suppress the “function unused” warning!
> scan_tcp_flags() should probably pass NULL as the mask to
> parse_flags() if its own 'mask' argument is NULL.
>
Will check.
> In *_bf(), what does "bf" stand for?
>
“bitfield”, did not want to make the name longer…
>
> I noticed that a lot of the macros could better protect their
> arguments, e.g. in the definition of SCAN_PUT_ATTR, parentheses around
> ATTR here:
> + if (ATTR == OVS_KEY_ATTR_TUNNEL) { \
> and around DATA here:
> + tun_key_to_attr(BUF, (const struct flow_tnl *)(void *)&DATA); \
> + } else { \
> and ATTR and DATA (twice) here:
> + nl_msg_put_unspec(BUF, ATTR, &DATA, sizeof DATA); \
> + }
>
Will fix.
> I am not sure whether this is the approach that I would have ended up
> with, but whether yes or no, it is a huge improvement in
> maintainability. Thank you.
>
Thanks for the review. I’ll wait for your response on the first point before
pushing this.
Jarno
> Acked-by: Ben Pfaff <[email protected]>
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
