3:28 a.m.
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(a)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
3:28 a.m.
New subject: [libvirt] [PATCH v3 2/2] qemu: add a target type check when hot/cold-plug a Chr device
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(a)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
8:37 a.m.
New subject: [libvirt] [PATCH v3 2/2] qemu: add a target type check when hot/cold-plug a Chr device
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(a)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
9:39 a.m.
New subject: [libvirt] [PATCH v3 2/2] qemu: add a target type check when hot/cold-plug a Chr device
On 01/22/2015 03:37 PM, Peter Krempa wrote:
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.
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(a)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
11:19 a.m.
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(a)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
2:20 p.m.
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(a)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
2:25 p.m.
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:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1164627
>>
...
> 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
3:12 p.m.
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:
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1164627
>>>
...
>> 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
3528
days inactive
3528
days old
7 comments
3 participants
participants (3)
-
lhuang -
Luyao Huang
-
Peter Krempa