[libvirt] [PATCH] Add source check before attaching device

Source device/file is not unique now, we should check it when attach device. Signed-off-by: YueWenyuan <yuewenyuan@huawei.com> Signed-off-by: Zhang Bo <oscar.zhangbo@huawei.com> --- src/conf/domain_conf.c | 15 +++++++++++++++ src/conf/domain_conf.h | 1 + src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 12 +++++++++++- src/qemu/qemu_hotplug.c | 42 +++++++++++++++++++++++++++--------------- 5 files changed, 55 insertions(+), 16 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 58b98c6..3fd729c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18191,6 +18191,21 @@ virDomainControllerDefFormat(virBufferPtr buf, int +virDomainFSIndexBySrc(virDomainDefPtr def, const char *src) +{ + virDomainFSDefPtr fs; + size_t i; + + for (i = 0; i < def->nfss; i++) { + fs = def->fss[i]; + if (STREQ(fs->src, src)) + return i; + } + return -1; +} + + +int virDomainFSIndexByName(virDomainDefPtr def, const char *name) { virDomainFSDefPtr fs; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index e6fa3c9..e23f289 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2804,6 +2804,7 @@ int virDiskNameToBusDeviceIndex(virDomainDiskDefPtr disk, virDomainFSDefPtr virDomainGetFilesystemForTarget(virDomainDefPtr def, const char *target); int virDomainFSInsert(virDomainDefPtr def, virDomainFSDefPtr fs); +int virDomainFSIndexBySrc(virDomainDefPtr def, const char *src); int virDomainFSIndexByName(virDomainDefPtr def, const char *name); virDomainFSDefPtr virDomainFSRemove(virDomainDefPtr def, size_t i); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7166283..611c0d4 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -274,6 +274,7 @@ virDomainDiskSourceIsBlockType; virDomainEmulatorPinAdd; virDomainEmulatorPinDel; virDomainFSDefFree; +virDomainFSIndexBySrc; virDomainFSIndexByName; virDomainFSInsert; virDomainFSRemove; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index cbb6e1b..3b187f0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7953,6 +7953,11 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps, _("target %s already exists"), disk->dst); return -1; } + if (virDomainDiskIndexByName(vmdef, disk->src->path, true) >= 0) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("source %s already exists"), disk->src->path); + return -1; + } if (qemuCheckDiskConfig(disk) < 0) return -1; if (virDomainDiskInsert(vmdef, disk)) @@ -8035,7 +8040,12 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps, fs = dev->data.fs; if (virDomainFSIndexByName(vmdef, fs->dst) >= 0) { virReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("Target already exists")); + _("Target %s already exists"), fs->dst); + return -1; + } + if (virDomainFSIndexBySrc(vmdef, fs->src) >= 0) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("Source %s already exists"), fs->src); return -1; } diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 2f0549e..5dd2453 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -315,12 +315,34 @@ qemuDomainCheckEjectableMedia(virQEMUDriverPtr driver, } static int +qemuDomainCheckDiskDeviceExists(virDomainObjPtr vm, + virDomainDiskDefPtr disk) +{ + int ret = -1; + size_t i; + for (i = 0; i < vm->def->ndisks; i++) { + if (STREQ(vm->def->disks[i]->dst, disk->dst)) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("target %s already exists"), disk->dst); + return ret; + } + if (disk->src && vm->def->disks[i]->src && + disk->src->path && vm->def->disks[i]->src->path && + STREQ(vm->def->disks[i]->src->path, disk->src->path)) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("source %s already exists"), disk->src->path); + return ret; + } + } + return 0; +} + +static int qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainDiskDefPtr disk) { - size_t i; int ret = -1; const char* type = virDomainDiskBusTypeToString(disk->bus); qemuDomainObjPrivatePtr priv = vm->privateData; @@ -338,13 +360,8 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, disk->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390; } - for (i = 0; i < vm->def->ndisks; i++) { - if (STREQ(vm->def->disks[i]->dst, disk->dst)) { - virReportError(VIR_ERR_OPERATION_FAILED, - _("target %s already exists"), disk->dst); - goto cleanup; - } - } + if (qemuDomainCheckDiskDeviceExists(vm, disk) < 0) + goto cleanup; if (qemuDomainPrepareDisk(driver, vm, disk, NULL, false) < 0) goto cleanup; @@ -579,13 +596,8 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn, int ret = -1; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); - for (i = 0; i < vm->def->ndisks; i++) { - if (STREQ(vm->def->disks[i]->dst, disk->dst)) { - virReportError(VIR_ERR_OPERATION_FAILED, - _("target %s already exists"), disk->dst); - goto cleanup; - } - } + if (qemuDomainCheckDiskDeviceExists(vm, disk) < 0) + goto cleanup; if (qemuDomainPrepareDisk(driver, vm, disk, NULL, false) < 0) goto cleanup; -- 1.7.12.4

On Thu, Apr 16, 2015 at 16:51:52 +0800, zhang bo wrote:
Source device/file is not unique now, we should check it when attach device.
The check is vary fragile and unreliable. It would be very hard (perhaps even impossible) to really detect that the two disks/filesystems point to the same source, which means libvirt would not really prevent anyone from attaching a single source twice. Moreover, the code as written would incorrectly refuse to attach disks that do not point to the same source (network disks, e.g.). But anyway, I don't think libvirt should be doing this in general, so NACK to this idea. Moreover, you should be able to achieve the same goal (and even more) by using a locking driver. Jirka

On 2015/4/16 17:44, Jiri Denemark wrote:
On Thu, Apr 16, 2015 at 16:51:52 +0800, zhang bo wrote:
Source device/file is not unique now, we should check it when attach device.
The check is vary fragile and unreliable. It would be very hard (perhaps even impossible) to really detect that the two disks/filesystems point to the same source, which means libvirt would not really prevent anyone from attaching a single source twice. Moreover, the code as written would incorrectly refuse to attach disks that do not point to the same source (network disks, e.g.).
But anyway, I don't think libvirt should be doing this in general, so NACK to this idea.
Moreover, you should be able to achieve the same goal (and even more) by using a locking driver.
Jirka
That's true! we didn't think of that. Thank you Jirka. oscar
participants (2)
-
Jiri Denemark
-
zhang bo