|
From: | lijun |
Subject: | Re: [Qemu-devel] [PATCH] Add remove_boot_device_path() function for hot-unplug device |
Date: | Wed, 16 Apr 2014 00:12:33 +0800 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.3.0 |
On 04/15/2014 02:04 AM, Andreas Färber wrote:
What's your meaning of "This ..." ? Do you mean to modify it as you show in the following ?Am 13.04.2014 15:24, schrieb Jun Li:Add remove_boot_device_path() function to remove bootindex when hot-unplug a device. This patch fixed virtio-blk/virtio-net/scsi-disk/scsi-generic device. So it has fixed bug1086603, ref: https://bugzilla.redhat.com/show_bug.cgi?id=1086603 Signed-off-by: Jun Li <address@hidden> --- hw/block/virtio-blk.c | 1 + hw/net/virtio-net.c | 1 + hw/scsi/scsi-disk.c | 1 + hw/scsi/scsi-generic.c | 1 + include/sysemu/sysemu.h | 2 ++ vl.c | 16 ++++++++++++++++ 6 files changed, 22 insertions(+) diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c index 8a568e5..ecdd266 100644 --- a/hw/block/virtio-blk.c +++ b/hw/block/virtio-blk.c @@ -752,6 +752,7 @@ static void virtio_blk_device_unrealize(DeviceState *dev, Error **errp) unregister_savevm(dev, "virtio-blk", s); blockdev_mark_auto_del(s->bs); virtio_cleanup(vdev); + remove_boot_device_path(s->conf->bootindex, dev, "/address@hidden,0"); }static Property virtio_blk_properties[] = {diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c index 33bd233..520c029 100644 --- a/hw/net/virtio-net.c +++ b/hw/net/virtio-net.c @@ -1633,6 +1633,7 @@ static void virtio_net_device_unrealize(DeviceState *dev, Error **errp) g_free(n->vqs); qemu_del_nic(n->nic); virtio_cleanup(vdev); + remove_boot_device_path(n->nic_conf.bootindex, dev, "/address@hidden"); }static void virtio_net_instance_init(Object *obj)diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c index 48a28ae..f7303e8 100644 --- a/hw/scsi/scsi-disk.c +++ b/hw/scsi/scsi-disk.c @@ -2156,6 +2156,7 @@ static void scsi_destroy(SCSIDevice *dev)scsi_device_purge_requests(&s->qdev, SENSE_CODE(NO_SENSE));blockdev_mark_auto_del(s->qdev.conf.bs); + remove_boot_device_path(s->qdev.conf.bootindex, &dev->qdev, NULL);This ...
Modify it as following, code will be safer. So agree.
Ok, agree with you. Using DEVICE(...), the code will be more safe. Thanks for your good suggestions.}static void scsi_disk_resize_cb(void *opaque)diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c index 8d92e0d..22b9752 100644 --- a/hw/scsi/scsi-generic.c +++ b/hw/scsi/scsi-generic.c @@ -390,6 +390,7 @@ static void scsi_destroy(SCSIDevice *s) { scsi_device_purge_requests(s, SENSE_CODE(NO_SENSE)); blockdev_mark_auto_del(s->conf.bs); + remove_boot_device_path(s->conf.bootindex, &s->qdev, NULL);... and this is not fully right. Here you probably want DeviceState *dev = DEVICE(s) and then use dev in place of &s->qdev. And for the above: static void scsi_destroy(SCSIDevice *dev) { SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev); scsi_device_purge_requests(&s->qdev, SENSE_CODE(NO_SENSE)); blockdev_mark_auto_del(s->qdev.conf.bs); } castling names into: static void scsi_destroy(SCSIDevice *sdev) { DeviceState *dev = DEVICE(sdev); SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, sdev); scsi_device_purge_requests(sdev, SENSE_CODE(NO_SENSE)); blockdev_mark_auto_del(sdev->conf.bs); remove_boot_device_path(sdev->conf.bootindex, dev, NULL); } or similar. Inline DEVICE(...) would also be possible.
}static int scsi_generic_initfn(SCSIDevice *s)diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h index ba5c7f8..f7ad1e2 100644 --- a/include/sysemu/sysemu.h +++ b/include/sysemu/sysemu.h @@ -193,6 +193,8 @@ void rtc_change_mon_event(struct tm *tm);void add_boot_device_path(int32_t bootindex, DeviceState *dev,const char *suffix); +void remove_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 9975e5a..8aaa1d6 100644 --- a/vl.c +++ b/vl.c @@ -1184,6 +1184,22 @@ void add_boot_device_path(int32_t bootindex, DeviceState *dev, QTAILQ_INSERT_TAIL(&fw_boot_order, node, link); }+void remove_boot_device_path(int32_t bootindex, DeviceState *dev,+ const char *suffix) +{ + FWBootEntry *i; + + if (bootindex == -1) { + return; + } + + QTAILQ_FOREACH(i, &fw_boot_order, link) + if (i->bootindex == bootindex) { + QTAILQ_REMOVE(&fw_boot_order, i, link);This looks dangerous... You are returning out of the loop below, which looks OK, but still QTAILQ_FOREACH_SAFE() would make me more comfortable reading this.
ok, agree, I will submit a update version later.
I am not familiar with ppc. So when you find that needs these calls, please add comments. Thanks a lot.+ return; + } +} + DeviceState *get_boot_device(uint32_t position) { uint32_t counter = 0;What I haven't checked yet is whether there might be more places, e.g. in spapr code since recently, that need these calls while at it.
Best regards, Jun Li
[Prev in Thread] | Current Thread | [Next in Thread] |