Xen devel - Oct 2011 - [PATCH] xl: Support backend domain ID for disks

Allow specification of backend domains for disks, either in the config
file or via block-attach

Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
---
 tools/libxl/libxlu_disk.c   |    3 ++-
 tools/libxl/libxlu_disk_i.h |    3 ++-
 tools/libxl/libxlu_disk_l.l |    8 ++++++++
 tools/libxl/libxlutil.h     |    2 +-
 tools/libxl/xl_cmdimpl.c    |    2 +-
 5 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/tools/libxl/libxlu_disk.c b/tools/libxl/libxlu_disk.c
index f8a1ba3..3342099 100644
--- a/tools/libxl/libxlu_disk.c
+++ b/tools/libxl/libxlu_disk.c
@@ -47,7 +47,7 @@ static void dpc_dispose(DiskParseContext *dpc) {
 
 int xlu_disk_parse(XLU_Config *cfg,
                    int nspecs, const char *const *specs,
-                   libxl_device_disk *disk) {
+                   libxl_device_disk *disk, libxl_ctx *ctx) {
     DiskParseContext dpc;
     int i, e;
 
@@ -55,6 +55,7 @@ int xlu_disk_parse(XLU_Config *cfg,
     dpc.cfg = cfg;
     dpc.scanner = 0;
     dpc.disk = disk;
+    dpc.ctx = ctx;
 
     disk->readwrite = 1;
 
diff --git a/tools/libxl/libxlu_disk_i.h b/tools/libxl/libxlu_disk_i.h
index 578920a..00988fa 100644
--- a/tools/libxl/libxlu_disk_i.h
+++ b/tools/libxl/libxlu_disk_i.h
@@ -2,7 +2,7 @@
 #define LIBXLU_DISK_I_H
 
 #include "libxlu_internal.h"
-
+#include "libxl_utils.h"
 
 typedef struct {
     XLU_Config *cfg;
@@ -12,6 +12,7 @@ typedef struct {
     libxl_device_disk *disk;
     int access_set, had_depr_prefix;
     const char *spec;
+    libxl_ctx *ctx;
 } DiskParseContext;
 
 void xlu__disk_err(DiskParseContext *dpc, const char *erroneous,
diff --git a/tools/libxl/libxlu_disk_l.l b/tools/libxl/libxlu_disk_l.l
index a3e7180..446ea65 100644
--- a/tools/libxl/libxlu_disk_l.l
+++ b/tools/libxl/libxlu_disk_l.l
@@ -108,6 +108,13 @@ static void setbackendtype(DiskParseContext *dpc, const
char *str) {
     else xlu__disk_err(dpc,str,"unknown value for backendtype");
 }
 
+/* Sets ->backend_domid from the string. */
+static void setbackend(DiskParseContext *dpc, const char *str) {
+    if (libxl_name_to_domid(dpc->ctx, str,
&dpc->disk->backend_domid)) {
+        xlu__disk_err(dpc,str,"unknown domain for backend");
+    }
+}
+
 #define DEPRECATE(usewhatinstead) /* not currently reported */
 
 %}
@@ -140,6 +147,7 @@ devtype=[^,]*,?	{ xlu__disk_err(DPC,yytext,"unknown
value for type"); }
 
 access=[^,]*,?	{ STRIP('',''); setaccess(DPC, FROMEQUALS); }
 backendtype=[^,]*,? { STRIP('','');
setbackendtype(DPC,FROMEQUALS); }
+backend=[^,]*,? { STRIP('',''); setbackend(DPC,FROMEQUALS); }
 
 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 c20de1d..482c5a0 100644
--- a/tools/libxl/libxlutil.h
+++ b/tools/libxl/libxlutil.h
@@ -64,7 +64,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 99e3c49..d2749da 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -501,7 +501,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));
-- 
1.7.6.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Daniel De Graaf writes ("[Xen-devel] [PATCH] xl: Support backend domain ID
for disks"):
> Allow specification of backend domains for disks, either in the config
> file or via block-attach
This is probably going in the right direction but I have some
questions and observations.

Firstly, much of the rest of the code in libxl assumes that the
pdev_path string can (depending on the backend type) be dereferenced
by libxl.  That is, if libxl is running in dom0, it assumes that the
block device can be dereferenced in dom0.

So for this to work properly I think at least you need to investigate
the backend type selection machinery and the pdev_path validation and
make sure they are somehow disabled.

Having never done driver domains: how does the backend domain know
what it is supposed to be doing ?  Does it just get the pdev_path via
xenstore and do the rest itself ?  Does it get told the backend type ?
What is the resulting xenstore protocol ?

Is this a reason to preserve the arrangement whereby the target of
blkback is set up by a hotplug script, rather than by a script
executed directly by libxl ?


Finally, one other relatively minor thing.  I don''t think
"backend" is
the appropriate name for "backend domid".  How about
"backenddomain" ?
(This may not be compatible with xm I guess...)

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 10/17/2011 10:04 AM, Ian Jackson wrote:
> Daniel De Graaf writes ("[Xen-devel] [PATCH] xl: Support backend
domain ID for disks"):
>> Allow specification of backend domains for disks, either in the config
>> file or via block-attach
> 
> This is probably going in the right direction but I have some
> questions and observations.
> 
> Firstly, much of the rest of the code in libxl assumes that the
> pdev_path string can (depending on the backend type) be dereferenced
> by libxl.  That is, if libxl is running in dom0, it assumes that the
> block device can be dereferenced in dom0.
> 
> So for this to work properly I think at least you need to investigate
> the backend type selection machinery and the pdev_path validation and
> make sure they are somehow disabled.
Adding the ability to disable the stat() in libxl__device_disk_set_backend
seems like it would be useful separate from setting the backend in order
to support formats where the pdev_path is not a file. Do the iscsi/nbd
backend types already do this somehow?
> 
> Having never done driver domains: how does the backend domain know
> what it is supposed to be doing ?  Does it just get the pdev_path via
> xenstore and do the rest itself ?  Does it get told the backend type ?
> What is the resulting xenstore protocol ?
> 
> Is this a reason to preserve the arrangement whereby the target of
> blkback is set up by a hotplug script, rather than by a script
> executed directly by libxl ?
For the "phy" backend type, libxl can populate this correctly from
outside
the backend as long as it can determine proper device major/minor numbers 
for the backend''s kernel (perhaps by sharing the backend''s
/dev via NFS).
Other backend types like blktap2 that require scripts to be started will 
require switching back to starting the devices via hotplug. I do think
running the script directly from libxl when possible is a good idea as this
makes it easier to debug.
> Finally, one other relatively minor thing.  I don''t think
"backend" is
> the appropriate name for "backend domid".  How about
"backenddomain" ?
> (This may not be compatible with xm I guess...)
> 
This was chosen to match the backend specification for network devices,
but for disks it is confusing with "backendtype" already taken. Since
smashed-together names are hard to read, would "backend_domain" be a
better choice?

-- 
Daniel De Graaf
National Security Agency

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Daniel De Graaf writes ("Re: [Xen-devel] [PATCH] xl: Support backend domain
ID for disks"):
> Adding the ability to disable the stat() in libxl__device_disk_set_backend
> seems like it would be useful separate from setting the backend in order
> to support formats where the pdev_path is not a file. Do the iscsi/nbd
> backend types already do this somehow?
No, but they don''t currently work, either :-/.  This is certainly
needed.
> For the "phy" backend type, libxl can populate this correctly
from outside
> the backend as long as it can determine proper device major/minor numbers 
> for the backend''s kernel (perhaps by sharing the
backend''s /dev via NFS).
OMG, that''s horrible.
> Other backend types like blktap2 that require scripts to be started will 
> require switching back to starting the devices via hotplug. I do think
> running the script directly from libxl when possible is a good idea as this
> makes it easier to debug.
I think that in the New World Order, a driver domain should be told
the pdev_path and left to get on with it.  So something in the driver
domain needs to watch xenstore.  Perhaps a BSD-style backendd ?
> This was chosen to match the backend specification for network devices,
> but for disks it is confusing with "backendtype" already taken.
Since
> smashed-together names are hard to read, would "backend_domain"
be a
> better choice?
If we have "backendtype" then we already have squashed together names.
But let''s see what other people say about the colour of this bikeshed.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 10/17/2011 11:42 AM, Ian Jackson wrote:
> Daniel De Graaf writes ("Re: [Xen-devel] [PATCH] xl: Support backend
domain ID for disks"):
>> Adding the ability to disable the stat() in
libxl__device_disk_set_backend
>> seems like it would be useful separate from setting the backend in
order
>> to support formats where the pdev_path is not a file. Do the iscsi/nbd
>> backend types already do this somehow?
> 
> No, but they don''t currently work, either :-/.  This is certainly
> needed.
> 
If we change the stat() so that it''s only done on types that require a
file,
then all that is required is to create a "remote-phy" type that does
not do
the stat locally. This type would also avoid trying to fill in nodes like
"physical-device" that are derived from the stat(), leaving those to
be
filled in by the backend domain.
>> For the "phy" backend type, libxl can populate this correctly
from outside
>> the backend as long as it can determine proper device major/minor
numbers
>> for the backend''s kernel (perhaps by sharing the
backend''s /dev via NFS).
> 
> OMG, that''s horrible.
Agreed, it''s not a viable solution for anything long-term.
>> Other backend types like blktap2 that require scripts to be started
will
>> require switching back to starting the devices via hotplug. I do think
>> running the script directly from libxl when possible is a good idea as
this
>> makes it easier to debug.
> 
> I think that in the New World Order, a driver domain should be told
> the pdev_path and left to get on with it.  So something in the driver
> domain needs to watch xenstore.  Perhaps a BSD-style backendd ?
> 
That would make the most sense. Linux does get events when this part of
xenstore is updated, so it may be possible to fire off the needed events
from udev/hotplug depending on what can be picked up there.
>> This was chosen to match the backend specification for network devices,
>> but for disks it is confusing with "backendtype" already
taken. Since
>> smashed-together names are hard to read, would
"backend_domain" be a
>> better choice?
> 
> If we have "backendtype" then we already have squashed together
names.
> But let''s see what other people say about the colour of this
bikeshed.
> 
> Ian.
> 
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Daniel De Graaf writes ("Re: [Xen-devel] [PATCH] xl: Support backend domain
ID for disks"):
> If we change the stat() so that it''s only done on types that
require a file,
> then all that is required is to create a "remote-phy" type that
does not do
> the stat locally. This type would also avoid trying to fill in nodes like
> "physical-device" that are derived from the stat(), leaving those
to be
> filled in by the backend domain.
Right, that seems like a good approach.
> > I think that in the New World Order, a driver domain should be told
> > the pdev_path and left to get on with it.  So something in the driver
> > domain needs to watch xenstore.  Perhaps a BSD-style backendd ?
> 
> That would make the most sense. Linux does get events when this part of
> xenstore is updated, so it may be possible to fire off the needed events
> from udev/hotplug depending on what can be picked up there.
Right.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel