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

["Daniel P. Berrange" <berrange@redhat.com>]

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 <berrange@redhat.com> 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.

> >> 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.

> > 
> >> {
> >> -    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. 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.


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/ :|