[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH v4 1/2] libxl: postpone backend name resolution
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 <dgdegra@xxxxxxxxxxxxx>
Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
Cc: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
Cc: Ian Campbell <ian.campbell@xxxxxxxxxx>
---
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=<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 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
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
