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

[Daniel P. Berrange]

On Thu, Jul 18, 2013 at 08:14:17PM +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 <bjzhang at suse.com>
> ---
> 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   |  88 ++++++++++++++++++++++++++++++++++
>  src/libxl/libxl_conf.h   |   3 ++
>  src/libxl/libxl_driver.c | 122 +++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 213 insertions(+)
> 

> +static int
>  libxlMakeDomBuildInfo(virDomainDefPtr def, libxl_domain_config *d_config)
>  {
>      libxl_domain_build_info *b_info = &d_config->b_info;
> @@ -403,6 +487,10 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, libxl_domain_config *d_config)
>          if (VIR_STRDUP(b_info->u.hvm.boot, bootorder) < 0)
>              goto error;
>  
> +        if (def->nserials &&
> +            (libxlMakeChrdevStr(def->serials[0], &b_info->u.hvm.serial) < 0))
> +            goto error;

If you're going to hardcode  def->serials[0], then you should explicitly
report an error if  'def->nserials > 1'.

Also you should probably report an error if  def->nparallels != 0, since
you don't support that at all.

> +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;
> +    size_t i;
> +    virDomainChrDefPtr chr = NULL;
> +    libxlDomainObjPrivatePtr priv;
> +    char *console = NULL;
> +    size_t num = 0;
> +    libxl_console_type type;
> +
> +    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 (dev_name) {
> +        for (i = 0; !chr && i < vm->def->nconsoles; i++) {
> +            if (vm->def->consoles[i]->info.alias &&
> +                STREQ(dev_name, vm->def->consoles[i]->info.alias)) {
> +                chr = vm->def->consoles[i];
> +                num = i;
> +            }
> +        }
> +        for (i = 0; !chr && i < vm->def->nserials; i++) {
> +            if (STREQ(dev_name, vm->def->serials[i]->info.alias)) {
> +                chr = vm->def->serials[i];
> +                num = i;
> +            }
> +        }
> +        for (i = 0; !chr && i < vm->def->nparallels; i++) {
> +            if (STREQ(dev_name, vm->def->parallels[i]->info.alias)) {
> +                chr = vm->def->parallels[i];
> +                num = i;
> +            }
> +        }

Nothing in the libxl driver appears to ever set the info.alias string
names. So this code will crash on a NULL pointer. You should either
ensure the alias is set, or use STREQ_NULLABLE to handle NULL pointers.

> +    } else {
> +        if (vm->def->nconsoles)
> +            chr = vm->def->consoles[0];
> +        else if (vm->def->nserials)
> +            chr = vm->def->serials[0];
> +    }

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|