From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?ISO-8859-1?Q?Roger_Pau_Monn=E9?= Subject: Re: [PATCH v4 1/2] libxl: postpone backend name resolution Date: Mon, 15 Apr 2013 16:55:08 +0200 Message-ID: <516C14CC.3000107@citrix.com> References: <1365780146-20785-1-git-send-email-dgdegra@tycho.nsa.gov> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1365780146-20785-1-git-send-email-dgdegra@tycho.nsa.gov> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Daniel De Graaf Cc: Stefano Stabellini , Ian Jackson , Ian Campbell , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org 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 > 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; 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?