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