From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51541) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bowkV-0000Uv-Kp for qemu-devel@nongnu.org; Tue, 27 Sep 2016 14:05:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bowkQ-0000NU-NI for qemu-devel@nongnu.org; Tue, 27 Sep 2016 14:04:58 -0400 Received: from mx1.redhat.com ([209.132.183.28]:36304) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bowkQ-0000ND-Cz for qemu-devel@nongnu.org; Tue, 27 Sep 2016 14:04:54 -0400 Date: Tue, 27 Sep 2016 19:04:48 +0100 From: "Daniel P. Berrange" Message-ID: <20160927180448.GU3967@redhat.com> Reply-To: "Daniel P. Berrange" References: <1474994958-6007-1-git-send-email-felipe@nutanix.com> <20160927172323.GS3967@redhat.com> <90EEF36E-117B-4103-B078-1F039A632298@nutanix.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <90EEF36E-117B-4103-B078-1F039A632298@nutanix.com> Subject: Re: [Qemu-devel] [PATCH] io: Fix double shift usages on QIOChannel features List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Felipe Franciosi Cc: Marc-Andre Lureau , Paolo Bonzini , QEMU Developers 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 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/ :|