Re: [Qemu-devel] [PATCH v6 02/27] bootindex: add del_boot_device_path function

[Gonglei (Arei)]

> From: Eduardo Habkost [mailto:address@hidden
> Sent: Friday, September 05, 2014 10:20 AM
> Subject: Re: [PATCH v6 02/27] bootindex: add del_boot_device_path function
> 
> On Fri, Sep 05, 2014 at 12:44:56AM +0000, Gonglei (Arei) wrote:
> [...]
> > > can we have a wrapper to reduce code
> > > duplication? e.g. a:
> > >   void device_add_bootindex_property(DeviceState *dev, int32_t *field,
> const
> > > char *suffix)
> > > function.
> > >
> > > Then instead of reimplementing set/get/finalize functions, device code
> > > could just call something like:
> > >   device_add_bootindex_property(dev, &n->nic_conf.bootindex,
> > >                                 "/address@hidden");
> > >
> >
> > This way we cannot attach our target that changing bootindex and take
> effect
> > during vm rebooting.
> 
> I don't understand what you mean, here. Whatever you are planning to do on
> the
> device-specific setter/getters, if they all look the same, you can write a
> common getter/setter to do exactly the same steps, and register it inside
> device_add_bootindex_property(). This way, patches 08/27 to 26/27 can
> become
> one-liners, instead of adding more 20 lines each.
> 
> I mean something like this:
> 
OK. Thanks for explanation. :)

I have two questions:

1) virtio-net-pci device do need special handle in device_set_bootindex(). 
2) isa-fdc' property is bootindexA and bootindexB, maybe we
can add more a parameter for device_add_bootindex_property()?

Best regards,
-Gonglei

> typedef struct BootIndexProperty
> {
>     int32_t *field;
>     const char *suffix;
> } BootIndexProperty;
> 
> static void device_get_bootindex(Object *obj, Visitor *v, void *opaque,
>                                  const char *name, Error **errp)
> {
>     BootIndexProperty *prop = opaque;
>     visit_type_int32(v, prop->field, name, errp);
> }
> 
> static void device_set_bootindex(Object *obj, Visitor *v, void *opaque,
>                                  const char *name, Error **errp)
> {
>     DeviceState *dev = DEVICE(obj);
>     BootIndexProperty *prop = opaque;
>     int32_t boot_index;
>     Error *local_err = NULL;
> 
>     visit_type_int32(v, &boot_index, name, &local_err);
>     if (local_err) {
>         goto out;
>     }
> 
>     check_boot_index(boot_index, &local_err);
>     if (local_err) {
>         goto out;
>     }
> 
>     *prop->field = boot_index;
>     add_boot_device_path(boot_index, dev, prop->suffix);
> 
> out:
>     if (local_err) {
>         error_propagate(errp, local_err);
>     }
> }
> 
> static void property_release_bootindex(Object *obj, const char *name,
>                                        void *opaque)
> 
> {
>     BootIndexProperty *prop = opaque;
>     DeviceState *dev = DEVICE(obj);
>     del_boot_device_path(dev);
>     g_free(prop);
> }
> 
> void device_add_bootindex_property(DeviceState *dev, int32_t *field,
>                                    const char *suffix, Error **errp)
> {
>     Error *local_err = NULL;
>     BootIndexProperty *prop = g_malloc0(sizeof(*prop));
> 
>     prop->field = field;
>     prop->suffix = suffix;
>     object_property_add(OBJECT(dev), "bootindex", "int32",
>                         device_get_bootindex,
>                         device_set_bootindex,
>                         property_release_bootindex,
>                         prop, &local_err);
> 
>     if (local_err) {
>         error_propagate(errp, local_err);
>         g_free(prop);
>     }
> }
> 
> --
> Eduardo