[libvirt] [PATCH 0/2] Check disk serial strings for uniqueness
Peter Krempa (2): qemu: Perform the disk WWN check only on fresh starts conf: Reuse virDomainDefCheckDuplicateDiskWWN to check disk serial too src/conf/domain_conf.c | 15 ++++++++++++++- src/conf/domain_conf.h | 2 +- src/libvirt_private.syms | 2 +- src/qemu/qemu_process.c | 3 ++- 4 files changed, 18 insertions(+), 4 deletions(-) -- 2.4.5
Since we'd disallow migration of a guest that would have possibly invalid config but still be able to work, relax the WWN check to be performed only on new starts of the VM. --- src/qemu/qemu_process.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index f2586a1..9c107bd 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4628,7 +4628,8 @@ int qemuProcessStart(virConnectPtr conn, goto cleanup; } - if (virDomainDefCheckDuplicateDiskWWN(vm->def) < 0) + if (!migrateFrom && !snapshot && + virDomainDefCheckDuplicateDiskWWN(vm->def) < 0) goto cleanup; /* "volume" type disk's source must be translated before -- 2.4.5
On Tue, Sep 29, 2015 at 18:38:04 +0200, Peter Krempa wrote:
Since we'd disallow migration of a guest that would have possibly invalid config but still be able to work, relax the WWN check to be performed only on new starts of the VM. --- src/qemu/qemu_process.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index f2586a1..9c107bd 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4628,7 +4628,8 @@ int qemuProcessStart(virConnectPtr conn, goto cleanup; }
- if (virDomainDefCheckDuplicateDiskWWN(vm->def) < 0) + if (!migrateFrom && !snapshot && + virDomainDefCheckDuplicateDiskWWN(vm->def) < 0) goto cleanup;
/* "volume" type disk's source must be translated before
In other words, this will allow domains started on an older libvirt (which didn't check for duplicate disk info) to be migrated to or restored on a new libvirtd, right? ACK Jirka
Rename the function to virDomainDefCheckDuplicateDiskInfo and make it check disk serials too. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1245013 --- src/conf/domain_conf.c | 15 ++++++++++++++- src/conf/domain_conf.h | 2 +- src/libvirt_private.syms | 2 +- src/qemu/qemu_process.c | 2 +- 4 files changed, 17 insertions(+), 4 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 393ece7..e9d61db 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -24451,7 +24451,7 @@ virDomainDefNeedsPlacementAdvice(virDomainDefPtr def) int -virDomainDefCheckDuplicateDiskWWN(virDomainDefPtr def) +virDomainDefCheckDuplicateDiskInfo(virDomainDefPtr def) { size_t i; size_t j; @@ -24469,6 +24469,19 @@ virDomainDefCheckDuplicateDiskWWN(virDomainDefPtr def) } } } + + if (def->disks[i]->serial) { + for (j = i + 1; j < def->ndisks; j++) { + if (STREQ_NULLABLE(def->disks[i]->serial, + def->disks[j]->serial)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Disks '%s' and '%s' have identical serial"), + def->disks[i]->dst, + def->disks[j]->dst); + return -1; + } + } + } } return 0; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index a101e2a..fd4ef82 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3219,7 +3219,7 @@ virDomainParseMemory(const char *xpath, bool virDomainDefNeedsPlacementAdvice(virDomainDefPtr def) ATTRIBUTE_NONNULL(1); -int virDomainDefCheckDuplicateDiskWWN(virDomainDefPtr def) +int virDomainDefCheckDuplicateDiskInfo(virDomainDefPtr def) ATTRIBUTE_NONNULL(1); #endif /* __DOMAIN_CONF_H */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c87efa1..c1b7b87 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -200,7 +200,7 @@ virDomainCpuPlacementModeTypeFromString; virDomainCpuPlacementModeTypeToString; virDomainDefAddImplicitControllers; virDomainDefCheckABIStability; -virDomainDefCheckDuplicateDiskWWN; +virDomainDefCheckDuplicateDiskInfo; virDomainDefCheckUnsupportedMemoryHotplug; virDomainDefClearCCWAddresses; virDomainDefClearDeviceAliases; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 9c107bd..8cd713f 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4629,7 +4629,7 @@ int qemuProcessStart(virConnectPtr conn, } if (!migrateFrom && !snapshot && - virDomainDefCheckDuplicateDiskWWN(vm->def) < 0) + virDomainDefCheckDuplicateDiskInfo(vm->def) < 0) goto cleanup; /* "volume" type disk's source must be translated before -- 2.4.5
On Tue, Sep 29, 2015 at 18:38:05 +0200, Peter Krempa wrote:
Rename the function to virDomainDefCheckDuplicateDiskInfo and make it check disk serials too.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1245013 --- src/conf/domain_conf.c | 15 ++++++++++++++- src/conf/domain_conf.h | 2 +- src/libvirt_private.syms | 2 +- src/qemu/qemu_process.c | 2 +- 4 files changed, 17 insertions(+), 4 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 393ece7..e9d61db 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -24451,7 +24451,7 @@ virDomainDefNeedsPlacementAdvice(virDomainDefPtr def)
int -virDomainDefCheckDuplicateDiskWWN(virDomainDefPtr def) +virDomainDefCheckDuplicateDiskInfo(virDomainDefPtr def) { size_t i; size_t j; @@ -24469,6 +24469,19 @@ virDomainDefCheckDuplicateDiskWWN(virDomainDefPtr def) } } } + + if (def->disks[i]->serial) { + for (j = i + 1; j < def->ndisks; j++) { + if (STREQ_NULLABLE(def->disks[i]->serial, + def->disks[j]->serial)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Disks '%s' and '%s' have identical serial"), + def->disks[i]->dst, + def->disks[j]->dst); + return -1; + } + } + }
Any reason for going through all disks twice? Wouldn't it be better to go through them once and check for both serial and wwn within a single loop? :-) Jirka
On Tue, Sep 29, 2015 at 18:50:48 +0200, Jiri Denemark wrote:
On Tue, Sep 29, 2015 at 18:38:05 +0200, Peter Krempa wrote:
Rename the function to virDomainDefCheckDuplicateDiskInfo and make it check disk serials too.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1245013 --- src/conf/domain_conf.c | 15 ++++++++++++++- src/conf/domain_conf.h | 2 +- src/libvirt_private.syms | 2 +- src/qemu/qemu_process.c | 2 +- 4 files changed, 17 insertions(+), 4 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 393ece7..e9d61db 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -24451,7 +24451,7 @@ virDomainDefNeedsPlacementAdvice(virDomainDefPtr def)
int -virDomainDefCheckDuplicateDiskWWN(virDomainDefPtr def) +virDomainDefCheckDuplicateDiskInfo(virDomainDefPtr def) { size_t i; size_t j; @@ -24469,6 +24469,19 @@ virDomainDefCheckDuplicateDiskWWN(virDomainDefPtr def) } } } + + if (def->disks[i]->serial) { + for (j = i + 1; j < def->ndisks; j++) { + if (STREQ_NULLABLE(def->disks[i]->serial, + def->disks[j]->serial)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Disks '%s' and '%s' have identical serial"), + def->disks[i]->dst, + def->disks[j]->dst); + return -1; + } + } + }
Any reason for going through all disks twice? Wouldn't it be better to go through them once and check for both serial and wwn within a single loop? :-)
Erm. I'm going to blame me writing suboptimal (read: crappy) code on hunger. Peter
participants (2)
-
Jiri Denemark -
Peter Krempa