Re: [Xen-devel] [PATCH V3] xl: create VFB for PV guest when VNC is specified

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Wei Liu writes ("Re: [Xen-devel] [PATCH V3] xl: create VFB for PV guest when 
VNC is specified"):
> On Tue, Dec 17, 2013 at 04:40:46PM +0000, Ian Campbell wrote:
> > There's also an argument that this should be functionality provided in
> > helpers generated by the IDL, but that certainly isn't 4.4 material...
> 
> OK. Now it looks like this.
> 
> #define ARRAY_EXTEND_INIT(ptr,count,name)                               \
>     ({                                                                  \
>         typeof((ptr)) *_xl_array_ptr = &(ptr);                          \
>         typeof((count)) *_xl_array_count = &(count);                    \
>         typeof((count)) _xl_array_count_saved = *_xl_array_count;       \
>         *_xl_array_ptr = xrealloc(*_xl_array_ptr,                       \
>                                   sizeof(**_xl_array_ptr) *             \
>                                   (*_xl_array_count + 1));              \
>         (*_xl_array_count)++;                                           \
>         libxl_device_##name##_init(*_xl_array_ptr + _xl_array_count_saved); \
>         (*_xl_array_ptr + _xl_array_count_saved);                       \
>     })
> 
> It 1) only evaluates its arguments once, 2) initializes the new element
> with corresponding function, 3) returns the new element.

I still think the bikeshed is the wrong colour and the result is very
ugly, but this will do.

Acked-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>

Here's my real opinion:

If you are going to only use "name" once I don't think the token
pasting is justified.  It would be justified if you used "name" to
automatically compute the right expression for ptr and count, but I
can see why you didn't want to do this.

> Longer local variables names are chosen in the hope that they don't
> clash with other names...

I realise you're copying what other people have done, but there is a
lot of cargo cult superstition about what needs to be, and what
doesn't need to be, in these kind of macro local variable names.

The leading _ is pretty pointless.  The "xl" is pointless too because
these are inside the code for xl, so only other code inside xl will
see them.

I would call these variables  array_extend_ptr  et al.

As I've said I think evaluating the arguments multiple times is
better.  After all they have to be lvalues.

So I would do:

 #define ARRAY_EXTEND_INIT(array,count,initfn)                       \
     ({                                                              \
         typeof((count)) array_extend_old_count = (count);           \
         (count)++;                                                  \
         (array) = xrealloc((array), sizeof(*array) * (count));      \
         (initfn)(&(array)[array_extend_old_count]);                 \
         &(array)[array_extend_old_count];                           \
     })

 #define ADD_VDEV(container,things,initfn)                \
     ARRAY_EXTEND_INIT((container)->things,               \
                       (container)->num_##things,         \
                       (initfn))
 ...

     vtpm = ADD_VDEV(d_config, vtpms, libxl_device_vtpm_init);
     vtpm->devid = d_config->num_vtpms;

(not even compiled by me, obviously)

Sadly you can't generate "init" from "kind" because
libxl_device_pci_init but d_config->pcidevs.

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel