[libvirt] [PATCH 0/3] Fix hotplug error messages
https://bugzilla.redhat.com/show_bug.cgi?id=982304 The problem is virDomainDeviceDefParse's incompleteness. When doing a hot(un-)plug or update, the passed XML snippet is parsed. However, if virDomainDeviceDefParse doesn't recognize the device (due its implementation incompleteness) an error is returned: error: XML error: unknown device type and the control doesn't even reach the check for supported devices for current operation, where much more correct error message would be printed out: error: Operation not supported: persistent detach of device 'memballoon' is not supported Michar Privoznik (3): conf: Extend virDomainDeviceDefParse handled types conf: Rework virDomainDeviceDefParse qemu: Fix hot (un-)plug error codes and messages src/conf/domain_conf.c | 111 ++++++++++++++++++++++++++++++++++--------------- src/qemu/qemu_driver.c | 42 ++++++++++--------- 2 files changed, 101 insertions(+), 52 deletions(-) -- 1.8.1.5
Not all device types are parsed in virDomainDeviceDefParse, currently. Since all functions needed do exist, nothing hold us back to make the implementation complete. Similarly, the virDomainDeviceDefFree needs to be updated as well. --- src/conf/domain_conf.c | 37 +++++++++++++++++++++++++++++++++++-- 1 file changed, 35 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 4013267..d0c87b2 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1756,13 +1756,23 @@ void virDomainDeviceDefFree(virDomainDeviceDefPtr def) case VIR_DOMAIN_DEVICE_RNG: virDomainRNGDefFree(def->data.rng); break; - case VIR_DOMAIN_DEVICE_NONE: + case VIR_DOMAIN_DEVICE_CHR: + virDomainChrDefFree(def->data.chr); + break; case VIR_DOMAIN_DEVICE_FS: + virDomainFSDefFree(def->data.fs); + break; case VIR_DOMAIN_DEVICE_SMARTCARD: - case VIR_DOMAIN_DEVICE_CHR: + virDomainSmartcardDefFree(def->data.smartcard); + break; case VIR_DOMAIN_DEVICE_MEMBALLOON: + virDomainMemballoonDefFree(def->data.memballoon); + break; case VIR_DOMAIN_DEVICE_NVRAM: + virDomainNVRAMDefFree(def->data.nvram); + break; case VIR_DOMAIN_DEVICE_LAST: + case VIR_DOMAIN_DEVICE_NONE: break; } @@ -9388,6 +9398,29 @@ virDomainDeviceDefParse(const char *xmlStr, dev->type = VIR_DOMAIN_DEVICE_RNG; if (!(dev->data.rng = virDomainRNGDefParseXML(node, ctxt, flags))) goto error; + } else if (xmlStrEqual(node->name, BAD_CAST "channel") || + xmlStrEqual(node->name, BAD_CAST "console") || + xmlStrEqual(node->name, BAD_CAST "parallel") || + xmlStrEqual(node->name, BAD_CAST "serial")) { + dev->type = VIR_DOMAIN_DEVICE_CHR; + if (!(dev->data.chr = virDomainChrDefParseXML(ctxt, + node, + def->seclabels, + def->nseclabels, + flags))) + goto error; + } else if (xmlStrEqual(node->name, BAD_CAST "smartcard")) { + dev->type = VIR_DOMAIN_DEVICE_SMARTCARD; + if (!(dev->data.smartcard = virDomainSmartcardDefParseXML(node, flags))) + goto error; + } else if (xmlStrEqual(node->name, BAD_CAST "memballoon")) { + dev->type = VIR_DOMAIN_DEVICE_MEMBALLOON; + if (!(dev->data.memballoon = virDomainMemballoonDefParseXML(node, flags))) + goto error; + } else if (xmlStrEqual(node->name, BAD_CAST "nvram")) { + dev->type = VIR_DOMAIN_DEVICE_NVRAM; + if (!(dev->data.nvram = virDomainNVRAMDefParseXML(node, flags))) + goto error; } else { virReportError(VIR_ERR_XML_ERROR, "%s", _("unknown device type")); goto error; -- 1.8.1.5
I'd reword the subject line as: conf: Extend device types handled by virDomainDeviceDefParse On 07/11/13 13:29, Michal Privoznik wrote:
Not all device types are parsed in virDomainDeviceDefParse, currently.
s/are/are currently/ s/, currently//
Since all functions needed do exist, nothing hold us back to make the
s/hold/holds/
implementation complete. Similarly, the virDomainDeviceDefFree needs to be updated as well. --- src/conf/domain_conf.c | 37 +++++++++++++++++++++++++++++++++++-- 1 file changed, 35 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 4013267..d0c87b2 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c
...
@@ -9388,6 +9398,29 @@ virDomainDeviceDefParse(const char *xmlStr, dev->type = VIR_DOMAIN_DEVICE_RNG; if (!(dev->data.rng = virDomainRNGDefParseXML(node, ctxt, flags))) goto error; + } else if (xmlStrEqual(node->name, BAD_CAST "channel") || + xmlStrEqual(node->name, BAD_CAST "console") || + xmlStrEqual(node->name, BAD_CAST "parallel") || + xmlStrEqual(node->name, BAD_CAST "serial")) {
hmm, right, those elements map into a single internal structure type
+ dev->type = VIR_DOMAIN_DEVICE_CHR; + if (!(dev->data.chr = virDomainChrDefParseXML(ctxt, + node, + def->seclabels, + def->nseclabels, + flags))) + goto error;
ACK with the commit message updated. Peter
When adding a new domain device, it is fairly easy to forget to add corresponding piece into virDomainDeviceDefParse. However, if the internal structure is changed to one bit switch() the compiler will warn about not handled enum item. --- src/conf/domain_conf.c | 96 ++++++++++++++++++++++++++++---------------------- 1 file changed, 54 insertions(+), 42 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d0c87b2..764ca6e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9336,94 +9336,106 @@ virDomainDeviceDefParse(const char *xmlStr, if (VIR_ALLOC(dev) < 0) goto error; - if (xmlStrEqual(node->name, BAD_CAST "disk")) { - dev->type = VIR_DOMAIN_DEVICE_DISK; + if ((dev->type = virDomainDeviceTypeFromString((const char *) node->name)) < 0) { + /* Some crazy mapping of serial, parallel, console and channel to + * VIR_DOMAIN_DEVICE_CHR. */ + if (xmlStrEqual(node->name, BAD_CAST "channel") || + xmlStrEqual(node->name, BAD_CAST "console") || + xmlStrEqual(node->name, BAD_CAST "parallel") || + xmlStrEqual(node->name, BAD_CAST "serial")) { + dev->type = VIR_DOMAIN_DEVICE_CHR; + } else { + virReportError(VIR_ERR_XML_ERROR, _("unknown device type '%s'"), node->name); + goto error; + } + } + + switch ((virDomainDeviceType) dev->type) { + case VIR_DOMAIN_DEVICE_DISK: if (!(dev->data.disk = virDomainDiskDefParseXML(xmlopt, node, ctxt, NULL, def->seclabels, def->nseclabels, flags))) goto error; - } else if (xmlStrEqual(node->name, BAD_CAST "lease")) { - dev->type = VIR_DOMAIN_DEVICE_LEASE; + break; + case VIR_DOMAIN_DEVICE_LEASE: if (!(dev->data.lease = virDomainLeaseDefParseXML(node))) goto error; - } else if (xmlStrEqual(node->name, BAD_CAST "filesystem")) { - dev->type = VIR_DOMAIN_DEVICE_FS; + break; + case VIR_DOMAIN_DEVICE_FS: if (!(dev->data.fs = virDomainFSDefParseXML(node, ctxt, flags))) goto error; - } else if (xmlStrEqual(node->name, BAD_CAST "interface")) { - dev->type = VIR_DOMAIN_DEVICE_NET; + break; + case VIR_DOMAIN_DEVICE_NET: if (!(dev->data.net = virDomainNetDefParseXML(xmlopt, node, ctxt, NULL, flags))) goto error; - } else if (xmlStrEqual(node->name, BAD_CAST "input")) { - dev->type = VIR_DOMAIN_DEVICE_INPUT; + break; + case VIR_DOMAIN_DEVICE_INPUT: if (!(dev->data.input = virDomainInputDefParseXML(def->os.type, node, flags))) goto error; - } else if (xmlStrEqual(node->name, BAD_CAST "sound")) { - dev->type = VIR_DOMAIN_DEVICE_SOUND; + break; + case VIR_DOMAIN_DEVICE_SOUND: if (!(dev->data.sound = virDomainSoundDefParseXML(node, ctxt, flags))) goto error; - } else if (xmlStrEqual(node->name, BAD_CAST "watchdog")) { - dev->type = VIR_DOMAIN_DEVICE_WATCHDOG; + break; + case VIR_DOMAIN_DEVICE_WATCHDOG: if (!(dev->data.watchdog = virDomainWatchdogDefParseXML(node, flags))) goto error; - } else if (xmlStrEqual(node->name, BAD_CAST "video")) { - dev->type = VIR_DOMAIN_DEVICE_VIDEO; + break; + case VIR_DOMAIN_DEVICE_VIDEO: if (!(dev->data.video = virDomainVideoDefParseXML(node, def, flags))) goto error; - } else if (xmlStrEqual(node->name, BAD_CAST "hostdev")) { - dev->type = VIR_DOMAIN_DEVICE_HOSTDEV; + break; + case VIR_DOMAIN_DEVICE_HOSTDEV: if (!(dev->data.hostdev = virDomainHostdevDefParseXML(xmlopt, def, node, ctxt, NULL, flags))) goto error; - } else if (xmlStrEqual(node->name, BAD_CAST "controller")) { - dev->type = VIR_DOMAIN_DEVICE_CONTROLLER; + break; + case VIR_DOMAIN_DEVICE_CONTROLLER: if (!(dev->data.controller = virDomainControllerDefParseXML(node, flags))) goto error; - } else if (xmlStrEqual(node->name, BAD_CAST "graphics")) { - dev->type = VIR_DOMAIN_DEVICE_GRAPHICS; + break; + case VIR_DOMAIN_DEVICE_GRAPHICS: if (!(dev->data.graphics = virDomainGraphicsDefParseXML(node, ctxt, flags))) goto error; - } else if (xmlStrEqual(node->name, BAD_CAST "hub")) { - dev->type = VIR_DOMAIN_DEVICE_HUB; + break; + case VIR_DOMAIN_DEVICE_HUB: if (!(dev->data.hub = virDomainHubDefParseXML(node, flags))) goto error; - } else if (xmlStrEqual(node->name, BAD_CAST "redirdev")) { - dev->type = VIR_DOMAIN_DEVICE_REDIRDEV; + break; + case VIR_DOMAIN_DEVICE_REDIRDEV: if (!(dev->data.redirdev = virDomainRedirdevDefParseXML(node, NULL, flags))) goto error; - } else if (xmlStrEqual(node->name, BAD_CAST "rng")) { - dev->type = VIR_DOMAIN_DEVICE_RNG; + break; + case VIR_DOMAIN_DEVICE_RNG: if (!(dev->data.rng = virDomainRNGDefParseXML(node, ctxt, flags))) goto error; - } else if (xmlStrEqual(node->name, BAD_CAST "channel") || - xmlStrEqual(node->name, BAD_CAST "console") || - xmlStrEqual(node->name, BAD_CAST "parallel") || - xmlStrEqual(node->name, BAD_CAST "serial")) { - dev->type = VIR_DOMAIN_DEVICE_CHR; + break; + case VIR_DOMAIN_DEVICE_CHR: if (!(dev->data.chr = virDomainChrDefParseXML(ctxt, node, def->seclabels, def->nseclabels, flags))) goto error; - } else if (xmlStrEqual(node->name, BAD_CAST "smartcard")) { - dev->type = VIR_DOMAIN_DEVICE_SMARTCARD; + break; + case VIR_DOMAIN_DEVICE_SMARTCARD: if (!(dev->data.smartcard = virDomainSmartcardDefParseXML(node, flags))) goto error; - } else if (xmlStrEqual(node->name, BAD_CAST "memballoon")) { - dev->type = VIR_DOMAIN_DEVICE_MEMBALLOON; + break; + case VIR_DOMAIN_DEVICE_MEMBALLOON: if (!(dev->data.memballoon = virDomainMemballoonDefParseXML(node, flags))) goto error; - } else if (xmlStrEqual(node->name, BAD_CAST "nvram")) { - dev->type = VIR_DOMAIN_DEVICE_NVRAM; + break; + case VIR_DOMAIN_DEVICE_NVRAM: if (!(dev->data.nvram = virDomainNVRAMDefParseXML(node, flags))) goto error; - } else { - virReportError(VIR_ERR_XML_ERROR, "%s", _("unknown device type")); - goto error; + break; + case VIR_DOMAIN_DEVICE_NONE: + case VIR_DOMAIN_DEVICE_LAST: + break; } /* callback to fill driver specific device aspects */ -- 1.8.1.5
On 07/11/13 13:29, Michal Privoznik wrote:
When adding a new domain device, it is fairly easy to forget to add corresponding piece into virDomainDeviceDefParse. However, if the internal structure is changed to one bit switch() the compiler will warn about not handled enum item. --- src/conf/domain_conf.c | 96 ++++++++++++++++++++++++++++---------------------- 1 file changed, 54 insertions(+), 42 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d0c87b2..764ca6e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9336,94 +9336,106 @@ virDomainDeviceDefParse(const char *xmlStr, if (VIR_ALLOC(dev) < 0) goto error;
- if (xmlStrEqual(node->name, BAD_CAST "disk")) { - dev->type = VIR_DOMAIN_DEVICE_DISK; + if ((dev->type = virDomainDeviceTypeFromString((const char *) node->name)) < 0) { + /* Some crazy mapping of serial, parallel, console and channel to + * VIR_DOMAIN_DEVICE_CHR. */
hmmm, this isn't very nice, but ...
+ if (xmlStrEqual(node->name, BAD_CAST "channel") || + xmlStrEqual(node->name, BAD_CAST "console") || + xmlStrEqual(node->name, BAD_CAST "parallel") || + xmlStrEqual(node->name, BAD_CAST "serial")) { + dev->type = VIR_DOMAIN_DEVICE_CHR; + } else { + virReportError(VIR_ERR_XML_ERROR, _("unknown device type '%s'"), node->name);
This line is probably too long. Please break it.
+ goto error; + } + } + + switch ((virDomainDeviceType) dev->type) { + case VIR_DOMAIN_DEVICE_DISK: if (!(dev->data.disk = virDomainDiskDefParseXML(xmlopt, node, ctxt, NULL, def->seclabels, def->nseclabels, flags))) goto error; - } else if (xmlStrEqual(node->name, BAD_CAST "lease")) { - dev->type = VIR_DOMAIN_DEVICE_LEASE; + break; + case VIR_DOMAIN_DEVICE_LEASE: if (!(dev->data.lease = virDomainLeaseDefParseXML(node))) goto error; - } else if (xmlStrEqual(node->name, BAD_CAST "filesystem")) { - dev->type = VIR_DOMAIN_DEVICE_FS; + break;
... this pattern looks better and as the commit message states, it lets the compiler warn us. ACK, with long line broken. Peter
With current code, error reporting for unsupported devices for hot plug, unplug and update is total mess. The VIR_ERR_CONFIG_UNSUPPORTED error code is reported instead of VIR_ERR_OPERATION_UNSUPPORTED. Moreover, the error messages are not helping to find the root cause (lack of implementation). --- src/qemu/qemu_driver.c | 42 +++++++++++++++++++++++------------------- 1 file changed, 23 insertions(+), 19 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0f4497d..495867a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6292,13 +6292,13 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn, } else if (disk->bus == VIR_DOMAIN_DISK_BUS_SCSI) { ret = qemuDomainAttachSCSIDisk(conn, driver, vm, disk); } else { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, _("disk bus '%s' cannot be hotplugged."), virDomainDiskBusTypeToString(disk->bus)); } break; default: - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, _("disk device type '%s' cannot be hotplugged"), virDomainDiskDeviceTypeToString(disk->device)); break; @@ -6331,8 +6331,8 @@ qemuDomainAttachDeviceControllerLive(virQEMUDriverPtr driver, ret = qemuDomainAttachPciControllerDevice(driver, vm, cont); break; default: - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("'%s' controller cannot be hotplugged."), + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("'%s' controller cannot be hot plugged."), virDomainControllerTypeToString(cont->type)); break; } @@ -6391,8 +6391,8 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm, break; default: - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("device type '%s' cannot be attached"), + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("live attach of device '%s' is not supported"), virDomainDeviceTypeToString(dev->type)); break; } @@ -6417,11 +6417,11 @@ qemuDomainDetachDeviceDiskLive(virQEMUDriverPtr driver, disk->bus == VIR_DOMAIN_DISK_BUS_USB) ret = qemuDomainDetachDiskDevice(driver, vm, dev); else - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("This type of disk cannot be hot unplugged")); break; default: - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, _("disk device type '%s' cannot be detached"), virDomainDiskDeviceTypeToString(disk->device)); break; @@ -6446,8 +6446,8 @@ qemuDomainDetachDeviceControllerLive(virQEMUDriverPtr driver, ret = qemuDomainDetachPciControllerDevice(driver, vm, dev); break; default : - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("'%s' controller cannot be hotunplugged."), + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("'%s' controller cannot be hot unplugged."), virDomainControllerTypeToString(cont->type)); } return ret; @@ -6478,8 +6478,9 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, ret = qemuDomainDetachHostDevice(driver, vm, dev); break; default: - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - "%s", _("This type of device cannot be hot unplugged")); + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("live detach of device '%s' is not supported"), + virDomainDeviceTypeToString(dev->type)); break; } @@ -6592,7 +6593,7 @@ qemuDomainUpdateDeviceLive(virConnectPtr conn, break; default: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("device type '%s' cannot be updated"), + _("live update of device '%s' is not supported"), virDomainDeviceTypeToString(dev->type)); break; } @@ -6686,8 +6687,9 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps, break; default: - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("persistent attach of device is not supported")); + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("persistent attach of device '%s' is not supported"), + virDomainDeviceTypeToString(dev->type)); return -1; } return 0; @@ -6771,8 +6773,9 @@ qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef, break; default: - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("persistent detach of device is not supported")); + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("persistent detach of device '%s' is not supported"), + virDomainDeviceTypeToString(dev->type)); return -1; } return 0; @@ -6850,8 +6853,9 @@ qemuDomainUpdateDeviceConfig(virQEMUCapsPtr qemuCaps, break; default: - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("persistent update of device is not supported")); + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("persistent update of device '%s' is not supported"), + virDomainDeviceTypeToString(dev->type)); return -1; } return 0; -- 1.8.1.5
On 07/11/13 13:29, Michal Privoznik wrote:
With current code, error reporting for unsupported devices for hot plug, unplug and update is total mess. The VIR_ERR_CONFIG_UNSUPPORTED error code is reported instead of VIR_ERR_OPERATION_UNSUPPORTED. Moreover, the error messages are not helping to find the root cause (lack of implementation). --- src/qemu/qemu_driver.c | 42 +++++++++++++++++++++++------------------- 1 file changed, 23 insertions(+), 19 deletions(-)
ACK, Peter
participants (2)
-
Michal Privoznik -
Peter Krempa