Daniel De Graaf
2013-Mar-14 14:23 UTC
[PATCH RESEND v3 1/2] libxl: postpone backend name resolution
This adds a backend_domname field in libxl devices that contain a
backend_domid field, allowing either a domid or a domain name to be
specified in the configuration structures. The domain name is resolved
into a domain ID in the _setdefault function when adding the device.
This change allows the backend of the block devices to be specified
(which previously required passing the libxl_ctx down into the block
device parser), and will simplify specification of backend domains in
other users of libxl.
Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
---
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.
docs/misc/xl-disk-configuration.txt | 12 +++++++++
tools/libxl/libxl.c | 49 ++++++++++++++++++++++++++++++++++++-
tools/libxl/libxl_types.idl | 5 ++++
tools/libxl/libxlu_disk_l.l | 1 +
tools/libxl/xl_cmdimpl.c | 36 ++++++---------------------
5 files changed, 73 insertions(+), 30 deletions(-)
diff --git a/docs/misc/xl-disk-configuration.txt
b/docs/misc/xl-disk-configuration.txt
index 86c16be..5bd456d 100644
--- a/docs/misc/xl-disk-configuration.txt
+++ b/docs/misc/xl-disk-configuration.txt
@@ -139,6 +139,18 @@ cdrom
Convenience alias for "devtype=cdrom".
+backend=<domain-name>
+---------------------
+
+Description: Designates a backend domain for the device
+Supported values: Valid domain names
+Mandatory: No
+
+Specifies the backend domain which this device should attach to. This
+defaults to domain 0. Specifying another domain requires setting up a
+driver domain which is outside the scope of this document.
+
+
backendtype=<backend-type>
--------------------------
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 73e0dc3..e442afc 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1730,12 +1730,35 @@ static int libxl__device_nextid(libxl__gc *gc, uint32_t
domid, char *device)
return nextid;
}
+/* backend domain name-to-domid conversion utility */
+static int libxl__resolve_domain(libxl__gc *gc, const char *name)
+{
+ int i, rv;
+ uint32_t domid;
+ for (i=0; name[i]; i++) {
+ if (!isdigit(name[i])) {
+ rv = libxl_name_to_domid(libxl__gc_owner(gc), name, &domid);
+ if (rv)
+ return rv;
+ return domid;
+ }
+ }
+ return atoi(name);
+}
+
/******************************************************************************/
int libxl__device_vtpm_setdefault(libxl__gc *gc, libxl_device_vtpm *vtpm)
{
+ int rc;
if(libxl_uuid_is_nil(&vtpm->uuid)) {
libxl_uuid_generate(&vtpm->uuid);
}
+ if (vtpm->backend_domname) {
+ rc = libxl__resolve_domain(gc, vtpm->backend_domname);
+ if (rc < 0)
+ return rc;
+ vtpm->backend_domid = rc;
+ }
return 0;
}
@@ -1968,7 +1991,13 @@ int libxl__device_disk_setdefault(libxl__gc *gc,
libxl_device_disk *disk)
rc = libxl__device_disk_set_backend(gc, disk);
if (rc) return rc;
- return rc;
+ if (disk->backend_domname) {
+ rc = libxl__resolve_domain(gc, disk->backend_domname);
+ if (rc < 0)
+ return rc;
+ disk->backend_domid = rc;
+ }
+ return 0;
}
int libxl__device_from_disk(libxl__gc *gc, uint32_t domid,
@@ -2796,6 +2825,12 @@ int libxl__device_nic_setdefault(libxl__gc *gc,
libxl_device_nic *nic,
abort();
}
+ if (nic->backend_domname) {
+ int rc = libxl__resolve_domain(gc, nic->backend_domname);
+ if (rc < 0)
+ return rc;
+ nic->backend_domid = rc;
+ }
return 0;
}
@@ -3149,6 +3184,12 @@ out:
int libxl__device_vkb_setdefault(libxl__gc *gc, libxl_device_vkb *vkb)
{
+ if (vkb->backend_domname) {
+ int rc = libxl__resolve_domain(gc, vkb->backend_domname);
+ if (rc < 0)
+ return rc;
+ vkb->backend_domid = rc;
+ }
return 0;
}
@@ -3248,6 +3289,12 @@ int libxl__device_vfb_setdefault(libxl__gc *gc,
libxl_device_vfb *vfb)
libxl_defbool_setdefault(&vfb->sdl.enable, false);
libxl_defbool_setdefault(&vfb->sdl.opengl, false);
+ if (vfb->backend_domname) {
+ int rc = libxl__resolve_domain(gc, vfb->backend_domname);
+ if (rc < 0)
+ return rc;
+ vfb->backend_domid = rc;
+ }
return 0;
}
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 5b080ed..d480824 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -348,6 +348,7 @@ libxl_domain_build_info =
Struct("domain_build_info",[
libxl_device_vfb = Struct("device_vfb", [
("backend_domid", libxl_domid),
+ ("backend_domname",string),
("devid", libxl_devid),
("vnc", libxl_vnc_info),
("sdl", libxl_sdl_info),
@@ -357,11 +358,13 @@ libxl_device_vfb = Struct("device_vfb", [
libxl_device_vkb = Struct("device_vkb", [
("backend_domid", libxl_domid),
+ ("backend_domname", string),
("devid", libxl_devid),
])
libxl_device_disk = Struct("device_disk", [
("backend_domid", libxl_domid),
+ ("backend_domname", string),
("pdev_path", string),
("vdev", string),
("backend", libxl_disk_backend),
@@ -374,6 +377,7 @@ libxl_device_disk = Struct("device_disk", [
libxl_device_nic = Struct("device_nic", [
("backend_domid", libxl_domid),
+ ("backend_domname", string),
("devid", libxl_devid),
("mtu", integer),
("model", string),
@@ -401,6 +405,7 @@ libxl_device_pci = Struct("device_pci", [
libxl_device_vtpm = Struct("device_vtpm", [
("backend_domid", libxl_domid),
+ ("backend_domname", string),
("devid", libxl_devid),
("uuid", libxl_uuid),
])
diff --git a/tools/libxl/libxlu_disk_l.l b/tools/libxl/libxlu_disk_l.l
index bee16a1..7c4e7f1 100644
--- a/tools/libxl/libxlu_disk_l.l
+++ b/tools/libxl/libxlu_disk_l.l
@@ -168,6 +168,7 @@ devtype=disk,? { DPC->disk->is_cdrom = 0; }
devtype=[^,]*,? { xlu__disk_err(DPC,yytext,"unknown value for type");
}
access=[^,]*,? { STRIP('',''); setaccess(DPC, FROMEQUALS); }
+backend=[^,]*,? { STRIP('',''); SAVESTRING("backend",
backend_domname, FROMEQUALS); }
backendtype=[^,]*,? { STRIP('','');
setbackendtype(DPC,FROMEQUALS); }
vdev=[^,]*,? { STRIP('',''); SAVESTRING("vdev", vdev,
FROMEQUALS); }
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index a98705e..2625e01 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1095,12 +1095,7 @@ static void parse_config_data(const char *config_source,
break;
*p2 = ''\0'';
if (!strcmp(p, "backend")) {
- if(domain_qualifier_to_domid(p2 + 1,
&(vtpm->backend_domid), 0))
- {
- fprintf(stderr,
- "Specified vtpm backend domain `%s''
does not exist!\n", p2 + 1);
- exit(1);
- }
+ vtpm->backend_domname = strdup(p2 + 1);
got_backend = true;
} else if(!strcmp(p, "uuid")) {
if( libxl_uuid_from_string(&vtpm->uuid, p2 + 1) ) {
@@ -1195,11 +1190,8 @@ static void parse_config_data(const char *config_source,
free(nic->ifname);
nic->ifname = strdup(p2 + 1);
} else if (!strcmp(p, "backend")) {
- if(libxl_name_to_domid(ctx, (p2 + 1),
&(nic->backend_domid))) {
- fprintf(stderr, "Specified backend domain does not
exist, defaulting to Dom0\n");
- nic->backend_domid = 0;
- }
- if (nic->backend_domid != 0 &&
run_hotplug_scripts) {
+ nic->backend_domname = strdup(p2 + 1);
+ if (run_hotplug_scripts) {
fprintf(stderr, "ERROR: the vif
''backend='' option "
"cannot be used in conjunction with "
"run_hotplug_scripts, please set "
@@ -2527,8 +2519,6 @@ static void cd_insert(uint32_t domid, const char *virtdev,
char *phys)
parse_disk_config(&config, buf, &disk);
- disk.backend_domid = 0;
-
libxl_cdrom_insert(ctx, domid, &disk, NULL);
libxl_device_disk_dispose(&disk);
@@ -5499,11 +5489,7 @@ int main_networkattach(int argc, char **argv)
} else if (MATCH_OPTION("script", *argv, oparg)) {
replace_string(&nic.script, oparg);
} else if (MATCH_OPTION("backend", *argv, oparg)) {
- if(libxl_name_to_domid(ctx, oparg, &val)) {
- fprintf(stderr, "Specified backend domain does not exist,
defaulting to Dom0\n");
- val = 0;
- }
- nic.backend_domid = val;
+ replace_string(&nic.backend_domname, oparg);
} else if (MATCH_OPTION("vifname", *argv, oparg)) {
replace_string(&nic.ifname, oparg);
} else if (MATCH_OPTION("model", *argv, oparg)) {
@@ -5608,8 +5594,8 @@ int main_networkdetach(int argc, char **argv)
int main_blockattach(int argc, char **argv)
{
int opt;
- uint32_t fe_domid, be_domid = 0;
- libxl_device_disk disk = { 0 };
+ uint32_t fe_domid;
+ libxl_device_disk disk;
XLU_Config *config = 0;
SWITCH_FOREACH_OPT(opt, "", NULL, "block-attach", 2) {
@@ -5625,8 +5611,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;
-
if (dryrun_only) {
char *json = libxl_device_disk_to_json(ctx, &disk);
printf("disk: %s\n", json);
@@ -5708,7 +5692,6 @@ int main_vtpmattach(int argc, char **argv)
int opt;
libxl_device_vtpm vtpm;
char *oparg;
- unsigned int val;
uint32_t domid;
SWITCH_FOREACH_OPT(opt, "", NULL, "vtpm-attach", 1) {
@@ -5729,12 +5712,7 @@ int main_vtpmattach(int argc, char **argv)
return 1;
}
} else if (MATCH_OPTION("backend", *argv, oparg)) {
- if(domain_qualifier_to_domid(oparg, &val, 0)) {
- fprintf(stderr,
- "Specified backend domain does not exist, defaulting
to Dom0\n");
- val = 0;
- }
- vtpm.backend_domid = val;
+ replace_string(&vtpm.backend_domname, oparg);
} else {
fprintf(stderr, "unrecognized argument `%s''\n",
*argv);
return 1;
--
1.8.1.4
Daniel De Graaf
2013-Mar-14 14:23 UTC
[PATCH 2/2] libxl: zero the vtpm structures on allocation
This avoids returning unallocated memory in the libxl_device_vtpm
structure since libxl_device_vtpm_init is not called here.
Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
---
tools/libxl/libxl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index e442afc..2b64700 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1853,7 +1853,7 @@ libxl_device_vtpm *libxl_device_vtpm_list(libxl_ctx *ctx,
uint32_t domid, int *n
fe_path = libxl__sprintf(gc, "%s/device/vtpm",
libxl__xs_get_dompath(gc, domid));
dir = libxl__xs_directory(gc, XBT_NULL, fe_path, &ndirs);
if(dir) {
- vtpms = malloc(sizeof(*vtpms) * ndirs);
+ vtpms = calloc(sizeof(*vtpms), ndirs);
libxl_device_vtpm* vtpm;
libxl_device_vtpm* end = vtpms + ndirs;
for(vtpm = vtpms; vtpm < end; ++vtpm, ++dir) {
--
1.8.1.4
Ian Campbell
2013-Apr-12 14:05 UTC
Re: [PATCH RESEND v3 1/2] libxl: postpone backend name resolution
On Thu, 2013-03-14 at 14:23 +0000, Daniel De Graaf wrote:> This adds a backend_domname field in libxl devices that contain a > backend_domid field, allowing either a domid or a domain name to be > specified in the configuration structures. The domain name is resolved > into a domain ID in the _setdefault function when adding the device. > This change allows the backend of the block devices to be specified > (which previously required passing the libxl_ctx down into the block > device parser), and will simplify specification of backend domains in > other users of libxl.please can we get a #define LIBXL_HAVE_DEVICE_BACKEND_NAME (or something similar) in libxl.h so applications can tell that it is safe to use this.> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov> > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > Cc: Ian Campbell <ian.campbell@citrix.com> > > --- > > 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. > > docs/misc/xl-disk-configuration.txt | 12 +++++++++ > tools/libxl/libxl.c | 49 ++++++++++++++++++++++++++++++++++++- > tools/libxl/libxl_types.idl | 5 ++++ > tools/libxl/libxlu_disk_l.l | 1 + > tools/libxl/xl_cmdimpl.c | 36 ++++++--------------------- > 5 files changed, 73 insertions(+), 30 deletions(-) > > diff --git a/docs/misc/xl-disk-configuration.txt b/docs/misc/xl-disk-configuration.txt > index 86c16be..5bd456d 100644 > --- a/docs/misc/xl-disk-configuration.txt > +++ b/docs/misc/xl-disk-configuration.txt > @@ -139,6 +139,18 @@ cdrom > Convenience alias for "devtype=cdrom". > > > +backend=<domain-name> > +--------------------- > + > +Description: Designates a backend domain for the device > +Supported values: Valid domain names > +Mandatory: No > + > +Specifies the backend domain which this device should attach to. This > +defaults to domain 0. Specifying another domain requires setting up a > +driver domain which is outside the scope of this document. > + > + > backendtype=<backend-type> > -------------------------- > > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c > index 73e0dc3..e442afc 100644 > --- a/tools/libxl/libxl.c > +++ b/tools/libxl/libxl.c > @@ -1730,12 +1730,35 @@ static int libxl__device_nextid(libxl__gc *gc, uint32_t domid, char *device) > return nextid; > } > > +/* backend domain name-to-domid conversion utility */ > +static int libxl__resolve_domain(libxl__gc *gc, const char *name) > +{ > + int i, rv; > + uint32_t domid; > + for (i=0; name[i]; i++) { > + if (!isdigit(name[i])) { > + rv = libxl_name_to_domid(libxl__gc_owner(gc), name, &domid); > + if (rv) > + return rv; > + return domid; > + } > + }What are the constraints on the names of domains? I suppose it is toolstack specific but the xl docs jsut say it has to be unique. I''d suggest a simple rule like they cannot start with a digit, that would make the loop in the above unnecessary. In any case the loop is a bit odd since AFAICT it will treat "123boo" as "boo", I think it should either treat the entire string as a name or a number. I think you should just move domain_qualifier_to_domid from xl to libxl and use that...> + return atoi(name); > +} > + > /******************************************************************************/ > int libxl__device_vtpm_setdefault(libxl__gc *gc, libxl_device_vtpm *vtpm) > { > + int rc; > if(libxl_uuid_is_nil(&vtpm->uuid)) { > libxl_uuid_generate(&vtpm->uuid); > } > + if (vtpm->backend_domname) { > + rc = libxl__resolve_domain(gc, vtpm->backend_domname); > + if (rc < 0) > + return rc; > + vtpm->backend_domid = rc; > + }How about rc = libxl__resole_domain(gc, vtpm->backend_domname, &vtpm->backend_domid); if (rc < 0) return rc; ? Then the if (...) which is repeated could be in the function and it''d be easier to keep the logic the same for all of them. Ian.
Ian Campbell
2013-Apr-12 14:10 UTC
Re: [PATCH 2/2] libxl: zero the vtpm structures on allocation
On Thu, 2013-03-14 at 14:23 +0000, Daniel De Graaf wrote:> This avoids returning unallocated memory in the libxl_device_vtpm > structure since libxl_device_vtpm_init is not called here.Does the code not either loop over ndirs and initialise every one or alternatively fail and cleanup without returning the uninitialised memory? If this is an issue it expect it is an issue for libxl_device_*_list? Probably they should all be calling their respective init functions, perhaps via a new libxl_..._init_array helper?> > Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov> > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > Cc: Ian Campbell <ian.campbell@citrix.com> > --- > tools/libxl/libxl.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c > index e442afc..2b64700 100644 > --- a/tools/libxl/libxl.c > +++ b/tools/libxl/libxl.c > @@ -1853,7 +1853,7 @@ libxl_device_vtpm *libxl_device_vtpm_list(libxl_ctx *ctx, uint32_t domid, int *n > fe_path = libxl__sprintf(gc, "%s/device/vtpm", libxl__xs_get_dompath(gc, domid)); > dir = libxl__xs_directory(gc, XBT_NULL, fe_path, &ndirs); > if(dir) { > - vtpms = malloc(sizeof(*vtpms) * ndirs); > + vtpms = calloc(sizeof(*vtpms), ndirs); > libxl_device_vtpm* vtpm; > libxl_device_vtpm* end = vtpms + ndirs; > for(vtpm = vtpms; vtpm < end; ++vtpm, ++dir) {
Daniel De Graaf
2013-Apr-12 14:23 UTC
Re: [PATCH 2/2] libxl: zero the vtpm structures on allocation
On 04/12/2013 10:10 AM, Ian Campbell wrote:> On Thu, 2013-03-14 at 14:23 +0000, Daniel De Graaf wrote: >> This avoids returning unallocated memory in the libxl_device_vtpm >> structure since libxl_device_vtpm_init is not called here. > > Does the code not either loop over ndirs and initialise every one or > alternatively fail and cleanup without returning the uninitialised > memory?The code used to initialize every member and so did not call the _init function for vtpms; adding a new member to the structure causes this to miss the new member. An alternative to this patch is to add a call to libxl_device_vtpm_init inside the loop, similar the disk device type. The nic device just does memset(nic, 0, sizeof(*nic)), like this patch.> If this is an issue it expect it is an issue for libxl_device_*_list? > Probably they should all be calling their respective init functions, > perhaps via a new libxl_..._init_array helper?At least disk and nic handle this on a per-element basis, so there''s no immediate advantage to an array helper.>> >> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov> >> Cc: Ian Jackson <ian.jackson@eu.citrix.com> >> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com> >> Cc: Ian Campbell <ian.campbell@citrix.com> >> --- >> tools/libxl/libxl.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c >> index e442afc..2b64700 100644 >> --- a/tools/libxl/libxl.c >> +++ b/tools/libxl/libxl.c >> @@ -1853,7 +1853,7 @@ libxl_device_vtpm *libxl_device_vtpm_list(libxl_ctx *ctx, uint32_t domid, int *n >> fe_path = libxl__sprintf(gc, "%s/device/vtpm", libxl__xs_get_dompath(gc, domid)); >> dir = libxl__xs_directory(gc, XBT_NULL, fe_path, &ndirs); >> if(dir) { >> - vtpms = malloc(sizeof(*vtpms) * ndirs); >> + vtpms = calloc(sizeof(*vtpms), ndirs); >> libxl_device_vtpm* vtpm; >> libxl_device_vtpm* end = vtpms + ndirs; >> for(vtpm = vtpms; vtpm < end; ++vtpm, ++dir) { > >-- Daniel De Graaf National Security Agency
Ian Campbell
2013-Apr-12 14:26 UTC
Re: [PATCH 2/2] libxl: zero the vtpm structures on allocation
On Fri, 2013-04-12 at 15:23 +0100, Daniel De Graaf wrote:> On 04/12/2013 10:10 AM, Ian Campbell wrote: > > On Thu, 2013-03-14 at 14:23 +0000, Daniel De Graaf wrote: > >> This avoids returning unallocated memory in the libxl_device_vtpm > >> structure since libxl_device_vtpm_init is not called here. > > > > Does the code not either loop over ndirs and initialise every one or > > alternatively fail and cleanup without returning the uninitialised > > memory? > > The code used to initialize every member and so did not call the _init > function for vtpms; adding a new member to the structure causes this > to miss the new member. An alternative to this patch is to add a call > to libxl_device_vtpm_init inside the loop, similar the disk device type.I think this is preferable since if a new member is added which doesn''t default to 0 then the memset is also wrong.> The nic device just does memset(nic, 0, sizeof(*nic)), like this patch.I think that''s wrong.> > If this is an issue it expect it is an issue for libxl_device_*_list? > > Probably they should all be calling their respective init functions, > > perhaps via a new libxl_..._init_array helper? > > At least disk and nic handle this on a per-element basis, so there''s no > immediate advantage to an array helper.OK then. Ian.
Daniel De Graaf
2013-Apr-12 14:41 UTC
Re: [PATCH RESEND v3 1/2] libxl: postpone backend name resolution
On 04/12/2013 10:05 AM, Ian Campbell wrote:> On Thu, 2013-03-14 at 14:23 +0000, Daniel De Graaf wrote: >> This adds a backend_domname field in libxl devices that contain a >> backend_domid field, allowing either a domid or a domain name to be >> specified in the configuration structures. The domain name is resolved >> into a domain ID in the _setdefault function when adding the device. >> This change allows the backend of the block devices to be specified >> (which previously required passing the libxl_ctx down into the block >> device parser), and will simplify specification of backend domains in >> other users of libxl. > > please can we get a #define LIBXL_HAVE_DEVICE_BACKEND_NAME (or something > similar) in libxl.h so applications can tell that it is safe to use > this.OK; I will use LIBXL_HAVE_DEVICE_BACKEND_DOMNAME to match the new field.> >> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov> >> Cc: Ian Jackson <ian.jackson@eu.citrix.com> >> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com> >> Cc: Ian Campbell <ian.campbell@citrix.com> >> >> --- >> >> 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. >> >> docs/misc/xl-disk-configuration.txt | 12 +++++++++ >> tools/libxl/libxl.c | 49 ++++++++++++++++++++++++++++++++++++- >> tools/libxl/libxl_types.idl | 5 ++++ >> tools/libxl/libxlu_disk_l.l | 1 + >> tools/libxl/xl_cmdimpl.c | 36 ++++++--------------------- >> 5 files changed, 73 insertions(+), 30 deletions(-) >> >> diff --git a/docs/misc/xl-disk-configuration.txt b/docs/misc/xl-disk-configuration.txt >> index 86c16be..5bd456d 100644 >> --- a/docs/misc/xl-disk-configuration.txt >> +++ b/docs/misc/xl-disk-configuration.txt >> @@ -139,6 +139,18 @@ cdrom >> Convenience alias for "devtype=cdrom". >> >> >> +backend=<domain-name> >> +--------------------- >> + >> +Description: Designates a backend domain for the device >> +Supported values: Valid domain names >> +Mandatory: No >> + >> +Specifies the backend domain which this device should attach to. This >> +defaults to domain 0. Specifying another domain requires setting up a >> +driver domain which is outside the scope of this document. >> + >> + >> backendtype=<backend-type> >> -------------------------- >> >> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c >> index 73e0dc3..e442afc 100644 >> --- a/tools/libxl/libxl.c >> +++ b/tools/libxl/libxl.c >> @@ -1730,12 +1730,35 @@ static int libxl__device_nextid(libxl__gc *gc, uint32_t domid, char *device) >> return nextid; >> } >> >> +/* backend domain name-to-domid conversion utility */ >> +static int libxl__resolve_domain(libxl__gc *gc, const char *name) >> +{ >> + int i, rv; >> + uint32_t domid; >> + for (i=0; name[i]; i++) { >> + if (!isdigit(name[i])) { >> + rv = libxl_name_to_domid(libxl__gc_owner(gc), name, &domid); >> + if (rv) >> + return rv; >> + return domid; >> + } >> + } > > What are the constraints on the names of domains? I suppose it is > toolstack specific but the xl docs jsut say it has to be unique. > > I''d suggest a simple rule like they cannot start with a digit, that > would make the loop in the above unnecessary. In any case the loop is a > bit odd since AFAICT it will treat "123boo" as "boo", I think it should > either treat the entire string as a name or a number.Actually, this will treat "123boo" properly, as name (not name+i) is passed to libxl_name_to_domid.> I think you should just move domain_qualifier_to_domid from xl to libxl > and use that...That would also work, although it would need a better name: libxl_qualifier_to_domid since libxl_name_to_domid is already used?>> + return atoi(name); >> +} >> + >> /******************************************************************************/ >> int libxl__device_vtpm_setdefault(libxl__gc *gc, libxl_device_vtpm *vtpm) >> { >> + int rc; >> if(libxl_uuid_is_nil(&vtpm->uuid)) { >> libxl_uuid_generate(&vtpm->uuid); >> } >> + if (vtpm->backend_domname) { >> + rc = libxl__resolve_domain(gc, vtpm->backend_domname); >> + if (rc < 0) >> + return rc; >> + vtpm->backend_domid = rc; >> + } > > How about > rc = libxl__resole_domain(gc, vtpm->backend_domname, &vtpm->backend_domid); > if (rc < 0) > return rc; > ? > > Then the if (...) which is repeated could be in the function and it''d be > easier to keep the logic the same for all of them. > > Ian. >This would conflict a bit with making libxl__resolve_domain public, although I guess the function could note that it does nothing if passed a NULL string which is all that is needed here. -- Daniel De Graaf National Security Agency
Ian Campbell
2013-Apr-12 14:49 UTC
Re: [PATCH RESEND v3 1/2] libxl: postpone backend name resolution
On Fri, 2013-04-12 at 15:41 +0100, Daniel De Graaf wrote:> On 04/12/2013 10:05 AM, Ian Campbell wrote: > > On Thu, 2013-03-14 at 14:23 +0000, Daniel De Graaf wrote: > >> This adds a backend_domname field in libxl devices that contain a > >> backend_domid field, allowing either a domid or a domain name to be > >> specified in the configuration structures. The domain name is resolved > >> into a domain ID in the _setdefault function when adding the device. > >> This change allows the backend of the block devices to be specified > >> (which previously required passing the libxl_ctx down into the block > >> device parser), and will simplify specification of backend domains in > >> other users of libxl. > > > > please can we get a #define LIBXL_HAVE_DEVICE_BACKEND_NAME (or something > > similar) in libxl.h so applications can tell that it is safe to use > > this. > > OK; I will use LIBXL_HAVE_DEVICE_BACKEND_DOMNAME to match the new field.Sounds good.> >> + int i, rv; > >> + uint32_t domid; > >> + for (i=0; name[i]; i++) { > >> + if (!isdigit(name[i])) { > >> + rv = libxl_name_to_domid(libxl__gc_owner(gc), name, &domid); > >> + if (rv) > >> + return rv; > >> + return domid; > >> + } > >> + } > > > > What are the constraints on the names of domains? I suppose it is > > toolstack specific but the xl docs jsut say it has to be unique. > > > > I''d suggest a simple rule like they cannot start with a digit, that > > would make the loop in the above unnecessary. In any case the loop is a > > bit odd since AFAICT it will treat "123boo" as "boo", I think it should > > either treat the entire string as a name or a number. > > Actually, this will treat "123boo" properly, as name (not name+i) is passed > to libxl_name_to_domid.So it is, cunning ;-)> > > I think you should just move domain_qualifier_to_domid from xl to libxl > > and use that... > > That would also work, although it would need a better name: > libxl_qualifier_to_domid since libxl_name_to_domid is already used?Maybe libxl_domain_qualifier_to... in case someone thinks of a qualifier for something else?> >> + return atoi(name); > >> +} > >> + > >> /******************************************************************************/ > >> int libxl__device_vtpm_setdefault(libxl__gc *gc, libxl_device_vtpm *vtpm) > >> { > >> + int rc; > >> if(libxl_uuid_is_nil(&vtpm->uuid)) { > >> libxl_uuid_generate(&vtpm->uuid); > >> } > >> + if (vtpm->backend_domname) { > >> + rc = libxl__resolve_domain(gc, vtpm->backend_domname); > >> + if (rc < 0) > >> + return rc; > >> + vtpm->backend_domid = rc; > >> + } > > > > How about > > rc = libxl__resole_domain(gc, vtpm->backend_domname, &vtpm->backend_domid); > > if (rc < 0) > > return rc; > > ? > > > > Then the if (...) which is repeated could be in the function and it''d be > > easier to keep the logic the same for all of them. > > > > Ian. > > > > This would conflict a bit with making libxl__resolve_domain public, although I > guess the function could note that it does nothing if passed a NULL string > which is all that is needed here.I guess that''s sensible behaviour in any case? I suppose the other alternative would be to set domid => 0 which isn''t want you want here but could be what other potential callers want (e.g. the current xl users of domain_qualifier_to_domid I guess). I''m happy to leave this up to you depending on whether this suggested change makes the xl callers worse or not and/or your preference. Ian.
Ian Jackson
2013-Apr-12 15:50 UTC
Re: [PATCH RESEND v3 1/2] libxl: postpone backend name resolution
Ian Campbell writes ("Re: [PATCH RESEND v3 1/2] libxl: postpone backend
name resolution"):> I think you should just move domain_qualifier_to_domid from xl to libxl
> and use that...
I agree.
Ian.