Re: [Qemu-devel] [PATCH] io: Fix double shift usages on QIOChannel features

[Felipe Franciosi]

Heya,

> On 27 Sep 2016, at 19:04, Daniel P. Berrange <address@hidden> wrote:
> 
> On Tue, Sep 27, 2016 at 05:57:12PM +0000, Felipe Franciosi wrote:
>> Hi Daniel,
>> 
>>> On 27 Sep 2016, at 18:23, Daniel P. Berrange <address@hidden> wrote:
>>> 
>>> On Tue, Sep 27, 2016 at 09:49:18AM -0700, Felipe Franciosi wrote:
>>>> When QIOChannels were introduced in 666a3af9, the feature bits were
>>>> defined shifted. However, when using them, the code was shifting them
>>>> again. The incorrect use was consistent until 74b6ce43, where
>>>> QIO_CHANNEL_FEATURE_LISTEN was defined shifted but tested unshifted.
>>> 
>>> I'm more inclined to actually change the header file, so that they
>>> are defined unshifted, and fix the single place that tests
>>> unshifted.  They are defined in an enum, so it was kind of odd to
>>> use shifted values in the first place.
>> 
>> It's not uncommon to specify shifted features/flags on enums:
>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/nl80211.h#n2661
>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/include/linux/nvme.h#n322
>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/include/linux/blk-mq.h#n194
> 
>> I actually prefer defining them shifted, as proposed in my patch.
>> And perhaps adding a qio_channel_set_feature() to completely
>> abstract their usage. But I don't have strong preferences towards
>> this and can change it if you really want me to.
> 
> I'd really prefer them to be defined unshifted, just using the
> enum default value assignment.

Ok I'll change it.

> 
>>>> This patch fixes all uses of QIOChannel features. They are defined
>>>> shifted and therefore set unshifted. It also makes all feature tests to
>>>> use the qio_channel_has_feature() function.
>>> 
>>> Switching to use of qio_channel_has_feature() is a useful, but
>>> independant fix, so should be a separate commit really.
>> 
>> Sure I can separate that in another patch. Should I also
>> add a qio_channel_set_feature()? I think that's a good idea.
> 
> Yep, a set feature helper sounds like a reasonable addition.

Sure I'll add it.

> 
>>> 
>>>> {
>>>> -    return ioc->features & (1 << feature);
>>>> +    return ioc->features & feature;
>>>> }
>>> 
>>> This is logically wrong - 'feature' can now contain multiple
>>> bits, but this is returning true if any single one of them
>>> is present, rather than if all are present. IMHO this is an
>>> example of why we should define them unshifted.
>> 
>> This looks correct to me. It's only wrong if we change
>> the definition to be unshifted, which I believe is still
>> on the table. :)
> 
> You've got it reversed - with the definitions shifted
> you can do
> 
>  qio_channel_has_feature(ioc, A | B)
> 
> and it'll return true, even if only A is set.

Then maybe I got it really wrong. If you write "has_feature(A or B)", I'd 
expect it to return true if you have features A or B. Not _only_ if you have 
both. For that, I'd call has_feature(A) && has_feature(B). Alternatively, I'd 
define a has_features(mask) which could take OR'd features (plural) and behave 
as you defined below.

> So if we
> were to keep the definitions shifted, then you'd actually
> need to have
> 
>  -    return ioc->features & (1 << feature);
>  +    (return ioc->features & feature) == feature;
> 
> but as above, I'd prefer to just have it unshifted.

Sure. As I said, I haven't got a strong preference.

Felipe

> 
> 
> Regards,
> Daniel
> -- 
> |: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
> |: http://libvirt.org              -o-             http://virt-manager.org :|
> |: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|