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
