Re: [PATCH 01/14] include/hw: add helpers for defining versioned machine types

[Daniel P . Berrangé]

On Thu, May 02, 2024 at 09:57:21AM -0500, Eric Blake wrote:
> On Wed, May 01, 2024 at 07:27:46PM +0100, Daniel P. Berrangé wrote:
> > The various targets which define versioned machine types have
> > a bunch of obfuscated macro code for defining unique function
> > and variable names using string concatenation.
> > 
> > This addes a couple of helpers to improve the clarity of such
> > code macro.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  include/hw/boards.h | 166 ++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 166 insertions(+)
> > 
> > diff --git a/include/hw/boards.h b/include/hw/boards.h
> > index 2fa800f11a..47ca450fca 100644
> > --- a/include/hw/boards.h
> > +++ b/include/hw/boards.h
> > @@ -414,6 +414,172 @@ struct MachineState {
> >      struct NumaState *numa_state;
> >  };
> >  
> > +/*
> > + * The macros which follow are intended to facilitate the
> > + * definition of versioned machine types, using a somewhat
> > + * similar pattern across targets:
> > + *
> > + *  #define DEFINE_VIRT_MACHINE_IMPL(latest, ...) \
> > + *      static void MACHINE_VER_SYM(class_init, virt, __VA_ARGS__)( \
> > + *          ObjectClass *oc, \
> > + *          void *data) \
> > + *      { \
> > + *          MachineClass *mc = MACHINE_CLASS(oc); \
> > + *          MACHINE_VER_SYM(options, virt, __VA_ARGS__)(mc); \
> 
> Nice to include example usage of the macros.  __VA_ARGS__ is getting
> expanded quite a few times here, but I don't see that as being too
> much of a problem.
> 
> > + *  // Normal 2 digit eg 'virt-9.0'
> > + *  #define DEFINE_VIRT_MACHINE(major, minor) \
> > + *      DEFINE_VIRT_MACHINE_IMPL(false, major, minor)
> > + *
> > + *  // Bugfix 3 digit  eg 'virt-9.0.1'
> 
> Inconsistent on whether you are using one or two spaces before 'eg'.

Will fix

> 
> > +
> > +#define _MACHINE_VER_PICK(x1, x2, x3, x4, x5, x6, ...) x6
> 
> This helper macro is powerful; it may be worth a short comment, maybe
> along the lines of:
> 
> /* Helper macro used to pick the right macro name based on the number
>  * of extra arguments passed to the containing macro; usage:
>  *
>  *  _MACHINE_VER_PICK(__VA_ARGS__, \
>  *                    MACRO_FOR_5_ARGS, \
>  *                    MACRO_FOR_4_ARGS, \
>  *                    MACRO_FOR_3_ARGS, \
>  *                    MACRO_FOR_2_ARGS)(optional prefix args, __VA_ARGS__)
>  */

Yes, will add something like this

> 
> But once understood, I see it comes in handy in several places below.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.
> Virtualization:  qemu.org | libguestfs.org
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|