Re: [Qemu-ppc] [Qemu-devel] [PATCH qemu v10 13/14] vfio: spapr: Add SPAPR IOMMU v2 support (DMA memory preregistering)

[Thomas Huth]

On Wed, 8 Jul 2015 14:30:29 +1000
David Gibson <address@hidden> 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 <address@hidden> wrote:
> > >
> > >>On 07/07/2015 05:23 PM, Thomas Huth wrote:
> > >>>On Mon,  6 Jul 2015 12:11:09 +1000
> > >>>Alexey Kardashevskiy <address@hidden> 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

pgpxEdzLHzxx4.pgp
Description: OpenPGP digital signature