[libvirt] [PATCH V3] add console support in libxl

[Jim Fehlig]

Bamvor Jian Zhang wrote:
> Hi, Denial
>
>
> thanks your reply. 
>
>
>   
>>>> "Daniel P. Berrange" 2013-7-19 下午 23:53 >>>
>>>>         
>> On Fri, Jul 19, 2013 at 11:48:56PM +0800, Bamvor Jian Zhang wrote:
>>     
>>> this patch introduce the console api in libxl driver for both pv and
>>> hvm guest. and import and update the libxlMakeChrdevStr function
>>> which was deleted in commit dfa1e1dd.
>>>
>>> Signed-off-by: Bamvor Jian Zhang 
>>> ---
>>> Changes since V2:
>>> 1), forbid parallel configure because libxl do not support it
>>> 2), only support one serial on libxl driver.
>>> 3), also remove console code in libxl driver, AFAICS serial is enough for connecting to libxl console.
>>> changes since V1:
>>> 1), add virDomainOpenConsoleEnsureACL
>>> 3), remove virReportOOMErrorFull when virAsprintf fail.
>>> 4), change size_t for non-nagetive number in libxlDomainOpenConsole
>>> size_t i;
>>> size_t num = 0;
>>> 5), fix for make check
>>> (1), replace virAsprintf with VIR_STRDUP in two places
>>> (2), delete space.
>>>
>>> src/libxl/libxl_conf.c | 100 +++++++++++++++++++++++++++++++++++++++++++++++
>>> src/libxl/libxl_conf.h | 3 ++
>>> src/libxl/libxl_driver.c | 87 +++++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 190 insertions(+)
>>>
>>> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
>>> index 4a0fba9..539d537 100644
>>> --- a/src/libxl/libxl_conf.c
>>> +++ b/src/libxl/libxl_conf.c
>>> @@ -331,6 +331,90 @@ error:
>>> }
>>>
>>> static int
>>> +libxlMakeChrdevStr(virDomainChrDefPtr def, char **buf)
>>> +{
>>> + const char *type = virDomainChrTypeToString(def->source.type);
>>> + int ret;
>>> +
>>> + if (!type) {
>>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>>> + "%s", _("unexpected chr device type"));
>>> + return -1;
>>> + }
>>> +
>>> + switch (def->source.type) {
>>> + case VIR_DOMAIN_CHR_TYPE_NULL:
>>> + case VIR_DOMAIN_CHR_TYPE_STDIO:
>>> + case VIR_DOMAIN_CHR_TYPE_VC:
>>> + case VIR_DOMAIN_CHR_TYPE_PTY:
>>> + if (VIR_STRDUP(*buf, type) < 0)
>>> + return -1;
>>> + break;
>>> +
>>> + case VIR_DOMAIN_CHR_TYPE_FILE:
>>> + case VIR_DOMAIN_CHR_TYPE_PIPE:
>>> + if (virAsprintf(buf, "%s:%s", type,
>>> + def->source.data.file.path) < 0)
>>> + return -1;
>>> + break;
>>> +
>>> + case VIR_DOMAIN_CHR_TYPE_DEV:
>>> + if (VIR_STRDUP(*buf, def->source.data.file.path) < 0)
>>> + return -1;
>>> + break;
>>> +
>>> + case VIR_DOMAIN_CHR_TYPE_UDP: {
>>> + const char *connectHost = def->source.data.udp.connectHost;
>>> + const char *bindHost = def->source.data.udp.bindHost;
>>> + const char *bindService = def->source.data.udp.bindService;
>>> +
>>> + if (connectHost == NULL)
>>> + connectHost = "";
>>> + if (bindHost == NULL)
>>> + bindHost = "";
>>> + if (bindService == NULL)
>>> + bindService = "0";
>>> +
>>> + ret = virAsprintf(buf, "udp:%s:%s@%s:%s",
>>> + connectHost,
>>> + def->source.data.udp.connectService,
>>> + bindHost,
>>> + bindService);
>>> + if (ret < 0)
>>> + return -1;
>>> + break;
>>> +
>>> + }
>>> + case VIR_DOMAIN_CHR_TYPE_TCP:
>>> + if (def->source.data.tcp.protocol == VIR_DOMAIN_CHR_TCP_PROTOCOL_TELNET)
>>> + ret = virAsprintf(buf, "telnet:%s:%s%s",
>>> + def->source.data.tcp.host,
>>> + def->source.data.tcp.service,
>>> + def->source.data.tcp.listen ? ",server,nowait" : "");
>>> + else
>>> + ret = virAsprintf(buf, "tcp:%s:%s%s",
>>> + def->source.data.tcp.host,
>>> + def->source.data.tcp.service,
>>> + def->source.data.tcp.listen ? ",server,nowait" : "");
>>> +
>>> + if (ret < 0)
>>> + return -1;
>>> + break;
>>> +
>>> + case VIR_DOMAIN_CHR_TYPE_UNIX:
>>> + ret = virAsprintf(buf, "unix:%s%s",
>>> + def->source.data.nix.path,
>>> + def->source.data.nix.listen ? ",server,nowait" : "");
>>> + if (ret < 0)
>>> + return -1;
>>> + break;
>>> +
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int
>>> libxlMakeDomBuildInfo(virDomainDefPtr def, libxl_domain_config *d_config)
>>> {
>>> libxl_domain_build_info *b_info = &d_config->b_info;
>>> @@ -403,6 +487,22 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, libxl_domain> > + if (def->nserials > 1) {
>>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>>> + "%s", _("Only one serial is supported by libxl"));
>>> + goto error;
>>> + }
>>> + if (libxlMakeChrdevStr(def->serials[0], &b_info->u.hvm.serial) < 0)
>>> + goto error;
>>> + }
>>> +
>>> + if (def->nparallels) {
>>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>>> + "%s", _("Parallel is not supported"));
>>> + goto error;
>>> + }
>>> +
>>> /*
>>> * The following comment and calculation were taken directly from
>>> * libxenlight's internal function libxl_get_required_shadow_memory():
>>> diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h
>>> index 2b4a281..861d689 100644
>>> --- a/src/libxl/libxl_conf.h
>>> +++ b/src/libxl/libxl_conf.h
>>> @@ -34,6 +34,7 @@
>>> # include "configmake.h"
>>> # include "virportallocator.h"
>>> # include "virobject.h"
>>> +# include "virchrdev.h"
>>>
>>>
>>> # define LIBXL_VNC_PORT_MIN 5900
>>> @@ -94,6 +95,8 @@ struct _libxlDomainObjPrivate {
>>>
>>> /* per domain libxl ctx */
>>> libxl_ctx *ctx;
>>> + /* console */
>>> + virChrdevsPtr devs;
>>> libxl_evgen_domain_death *deathW;
>>>
>>> /* list of libxl timeout registrations */
>>> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
>>> index 358d387..b3a8d50 100644
>>> --- a/src/libxl/libxl_driver.c
>>> +++ b/src/libxl/libxl_driver.c
>>> @@ -417,6 +417,9 @@ libxlDomainObjPrivateAlloc(void)
>>>
>>> libxl_osevent_register_hooks(priv->ctx, &libxl_event_callbacks, priv);
>>>
>>> + if (!(priv->devs = virChrdevAlloc()))
>>> + return NULL;
>>> +
>>> return priv;
>>> }
>>>
>>> @@ -429,6 +432,7 @@ libxlDomainObjPrivateDispose(void *obj)
>>> libxl_evdisable_domain_death(priv->ctx, priv->deathW);
>>>
>>> libxl_ctx_free(priv->ctx);
>>> + virChrdevFree(priv->devs);
>>> }
>>>
>>> static void
>>> @@ -4493,6 +4497,88 @@ cleanup:
>>> return ret;
>>> }
>>>
>>> +
>>> +static int
>>> +libxlDomainOpenConsole(virDomainPtr dom,
>>> + const char *dev_name,
>>> + virStreamPtr st,
>>> + unsigned int flags)
>>> +{
>>> + libxlDriverPrivatePtr driver = dom->conn->privateData;
>>> + virDomainObjPtr vm = NULL;
>>> + int ret = -1;
>>> + virDomainChrDefPtr chr = NULL;
>>> + libxlDomainObjPrivatePtr priv;
>>> + char *console = NULL;
>>> +
>>> + virCheckFlags(VIR_DOMAIN_CONSOLE_SAFE |
>>> + VIR_DOMAIN_CONSOLE_FORCE, -1);
>>> +
>>> + libxlDriverLock(driver);
>>> + vm = virDomainObjListFindByUUID(driver->domains, dom->uuid);
>>> + libxlDriverUnlock(driver);
>>> + if (!vm) {
>>> + char uuidstr[VIR_UUID_STRING_BUFLEN];
>>> + virUUIDFormat(dom->uuid, uuidstr);
>>> + virReportError(VIR_ERR_NO_DOMAIN,
>>> + _("No domain with matching uuid '%s'"), uuidstr);
>>> + goto cleanup;
>>> + }
>>> +
>>> + if (virDomainOpenConsoleEnsureACL(dom->conn, vm->def) < 0)
>>> + goto cleanup;
>>> +
>>> + if (!virDomainObjIsActive(vm)) {
>>> + virReportError(VIR_ERR_OPERATION_INVALID,
>>> + "%s", _("domain is not running"));
>>> + goto cleanup;
>>> + }
>>> +
>>> + priv = vm->privateData;
>>> +
>>> + if (vm->def->nserials)
>>> + chr = vm->def->serials[0];
>>>       
>> Now you're ignoring 'dev_name' completely which is definitely not
>> right. You should be assigning aliases to the serial devices when
>> starting the guest, so you can match them here.
>>     
> sorry, maybe i am not get your point. the pty device is get from libxl
> lib through the libxl_primary_console_get_tty. then pass it to
> virChrdevOpen. i do not understand why save it in another place.
>   

You can look at the uml or qemu driver as an example of Daniel's point. 
When a domain is started, aliases are created and assigned to the serial
devices, and are available in the device XML.  E.g.,

    <serial type='pty'>
      <source path='/dev/pts/2'/>
      <target port='0'/>
      <alias name='serial0'/>
    </serial>

The alias can then be used as 'dev_name'.

But libxl only supports one serial device, so I think its behavior wrt
aliases should be the same as the legacy Xen driver

    if (dev_name) {
        /* XXX support device aliases in future */
        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
                       _("Named device aliases are not supported"));
        goto cleanup;
    }

Support for generating aliases on domain startup and matching them in
domainOpenConsole can be added if/when libxl supports multiple serial
devices.

Regards,
Jim