7:48 a.m.
When use attach-device to hotplug a qemu unsupported console, command
will success and add a XML to the running guest, but donnot do anything
in qemu side. Add a check in qemuBuildConsoleChrDeviceStr, and output a
error when try to attach a qemu unsupport console.
About report error for qemu unsupported Chr device when cold-plug,
I think this maybe unnessary in this place, because we will check it
when we start the guest and it will report a clear error.But if we
use qemu* header func add a qemu unsupported things to qemu guest, it seems
strange.
Luyao Huang (2):
qemu: output error when try to hotplug unsupport console
qemu: add a check when cold-plug a Chr device
src/qemu/qemu_command.c | 6 ++++-
src/qemu/qemu_hotplug.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 69 insertions(+), 1 deletion(-)
--
1.8.3.1
7:48 a.m.
New subject: [libvirt] [PATCHv2 1/2] qemu: output error when try to hotplug unsupport console
https://bugzilla.redhat.com/show_bug.cgi?id=1164627
When try to use attach-device to hotplug a qemu
unsupport sonsole, command will return success,
and add a console in vm's xml.But this doesn't work
in qemu, qemu doesn't support these console.Add
a error output when try to hotplug a unsupport console
in qemuBuildConsoleChrDeviceStr.
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 1399ce4..7dced5f 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -10069,13 +10069,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)));
+ return ret;
}
ret = 0;
--
1.8.3.1
7:59 p.m.
New subject: [libvirt] [PATCHv2 1/2] qemu: output error when try to hotplug unsupport console
Looks like this one may have been "lost" in the holidays...
On 12/09/2014 01:48 AM, Luyao Huang wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1164627
When try to use attach-device to hotplug a qemu
unsupport sonsole, command will return success,
and add a console in vm's xml.But this doesn't work
in qemu, qemu doesn't support these console.Add
a error output when try to hotplug a unsupport console
in qemuBuildConsoleChrDeviceStr.
Needs some grammar fixups...
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 1399ce4..7dced5f 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -10069,13 +10069,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)));
+ return ret;
This should be a goto cleanup;
I can clean this up; however, will it be necessary given patch 2/2?
John
}
ret = 0;
3:57 a.m.
New subject: [libvirt] [PATCHv2 1/2] qemu: output error when try to hotplug unsupport console
On 01/21/2015 02:59 AM, John Ferlan wrote:
Looks like this one may have been "lost" in the holidays...
Aha, thanks a lot for 'save' this patch, I almost forget this patch ;)
On 12/09/2014 01:48 AM, Luyao Huang wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1164627
>
> When try to use attach-device to hotplug a qemu
> unsupport sonsole, command will return success,
> and add a console in vm's xml.But this doesn't work
> in qemu, qemu doesn't support these console.Add
> a error output when try to hotplug a unsupport console
> in qemuBuildConsoleChrDeviceStr.
Needs some grammar fixups...
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.
Thanks a lot for improving the description.
> 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 1399ce4..7dced5f 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -10069,13 +10069,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)));
> + return ret;
This should be a goto cleanup;
I can clean this up; however, will it be necessary given patch 2/2?
Thanks and about Patch 2/2 is not fix this bug (although patch 2/2 will
also fix hot-plug a qemu unsupported chr device). I have a discussion
with Jan and Pavel in v1 of this patch (please see
https://www.redhat.com/archives/libvir-list/2014-December/msg00157.html
and
https://www.redhat.com/archives/libvir-list/2014-November/msg00988.html), so
write patch 2/2 for avoiding cold-plug a qemu unsupported device to a
qemu vm.
John
> }
>
> ret = 0;
>
Luyao
7:48 a.m.
New subject: [libvirt] [PATCH 2/2] qemu: add a check when cold-plug a Chr device
Add a func just check the base target type which
qemu support. But i still doubt this will be useful
, we already have a check when we try to start the
vm. And this check only check the target type, and
the other things will be done in virDomainChrDefParseXML.
Signed-off-by: Luyao Huang <lhuang(a)redhat.com>
---
src/qemu/qemu_hotplug.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 64 insertions(+)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index b9a0cee..fe91ec7 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1384,10 +1384,74 @@ int qemuDomainAttachRedirdevDevice(virQEMUDriverPtr driver,
}
+static int
+qemuDomainChrCheckDefSupport(virDomainChrDefPtr chr)
+{
+ int ret = -1;
+
+ 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:
+ ret = 0;
+ break;
+
+ default:
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unsupported serial target type %s for qemu"),
+
NULLSTR(virDomainChrSerialTargetTypeToString(chr->targetType)));
+ break;
+ }
+ break;
+
+ case VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL:
+ ret = 0;
+ 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:
+ ret = 0;
+ break;
+
+ default:
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unsupported channel target type %s for qemu"),
+
NULLSTR(virDomainChrChannelTargetTypeToString(chr->targetType)));
+ 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:
+ ret = 0;
+ break;
+
+ default:
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unsupported console target type %s for qemu"),
+
NULLSTR(virDomainChrConsoleTargetTypeToString(chr->targetType)));
+ break;
+ }
+ break;
+ }
+
+ return ret;
+}
+
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 p.m.
New subject: [libvirt] [PATCH 2/2] qemu: add a check when cold-plug a Chr device
On 12/09/2014 01:48 AM, Luyao Huang wrote:
Add a func just check the base target type which
qemu support. But i still doubt this will be useful
, we already have a check when we try to start the
vm. And this check only check the target type, and
the other things will be done in virDomainChrDefParseXML.
The commit message needs some massaging.
Essentially, this patch fixes the "condition" where if a guest has been
started and someone uses attach-device with (or without) the --config
option, then these checks will avoid the "next" guest being modified,
correct?
This will also cause an error earlier that patch 1/2 as
qemuDomainChrInsert is called in the path before qemuDomainAttachDeviceLive
Signed-off-by: Luyao Huang <lhuang(a)redhat.com>
---
src/qemu/qemu_hotplug.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 64 insertions(+)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index b9a0cee..fe91ec7 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1384,10 +1384,74 @@ int qemuDomainAttachRedirdevDevice(virQEMUDriverPtr driver,
}
+static int
+qemuDomainChrCheckDefSupport(virDomainChrDefPtr chr)
+{
+ int ret = -1;
+
+ 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:
+ ret = 0;
+ break;
+
+ default:
Typically in switches listing other options rather than default:
case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_LAST:
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unsupported serial target type %s for qemu"),
+
NULLSTR(virDomainChrSerialTargetTypeToString(chr->targetType)));
+ break;
+ }
+ break;
+
+ case VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL:
+ ret = 0;
+ 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:
+ ret = 0;
+ break;
+
+ default:
Same, but:
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)));
+ 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:
+ ret = 0;
+ break;
+
+ default:
Same, but:
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:
I think perhaps this one is better than 1/2, but will see if my
responses result in other opinions...
John
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unsupported console target type %s for qemu"),
+
NULLSTR(virDomainChrConsoleTargetTypeToString(chr->targetType)));
+ break;
+ }
+ break;
+ }
+
+ return ret;
+}
+
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",
4:10 a.m.
New subject: [libvirt] [PATCH 2/2] qemu: add a check when cold-plug a Chr device
On 01/21/2015 03:00 AM, John Ferlan wrote:
On 12/09/2014 01:48 AM, Luyao Huang wrote:
> Add a func just check the base target type which
> qemu support. But i still doubt this will be useful
> , we already have a check when we try to start the
> vm. And this check only check the target type, and
> the other things will be done in virDomainChrDefParseXML.
>
The commit message needs some massaging.
Essentially, this patch fixes the "condition" where if a guest has been
started and someone uses attach-device with (or without) the --config
option, then these checks will avoid the "next" guest being modified,
correct?
Right
This will also cause an error earlier that patch 1/2 as
qemuDomainChrInsert is called in the path before qemuDomainAttachDeviceLive
Yes and if this patch have been pushed then the patch 1/2 will be a
patch for improving exist code.
> Signed-off-by: Luyao Huang <lhuang(a)redhat.com>
> ---
> src/qemu/qemu_hotplug.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 64 insertions(+)
>
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index b9a0cee..fe91ec7 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -1384,10 +1384,74 @@ int qemuDomainAttachRedirdevDevice(virQEMUDriverPtr driver,
>
> }
>
> +static int
> +qemuDomainChrCheckDefSupport(virDomainChrDefPtr chr)
> +{
> + int ret = -1;
> +
> + 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:
> + ret = 0;
> + break;
> +
> + default:
Typically in switches listing other options rather than default:
case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_LAST:
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("unsupported serial target type %s for
qemu"),
> +
NULLSTR(virDomainChrSerialTargetTypeToString(chr->targetType)));
> + break;
> + }
> + break;
> +
> + case VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL:
> + ret = 0;
> + 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:
> + ret = 0;
> + break;
> +
> + default:
Same, but:
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)));
> + 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:
> + ret = 0;
> + break;
> +
> + default:
Same, but:
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:
I think perhaps this one is better than 1/2, but will see if my
responses result in other opinions...
Thanks for pointing out and i forgot cc Jan and Pavel when sent this
patch, maybe they have some other opinions.
John
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("unsupported console target type %s for
qemu"),
> +
NULLSTR(virDomainChrConsoleTargetTypeToString(chr->targetType)));
> + break;
> + }
> + break;
> + }
> +
> + return ret;
> +}
> +
> 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",
>
Luyao
1 p.m.
New subject: [libvirt] [PATCH 2/2] qemu: add a check when cold-plug a Chr device
On 01/21/2015 04:10 AM, lhuang wrote:
On 01/21/2015 03:00 AM, John Ferlan wrote:
>
> On 12/09/2014 01:48 AM, Luyao Huang wrote:
>> Add a func just check the base target type which
>> qemu support. But i still doubt this will be useful
>> , we already have a check when we try to start the
>> vm. And this check only check the target type, and
>> the other things will be done in virDomainChrDefParseXML.
>>
> The commit message needs some massaging.
>
> Essentially, this patch fixes the "condition" where if a guest has been
> started and someone uses attach-device with (or without) the --config
> option, then these checks will avoid the "next" guest being modified,
> correct?
Right
> This will also cause an error earlier that patch 1/2 as
> qemuDomainChrInsert is called in the path before qemuDomainAttachDeviceLive
>
>
Yes and if this patch have been pushed then the patch 1/2 will be a patch for
improving exist code.
ChrInsert is called after qemuBuildConsoleChrDeviceStr in AttachDevice. We
should error out earlier and include the first patch too.
>> Signed-off-by: Luyao Huang <lhuang(a)redhat.com>
>> ---
>> src/qemu/qemu_hotplug.c | 64
>> +++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 64 insertions(+)
>>
>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>> index b9a0cee..fe91ec7 100644
>> --- a/src/qemu/qemu_hotplug.c
>> +++ b/src/qemu/qemu_hotplug.c
>> @@ -1384,10 +1384,74 @@ int qemuDomainAttachRedirdevDevice(virQEMUDriverPtr
>> driver,
>> }
>> +static int
>> +qemuDomainChrCheckDefSupport(virDomainChrDefPtr chr)
>> +{
>> + int ret = -1;
>> +
>> + 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:
>> + ret = 0;
>> + break;
>> +
>> + default:
> Typically in switches listing other options rather than default:
The point of casting it to virDomainChrSerialTargetType is to catch all the
unhandled values and it only works if there's no default: clause.
Also I don't think we need the ret variable at all. Just return 0 at the end
of the function, or -1 if we reached an unsupported combination.
>
>
> I think perhaps this one is better than 1/2, but will see if my
> responses result in other opinions...
Even if this one fixes the bug, 1/2 is nice to have.
Thanks for pointing out and i forgot cc Jan and Pavel when sent this patch,
maybe they have some other opinions.
No need to cc me, I am subscribed to the list. :)
Jan
2:03 p.m.
New subject: [libvirt] [PATCH 2/2] qemu: add a check when cold-plug a Chr device
On 01/21/2015 08:00 PM, Ján Tomko wrote:
On 01/21/2015 04:10 AM, lhuang wrote:
> On 01/21/2015 03:00 AM, John Ferlan wrote:
>> On 12/09/2014 01:48 AM, Luyao Huang wrote:
>>> Add a func just check the base target type which
>>> qemu support. But i still doubt this will be useful
>>> , we already have a check when we try to start the
>>> vm. And this check only check the target type, and
>>> the other things will be done in virDomainChrDefParseXML.
>>>
>> The commit message needs some massaging.
>>
>> Essentially, this patch fixes the "condition" where if a guest has
been
>> started and someone uses attach-device with (or without) the --config
>> option, then these checks will avoid the "next" guest being modified,
>> correct?
> Right
>> This will also cause an error earlier that patch 1/2 as
>> qemuDomainChrInsert is called in the path before qemuDomainAttachDeviceLive
>>
>>
> Yes and if this patch have been pushed then the patch 1/2 will be a patch for
> improving exist code.
ChrInsert is called after qemuBuildConsoleChrDeviceStr in AttachDevice. We
should error out earlier and include the first patch too.
Thanks for pointing out, error output more earlier more better :)
>>> Signed-off-by: Luyao Huang <lhuang(a)redhat.com>
>>> ---
>>> src/qemu/qemu_hotplug.c | 64
>>> +++++++++++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 64 insertions(+)
>>>
>>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>>> index b9a0cee..fe91ec7 100644
>>> --- a/src/qemu/qemu_hotplug.c
>>> +++ b/src/qemu/qemu_hotplug.c
>>> @@ -1384,10 +1384,74 @@ int qemuDomainAttachRedirdevDevice(virQEMUDriverPtr
>>> driver,
>>> }
>>> +static int
>>> +qemuDomainChrCheckDefSupport(virDomainChrDefPtr chr)
>>> +{
>>> + int ret = -1;
>>> +
>>> + 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:
>>> + ret = 0;
>>> + break;
>>> +
>>> + default:
>> Typically in switches listing other options rather than default:
The point of casting it to virDomainChrSerialTargetType is to catch all the
unhandled values and it only works if there's no default: clause.
Also I don't think we need the ret variable at all. Just return 0 at the end
of the function, or -1 if we reached an unsupported combination.
Good idea ! i will remove the ret and use return in these place.
>>
>> I think perhaps this one is better than 1/2, but will see if my
>> responses result in other opinions...
Even if this one fixes the bug, 1/2 is nice to have.
> Thanks for pointing out and i forgot cc Jan and Pavel when sent this patch,
> maybe they have some other opinions.
>
No need to cc me, I am subscribed to the list. :)
I just saw other people cc the people who help review the patch when
send a new version
patch to upstream, so i think it is better to cc the people who help
review the patch when write
a new version.
Thanks a lot for your review.
Jan
Luyao
12:16 a.m.
New subject: [libvirt] [PATCH 2/2] qemu: add a check when cold-plug a Chr device
On 12/09/2014 01:48 AM, Luyao Huang wrote:
Add a func just check the base target type which
qemu support. But i still doubt this will be useful
, we already have a check when we try to start the
vm. And this check only check the target type, and
the other things will be done in virDomainChrDefParseXML.
Signed-off-by: Luyao Huang <lhuang(a)redhat.com>
---
src/qemu/qemu_hotplug.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 64 insertions(+)
I forgot to add that this didn't build without:
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -145,6 +145,8 @@ virDomainBlockedReasonTypeFromString;
virDomainBlockedReasonTypeToString;
virDomainCapabilitiesPolicyTypeToString;
virDomainCapsFeatureTypeToString;
+virDomainChrChannelTargetTypeFromString;
+virDomainChrChannelTargetTypeToString;
virDomainChrConsoleTargetTypeFromString;
virDomainChrConsoleTargetTypeToString;
virDomainChrDefForeach;
Since it seems you'll be resending your patches, I'll wait for your
patches before proceeding...
John
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index b9a0cee..fe91ec7 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1384,10 +1384,74 @@ int qemuDomainAttachRedirdevDevice(virQEMUDriverPtr driver,
}
+static int
+qemuDomainChrCheckDefSupport(virDomainChrDefPtr chr)
+{
+ int ret = -1;
+
+ 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:
+ ret = 0;
+ break;
+
+ default:
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unsupported serial target type %s for qemu"),
+
NULLSTR(virDomainChrSerialTargetTypeToString(chr->targetType)));
+ break;
+ }
+ break;
+
+ case VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL:
+ ret = 0;
+ 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:
+ ret = 0;
+ break;
+
+ default:
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unsupported channel target type %s for qemu"),
+
NULLSTR(virDomainChrChannelTargetTypeToString(chr->targetType)));
+ 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:
+ ret = 0;
+ break;
+
+ default:
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unsupported console target type %s for qemu"),
+
NULLSTR(virDomainChrConsoleTargetTypeToString(chr->targetType)));
+ break;
+ }
+ break;
+ }
+
+ return ret;
+}
+
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",
2:45 a.m.
New subject: [libvirt] [PATCH 2/2] qemu: add a check when cold-plug a Chr device
On 01/22/2015 07:16 AM, John Ferlan wrote:
On 12/09/2014 01:48 AM, Luyao Huang wrote:
> Add a func just check the base target type which
> qemu support. But i still doubt this will be useful
> , we already have a check when we try to start the
> vm. And this check only check the target type, and
> the other things will be done in virDomainChrDefParseXML.
>
> Signed-off-by: Luyao Huang <lhuang(a)redhat.com>
> ---
> src/qemu/qemu_hotplug.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 64 insertions(+)
>
I forgot to add that this didn't build without:
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -145,6 +145,8 @@ virDomainBlockedReasonTypeFromString;
virDomainBlockedReasonTypeToString;
virDomainCapabilitiesPolicyTypeToString;
virDomainCapsFeatureTypeToString;
+virDomainChrChannelTargetTypeFromString;
+virDomainChrChannelTargetTypeToString;
virDomainChrConsoleTargetTypeFromString;
virDomainChrConsoleTargetTypeToString;
virDomainChrDefForeach;
Since it seems you'll be resending your patches, I'll wait for your
patches before proceeding1...
Thanks a lot for pointing out, I will add these code in next version.
John
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index b9a0cee..fe91ec7 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -1384,10 +1384,74 @@ int qemuDomainAttachRedirdevDevice(virQEMUDriverPtr driver,
>
> }
>
> +static int
> +qemuDomainChrCheckDefSupport(virDomainChrDefPtr chr)
> +{
> + int ret = -1;
> +
> + 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:
> + ret = 0;
> + break;
> +
> + default:
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("unsupported serial target type %s for
qemu"),
> +
NULLSTR(virDomainChrSerialTargetTypeToString(chr->targetType)));
> + break;
> + }
> + break;
> +
> + case VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL:
> + ret = 0;
> + 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:
> + ret = 0;
> + break;
> +
> + default:
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("unsupported channel target type %s for
qemu"),
> +
NULLSTR(virDomainChrChannelTargetTypeToString(chr->targetType)));
> + 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:
> + ret = 0;
> + break;
> +
> + default:
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("unsupported console target type %s for
qemu"),
> +
NULLSTR(virDomainChrConsoleTargetTypeToString(chr->targetType)));
> + break;
> + }
> + break;
> + }
> +
> + return ret;
> +}
> +
> 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",
>
Luyao
3525
days inactive
3569
days old
10 comments
4 participants
participants (4)
-
John Ferlan -
Ján Tomko -
lhuang -
Luyao Huang