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

["Roger Pau Monné" <roger.pau@citrix.com>]

On 12/04/13 17:22, 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.
> 
> 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.
> 
> 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;

This is just my opinion, but I find this function ambiguous, it will
return 0 (success), if either name is NULL, or name != NULL and it can
be resolved to a domid, which means that even when returning 0 the value
in *domid might be uninitialized (if name == NULL). I would rather do
something like

if (!name || !domid)
	return ERROR_INVAL;

And make sure callers only call this function if there's a real need to
resolve a domid.

Also, do we check somewhere that both backend_domid and backend_domname
are not specified at the same time?