From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel De Graaf Subject: Re: [PATCH RESEND v3 1/2] libxl: postpone backend name resolution Date: Fri, 12 Apr 2013 10:41:40 -0400 Message-ID: <51681D24.20909@tycho.nsa.gov> References: <1363270993-12872-1-git-send-email-dgdegra@tycho.nsa.gov> <1365775553.15783.59.camel@zakaz.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1365775553.15783.59.camel@zakaz.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: Stefano Stabellini , Ian Jackson , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org 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 >> Cc: Ian Jackson >> Cc: Stefano Stabellini >> Cc: Ian Campbell >> >> --- >> >> 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= >> +--------------------- >> + >> +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= >> -------------------------- >> >> 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