diff mbox series

[34/36] machine: Use DEFINE_PROP_STRING for string properties

Message ID 20201029220246.472693-35-ehabkost@redhat.com
State New
Headers show
Series Make qdev static property API usable by any QOM type | expand

Commit Message

Eduardo Habkost Oct. 29, 2020, 10:02 p.m. UTC 
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/core/machine.c | 166 ++++++----------------------------------------
 1 file changed, 19 insertions(+), 147 deletions(-)

Comments

Paolo Bonzini Oct. 30, 2020, 5:10 p.m. UTC | #1 
On 29/10/20 23:02, Eduardo Habkost wrote:
> +static Property machine_props[] = {
> +    DEFINE_PROP_STRING("kernel", MachineState, kernel_filename),
> +    DEFINE_PROP_STRING("initrd", MachineState, initrd_filename),
> +    DEFINE_PROP_STRING("append", MachineState, kernel_cmdline),
> +    DEFINE_PROP_STRING("dtb", MachineState, dtb),
> +    DEFINE_PROP_STRING("dumpdtb", MachineState, dumpdtb),
> +    DEFINE_PROP_STRING("dt-compatible", MachineState, dt_compatible),
> +    DEFINE_PROP_STRING("firmware", MachineState, firmware),
> +    DEFINE_PROP_STRING("memory-backend", MachineState, ram_memdev_id),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +

While I think generalizing the _code_ for static properties is obviously
a good idea, I am not sure about generalizing the interface for adding them.

The reason is that we already have a place for adding properties in
class_init, and having a second makes things "less local", so to speak.

What do you think about adding macros like

    OBJECT_CLASS_PROPERTY_ADD_STR(oc, MachineState, kernel_filename,
                                  "kernel", prop_allow_set_always);

?

Paolo
Eduardo Habkost Oct. 30, 2020, 8:03 p.m. UTC | #2 
On Fri, Oct 30, 2020 at 06:10:34PM +0100, Paolo Bonzini wrote:
> On 29/10/20 23:02, Eduardo Habkost wrote:
> > +static Property machine_props[] = {
> > +    DEFINE_PROP_STRING("kernel", MachineState, kernel_filename),
> > +    DEFINE_PROP_STRING("initrd", MachineState, initrd_filename),
> > +    DEFINE_PROP_STRING("append", MachineState, kernel_cmdline),
> > +    DEFINE_PROP_STRING("dtb", MachineState, dtb),
> > +    DEFINE_PROP_STRING("dumpdtb", MachineState, dumpdtb),
> > +    DEFINE_PROP_STRING("dt-compatible", MachineState, dt_compatible),
> > +    DEFINE_PROP_STRING("firmware", MachineState, firmware),
> > +    DEFINE_PROP_STRING("memory-backend", MachineState, ram_memdev_id),
> > +    DEFINE_PROP_END_OF_LIST(),
> > +};
> > +
> 
> While I think generalizing the _code_ for static properties is obviously
> a good idea, I am not sure about generalizing the interface for adding them.
> 
> The reason is that we already have a place for adding properties in
> class_init, and having a second makes things "less local", so to speak.
> 
> What do you think about adding macros like
> 
>     OBJECT_CLASS_PROPERTY_ADD_STR(oc, MachineState, kernel_filename,
>                                   "kernel", prop_allow_set_always);

I like the idea of having an interface like this, but I would
like to avoid having to write even more boilerplate for each
property type to make this work.

What would you think of:

   OBJECT_CLASS_PROPERTY_ADD(oc,
       PROP_STRING("kernel", MachineState, kernel_filename),
       prop_allow_set_always);

Then we could make the same PROP_STRING macro usable both as
object_class_property_add_static() argument and as initializer
for existing static Property arrays.
Paolo Bonzini Oct. 30, 2020, 8:41 p.m. UTC | #3 
Il ven 30 ott 2020, 21:03 Eduardo Habkost <ehabkost@redhat.com> ha scritto:

> >     OBJECT_CLASS_PROPERTY_ADD_STR(oc, MachineState, kernel_filename,
> >                                   "kernel", prop_allow_set_always);
>
> I like the idea of having an interface like this, but I would
> like to avoid having to write even more boilerplate for each
> property type to make this work.
>
> What would you think of:
>    OBJECT_CLASS_PROPERTY_ADD(oc,
>        PROP_STRING("kernel", MachineState, kernel_filename),
>        prop_allow_set_always);
>
> Then we could make the same PROP_STRING macro usable both as
> object_class_property_add_static() argument and as initializer
> for existing static Property arrays.
>

The name should be an argument to OBJECT_CLASS_PROPERTY_ADD though (which
could be a function and not  macro; perhaps
object_class_property_add_field?). PROP_STRING would be
DEFINE_PROP_STRING(NULL, etc.) and would not be entirely reusable in
Property arrays.

But even with that snag I agree with your less-boilerplate argument against
my proposal.

Since most if not all device properties would have to specify the same
allow-set function, we would end up defining more macros
DEVICE_CLASS_PROPERTY_ADD_STR, and so on. If the Property isbpassed a
struct, instead, we can define just one wrapper
device_class_property_add_field.

So what about:

1) add new constructors without the DEFINE prefix and without the name
argument

2) add object_class_property_add_field

And later:

3) add device_class_property_add_field and remove dc->props

4) remove the name field from Property.

Paolo


> --
> Eduardo
>
>
Eduardo Habkost Oct. 30, 2020, 9 p.m. UTC | #4 
On Fri, Oct 30, 2020 at 09:41:46PM +0100, Paolo Bonzini wrote:
> Il ven 30 ott 2020, 21:03 Eduardo Habkost <ehabkost@redhat.com> ha scritto:
> 
> > >     OBJECT_CLASS_PROPERTY_ADD_STR(oc, MachineState, kernel_filename,
> > >                                   "kernel", prop_allow_set_always);
> >
> > I like the idea of having an interface like this, but I would
> > like to avoid having to write even more boilerplate for each
> > property type to make this work.
> >
> > What would you think of:
> >    OBJECT_CLASS_PROPERTY_ADD(oc,
> >        PROP_STRING("kernel", MachineState, kernel_filename),
> >        prop_allow_set_always);
> >
> > Then we could make the same PROP_STRING macro usable both as
> > object_class_property_add_static() argument and as initializer
> > for existing static Property arrays.
> >
> 
> The name should be an argument to OBJECT_CLASS_PROPERTY_ADD though (which
> could be a function and not  macro; perhaps
> object_class_property_add_field?). PROP_STRING would be
> DEFINE_PROP_STRING(NULL, etc.) and would not be entirely reusable in
> Property arrays.
> 
> But even with that snag I agree with your less-boilerplate argument against
> my proposal.
> 
> Since most if not all device properties would have to specify the same
> allow-set function, we would end up defining more macros
> DEVICE_CLASS_PROPERTY_ADD_STR, and so on. If the Property isbpassed a
> struct, instead, we can define just one wrapper
> device_class_property_add_field.
> 
> So what about:
> 
> 1) add new constructors without the DEFINE prefix and without the name
> argument
> 
> 2) add object_class_property_add_field
> 
> And later:
> 
> 3) add device_class_property_add_field and remove dc->props
> 
> 4) remove the name field from Property.

Sounds good, and I like the "field property" name.  It is more
descriptive than "static property".
diff mbox series

Patch

diff --git a/hw/core/machine.c b/hw/core/machine.c
index c5e0e79e6d..97e102911a 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -27,6 +27,7 @@ 
 #include "hw/pci/pci.h"
 #include "hw/mem/nvdimm.h"
 #include "migration/vmstate.h"
+#include "qom/static-property.h"
 
 GlobalProperty hw_compat_5_1[] = {
     { "vhost-scsi", "num_queues", "1"},
@@ -211,81 +212,6 @@  GlobalProperty hw_compat_2_1[] = {
 };
 const size_t hw_compat_2_1_len = G_N_ELEMENTS(hw_compat_2_1);
 
-static char *machine_get_kernel(Object *obj, Error **errp)
-{
-    MachineState *ms = MACHINE(obj);
-
-    return g_strdup(ms->kernel_filename);
-}
-
-static void machine_set_kernel(Object *obj, const char *value, Error **errp)
-{
-    MachineState *ms = MACHINE(obj);
-
-    g_free(ms->kernel_filename);
-    ms->kernel_filename = g_strdup(value);
-}
-
-static char *machine_get_initrd(Object *obj, Error **errp)
-{
-    MachineState *ms = MACHINE(obj);
-
-    return g_strdup(ms->initrd_filename);
-}
-
-static void machine_set_initrd(Object *obj, const char *value, Error **errp)
-{
-    MachineState *ms = MACHINE(obj);
-
-    g_free(ms->initrd_filename);
-    ms->initrd_filename = g_strdup(value);
-}
-
-static char *machine_get_append(Object *obj, Error **errp)
-{
-    MachineState *ms = MACHINE(obj);
-
-    return g_strdup(ms->kernel_cmdline);
-}
-
-static void machine_set_append(Object *obj, const char *value, Error **errp)
-{
-    MachineState *ms = MACHINE(obj);
-
-    g_free(ms->kernel_cmdline);
-    ms->kernel_cmdline = g_strdup(value);
-}
-
-static char *machine_get_dtb(Object *obj, Error **errp)
-{
-    MachineState *ms = MACHINE(obj);
-
-    return g_strdup(ms->dtb);
-}
-
-static void machine_set_dtb(Object *obj, const char *value, Error **errp)
-{
-    MachineState *ms = MACHINE(obj);
-
-    g_free(ms->dtb);
-    ms->dtb = g_strdup(value);
-}
-
-static char *machine_get_dumpdtb(Object *obj, Error **errp)
-{
-    MachineState *ms = MACHINE(obj);
-
-    return g_strdup(ms->dumpdtb);
-}
-
-static void machine_set_dumpdtb(Object *obj, const char *value, Error **errp)
-{
-    MachineState *ms = MACHINE(obj);
-
-    g_free(ms->dumpdtb);
-    ms->dumpdtb = g_strdup(value);
-}
-
 static void machine_get_phandle_start(Object *obj, Visitor *v,
                                       const char *name, void *opaque,
                                       Error **errp)
@@ -310,21 +236,6 @@  static void machine_set_phandle_start(Object *obj, Visitor *v,
     ms->phandle_start = value;
 }
 
-static char *machine_get_dt_compatible(Object *obj, Error **errp)
-{
-    MachineState *ms = MACHINE(obj);
-
-    return g_strdup(ms->dt_compatible);
-}
-
-static void machine_set_dt_compatible(Object *obj, const char *value, Error **errp)
-{
-    MachineState *ms = MACHINE(obj);
-
-    g_free(ms->dt_compatible);
-    ms->dt_compatible = g_strdup(value);
-}
-
 static bool machine_get_dump_guest_core(Object *obj, Error **errp)
 {
     MachineState *ms = MACHINE(obj);
@@ -353,6 +264,18 @@  static void machine_set_mem_merge(Object *obj, bool value, Error **errp)
     ms->mem_merge = value;
 }
 
+static Property machine_props[] = {
+    DEFINE_PROP_STRING("kernel", MachineState, kernel_filename),
+    DEFINE_PROP_STRING("initrd", MachineState, initrd_filename),
+    DEFINE_PROP_STRING("append", MachineState, kernel_cmdline),
+    DEFINE_PROP_STRING("dtb", MachineState, dtb),
+    DEFINE_PROP_STRING("dumpdtb", MachineState, dumpdtb),
+    DEFINE_PROP_STRING("dt-compatible", MachineState, dt_compatible),
+    DEFINE_PROP_STRING("firmware", MachineState, firmware),
+    DEFINE_PROP_STRING("memory-backend", MachineState, ram_memdev_id),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static bool machine_get_usb(Object *obj, Error **errp)
 {
     MachineState *ms = MACHINE(obj);
@@ -382,21 +305,6 @@  static void machine_set_graphics(Object *obj, bool value, Error **errp)
     ms->enable_graphics = value;
 }
 
-static char *machine_get_firmware(Object *obj, Error **errp)
-{
-    MachineState *ms = MACHINE(obj);
-
-    return g_strdup(ms->firmware);
-}
-
-static void machine_set_firmware(Object *obj, const char *value, Error **errp)
-{
-    MachineState *ms = MACHINE(obj);
-
-    g_free(ms->firmware);
-    ms->firmware = g_strdup(value);
-}
-
 static void machine_set_suppress_vmdesc(Object *obj, bool value, Error **errp)
 {
     MachineState *ms = MACHINE(obj);
@@ -519,21 +427,6 @@  static void validate_sysbus_device(SysBusDevice *sbdev, void *opaque)
     }
 }
 
-static char *machine_get_memdev(Object *obj, Error **errp)
-{
-    MachineState *ms = MACHINE(obj);
-
-    return g_strdup(ms->ram_memdev_id);
-}
-
-static void machine_set_memdev(Object *obj, const char *value, Error **errp)
-{
-    MachineState *ms = MACHINE(obj);
-
-    g_free(ms->ram_memdev_id);
-    ms->ram_memdev_id = g_strdup(value);
-}
-
 
 static void machine_init_notify(Notifier *notifier, void *data)
 {
@@ -773,28 +666,20 @@  static void machine_class_init(ObjectClass *oc, void *data)
      */
     mc->numa_mem_align_shift = 23;
 
-    object_class_property_add_str(oc, "kernel",
-        machine_get_kernel, machine_set_kernel);
+    /*
+     * TODO: provide a allow_set callback and prevent properties
+     * from being set after machine was already initialized
+     */
+    object_class_add_static_props(oc, machine_props, NULL);
+
     object_class_property_set_description(oc, "kernel",
         "Linux kernel image file");
-
-    object_class_property_add_str(oc, "initrd",
-        machine_get_initrd, machine_set_initrd);
     object_class_property_set_description(oc, "initrd",
         "Linux initial ramdisk file");
-
-    object_class_property_add_str(oc, "append",
-        machine_get_append, machine_set_append);
     object_class_property_set_description(oc, "append",
         "Linux kernel command line");
-
-    object_class_property_add_str(oc, "dtb",
-        machine_get_dtb, machine_set_dtb);
     object_class_property_set_description(oc, "dtb",
         "Linux kernel device tree file");
-
-    object_class_property_add_str(oc, "dumpdtb",
-        machine_get_dumpdtb, machine_set_dumpdtb);
     object_class_property_set_description(oc, "dumpdtb",
         "Dump current dtb to a file and quit");
 
@@ -804,8 +689,6 @@  static void machine_class_init(ObjectClass *oc, void *data)
     object_class_property_set_description(oc, "phandle-start",
         "The first phandle ID we may generate dynamically");
 
-    object_class_property_add_str(oc, "dt-compatible",
-        machine_get_dt_compatible, machine_set_dt_compatible);
     object_class_property_set_description(oc, "dt-compatible",
         "Overrides the \"compatible\" property of the dt root node");
 
@@ -829,8 +712,6 @@  static void machine_class_init(ObjectClass *oc, void *data)
     object_class_property_set_description(oc, "graphics",
         "Set on/off to enable/disable graphics emulation");
 
-    object_class_property_add_str(oc, "firmware",
-        machine_get_firmware, machine_set_firmware);
     object_class_property_set_description(oc, "firmware",
         "Firmware image");
 
@@ -844,8 +725,6 @@  static void machine_class_init(ObjectClass *oc, void *data)
     object_class_property_set_description(oc, "memory-encryption",
         "Set memory encryption object to use");
 
-    object_class_property_add_str(oc, "memory-backend",
-                                  machine_get_memdev, machine_set_memdev);
     object_class_property_set_description(oc, "memory-backend",
                                           "Set RAM backend"
                                           "Valid value is ID of hostmem based backend");
@@ -920,13 +799,6 @@  static void machine_finalize(Object *obj)
 {
     MachineState *ms = MACHINE(obj);
 
-    g_free(ms->kernel_filename);
-    g_free(ms->initrd_filename);
-    g_free(ms->kernel_cmdline);
-    g_free(ms->dtb);
-    g_free(ms->dumpdtb);
-    g_free(ms->dt_compatible);
-    g_free(ms->firmware);
     g_free(ms->device_memory);
     g_free(ms->nvdimms_state);
     g_free(ms->numa_state);