Re: [Qemu-devel] [PATCH v6 07/27] vl.c: add setter/getter functions for bootindex property

[Gonglei (Arei)]

> From: Eduardo Habkost [mailto:address@hidden
> Sent: Friday, September 05, 2014 9:55 AM
> Subject: Re: [PATCH v6 07/27] vl.c: add setter/getter functions for bootindex
> property
> 
> On Fri, Sep 05, 2014 at 12:37:35AM +0000, Gonglei (Arei) wrote:
> > > Subject: Re: [PATCH v6 07/27] vl.c: add setter/getter functions for 
> > > bootindex
> > > property
> > >
> > > On Sat, Aug 30, 2014 at 06:00:07PM +0800, address@hidden
> wrote:
> > > > From: Gonglei <address@hidden>
> > > >
> > > > when we remove bootindex form qdev.property to qom.property,
> > > > we can use those functions set/get bootindex property for all
> > > > correlative devices.
> > > >
> > > > Signed-off-by: Gonglei <address@hidden>
> > > > ---
> > > >  include/sysemu/sysemu.h |  4 ++++
> > > >  vl.c                    | 27 +++++++++++++++++++++++++++
> > > >  2 files changed, 31 insertions(+)
> > > >
> > > > diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> > > > index 672984c..ca231e4 100644
> > > > --- a/include/sysemu/sysemu.h
> > > > +++ b/include/sysemu/sysemu.h
> > > > @@ -208,6 +208,10 @@ void do_usb_del(Monitor *mon, const QDict
> > > *qdict);
> > > >  void usb_info(Monitor *mon, const QDict *qdict);
> > > >
> > > >  void check_boot_index(int32_t bootindex, Error **errp);
> > > > +void get_bootindex(int32_t *bootindex, Visitor *v,
> > > > +                   const char *name, Error **errp);
> > > > +void set_bootindex(int32_t *bootindex, Visitor *v,
> > > > +                   const char *name, Error **errp);
> > > >  void del_boot_device_path(DeviceState *dev);
> > > >  void add_boot_device_path(int32_t bootindex, DeviceState *dev,
> > > >                            const char *suffix);
> > > > diff --git a/vl.c b/vl.c
> > > > index f2c3b2d..4363185 100644
> > > > --- a/vl.c
> > > > +++ b/vl.c
> > > > @@ -1252,6 +1252,33 @@ void check_boot_index(int32_t bootindex,
> Error
> > > **errp)
> > > >      }
> > > >  }
> > > >
> > > > +void get_bootindex(int32_t *bootindex, Visitor *v,
> > > > +                   const char *name, Error **errp)
> > > > +{
> > > > +    visit_type_int32(v, bootindex, name, errp);
> > > > +}
> > > > +
> > > > +void set_bootindex(int32_t *bootindex, Visitor *v,
> > > > +                   const char *name, Error **errp)
> > > > +{
> > > > +    int32_t boot_index;
> > > > +    Error *local_err = NULL;
> > > > +
> > > > +    visit_type_int32(v, &boot_index, name, &local_err);
> > > > +
> > > > +    if (local_err == NULL) {
> > > > +        /* check the bootindex existes or not in fw_boot_order list  */
> > > > +        check_boot_index(boot_index, &local_err);
> > > > +    }
> > > > +
> > > > +    if (local_err) {
> > > > +        error_propagate(errp, local_err);
> > > > +        return;
> > > > +    }
> > > > +    /* change bootindex to a new one */
> > > > +    *bootindex = boot_index;
> > >
> > > I believe the following pattern is more usual (and I find it easier to
> > > read):
> > >
> > >     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;
> > >     }
> > >
> > >     *bootindex = boot_index;
> > >
> > > out:
> > >     if (local_err) {
> > >         error_propagate(errp, local_err);
> > >     }
> > >
> > OK, agreed. Thanks!
> >
> > > Also, this function (the property setter) is where I expected
> > > add_boot_device_path() to be called, instead of requiring every device
> > > to add a reset handler and call add_boot_device_path() manually.
> > >
> > Optional, I have said in previous mail. I think we should lay call
> > add_boot_device_path() in $device_set_bootindex(), not the common
> > function set_bootindex(). We can save the unitariness of a function, right?
> 
> If all (or most) $device_set_bootindex() functions you are adding look
> exactly the same (with just a different struct field), we can have a
> device_add_bootindex_property() wrapper like I suggested on my reply to
> 02/27, instead of copying/pasting the same setter/getter code on every
> device.
> 
I want to know where the device_add_bootindex_property() be called?
which assure that new bootindex take effect during vm rebooting?

> >
> > > I suggest creating a new file for all the bootindex/boot-device code
> > > (maybe call it bootdevice.c?), instead of adding more code to vl.c.
> > >
> > Agreed. But how do we do for the original code in vl.c, move them to
> > new file too? Maybe vl.c is need a big reconstruction, but is out of scope
> > of this series IMHO.
> 
> Yes, I would move the existing bootindex code (fw_boot_order,
> add_boot_device_path(), get_boot_device(), get_boot_devices_list()) to
> the new file as the first step, and then make the changes inside the new
> file. See, for example, how we created numa.c.
> 
Sounds good to me. Thanks!

Best regards,
-Gonglei