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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3647 bytes --]

On Wed, Jul 08, 2015 at 08:24:56AM +0200, Thomas Huth wrote:
> On Wed, 8 Jul 2015 14:30:29 +1000
> David Gibson <david@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 <aik@ozlabs.ru> wrote:
> > > >
> > > >>On 07/07/2015 05:23 PM, Thomas Huth wrote:
> > > >>>On Mon,  6 Jul 2015 12:11:09 +1000
> > > >>>Alexey Kardashevskiy <aik@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" :-)

Huh.  I had thought that C99 merely required that there be the
stdbool.h header declaring the bool type, rather than defining it as a
true first class type.

I'm very glad to be wrong.

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

But bike-shedding is a qemu tradition! ;-/

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]