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(a)redhat.com&gt; --- 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

On Tue, Jul 19, 2016 at 10:30:45AM -0400, John Ferlan wrote: 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

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(a)redhat.com&gt; --- 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

The qemuDomainSnapshotCreateSingleDiskActive open coded generating the disk drive alias. Let's use the common function. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- 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

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(a)redhat.com&gt; --- 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

Rather than open code build the drive alias command in multiple places, use the helper to ensure consistency. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- 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

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(a)redhat.com&gt; --- 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

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(a)redhat.com&gt; --- 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

On 07/19/2016 10:30 AM, John Ferlan wrote: Considering the other changes and for consistency, changing this to qemuAliasFromHostdev (to mimic qemuAliasFromDisk) seems appropriate. John

On 07/19/2016 10:30 AM, John Ferlan wrote: 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);