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.

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

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 80969d60c63092444e7f35274a96d78808adddb9..b8ed890a39b5c6e123e55bcec8c599fbc71314da 100644 (file)
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -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_suspend(struct libxl_ctx *ctx, libxl_domain_suspend_info *info,
 
 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 @@ int libxl_domain_unpause(struct libxl_ctx *ctx, uint32_t domid)
             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 @@ int libxl_domain_shutdown(struct libxl_ctx *ctx, uint32_t domid, int req)
 {
     char *shutdown_path;
     char *dom_path;
+    int rc = 0;
 
     if (req > ARRAY_SIZE(req_table))
         return ERROR_INVAL;
@@ -577,9 +601,9 @@ int libxl_domain_shutdown(struct libxl_ctx *ctx, uint32_t domid, int req)
         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 @@ int libxl_get_event(struct libxl_ctx *ctx, libxl_event *event)
     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_get_event(struct libxl_ctx *ctx, libxl_event *event)
 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 @@ static int libxl_destroy_device_model(struct libxl_ctx *ctx, uint32_t domid)
         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 @@ int libxl_domain_destroy(struct libxl_ctx *ctx, uint32_t domid, int force)
 
     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 @@ int libxl_domain_destroy(struct libxl_ctx *ctx, uint32_t domid, int force)
     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 @@ int libxl_domain_destroy(struct libxl_ctx *ctx, uint32_t domid, int force)
     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;
 }
@@ -1152,11 +1176,11 @@ retry_transaction:
     }
     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);
@@ -1360,7 +1384,7 @@ int libxl_device_disk_add(struct libxl_ctx *ctx, uint32_t domid, libxl_device_di
                 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");
@@ -2060,7 +2084,7 @@ int libxl_cdrom_insert(struct libxl_ctx *ctx, uint32_t domid, libxl_device_disk
     }
     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);
@@ -2310,7 +2334,7 @@ static int libxl_device_pci_add_xenstore(struct libxl_ctx *ctx, uint32_t domid,
 
     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);
@@ -2359,13 +2383,13 @@ static int libxl_device_pci_remove_xenstore(struct libxl_ctx *ctx, uint32_t domi
     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;
         }
     }
 
@@ -2379,7 +2403,7 @@ static int libxl_device_pci_remove_xenstore(struct libxl_ctx *ctx, uint32_t domi
     }
     if (i == num) {
         XL_LOG(ctx, XL_LOG_ERROR, "Couldn't find the device on xenstore");
-        return -1;
+        return ERROR_INVAL;
     }
 
 retry_transaction:
@@ -2393,7 +2417,7 @@ retry_transaction:
     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;
         }
     }
 
@@ -2473,7 +2497,7 @@ int libxl_device_pci_add(struct libxl_ctx *ctx, uint32_t domid, libxl_device_pci
     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);
@@ -2503,7 +2527,7 @@ int libxl_device_pci_add(struct libxl_ctx *ctx, uint32_t domid, libxl_device_pci
 
         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)
@@ -2566,7 +2590,7 @@ int libxl_device_pci_remove(struct libxl_ctx *ctx, uint32_t domid, libxl_device_
     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);
@@ -2577,7 +2601,7 @@ int libxl_device_pci_remove(struct libxl_ctx *ctx, uint32_t domid, libxl_device_
         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));
@@ -2701,7 +2725,7 @@ int libxl_device_pci_shutdown(struct libxl_ctx *ctx, uint32_t domid)
     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;
@@ -2987,14 +3011,14 @@ int libxl_sched_credit_domain_set(struct libxl_ctx *ctx, uint32_t domid, struct
     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;
@@ -3002,7 +3026,7 @@ int libxl_sched_credit_domain_set(struct libxl_ctx *ctx, uint32_t domid, struct
 
     rc = xc_sched_credit_domain_set(ctx->xch, domid, &sdom);
     if (rc != 0)
-        return rc;
+        return libxl_xc_error(rc);
 
     return 0;
 }
@@ -3031,7 +3055,7 @@ int libxl_send_trigger(struct libxl_ctx *ctx, uint32_t domid, char *trigger_name
     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);
@@ -3039,7 +3063,7 @@ int libxl_send_trigger(struct libxl_ctx *ctx, uint32_t domid, char *trigger_name
         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)
@@ -3165,7 +3189,7 @@ int libxl_tmem_freeze(struct libxl_ctx *ctx, uint32_t domid)
     if (rc < 0) {
         XL_LOG_ERRNOVAL(ctx, XL_LOG_ERROR, rc,
             "Can not freeze tmem pools");
-        return -1;
+        return ERROR_FAIL;
     }
 
     return rc;
@@ -3180,7 +3204,7 @@ int libxl_tmem_destroy(struct libxl_ctx *ctx, uint32_t domid)
     if (rc < 0) {
         XL_LOG_ERRNOVAL(ctx, XL_LOG_ERROR, rc,
             "Can not destroy tmem pools");
-        return -1;
+        return ERROR_FAIL;
     }
 
     return rc;
@@ -3195,7 +3219,7 @@ int libxl_tmem_thaw(struct libxl_ctx *ctx, uint32_t domid)
     if (rc < 0) {
         XL_LOG_ERRNOVAL(ctx, XL_LOG_ERROR, rc,
             "Can not thaw tmem pools");
-        return -1;
+        return ERROR_FAIL;
     }
 
     return rc;
@@ -3221,13 +3245,13 @@ int libxl_tmem_set(struct libxl_ctx *ctx, uint32_t domid, char* name, uint32_t s
     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;
@@ -3242,7 +3266,7 @@ int libxl_tmem_shared_auth(struct libxl_ctx *ctx, uint32_t domid,
     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 --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 3b398cb5dbb2e2c53ba02702ce84e0d283c5ab31..3dcd2549559fc29fccd395c4bd512ce235b8b787 100644 (file)
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -212,7 +212,7 @@ int build_pv(struct libxl_ctx *ctx, uint32_t domid,
     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 restore_common(struct libxl_ctx *ctx, uint32_t domid,
                    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 @@ int core_suspend(struct libxl_ctx *ctx, uint32_t domid, int fd,
 
     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 @@ static const char *userdata_path(struct libxl_ctx *ctx, uint32_t domid,
     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 @@ int libxl_userdata_store(struct libxl_ctx *ctx, uint32_t domid,
     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 @@ int libxl_userdata_store(struct libxl_ctx *ctx, uint32_t domid,
     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 @@ int libxl_userdata_retrieve(struct libxl_ctx *ctx, uint32_t domid,
     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 --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 10d45c70e6df69916506755122364b2de6850d4a..129e22cd5090a62fd6c2cf6026e20c6d33044f0e 100644 (file)
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -222,5 +222,8 @@ char *libxl_abs_path(struct libxl_ctx *ctx, char *s, const char *path);
 #define XL_LOG_WARNING XTL_WARN
 #define XL_LOG_ERROR   XTL_ERROR
 
+/* Error handling */
+int libxl_xc_error(int xc_err);
+
 #endif