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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2013-04-15 at 15:55 +0100, Roger Pau Monne wrote:
> 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@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;
> 
> 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.

Did you mean !name && !domid? i.e. fail if no name is given AND domid
isn't already set? But domid == 0 is a valid domid...

The nice property which the existing function has is you can initialise
domid as you wish (including to 0) and then blindly call this on the
given name (including NULL) and it will correctly override the domid if
a name was given.

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

I think saw words somewhere in the patch which documented explicitly
that domname takes precedence. Which isn't quite what you asked, but is
sufficient IMHO.

Ian.


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