Re: [Qemu-devel] [RFC PATCH 1/5] bootindex: add *_boot_device_path function

[Gonglei (Arei)]

> -----Original Message-----
> From: chenliang (T)
> Sent: Tuesday, July 08, 2014 7:03 PM
> To: Amos Kong
> Cc: Gonglei (Arei); address@hidden; address@hidden;
> address@hidden; address@hidden; address@hidden;
> address@hidden; address@hidden; address@hidden;
> address@hidden; address@hidden; address@hidden;
> address@hidden; address@hidden; address@hidden;
> address@hidden; address@hidden; address@hidden;
> address@hidden; address@hidden; address@hidden;
> address@hidden; 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, address@hidden
> wrote:
> >> From: Chenliang <address@hidden>
> >>
> >> 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 <address@hidden>
> >> Signed-off-by: Gonglei <address@hidden>
> >> ---
> >>  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 && 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.

> 
> 
> 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