[libvirt] [PATCH] libxl: use serial device for console when targetType is serial

[Joao Martins]

On 06/22/2016 10:18 PM, Jim Fehlig wrote:
> On 06/22/2016 02:03 PM, Cole Robinson wrote:
>> On 06/22/2016 03:45 PM, Jim Fehlig wrote:
>>> When domXML contains only <console type='pty'> and no corresponding
>>> <serial>, the console is "stolen" [1] and used as the first <serial>
>>> device. When this "stolen" console is accessed from the libxl driver
>>> (in libxlConsoleCallback and libxlDomainOpenConsole), check if the
>>> targetType is VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL, and use the
>>> "stolen" device in def->serials[0] instead. Prior to this change,
>>> creating a domain with input XML containing only a <console> device
>>> and subsequently attempting to access its console with
>>> 'virsh console' would fail
>>>
>>> error: internal error: character device <null> is not using a PTY
>>>
>>> [1] See comments associated with virDomainDefAddConsoleCompat() in
>>>     $LIBVIRT-SRC/src/conf/domain_conf.c:
>>>
>>> Signed-off-by: Jim Fehlig <jfehlig at suse.com>
>>> ---
>>>  src/libxl/libxl_domain.c | 8 ++++++--
>>>  src/libxl/libxl_driver.c | 5 ++++-
>>>  2 files changed, 10 insertions(+), 3 deletions(-)
>>>
>>> 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,
>>> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
>>> index c8c1f07..3189f1c 100644
>>> --- a/src/libxl/libxl_driver.c
>>> +++ b/src/libxl/libxl_driver.c
>>> @@ -4443,8 +4443,11 @@ libxlDomainOpenConsole(virDomainPtr dom,
>>>  
>>>      priv = vm->privateData;
>>>  
>>> -    if (vm->def->nconsoles)
>>> +    if (vm->def->nconsoles) {
>>>          chr = vm->def->consoles[0];
>>> +        if (chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL)
>>> +            chr = vm->def->serials[0];
>>> +    }
>>>  
>>>      if (!chr) {
>>>          virReportError(VIR_ERR_INTERNAL_ERROR,
>>>
>> ACK (if my ACK is sufficient for functional libxl changes :) )
> 
> Thanks, I've pushed this. You recognized the convention and provided half the
> patch, so I think it is sufficient :).
Saw this a little late - but it seems that you shot two birds with this single patch.
Since we're now effectively changing the serials as opposed the consoles def (as per
Cole mentioned convention) you also ended up fixing the bug about the source path not
being in the XML. So I am dropping my patch [0] :) Thanks!

Not sure about this, but it appears this patch could be -maint material? IIUC, these
bugs have been here for a fair amount of releases potentially dating back more than
one year.

Joao

[0] https://www.redhat.com/archives/libvir-list/2016-June/msg01312.html