From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel De Graaf Subject: Re: [PATCH v4 1/2] libxl: postpone backend name resolution Date: Mon, 15 Apr 2013 10:29:09 -0400 Message-ID: <516C0EB5.2080300@tycho.nsa.gov> References: <1365780146-20785-1-git-send-email-dgdegra@tycho.nsa.gov> <1366027867.4963.120.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: <1366027867.4963.120.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: George Dunlap , Stefano Stabellini , Ian Jackson , Roger Pau Monne , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org On 04/15/2013 08:11 AM, Ian Campbell wrote: > On Fri, 2013-04-12 at 16:22 +0100, 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. > > Looks good to me (some minor comments below). Given that the initial > version of this was posted ages ago I think this should be granted a > freeze exception. George? If it helps, this was discussed prior to 4.2's release as a feature that would be considered for backporting to 4.2.x. I don't think it's suitable for that due to the fact it changes the IDL - but it is functionality whose absence blocks the use of disk driver domains. >> >> 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. >> >> Changes since v3: >> - Add #define LIBXL_HAVE_DEVICE_BACKEND_DOMNAME in libxl.h >> - Create libxl_domain_qualifier_to_domid function in libxl_util.h >> >> docs/misc/xl-disk-configuration.txt | 12 ++++++ >> tools/libxl/libxl.c | 17 +++++++-- >> tools/libxl/libxl.h | 12 ++++++ >> tools/libxl/libxl_types.idl | 5 +++ >> tools/libxl/libxl_utils.c | 19 ++++++++++ >> tools/libxl/libxl_utils.h | 1 + >> tools/libxl/libxlu_disk_l.l | 1 + >> tools/libxl/xl_cmdimpl.c | 75 ++++++++----------------------------- >> 8 files changed, 78 insertions(+), 64 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 572c2c6..b994fea 100644 >> --- a/tools/libxl/libxl.c >> +++ b/tools/libxl/libxl.c >> @@ -1730,13 +1730,21 @@ static int libxl__device_nextid(libxl__gc *gc, uint32_t domid, char *device) >> return nextid; >> } >> >> +static int libxl__resolve_domid(libxl__gc *gc, const char *name, >> + uint32_t *domid) >> +{ >> + if (!name) >> + return 0; >> + return libxl_domain_qualifier_to_domid(libxl__gc_owner(gc), name, domid); > > You can use CTX for libxl__gc_owner(gc). > >> +} >> + >> /******************************************************************************/ >> int libxl__device_vtpm_setdefault(libxl__gc *gc, libxl_device_vtpm *vtpm) >> { >> if(libxl_uuid_is_nil(&vtpm->uuid)) { >> libxl_uuid_generate(&vtpm->uuid); >> } >> - return 0; >> + return libxl__resolve_domid(gc, vtpm->backend_domname, &vtpm->backend_domid); > > I'm not terribly keen on this pattern, it makes it seem (at least to me) > like this final call is somehow special and has to be a tail call, > rather than just happening to be the last item. That might be just me > though. I also think it looks better assigning to rc and returning. > Anyway, both of those a just picking nits. However this... > >> @@ -1200,11 +1172,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 " > > ... changes when this error (with the exit() which follows outside the > context presented here) happens. Before it was only if the specified > domain was non-zero but now it is if the backend is given at all. > > That might be OK, if you want to use dom0 as the backend, don't use the > option. But maybe we should just nuke the warning if something else will > also check later on once the domid is known in libxl so that we don't > get strange behaviour? I think the check in > libxl__device_nic_setdefault is sufficient? Right, we do check later which should suffice - even though it will take a bit longer to get to the error, it's a configuration problem that won't start happening by surprise so that shouldn't be an issue. Removing this check will also make changes to LIBXL_TOOLSTACK_DOMID do the right thing. > Are there any docs which need updating for this subtle change? I suppose > it ought to at least say "don't specify backend=0 or backend=Domain-0"? > > Ian. > Removing the earlier check will resolve this. -- Daniel De Graaf National Security Agency