From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33195) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X4Wno-0001MD-PA for qemu-devel@nongnu.org; Tue, 08 Jul 2014 10:55:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1X4Wnk-0003zw-SN for qemu-devel@nongnu.org; Tue, 08 Jul 2014 10:55:28 -0400 Received: from mx1.redhat.com ([209.132.183.28]:34626) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X4Wnk-0003zs-Kn for qemu-devel@nongnu.org; Tue, 08 Jul 2014 10:55:24 -0400 Date: Tue, 8 Jul 2014 22:55:12 +0800 From: Amos Kong Message-ID: <20140708145512.GA3061@z.redhat.com> References: <1404724261-9412-1-git-send-email-arei.gonglei@huawei.com> <1404724261-9412-2-git-send-email-arei.gonglei@huawei.com> <20140708083302.GB3962@z.redhat.com> <53BBCFCA.3090606@huawei.com> <33183CC9F5247A488A2544077AF1902086C19408@SZXEMA503-MBS.china.huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <33183CC9F5247A488A2544077AF1902086C19408@SZXEMA503-MBS.china.huawei.com> Subject: Re: [Qemu-devel] [RFC PATCH 1/5] bootindex: add *_boot_device_path function List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Gonglei (Arei)" Cc: "chenliang (T)" , "Huangweidong (C)" , "peter.maydell@linaro.org" , "aik@ozlabs.ru" , "qemu-devel@nongnu.org" , "agraf@suse.de" , "kraxel@redhat.com" , "dmitry@daynix.com" , "mst@redhat.com" , "armbru@redhat.com" , "lersek@redhat.com" , "marcel.a@redhat.com" , "somlo@cmu.edu" , Luonengjun , "Huangpeng (Peter)" , "alex.williamson@redhat.com" , "stefanha@redhat.com" , "pbonzini@redhat.com" , "lcapitulino@redhat.com" , "rth@twiddle.net" , "kwolf@redhat.com" , "peter.crosthwaite@xilinx.com" , "imammedo@redhat.com" , "afaerber@suse.de" 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; afaerber@suse.de; > > agraf@suse.de; stefanha@redhat.com; aik@ozlabs.ru; > > alex.williamson@redhat.com; armbru@redhat.com; eblake@redhat.com; > > kwolf@redhat.com; peter.maydell@linaro.org; lcapitulino@redhat.com; > > mst@redhat.com; pbonzini@redhat.com; lersek@redhat.com; > > kraxel@redhat.com; imammedo@redhat.com; dmitry@daynix.com; > > marcel.a@redhat.com; peter.crosthwaite@xilinx.com; rth@twiddle.net; > > somlo@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.gonglei@huawei.com > > wrote: > > >> From: Chenliang > > >> > > >> 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 > > >> Signed-off-by: Gonglei > > >> --- > > >> 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.