From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jim Fehlig Subject: Re: [PATCH] xl: Return error when no userdata exists Date: Thu, 27 Jan 2011 12:10:12 -0700 Message-ID: <4D41C314.4040203@novell.com> References: <19777.49190.822476.585064@mariner.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <19777.49190.822476.585064@mariner.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Ian Jackson Cc: xen-devel@lists.xensource.com List-Id: xen-devel@lists.xenproject.org Ian Jackson wrote: > Jim Fehlig writes ("[Xen-devel] [PATCH] xl: Return error when no userdata exists"): > >> xl: Return error when no userdata exists >> >> The libvirt libxenlight driver will store its own userdata with >> id 'libvirt-xml', but currently libxl_userdata_retrieve() does >> not fail on non-existent userdata due to inverted error check. >> > > Nonexistent userdata is the same as zero-length userdata, according to > the specification. From libxl.h: > > int libxl_userdata_store(libxl_ctx *ctx, uint32_t domid, > const char *userdata_userid, > const uint8_t *data, int datalen); > /* If datalen==0, data is not used and the user data for > * that domain and userdata_userid is deleted. */ > > int libxl_userdata_retrieve(libxl_ctx *ctx, uint32_t domid, > const char *userdata_userid, > uint8_t **data_r, int *datalen_r); > /* On successful return, *data_r is from malloc. > * If there is no data for that domain and userdata_userid, > * *data_r and *datalen_r will be set to 0. > * data_r and datalen_r may be 0. > * On error return, *data_r and *datalen_r are undefined. > */ > > So I think to detect absence of userdata you should be checking > *data_r or *datalen_r on successful return from > libxl_userdata_retrieve. > Okay. > You are however correct in that there is a bug in the error handling: > if libxl_read_file_contents fails (ie, e!=0) for a reason other than > ENOENT, libxl_userdata_retrieve improperly ignores it. > Right. > I think the patch below fixes this. > > Do you agree ? > Yes. Acked-by: Jim Fehlig Thanks, Jim > Thanks, > Ian. > > > libxl: correct error path in libxl_userdata_retrieve > > Firstly, if libxl_read_file_contents fails, it doesn't really leave > *data and *datalen_r undefined - it leaves them unchanged. Tighten up > the spec for the benefit of libxl_userdata_retrieve. > > Secondly, libxl_userdata_retrieve ignored errors, assuming they were > all ENOENT. Instead it should fail on unexpected errors. > > Signed-off-by: Ian Jackson > > > diff -r 6067a17114bc tools/libxl/libxl_dom.c > --- a/tools/libxl/libxl_dom.c Thu Jan 27 16:17:27 2011 +0000 > +++ b/tools/libxl/libxl_dom.c Thu Jan 27 18:55:41 2011 +0000 > @@ -671,7 +671,10 @@ int libxl_userdata_retrieve(libxl_ctx *c > } > > e = libxl_read_file_contents(ctx, filename, data_r ? &data : 0, &datalen); > - > + if (e && errno != ENOENT) { > + rc = ERROR_FAIL; > + goto out; > + } > if (!e && !datalen) { > LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "userdata file %s is empty", filename); > if (data_r) assert(!*data_r); > diff -r 6067a17114bc tools/libxl/libxl_utils.h > --- a/tools/libxl/libxl_utils.h Thu Jan 27 16:17:27 2011 +0000 > +++ b/tools/libxl/libxl_utils.h Thu Jan 27 18:55:41 2011 +0000 > @@ -36,7 +36,7 @@ int libxl_read_file_contents(libxl_ctx * > /* Reads the contents of the plain file filename into a mallocd > * buffer. Returns 0 or errno. Any errors other than ENOENT are logged. > * If the file is empty, *data_r and *datalen_r are set to 0. > - * On error, *data_r and *datalen_r are undefined. > + * On error, *data_r and *datalen_r are unchanged. > * data_r and/or datalen_r may be 0. > */ > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel >