[libvirt] [PATCH v3 1/2] qemu: output error when try to hotplug unsupport console
https://bugzilla.redhat.com/show_bug.cgi?id=1164627 When using 'virsh attach-device' to hotplug an unsupported console type into a qemu guest, the attachment will erroneously allows the attachment. This patch will check to ensure that only the support target types are used for a running guest. NB, Although a guest can be defined with an improper target, startup will cause failure. Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/qemu/qemu_command.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index c041ee7..df87e1e 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -10067,13 +10067,17 @@ qemuBuildConsoleChrDeviceStr(char **deviceStr, break; case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL: + break; case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_NONE: case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_XEN: case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_UML: case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_LXC: case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_OPENVZ: case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_LAST: - break; + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unsupported console target type %s"), + NULLSTR(virDomainChrConsoleTargetTypeToString(chr->targetType))); + goto cleanup; } ret = 0; -- 1.8.3.1
https://bugzilla.redhat.com/show_bug.cgi?id=1164627 Add a func just check the base target type which qemu support. And this check will help to avoid add a qemu unsupport target type chr device to a running guest(hotplug) or to the guest inactive XML (coldplug, will effect next boot) And this check only check the target type, and other things have been checked in virDomainChrDefParseXML. Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/libvirt_private.syms | 2 ++ src/qemu/qemu_hotplug.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a2eec83..7fb722d 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -145,6 +145,8 @@ virDomainBlockedReasonTypeFromString; virDomainBlockedReasonTypeToString; virDomainCapabilitiesPolicyTypeToString; virDomainCapsFeatureTypeToString; +virDomainChrChannelTargetTypeFromString; +virDomainChrChannelTargetTypeToString; virDomainChrConsoleTargetTypeFromString; virDomainChrConsoleTargetTypeToString; virDomainChrDefForeach; diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 9ed96dc..2e02d05 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1401,10 +1401,77 @@ int qemuDomainAttachRedirdevDevice(virQEMUDriverPtr driver, } +static int +qemuDomainChrCheckDefSupport(virDomainChrDefPtr chr) +{ + switch (chr->deviceType) { + case VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL: + switch ((virDomainChrSerialTargetType) chr->targetType) { + case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA: + case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_USB: + break; + + case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_LAST: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unsupported serial target type %s for qemu"), + NULLSTR(virDomainChrSerialTargetTypeToString(chr->targetType))); + return -1; + break; + } + break; + + case VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL: + break; + + case VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL: + switch ((virDomainChrChannelTargetType) chr->targetType) { + case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_GUESTFWD: + case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO: + break; + + case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_NONE: + case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_LAST: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unsupported channel target type %s for qemu"), + NULLSTR(virDomainChrChannelTargetTypeToString(chr->targetType))); + return -1; + break; + } + break; + + case VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE: + switch ((virDomainChrConsoleTargetType) chr->targetType) { + case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL: + case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO: + case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SCLP: + case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SCLPLM: + break; + + case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_NONE: + case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_XEN: + case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_UML: + case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_LXC: + case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_OPENVZ: + case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_LAST: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unsupported console target type %s for qemu"), + NULLSTR(virDomainChrConsoleTargetTypeToString(chr->targetType))); + return -1; + break; + } + break; + } + + return 0; +} + int qemuDomainChrInsert(virDomainDefPtr vmdef, virDomainChrDefPtr chr) { + if (qemuDomainChrCheckDefSupport(chr) < 0) + return -1; + if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE && chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", -- 1.8.3.1
On Thu, Jan 22, 2015 at 10:28:19 +0800, Luyao Huang wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1164627
Add a func just check the base target type which qemu support. And this check will help to avoid add a qemu unsupport target type chr device to a running guest(hotplug) or to the guest inactive XML (coldplug, will effect next boot)
You can get exactly the same behavior adding the device by specifying a new XML containing the device (via virsh edit for example). Unfortunately the unsupported devices can't be checked in the post parse callback as it would make existing VMs with broken config disappear. Additionally even if you forbid known-bad targets you still even with this code can get a failure by adding a SCLP console on a x86 machine: <console type='pty'> <target type='sclp' port='0'/> </console> Starting such VM gets you: error: unsupported configuration: sclp console requires QEMU to support s390-sclp This kind of failure depends on the actual qemu process running the VM and can be checked only when the VM is starting.
And this check only check the target type, and other things have been checked in virDomainChrDefParseXML.
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/libvirt_private.syms | 2 ++ src/qemu/qemu_hotplug.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+)
Said this. I don't think it's worth adding the check to the coldplug path. Peter
On 01/22/2015 03:37 PM, Peter Krempa wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1164627
Add a func just check the base target type which qemu support. And this check will help to avoid add a qemu unsupport target type chr device to a running guest(hotplug) or to the guest inactive XML (coldplug, will effect next boot) You can get exactly the same behavior adding the device by specifying a new XML containing the device (via virsh edit for example). Unfortunately the unsupported devices can't be checked in the post parse callback as it would make existing VMs with broken config disappear. Additionally even if you forbid known-bad targets you still even with
On Thu, Jan 22, 2015 at 10:28:19 +0800, Luyao Huang wrote: this code can get a failure by adding a SCLP console on a x86 machine:
<console type='pty'> <target type='sclp' port='0'/> </console>
Starting such VM gets you: error: unsupported configuration: sclp console requires QEMU to support s390-sclp
This kind of failure depends on the actual qemu process running the VM and can be checked only when the VM is starting.
Yes, this check cannot avoid attach a unsupported chr device in all scene (too old qemu or a chr device not support for x86_64), because we cannot do a check to qemu caps in this place. As you said we should check if actual qemu support it when start the vm.
And this check only check the target type, and other things have been checked in virDomainChrDefParseXML.
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/libvirt_private.syms | 2 ++ src/qemu/qemu_hotplug.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+)
Said this. I don't think it's worth adding the check to the coldplug path.
Eww, I thought about this before and i think maybe we can have more check to the Chr device when we do coldplug than we define it (add new check in parse will make guest which have broken settings disappear when update libvirt), although this check cannot cover all the sence. And thanks a lot for your opinion and review!
Peter
Luyao
On Thu, Jan 22, 2015 at 10:28:18 +0800, Luyao Huang wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1164627
When using 'virsh attach-device' to hotplug an unsupported console type into a qemu guest, the attachment will erroneously allows the attachment. This patch will check to ensure that only the support target types are used for a running guest. NB, Although a guest can be defined with an improper target, startup will cause failure.
I tweaked the subject and commit message as some sentences didn't make sense.
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/qemu/qemu_command.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index c041ee7..df87e1e 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -10067,13 +10067,17 @@ qemuBuildConsoleChrDeviceStr(char **deviceStr, break;
case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL: + break; case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_NONE: case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_XEN: case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_UML: case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_LXC: case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_OPENVZ: case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_LAST: - break; + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unsupported console target type %s"), + NULLSTR(virDomainChrConsoleTargetTypeToString(chr->targetType))); + goto cleanup; }
ACK, although the rest of the chardev command and hotplug code is a big mess and would benefit from a cleanup/refactor. Pushed. Peter
On 01/22/2015 06:19 PM, Peter Krempa wrote:
On Thu, Jan 22, 2015 at 10:28:18 +0800, Luyao Huang wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1164627
When using 'virsh attach-device' to hotplug an unsupported console type into a qemu guest, the attachment will erroneously allows the attachment. This patch will check to ensure that only the support target types are used for a running guest. NB, Although a guest can be defined with an improper target, startup will cause failure. I tweaked the subject and commit message as some sentences didn't make sense.
Thanks for your review and help.
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/qemu/qemu_command.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index c041ee7..df87e1e 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -10067,13 +10067,17 @@ qemuBuildConsoleChrDeviceStr(char **deviceStr, break;
case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL: + break; case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_NONE: case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_XEN: case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_UML: case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_LXC: case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_OPENVZ: case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_LAST: - break; + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unsupported console target type %s"), + NULLSTR(virDomainChrConsoleTargetTypeToString(chr->targetType))); + goto cleanup; } ACK, although the rest of the chardev command and hotplug code is a big mess and would benefit from a cleanup/refactor.
Yes, i noticed this mess and i will try to fix this in next step.
Pushed.
Peter
Luyao
On Thu, Jan 22, 2015 at 21:20:46 +0800, Luyao Huang wrote:
On 01/22/2015 06:19 PM, Peter Krempa wrote:
On Thu, Jan 22, 2015 at 10:28:18 +0800, Luyao Huang wrote:
...
ACK, although the rest of the chardev command and hotplug code is a big mess and would benefit from a cleanup/refactor.
Yes, i noticed this mess and i will try to fix this in next step.
I don't insist on you doing that. It was just an observation from looking through the code. Peter
On 01/22/2015 09:25 PM, Peter Krempa wrote:
On Thu, Jan 22, 2015 at 21:20:46 +0800, Luyao Huang wrote:
On 01/22/2015 06:19 PM, Peter Krempa wrote:
On Thu, Jan 22, 2015 at 10:28:18 +0800, Luyao Huang wrote:
...
ACK, although the rest of the chardev command and hotplug code is a big mess and would benefit from a cleanup/refactor. Yes, i noticed this mess and i will try to fix this in next step. I don't insist on you doing that. It was just an observation from looking through the code.
yes, I know that, i am just interested in this part of code :) Although i know there are some thorny problems when do a refactor, and seems this will take some time to do it.
Peter
Luyao
participants (3)
-
lhuang -
Luyao Huang
-
Peter Krempa