Re: [Xen-devel] [PATCH RFC/for-4.2?] libxl: Support backend domain ID for disks

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2012-08-06 at 22:51 +0100, Daniel De Graaf wrote:
> Allow specification of backend domains for disks, either in the config
> file or via xl block-attach.
> 
> A version of this patch was submitted in October 2011 but was not
> suitable at the time because libxl did not support the "script=" option
> for disks in libxl. Now that this option exists, it is possible to
> specify a backend domain without needing to duplicate the device tree of
> domain providing the disk in the domain using libxl; just specify
> script=/bin/true (or any more useful script) to prevent the block script
> from running in the domain using libxl.

You can also set run_hotplug_scripts=0 in /etc/xen/xl.conf which causes
the scripts to be run from udev again -- which means they should run in
the appropriate domain automatically. (although I confess I don't
understand the reliance on script= here, so perhaps I've got the totally
wrong end of the stick)

Given that this patch was originally posted so long ago, that the
script= stuff just went in, that driver domains were on the TODO at one
point (I think) and the relative simplicity of this patch I'm leaning
towards taking this in 4.2.

> In order to support named backend domains like network-attach, the
> prototype of xlu_disk_parse in libxlutil.h needs a libxl_ctx. Without
> this parameter, it would only be only possible to support numeric domain
> IDs in the block device specification.

I'm not sure if using libxl in libxlu is a layering violation or not
(perhaps Ian J has an opinion), but I suppose it is acceptable (the
alternative is a twisty maze of callbacks).

If we are going to expose libxl down to libxlu maybe we should go all
the way and add the ctx to the XLU_Config?

> Signed-off-by: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
> 
> ---
> 
> This patch does not include the changes to tools/libxl/libxlu_disk_l.c
> and tools/libxl/libxlu_disk_l.h because the diffs contain unrelated
> changes due to different generator versions.

I'm afraid it did, but I've ignored them.

>  tools/libxl/libxlu_disk.c   |   3 +-
>  tools/libxl/libxlu_disk_i.h |   3 +-
>  tools/libxl/libxlu_disk_l.c | 581 
> ++++++++++++++++++++++----------------------
>  tools/libxl/libxlu_disk_l.h |  24 +-
>  tools/libxl/libxlu_disk_l.l |   8 +
>  tools/libxl/libxlutil.h     |   2 +-
>  tools/libxl/xl_cmdimpl.c    |   6 +-

This patch should also touch docs/misc/xl-disk-configuration.txt.

[...]
> @@ -169,6 +176,7 @@ devtype=[^,]*,?     { xlu__disk_err(DPC,yytext,"unknown 
> value for type"); }
> 
>  access=[^,]*,? { STRIP(','); setaccess(DPC, FROMEQUALS); }
>  backendtype=[^,]*,? { STRIP(','); setbackendtype(DPC,FROMEQUALS); }
> +backenddomain=[^,]*,? { STRIP(','); setbackend(DPC,FROMEQUALS); }

Is this compatible with xend? Grep doesn't show the string
"backenddomain" at all. Maybe xend simply didn't have this
functionality?

xl seems to use just backend for network devices. They probably ought to
match.

>  vdev=[^,]*,?   { STRIP(','); SAVESTRING("vdev", vdev, FROMEQUALS); }
>  script=[^,]*,? { STRIP(','); SAVESTRING("script", script, FROMEQUALS); }
> diff --git a/tools/libxl/libxlutil.h b/tools/libxl/libxlutil.h
> index 0333e55..87eb399 100644
> --- a/tools/libxl/libxlutil.h
> +++ b/tools/libxl/libxlutil.h
> @@ -72,7 +72,7 @@ const char *xlu_cfg_get_listitem(const XLU_ConfigList*, int 
> entry);
>   */
> 
>  int xlu_disk_parse(XLU_Config *cfg, int nspecs, const char *const *specs,
> -                   libxl_device_disk *disk);
> +                   libxl_device_disk *disk, libxl_ctx *ctx);
>    /* disk must have been initialised.
>     *
>     * On error, returns errno value.  Bad strings cause EINVAL and
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 138cd72..fd00d61 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -420,7 +420,7 @@ static void parse_disk_config_multistring(XLU_Config 
> **config,
>          if (!*config) { perror("xlu_cfg_init"); exit(-1); }
>      }
> 
> -    e = xlu_disk_parse(*config, nspecs, specs, disk);
> +    e = xlu_disk_parse(*config, nspecs, specs, disk, ctx);
>      if (e == EINVAL) exit(-1);
>      if (e) {
>          fprintf(stderr,"xlu_disk_parse failed: %s\n",strerror(errno));
> @@ -5335,7 +5335,7 @@ int main_networkdetach(int argc, char **argv)
>  int main_blockattach(int argc, char **argv)
>  {
>      int opt;
> -    uint32_t fe_domid, be_domid = 0;
> +    uint32_t fe_domid;
>      libxl_device_disk disk = { 0 };
>      XLU_Config *config = 0;
> 
> @@ -5351,8 +5351,6 @@ int main_blockattach(int argc, char **argv)
>      parse_disk_config_multistring
>          (&config, argc-optind, (const char* const*)argv + optind, &disk);
> 
> -    disk.backend_domid = be_domid;
> -

xm block-attach's syntax was (allegedly): 
        Usage: xm block-attach <Domain> <BackDev> <FrontDev> <Mode> [BackDomain]

i.e. it accepted backdomain on the command line. Do we want/need to
support that? I'd be happy enough not to.

>      if (dryrun_only) {
>          char *json = libxl_device_disk_to_json(ctx, &disk);
>          printf("disk: %s\n", json);
> --
> 1.7.11.2
> 



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel