| Message ID | 1409392827-9372-8-git-send-email-arei.gonglei@huawei.com |
|---|---|
| State | New |
| Headers | show |
On Sat, Aug 30, 2014 at 06:00:07PM +0800, arei.gonglei@huawei.com wrote: > From: Gonglei <arei.gonglei@huawei.com> > > 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 <arei.gonglei@huawei.com> > --- > 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 */ should be: check whether bootindex is present 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; > +} > + > static bool is_same_fw_dev_path(DeviceState *src, DeviceState *dst) > { > bool ret = false; > -- > 1.7.12.4 >
> From: Michael S. Tsirkin [mailto:mst@redhat.com] > Sent: Sunday, August 31, 2014 5:58 PM > 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, arei.gonglei@huawei.com wrote: > > From: Gonglei <arei.gonglei@huawei.com> > > > > 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 <arei.gonglei@huawei.com> > > --- > > 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 */ > > should be: > check whether bootindex is present in fw_boot_order list > OK, will fix this. Thanks! Best regards, -Gonglei
Hi, > -----Original Message----- > From: Gonglei (Arei) > Sent: Saturday, August 30, 2014 6:00 PM > Subject: [PATCH v6 07/27] vl.c: add setter/getter functions for bootindex > property > > From: Gonglei <arei.gonglei@huawei.com> > > 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 <arei.gonglei@huawei.com> > --- > 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; > +} > + In this version, I just change devices' bootindex value, but not update fw_boot_order immediately. When we reboot the vm, we will call add_boot_device_path to update fw_boot_order list. Do you think it is a good method? This method will cause a problem that when we want set a existed bootindex which has been changed, we will get failure. Only after vm rebooting, we can set the same bootindex again. I have listed the example in PATCH 00/27 cover-letter: (qemu) qom-get /machine/peripheral/nic1 bootindex 3 (0x3) (qemu) qom-set /machine/peripheral/nic1 bootindex 3 The bootindex 3 has already been used (qemu) qom-set /machine/peripheral/nic1 bootindex 0 --->change nic1's bootindex to 0, successful (qemu) qom-get /machine/peripheral/nic1 bootindex 0 (0x0) (qemu) qom-set /machine/peripheral/floppy1 bootindexA 3 -->set floppy1's bootindex to 3, failed The bootindex 3 has already been used (qemu) system_reset --> reboot vm (qemu) qom-get /machine/peripheral/nic1 bootindex 0 (0x0) (qemu) qom-get /machine/peripheral/floppy1 bootindexA 5 (0x5) (qemu) qom-set /machine/peripheral/floppy1 bootindexA 3 -->set floppy1's bootindex to 3, successful (qemu) qom-get /machine/peripheral/floppy1 bootindexA 3 (0x3) (qemu) Any ideas will be appreciated! Best regards, -Gonglei > static bool is_same_fw_dev_path(DeviceState *src, DeviceState *dst) > { > bool ret = false; > -- > 1.7.12.4 >
Hi, > +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; > > +} > > + > > In this version, I just change devices' bootindex value, but not update fw_boot_order > immediately. When we reboot the vm, we will call add_boot_device_path to update > fw_boot_order list. Do you think it is a good method? This method will cause a problem > that when we want set a existed bootindex which has been changed, we will get failure. > Only after vm rebooting, we can set the same bootindex again. Good point. check_boot_index() doing the verification against stale data is pointless and may even throw an error in cases where it should not. I guess we should add a add_boot_device_path() call to the ${device}_set_bootindex functions. Which also means we don't need to do it on reset (patch #6 can be dropped). And I think we can also drop the add_boot_device_path() calls from ${device}_realize then. cheers, Gerd
> From: Gerd Hoffmann [mailto:kraxel@redhat.com] > > Hi, > > > +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; > > > +} > > > + > > > > In this version, I just change devices' bootindex value, but not update > fw_boot_order > > immediately. When we reboot the vm, we will call add_boot_device_path to > update > > fw_boot_order list. Do you think it is a good method? This method will cause > a problem > > that when we want set a existed bootindex which has been changed, we will > get failure. > > Only after vm rebooting, we can set the same bootindex again. > > Good point. check_boot_index() doing the verification against stale > data is pointless and may even throw an error in cases where it should > not. > Yes. > I guess we should add a add_boot_device_path() call to the > ${device}_set_bootindex functions. Which also means we don't need to do > it on reset (patch #6 can be dropped). And I think we can also drop the > add_boot_device_path() calls from ${device}_realize then. > Bravo! I can't agree more with you. Thanks, Gerd. Will do it. Best regards, -Gonglei
On Sat, Aug 30, 2014 at 06:00:07PM +0800, arei.gonglei@huawei.com wrote: > From: Gonglei <arei.gonglei@huawei.com> > > 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 <arei.gonglei@huawei.com> > --- > 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); } 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. 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. > +} > + > static bool is_same_fw_dev_path(DeviceState *src, DeviceState *dst) > { > bool ret = false; > -- > 1.7.12.4 > >
> 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, arei.gonglei@huawei.com wrote: > > From: Gonglei <arei.gonglei@huawei.com> > > > > 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 <arei.gonglei@huawei.com> > > --- > > 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? > 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. Best regards, -Gonglei
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, arei.gonglei@huawei.com wrote: > > > From: Gonglei <arei.gonglei@huawei.com> > > > > > > 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 <arei.gonglei@huawei.com> > > > --- > > > 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 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.
> From: Eduardo Habkost [mailto:ehabkost@redhat.com] > 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, arei.gonglei@huawei.com > wrote: > > > > From: Gonglei <arei.gonglei@huawei.com> > > > > > > > > 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 <arei.gonglei@huawei.com> > > > > --- > > > > 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
On Fri, Sep 05, 2014 at 02:07:23AM +0000, Gonglei (Arei) wrote: [...] > > > > 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? In exactly the same place you were going to call object_property_add() (instance_init). It would be just a wrapper around object_property_add() that won't require you to write new setter/getter functions for each device.
> From: Eduardo Habkost [mailto:ehabkost@redhat.com] > Sent: Friday, September 05, 2014 10:22 AM > Subject: Re: [PATCH v6 07/27] vl.c: add setter/getter functions for bootindex > property > > On Fri, Sep 05, 2014 at 02:07:23AM +0000, Gonglei (Arei) wrote: > [...] > > > > > 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? > > In exactly the same place you were going to call object_property_add() > (instance_init). It would be just a wrapper around object_property_add() > that won't require you to write new setter/getter functions for each > device. > Got it, thanks! Best regards, -Gonglei > -- > Eduardo
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; +} + static bool is_same_fw_dev_path(DeviceState *src, DeviceState *dst) { bool ret = false;