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

[zhang bo]

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

[Jiri Denemark]

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

[zhang bo]

Source device/file is not unique now, we should check it when attach device.

Signed-off-by: YueWenyuan 
Signed-off-by: Zhang Bo 
---
 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->ds