On Tue, Jul 08, 2014 at 01:22:53PM +0000, Gonglei (Arei) wrote:
> > -----Original Message-----
> > From: chenliang (T)
> > Sent: Tuesday, July 08, 2014 7:03 PM
> > To: Amos Kong
> > Cc: Gonglei (Arei); qemu-devel@nongnu.org; afaer...@suse.de;
> > ag...@suse.de; stefa...@redhat.com; a...@ozlabs.ru;
> > alex.william...@redhat.com; arm...@redhat.com; ebl...@redhat.com;
> > kw...@redhat.com; peter.mayd...@linaro.org; lcapitul...@redhat.com;
> > m...@redhat.com; pbonz...@redhat.com; ler...@redhat.com;
> > kra...@redhat.com; imamm...@redhat.com; dmi...@daynix.com;
> > marce...@redhat.com; peter.crosthwa...@xilinx.com; r...@twiddle.net;
> > so...@cmu.edu; Huangweidong (C); Luonengjun; Huangpeng (Peter)
> > Subject: Re: [RFC PATCH 1/5] bootindex: add *_boot_device_path function
> >
> > On 2014/7/8 16:33, Amos Kong wrote:
> >
> > > On Mon, Jul 07, 2014 at 05:10:57PM +0800, arei.gong...@huawei.com
> > wrote:
> > >> From: Chenliang <chenlian...@huawei.com>
> > >>
> > >> Add del_boot_device_path and modify_boot_device_path. Device should
> > >> be removed from boot device list by del_boot_device_path when device
> > >> hotplug. modify_boot_device_path is used to modify deviceboot order.
> > >
> > > s/hotplug/is unhotplugged/
> > >
> > > same issue in commitlog of patch 3/5
> > >
>
> Yep, thanks!
>
> > >> Signed-off-by: Chenliang <chenlian...@huawei.com>
> > >> Signed-off-by: Gonglei <arei.gong...@huawei.com>
> > >> ---
> > >> include/sysemu/sysemu.h | 4 ++++
> > >> vl.c | 55
> > +++++++++++++++++++++++++++++++++++++++++++++++++
> > >> 2 files changed, 59 insertions(+)
> > >>
> > >> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> > >> index 285c45b..38ef1cd 100644
> > >> --- a/include/sysemu/sysemu.h
> > >> +++ b/include/sysemu/sysemu.h
> > >> @@ -204,6 +204,10 @@ void usb_info(Monitor *mon, const QDict *qdict);
> > >>
> > >> void add_boot_device_path(int32_t bootindex, DeviceState *dev,
> > >> const char *suffix);
> > >> +void del_boot_device_path(int32_t bootindex, DeviceState *dev,
> > >> + const char *suffix);
> > >> +void modify_boot_device_path(int32_t bootindex, DeviceState *dev,
> > >> + const char *suffix);
> > >> char *get_boot_devices_list(size_t *size, bool ignore_suffixes);
> > >>
> > >> DeviceState *get_boot_device(uint32_t position);
> > >> diff --git a/vl.c b/vl.c
> > >> index a1686ef..6264e11 100644
> > >> --- a/vl.c
> > >> +++ b/vl.c
> > >> @@ -1247,6 +1247,61 @@ void add_boot_device_path(int32_t bootindex,
> > DeviceState *dev,
> > >> QTAILQ_INSERT_TAIL(&fw_boot_order, node, link);
> > >> }
> > >>
> > >> +static bool is_same_fw_dev_path(DeviceState *src, DeviceState *target)
> > >> +{
> > >> + bool ret = false;
> > >> + char *devpath_src = qdev_get_fw_dev_path(src);
> > >> + char *devpath_target = qdev_get_fw_dev_path(target);
> > >> +
> > >> + if (!strcmp(devpath_src, devpath_target)) {
> > >> + ret = true;
> > >> + }
> > >> +
> > >> + g_free(devpath_src);
> > >> + g_free(devpath_target);
> > >> + return ret;
> > >> +}
> > >> +
> > >> +void del_boot_device_path(int32_t bootindex, DeviceState *dev,
> > >> + const char *suffix)
> > >> +{
> > >> + FWBootEntry *i;
> > >> +
> > >> + assert(dev != NULL);
> > >> +
> > >
> > > assert(booindex >= 0 || suffix != NULL);
> > >
>
> suffix call be NULL.
>
> > >> + QTAILQ_FOREACH(i, &fw_boot_order, link) {
> > >> + if (is_same_fw_dev_path(i->dev, dev)) {
> > >
> > > if (!suffix) {
> > > break;
> > > }
If suffix is NULL, at least we should do nothing in the loop.
> > >
> > >> + if (suffix && i->suffix && strcmp(i->suffix, suffix)) {
> > >> + continue;
> > >> + }
How about this one?
if (!suffix) {
break;
} else (i->suffix && strcmp(i->suffix, suffix)) {
continue;
}
> > >
> > > If suffix is NULL, then all the entries will be removed?
>
> If suffix is NULL, the entry will be checked by the bootindex as below
> QTAILQ_FOREACH loop. If suffix and bootindex are all not match,
> no entry will not be deleted from the global fw_boot_order list.
This is why I added "assert(booindex >= 0 || suffix != NULL);" on
above.
^^^^
> >
> >
> > yes, it will be if caller don't give suffix.
> >
> > >
> > >> + QTAILQ_REMOVE(&fw_boot_order, i, link);
> > >> + g_free(i->suffix);
> > >> + g_free(i);
> > >> + break;
> > >> + }
> > >> + }
> > >> +
> > >> + if (bootindex == -1) {
> > >
> > > if (bootindex < 0) {
> >
> >
> > acked
> >
> > >
> > >> + return;
> > >> + }
> > >> +
> > >> + QTAILQ_FOREACH(i, &fw_boot_order, link) {
> > >> + if (i->bootindex == bootindex) {
> > >> + QTAILQ_REMOVE(&fw_boot_order, i, link);
> > >> + g_free(i->suffix);
> > >> + g_free(i);
> > >> + break;
> > >> + }
> > >> + }
> > >> +}
> > >> +
> > >> +void modify_boot_device_path(int32_t bootindex, DeviceState *dev,
> > >> + const char *suffix)
> > >> +{
> > >> + del_boot_device_path(bootindex, dev, suffix);
> > >> + add_boot_device_path(bootindex, dev, suffix);
> > >
> > > Why do you directly modify existed entry?
> >
> >
> > Sometimes, in old boot device list:
> >
> > device_id bootindex
> > net0 1
> > net1 2
> > net2 3
> >
> > we want to make vm reboot from net2, we can do it like this:
> >
> > modify_boot_device_path(bootindex=1, DeviceState=net2, suffix=NULL), the
> > new boot device list will like this:
> >
> > device_id bootindex
> > net2 1
> > net1 2
> >
>
> Yes.
> the visual bootindex of net0 will be deleted, and then we can look
> it as default value.
>
> Best regards,
> -Gonglei
--
Amos.
