6:29 a.m.
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
6:29 a.m.
New subject: [libvirt] [PATCH 1/3] conf: Extend virDomainDeviceDefParse handled types
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
7:05 a.m.
New subject: [libvirt] [PATCH 1/3] conf: Extend virDomainDeviceDefParse handled types
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
6:29 a.m.
New subject: [libvirt] [PATCH 2/3] conf: Rework virDomainDeviceDefParse
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
7:08 a.m.
New subject: [libvirt] [PATCH 2/3] conf: Rework virDomainDeviceDefParse
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
6:29 a.m.
New subject: [libvirt] [PATCH 3/3] qemu: Fix hot (un-)plug error codes and messages
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
6:50 a.m.
New subject: [libvirt] [PATCH 3/3] qemu: Fix hot (un-)plug error codes and messages
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
4153
days inactive
4153
days old
6 comments
2 participants
participants (2)
-
Michal Privoznik -
Peter Krempa