Re: [PATCH RFC] libxl: set serial source path for console type=serial

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 06/22/2016 12:45 PM, Cole Robinson wrote:
> On 06/22/2016 12:45 PM, Jim Fehlig wrote:
>> On 06/22/2016 06:07 AM, Cole Robinson wrote:
>>> On 06/21/2016 10:32 PM, Jim Fehlig wrote:
>>>> On 06/21/2016 04:20 AM, Joao Martins wrote:
>>>>> On 06/21/2016 01:38 AM, Jim Fehlig wrote:
>>>>>> Joao Martins wrote:
>>>>>>> Guests use a <console /> (and sometimes <serial /> pair) to represent
>>>>>>> the console. On the callback that is called when console is brought up
>>>>>>> (NB: before domain starts), we fill the path of the console element with
>>>>>>> the appropriate "/dev/pts/X". For PV guests it all works fine, although
>>>>>>> for HVM guests it doesn't. Consequently we end up seeing erronous
>>>>>>> behaviour on HVM guests where console path is not displayed on the XML
>>>>>>> but still can be accessed with virDomainOpenConsole after booting guest.
>>>>>>> Consumers of this XML (e.g. Openstack) also won't be able to use the
>>>>>>> console path.
>>>>>> Can you provide example input domXML config that causes this problem?
>>>>>>
>>>>> Ah yes - I probably should include that in the commit description?
>>>> Yes, I think it helps clarify the problem  being solved by the commit.
>>>>
>>>>> So, for PV guests for an input console XML element:
>>>>>
>>>>>     <console type='pty'>
>>>>>       <target type='xen' port='0'/>
>>>>>     </console>
>>>>>
>>>>> Which then on domain start gets filled like:
>>>>>
>>>>>     <console type='pty' tty='/dev/pts/3'>
>>>>>       <source path='/dev/pts/3'/>
>>>>>       <target type='xen' port='0'/>
>>>>>     </console>
>>>>>
>>>>> Although for HVM guests we have:
>>>>>
>>>>>     <serial type='pty'>
>>>>>       <target port='0'/>
>>>>>     </serial>
>>>>>     <console type='pty'>
>>>>>       <target type='serial' port='0'/>
>>>>>     </console>
>>>>>
>>>>> And no changes happen i.e. source path isn't written there _even though_ it's
>>>>> set on the console element - being the reason why we can access the console in the
>>>>> first place. IIUC The expected output should be:
>>>>>
>>>>>     <serial type='pty'>
>>>>>       <source path='/dev/pts/4'/>
>>>>>       <target port='0'/>
>>>>>     </serial>
>>>>>     <console type='pty' tty='/dev/pts/4'>
>>>>>       <source path='/dev/pts/4'/>
>>>>>       <target type='serial' port='0'/>
>>>>>     </console>
>>>> I asked for an example after having problems testing your patch, but it turned
>>>> out to be an issue which I think was introduced long ago by commit 482e5f15. I
>>>> was testing with transient domains and noticed 'virsh create; 'virsh console'
>>>> always failed with
>>>>
>>>> error: internal error: character device <null> is not using a PTY
>>>>
>>>> My config only contained
>>>>
>>>>   <console type='pty'>
>>>>     <target port='0'/>
>>>>   </console>
>>>>
>>>> so the "really crazy backcompat stuff for consoles" in
>>>> virDomainDefAddConsoleCompat() moved the config over to def->serials[0] and
>>>> created a new def->consoles[0] without def->consoles[0].source.type set to
>>>> VIR_DOMAIN_CHR_TYPE_PTY. Without source.type set, libxlConsoleCallback() would
>>>> ignore the console and not copy the PTY path to source.data.file.path. 'virsh
>>>> console' would then fail with the error noted above.
>>>>
>>>> I spent too much time debugging this, partly because I was confused by odd
>>>> behavior with persistent domains. E.g. 'virsh define; virsh start; virsh
>>>> console' would fail with the same error, but all subsequent 'virsh shutdown;
>>>> virsh start; virsh console' sequences would work :-). That behavior was due to
>>>> vm->newDef being populated with correct console config when calling
>>>> virDomainObjSetDefTransient() in libxlDomainStart(). After the first shutdown of
>>>> the domain, vm->newDef would be copied over to vm->def, allowing subsequent
>>>> 'virsh start; virsh console' to work correctly.
>>>>
>>> Hmm, that sounds weird. Can you explain more how exactly the XML changes?
>> It only changes when defining the vm.
>>
>>>  What
>>> is the diff between the initial 'virsh define; virsh dumpxml'
>> Initial XML has
>>
>>   <console type='pty'>
>>     <target port='0'/>
>>   </console>
>>  
>> After define
>>
>>   <serial type='pty'>
>>     <target port='0'/>
>>   </serial>
>>   <console type='pty'>
>>     <target type='serial' port='0'/>
>>   </console>
>>
>> The config doesn't change after start; shutdown. But the contents of
>> virDomainDef does change, specifically def->consoles[0]->source.type changes
>> from VIR_DOMAIN_CHR_TYPE_PTY to VIR_DOMAIN_CHR_TYPE_NULL. And as mentioned
>> below, libxlConsoleCallback() only checks for source.type ==
>> VIR_DOMAIN_CHR_TYPE_PTY before copying the PTY path to
>> def->consoles[0]->source.data.file.path. Another potential fix I also mentioned
>> below is to loosen that condition in libxlConsoleCallback(). I.e. copy the path
>> if source.type equals VIR_DOMAIN_CHR_TYPE_PTY or VIR_DOMAIN_CHR_TYPE_NULL (and
>> perhaps VIR_DOMAIN_CHR_TYPE_FILE too?).
>>
> Okay I think I follow. The thing I was missing is that
> virDomainDefAddConsoleCompat will alter ->def, but that's _not_ what is
> formated/printed in the XML, which has some special handling to fully
> duplicate serials[0] XML to consoles[0], if console target type=serial

Yes, correct.

>
> I _think_ what the proper thing to do here is something like:
>
>
> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
> index 221af87..6bcb4d9 100644
> --- a/src/libxl/libxl_domain.c
> +++ b/src/libxl/libxl_domain.c
> @@ -964,13 +964,17 @@ libxlConsoleCallback(libxl_ctx *ctx, libxl_event *ev,
> void *for_callback)
>      virObjectLock(vm);
>      for (i = 0; i < vm->def->nconsoles; i++) {
>          virDomainChrDefPtr chr = vm->def->consoles[i];
> -        if (chr && chr->source.type == VIR_DOMAIN_CHR_TYPE_PTY) {
> +        if (i == 0 &&
> +            chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL)
> +            chr = vm->def->serials[0];
> +
> +        if (chr->source.type == VIR_DOMAIN_CHR_TYPE_PTY) {
>              libxl_console_type console_type;
>              char *console = NULL;
>              int ret;
>
>              console_type =
> -                (chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL ?
> +                (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL ?
>                   LIBXL_CONSOLE_TYPE_SERIAL : LIBXL_CONSOLE_TYPE_PV);
>              ret = libxl_console_get_tty(ctx, ev->domid,
>                                          chr->target.port, console_type,
>
>
> Untested, but basically, libxl driver needs to learn that if
> consoles[0].target.type == SERIAL, then you should use serials[0] instead. The
> qemu driver has some magic like this sprinkled around...

This works, with one other similar sprinkle in libxlDomainOpenConsole(). I've
sent a patch which includes both changes

https://www.redhat.com/archives/libvir-list/2016-June/msg01580.html

>
> The problem with your patch below is that it's not a whole fix, there's many
> other bits besides source.type that would technically need to be copied over.

Right. I could have expanded the patch to do that if this approach was the
correct fix.

> Rather than doing that, which has its own problems trying to keep the
> console[0] and serial[0] data in sync, the convention seems to be that console
> gets only target.type=serial and the drivers need to map that to serials[0].
> That's my reading of things anyways

The referenced patch changes the libxl driver to follow the convention :-).

Regards,
Jim

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]