diff mbox

[v17,7/9] add MachineClass->default_props for setting default device properties

Message ID 1453208789-42479-8-git-send-email-imammedo@redhat.com
State New
Headers show

Commit Message

Igor Mammedov Jan. 19, 2016, 1:06 p.m. UTC 
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 include/hw/boards.h | 1 +
 vl.c                | 4 ++++
 2 files changed, 5 insertions(+)

Comments

Eduardo Habkost Jan. 23, 2016, 2:59 p.m. UTC | #1 
On Tue, Jan 19, 2016 at 02:06:27PM +0100, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  include/hw/boards.h | 1 +
>  vl.c                | 4 ++++
>  2 files changed, 5 insertions(+)
> 
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 0f30959..d495611 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -90,6 +90,7 @@ struct MachineClass {
>      const char *default_machine_opts;
>      const char *default_boot_order;
>      const char *default_display;
> +    GlobalProperty *default_props;
>      GlobalProperty *compat_props;

Could you explain (in a comment?) the purpose of each field? They
seem to do exactly the same thing, so why couldn't they become a
single linked list, where the compat classes just append new
items to the existing default_props list?

(If we build default_props by appending instead of overwriting
the parent class list, we will be able to finally eliminate
PC_COMPAT_* macro nesting)
Igor Mammedov Jan. 26, 2016, 10:28 a.m. UTC | #2 
On Sat, 23 Jan 2016 12:59:56 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Tue, Jan 19, 2016 at 02:06:27PM +0100, Igor Mammedov wrote:
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  include/hw/boards.h | 1 +
> >  vl.c                | 4 ++++
> >  2 files changed, 5 insertions(+)
> > 
> > diff --git a/include/hw/boards.h b/include/hw/boards.h
> > index 0f30959..d495611 100644
> > --- a/include/hw/boards.h
> > +++ b/include/hw/boards.h
> > @@ -90,6 +90,7 @@ struct MachineClass {
> >      const char *default_machine_opts;
> >      const char *default_boot_order;
> >      const char *default_display;
> > +    GlobalProperty *default_props;
> >      GlobalProperty *compat_props;  
> 
> Could you explain (in a comment?) the purpose of each field? They
> seem to do exactly the same thing, so why couldn't they become a
> single linked list, where the compat classes just append new
> items to the existing default_props list?
> 
> (If we build default_props by appending instead of overwriting
> the parent class list, we will be able to finally eliminate
> PC_COMPAT_* macro nesting)
The only reason I've added it as separate field is to keep the
current way compat_props are working instead of rewriting
not related to this series part.

Alternatively we could add qdev_prop_prepend_global_list() API
and add static defaults calling it from board's machine-init.
Eduardo Habkost Jan. 26, 2016, 2:12 p.m. UTC | #3 
On Tue, Jan 26, 2016 at 11:28:01AM +0100, Igor Mammedov wrote:
> On Sat, 23 Jan 2016 12:59:56 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Tue, Jan 19, 2016 at 02:06:27PM +0100, Igor Mammedov wrote:
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > >  include/hw/boards.h | 1 +
> > >  vl.c                | 4 ++++
> > >  2 files changed, 5 insertions(+)
> > > 
> > > diff --git a/include/hw/boards.h b/include/hw/boards.h
> > > index 0f30959..d495611 100644
> > > --- a/include/hw/boards.h
> > > +++ b/include/hw/boards.h
> > > @@ -90,6 +90,7 @@ struct MachineClass {
> > >      const char *default_machine_opts;
> > >      const char *default_boot_order;
> > >      const char *default_display;
> > > +    GlobalProperty *default_props;
> > >      GlobalProperty *compat_props;  
> > 
> > Could you explain (in a comment?) the purpose of each field? They
> > seem to do exactly the same thing, so why couldn't they become a
> > single linked list, where the compat classes just append new
> > items to the existing default_props list?
> > 
> > (If we build default_props by appending instead of overwriting
> > the parent class list, we will be able to finally eliminate
> > PC_COMPAT_* macro nesting)
> The only reason I've added it as separate field is to keep the
> current way compat_props are working instead of rewriting
> not related to this series part.
> 

OK to me, but I would like to have the reason for having two very
similar fields documented in struct MachineClass.

> Alternatively we could add qdev_prop_prepend_global_list() API
> and add static defaults calling it from board's machine-init.

If we can choose between having stuff in MachineClass and adding
extra machine-init code, I prefer to have it in MachineClass.
diff mbox

Patch

diff --git a/include/hw/boards.h b/include/hw/boards.h
index 0f30959..d495611 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -90,6 +90,7 @@  struct MachineClass {
     const char *default_machine_opts;
     const char *default_boot_order;
     const char *default_display;
+    GlobalProperty *default_props;
     GlobalProperty *compat_props;
     const char *hw_version;
     ram_addr_t default_ram_size;
diff --git a/vl.c b/vl.c
index b7a083e..0db74b1 100644
--- a/vl.c
+++ b/vl.c
@@ -4492,6 +4492,10 @@  int main(int argc, char **argv, char **envp)
             exit (i == 1 ? 1 : 0);
     }
 
+    if (machine_class->default_props) {
+        qdev_prop_register_global_list(machine_class->default_props);
+    }
+
     if (machine_class->compat_props) {
         qdev_prop_register_global_list(machine_class->compat_props);
     }