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

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Tue, Oct 16, 2018 at 06:16:41PM +0100, Ian Jackson wrote:
> 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 ?

Given updated stubdomain doesn't need qemu-ifup/ifdown at all, this
whole chunk can be dropped.

> > @@ -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.

Oh, indeed.

> 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" ?

"format" is actually passed to "driver" option for -drive. And according
to qemu documentation, "raw" should be used only for regular file, not
block devices. And since qemu 3.0, "raw" driver rejects block devices[1].

> > +            } 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.

Ok.

> Thanks,
> Ian.

[1] 
https://qemu.weilnetz.de/doc/qemu-doc.html#g_t_002ddrive-file_003djson_003a_007b_002e_002e_002e_007b_0027driver_0027_003a_0027file_0027_007d_007d-_0028since-3_002e0_0029

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

Attachment: signature.asc
Description: PGP signature

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