[libvirt] [PATCH v2 0/9] Various hotplug cleanups
v1: http://www.redhat.com/archives/libvir-list/2016-June/msg02207.html Changes since v1... - Use the bz noted by Cole from the v1 series - Merged in recent hotplug cleanup changes - Patches 6 - 9 are new Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1336225 and probably: https://bugzilla.redhat.com/show_bug.cgi?id=1289391 Patches 1-5 address the USB hotplug cleanup Patch 6 addresses the SCSI hotplug cleanup Patch 7-9 address some SCSI host device cleanups found while looking at the other cleanups... John Ferlan (9): qemu: Reorder qemuDomainAttachUSBMassStorageDevice failure path qemu: Move drive alias processing to qemu_alias qemu: Use qemuAssignDeviceDiskDriveAlias qemu: Make QEMU_DRIVE_HOST_PREFIX more private qemu: Add attempt to call qemuMonitorDriveDel for USB failure path qemu: Add attempt to call qemuMonitorDriveDel for AttachSCSI failure path qemu: Introduce qemuAssignSCSIHostDeviceDriveAlias qemu: Use qemuAssignSCSIHostDeviceDriveAlias qemu: Use the hostdev alias in qemuDomainAttachHostSCSIDevice error path src/qemu/qemu_alias.c | 52 +++++++++++++++++++++++++ src/qemu/qemu_alias.h | 6 +++ src/qemu/qemu_command.c | 35 ++++++++--------- src/qemu/qemu_command.h | 1 - src/qemu/qemu_domain.c | 3 +- src/qemu/qemu_driver.c | 3 +- src/qemu/qemu_hotplug.c | 92 +++++++++++++++++++++++++++++--------------- src/qemu/qemu_migration.c | 11 ++---- src/qemu/qemu_monitor_json.c | 10 ++--- src/qemu/qemu_monitor_text.c | 18 +++++---- src/qemu/qemu_process.c | 3 +- 11 files changed, 156 insertions(+), 78 deletions(-) -- 2.5.5
Modify the error/exit path to match what was done for Virtio and SCSI. If nothing else it'll have a consistent look'n'feel Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_hotplug.c | 38 ++++++++++++++++++++++---------------- 1 file changed, 22 insertions(+), 16 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 5e18d34..6364d68 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -709,6 +709,7 @@ qemuDomainAttachUSBMassStorageDevice(virQEMUDriverPtr driver, int ret = -1; char *drivestr = NULL; char *devstr = NULL; + bool driveAdded = false; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); const char *src = virDomainDiskGetSource(disk); @@ -733,27 +734,21 @@ qemuDomainAttachUSBMassStorageDevice(virQEMUDriverPtr driver, goto error; qemuDomainObjEnterMonitor(driver, vm); - ret = qemuMonitorAddDrive(priv->mon, drivestr); - if (ret == 0) { - ret = qemuMonitorAddDevice(priv->mon, devstr); - if (ret < 0) { - VIR_WARN("qemuMonitorAddDevice failed on %s (%s)", - drivestr, devstr); - /* XXX should call 'drive_del' on error but this does not - exist yet */ - } - } - if (qemuDomainObjExitMonitor(driver, vm) < 0) { - ret = -1; - goto error; - } - virDomainAuditDisk(vm, NULL, disk->src, "attach", ret >= 0); + if (qemuMonitorAddDrive(priv->mon, drivestr) < 0) + goto exit_monitor; + driveAdded = true; - if (ret < 0) + if (qemuMonitorAddDevice(priv->mon, devstr) < 0) + goto exit_monitor; + + if (qemuDomainObjExitMonitor(driver, vm) < 0) goto error; + virDomainAuditDisk(vm, NULL, disk->src, "attach", true); + virDomainDiskInsertPreAlloced(vm->def, disk); + ret = 0; cleanup: VIR_FREE(devstr); @@ -761,6 +756,17 @@ qemuDomainAttachUSBMassStorageDevice(virQEMUDriverPtr driver, virObjectUnref(cfg); return ret; + exit_monitor: + if (driveAdded) { + VIR_WARN("qemuMonitorAddDevice failed on %s (%s)", + drivestr, devstr); + /* XXX should call 'drive_del' on error but this does not + exist yet */ + } + + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + virDomainAuditDisk(vm, NULL, disk->src, "attach", false); + error: ignore_value(qemuDomainPrepareDisk(driver, vm, disk, NULL, true)); goto cleanup; -- 2.5.5
Move qemuDeviceDriveHostAlias from qemu_command and rename to qemuAssignDeviceDiskDriveAlias in qemu_alias. Also change the parameter to just be the disk->info.alias (e.g. srcalias) Includes some innocent bystanders that seem to need QEMU_DRIVE_HOST_PREFIX which requires include qemu_alias.h instead of qemu_command.h Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_alias.c | 17 +++++++++++++++++ src/qemu/qemu_alias.h | 4 ++++ src/qemu/qemu_command.c | 11 ----------- src/qemu/qemu_command.h | 1 - src/qemu/qemu_hotplug.c | 4 ++-- src/qemu/qemu_migration.c | 2 +- src/qemu/qemu_monitor_json.c | 2 +- src/qemu/qemu_monitor_text.c | 2 +- 8 files changed, 26 insertions(+), 17 deletions(-) diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c index 51a654a..70cd9f1 100644 --- a/src/qemu/qemu_alias.c +++ b/src/qemu/qemu_alias.c @@ -170,6 +170,23 @@ qemuAssignDeviceControllerAlias(virDomainDefPtr domainDef, return virAsprintf(&controller->info.alias, "%s%d", prefix, controller->idx); } +/* qemuAssignDeviceDiskDriveAlias: + * @srcalias: Disk source alias + * + * Generate and return an alias for the device disk '-drive' + * + * Returns NULL or a string containing the alias + */ +char * +qemuAssignDeviceDiskDriveAlias(const char *srcalias) +{ + char *ret; + + if (virAsprintf(&ret, "%s%s", QEMU_DRIVE_HOST_PREFIX, srcalias) < 0) + return NULL; + return ret; +} + /* Our custom -drive naming scheme used with id= */ int diff --git a/src/qemu/qemu_alias.h b/src/qemu/qemu_alias.h index d1c6ba8..db4b690 100644 --- a/src/qemu/qemu_alias.h +++ b/src/qemu/qemu_alias.h @@ -30,6 +30,8 @@ # include "qemu_domain.h" # include "qemu_domain_address.h" +# define QEMU_DRIVE_HOST_PREFIX "drive-" + int qemuAssignDeviceChrAlias(virDomainDefPtr def, virDomainChrDefPtr chr, ssize_t idx); @@ -38,6 +40,8 @@ int qemuAssignDeviceControllerAlias(virDomainDefPtr domainDef, virQEMUCapsPtr qemuCaps, virDomainControllerDefPtr controller); +char *qemuAssignDeviceDiskDriveAlias(const char *srcalias); + int qemuAssignDeviceDiskAlias(virDomainDefPtr vmdef, virDomainDiskDefPtr def, virQEMUCapsPtr qemuCaps); diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 4558b9f..affd0b0 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -272,17 +272,6 @@ qemuVirCommandGetDevSet(virCommandPtr cmd, int fd) } -char *qemuDeviceDriveHostAlias(virDomainDiskDefPtr disk) -{ - char *ret; - - if (virAsprintf(&ret, "%s%s", - QEMU_DRIVE_HOST_PREFIX, disk->info.alias) < 0) - return NULL; - return ret; -} - - static int qemuBuildDeviceAddressStr(virBufferPtr buf, const virDomainDef *domainDef, diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index c4d0567..dcf9ba6 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -37,7 +37,6 @@ /* Config type for XML import/export conversions */ # define QEMU_CONFIG_FORMAT_ARGV "qemu-argv" -# define QEMU_DRIVE_HOST_PREFIX "drive-" # define QEMU_FSDEV_HOST_PREFIX "fsdev-" # define QEMU_BLOCK_IOTUNE_MAX 1000000000000000LL diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 6364d68..4f521a4 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -229,7 +229,7 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, if (qemuDomainPrepareDisk(driver, vm, disk, newsrc, false) < 0) goto cleanup; - if (!(driveAlias = qemuDeviceDriveHostAlias(disk))) + if (!(driveAlias = qemuAssignDeviceDiskDriveAlias(disk->info.alias))) goto error; qemuDomainObjEnterMonitor(driver, vm); @@ -365,7 +365,7 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, if (!(drivestr = qemuBuildDriveStr(disk, false, priv->qemuCaps))) goto error; - if (!(drivealias = qemuDeviceDriveHostAlias(disk))) + if (!(drivealias = qemuAssignDeviceDiskDriveAlias(disk->info.alias))) goto error; if (!(devstr = qemuBuildDriveDevStr(vm->def, disk, 0, priv->qemuCaps))) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 463e624..04c847e 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -36,7 +36,7 @@ #include "qemu_domain.h" #include "qemu_process.h" #include "qemu_capabilities.h" -#include "qemu_command.h" +#include "qemu_alias.h" #include "qemu_cgroup.h" #include "qemu_hotplug.h" #include "qemu_blockjob.h" diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index bb426dc..a54ff8d 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -33,7 +33,7 @@ #include "qemu_monitor_text.h" #include "qemu_monitor_json.h" -#include "qemu_command.h" +#include "qemu_alias.h" #include "qemu_parse_command.h" #include "qemu_capabilities.h" #include "viralloc.h" diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index 9295219..6c458e2 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -31,7 +31,7 @@ #include <string.h> #include "qemu_monitor_text.h" -#include "qemu_command.h" +#include "qemu_alias.h" #include "c-ctype.h" #include "c-strcasestr.h" #include "viralloc.h" -- 2.5.5
On Tue, Jul 19, 2016 at 10:30:45AM -0400, John Ferlan wrote:
Move qemuDeviceDriveHostAlias from qemu_command and rename to qemuAssignDeviceDiskDriveAlias in qemu_alias. Also change the parameter to just be the disk->info.alias (e.g. srcalias)
Includes some innocent bystanders that seem to need QEMU_DRIVE_HOST_PREFIX which requires include qemu_alias.h instead of qemu_command.h
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_alias.c | 17 +++++++++++++++++ src/qemu/qemu_alias.h | 4 ++++ src/qemu/qemu_command.c | 11 ----------- src/qemu/qemu_command.h | 1 - src/qemu/qemu_hotplug.c | 4 ++-- src/qemu/qemu_migration.c | 2 +- src/qemu/qemu_monitor_json.c | 2 +- src/qemu/qemu_monitor_text.c | 2 +- 8 files changed, 26 insertions(+), 17 deletions(-)
diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c index 51a654a..70cd9f1 100644 --- a/src/qemu/qemu_alias.c +++ b/src/qemu/qemu_alias.c @@ -170,6 +170,23 @@ qemuAssignDeviceControllerAlias(virDomainDefPtr domainDef, return virAsprintf(&controller->info.alias, "%s%d", prefix, controller->idx); }
+/* qemuAssignDeviceDiskDriveAlias: + * @srcalias: Disk source alias + * + * Generate and return an alias for the device disk '-drive'
The other Assign* functions fill out the alias in device info. This one just returns the string. Also, most of the functions in this file do not have a prefix derived from the file name, they start with qemuAssign instead of qemuAlias I propose qemuAliasDiskDrive or qemuAliasDiskDriveStr. Jan
On 08/01/2016 04:39 AM, Ján Tomko wrote:
On Tue, Jul 19, 2016 at 10:30:45AM -0400, John Ferlan wrote:
Move qemuDeviceDriveHostAlias from qemu_command and rename to qemuAssignDeviceDiskDriveAlias in qemu_alias. Also change the parameter to just be the disk->info.alias (e.g. srcalias)
Includes some innocent bystanders that seem to need QEMU_DRIVE_HOST_PREFIX which requires include qemu_alias.h instead of qemu_command.h
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_alias.c | 17 +++++++++++++++++ src/qemu/qemu_alias.h | 4 ++++ src/qemu/qemu_command.c | 11 ----------- src/qemu/qemu_command.h | 1 - src/qemu/qemu_hotplug.c | 4 ++-- src/qemu/qemu_migration.c | 2 +- src/qemu/qemu_monitor_json.c | 2 +- src/qemu/qemu_monitor_text.c | 2 +- 8 files changed, 26 insertions(+), 17 deletions(-)
diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c index 51a654a..70cd9f1 100644 --- a/src/qemu/qemu_alias.c +++ b/src/qemu/qemu_alias.c @@ -170,6 +170,23 @@ qemuAssignDeviceControllerAlias(virDomainDefPtr domainDef, return virAsprintf(&controller->info.alias, "%s%d", prefix, controller->idx); }
+/* qemuAssignDeviceDiskDriveAlias: + * @srcalias: Disk source alias + * + * Generate and return an alias for the device disk '-drive'
The other Assign* functions fill out the alias in device info. This one just returns the string.
Also, most of the functions in this file do not have a prefix derived from the file name, they start with qemuAssign instead of qemuAlias
I propose qemuAliasDiskDrive or qemuAliasDiskDriveStr.
OK... Interesting though - searching on qemuAlias in qemu_alias.c discovered qemuAliasFromDisk. So how about if I use qemuAliasFromDisk instead? Of course I'd replace the "drive-" in there with QEMU_DRIVE_HOST_PREFIX. Also, searching on \"drive- finds a function that open coded things: qemuDomainSnapshotCreateSingleDiskActive - it probably should have used qemuAliasFromDisk as well - I can send a separate patch on that. Finally, of course the one path that doesn't conform to the existing API and passes 'alias' in order to generate "drive-%s", alias is the qcow passphrase generation code... That's easily changed though and I can send a separate patch for that. John
Rather than pass the disks[i]->info.alias to qemuMonitorSetDrivePassphrase and then generate the "drive-%s" alias from that, let's use qemuAliasFromDisk prior to the call to generate the drive alias and then pass that along thus removing the need to generate the alias from the monitor code. Signed-off-by: John Ferlan <jferlan@redhat.com> --- Mainly so it's more apparent/transparent what I was planning to do. If you'd rather see the whole series again - that's not a problem... src/qemu/qemu_monitor_json.c | 7 +------ src/qemu/qemu_monitor_text.c | 3 +-- src/qemu/qemu_process.c | 2 +- tests/qemumonitorjsontest.c | 2 +- 4 files changed, 4 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 93233ea..2995264 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -3688,16 +3688,11 @@ int qemuMonitorJSONSetDrivePassphrase(qemuMonitorPtr mon, int ret = -1; virJSONValuePtr cmd; virJSONValuePtr reply = NULL; - char *drive; - - if (virAsprintf(&drive, "%s%s", QEMU_DRIVE_HOST_PREFIX, alias) < 0) - return -1; cmd = qemuMonitorJSONMakeCommand("block_passwd", - "s:device", drive, + "s:device", alias, "s:password", passphrase, NULL); - VIR_FREE(drive); if (!cmd) return -1; diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index 6c458e2..cf2979d 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -2008,8 +2008,7 @@ int qemuMonitorTextSetDrivePassphrase(qemuMonitorPtr mon, if (!safe_str) return -1; - if (virAsprintf(&cmd, "block_passwd %s%s \"%s\"", - QEMU_DRIVE_HOST_PREFIX, alias, safe_str) < 0) + if (virAsprintf(&cmd, "block_passwd %s \"%s\"", alias, safe_str) < 0) goto cleanup; if (qemuMonitorHMPCommand(mon, cmd, &reply) < 0) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 8e1b896..bcc5d92 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2476,7 +2476,7 @@ qemuProcessInitPasswords(virConnectPtr conn, goto cleanup; VIR_FREE(alias); - if (VIR_STRDUP(alias, vm->def->disks[i]->info.alias) < 0) + if (!(alias = qemuAliasFromDisk(vm->def->disks[i]))) goto cleanup; if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) goto cleanup; diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index f698c14..e8946c2 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -1190,7 +1190,7 @@ GEN_TEST_FUNC(qemuMonitorJSONAddNetdev, "some_dummy_netdevstr") GEN_TEST_FUNC(qemuMonitorJSONRemoveNetdev, "net0") GEN_TEST_FUNC(qemuMonitorJSONDelDevice, "ide0") GEN_TEST_FUNC(qemuMonitorJSONAddDevice, "some_dummy_devicestr") -GEN_TEST_FUNC(qemuMonitorJSONSetDrivePassphrase, "vda", "secret_passhprase") +GEN_TEST_FUNC(qemuMonitorJSONSetDrivePassphrase, "drive-vda", "secret_passhprase") GEN_TEST_FUNC(qemuMonitorJSONDriveMirror, "vdb", "/foo/bar", NULL, 1024, 0, 0, VIR_DOMAIN_BLOCK_REBASE_SHALLOW | VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT) GEN_TEST_FUNC(qemuMonitorJSONBlockCommit, "vdb", "/foo/bar1", "/foo/bar2", NULL, 1024) -- 2.7.4
On Mon, Aug 01, 2016 at 08:23:37AM -0400, John Ferlan wrote:
Rather than pass the disks[i]->info.alias to qemuMonitorSetDrivePassphrase and then generate the "drive-%s" alias from that, let's use qemuAliasFromDisk prior to the call to generate the drive alias and then pass that along thus removing the need to generate the alias from the monitor code.
Signed-off-by: John Ferlan <jferlan@redhat.com> ---
Mainly so it's more apparent/transparent what I was planning to do. If you'd rather see the whole series again - that's not a problem...
src/qemu/qemu_monitor_json.c | 7 +------ src/qemu/qemu_monitor_text.c | 3 +-- src/qemu/qemu_process.c | 2 +- tests/qemumonitorjsontest.c | 2 +- 4 files changed, 4 insertions(+), 10 deletions(-)
ACK Jan
The qemuDomainSnapshotCreateSingleDiskActive open coded generating the disk drive alias. Let's use the common function. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index abb777a..b94e6dc 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -13799,7 +13799,7 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, return -1; } - if (virAsprintf(&device, "drive-%s", disk->info.alias) < 0) + if (!(device = qemuAliasFromDisk(disk))) goto cleanup; if (!(newDiskSrc = virStorageSourceCopy(snap->src, false))) -- 2.7.4
On Mon, Aug 01, 2016 at 08:29:10AM -0400, John Ferlan wrote:
The qemuDomainSnapshotCreateSingleDiskActive open coded generating the disk drive alias. Let's use the common function.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ACK Jan
Since we already have a function that will generate the drivestr from the alias, let's use it and remove the qemuDeviceDriveHostAlias. Move the QEMU_DRIVE_HOST_PREFIX definition into qemu_alias.h. Also alter qemuAliasFromDisk to use the QEMU_DRIVE_HOST_PREFIX instead of "drive-%s". Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_alias.c | 13 +++++++++++-- src/qemu/qemu_alias.h | 2 ++ src/qemu/qemu_command.c | 11 ----------- src/qemu/qemu_command.h | 1 - src/qemu/qemu_hotplug.c | 4 ++-- src/qemu/qemu_migration.c | 2 +- src/qemu/qemu_monitor_json.c | 2 +- src/qemu/qemu_monitor_text.c | 2 +- 8 files changed, 18 insertions(+), 19 deletions(-) diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c index 51a654a..f263586 100644 --- a/src/qemu/qemu_alias.c +++ b/src/qemu/qemu_alias.c @@ -451,17 +451,26 @@ qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps) } +/* qemuAliasFromDisk + * @disk: Pointer to a disk definition + * + * Generate and return an alias for the device disk '-drive' + * + * Returns NULL with error or a string containing the alias + */ char * qemuAliasFromDisk(const virDomainDiskDef *disk) { char *ret; if (!disk->info.alias) { - virReportError(VIR_ERR_INVALID_ARG, "%s", _("disk does not have an alias")); + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("disk does not have an alias")); return NULL; } - ignore_value(virAsprintf(&ret, "drive-%s", disk->info.alias)); + ignore_value(virAsprintf(&ret, "%s%s", QEMU_DRIVE_HOST_PREFIX, + disk->info.alias)); return ret; } diff --git a/src/qemu/qemu_alias.h b/src/qemu/qemu_alias.h index d1c6ba8..5202f73 100644 --- a/src/qemu/qemu_alias.h +++ b/src/qemu/qemu_alias.h @@ -30,6 +30,8 @@ # include "qemu_domain.h" # include "qemu_domain_address.h" +# define QEMU_DRIVE_HOST_PREFIX "drive-" + int qemuAssignDeviceChrAlias(virDomainDefPtr def, virDomainChrDefPtr chr, ssize_t idx); diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 5325f48..5141612 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -272,17 +272,6 @@ qemuVirCommandGetDevSet(virCommandPtr cmd, int fd) } -char *qemuDeviceDriveHostAlias(virDomainDiskDefPtr disk) -{ - char *ret; - - if (virAsprintf(&ret, "%s%s", - QEMU_DRIVE_HOST_PREFIX, disk->info.alias) < 0) - return NULL; - return ret; -} - - static int qemuBuildDeviceAddressStr(virBufferPtr buf, const virDomainDef *domainDef, diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index c4d0567..dcf9ba6 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -37,7 +37,6 @@ /* Config type for XML import/export conversions */ # define QEMU_CONFIG_FORMAT_ARGV "qemu-argv" -# define QEMU_DRIVE_HOST_PREFIX "drive-" # define QEMU_FSDEV_HOST_PREFIX "fsdev-" # define QEMU_BLOCK_IOTUNE_MAX 1000000000000000LL diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index fd365de..356741f 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -229,7 +229,7 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, if (qemuDomainPrepareDisk(driver, vm, disk, newsrc, false) < 0) goto cleanup; - if (!(driveAlias = qemuDeviceDriveHostAlias(disk))) + if (!(driveAlias = qemuAliasFromDisk(disk))) goto error; qemuDomainObjEnterMonitor(driver, vm); @@ -369,7 +369,7 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, if (!(drivestr = qemuBuildDriveStr(disk, false, priv->qemuCaps))) goto error; - if (!(drivealias = qemuDeviceDriveHostAlias(disk))) + if (!(drivealias = qemuAliasFromDisk(disk))) goto error; if (!(devstr = qemuBuildDriveDevStr(vm->def, disk, 0, priv->qemuCaps))) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 463e624..04c847e 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -36,7 +36,7 @@ #include "qemu_domain.h" #include "qemu_process.h" #include "qemu_capabilities.h" -#include "qemu_command.h" +#include "qemu_alias.h" #include "qemu_cgroup.h" #include "qemu_hotplug.h" #include "qemu_blockjob.h" diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index a415e59..2995264 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -33,7 +33,7 @@ #include "qemu_monitor_text.h" #include "qemu_monitor_json.h" -#include "qemu_command.h" +#include "qemu_alias.h" #include "qemu_parse_command.h" #include "qemu_capabilities.h" #include "viralloc.h" diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index 00310d9..cf2979d 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -31,7 +31,7 @@ #include <string.h> #include "qemu_monitor_text.h" -#include "qemu_command.h" +#include "qemu_alias.h" #include "c-ctype.h" #include "c-strcasestr.h" #include "viralloc.h" -- 2.7.4
On Mon, Aug 01, 2016 at 08:48:27AM -0400, John Ferlan wrote:
Since we already have a function that will generate the drivestr from the alias, let's use it and remove the qemuDeviceDriveHostAlias.
Move the QEMU_DRIVE_HOST_PREFIX definition into qemu_alias.h.
Also alter qemuAliasFromDisk to use the QEMU_DRIVE_HOST_PREFIX instead of "drive-%s".
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_alias.c | 13 +++++++++++-- src/qemu/qemu_alias.h | 2 ++ src/qemu/qemu_command.c | 11 ----------- src/qemu/qemu_command.h | 1 - src/qemu/qemu_hotplug.c | 4 ++-- src/qemu/qemu_migration.c | 2 +- src/qemu/qemu_monitor_json.c | 2 +- src/qemu/qemu_monitor_text.c | 2 +- 8 files changed, 18 insertions(+), 19 deletions(-)
ACK Jan
Rather than open code build the drive alias command in multiple places, use the helper to ensure consistency. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 12 ++++++++++-- src/qemu/qemu_driver.c | 3 +-- src/qemu/qemu_hotplug.c | 3 +-- src/qemu/qemu_migration.c | 9 +++------ src/qemu/qemu_monitor_json.c | 2 +- src/qemu/qemu_monitor_text.c | 8 ++++++-- 6 files changed, 22 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index affd0b0..482f993 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1245,7 +1245,11 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk, } if (emitDeviceSyntax) { - virBufferAsprintf(&opt, ",id=%s%s", QEMU_DRIVE_HOST_PREFIX, disk->info.alias); + char *drivealias = qemuAssignDeviceDiskDriveAlias(disk->info.alias); + if (!drivealias) + goto error; + virBufferAsprintf(&opt, ",id=%s", drivealias); + VIR_FREE(drivealias); } else { if (busid == -1 && unitid == -1) { if (idx != -1) @@ -1607,6 +1611,7 @@ qemuBuildDriveDevStr(const virDomainDef *def, virBuffer opt = VIR_BUFFER_INITIALIZER; const char *bus = virDomainDiskQEMUBusTypeToString(disk->bus); const char *contAlias; + char *drivealias; int controllerModel; if (qemuCheckDiskConfig(disk) < 0) @@ -1832,7 +1837,10 @@ qemuBuildDriveDevStr(const virDomainDef *def, goto error; } - virBufferAsprintf(&opt, ",drive=%s%s", QEMU_DRIVE_HOST_PREFIX, disk->info.alias); + if (!(drivealias = qemuAssignDeviceDiskDriveAlias(disk->info.alias))) + goto error; + virBufferAsprintf(&opt, ",drive=%s", drivealias); + VIR_FREE(drivealias); virBufferAsprintf(&opt, ",id=%s", disk->info.alias); if (bootindex && virQEMUCapsGet(qemuCaps, QEMU_CAPS_BOOTINDEX)) virBufferAsprintf(&opt, ",bootindex=%u", bootindex); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index cda85f6..a152efc 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10356,8 +10356,7 @@ qemuDomainBlockResize(virDomainPtr dom, disk->src->format == VIR_STORAGE_FILE_QED) size = VIR_ROUND_UP(size, 512); - if (virAsprintf(&device, "%s%s", QEMU_DRIVE_HOST_PREFIX, - disk->info.alias) < 0) + if (!(device = qemuAssignDeviceDiskDriveAlias(disk->info.alias))) goto endjob; qemuDomainObjEnterMonitor(driver, vm); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 4f521a4..1b6e21e 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2917,8 +2917,7 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, /* build the actual drive id string as the disk->info.alias doesn't * contain the QEMU_DRIVE_HOST_PREFIX that is passed to qemu */ - if (virAsprintf(&drivestr, "%s%s", - QEMU_DRIVE_HOST_PREFIX, disk->info.alias) < 0) + if (!(drivestr = qemuAssignDeviceDiskDriveAlias(disk->info.alias))) return -1; /* Let's look for some markers for a secret object and create an alias diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 04c847e..31d5e46 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1761,8 +1761,7 @@ qemuMigrationStartNBDServer(virQEMUDriverPtr driver, continue; VIR_FREE(diskAlias); - if (virAsprintf(&diskAlias, "%s%s", - QEMU_DRIVE_HOST_PREFIX, disk->info.alias) < 0) + if (!(diskAlias = qemuAssignDeviceDiskDriveAlias(disk->info.alias))) goto cleanup; if (qemuDomainObjEnterMonitorAsync(driver, vm, @@ -1978,8 +1977,7 @@ qemuMigrationCancelOneDriveMirror(virQEMUDriverPtr driver, return 1; } - if (virAsprintf(&diskAlias, "%s%s", - QEMU_DRIVE_HOST_PREFIX, disk->info.alias) < 0) + if (!(diskAlias = qemuAssignDeviceDiskDriveAlias(disk->info.alias))) return -1; if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) @@ -2154,8 +2152,7 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver, if (!qemuMigrateDisk(disk, nmigrate_disks, migrate_disks)) continue; - if ((virAsprintf(&diskAlias, "%s%s", - QEMU_DRIVE_HOST_PREFIX, disk->info.alias) < 0) || + if (!(diskAlias = qemuAssignDeviceDiskDriveAlias(disk->info.alias)) || (virAsprintf(&nbd_dest, "nbd:%s:%d:exportname=%s", hoststr, port, diskAlias) < 0)) goto cleanup; diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index a54ff8d..633fb2a 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -3686,7 +3686,7 @@ int qemuMonitorJSONSetDrivePassphrase(qemuMonitorPtr mon, virJSONValuePtr reply = NULL; char *drive; - if (virAsprintf(&drive, "%s%s", QEMU_DRIVE_HOST_PREFIX, alias) < 0) + if (!(drive = qemuAssignDeviceDiskDriveAlias(alias))) return -1; cmd = qemuMonitorJSONMakeCommand("block_passwd", diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index 6c458e2..ff92bb1 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -2001,6 +2001,7 @@ int qemuMonitorTextSetDrivePassphrase(qemuMonitorPtr mon, { char *cmd = NULL; char *reply = NULL; + char *drivealias = NULL; int ret = -1; char *safe_str; @@ -2008,8 +2009,10 @@ int qemuMonitorTextSetDrivePassphrase(qemuMonitorPtr mon, if (!safe_str) return -1; - if (virAsprintf(&cmd, "block_passwd %s%s \"%s\"", - QEMU_DRIVE_HOST_PREFIX, alias, safe_str) < 0) + if (!(drivealias = qemuAssignDeviceDiskDriveAlias(alias))) + goto cleanup; + + if (virAsprintf(&cmd, "block_passwd %s \"%s\"", drivealias, safe_str) < 0) goto cleanup; if (qemuMonitorHMPCommand(mon, cmd, &reply) < 0) @@ -2030,6 +2033,7 @@ int qemuMonitorTextSetDrivePassphrase(qemuMonitorPtr mon, cleanup: VIR_FREE(cmd); VIR_FREE(reply); + VIR_FREE(drivealias); VIR_FREE(safe_str); return ret; } -- 2.5.5
On Tue, Jul 19, 2016 at 10:30:46AM -0400, John Ferlan wrote:
Rather than open code build the drive alias command in multiple places, use the helper to ensure consistency.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 12 ++++++++++-- src/qemu/qemu_driver.c | 3 +-- src/qemu/qemu_hotplug.c | 3 +-- src/qemu/qemu_migration.c | 9 +++------ src/qemu/qemu_monitor_json.c | 2 +- src/qemu/qemu_monitor_text.c | 8 ++++++-- 6 files changed, 22 insertions(+), 15 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index affd0b0..482f993 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1245,7 +1245,11 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk, }
if (emitDeviceSyntax) { - virBufferAsprintf(&opt, ",id=%s%s", QEMU_DRIVE_HOST_PREFIX, disk->info.alias); + char *drivealias = qemuAssignDeviceDiskDriveAlias(disk->info.alias); + if (!drivealias) + goto error; + virBufferAsprintf(&opt, ",id=%s", drivealias); + VIR_FREE(drivealias);
Creating a separate 'qemuAliasDiskDriveBuf' would get rid of the extra free and error handling. On the other hand, this way both 'id' and its value are on the same line. Jan
Move QEMU_DRIVE_HOST_PREFIX into the qemu_alias.c to disuade future callers from using it. Create qemuAliasDeviceDiskDriveSkipPrefix in order to handle the current consumers that desire to check if an alias has the drive- prefix and "get beyond it" in order to get the disk alias. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_alias.c | 17 +++++++++++++++++ src/qemu/qemu_alias.h | 4 ++-- src/qemu/qemu_domain.c | 3 +-- src/qemu/qemu_monitor_json.c | 6 ++---- src/qemu/qemu_monitor_text.c | 8 +++----- src/qemu/qemu_process.c | 3 +-- 6 files changed, 26 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c index 70cd9f1..d80537b 100644 --- a/src/qemu/qemu_alias.c +++ b/src/qemu/qemu_alias.c @@ -29,6 +29,8 @@ #include "virstring.h" #include "network/bridge_driver.h" +#define QEMU_DRIVE_HOST_PREFIX "drive-" + #define VIR_FROM_THIS VIR_FROM_QEMU VIR_LOG_INIT("qemu.qemu_alias"); @@ -188,6 +190,21 @@ qemuAssignDeviceDiskDriveAlias(const char *srcalias) } +/* qemuAliasDeviceDiskDriveSkipPrefix: + * @dev_name: Pointer to a const char string + * + * If the QEMU_DRIVE_HOST_PREFIX exists in the input string, then + * increment the pointer and return it + */ +const char * +qemuAliasDeviceDiskDriveSkipPrefix(const char *dev_name) +{ + if (STRPREFIX(dev_name, QEMU_DRIVE_HOST_PREFIX)) + dev_name += strlen(QEMU_DRIVE_HOST_PREFIX); + return dev_name; +} + + /* Our custom -drive naming scheme used with id= */ int qemuAssignDeviceDiskAlias(virDomainDefPtr def, diff --git a/src/qemu/qemu_alias.h b/src/qemu/qemu_alias.h index db4b690..b2acea5 100644 --- a/src/qemu/qemu_alias.h +++ b/src/qemu/qemu_alias.h @@ -30,8 +30,6 @@ # include "qemu_domain.h" # include "qemu_domain_address.h" -# define QEMU_DRIVE_HOST_PREFIX "drive-" - int qemuAssignDeviceChrAlias(virDomainDefPtr def, virDomainChrDefPtr chr, ssize_t idx); @@ -42,6 +40,8 @@ int qemuAssignDeviceControllerAlias(virDomainDefPtr domainDef, char *qemuAssignDeviceDiskDriveAlias(const char *srcalias); +const char *qemuAliasDeviceDiskDriveSkipPrefix(const char *dev_name); + int qemuAssignDeviceDiskAlias(virDomainDefPtr vmdef, virDomainDiskDefPtr def, virQEMUCapsPtr qemuCaps); diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 6372080..0286dd8 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4369,8 +4369,7 @@ qemuDomainStorageAlias(const char *device, int depth) { char *alias; - if (STRPREFIX(device, QEMU_DRIVE_HOST_PREFIX)) - device += strlen(QEMU_DRIVE_HOST_PREFIX); + device = qemuAliasDeviceDiskDriveSkipPrefix(device); if (!depth) ignore_value(VIR_STRDUP(alias, device)); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 633fb2a..4be4c6e 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -1796,8 +1796,7 @@ int qemuMonitorJSONGetBlockInfo(qemuMonitorPtr mon, goto cleanup; } - if (STRPREFIX(thisdev, QEMU_DRIVE_HOST_PREFIX)) - thisdev += strlen(QEMU_DRIVE_HOST_PREFIX); + thisdev = qemuAliasDeviceDiskDriveSkipPrefix(thisdev); if (VIR_ALLOC(info) < 0) goto cleanup; @@ -4196,8 +4195,7 @@ qemuMonitorJSONParseBlockJobInfo(virHashTablePtr blockJobs, _("entry was missing 'device'")); return -1; } - if (STRPREFIX(device, QEMU_DRIVE_HOST_PREFIX)) - device += strlen(QEMU_DRIVE_HOST_PREFIX); + device = qemuAliasDeviceDiskDriveSkipPrefix(device); if (VIR_ALLOC(info) < 0 || virHashAddEntry(blockJobs, device, info) < 0) { diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index ff92bb1..b924ce6 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -753,8 +753,7 @@ int qemuMonitorTextGetBlockInfo(qemuMonitorPtr mon, p = reply; while (*p) { - if (STRPREFIX(p, QEMU_DRIVE_HOST_PREFIX)) - p += strlen(QEMU_DRIVE_HOST_PREFIX); + p = (char *)qemuAliasDeviceDiskDriveSkipPrefix(p); eol = strchr(p, '\n'); if (!eol) @@ -839,7 +838,7 @@ qemuMonitorTextGetAllBlockStatsInfo(qemuMonitorPtr mon, { qemuBlockStatsPtr stats = NULL; char *info = NULL; - char *dev_name; + const char *dev_name; char **lines = NULL; char **values = NULL; char *line; @@ -901,8 +900,7 @@ qemuMonitorTextGetAllBlockStatsInfo(qemuMonitorPtr mon, *line = '\0'; line += 2; - if (STRPREFIX(dev_name, QEMU_DRIVE_HOST_PREFIX)) - dev_name += strlen(QEMU_DRIVE_HOST_PREFIX); + dev_name = qemuAliasDeviceDiskDriveSkipPrefix(dev_name); if (!(values = virStringSplit(line, " ", 0))) goto cleanup; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 4adb14e..6c0499d 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -355,8 +355,7 @@ qemuProcessFindDomainDiskByAlias(virDomainObjPtr vm, { size_t i; - if (STRPREFIX(alias, QEMU_DRIVE_HOST_PREFIX)) - alias += strlen(QEMU_DRIVE_HOST_PREFIX); + alias = qemuAliasDeviceDiskDriveSkipPrefix(alias); for (i = 0; i < vm->def->ndisks; i++) { virDomainDiskDefPtr disk; -- 2.5.5
On Tue, Jul 19, 2016 at 10:30:47AM -0400, John Ferlan wrote:
Move QEMU_DRIVE_HOST_PREFIX into the qemu_alias.c to disuade future
s/disuade/dissuade/ disuade is the Spanish spelling. I know that because I had to look it up in the dictionary anyway.
callers from using it. Create qemuAliasDeviceDiskDriveSkipPrefix in order to handle the current consumers that desire to check if an alias has the drive- prefix and "get beyond it" in order to get the disk alias.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_alias.c | 17 +++++++++++++++++ src/qemu/qemu_alias.h | 4 ++-- src/qemu/qemu_domain.c | 3 +-- src/qemu/qemu_monitor_json.c | 6 ++---- src/qemu/qemu_monitor_text.c | 8 +++----- src/qemu/qemu_process.c | 3 +-- 6 files changed, 26 insertions(+), 15 deletions(-)
diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c index 70cd9f1..d80537b 100644 --- a/src/qemu/qemu_alias.c +++ b/src/qemu/qemu_alias.c @@ -29,6 +29,8 @@ #include "virstring.h" #include "network/bridge_driver.h"
+#define QEMU_DRIVE_HOST_PREFIX "drive-" + #define VIR_FROM_THIS VIR_FROM_QEMU
VIR_LOG_INIT("qemu.qemu_alias"); @@ -188,6 +190,21 @@ qemuAssignDeviceDiskDriveAlias(const char *srcalias) }
+/* qemuAliasDeviceDiskDriveSkipPrefix: + * @dev_name: Pointer to a const char string + * + * If the QEMU_DRIVE_HOST_PREFIX exists in the input string, then + * increment the pointer and return it + */ +const char * +qemuAliasDeviceDiskDriveSkipPrefix(const char *dev_name)
I think you can drop the Device without hurting readability. Jan
Partial fix for: https://bugzilla.redhat.com/show_bug.cgi?id=1336225 Similar to the other disk types, add the qemuMonitorDriveDel in the failure to add/hotplug a USB. Added a couple of other formatting changes just to have a less cluttered look Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_hotplug.c | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 1b6e21e..47ca38c 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -706,7 +706,9 @@ qemuDomainAttachUSBMassStorageDevice(virQEMUDriverPtr driver, virDomainDiskDefPtr disk) { qemuDomainObjPrivatePtr priv = vm->privateData; + virErrorPtr orig_err; int ret = -1; + char *drivealias = NULL; char *drivestr = NULL; char *devstr = NULL; bool driveAdded = false; @@ -725,8 +727,13 @@ qemuDomainAttachUSBMassStorageDevice(virQEMUDriverPtr driver, if (qemuAssignDeviceDiskAlias(vm->def, disk, priv->qemuCaps) < 0) goto error; + if (!(drivestr = qemuBuildDriveStr(disk, false, priv->qemuCaps))) goto error; + + if (!(drivealias = qemuAssignDeviceDiskDriveAlias(disk->info.alias))) + goto error; + if (!(devstr = qemuBuildDriveDevStr(vm->def, disk, 0, priv->qemuCaps))) goto error; @@ -752,16 +759,20 @@ qemuDomainAttachUSBMassStorageDevice(virQEMUDriverPtr driver, cleanup: VIR_FREE(devstr); + VIR_FREE(drivealias); VIR_FREE(drivestr); virObjectUnref(cfg); return ret; exit_monitor: - if (driveAdded) { - VIR_WARN("qemuMonitorAddDevice failed on %s (%s)", - drivestr, devstr); - /* XXX should call 'drive_del' on error but this does not - exist yet */ + orig_err = virSaveLastError(); + if (driveAdded && qemuMonitorDriveDel(priv->mon, drivealias) < 0) { + VIR_WARN("Unable to remove drive %s (%s) after failed " + "qemuMonitorAddDevice", drivealias, drivestr); + } + if (orig_err) { + virSetError(orig_err); + virFreeError(orig_err); } ignore_value(qemuDomainObjExitMonitor(driver, vm)); -- 2.5.5
Completion of fix for: https://bugzilla.redhat.com/show_bug.cgi?id=1336225 Similar to the other disk types, add the qemuMonitorDriveDel in the failure to add/hotplug a SCSI disk. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_hotplug.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 47ca38c..9576509 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -593,6 +593,7 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn, char *devstr = NULL; bool driveAdded = false; bool encobjAdded = false; + char *drivealias = NULL; int ret = -1; int rv; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); @@ -640,6 +641,9 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn, if (!(drivestr = qemuBuildDriveStr(disk, false, priv->qemuCaps))) goto error; + if (!(drivealias = qemuAssignDeviceDiskDriveAlias(disk->info.alias))) + goto error; + if (VIR_REALLOC_N(vm->def->disks, vm->def->ndisks+1) < 0) goto error; @@ -674,13 +678,20 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn, qemuDomainSecretDiskDestroy(disk); VIR_FREE(devstr); VIR_FREE(drivestr); + VIR_FREE(drivealias); virObjectUnref(cfg); return ret; exit_monitor: - /* XXX should call 'drive_del' on error but this does not exist yet */ - if (driveAdded) - VIR_WARN("qemuMonitorAddDevice failed on %s (%s)", drivestr, devstr); + orig_err = virSaveLastError(); + if (driveAdded && qemuMonitorDriveDel(priv->mon, drivealias) < 0) { + VIR_WARN("Unable to remove drive %s (%s) after failed " + "qemuMonitorAddDevice", drivealias, drivestr); + } + if (orig_err) { + virSetError(orig_err); + virFreeError(orig_err); + } orig_err = virSaveLastError(); if (encobjAdded) -- 2.5.5
On Tue, Jul 19, 2016 at 10:30:49AM -0400, John Ferlan wrote:
Completion of fix for: https://bugzilla.redhat.com/show_bug.cgi?id=1336225
Similar to the other disk types, add the qemuMonitorDriveDel in the failure to add/hotplug a SCSI disk.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_hotplug.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)
exit_monitor: - /* XXX should call 'drive_del' on error but this does not exist yet */ - if (driveAdded) - VIR_WARN("qemuMonitorAddDevice failed on %s (%s)", drivestr, devstr); + orig_err = virSaveLastError(); + if (driveAdded && qemuMonitorDriveDel(priv->mon, drivealias) < 0) { + VIR_WARN("Unable to remove drive %s (%s) after failed " + "qemuMonitorAddDevice", drivealias, drivestr); + }
+ if (orig_err) { + virSetError(orig_err); + virFreeError(orig_err); + }
orig_err = virSaveLastError();
This can be dropped. The block below also needs to preserve the original error. Jan
if (encobjAdded) -- 2.5.5
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Introduce a common API to generate the alias for a SCSI Host device Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_alias.c | 18 ++++++++++++++++++ src/qemu/qemu_alias.h | 2 ++ 2 files changed, 20 insertions(+) diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c index d80537b..d8365ea 100644 --- a/src/qemu/qemu_alias.c +++ b/src/qemu/qemu_alias.c @@ -190,6 +190,24 @@ qemuAssignDeviceDiskDriveAlias(const char *srcalias) } +/* qemuAssignSCSIHostDeviceDriveAlias(const char *srcalias) + * @hostdev: Pointer to host device + * + * Generate and return a string containing a drive alias + */ +char * +qemuAssignSCSIHostDeviceDriveAlias(virDomainHostdevDefPtr hostdev) +{ + char *ret; + + if (virAsprintf(&ret, "%s-%s", + virDomainDeviceAddressTypeToString(hostdev->info->type), + hostdev->info->alias) < 0) + return NULL; + return ret; +} + + /* qemuAliasDeviceDiskDriveSkipPrefix: * @dev_name: Pointer to a const char string * diff --git a/src/qemu/qemu_alias.h b/src/qemu/qemu_alias.h index b2acea5..d5d6fb5 100644 --- a/src/qemu/qemu_alias.h +++ b/src/qemu/qemu_alias.h @@ -40,6 +40,8 @@ int qemuAssignDeviceControllerAlias(virDomainDefPtr domainDef, char *qemuAssignDeviceDiskDriveAlias(const char *srcalias); +char *qemuAssignSCSIHostDeviceDriveAlias(virDomainHostdevDefPtr hostdev); + const char *qemuAliasDeviceDiskDriveSkipPrefix(const char *dev_name); int qemuAssignDeviceDiskAlias(virDomainDefPtr vmdef, -- 2.5.5
On 07/19/2016 10:30 AM, John Ferlan wrote:
Introduce a common API to generate the alias for a SCSI Host device
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_alias.c | 18 ++++++++++++++++++ src/qemu/qemu_alias.h | 2 ++ 2 files changed, 20 insertions(+)
Considering the other changes and for consistency, changing this to qemuAliasFromHostdev (to mimic qemuAliasFromDisk) seems appropriate. John
diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c index d80537b..d8365ea 100644 --- a/src/qemu/qemu_alias.c +++ b/src/qemu/qemu_alias.c @@ -190,6 +190,24 @@ qemuAssignDeviceDiskDriveAlias(const char *srcalias) }
+/* qemuAssignSCSIHostDeviceDriveAlias(const char *srcalias) + * @hostdev: Pointer to host device + * + * Generate and return a string containing a drive alias + */ +char * +qemuAssignSCSIHostDeviceDriveAlias(virDomainHostdevDefPtr hostdev) +{ + char *ret; + + if (virAsprintf(&ret, "%s-%s", + virDomainDeviceAddressTypeToString(hostdev->info->type), + hostdev->info->alias) < 0) + return NULL; + return ret; +} + + /* qemuAliasDeviceDiskDriveSkipPrefix: * @dev_name: Pointer to a const char string * diff --git a/src/qemu/qemu_alias.h b/src/qemu/qemu_alias.h index b2acea5..d5d6fb5 100644 --- a/src/qemu/qemu_alias.h +++ b/src/qemu/qemu_alias.h @@ -40,6 +40,8 @@ int qemuAssignDeviceControllerAlias(virDomainDefPtr domainDef,
char *qemuAssignDeviceDiskDriveAlias(const char *srcalias);
+char *qemuAssignSCSIHostDeviceDriveAlias(virDomainHostdevDefPtr hostdev); + const char *qemuAliasDeviceDiskDriveSkipPrefix(const char *dev_name);
int qemuAssignDeviceDiskAlias(virDomainDefPtr vmdef,
When building the command line alias and for SCSI Host Device deletion, use the common API to build the alias Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 12 +++++++----- src/qemu/qemu_hotplug.c | 12 ++++-------- 2 files changed, 11 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 482f993..0c4b1f5 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4534,6 +4534,7 @@ qemuBuildSCSIHostdevDrvStr(virDomainHostdevDefPtr dev) { virBuffer buf = VIR_BUFFER_INITIALIZER; char *source = NULL; + char *drivealias = NULL; virDomainHostdevSubsysSCSIPtr scsisrc = &dev->source.subsys.u.scsi; if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) { @@ -4545,9 +4546,12 @@ qemuBuildSCSIHostdevDrvStr(virDomainHostdevDefPtr dev) goto error; virBufferAsprintf(&buf, "file=/dev/%s,if=none", source); } - virBufferAsprintf(&buf, ",id=%s-%s", - virDomainDeviceAddressTypeToString(dev->info->type), - dev->info->alias); + VIR_FREE(source); + + if (!(drivealias = qemuAssignSCSIHostDeviceDriveAlias(dev))) + goto error; + virBufferAsprintf(&buf, ",id=%s", drivealias); + VIR_FREE(drivealias); if (dev->readonly) virBufferAddLit(&buf, ",readonly=on"); @@ -4555,10 +4559,8 @@ qemuBuildSCSIHostdevDrvStr(virDomainHostdevDefPtr dev) if (virBufferCheckError(&buf) < 0) goto error; - VIR_FREE(source); return virBufferContentAndReset(&buf); error: - VIR_FREE(source); virBufferFreeAndReset(&buf); return NULL; } diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 9576509..4d3e913 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3132,7 +3132,7 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver, size_t i; int ret = -1; qemuDomainObjPrivatePtr priv = vm->privateData; - char *drivestr = NULL; + char *drivealias = NULL; bool is_vfio = false; VIR_DEBUG("Removing host device %s from domain %p %s", @@ -3144,15 +3144,11 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver, } if (hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) { - /* build the actual drive id string as generated during - * qemuBuildSCSIHostdevDrvStr that is passed to qemu */ - if (virAsprintf(&drivestr, "%s-%s", - virDomainDeviceAddressTypeToString(hostdev->info->type), - hostdev->info->alias) < 0) + if (!(drivealias = qemuAssignSCSIHostDeviceDriveAlias(hostdev))) goto cleanup; qemuDomainObjEnterMonitor(driver, vm); - qemuMonitorDriveDel(priv->mon, drivestr); + qemuMonitorDriveDel(priv->mon, drivealias); if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup; } @@ -3216,7 +3212,7 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver, ret = 0; cleanup: - VIR_FREE(drivestr); + VIR_FREE(drivealias); virObjectUnref(cfg); return ret; } -- 2.5.5
On 07/19/2016 10:30 AM, John Ferlan wrote:
When building the command line alias and for SCSI Host Device deletion, use the common API to build the alias
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 12 +++++++----- src/qemu/qemu_hotplug.c | 12 ++++-------- 2 files changed, 11 insertions(+), 13 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 482f993..0c4b1f5 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4534,6 +4534,7 @@ qemuBuildSCSIHostdevDrvStr(virDomainHostdevDefPtr dev) { virBuffer buf = VIR_BUFFER_INITIALIZER; char *source = NULL; + char *drivealias = NULL; virDomainHostdevSubsysSCSIPtr scsisrc = &dev->source.subsys.u.scsi;
if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) { @@ -4545,9 +4546,12 @@ qemuBuildSCSIHostdevDrvStr(virDomainHostdevDefPtr dev) goto error; virBufferAsprintf(&buf, "file=/dev/%s,if=none", source); } - virBufferAsprintf(&buf, ",id=%s-%s", - virDomainDeviceAddressTypeToString(dev->info->type), - dev->info->alias); + VIR_FREE(source); + + if (!(drivealias = qemuAssignSCSIHostDeviceDriveAlias(dev))) + goto error; + virBufferAsprintf(&buf, ",id=%s", drivealias); + VIR_FREE(drivealias);
if (dev->readonly) virBufferAddLit(&buf, ",readonly=on"); @@ -4555,10 +4559,8 @@ qemuBuildSCSIHostdevDrvStr(virDomainHostdevDefPtr dev) if (virBufferCheckError(&buf) < 0) goto error;
- VIR_FREE(source); return virBufferContentAndReset(&buf); error: - VIR_FREE(source); virBufferFreeAndReset(&buf); return NULL; } diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 9576509..4d3e913 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3132,7 +3132,7 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver, size_t i; int ret = -1; qemuDomainObjPrivatePtr priv = vm->privateData; - char *drivestr = NULL; + char *drivealias = NULL; bool is_vfio = false;
VIR_DEBUG("Removing host device %s from domain %p %s", @@ -3144,15 +3144,11 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver, }
if (hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) { - /* build the actual drive id string as generated during - * qemuBuildSCSIHostdevDrvStr that is passed to qemu */ - if (virAsprintf(&drivestr, "%s-%s", - virDomainDeviceAddressTypeToString(hostdev->info->type), - hostdev->info->alias) < 0) + if (!(drivealias = qemuAssignSCSIHostDeviceDriveAlias(hostdev))) goto cleanup;
qemuDomainObjEnterMonitor(driver, vm); - qemuMonitorDriveDel(priv->mon, drivestr); + qemuMonitorDriveDel(priv->mon, drivealias); if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup; } @@ -3216,7 +3212,7 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver, ret = 0;
cleanup: - VIR_FREE(drivestr); + VIR_FREE(drivealias); virObjectUnref(cfg); return ret; }
Beyond changing the function name, squash the following in too: diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 99c41c8..fb0fb46 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4749,6 +4749,7 @@ qemuBuildSCSIHostdevDevStr(const virDomainDef *def, { virBuffer buf = VIR_BUFFER_INITIALIZER; int model = -1; + char *driveAlias; const char *contAlias; model = virDomainDeviceFindControllerModel(def, dev->info, @@ -4792,9 +4793,10 @@ qemuBuildSCSIHostdevDevStr(const virDomainDef *def, dev->info->addr.drive.unit); } - virBufferAsprintf(&buf, ",drive=%s-%s,id=%s", - virDomainDeviceAddressTypeToString(dev->info->type), - dev->info->alias, dev->info->alias); + if (!(driveAlias = qemuAliasFromHostdev(dev))) + goto error; + virBufferAsprintf(&buf, ",drive=%s,id=%s", driveAlias, dev->info->alias); + VIR_FREE(driveAlias); if (dev->info->bootIndex) virBufferAsprintf(&buf, ",bootindex=%u", dev->info->bootIndex);
Rather than pass the whole drive string (which contained the alias), pass only the alias for the qemuMonitorDriveDel call in the error path when adding a host device in the monitor fails. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_hotplug.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 4d3e913..eaf4e46 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1996,6 +1996,7 @@ qemuDomainAttachHostSCSIDevice(virConnectPtr conn, virErrorPtr orig_err; char *devstr = NULL; char *drvstr = NULL; + char *drivealias = NULL; bool teardowncgroup = false; bool teardownlabel = false; bool driveAdded = false; @@ -2054,6 +2055,9 @@ qemuDomainAttachHostSCSIDevice(virConnectPtr conn, if (!(drvstr = qemuBuildSCSIHostdevDrvStr(hostdev))) goto cleanup; + if (!(drivealias = qemuAssignSCSIHostDeviceDriveAlias(hostdev))) + goto cleanup; + if (!(devstr = qemuBuildSCSIHostdevDevStr(vm->def, hostdev, priv->qemuCaps))) goto cleanup; @@ -2089,13 +2093,14 @@ qemuDomainAttachHostSCSIDevice(virConnectPtr conn, vm->def, hostdev, NULL) < 0) VIR_WARN("Unable to restore host device labelling on hotplug fail"); } + VIR_FREE(drivealias); VIR_FREE(drvstr); VIR_FREE(devstr); return ret; exit_monitor: orig_err = virSaveLastError(); - if (driveAdded && qemuMonitorDriveDel(priv->mon, drvstr) < 0) { + if (driveAdded && qemuMonitorDriveDel(priv->mon, drivealias) < 0) { VIR_WARN("Unable to remove drive %s (%s) after failed " "qemuMonitorAddDevice", drvstr, devstr); -- 2.5.5
ping Update: Patches 7-9 do resolve bz 1289391 Tks - John On 07/19/2016 10:30 AM, John Ferlan wrote:
v1: http://www.redhat.com/archives/libvir-list/2016-June/msg02207.html
Changes since v1... - Use the bz noted by Cole from the v1 series - Merged in recent hotplug cleanup changes - Patches 6 - 9 are new
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1336225
and probably: https://bugzilla.redhat.com/show_bug.cgi?id=1289391
Patches 1-5 address the USB hotplug cleanup
Patch 6 addresses the SCSI hotplug cleanup
Patch 7-9 address some SCSI host device cleanups found while looking at the other cleanups...
John Ferlan (9): qemu: Reorder qemuDomainAttachUSBMassStorageDevice failure path qemu: Move drive alias processing to qemu_alias qemu: Use qemuAssignDeviceDiskDriveAlias qemu: Make QEMU_DRIVE_HOST_PREFIX more private qemu: Add attempt to call qemuMonitorDriveDel for USB failure path qemu: Add attempt to call qemuMonitorDriveDel for AttachSCSI failure path qemu: Introduce qemuAssignSCSIHostDeviceDriveAlias qemu: Use qemuAssignSCSIHostDeviceDriveAlias qemu: Use the hostdev alias in qemuDomainAttachHostSCSIDevice error path
src/qemu/qemu_alias.c | 52 +++++++++++++++++++++++++ src/qemu/qemu_alias.h | 6 +++ src/qemu/qemu_command.c | 35 ++++++++--------- src/qemu/qemu_command.h | 1 - src/qemu/qemu_domain.c | 3 +- src/qemu/qemu_driver.c | 3 +- src/qemu/qemu_hotplug.c | 92 +++++++++++++++++++++++++++++--------------- src/qemu/qemu_migration.c | 11 ++---- src/qemu/qemu_monitor_json.c | 10 ++--- src/qemu/qemu_monitor_text.c | 18 +++++---- src/qemu/qemu_process.c | 3 +- 11 files changed, 156 insertions(+), 78 deletions(-)
On Tue, Jul 19, 2016 at 10:30:43AM -0400, John Ferlan wrote:
v1: http://www.redhat.com/archives/libvir-list/2016-June/msg02207.html
Changes since v1... - Use the bz noted by Cole from the v1 series - Merged in recent hotplug cleanup changes - Patches 6 - 9 are new
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1336225
and probably: https://bugzilla.redhat.com/show_bug.cgi?id=1289391
Patches 1-5 address the USB hotplug cleanup
Patch 6 addresses the SCSI hotplug cleanup
Patch 7-9 address some SCSI host device cleanups found while looking at the other cleanups...
John Ferlan (9): qemu: Reorder qemuDomainAttachUSBMassStorageDevice failure path qemu: Move drive alias processing to qemu_alias qemu: Use qemuAssignDeviceDiskDriveAlias qemu: Make QEMU_DRIVE_HOST_PREFIX more private qemu: Add attempt to call qemuMonitorDriveDel for USB failure path qemu: Add attempt to call qemuMonitorDriveDel for AttachSCSI failure path qemu: Introduce qemuAssignSCSIHostDeviceDriveAlias qemu: Use qemuAssignSCSIHostDeviceDriveAlias qemu: Use the hostdev alias in qemuDomainAttachHostSCSIDevice error path
src/qemu/qemu_alias.c | 52 +++++++++++++++++++++++++ src/qemu/qemu_alias.h | 6 +++ src/qemu/qemu_command.c | 35 ++++++++--------- src/qemu/qemu_command.h | 1 - src/qemu/qemu_domain.c | 3 +- src/qemu/qemu_driver.c | 3 +- src/qemu/qemu_hotplug.c | 92 +++++++++++++++++++++++++++++--------------- src/qemu/qemu_migration.c | 11 ++---- src/qemu/qemu_monitor_json.c | 10 ++--- src/qemu/qemu_monitor_text.c | 18 +++++---- src/qemu/qemu_process.c | 3 +- 11 files changed, 156 insertions(+), 78 deletions(-)
ACK series. Jan
participants (2)
-
John Ferlan -
Ján Tomko