[libvirt] [PATCH 0/5] Some USB hotplug and alias cleanups
A post 2.0.0 type adjustment... While answering a question for bz 1289391 regarding the USB path - I figured that making the code be more the virtio and scsi code paths would at least have a better look'n'feel. Then I tripped in to the alias morass and cleaned that up a bit too. John Ferlan (5): 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 src/qemu/qemu_alias.c | 34 +++++++++++++++++++++++++ src/qemu/qemu_alias.h | 4 +++ src/qemu/qemu_command.c | 23 ++++++++--------- src/qemu/qemu_command.h | 1 - src/qemu/qemu_domain.c | 3 +-- src/qemu/qemu_driver.c | 3 +-- src/qemu/qemu_hotplug.c | 59 ++++++++++++++++++++++++++++---------------- 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, 107 insertions(+), 62 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 | 39 ++++++++++++++++++++++----------------- 1 file changed, 22 insertions(+), 17 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index e0b8230..ae5203e 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -649,27 +649,20 @@ 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; - if (ret < 0) - goto error; + if (qemuMonitorAddDevice(priv->mon, devstr) < 0) + goto failadddevice; + + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto failexitmonitor; + + virDomainAuditDisk(vm, NULL, disk->src, "attach", true); virDomainDiskInsertPreAlloced(vm->def, disk); + ret = 0; cleanup: VIR_FREE(devstr); @@ -677,6 +670,18 @@ qemuDomainAttachUSBMassStorageDevice(virQEMUDriverPtr driver, virObjectUnref(cfg); return ret; + failadddevice: + VIR_WARN("qemuMonitorAddDevice failed on %s (%s)", + drivestr, devstr); + /* XXX should call 'drive_del' on error but this does not + exist yet */ + + exit_monitor: + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + + failexitmonitor: + 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 d624071..b4a6e52 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 e328a9b..972de55 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 71e9e63..c42ae16 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -287,17 +287,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 ae5203e..612b6ec 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); @@ -345,7 +345,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 c411dab..25dabb8 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
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 c42ae16..a11ebd0 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1251,7 +1251,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) @@ -1613,6 +1617,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) @@ -1838,7 +1843,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 61d184b..a65b299 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10353,8 +10353,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 612b6ec..be3ba4d 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2798,8 +2798,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; qemuDomainObjEnterMonitor(driver, vm); diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 25dabb8..da9e710 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@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 b4a6e52..c7ef45a 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 972de55..31bab07 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 fd25669..3264b94 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4225,8 +4225,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 63da600..a4adeac 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -354,8 +354,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
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, 17 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index be3ba4d..f73a0a8 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -622,7 +622,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; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); @@ -640,8 +642,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; @@ -666,15 +673,21 @@ qemuDomainAttachUSBMassStorageDevice(virQEMUDriverPtr driver, cleanup: VIR_FREE(devstr); + VIR_FREE(drivealias); VIR_FREE(drivestr); virObjectUnref(cfg); return ret; failadddevice: - 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 (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); + } exit_monitor: ignore_value(qemuDomainObjExitMonitor(driver, vm)); -- 2.5.5
On 06/29/2016 05:37 PM, John Ferlan wrote:
A post 2.0.0 type adjustment...
While answering a question for bz 1289391 regarding the USB path - I figured that making the code be more the virtio and scsi code paths would at least have a better look'n'feel. Then I tripped in to the alias morass and cleaned that up a bit too.
John Ferlan (5): 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
Note there is https://bugzilla.redhat.com/show_bug.cgi?id=1336225 covering some of these changes. There's also a missing drive_del call in AttachSCSI. And in fact the whole drive_add, device_add, and unwinding on failure can probably be shared between AttachVirtio/AttachUSB/AttachSCSI . Not saying it blocks this series, but there's more cleanup in this area if you're motivated :) Thanks, Cole
On 06/30/2016 11:14 AM, Cole Robinson wrote:
On 06/29/2016 05:37 PM, John Ferlan wrote:
A post 2.0.0 type adjustment...
While answering a question for bz 1289391 regarding the USB path - I figured that making the code be more the virtio and scsi code paths would at least have a better look'n'feel. Then I tripped in to the alias morass and cleaned that up a bit too.
John Ferlan (5): 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
Note there is https://bugzilla.redhat.com/show_bug.cgi?id=1336225 covering some of these changes. There's also a missing drive_del call in AttachSCSI. And in fact the whole drive_add, device_add, and unwinding on failure can probably be shared between AttachVirtio/AttachUSB/AttachSCSI . Not saying it blocks this series, but there's more cleanup in this area if you're motivated :)
I thought about touching AttachSCSI, but I knew I still had changes from the pending LUKS secret series and I didn't want to have to continue messing with it until that got pushed. And yes, as I was going through this I started thinking - there's a lot of similar code that perhaps could be combined. One thing at a time I guess though. Thanks! John
participants (2)
-
Cole Robinson -
John Ferlan