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 <berra...@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/ :|
