Re: [Qemu-devel] [PATCH] pc: reduce duplication, fix PIIX descriptions

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Aug 27, 2013 at 09:29:33AM +0200, Paolo Bonzini wrote:
> Il 27/08/2013 09:01, Michael S. Tsirkin ha scritto:
> > We have a lot of code duplication between machine types,
> > this increases with each new machine type
> > and each new field.
> > 
> > This has already introduced a minor bug: description
> > for pc-1.3 says "Standard PC" while description for
> > pc-1.4 is "Standard PC (i440FX + PIIX, 1996)"
> > which makes you think 1.3 is somehow more standard,
> > or newer, while in fact it's a revision of the same PC.
> > 
> > This patch addresses this issue by using macros, along
> > the lines used by PC_COMPAT_X_X - only for
> > non-property options.
> > 
> > Note: this conflicts (trivially) with patch
> >     [PATCH v3 6/6] hw: Clean up bogus default boot order
> > by Markus. Sent separately for ease of review.  If people like this
> > approach, I can rebase on top of that patch.
> > 
> > The approach can extend to non-PC machine types.
> > 
> > Cc: Markus Armbruster <armbru@redhat.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  static QEMUMachine isapc_machine = {
> > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> > index 198c785..084ecac 100644
> > --- a/hw/i386/pc_q35.c
> > +++ b/hw/i386/pc_q35.c
> > @@ -253,39 +253,41 @@ static void pc_q35_init_1_4(QEMUMachineInitArgs *args)
> >      pc_q35_init(args);
> >  }
> >  
> > +#define PC_Q35_MACHINE_OPTIONS \
> > +    PC_DEFAULT_MACHINE_OPTIONS, \
> > +    .hot_add_cpu = pc_hot_add_cpu
> 
> .desc is missing, right?
> 
> Otherwise looks good.
> 
> Paolo

Good catch, I'll fix that up.

> > +
> > +#define PC_Q35_1_6_MACHINE_OPTIONS PC_Q35_MACHINE_OPTIONS
> >  static QEMUMachine pc_q35_machine_v1_6 = {
> > +    PC_Q35_1_6_MACHINE_OPTIONS,
> >      .name = "pc-q35-1.6",
> >      .alias = "q35",
> > -    .desc = "Standard PC (Q35 + ICH9, 2009)",
> >      .init = pc_q35_init_1_6,
> > -    .hot_add_cpu = pc_hot_add_cpu,
> > -    .max_cpus = 255,
> > -    DEFAULT_MACHINE_OPTIONS,
> >  };
> >  
> >  static QEMUMachine pc_q35_machine_v1_5 = {
> > +    PC_Q35_1_6_MACHINE_OPTIONS,
> >      .name = "pc-q35-1.5",
> > -    .desc = "Standard PC (Q35 + ICH9, 2009)",
> >      .init = pc_q35_init_1_5,
> > -    .hot_add_cpu = pc_hot_add_cpu,
> > -    .max_cpus = 255,
> >      .compat_props = (GlobalProperty[]) {
> >          PC_COMPAT_1_5,
> >          { /* end of list */ }
> >      },
> > -    DEFAULT_MACHINE_OPTIONS,
> >  };
> >  
> > +#define PC_Q35_1_4_MACHINE_OPTIONS \
> > +    PC_Q35_1_6_MACHINE_OPTIONS, \
> > +    .hot_add_cpu = NULL
> > +
> >  static QEMUMachine pc_q35_machine_v1_4 = {
> > +    PC_Q35_1_4_MACHINE_OPTIONS,
> >      .name = "pc-q35-1.4",
> > -    .desc = "Standard PC (Q35 + ICH9, 2009)",
> >      .init = pc_q35_init_1_4,
> > -    .max_cpus = 255,
> >      .compat_props = (GlobalProperty[]) {
> >          PC_COMPAT_1_4,
> >          { /* end of list */ }
> >      },
> > -    DEFAULT_MACHINE_OPTIONS,
> >  };
> >  
> >  static void pc_q35_machine_init(void)
> > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > index 475ba9e..4a38fad 100644
> > --- a/include/hw/i386/pc.h
> > +++ b/include/hw/i386/pc.h
> > @@ -325,4 +325,9 @@ int e820_add_entry(uint64_t, uint64_t, uint32_t);
> >              .value    = stringify(0),\
> >          }
> >  
> > +#define PC_DEFAULT_MACHINE_OPTIONS \
> > +    DEFAULT_MACHINE_OPTIONS, \
> > +    .hot_add_cpu = pc_hot_add_cpu, \
> > +    .max_cpus = 255
> > +
> >  #endif
> >