3:36 p.m.
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
3:36 p.m.
New subject: [libvirt PATCH 1/2] ch: drop the check to have a single console/serial
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(a)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
8:03 a.m.
New subject: [libvirt PATCH 1/2] ch: drop the check to have a single console/serial
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(a)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.
3:36 p.m.
New subject: [libvirt PATCH 2/2] conf: set the default chr device type to pty
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(a)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
8:01 a.m.
New subject: [libvirt PATCH 2/2] conf: set the default chr device type to pty
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(a)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.
11:01 a.m.
New subject: [libvirt PATCH 2/2] conf: set the default chr device type to pty
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(a)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
843
days inactive
847
days old
5 comments
2 participants
participants (2)
-
Peter Krempa -
Praveen K Paladugu