thr3ads.net - Xen devel - [Xen-devel] [PATCH,RFC] More consistent error handling in libxl [Jul 2010]

If this information is useful, please help other people find it:
Share via:

Gianni Tedesco

2010-Jul-19 15:44 UTC

[Xen-devel] [PATCH,RFC] More consistent error handling in libxl

Lots of places in libxl return -1 as an error which is inconsistent with
libxl error codes since that is ERROR_VERSION. Also in other places the
xc_* function to implement a command is called but the return value is
either never checked or not passed on.

This patch introduces a new internal function libxl_xc_error() which
maps xc error codes to xl error codes. Callers of libxc functions are
converted to use this when returning error codes back to libxl callers.

Also a bug is fixed where a caller depends on errno being set but is
cleared by cleanup code which calls in to library functions which modify
errno as a side-effect.

Patch is RFC since not all call-paths modified have been tested and I
may have missed some. Such a sweeping change has a possibility to break
something.

Signed-off-by: Gianni Tedesco <gianni.tedesco@citrix.com>

 libxl.c          |  106 +++++++++++++++++++++++++++++++++----------------------
 libxl_dom.c      |   21 ++++++----
 libxl_internal.h |    3 +
 3 files changed, 80 insertions(+), 50 deletions(-)

diff -r 238587759d20 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Mon Jul 19 15:47:58 2010 +0100
+++ b/tools/libxl/libxl.c	Mon Jul 19 16:37:12 2010 +0100
@@ -38,6 +38,28 @@
 
 #define PAGE_TO_MEMKB(pages) ((pages) * 4)
 
+int libxl_xc_error(int xc_err)
+{
+    int ret = ERROR_FAIL;
+
+    switch(xc_err) {
+    case XC_ERROR_NONE:
+        return 0;
+    case XC_INVALID_PARAM:
+        ret = ERROR_INVAL;
+        break;
+    case XC_OUT_OF_MEMORY:
+        ret = ERROR_NOMEM;
+        break;
+    case XC_INTERNAL_ERROR:
+    case XC_INVALID_KERNEL:
+    default:
+        break;
+    }
+
+    return ret;
+}
+
 int libxl_ctx_init(struct libxl_ctx *ctx, int version, xentoollog_logger *lg)
 {
     if (version != LIBXL_VERSION)
@@ -519,21 +541,23 @@
 
 int libxl_domain_pause(struct libxl_ctx *ctx, uint32_t domid)
 {
-    xc_domain_pause(ctx->xch, domid);
-    return 0;
+    int rc;
+    rc = xc_domain_pause(ctx->xch, domid);
+    return libxl_xc_error(rc);
 }
 
 int libxl_domain_core_dump(struct libxl_ctx *ctx, uint32_t domid, const char
*filename)
 {
-    if ( xc_domain_dumpcore(ctx->xch, domid, filename) )
-        return ERROR_FAIL;
-    return 0;
+    int rc;
+    rc = xc_domain_dumpcore(ctx->xch, domid, filename);
+    return libxl_xc_error(rc);
 }
 
 int libxl_domain_unpause(struct libxl_ctx *ctx, uint32_t domid)
 {
     char *path;
     char *state;
+    int rc;
 
     if (is_hvm(ctx, domid)) {
         path = libxl_sprintf(ctx,
"/local/domain/0/device-model/%d/state", domid);
@@ -543,9 +567,8 @@
             libxl_wait_for_device_model(ctx, domid, "running", NULL,
NULL);
         }
     }
-    xc_domain_unpause(ctx->xch, domid);
-
-    return 0;
+    rc = xc_domain_unpause(ctx->xch, domid);
+    return libxl_xc_error(rc);
 }
 
 static char *req_table[] = {
@@ -560,6 +583,7 @@
 {
     char *shutdown_path;
     char *dom_path;
+    int rc = 0;
 
     if (req > ARRAY_SIZE(req_table))
         return ERROR_INVAL;
@@ -577,9 +601,9 @@
         xc_get_hvm_param(ctx->xch, domid, HVM_PARAM_ACPI_S_STATE,
&acpi_s_state);
         xc_get_hvm_param(ctx->xch, domid, HVM_PARAM_CALLBACK_IRQ,
&pvdriver);
         if (!pvdriver || acpi_s_state != 0)
-            xc_domain_shutdown(ctx->xch, domid, req);
+            rc = xc_domain_shutdown(ctx->xch, domid, req);
     }
-    return 0;
+    return libxl_xc_error(rc);
 }
 
 int libxl_get_wait_fd(struct libxl_ctx *ctx, int *fd)
@@ -624,7 +648,7 @@
     char **events = xs_read_watch(ctx->xsh, &num);
     if (num != 2) {
         free(events);
-        return -1;
+        return ERROR_FAIL;
     }
     event->path = strdup(events[XS_WATCH_PATH]);
     event->token = strdup(events[XS_WATCH_TOKEN]);
@@ -636,7 +660,7 @@
 int libxl_stop_waiting(struct libxl_ctx *ctx, libxl_waiter *waiter)
 {
     if (!xs_unwatch(ctx->xsh, waiter->path, waiter->token))
-        return -1;
+        return ERROR_FAIL;
     else
         return 0;
 }
@@ -716,7 +740,7 @@
         int stubdomid = libxl_get_stubdom_id(ctx, domid);
         if (!stubdomid) {
             XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "Couldn''t find device
model''s pid");
-            return -1;
+            return ERROR_INVAL;
         }
         XL_LOG(ctx, XL_LOG_ERROR, "Device model is a stubdom,
domid=%d\n", stubdomid);
         return libxl_domain_destroy(ctx, stubdomid, 0);
@@ -754,7 +778,7 @@
 
     dom_path = libxl_xs_get_dompath(ctx, domid);
     if (!dom_path)
-        return -1;
+        return ERROR_FAIL;
 
     if (libxl_device_pci_shutdown(ctx, domid) < 0)
         XL_LOG(ctx, XL_LOG_ERROR, "pci shutdown failed for domid %d",
domid);
@@ -766,7 +790,7 @@
     rc = xc_domain_pause(ctx->xch, domid);
     if (rc < 0) {
         XL_LOG_ERRNOVAL(ctx, XL_LOG_ERROR, rc, "xc_domain_pause failed for
%d", domid);
-        return -1;
+        return libxl_xc_error(rc);
     }
     if (dm_present) {
         if (libxl_destroy_device_model(ctx, domid) < 0)
@@ -797,7 +821,7 @@
     rc = xc_domain_destroy(ctx->xch, domid);
     if (rc < 0) {
         XL_LOG_ERRNOVAL(ctx, XL_LOG_ERROR, rc, "xc_domain_destroy failed
for %d", domid);
-        return -1;
+        return libxl_xc_error(rc);
     }
     return 0;
 }
@@ -1143,11 +1167,11 @@
     }
     if (libxl_create_xenpv_qemu(ctx, vfb, num_console, console,
&dm_starting) < 0) {
         free(args);
-        return -1;
+        return ERROR_FAIL;
     }
     if (libxl_confirm_device_model_startup(ctx, dm_starting) < 0) {
         free(args);
-        return -1;
+        return ERROR_FAIL;
     }
 
     libxl_domain_unpause(ctx, domid);
@@ -1351,7 +1375,7 @@
                 if (!dev)
                     dev = make_blktap2_device(ctx, disk->physpath, type);
                 if (!dev)
-                    return -1;
+                    return ERROR_FAIL;
                 flexarray_set(back, boffset++, "tapdisk-params");
                 flexarray_set(back, boffset++, libxl_sprintf(ctx,
"%s:%s", device_disk_string_of_phystype(disk->phystype),
disk->physpath));
                 flexarray_set(back, boffset++, "params");
@@ -2051,7 +2075,7 @@
     }
     if (i == num) {
         XL_LOG(ctx, XL_LOG_ERROR, "Virtual device not found");
-        return -1;
+        return ERROR_FAIL;
     }
     libxl_device_disk_del(ctx, disks + i, 1);
     libxl_device_disk_add(ctx, domid, disk);
@@ -2301,7 +2325,7 @@
 
     if (!is_hvm(ctx, domid)) {
         if (libxl_wait_for_backend(ctx, be_path, "4") < 0)
-            return -1;
+            return ERROR_FAIL;
     }
 
     back = flexarray_make(16, 1);
@@ -2350,13 +2374,13 @@
     num_devs_path = libxl_sprintf(ctx, "%s/num_devs", be_path);
     num_devs = libxl_xs_read(ctx, XBT_NULL, num_devs_path);
     if (!num_devs)
-        return -1;
+        return ERROR_INVAL;
     num = atoi(num_devs);
 
     if (!is_hvm(ctx, domid)) {
         if (libxl_wait_for_backend(ctx, be_path, "4") < 0) {
             XL_LOG(ctx, XL_LOG_DEBUG, "pci backend at %s is not
ready", be_path);
-            return -1;
+            return ERROR_FAIL;
         }
     }
 
@@ -2370,7 +2394,7 @@
     }
     if (i == num) {
         XL_LOG(ctx, XL_LOG_ERROR, "Couldn''t find the device on
xenstore");
-        return -1;
+        return ERROR_INVAL;
     }
 
 retry_transaction:
@@ -2384,7 +2408,7 @@
     if (!is_hvm(ctx, domid)) {
         if (libxl_wait_for_backend(ctx, be_path, "4") < 0) {
             XL_LOG(ctx, XL_LOG_DEBUG, "pci backend at %s is not
ready", be_path);
-            return -1;
+            return ERROR_FAIL;
         }
     }
 
@@ -2464,7 +2488,7 @@
     hvm = is_hvm(ctx, domid);
     if (hvm) {
         if (libxl_wait_for_device_model(ctx, domid, "running", NULL,
NULL) < 0) {
-            return -1;
+            return ERROR_FAIL;
         }
         path = libxl_sprintf(ctx,
"/local/domain/0/device-model/%d/state", domid);
         state = libxl_xs_read(ctx, XBT_NULL, path);
@@ -2494,7 +2518,7 @@
 
         if (f == NULL) {
             XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "Couldn''t open
%s", sysfs_path);
-            return -1;
+            return ERROR_FAIL;
         }
         for (i = 0; i < PROC_PCI_NUM_RESOURCES; i++) {
             if (fscanf(f, "0x%llx 0x%llx 0x%llx\n", &start,
&end, &flags) != 3)
@@ -2557,7 +2581,7 @@
     hvm = is_hvm(ctx, domid);
     if (hvm) {
         if (libxl_wait_for_device_model(ctx, domid, "running", NULL,
NULL) < 0) {
-            return -1;
+            return ERROR_FAIL;
         }
         path = libxl_sprintf(ctx,
"/local/domain/0/device-model/%d/state", domid);
         state = libxl_xs_read(ctx, XBT_NULL, path);
@@ -2568,7 +2592,7 @@
         xs_write(ctx->xsh, XBT_NULL, path, "pci-rem",
strlen("pci-rem"));
         if (libxl_wait_for_device_model(ctx, domid, "pci-removed",
NULL, NULL) < 0) {
             XL_LOG(ctx, XL_LOG_ERROR, "Device Model didn''t
respond in time");
-            return -1;
+            return ERROR_FAIL;
         }
         path = libxl_sprintf(ctx,
"/local/domain/0/device-model/%d/state", domid);
         xs_write(ctx->xsh, XBT_NULL, path, state, strlen(state));
@@ -2692,7 +2716,7 @@
     pcidevs = libxl_device_pci_list(ctx, domid, &num);
     for (i = 0; i < num; i++) {
         if (libxl_device_pci_remove(ctx, domid, pcidevs + i) < 0)
-            return -1;
+            return ERROR_FAIL;
     }
     free(pcidevs);
     return 0;
@@ -2978,14 +3002,14 @@
     if (scinfo->weight < 1 || scinfo->weight > 65535) {
         XL_LOG_ERRNOVAL(ctx, XL_LOG_ERROR, rc,
             "Cpu weight out of range, valid values are within range from 1
to 65535");
-        return -1;
+        return ERROR_INVAL;
     }
 
     if (scinfo->cap < 0 || scinfo->cap > (domaininfo.max_vcpu_id +
1) * 100) {
         XL_LOG_ERRNOVAL(ctx, XL_LOG_ERROR, rc,
             "Cpu cap out of range, valid range is from 0 to %d for
specified number of vcpus",
             ((domaininfo.max_vcpu_id + 1) * 100));
-        return -1;
+        return ERROR_INVAL;
     }
 
     sdom.weight = scinfo->weight;
@@ -2993,7 +3017,7 @@
 
     rc = xc_sched_credit_domain_set(ctx->xch, domid, &sdom);
     if (rc != 0)
-        return rc;
+        return libxl_xc_error(rc);
 
     return 0;
 }
@@ -3022,7 +3046,7 @@
     if (trigger_type == -1) {
         XL_LOG_ERRNOVAL(ctx, XL_LOG_ERROR, -1,
             "Invalid trigger, valid triggers are
<nmi|reset|init|power|sleep>");
-        return -1;
+        return ERROR_INVAL;
     }
 
     rc = xc_domain_send_trigger(ctx->xch, domid, trigger_type, vcpuid);
@@ -3030,7 +3054,7 @@
         XL_LOG_ERRNOVAL(ctx, XL_LOG_ERROR, rc,
             "Send trigger ''%s'' failed",
trigger_name);
 
-    return rc;
+    return libxl_xc_error(rc);
 }
 
 int libxl_send_sysrq(struct libxl_ctx *ctx, uint32_t domid, char sysrq)
@@ -3156,7 +3180,7 @@
     if (rc < 0) {
         XL_LOG_ERRNOVAL(ctx, XL_LOG_ERROR, rc,
             "Can not freeze tmem pools");
-        return -1;
+        return ERROR_FAIL;
     }
 
     return rc;
@@ -3171,7 +3195,7 @@
     if (rc < 0) {
         XL_LOG_ERRNOVAL(ctx, XL_LOG_ERROR, rc,
             "Can not destroy tmem pools");
-        return -1;
+        return ERROR_FAIL;
     }
 
     return rc;
@@ -3186,7 +3210,7 @@
     if (rc < 0) {
         XL_LOG_ERRNOVAL(ctx, XL_LOG_ERROR, rc,
             "Can not thaw tmem pools");
-        return -1;
+        return ERROR_FAIL;
     }
 
     return rc;
@@ -3212,13 +3236,13 @@
     if (subop == -1) {
         XL_LOG_ERRNOVAL(ctx, XL_LOG_ERROR, -1,
             "Invalid set, valid sets are
<weight|cap|compress>");
-        return -1;
+        return ERROR_INVAL;
     }
     rc = xc_tmem_control(ctx->xch, -1, subop, domid, set, 0, 0, NULL);
     if (rc < 0) {
         XL_LOG_ERRNOVAL(ctx, XL_LOG_ERROR, rc,
             "Can not set tmem %s", name);
-        return -1;
+        return libxl_xc_error(rc);
     }
 
     return rc;
@@ -3233,7 +3257,7 @@
     if (rc < 0) {
         XL_LOG_ERRNOVAL(ctx, XL_LOG_ERROR, rc,
             "Can not set tmem shared auth");
-        return -1;
+        return libxl_xc_error(rc);
     }
 
     return rc;
diff -r 238587759d20 tools/libxl/libxl_dom.c
--- a/tools/libxl/libxl_dom.c	Mon Jul 19 15:47:58 2010 +0100
+++ b/tools/libxl/libxl_dom.c	Mon Jul 19 16:37:12 2010 +0100
@@ -212,7 +212,7 @@
     ret = 0;
 out:
     xc_dom_release(dom);
-    return ret == 0 ? 0 : ERROR_FAIL;
+    return libxl_xc_error(ret);
 }
 
 int build_hvm(struct libxl_ctx *ctx, uint32_t domid,
@@ -250,10 +250,12 @@
                    int fd)
 {
     /* read signature */
-    return xc_domain_restore(ctx->xch, fd, domid,
+    int rc;
+    rc = xc_domain_restore(ctx->xch, fd, domid,
                              state->store_port, &state->store_mfn,
                              state->console_port,
&state->console_mfn,
                              info->hvm, info->u.hvm.pae, 0);
+    return libxl_xc_error(rc);
 }
 
 struct suspendinfo {
@@ -359,7 +361,7 @@
 
     si.xce = xc_evtchn_open();
     if (si.xce < 0)
-        return -1;
+        return ERROR_FAIL;
 
     if (si.xce > 0) {
         port = xs_suspend_evtchn_port(si.domid);
@@ -438,7 +440,7 @@
     if (rc) {
         XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "unable to find domain info"
                      " for domain %"PRIu32, domid);
-        return 0;
+        return NULL;
     }
     uuid_string = string_of_uuid(ctx, info.uuid);
 
@@ -493,13 +495,13 @@
     size_t rs;
 
     filename = userdata_path(ctx, domid, userdata_userid, "d");
-    if (!filename) return ENOMEM;
+    if (!filename) return ERROR_NOMEM;
 
     if (!datalen)
         return userdata_delete(ctx, filename);
 
     newfilename = userdata_path(ctx, domid, userdata_userid, "n");
-    if (!newfilename) return ENOMEM;
+    if (!newfilename) return ERROR_NOMEM;
 
     fd= open(newfilename, O_RDWR|O_CREAT|O_TRUNC, 0600);
     if (fd<0) goto xe;
@@ -523,9 +525,10 @@
     if (f) fclose(f);
     if (fd>=0) close(fd);
 
+    errno = e;
     XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "cannot write %s for %s",
                  newfilename, filename);
-    return e;
+    return ERROR_FAIL;
 }
 
 int libxl_userdata_retrieve(struct libxl_ctx *ctx, uint32_t domid,
@@ -537,14 +540,14 @@
     void *data = 0;
 
     filename = userdata_path(ctx, domid, userdata_userid, "d");
-    if (!filename) return ENOMEM;
+    if (!filename) return ERROR_NOMEM;
 
     e = libxl_read_file_contents(ctx, filename, data_r ? &data : 0,
&datalen);
 
     if (!e && !datalen) {
         XL_LOG(ctx, XL_LOG_ERROR, "userdata file %s is empty",
filename);
         if (data_r) assert(!*data_r);
-        return EPROTO;
+        return ERROR_FAIL;
     }
 
     if (data_r) *data_r = data;
diff -r 238587759d20 tools/libxl/libxl_internal.h
--- a/tools/libxl/libxl_internal.h	Mon Jul 19 15:47:58 2010 +0100
+++ b/tools/libxl/libxl_internal.h	Mon Jul 19 16:37:12 2010 +0100
@@ -222,5 +222,8 @@
 #define XL_LOG_WARNING XTL_WARN
 #define XL_LOG_ERROR   XTL_ERROR
 
+/* Error handling */
+int libxl_xc_error(int xc_err);
+
 #endif
 



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Ian Jackson

2010-Jul-21 15:48 UTC

head link

Re: [Xen-devel] [PATCH,RFC] More consistent error handling in libxl

Gianni Tedesco writes ("[Xen-devel] [PATCH,RFC] More consistent error
handling in libxl"):> +    int rc;
> +    rc = xc_domain_pause(ctx->xch, domid);
> +    return libxl_xc_error(rc);
I''m afraid this pattern is wrong for two reasons.  Firstly,
xc_domain_pause and functions like it do not return XC_ERROR_* values.
They typically return -1 on error, and set errno.

Secondly, the error codes from libxc are not really all that useful.
They mostly serve to identify where the error originated.

See the comment in xenctrl.h near line 70.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Ian Jackson

2010-Jul-21 15:49 UTC

head link

Re: [Xen-devel] [PATCH,RFC] More consistent error handling in libxl

I wrote:> Gianni Tedesco writes ("[Xen-devel] [PATCH,RFC] More consistent error
handling in libxl"):
> > +    int rc;
> > +    rc = xc_domain_pause(ctx->xch, domid);
> > +    return libxl_xc_error(rc);
> 
> I''m afraid this pattern is wrong for two reasons.  Firstly,
> xc_domain_pause and functions like it do not return XC_ERROR_* values.
> They typically return -1 on error, and set errno.
> 
> Secondly, the error codes from libxc are not really all that useful.
> They mostly serve to identify where the error originated.
> 
> See the comment in xenctrl.h near line 70.
I see Stefano has applied this already.  I''m going to revert it.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Gianni Tedesco

2010-Jul-21 16:05 UTC

head link

Re: [Xen-devel] [PATCH,RFC] More consistent error handling in libxl

On Wed, 2010-07-21 at 16:49 +0100, Ian Jackson wrote:> I wrote:
> > Gianni Tedesco writes ("[Xen-devel] [PATCH,RFC] More consistent
error handling in libxl"):
> > > +    int rc;
> > > +    rc = xc_domain_pause(ctx->xch, domid);
> > > +    return libxl_xc_error(rc);
> > 
> > I''m afraid this pattern is wrong for two reasons.  Firstly,
> > xc_domain_pause and functions like it do not return XC_ERROR_* values.
> > They typically return -1 on error, and set errno.
> > 
> > Secondly, the error codes from libxc are not really all that useful.
> > They mostly serve to identify where the error originated.
Well in this case the default ERROR_FAIL is set. The thing here would
either be to introduce ERROR_ERRNO in libxl for the case errno is
expected to hold something valid or indeed to use errno instead of an xl
specific error handling system.
> > See the comment in xenctrl.h near line 70.
> 
> I see Stefano has applied this already.  I''m going to revert it.
Well something needs to be done, in many of the patched cases, any
errors were silently ignored.

What would you suggest?

Gianni


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Stefano Stabellini

2010-Jul-21 16:13 UTC

head link

Re: [Xen-devel] [PATCH,RFC] More consistent error handling in libxl

On Wed, 21 Jul 2010, Ian Jackson wrote:> Gianni Tedesco writes ("[Xen-devel] [PATCH,RFC] More consistent error
handling in libxl"):
> > +    int rc;
> > +    rc = xc_domain_pause(ctx->xch, domid);
> > +    return libxl_xc_error(rc);
> 
> I''m afraid this pattern is wrong for two reasons.  Firstly,
> xc_domain_pause and functions like it do not return XC_ERROR_* values.
> They typically return -1 on error, and set errno.
> 
> Secondly, the error codes from libxc are not really all that useful.
> They mostly serve to identify where the error originated.
> 
> See the comment in xenctrl.h near line 70.
>  
The patch is an improvement over what we have now, besides
libxl_xc_error is capable of handling -1, even though it just returns
ERROR_FAIL in that case. The other libxc return codes might be useful so
it would be nice to keep them. We could make libxl_xc_error smarter too.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Xen devel - Jul 2010 - [PATCH,RFC] More consistent error handling in libxl

[Xen-devel] [PATCH,RFC] More consistent error handling in libxl

Re: [Xen-devel] [PATCH,RFC] More consistent error handling in libxl

Re: [Xen-devel] [PATCH,RFC] More consistent error handling in libxl

Re: [Xen-devel] [PATCH,RFC] More consistent error handling in libxl

Re: [Xen-devel] [PATCH,RFC] More consistent error handling in libxl