Re: [Xen-devel] [RFC PATCH v2 03/17] libxl: Handle Linux stubdomain specific QEMU options.

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

Marek Marczykowski-Górecki writes ("[RFC PATCH v2 03/17] libxl: Handle Linux 
stubdomain specific QEMU options."):
> From: Eric Shelton <eshelton@xxxxxxxxx>
> 
> This patch creates an appropriate command line for the QEMU instance
> running in a Linux-based stubdomain.
...
> -static const char *libxl_tapif_script(libxl__gc *gc)
> +static const char *libxl_tapif_script(libxl__gc *gc,
> +                                      const libxl_domain_build_info *info)
>  {
>  #if defined(__linux__) || defined(__FreeBSD__)
> +    if (info->stubdomain_version == LIBXL_STUBDOMAIN_VERSION_LINUX)
> +        return libxl__sprintf(gc, "/etc/qemu-ifup");
> +    return libxl__strdup(gc, "no");
> +#else
> +    return GCSPRINTF("%s/qemu-ifup", libxl__xen_script_dir_path());
> +#endif
> +}
> +
> +static const char *libxl_tapif_downscript(libxl__gc *gc,
> +                                          const libxl_domain_build_info 
> *info)
> +{
> +#if defined(__linux__) || defined(__FreeBSD__)
> +    if (info->stubdomain_version == LIBXL_STUBDOMAIN_VERSION_LINUX)
> +        return libxl__sprintf(gc, "/etc/qemu-ifdown");

We should never have permitted this #ifdefery.  The resulting diff
here is almost incomprehensible due to the 3 levels of improper
nesting: diff, ifdef, and code.

Also we do not currently support any dom0's other than Linux and
FreeBSD anyway!  So the #ifdef is entirely redundant.  This wasn't 
noticed when 2b2ef0c54459722943db6166da28e098af12a9e6
  "libxl: don't use a qemu-ifup script on FreeBSD"
was prepared and accepted.

AFAICT this part of this patch is separating out the down and up
versions of libxl_tapif_script.  The resulting two functions are quite
similar though.

I suggest the following series of small changes:
   1. Drop the #if and all the code in the #else
   2. Add a libxl__device_action parameter to libxl_tapif_script
   3. Make your new code check for linux stubdom and if so
       pass    "qemu" + (action == add) ? "up" :
                        (action == remove) ? "down" : (abort(),0)
       or some such

What do you think ?

> @@ -1099,10 +1118,31 @@ static int 
> libxl__build_device_model_args_new(libxl__gc *gc,
>                  return ERROR_INVAL;
>              }
>              if (b_info->u.hvm.serial) {
> -                flexarray_vappend(dm_args,
> -                                  "-serial", b_info->u.hvm.serial, NULL);
> -            } else if (b_info->u.hvm.serial_list) {
> +                if (is_stubdom) {
> +                    flexarray_vappend(dm_args,
> +                                      "-serial",
> +                                      GCSPRINTF("/dev/hvc%d",
> +                                                STUBDOM_CONSOLE_SERIAL),
> +                                      NULL);
> +                } else {
> +                    flexarray_vappend(dm_args,
> +                                      "-serial", b_info->u.hvm.serial, NULL);
> +                }
> +            } else if (b_info->u.hvm.serial_list &&
> +                    b_info->u.hvm.serial_list[0]) {
>                  char **p;
> +                if (is_stubdom) {
> +                    if (b_info->u.hvm.serial_list[1]) {

This can't possibly be right.  The 2nd if is in the else of a
condition which will always catch all of its cases.

Also,

> +                    flexarray_vappend(dm_args,
> +                                      "-serial",
> +                                      GCSPRINTF("/dev/hvc%d",
> +                                                STUBDOM_CONSOLE_SERIAL),

it repeats some of the code.

> @@ -1503,7 +1543,9 @@ static int libxl__build_device_model_args_new(libxl__gc 
> *gc,
>               * If qemu isn't doing the interpreting, the parameter is
>               * always raw
>               */
> -            if (disks[i].backend == LIBXL_DISK_BACKEND_QDISK)
> +            if (libxl_defbool_val(b_info->device_model_stubdomain))
> +                format = "host_device";

So I infer that you have created in qemu a "block device format"
called "host_device" which is actually a pv frontend ?  Or are we
using Linux's blkfront here ?  In which case why not "raw" ?

> +            } else if (libxl_defbool_val(b_info->device_model_stubdomain)) {
> +                target_path = GCSPRINTF("/dev/xvd%c", 'a' + disk);

Needs an error check in case disk is too large.

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel