[libvirt PATCH 0/2] fix console device access in ch driver
Patch1: Drops the check to only have a console/serial device configured for ch VMs. This is no longer needed. Patch2: Addresses https://gitlab.com/libvirt/libvirt/-/issues/344. Libvirt adds compat console devices for "hvm" VMs. The added console/serial device type is initialized to VIR_DOMAIN_CHR_TYPE_NULL, when in fact the device added is a pty device. This patch modifies the default console device type to "pty". I looked at NOT adding Compat Console devices for ch VMs. But this looked like a better fix. Praveen K Paladugu (2): ch: drop the check to have a single console/serial conf: set the default chr device type to pty src/ch/ch_domain.c | 9 +-------- src/conf/domain_conf.c | 1 + 2 files changed, 2 insertions(+), 8 deletions(-) -- 2.27.0
Starting with version 18.0, cloud-hypervisor supports a console and a serial device in parallel. https://github.com/cloud-hypervisor/cloud-hypervisor/issues/3012 Signed-off-by: Praveen K Paladugu <prapal@linux.microsoft.com> --- src/ch/ch_domain.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/src/ch/ch_domain.c b/src/ch/ch_domain.c index ae53c6fe05..bf687a6c32 100644 --- a/src/ch/ch_domain.c +++ b/src/ch/ch_domain.c @@ -281,14 +281,7 @@ chValidateDomainDeviceDef(const virDomainDeviceDef *dev, return -1; } - if ((def->nconsoles && - def->consoles[0]->source->type == VIR_DOMAIN_CHR_TYPE_PTY) - && (def->nserials && - def->serials[0]->source->type == VIR_DOMAIN_CHR_TYPE_PTY)) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Only a single console or serial can be configured for this domain")); - return -1; - } else if (def->nconsoles > 1) { + if (def->nconsoles > 1) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Only a single console can be configured for this domain")); return -1; -- 2.27.0
On Fri, Jul 29, 2022 at 20:36:56 +0000, Praveen K Paladugu wrote:
Starting with version 18.0, cloud-hypervisor supports a console and a serial device in parallel. https://github.com/cloud-hypervisor/cloud-hypervisor/issues/3012
Signed-off-by: Praveen K Paladugu <prapal@linux.microsoft.com> --- src/ch/ch_domain.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-)
diff --git a/src/ch/ch_domain.c b/src/ch/ch_domain.c index ae53c6fe05..bf687a6c32 100644 --- a/src/ch/ch_domain.c +++ b/src/ch/ch_domain.c @@ -281,14 +281,7 @@ chValidateDomainDeviceDef(const virDomainDeviceDef *dev, return -1; }
- if ((def->nconsoles && - def->consoles[0]->source->type == VIR_DOMAIN_CHR_TYPE_PTY) - && (def->nserials && - def->serials[0]->source->type == VIR_DOMAIN_CHR_TYPE_PTY)) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Only a single console or serial can be configured for this domain")); - return -1; - } else if (def->nconsoles > 1) { + if (def->nconsoles > 1) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Only a single console can be configured for this domain")); return -1;
So, what happens now when newer libvirt is used with an older cloud hypervisor version and the configuration which was previously rejected would be used? In case of the qemu driver we have a concept of hypervisor capabilities, which are detected based on the installed hypervisor version, and in case some feature was added later, the capabilities are consulted to provide a proper error in case an older version is used.
explicitly set the chr device type to pty to pass the checks of ch driver. https://gitlab.com/libvirt/libvirt/-/issues/344 Signed-off-by: Praveen K Paladugu <prapal@linux.microsoft.com> --- src/conf/domain_conf.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e85cc1f809..abe9d8ae08 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10078,6 +10078,7 @@ virDomainChrSourceDefNew(virDomainXMLOption *xmlopt) if (!(def = virObjectNew(virDomainChrSourceDefClass))) return NULL; + def->type = VIR_DOMAIN_CHR_TYPE_PTY; if (xmlopt && xmlopt->privateData.chrSourceNew && !(def->privateData = xmlopt->privateData.chrSourceNew())) { -- 2.27.0
On Fri, Jul 29, 2022 at 20:36:57 +0000, Praveen K Paladugu wrote:
explicitly set the chr device type to pty to pass the checks of ch driver. https://gitlab.com/libvirt/libvirt/-/issues/344
This is not a persuading enough enough commit message ...
Signed-off-by: Praveen K Paladugu <prapal@linux.microsoft.com> --- src/conf/domain_conf.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e85cc1f809..abe9d8ae08 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10078,6 +10078,7 @@ virDomainChrSourceDefNew(virDomainXMLOption *xmlopt)
if (!(def = virObjectNew(virDomainChrSourceDefClass))) return NULL; + def->type = VIR_DOMAIN_CHR_TYPE_PTY;
... to justify a global change like this. Could you please at least clarify which code path is problematic? I see e.g. that virDomainChrDefParseXML sets VIR_DOMAIN_CHR_TYPE_PTY as the source type if the user doesn't set one. If this is a workaround for the stub console added by default in virDomainDefAddConsoleCompat and the cloud hypervisor e.g. doesn't need that or it's wrong for it's usage, the modification shoud IMO be limited to the cloud hypervisor similarly to what I suggested in the issue. Note that we require commit messages to be self-explanatory without refering to e.g. outside discussions.
On 8/1/2022 8:01 AM, Peter Krempa wrote:
On Fri, Jul 29, 2022 at 20:36:57 +0000, Praveen K Paladugu wrote:
explicitly set the chr device type to pty to pass the checks of ch driver. https://gitlab.com/libvirt/libvirt/-/issues/344
This is not a persuading enough enough commit message ...
Signed-off-by: Praveen K Paladugu <prapal@linux.microsoft.com> --- src/conf/domain_conf.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e85cc1f809..abe9d8ae08 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10078,6 +10078,7 @@ virDomainChrSourceDefNew(virDomainXMLOption *xmlopt)
if (!(def = virObjectNew(virDomainChrSourceDefClass))) return NULL; + def->type = VIR_DOMAIN_CHR_TYPE_PTY;
... to justify a global change like this.
Could you please at least clarify which code path is problematic? I see e.g. that virDomainChrDefParseXML sets VIR_DOMAIN_CHR_TYPE_PTY as the source type if the user doesn't set one.
To summarize the issue, a post parsing step appends a stub console via virDomainDefAddConsoleCompat to Guest Domain XML. Below is the trace for how the stub console is added. virDomainDefPostParse -> virDomainDefPostParseCommon -> virDomainDefAddImplicitDevices -> virDomainDefAddConsoleCompat The stub console added in virDomainDefAddConsoleCompat is initialized to type VIR_DOMAIN_CHR_TYPE_NULL, when a zero_intialized console object is created. Cloud-Hypervisor driver's deviceValidateCallback (chValidateDomainDeviceDef) checks that the type of stub console is not VIR_DOMAIN_CHR_TYPE_PTY and returns an error. Because of the above sequence of steps, cloud-hypervisor VMs fail to be created with the following error: $ virsh -c ch:///system create /files/ch-impish.xml error: Failed to create domain from /nfs/ch-impish.xml error: internal error: Console can only be enabled for a PTY
If this is a workaround for the stub console added by default in virDomainDefAddConsoleCompat and the cloud hypervisor e.g. doesn't need that or it's wrong for it's usage, the modification shoud IMO be limited to the cloud hypervisor similarly to what I suggested in the issue.
By running a test Qemu VM, I verified that virDomainDefAddConsoleCompat is adding a console of type PTY. By passing below serial device configuration for a Qemu VM: <serial type='pty'> <target port='0'/> </serial> I see the below stub console being added: $ virsh dumpxml qemu_impish ... <serial type='pty'> <source path='/dev/pts/1'/> <target type='isa-serial' port='0'> <model name='isa-serial'/> </target> <alias name='serial0'/> </serial> <console type='pty' tty='/dev/pts/1'> <source path='/dev/pts/1'/> <target type='serial' port='0'/> <alias name='serial0'/> </console> ... So, I wanted to check if setting the default chr device to PTY makes sense. Thinking further, it makes sense to limit the change to "ch" driver only. I will send out an updated patch with the suggested change. Hope the above details provide enough context on the source of the issue.
Note that we require commit messages to be self-explanatory without refering to e.g. outside discussions.
Thanks, will add all relevant details in the commit message itself, in my next revision. -- Regards, Praveen K Paladugu
participants (2)
-
Peter Krempa -
Praveen K Paladugu