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@xxxxxxxxxxxxxxxxxxx>
---
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
