[PATCH] xl: Return error when no userdata exists

[PATCH] xl: Return error when no userdata exists

[Jim Fehlig]

# HG changeset patch
# User Jim Fehlig <jfehlig@novell.com>
# Date 1296149593 25200
# Node ID aa57d7b164e246fb4cf8e971b890bfeb3a287fa9
# Parent  b59f04eb89786e5ae6cb99c5f5dcd8e3790bc3eb
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.

Fix error checking of libxl_read_file_contents() results.

    Signed-off-by: Jim Fehlig <jfehlig@novell.com>

diff -r b59f04eb8978 -r aa57d7b164e2 tools/libxl/libxl_dom.c
--- a/tools/libxl/libxl_dom.c	Fri Jan 21 18:06:23 2011 +0000
+++ b/tools/libxl/libxl_dom.c	Thu Jan 27 10:33:13 2011 -0700
@@ -672,7 +672,7 @@
 
     e = libxl_read_file_contents(ctx, filename, data_r ? &data : 0, &datalen);
 
-    if (!e && !datalen) {
+    if (e && !datalen) {
         LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "userdata file %s is empty", filename);
         if (data_r) assert(!*data_r);
         rc = ERROR_FAIL;

Re: [PATCH] xl: Return error when no userdata exists

[Ian Jackson]

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.


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.

I think the patch below fixes this.

Do you agree ?

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 <ian.jackson@eu.citrix.com>


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

Re: [PATCH] xl: Return error when no userdata exists

[Jim Fehlig]

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 <jfehlig@novell.com>

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 <ian.jackson@eu.citrix.com>
>
>
> 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
>   

Re: [PATCH] xl: Return error when no userdata exists

[Ian Jackson]

Jim Fehlig writes ("Re: [Xen-devel] [PATCH] xl: Return error when no userdata exists"):
> Ian Jackson wrote:
> > I think the patch below fixes this.
> 
> Yes.
> 
> Acked-by: Jim Fehlig <jfehlig@novell.com>

Thanks, applied.

Do let us know what else you discover that needs fixing.  I won't be
surprised if the API needs changes, in which case we'll put those in
4.2.

Ian.