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

[Daniel De Graaf <dgdegra@tycho.nsa.gov>]

On 08/07/2012 05:36 AM, Ian Campbell wrote:
> 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)

No, I just missed that setting that option would also do this.

> 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@tycho.nsa.gov>
>>
>> ---
>>
>> 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.

I'll add that in v2.

> [...]
>> @@ -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?

The implementation I have for xend was in the comma-separated syntax only,
but I think that may have been due to non-upstreamed patches.
 
> xl seems to use just backend for network devices. They probably ought to
> match.

Originally I had the patch using "backend" but received comments that it could
be confused with backendtype. Matching the network device specification is a
good idea, and I'm fine with either name.
 
>>  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
>>
> 
> 
> 


-- 
Daniel De Graaf
National Security Agency