[libvirt] [PATCH] libxl: support hvm direct kernel boot

[Jim Fehlig]

Chunyan Liu wrote:
> Xen libxl can support Xen HVM direct kernel boot now. To support
> Xen HVM direct kernel boot in libvirt, it still needs some changes
> to libvirt code: accept HVM direct kernel boot related configuration
> in xml, parse those info into virDomainDefPtr, and fill in related
> parts of libxl_domain_build_info, so that libxl can handle. This
> patch is just to do this.
>
> Signed-off-by: Chunyan Liu <cyliu at suse.com>
> ---
>  src/libxl/libxl_conf.c     | 18 ++++++++++++++++
>  src/xenconfig/xen_common.c | 51 +++++++++++++++++++++++++++++++++-------------
>  2 files changed, 55 insertions(+), 14 deletions(-)
>
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index acba69c..a5bda64 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -704,6 +704,15 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
>          if (VIR_STRDUP(b_info->u.hvm.boot, bootorder) < 0)
>              goto error;
>  
> +#ifdef LIBXL_HAVE_BUILDINFO_KERNEL
> +        if (VIR_STRDUP(b_info->cmdline, def->os.cmdline) < 0)
> +            goto error;
> +        if (VIR_STRDUP(b_info->kernel, def->os.kernel) < 0)
> +            goto error;
> +        if (VIR_STRDUP(b_info->ramdisk, def->os.initrd) < 0)
> +            goto error;
> +#endif
> +
>          if (def->nserials) {
>              if (def->nserials > 1) {
>                  virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> @@ -748,6 +757,14 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
>                    virStringSplit(def->os.bootloaderArgs, " \t\n", 0)))
>                  goto error;
>          }
> +#ifdef LIBXL_HAVE_BUILDINFO_KERNEL
> +        if (VIR_STRDUP(b_info->cmdline, def->os.cmdline) < 0)
> +            goto error;
> +        if (VIR_STRDUP(b_info->kernel, def->os.kernel) < 0)
> +            goto error;
> +        if (VIR_STRDUP(b_info->ramdisk, def->os.initrd) < 0)
> +            goto error;
> +#else
>          if (VIR_STRDUP(b_info->u.pv.cmdline, def->os.cmdline) < 0)
>              goto error;
>          if (def->os.kernel) {
> @@ -758,6 +775,7 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
>          }
>          if (VIR_STRDUP(b_info->u.pv.ramdisk, def->os.initrd) < 0)
>              goto error;
> +#endif
>      }
>  
>      return 0;
> diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c
> index 32954f3..290ba3d 100644
> --- a/src/xenconfig/xen_common.c
> +++ b/src/xenconfig/xen_common.c
> @@ -1053,6 +1053,26 @@ xenParseGeneralMeta(virConfPtr conf, virDomainDefPtr def, virCapsPtr caps)
>      return 0;
>  }
>  
> +static int xenParseCmdline(virConfPtr conf, virDomainDefPtr def)
>   

Preferred pattern in libvirt would be

static int
xenParseCmdLine(virConfPtr conf, virDomainDefPtr def)

> +{
> +    const char *extra, *root;
> +
> +    if (xenConfigGetString(conf, "extra", &extra, NULL) < 0)
> +        return -1;
> +
> +    if (xenConfigGetString(conf, "root", &root, NULL) < 0)
> +        return -1;
> +
> +    if (root) {
> +        if (virAsprintf(&def->os.cmdline, "root=%s %s", root, extra) < 0)
> +            return -1;
> +    } else {
> +        if (VIR_STRDUP(def->os.cmdline, extra) < 0)
> +            return -1;
> +    }
> +
> +    return 0;
> +}
>  
>  static int
>  xenParseOS(virConfPtr conf, virDomainDefPtr def)
> @@ -1065,9 +1085,25 @@ xenParseOS(virConfPtr conf, virDomainDefPtr def)
>      if (STREQ(def->os.type, "hvm")) {
>          const char *boot;
>  
> +#ifdef LIBXL_HAVE_BUILDINFO_KERNEL
>   

LIBXL_HAVE_BUILDINFO_KERNEL is defined in libxl.h, which is currently
not included in this file.  You'll need to add something like

#if WITH_LIBXL
# include <libxl.h>
#endif

> +        if (xenConfigCopyStringOpt(conf, "kernel", &def->os.kernel) < 0)
>   

If 'kernel' is not set in the Xen config, def->os.kernel will be NULL...

> +            return -1;
> +
> +        if (strstr(def->os.kernel, "hvmloader")) {
>   

which will result in a segfault here.  Maybe it would be best to copy to
a temp variable, check the value, and only then assign to def->os.kernel.

> +            /* 'kernel' set to 'hvmloader' will be ignored in libxl */
> +            virFree(def->os.kernel);
> +        }
> +
> +        if (xenConfigCopyStringOpt(conf, "ramdisk", &def->os.initrd) < 0)
> +            return -1;
> +
> +        if (xenParseCmdline(conf, def) < 0)
> +           return -1;
> +#else
>          if (VIR_ALLOC(def->os.loader) < 0 ||
>              xenConfigCopyString(conf, "kernel", &def->os.loader->path) < 0)
>   

Pre-existing, but this should be xenConfigCopyStringOpt since specifying
'kernel=' is not required.

Regards,
Jim

>              return -1;
> +#endif
>  
>          if (xenConfigGetString(conf, "boot", &boot, "c") < 0)
>              return -1;
> @@ -1091,8 +1127,6 @@ xenParseOS(virConfPtr conf, virDomainDefPtr def)
>              def->os.nBootDevs++;
>          }
>      } else {
> -        const char *extra, *root;
> -
>          if (xenConfigCopyStringOpt(conf, "bootloader", &def->os.bootloader) < 0)
>              return -1;
>          if (xenConfigCopyStringOpt(conf, "bootargs", &def->os.bootloaderArgs) < 0)
> @@ -1104,19 +1138,8 @@ xenParseOS(virConfPtr conf, virDomainDefPtr def)
>          if (xenConfigCopyStringOpt(conf, "ramdisk", &def->os.initrd) < 0)
>              return -1;
>  
> -        if (xenConfigGetString(conf, "extra", &extra, NULL) < 0)
> +        if (xenParseCmdline(conf, def) < 0)
>              return -1;
> -
> -        if (xenConfigGetString(conf, "root", &root, NULL) < 0)
> -            return -1;
> -
> -        if (root) {
> -            if (virAsprintf(&def->os.cmdline, "root=%s %s", root, extra) < 0)
> -                return -1;
> -        } else {
> -            if (VIR_STRDUP(def->os.cmdline, extra) < 0)
> -                return -1;
> -        }
>      }
>  
>      return 0;
>