On Wed, Jun 24, 2015 at 11:38 AM, Ben Pfaff <b...@nicira.com> wrote:
> On Fri, Jun 19, 2015 at 04:13:17PM -0700, Jesse Gross wrote:
>> This adds support for receiving variable length fields encoded in
>> NXM/OXM and mapping them into OVS internal structures. In order
>> for this to make sense, we need to define some semantics:
>>
>> There are three lengths that matter in this process: the maximum
>> size of the field (represented as the existing mf->n_bytes), the
>> size of the field in the incoming NXM (given by the length in the
>> NXM header), and the currently configured length of the field
>> (defined by the consumer of the field and outside the scope of
>> this patch).
>>
>> Fields are modeled as being their maximum length and have the
>> characteristics expected by exsiting code (i.e. exact match fields
>> have masks that are all 1's for the whole field, etc.). Incoming
>> NXMs are stored in the field in the least significant bits. If
>> the NXM length is larger than the field, is is truncated, if it
>> is smaller it is zero-extended. When the field is consumed, the
>> component that needs data picks the configured length out of the
>> generated field.
>>
>> In most cases, the configured and NXM lengths will be equal and
>> these edge cases do not matter. However, since we cannot easily
>> enforce that the lengths match (and might not even know what the
>> right length is, such as in the case of a string parsed by
>> ovs-ofctl), these semantics should provide deterministic results
>> that are easy to understand and not require most existing code
>> to be aware of variable length fields.
>>
>> Signed-off-by: Jesse Gross <je...@nicira.com>
>
> I think that this changes behavior in a corner case. Previously, if
> nx_pull_entry__() were passed a null 'field' parameter, then
> nx_pull_header__() would not check for a field at all and would never
> return OFPERR_OFPBMC_BAD_FIELD. Now, nx_pull_header__() always checks
> for a field, and although nx_pull_entry__() initially ignores
> OFPERR_OFPBMC_BAD_FIELD, it still eventually passes that error along to
> its caller where it previously returned 0.
>
> I don't know whether that's a big deal but I do recall trying to get
> this exactly right before. The fix seems simple, just change the last
> bit of the function to:
> if (field_) {
> *field_ = field;
> return header_error;
> }
> return 0;
Thanks for noticing that - the intention was to keep the behavior the
same for non-variable length fields. I took your suggested code.
> There's cut-and-paste copy code in this function, maybe a helper would
> be nice.
Agreed; it does look much nicer this way.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
