On Wed, 8 Jul 2015 14:30:29 +1000 David Gibson <da...@gibson.dropbear.id.au> wrote:
> On Tue, Jul 07, 2015 at 09:05:02PM +1000, Alexey Kardashevskiy wrote: > > On 07/07/2015 08:21 PM, Thomas Huth wrote: > > >On Tue, 7 Jul 2015 20:05:25 +1000 > > >Alexey Kardashevskiy <a...@ozlabs.ru> wrote: > > > > > >>On 07/07/2015 05:23 PM, Thomas Huth wrote: > > >>>On Mon, 6 Jul 2015 12:11:09 +1000 > > >>>Alexey Kardashevskiy <a...@ozlabs.ru> wrote: ... > > >>>>@@ -698,14 +768,18 @@ static int vfio_connect_container(VFIOGroup > > >>>>*group, AddressSpace *as) > > >>>> > > >>>> container->iommu_data.type1.initialized = true; > > >>>> > > >>>>- } else if (ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_SPAPR_TCE_IOMMU)) { > > >>>>+ } else if (ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_SPAPR_TCE_IOMMU) || > > >>>>+ ioctl(fd, VFIO_CHECK_EXTENSION, > > >>>>VFIO_SPAPR_TCE_v2_IOMMU)) { > > >>>>+ bool v2 = !!ioctl(fd, VFIO_CHECK_EXTENSION, > > >>>>VFIO_SPAPR_TCE_v2_IOMMU); > > >>> > > >>>That "!!" sounds somewhat wrong here. I think you either want to check > > >>>for "ioctl() == 1" (because only in this case you can be sure that v2 > > >>>is supported), or you can simply omit the "!!" because you're 100% sure > > >>>that the ioctl only returns 0 or 1 (and never a negative error code). > > >> > > >> > > >>The host kernel does not return an error on these ioctls, it returns 0 or > > >>1. And "!!" is shorter than "(bool)". VFIO_CHECK_EXTENSION for Type1 does > > >>exactly the same already. > > > > > >Simply using nothing instead is even shorter than using "!!". The > > >compiler is smart enough to convert from 0 and 1 to bool. > > >"!!" is IMHO quite ugly and should only be used when it is really > > >necessary. > > > > > > imho it is not but either way I'd rather follow the existing style, > > especially if I do literally the same thing (checking IOMMU version). Unless > > the original author tells me to convert all the existing occurences of "!!" > > to "!=0" (or something like this) before I post new ones. > > > > Alex, should I get rid of "!!"s in the patch? > > I think !! is the lesser evil here. The trouble is that in C "bool" > is not a first-class datatype, but just a typedef for some integer > type. Which means that, confusingly, (bool)2 != (bool)1. So using > the !! trick to force a value to be either 0 or 1 when assigning it to > a bool variable is probably a good idea. That was maybe the case > 15 years ago, but since C99, there is a proper bool type in C, as far as I know. But I am also not an expert here... However, I tried the following small test program: #include <stdio.h> #include <stdbool.h> int main() { bool a = 1; bool b = 2; printf("a=%i b=%i\n", a, b); return 0; } ... and indeed, it prints out "a=1 b=1" here, so the "2" got properly changed to "true" :-) Anyway, that was already too much bike-shed painting now, if you want to keep the "!!", then keep it, that's fine for me, too. Thomas
pgpQGiKV6IRAG.pgp
Description: OpenPGP digital signature