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 <[email protected]> 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 <[email protected]> wrote:
> >> > To be first used in upcoming commits.
> >> >
> >> > Signed-off-by: Ben Pfaff <[email protected]>
> >>
> >> > +/* 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
[email protected]
http://openvswitch.org/mailman/listinfo/dev
