[libvirt] [PATCH 0/2] Fix errors on unsuccessful chardev hotplug
Ján Tomko (2): Fix crash after attempting to hotplug a spicevmc channel Display nicer error message for unsupported chardev hotplug src/conf/domain_conf.c | 3 +-- src/qemu/qemu_monitor_json.c | 12 +++++++++--- 2 files changed, 10 insertions(+), 5 deletions(-) -- 2.0.4
In qemuDomainAttachChrDevice, we add the device to the domain XML, then we fail to construct the JSON command, saying we don't support it. But we don't clean up the device from the domain XML, because virDomainChrEquals returns false when comparing the device against itself. https://bugzilla.redhat.com/show_bug.cgi?id=1162097 --- src/conf/domain_conf.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e8d8f7d..31378cd 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1596,8 +1596,7 @@ virDomainChrSourceDefIsEqual(const virDomainChrSourceDef *src, case VIR_DOMAIN_CHR_TYPE_STDIO: case VIR_DOMAIN_CHR_TYPE_SPICEVMC: case VIR_DOMAIN_CHR_TYPE_LAST: - /* nada */ - break; + return true; } return false; -- 2.0.4
On 11/10/2014 04:56 PM, Ján Tomko wrote:
In qemuDomainAttachChrDevice, we add the device to the domain XML, then we fail to construct the JSON command, saying we don't support it.
But we don't clean up the device from the domain XML, because virDomainChrEquals returns false when comparing the device against itself.
https://bugzilla.redhat.com/show_bug.cgi?id=1162097 --- src/conf/domain_conf.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e8d8f7d..31378cd 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1596,8 +1596,7 @@ virDomainChrSourceDefIsEqual(const virDomainChrSourceDef *src, case VIR_DOMAIN_CHR_TYPE_STDIO: case VIR_DOMAIN_CHR_TYPE_SPICEVMC: case VIR_DOMAIN_CHR_TYPE_LAST: - /* nada */ - break; + return true; }
return false;
This isn't true for VIR_DOMAIN_CHR_TYPE_SPICEVMC as we are setting data.spicevmc to some value and it could have different value. See enum virDomainChrSpicevmcName. Pavel
virDomainChrSourceDefIsEqual should return 'true' for identical SPICEVMC chardevs, and those that have no source specification. After this change, a failed hotplug no longer leaves a stale pointer in the domain definition. https://bugzilla.redhat.com/show_bug.cgi?id=1162097 --- src/conf/domain_conf.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 54b2bfe..73b2393 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1591,13 +1591,14 @@ virDomainChrSourceDefIsEqual(const virDomainChrSourceDef *src, tgt->data.spiceport.channel); break; + case VIR_DOMAIN_CHR_TYPE_SPICEVMC: + return src->data.spicevmc == tgt->data.spicevmc; + case VIR_DOMAIN_CHR_TYPE_NULL: case VIR_DOMAIN_CHR_TYPE_VC: case VIR_DOMAIN_CHR_TYPE_STDIO: - case VIR_DOMAIN_CHR_TYPE_SPICEVMC: case VIR_DOMAIN_CHR_TYPE_LAST: - /* nada */ - break; + return true; } return false; -- 2.0.4
On 11/11/2014 12:23 PM, Ján Tomko wrote:
virDomainChrSourceDefIsEqual should return 'true' for identical SPICEVMC chardevs, and those that have no source specification.
After this change, a failed hotplug no longer leaves a stale pointer in the domain definition.
https://bugzilla.redhat.com/show_bug.cgi?id=1162097 --- src/conf/domain_conf.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 54b2bfe..73b2393 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1591,13 +1591,14 @@ virDomainChrSourceDefIsEqual(const virDomainChrSourceDef *src, tgt->data.spiceport.channel); break;
+ case VIR_DOMAIN_CHR_TYPE_SPICEVMC: + return src->data.spicevmc == tgt->data.spicevmc; + case VIR_DOMAIN_CHR_TYPE_NULL: case VIR_DOMAIN_CHR_TYPE_VC: case VIR_DOMAIN_CHR_TYPE_STDIO: - case VIR_DOMAIN_CHR_TYPE_SPICEVMC: case VIR_DOMAIN_CHR_TYPE_LAST: - /* nada */ - break; + return true; }
return false;
ACK, Pavel
On Tue, Nov 11, 2014 at 12:23:44PM +0100, Ján Tomko wrote:
virDomainChrSourceDefIsEqual should return 'true' for identical SPICEVMC chardevs, and those that have no source specification.
After this change, a failed hotplug no longer leaves a stale pointer in the domain definition.
https://bugzilla.redhat.com/show_bug.cgi?id=1162097 --- src/conf/domain_conf.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 54b2bfe..73b2393 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1591,13 +1591,14 @@ virDomainChrSourceDefIsEqual(const virDomainChrSourceDef *src, tgt->data.spiceport.channel); break;
+ case VIR_DOMAIN_CHR_TYPE_SPICEVMC: + return src->data.spicevmc == tgt->data.spicevmc; + case VIR_DOMAIN_CHR_TYPE_NULL: case VIR_DOMAIN_CHR_TYPE_VC: case VIR_DOMAIN_CHR_TYPE_STDIO: - case VIR_DOMAIN_CHR_TYPE_SPICEVMC: case VIR_DOMAIN_CHR_TYPE_LAST: - /* nada */ - break; + return true; }
return false;
Probably a dead code and very possibly an ewww statement. Either remove it or change it to true and remove the one you added few lines up, please. Martin
Use the device type name if we know it instead of its number, even if we can't hotplug it: qemuMonitorJSONAttachCharDevCommand:6094 : operation failed: Unsupported char device type '10' --- src/qemu/qemu_monitor_json.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 7870664..2d082bc 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -5915,9 +5915,15 @@ qemuMonitorJSONAttachCharDevCommand(const char *chrID, case VIR_DOMAIN_CHR_TYPE_STDIO: case VIR_DOMAIN_CHR_TYPE_NMDM: case VIR_DOMAIN_CHR_TYPE_LAST: - virReportError(VIR_ERR_OPERATION_FAILED, - _("Unsupported char device type '%d'"), - chr->type); + if (virDomainChrTypeToString(chr->type)) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("Hotplug unsupported char device type '%s'"), + virDomainChrTypeToString(chr->type)); + } else { + virReportError(VIR_ERR_OPERATION_FAILED, + _("Unsupported char device type '%d'"), + chr->type); + } goto error; } -- 2.0.4
On 11/10/2014 04:57 PM, Ján Tomko wrote:
Use the device type name if we know it instead of its number, even if we can't hotplug it: qemuMonitorJSONAttachCharDevCommand:6094 : operation failed: Unsupported char device type '10' --- src/qemu/qemu_monitor_json.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 7870664..2d082bc 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -5915,9 +5915,15 @@ qemuMonitorJSONAttachCharDevCommand(const char *chrID, case VIR_DOMAIN_CHR_TYPE_STDIO: case VIR_DOMAIN_CHR_TYPE_NMDM: case VIR_DOMAIN_CHR_TYPE_LAST: - virReportError(VIR_ERR_OPERATION_FAILED, - _("Unsupported char device type '%d'"), - chr->type); + if (virDomainChrTypeToString(chr->type)) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("Hotplug unsupported char device type '%s'"), + virDomainChrTypeToString(chr->type)); + } else { + virReportError(VIR_ERR_OPERATION_FAILED, + _("Unsupported char device type '%d'"), + chr->type); + } goto error; }
Both error messages could be the same as they are reporting the same issue. ACK with that change. Pavel
participants (3)
-
Ján Tomko -
Martin Kletzander -
Pavel Hrdina