Thanks. To make this invariant really clear, I added a comment:
*
* (As usually true for value/mask pairs in OVS, any 1-bit in a value must have
* a corresponding 1-bit in its mask.) */
On Wed, Apr 15, 2015 at 12:36:40PM -0700, Andy Zhou wrote:
> Thanks for explaining. Given av == av & am is a prerequisite, this
> function looks fine.
>
> For the patch:
> Acked-by: Andy Zhou <az...@nicira.com>
>
> On Wed, Apr 15, 2015 at 11:35 AM, Ben Pfaff <b...@nicira.com> wrote:
> > This combination of av with am is invalid because av has a 1-bit (its
> > least-significant bit) where am has a 0-bit. Suppose that we pretend
> > that am is instead 1001, which would make av/am valid. Then av/am
> > requires the least-significant bit to be 1 and bv/bm requires it to be
> > 0. This is the "impossible" case described in the function-level
> > comment, which makes the function return false because:
> >
> > (av ^ bv) & (am & bm)
> > == (1001 ^ 1000) & (1001 & 1001)
> > == 0001 & 1001
> > == 1
> >
> > which is nonzero, hence the if statement returns false.
> >
> > On Tue, Apr 07, 2015 at 02:14:31PM -0700, Andy Zhou wrote:
> >> assume av=1001, am = 1000, bv=1000, bm=1001.
> >>
> >> Should dv's last bit be zero? if not, then my interpretation of what
> >> this function does
> >> is wrong.
> >>
> >> On Mon, Apr 6, 2015 at 9:31 PM, Ben Pfaff <b...@nicira.com> wrote:
> >> > On Mon, Apr 06, 2015 at 03:28:37PM -0700, Andy Zhou wrote:
> >> >> On Tue, Mar 31, 2015 at 9:52 PM, Ben Pfaff <b...@nicira.com> wrote:
> >> >> > To be first used in upcoming commits.
> >> >> >
> >> >> > Signed-off-by: Ben Pfaff <b...@nicira.com>
> >> >>
> >> >> > +/* Consider the two value/mask pairs 'a_value/a_mask' and
> >> >> > 'b_value/b_mask' as
> >> >> > + * restrictions on a field's value. Then, this function initializes
> >> >> > + * 'dst_value/dst_mask' such that it combines the restrictions of
> >> >> > both pairs.
> >> >> > + * This is not always possible, i.e. if one pair insists on a value
> >> >> > of 0 in
> >> >> > + * some bit and the other pair insists on a value of 1 in that bit.
> >> >> > This
> >> >> > + * function returns false in a case where the combined restriction is
> >> >> > + * impossible (in which case 'dst_value/dst_mask' is not fully
> >> >> > initialized),
> >> >> > + * true otherwise. */
> >> >> > +bool
> >> >> > +mf_subvalue_intersect(const union mf_subvalue *a_value,
> >> >> > + const union mf_subvalue *a_mask,
> >> >> > + const union mf_subvalue *b_value,
> >> >> > + const union mf_subvalue *b_mask,
> >> >> > + union mf_subvalue *dst_value,
> >> >> > + union mf_subvalue *dst_mask)
> >> >> > +{
> >> >> > + for (int i = 0; i < ARRAY_SIZE(a_value->be64); i++) {
> >> >> > + ovs_be64 av = a_value->be64[i];
> >> >> > + ovs_be64 am = a_mask->be64[i];
> >> >> > + ovs_be64 bv = b_value->be64[i];
> >> >> > + ovs_be64 bm = b_mask->be64[i];
> >> >> > + ovs_be64 *dv = &dst_value->be64[i];
> >> >> > + ovs_be64 *dm = &dst_mask->be64[i];
> >> >> > +
> >> >> > + if ((av ^ bv) & (am & bm)) {
> >> >> > + return false;
> >> >> > + }
> >> >> > + *dv = av | bv;
> >> >> > + *dm = am | bm;
> >> >> Should this be *dv = (av & am) | (bv & bm)? I have not read the
> >> >> following patch
> >> >> to check the actual usage, so my interpretation of 'intersect' may be
> >> >> different than
> >> >> what's intended here.
> >> >
> >> > I think that in the cases where the expression you suggest would have a
> >> > different value, the "if" statement just before would have bailed out.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
