[libvirt] [PATCH 0/2]qemu: output error when try to hotplug/coldplug a unsupported device
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
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@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
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@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;
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@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
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@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
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@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",
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@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
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@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
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@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
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@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",
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@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
participants (4)
-
John Ferlan -
Ján Tomko -
lhuang -
Luyao Huang