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

[Joao Martins]

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?

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>

Joao

> Regards,
> Jim
> 
>> Finally, the path set in consoles array won't persist
>> across daemon reboots, thus rendering admins/users with no access to
>> console with a message such as:
>>
>> "error: operation failed: PTY device is not yet assigned"
>>
>> This is because: for HVM guests, DefFormatInternal will ignore the
>> console element and instead write it with the content of the serial
>> element for target type = serial which isn't touched in our
>> libxlConsoleCallback. To address that we introduce a new helper routine
>> libxlConsoleSetSourcePath() that sets the source path and thus also
>> handling this case. That is by setting the source path on with serial
>> element akin to the one indexed by console "port". This way we keep
>> similar behaviour for PV and HVM guests.
>>
>> Signed-off-by: Joao Martins <joao.m.martins at oracle.com>
>> ---
>>  src/libxl/libxl_domain.c | 35 +++++++++++++++++++++++++++++++----
>>  1 file changed, 31 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
>> index 221af87..4a46143 100644
>> --- a/src/libxl/libxl_domain.c
>> +++ b/src/libxl/libxl_domain.c
>> @@ -955,6 +955,35 @@ libxlNetworkPrepareDevices(virDomainDefPtr def)
>>      return 0;
>>  }
>>  
>> +static int
>> +libxlConsoleSetSourcePath(virDomainDefPtr def, virDomainChrDefPtr console,
>> +                          char *path)
>> +{
>> +    int ret = -1;
>> +    size_t i;
>> +
>> +    if (!path || path[0] == '\0')
>> +        return ret;
>> +
>> +    if (VIR_STRDUP(console->source.data.file.path, path) < 0)
>> +        return ret;
>> +
>> +    if (console->targetType != VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL)
>> +        return 0;
>> +
>> +    for (i = 0; i < def->nserials; i++) {
>> +        virDomainChrDefPtr serial = def->serials[i];
>> +
>> +        if (serial->target.port == console->target.port &&
>> +            VIR_STRDUP(serial->source.data.file.path, path) >= 0) {
>> +            ret = 0;
>> +            break;
>> +        }
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>>  static void
>>  libxlConsoleCallback(libxl_ctx *ctx, libxl_event *ev, void *for_callback)
>>  {
>> @@ -977,10 +1006,8 @@ libxlConsoleCallback(libxl_ctx *ctx, libxl_event *ev, void *for_callback)
>>                                          &console);
>>              if (!ret) {
>>                  VIR_FREE(chr->source.data.file.path);
>> -                if (console && console[0] != '\0') {
>> -                    ignore_value(VIR_STRDUP(chr->source.data.file.path,
>> -                                            console));
>> -                }
>> +                if (libxlConsoleSetSourcePath(vm->def, chr, console) < 0)
>> +                    VIR_WARN("Failed to set console source path");
>>              }
>>              VIR_FREE(console);
>>          }
>