[libvirt PATCH 2/2] conf: set the default chr device type to pty

[Praveen K Paladugu]

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 at 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