On Wed, Jul 15, 2015 at 11:35 AM, Ben Pfaff <b...@nicira.com> wrote:
> On Mon, Jul 13, 2015 at 02:53:56PM -0700, Jesse Gross wrote:
>> There are several implementations of functions that parse/format
>> flags and their binary representation. This factors them out into
>> common routines. In addition to reducing code, it also makes things
>> more consistent across different parts of OVS.
>>
>> Signed-off-by: Jesse Gross <je...@nicira.com>
>
> Thanks, I like reducing code size!
>
> This patch deletes a test and part of a test in ovs-ofctl.at. Why?
I'm not sure that these tests really make sense and arguably the
behavior there were checking wasn't even right in the first place. In
theory they are verifying that the correct OpenFlow version is
selected based on the fields provided (in this case the correct
version is "none") but it is being done using names that weren't
supposed to be public, though the old parsing code allowed them to
leak through. The new parsing code is enforcing the same invariants as
before but more carefully and now rejects these commands as a parse
error before it even gets to the OpenFlow layer that is supposed to be
exercised. The next patch makes this field valid anyways and verifies
the correct OpenFlow behavior, so it didn't seem like it made sense to
keep the test around.
> In parse_flags(), s/preceed/preceded/:
> *res_string = xasprintf("%s: %s must be preceed by '+' "
Fixed.
> parse_flags() could use a function-level comment.
That's a good idea. I added this:
/* Scans a string 's' of flags to determine their numerical value and
* returns the number of characters parsed using 'bit_to_string' to
* lookup flag names. Scanning continues until the character 'end' is
* reached.
*
* In the event of a failure, a negative error code will be returned. In
* addition, if 'res_string' is non-NULL then a descriptive string will
* be returned incorporating the identifying string 'field_name'. This
* error string must be freed by the caller.
*
* Upon success, the flag values will be stored in 'res_flags' and
* optionally 'res_mask', if it is non-NULL (if it is NULL then any masks
* present in the original string will be considered an error). The
* caller may restrict the acceptable set of values through the mask
* 'allowed'. */
> format_flags_masked() will output the empty string if flags == 0 && mask
> == 0, and I think that parse_flags() will accept that too, but I don't
> know if it's the most user-friendly form. Don't know if that matters.
I agree, it's clearer and less ambiguous to output something in all
cases. I added this:
diff --git a/lib/flow.c b/lib/flow.c
index 0fc19d7..a13d832 100644
--- a/lib/flow.c
+++ b/lib/flow.c
@@ -843,6 +843,7 @@ format_flags(struct ds *ds, const char
*(*bit_to_string)(uint32_t),
uint32_t bad = 0;
if (!flags) {
+ ds_put_char(ds, '0');
return;
}
while (flags) {
@@ -875,12 +876,12 @@ format_flags_masked(struct ds *ds, const char *name,
}
if (mask == max_mask) {
- if (flags) {
- format_flags(ds, bit_to_string, flags, '|');
- } else {
- ds_put_char(ds, '0');
- }
+ format_flags(ds, bit_to_string, flags, '|');
+ return;
+ }
+ if (!mask) {
+ ds_put_cstr(ds, "0/0");
return;
}
> format_flags_masked() will output "0" if flags == 0 && mask == max_mask,
> but parse_flags() won't accept "0". It would be nice to resolve that.
I'm not sure if I understand correctly - parse_flags() should accept
both numbers as well as flag names either with or without masks. At
the top it scans for masked numeric values (which won't match in this
case) but in the second half of the function after it has looked for
masked flags, it tries again to look for both unmasked numbers and
names. I believe the unmasked numbers check should handle this
correctly.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
