Re: [Xen-devel] [PATCH v4 1/2] libxl: postpone backend name resolution

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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?

> 
> 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.

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?

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.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel