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

[Jim Fehlig]

On 06/22/2016 04:34 PM, Joao Martins wrote:
>
> 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!

NP. I was about to respond to that thread saying we could drop it :).

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

I didn't bisect to the troublesome commit, but just some basic poking tells me
it is not 482e5f15. I checked one of my machines running 1.2.18 and don't see
the problem there.

Regards,
Jim

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