Gianni Tedesco
2010-Jul-22 17:13 UTC
[Xen-devel] [PATCH] Consistent handling of libxc errors in libxl (take 2)
tools/libxl/libxl.c | 202 ++++++++++++++++++++++++++++++++---------------
tools/libxl/libxl_dom.c | 8 +-
2 files changed, 142 insertions(+), 68 deletions(-)
Removed mapping from libxc error codes to libxl - this is redundant because
libxl ought to return -1 with errno set to a meaningful value. Now simply log
the contents of errno before returning ERROR_FAIL if an xc call fails. Every
caller in libxl should now be covered this way except for pci device removals
and domain destroys where release of resources ought to continue even if an
error occurs in an early step. In this case the error is still logged at least
Signed-off-by: Gianni Tedesco <gianni.tedesco@citrix.com>
diff -r b0b1a6163203 -r 8eaf5d79718f tools/libxl/libxl.c
--- a/tools/libxl/libxl.c Thu Jul 22 15:24:49 2010 +0100
+++ b/tools/libxl/libxl.c Thu Jul 22 18:11:52 2010 +0100
@@ -38,28 +38,6 @@
#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)
@@ -456,10 +434,16 @@ struct libxl_dominfo * libxl_list_domain
int size = 1024;
ptr = calloc(size, sizeof(struct libxl_dominfo));
- if (!ptr) return NULL;
+ if (!ptr) {
+ XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "allocating domain info");
+ return NULL;
+ }
ret = xc_domain_getinfolist(ctx->xch, 0, 1024, info);
- if (ret<0) return NULL;
+ if (ret<0) {
+ XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "geting domain info list");
+ return NULL;
+ }
for (i = 0; i < ret; i++) {
xcinfo2xlinfo(&info[i], &ptr[i]);
@@ -474,7 +458,10 @@ int libxl_domain_info(struct libxl_ctx *
int ret;
ret = xc_domain_getinfolist(ctx->xch, domid, 1, &xcinfo);
- if (ret<0) return ERROR_FAIL;
+ if (ret<0) {
+ XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "geting domain info list");
+ return ERROR_FAIL;
+ }
if (ret==0 || xcinfo.domain != domid) return ERROR_INVAL;
xcinfo2xlinfo(&xcinfo, info_r);
@@ -489,10 +476,16 @@ struct libxl_poolinfo * libxl_list_pool(
int size = 256;
ptr = calloc(size, sizeof(struct libxl_poolinfo));
- if (!ptr) return NULL;
+ if (!ptr) {
+ XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "allocating cpupool info");
+ return NULL;
+ }
ret = xc_cpupool_getinfo(ctx->xch, 0, 256, info);
- if (ret<0) return NULL;
+ if (ret<0) {
+ XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "getting cpupool info");
+ return NULL;
+ }
for (i = 0; i < ret; i++) {
ptr[i].poolid = info[i].cpupool_id;
@@ -514,6 +507,10 @@ struct libxl_vminfo * libxl_list_vm(stru
return NULL;
ret = xc_domain_getinfolist(ctx->xch, 1, 1024, info);
+ if (ret<0) {
+ XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "geting domain info list");
+ return NULL;
+ }
for (index = i = 0; i < ret; i++) {
if (libxl_is_stubdom(ctx, info[i].domain, NULL))
continue;
@@ -541,23 +538,33 @@ int libxl_domain_suspend(struct libxl_ct
int libxl_domain_pause(struct libxl_ctx *ctx, uint32_t domid)
{
- int rc;
- rc = xc_domain_pause(ctx->xch, domid);
- return libxl_xc_error(rc);
+ int ret;
+ ret = xc_domain_pause(ctx->xch, domid);
+ if (ret<0) {
+ XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "pausing domain %d", domid);
+ return ERROR_FAIL;
+ }
+ return 0;
}
-int libxl_domain_core_dump(struct libxl_ctx *ctx, uint32_t domid, const char
*filename)
+int libxl_domain_core_dump(struct libxl_ctx *ctx, uint32_t domid,
+ const char *filename)
{
- int rc;
- rc = xc_domain_dumpcore(ctx->xch, domid, filename);
- return libxl_xc_error(rc);
+ int ret;
+ ret = xc_domain_dumpcore(ctx->xch, domid, filename);
+ if (ret<0) {
+ XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "core dumping domain %d to
%s",
+ domid, filename);
+ return ERROR_FAIL;
+ }
+ return 0;
}
int libxl_domain_unpause(struct libxl_ctx *ctx, uint32_t domid)
{
char *path;
char *state;
- int rc;
+ int ret;
if (is_hvm(ctx, domid)) {
path = libxl_sprintf(ctx,
"/local/domain/0/device-model/%d/state", domid);
@@ -567,8 +574,12 @@ int libxl_domain_unpause(struct libxl_ct
libxl_wait_for_device_model(ctx, domid, "running", NULL,
NULL);
}
}
- rc = xc_domain_unpause(ctx->xch, domid);
- return libxl_xc_error(rc);
+ ret = xc_domain_unpause(ctx->xch, domid);
+ if (ret<0) {
+ XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "unpausing domain %d",
domid);
+ return ERROR_FAIL;
+ }
+ return 0;
}
static char *req_table[] = {
@@ -583,7 +594,6 @@ int libxl_domain_shutdown(struct libxl_c
{
char *shutdown_path;
char *dom_path;
- int rc = 0;
if (req > ARRAY_SIZE(req_table))
return ERROR_INVAL;
@@ -598,12 +608,26 @@ int libxl_domain_shutdown(struct libxl_c
if (is_hvm(ctx,domid)) {
unsigned long acpi_s_state = 0;
unsigned long pvdriver = 0;
- 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)
- rc = xc_domain_shutdown(ctx->xch, domid, req);
+ int ret;
+ ret = xc_get_hvm_param(ctx->xch, domid, HVM_PARAM_ACPI_S_STATE,
&acpi_s_state);
+ if (ret<0) {
+ XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "getting ACPI S-state");
+ return ERROR_FAIL;
+ }
+ ret = xc_get_hvm_param(ctx->xch, domid, HVM_PARAM_CALLBACK_IRQ,
&pvdriver);
+ if (ret<0) {
+ XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "getting HVM callback
IRQ");
+ return ERROR_FAIL;
+ }
+ if (!pvdriver || acpi_s_state != 0) {
+ ret = xc_domain_shutdown(ctx->xch, domid, req);
+ if (ret<0) {
+ XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "unpausing domain");
+ return ERROR_FAIL;
+ }
+ }
}
- return libxl_xc_error(rc);
+ return 0;
}
int libxl_get_wait_fd(struct libxl_ctx *ctx, int *fd)
@@ -790,7 +814,6 @@ int libxl_domain_destroy(struct libxl_ct
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 libxl_xc_error(rc);
}
if (dm_present) {
if (libxl_destroy_device_model(ctx, domid) < 0)
@@ -821,7 +844,7 @@ int libxl_domain_destroy(struct libxl_ct
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 libxl_xc_error(rc);
+ return ERROR_FAIL;
}
return 0;
}
@@ -1187,7 +1210,11 @@ static int libxl_create_stubdom(struct l
libxl_xs_write(ctx, XBT_NULL,
libxl_sprintf(ctx, "%s/target",
libxl_xs_get_dompath(ctx, domid)),
"%d", info->domid);
- xc_domain_set_target(ctx->xch, domid, info->domid);
+ ret = xc_domain_set_target(ctx->xch, domid, info->domid);
+ if (ret<0) {
+ XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "setting target domain %d ->
%d", domid, info->domid);
+ return ERROR_FAIL;
+ }
xs_set_target(ctx->xsh, domid, info->domid);
perm[0].id = domid;
@@ -2599,13 +2626,19 @@ int libxl_device_pci_add(struct libxl_ct
if (start) {
if (flags & PCI_BAR_IO) {
rc = xc_domain_ioport_permission(ctx->xch, domid, start,
size, 1);
- if (rc < 0)
+ if (rc < 0) {
XL_LOG_ERRNOVAL(ctx, XL_LOG_ERROR, rc, "Error:
xc_domain_ioport_permission error 0x%llx/0x%llx", start, size);
+ fclose(f);
+ return ERROR_FAIL;
+ }
} else {
rc = xc_domain_iomem_permission(ctx->xch, domid,
start>>XC_PAGE_SHIFT,
(size+(XC_PAGE_SIZE-1))>>XC_PAGE_SHIFT, 1);
- if (rc < 0)
+ if (rc < 0) {
XL_LOG_ERRNOVAL(ctx, XL_LOG_ERROR, rc, "Error:
xc_domain_iomem_permission error 0x%llx/0x%llx", start, size);
+ fclose(f);
+ return ERROR_FAIL;
+ }
}
}
}
@@ -2621,10 +2654,14 @@ int libxl_device_pci_add(struct libxl_ct
rc = xc_physdev_map_pirq(ctx->xch, domid, irq, &irq);
if (rc < 0) {
XL_LOG_ERRNOVAL(ctx, XL_LOG_ERROR, rc, "Error:
xc_physdev_map_pirq irq=%d", irq);
+ fclose(f);
+ return ERROR_FAIL;
}
rc = xc_domain_irq_permission(ctx->xch, domid, irq, 1);
if (rc < 0) {
XL_LOG_ERRNOVAL(ctx, XL_LOG_ERROR, rc, "Error:
xc_domain_irq_permission irq=%d", irq);
+ fclose(f);
+ return ERROR_FAIL;
}
}
fclose(f);
@@ -2632,8 +2669,10 @@ int libxl_device_pci_add(struct libxl_ct
out:
if (!libxl_is_stubdom(ctx, domid, NULL)) {
rc = xc_assign_device(ctx->xch, domid, pcidev->value);
- if (rc < 0)
+ if (rc < 0) {
XL_LOG_ERRNOVAL(ctx, XL_LOG_ERROR, rc, "xc_assign_device
failed");
+ return ERROR_FAIL;
+ }
}
libxl_device_pci_add_xenstore(ctx, domid, pcidev);
@@ -2899,7 +2938,8 @@ int libxl_get_physinfo(struct libxl_ctx
rc = xc_physinfo(ctx->xch, &xcphysinfo);
if (rc != 0) {
- return rc;
+ XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "getting physinfo");
+ return ERROR_FAIL;
}
physinfo->threads_per_core = xcphysinfo.threads_per_core;
physinfo->cores_per_socket = xcphysinfo.cores_per_socket;
@@ -2970,9 +3010,11 @@ struct libxl_vcpuinfo *libxl_list_vcpu(s
xc_physinfo_t physinfo = { 0 };
if (xc_domain_getinfolist(ctx->xch, domid, 1, &domaininfo) != 1) {
+ XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "getting infolist");
return NULL;
}
if (xc_physinfo(ctx->xch, &physinfo) == -1) {
+ XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "getting physinfo");
return NULL;
}
*cpusize = physinfo.max_cpu_id + 1;
@@ -2988,9 +3030,11 @@ struct libxl_vcpuinfo *libxl_list_vcpu(s
return NULL;
}
if (xc_vcpu_getinfo(ctx->xch, domid, *nb_vcpu, &vcpuinfo) == -1)
{
+ XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "getting vcpu info");
return NULL;
}
if (xc_vcpu_getaffinity(ctx->xch, domid, *nb_vcpu, ptr->cpumap,
*cpusize) == -1) {
+ XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "getting vcpu affinity");
return NULL;
}
ptr->vcpuid = *nb_vcpu;
@@ -3006,7 +3050,11 @@ struct libxl_vcpuinfo *libxl_list_vcpu(s
int libxl_set_vcpuaffinity(struct libxl_ctx *ctx, uint32_t domid, uint32_t
vcpuid,
uint64_t *cpumap, int cpusize)
{
- return (xc_vcpu_setaffinity(ctx->xch, domid, vcpuid, cpumap, cpusize));
+ if (xc_vcpu_setaffinity(ctx->xch, domid, vcpuid, cpumap, cpusize)) {
+ XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "setting vcpu affinity");
+ return ERROR_FAIL;
+ }
+ return 0;
}
int libxl_set_vcpucount(struct libxl_ctx *ctx, uint32_t domid, uint32_t count)
@@ -3016,6 +3064,7 @@ int libxl_set_vcpucount(struct libxl_ctx
int i;
if (xc_domain_getinfolist(ctx->xch, domid, 1, &domaininfo) != 1) {
+ XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "getting domain info list");
return ERROR_FAIL;
}
if (!count || ((domaininfo.max_vcpu_id + 1) < count)) {
@@ -3034,14 +3083,15 @@ int libxl_set_vcpucount(struct libxl_ctx
/*
* returns one of the XEN_SCHEDULER_* constants from public/domctl.h
- * or -1 if an error occured.
*/
int libxl_get_sched_id(struct libxl_ctx *ctx)
{
int sched, ret;
- if ((ret = xc_sched_id(ctx->xch, &sched)) != 0)
- return ret;
+ if ((ret = xc_sched_id(ctx->xch, &sched)) != 0) {
+ XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "getting domain info list");
+ return ERROR_FAIL;
+ }
return sched;
}
@@ -3051,8 +3101,10 @@ int libxl_sched_credit_domain_get(struct
int rc;
rc = xc_sched_credit_domain_get(ctx->xch, domid, &sdom);
- if (rc != 0)
- return rc;
+ if (rc != 0) {
+ XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "setting domain sched
credit");
+ return ERROR_FAIL;
+ }
scinfo->weight = sdom.weight;
scinfo->cap = sdom.cap;
@@ -3067,8 +3119,12 @@ int libxl_sched_credit_domain_set(struct
int rc;
rc = xc_domain_getinfolist(ctx->xch, domid, 1, &domaininfo);
+ if (rc < 0) {
+ XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "getting domain info list");
+ return ERROR_FAIL;
+ }
if (rc != 1 || domaininfo.domain != domid)
- return rc;
+ return ERROR_INVAL;
if (scinfo->weight < 1 || scinfo->weight > 65535) {
@@ -3088,8 +3144,10 @@ int libxl_sched_credit_domain_set(struct
sdom.cap = scinfo->cap;
rc = xc_sched_credit_domain_set(ctx->xch, domid, &sdom);
- if (rc != 0)
- return libxl_xc_error(rc);
+ if ( rc < 0 ) {
+ XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "setting domain sched
credit");
+ return ERROR_FAIL;
+ }
return 0;
}
@@ -3122,11 +3180,13 @@ int libxl_send_trigger(struct libxl_ctx
}
rc = xc_domain_send_trigger(ctx->xch, domid, trigger_type, vcpuid);
- if (rc != 0)
+ if (rc != 0) {
XL_LOG_ERRNOVAL(ctx, XL_LOG_ERROR, rc,
"Send trigger ''%s'' failed",
trigger_name);
-
- return libxl_xc_error(rc);
+ return ERROR_FAIL;
+ }
+
+ return 0;
}
int libxl_send_sysrq(struct libxl_ctx *ctx, uint32_t domid, char sysrq)
@@ -3140,7 +3200,13 @@ int libxl_send_sysrq(struct libxl_ctx *c
int libxl_send_debug_keys(struct libxl_ctx *ctx, char *keys)
{
- return xc_send_debug_keys(ctx->xch, keys);
+ int ret;
+ ret = xc_send_debug_keys(ctx->xch, keys);
+ if ( ret < 0 ) {
+ XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "sending debug keys");
+ return ERROR_FAIL;
+ }
+ return 0;
}
struct libxl_xen_console_reader *
@@ -3189,6 +3255,10 @@ int libxl_xen_console_read_line(struct l
memset(cr->buffer, 0, cr->size);
ret = xc_readconsolering(ctx->xch, &cr->buffer,
&cr->count,
cr->clear, cr->incremental,
&cr->index);
+ if (ret < 0) {
+ XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "reading console ring
buffer");
+ return ERROR_FAIL;
+ }
if (!ret) {
if (cr->count) {
*line_r = cr->buffer;
@@ -3314,7 +3384,7 @@ int libxl_tmem_set(struct libxl_ctx *ctx
if (rc < 0) {
XL_LOG_ERRNOVAL(ctx, XL_LOG_ERROR, rc,
"Can not set tmem %s", name);
- return libxl_xc_error(rc);
+ return ERROR_FAIL;
}
return rc;
@@ -3329,7 +3399,7 @@ int libxl_tmem_shared_auth(struct libxl_
if (rc < 0) {
XL_LOG_ERRNOVAL(ctx, XL_LOG_ERROR, rc,
"Can not set tmem shared auth");
- return libxl_xc_error(rc);
+ return ERROR_FAIL;
}
return rc;
@@ -3343,7 +3413,7 @@ int libxl_tmem_freeable(struct libxl_ctx
if (rc < 0) {
XL_LOG_ERRNOVAL(ctx, XL_LOG_ERROR, rc,
"Can not get tmem freeable memory");
- return -1;
+ return ERROR_FAIL;
}
return rc;
diff -r b0b1a6163203 -r 8eaf5d79718f tools/libxl/libxl_dom.c
--- a/tools/libxl/libxl_dom.c Thu Jul 22 15:24:49 2010 +0100
+++ b/tools/libxl/libxl_dom.c Thu Jul 22 18:11:52 2010 +0100
@@ -212,7 +212,7 @@ int build_pv(struct libxl_ctx *ctx, uint
ret = 0;
out:
xc_dom_release(dom);
- return libxl_xc_error(ret);
+ return ERROR_FAIL;
}
int build_hvm(struct libxl_ctx *ctx, uint32_t domid,
@@ -255,7 +255,11 @@ int restore_common(struct libxl_ctx *ctx
state->store_port, &state->store_mfn,
state->console_port,
&state->console_mfn,
info->hvm, info->u.hvm.pae, 0);
- return libxl_xc_error(rc);
+ if ( rc < 0 ) {
+ XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "restoring domain");
+ return ERROR_FAIL;
+ }
+ return 0;
}
struct suspendinfo {
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Ian Jackson
2010-Jul-23 16:26 UTC
Re: [Xen-devel] [PATCH] Consistent handling of libxc errors in libxl (take 2)
Gianni Tedesco writes ("[Xen-devel] [PATCH] Consistent handling of libxc
errors in libxl (take 2)"):> Removed mapping from libxc error codes to libxl - this is redundant because
> libxl ought to return -1 with errno set to a meaningful value. Now simply
log
> the contents of errno before returning ERROR_FAIL if an xc call fails.
Every
> caller in libxl should now be covered this way except for pci device
removals
> and domain destroys where release of resources ought to continue even if an
> error occurs in an early step. In this case the error is still logged at
least
Thanks for that patch. It all looks great apart from this:
> diff -r b0b1a6163203 -r 8eaf5d79718f tools/libxl/libxl_dom.c
> --- a/tools/libxl/libxl_dom.c Thu Jul 22 15:24:49 2010 +0100
> +++ b/tools/libxl/libxl_dom.c Thu Jul 22 18:11:52 2010 +0100
> @@ -212,7 +212,7 @@ int build_pv(struct libxl_ctx *ctx, uint
> ret = 0;
> out:
> xc_dom_release(dom);
> - return libxl_xc_error(ret);
> + return ERROR_FAIL;
> }
This isn''t correct because ret might be 0. I''ve fixed it up
and
applied it.
Thanks,
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel