This also includes a couple of non-Coverity-spotted issues which were found while working on said Coverity-spotted issues. Tested by make installing the entire tree, creating a few PV domains w/ xl, using vchan-node2 (libvchan) and xenctx.
Matthew Daley
2013-Oct-30 07:51 UTC
[PATCH 01/29] libxc: check for xc_domain_get_guest_width failure and pass it down
...in xc_cpuid_pv_policy.
Coverity-ID: 1093050
Signed-off-by: Matthew Daley <mattjd@gmail.com>
---
tools/libxc/xc_cpuid_x86.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
index bbbf9b8..865d02a 100644
--- a/tools/libxc/xc_cpuid_x86.c
+++ b/tools/libxc/xc_cpuid_x86.c
@@ -431,7 +431,7 @@ static void xc_cpuid_hvm_policy(
}
-static void xc_cpuid_pv_policy(
+static int xc_cpuid_pv_policy(
xc_interface *xch, domid_t domid,
const unsigned int *input, unsigned int *regs)
{
@@ -443,7 +443,8 @@ static void xc_cpuid_pv_policy(
xc_cpuid_brand_get(brand);
- xc_domain_get_guest_width(xch, domid, &guest_width);
+ if ( xc_domain_get_guest_width(xch, domid, &guest_width) != 0 )
+ return 1;
guest_64bit = (guest_width == 8);
/* Detecting Xen''s atitude towards XSAVE */
@@ -547,6 +548,8 @@ static void xc_cpuid_pv_policy(
regs[0] = regs[1] = regs[2] = regs[3] = 0;
break;
}
+
+ return 0;
}
static int xc_cpuid_policy(
@@ -561,7 +564,8 @@ static int xc_cpuid_policy(
if ( info.hvm )
xc_cpuid_hvm_policy(xch, domid, input, regs);
else
- xc_cpuid_pv_policy(xch, domid, input, regs);
+ if ( xc_cpuid_pv_policy(xch, domid, input, regs) != 0 )
+ return -EINVAL;
return 0;
}
@@ -632,7 +636,9 @@ int xc_cpuid_apply_policy(xc_interface *xch, domid_t domid)
for ( ; ; )
{
cpuid(input, regs);
- xc_cpuid_policy(xch, domid, input, regs);
+ rc = xc_cpuid_policy(xch, domid, input, regs);
+ if ( rc )
+ return rc;
if ( regs[0] || regs[1] || regs[2] || regs[3] )
{
--
1.7.10.4
Matthew Daley
2013-Oct-30 07:51 UTC
[PATCH 02/29] libxc: fix retrieval of and remove pointless check on gzip size
Coverity-ID: 1055587
Coverity-ID: 1055963
Signed-off-by: Matthew Daley <mattjd@gmail.com>
---
tools/libxc/xc_dom_core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/libxc/xc_dom_core.c b/tools/libxc/xc_dom_core.c
index 0f367f6..c9366a9 100644
--- a/tools/libxc/xc_dom_core.c
+++ b/tools/libxc/xc_dom_core.c
@@ -294,8 +294,8 @@ size_t xc_dom_check_gzip(xc_interface *xch, void *blob,
size_t ziplen)
return 0;
gzlen = blob + ziplen - 4;
- unziplen = gzlen[3] << 24 | gzlen[2] << 16 | gzlen[1] <<
8 | gzlen[0];
- if ( (unziplen < 0) || (unziplen > XC_DOM_DECOMPRESS_MAX) )
+ unziplen = (unsigned int)gzlen[3] << 24 | gzlen[2] << 16 |
gzlen[1] << 8 | gzlen[0];
+ if ( unziplen > XC_DOM_DECOMPRESS_MAX )
{
xc_dom_printf
(xch,
--
1.7.10.4
Matthew Daley
2013-Oct-30 07:51 UTC
[PATCH 03/29] libxc: fix memory leak in move_l3_below_4G error handling
...otherwise mmu gets leaked.
Coverity-ID: 1055844
Signed-off-by: Matthew Daley <mattjd@gmail.com>
---
tools/libxc/xc_dom_x86.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
index 7cc2ff2..60fc544 100644
--- a/tools/libxc/xc_dom_x86.c
+++ b/tools/libxc/xc_dom_x86.c
@@ -220,7 +220,7 @@ static xen_pfn_t move_l3_below_4G(struct xc_dom_image *dom,
{
DOMPRINTF("%s: xc_dom_pfn_to_ptr(dom, l3pfn, 1) => NULL",
__FUNCTION__);
- return l3mfn; /* our one call site will call xc_dom_panic and fail */
+ goto out; /* our one call site will call xc_dom_panic and fail */
}
memset(l3tab, 0, XC_DOM_PAGE_SIZE(dom));
--
1.7.10.4
Matthew Daley
2013-Oct-30 07:51 UTC
[PATCH 04/29] libxc: don''t ignore fd read failures when restoring a domain
Coverity-ID: 1055042
Coverity-ID: 1055043
Signed-off-by: Matthew Daley <mattjd@gmail.com>
---
tools/libxc/xc_domain_restore.c | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/tools/libxc/xc_domain_restore.c b/tools/libxc/xc_domain_restore.c
index ecaf25d..80769a7 100644
--- a/tools/libxc/xc_domain_restore.c
+++ b/tools/libxc/xc_domain_restore.c
@@ -327,7 +327,11 @@ static xen_pfn_t *load_p2m_frame_list(
else if ( !strncmp(chunk_sig, "xcnt", 4) )
{
*vcpuextstate = 1;
- RDEXACT(io_fd, vcpuextstate_size, sizeof(*vcpuextstate_size));
+ if ( RDEXACT(io_fd, vcpuextstate_size,
sizeof(*vcpuextstate_size)) )
+ {
+ PERROR("read extended vcpu state size failed");
+ return NULL;
+ }
tot_bytes -= chunk_bytes;
chunk_bytes = 0;
}
@@ -928,14 +932,22 @@ static int pagebuf_get_one(xc_interface *xch, struct
restore_ctx *ctx,
case XC_SAVE_ID_TOOLSTACK:
{
- RDEXACT(fd, &buf->tdata.len, sizeof(buf->tdata.len));
+ if ( RDEXACT(fd, &buf->tdata.len, sizeof(buf->tdata.len))
)
+ {
+ PERROR("error read toolstack id size");
+ return -1;
+ }
buf->tdata.data = (uint8_t*) realloc(buf->tdata.data,
buf->tdata.len);
if ( buf->tdata.data == NULL )
{
PERROR("error memory allocation");
return -1;
}
- RDEXACT(fd, buf->tdata.data, buf->tdata.len);
+ if ( RDEXACT(fd, buf->tdata.data, buf->tdata.len) )
+ {
+ PERROR("error read toolstack id");
+ return -1;
+ }
return pagebuf_get_one(xch, ctx, buf, fd, dom);
}
--
1.7.10.4
Matthew Daley
2013-Oct-30 07:51 UTC
[PATCH 05/29] libxc: tidy up loop construct in write_compressed
...otherwise the return 0 at the bottom is dead code.
Coverity-ID: 1055189
Signed-off-by: Matthew Daley <mattjd@gmail.com>
---
tools/libxc/xc_domain_save.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/tools/libxc/xc_domain_save.c b/tools/libxc/xc_domain_save.c
index fbc15e9..9f104a4 100644
--- a/tools/libxc/xc_domain_save.c
+++ b/tools/libxc/xc_domain_save.c
@@ -233,7 +233,7 @@ static int write_compressed(xc_interface *xch, comp_ctx
*compress_ctx,
int marker = XC_SAVE_ID_COMPRESSED_DATA;
unsigned long compbuf_len = 0;
- do
+ for(;;)
{
/* check for available space (atleast 8k) */
if ((ob->pos + header + XC_PAGE_SIZE * 2) > ob->size)
@@ -250,7 +250,7 @@ static int write_compressed(xc_interface *xch, comp_ctx
*compress_ctx,
ob->size - ob->pos - header,
&compbuf_len);
if (!rc)
- return 0;
+ break;
if (outbuf_hardwrite(xch, ob, fd, &marker, sizeof(marker)) < 0)
{
@@ -270,7 +270,7 @@ static int write_compressed(xc_interface *xch, comp_ctx
*compress_ctx,
ERROR("Error when writing compressed chunk");
return -1;
}
- } while (rc != 0);
+ }
return 0;
}
--
1.7.10.4
Matthew Daley
2013-Oct-30 07:51 UTC
[PATCH 06/29] libxc: don''t read uninitialized size value in xc_read_image
This error case can only be triggered by gzread returning 0 (and having
not read anything), so move it there.
Coverity-ID: 1056076
Signed-off-by: Matthew Daley <mattjd@gmail.com>
---
tools/libxc/xg_private.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/tools/libxc/xg_private.c b/tools/libxc/xg_private.c
index 8fa068e..a914068 100644
--- a/tools/libxc/xg_private.c
+++ b/tools/libxc/xg_private.c
@@ -71,6 +71,12 @@ char *xc_read_image(xc_interface *xch,
image = NULL;
goto out;
case 0: /* EOF */
+ if ( *size == 0 )
+ {
+ PERROR("Could not read kernel image");
+ free(image);
+ image = NULL;
+ }
goto out;
default:
*size += bytes;
@@ -80,13 +86,7 @@ char *xc_read_image(xc_interface *xch,
#undef CHUNK
out:
- if ( *size == 0 )
- {
- PERROR("Could not read kernel image");
- free(image);
- image = NULL;
- }
- else if ( image )
+ if ( image )
{
/* Shrink allocation to fit image. */
tmp = realloc(image, *size);
--
1.7.10.4
Matthew Daley
2013-Oct-30 07:51 UTC
[PATCH 07/29] libxc: remove pointless null pointer check in xc_interface_open_common
xch is guaranteed non-null here.
Coverity-ID: 1055944
Signed-off-by: Matthew Daley <mattjd@gmail.com>
---
tools/libxc/xc_private.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/libxc/xc_private.c b/tools/libxc/xc_private.c
index a260257..524862e 100644
--- a/tools/libxc/xc_private.c
+++ b/tools/libxc/xc_private.c
@@ -189,7 +189,7 @@ static struct xc_interface_core
*xc_interface_open_common(xentoollog_logger *log
err_put_iface:
xc_osdep_put(&xch->osdep);
err:
- if (xch) xtl_logger_destroy(xch->error_handler_tofree);
+ xtl_logger_destroy(xch->error_handler_tofree);
if (xch != &xch_buf) free(xch);
return NULL;
}
--
1.7.10.4
Matthew Daley
2013-Oct-30 07:51 UTC
[PATCH 08/29] libxc: check for xc_domain_get_guest_width failure in xc_domain_resume_any
Coverity-ID: 1090351
Signed-off-by: Matthew Daley <mattjd@gmail.com>
---
tools/libxc/xc_resume.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/tools/libxc/xc_resume.c b/tools/libxc/xc_resume.c
index cb61650..50724f2 100644
--- a/tools/libxc/xc_resume.c
+++ b/tools/libxc/xc_resume.c
@@ -134,7 +134,11 @@ static int xc_domain_resume_any(xc_interface *xch, uint32_t
domid)
return rc;
}
- xc_domain_get_guest_width(xch, domid, &dinfo->guest_width);
+ if ( xc_domain_get_guest_width(xch, domid, &dinfo->guest_width) != 0
)
+ {
+ PERROR("Could not get domain width");
+ return rc;
+ }
if ( dinfo->guest_width != sizeof(long) )
{
ERROR("Cannot resume uncooperative cross-address-size
guests");
--
1.7.10.4
Matthew Daley
2013-Oct-30 07:51 UTC
[PATCH 09/29] libxc: check for xc_vcpu_setcontext failure in xc_domain_resume_any
Coverity-ID: 1090352
Signed-off-by: Matthew Daley <mattjd@gmail.com>
---
tools/libxc/xc_resume.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/tools/libxc/xc_resume.c b/tools/libxc/xc_resume.c
index 50724f2..a4c0f53 100644
--- a/tools/libxc/xc_resume.c
+++ b/tools/libxc/xc_resume.c
@@ -211,7 +211,11 @@ static int xc_domain_resume_any(xc_interface *xch, uint32_t
domid)
/* Reset all secondary CPU states. */
for ( i = 1; i <= info.max_vcpu_id; i++ )
- xc_vcpu_setcontext(xch, domid, i, NULL);
+ if ( xc_vcpu_setcontext(xch, domid, i, NULL) != 0 )
+ {
+ ERROR("Couldn''t reset vcpu state");
+ goto out;
+ }
/* Ready to resume domain execution now. */
domctl.cmd = XEN_DOMCTL_resumedomain;
--
1.7.10.4
Matthew Daley
2013-Oct-30 07:51 UTC
[PATCH 10/29] libxc: initalize stdio loggers'' progress_last_percent values to 0
...otherwise they are undefined in the first progress callback.
Coverity-ID: 1056055
Signed-off-by: Matthew Daley <mattjd@gmail.com>
---
tools/libxc/xtl_logger_stdio.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/tools/libxc/xtl_logger_stdio.c b/tools/libxc/xtl_logger_stdio.c
index 2e73c86..aa5501f 100644
--- a/tools/libxc/xtl_logger_stdio.c
+++ b/tools/libxc/xtl_logger_stdio.c
@@ -172,6 +172,7 @@ xentoollog_logger_stdiostream *xtl_createlogger_stdiostream
if (newlogger.flags & XTL_STDIOSTREAM_SHOW_DATE) tzset();
newlogger.progress_erase_len = 0;
+ newlogger.progress_last_percent = 0;
return XTL_NEW_LOGGER(stdiostream, newlogger);
}
--
1.7.10.4
Matthew Daley
2013-Oct-30 07:51 UTC
[PATCH 11/29] libxc: remove null pointer checks in xc_pm_get_{c, p}xstat
We don''t check for null pointers in similar functions, and
it''s silly to
do so for these arguments here anyway; they should fail noisily if null
is passed.
While at it, remove pointless brackets.
Coverity-ID: 1055938
Coverity-ID: 1055939
Signed-off-by: Matthew Daley <mattjd@gmail.com>
---
tools/libxc/xc_pm.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/libxc/xc_pm.c b/tools/libxc/xc_pm.c
index ea1e251..0da2d89 100644
--- a/tools/libxc/xc_pm.c
+++ b/tools/libxc/xc_pm.c
@@ -51,7 +51,7 @@ int xc_pm_get_pxstat(xc_interface *xch, int cpuid, struct
xc_px_stat *pxpt)
int max_px, ret;
- if ( !pxpt || !(pxpt->trans_pt) || !(pxpt->pt) )
+ if ( !pxpt->trans_pt || !pxpt->pt )
return -EINVAL;
if ( (ret = xc_pm_get_max_px(xch, cpuid, &max_px)) != 0)
@@ -128,7 +128,7 @@ int xc_pm_get_cxstat(xc_interface *xch, int cpuid, struct
xc_cx_stat *cxpt)
DECLARE_NAMED_HYPERCALL_BOUNCE(residencies, cxpt->residencies, 0,
XC_HYPERCALL_BUFFER_BOUNCE_BOTH);
int max_cx, ret;
- if( !cxpt || !(cxpt->triggers) || !(cxpt->residencies) )
+ if( !cxpt->triggers || !cxpt->residencies )
return -EINVAL;
if ( (ret = xc_pm_get_max_cx(xch, cpuid, &max_cx)) )
--
1.7.10.4
Matthew Daley
2013-Oct-30 07:51 UTC
[PATCH 12/29] libxl: check for transaction abortion failure in libxl_set_memory_target
While at it, correct the error handling of libxl__fill_dom0_memory_info;
at that point there is no transaction to end in any manner.
Coverity-ID: 1055046
Signed-off-by: Matthew Daley <mattjd@gmail.com>
---
tools/libxl/libxl.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 29e66f2..0b29f32 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -3676,12 +3676,11 @@ retry_transaction:
target = libxl__xs_read(gc, t, libxl__sprintf(gc,
"%s/memory/target", dompath));
if (!target && !domid) {
- xs_transaction_end(ctx->xsh, t, 1);
+ if (!xs_transaction_end(ctx->xsh, t, 1))
+ goto out_no_transaction;
rc = libxl__fill_dom0_memory_info(gc, ¤t_target_memkb);
- if (rc < 0) {
- abort_transaction = 1;
- goto out;
- }
+ if (rc < 0)
+ goto out_no_transaction;
goto retry_transaction;
} else if (!target) {
LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
@@ -3786,6 +3785,7 @@ out:
if (errno == EAGAIN)
goto retry_transaction;
+out_no_transaction:
GC_FREE;
return rc;
}
--
1.7.10.4
Matthew Daley
2013-Oct-30 07:51 UTC
[PATCH 13/29] libxl: check for xc_domain_max_vcpus failure in libxl__build_pre
Coverity-ID: 1055047
Signed-off-by: Matthew Daley <mattjd@gmail.com>
---
tools/libxl/libxl_dom.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 1873459..5b9fd27 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -209,7 +209,10 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
char *xs_domid, *con_domid;
int rc;
- xc_domain_max_vcpus(ctx->xch, domid, info->max_vcpus);
+ if (xc_domain_max_vcpus(ctx->xch, domid, info->max_vcpus) != 0) {
+ LOG(ERROR, "Couldn''t get max vcpu count");
+ return ERROR_FAIL;
+ }
/*
* Check if the domain has any CPU affinity. If not, try to build
--
1.7.10.4
Matthew Daley
2013-Oct-30 07:51 UTC
[PATCH 14/29] libxl: don''t leak xs_read results in libxl__device_nic_from_xs_be
Coverity-ID: 1055866
Signed-off-by: Matthew Daley <mattjd@gmail.com>
---
tools/libxl/libxl.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 0b29f32..234f3c1 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2975,9 +2975,10 @@ static void libxl__device_nic_from_xs_be(libxl__gc *gc,
tmp = xs_read(ctx->xsh, XBT_NULL,
libxl__sprintf(gc, "%s/handle", be_path),
&len);
- if ( tmp )
+ if ( tmp ) {
nic->devid = atoi(tmp);
- else
+ free(tmp);
+ } else
nic->devid = 0;
/* nic->mtu = */
@@ -2987,6 +2988,7 @@ static void libxl__device_nic_from_xs_be(libxl__gc *gc,
rc = libxl__parse_mac(tmp, nic->mac);
if (rc)
memset(nic->mac, 0, sizeof(nic->mac));
+ free(tmp);
nic->ip = xs_read(ctx->xsh, XBT_NULL,
libxl__sprintf(gc, "%s/ip", be_path),
&len);
--
1.7.10.4
Matthew Daley
2013-Oct-30 07:51 UTC
[PATCH 15/29] libxl: correct file open success check in libxl__device_pci_reset
It could, even if only in theory, be fd 0.
Coverity-ID: 1055895
Signed-off-by: Matthew Daley <mattjd@gmail.com>
---
tools/libxl/libxl_pci.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index b9960bb..26ba3e6 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -967,7 +967,7 @@ static int libxl__device_pci_reset(libxl__gc *gc, unsigned
int domain, unsigned
reset = libxl__sprintf(gc, "%s/pciback/do_flr", SYSFS_PCI_DEV);
fd = open(reset, O_WRONLY);
- if (fd > 0) {
+ if (fd >= 0) {
char *buf = libxl__sprintf(gc, PCI_BDF, domain, bus, dev, func);
rc = write(fd, buf, strlen(buf));
if (rc < 0)
--
1.7.10.4
Matthew Daley
2013-Oct-30 07:51 UTC
[PATCH 16/29] libxl: check for xc_domain_shutdown failure in libxl__domain_suspend_common_callback
Coverity-ID: 1055048
Signed-off-by: Matthew Daley <mattjd@gmail.com>
---
tools/libxl/libxl_dom.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 5b9fd27..4606a12 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1041,7 +1041,11 @@ int libxl__domain_suspend_common_callback(void *user)
if (dss->hvm && (!hvm_pvdrv || hvm_s_state)) {
LOG(DEBUG, "Calling xc_domain_shutdown on HVM domain");
- xc_domain_shutdown(CTX->xch, domid, SHUTDOWN_suspend);
+ ret = xc_domain_shutdown(CTX->xch, domid, SHUTDOWN_suspend);
+ if (ret < 0) {
+ LOG(ERROR, "xc_domain_shutdown failed ret=%d", ret);
+ return 0;
+ }
/* The guest does not (need to) respond to this sort of request. */
dss->guest_responded = 1;
} else {
--
1.7.10.4
Matthew Daley
2013-Oct-30 07:51 UTC
[PATCH 17/29] libxl: don''t leak memory in libxl__poller_get failure case
Coverity-ID: 1055894
Signed-off-by: Matthew Daley <mattjd@gmail.com>
---
tools/libxl/libxl_event.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
index a5c52bc..824bdd2 100644
--- a/tools/libxl/libxl_event.c
+++ b/tools/libxl/libxl_event.c
@@ -1340,7 +1340,10 @@ libxl__poller *libxl__poller_get(libxl_ctx *ctx)
memset(p, 0, sizeof(*p));
rc = libxl__poller_init(ctx, p);
- if (rc) return NULL;
+ if (rc) {
+ free(p);
+ return NULL;
+ }
return p;
}
--
1.7.10.4
Matthew Daley
2013-Oct-30 07:51 UTC
[PATCH 18/29] libxl: only close fds which successfully opened in libxl__spawn_local_dm
Coverity-ID: 1055565
Signed-off-by: Matthew Daley <mattjd@gmail.com>
---
tools/libxl/libxl_dm.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index ef29d0b..24eebda 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1259,8 +1259,8 @@ retry_transaction:
rc = 0;
out_close:
- close(null);
- close(logfile_w);
+ if (null != -1) close(null);
+ if (logfile_w != -1) close(logfile_w);
out:
if (rc)
device_model_spawn_outcome(egc, dmss, rc);
--
1.7.10.4
Matthew Daley
2013-Oct-30 07:51 UTC
[PATCH 19/29] xl: libxl_list_vm returns a pointer, not a handle
Coverity-ID: 1054978
Signed-off-by: Matthew Daley <mattjd@gmail.com>
---
tools/libxl/xl_cmdimpl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index fddaa80..43d1519 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -3247,7 +3247,7 @@ static void list_vm(void)
info = libxl_list_vm(ctx, &nb_vm);
- if (info < 0) {
+ if (!info) {
fprintf(stderr, "libxl_domain_infolist failed.\n");
exit(1);
}
--
1.7.10.4
Matthew Daley
2013-Oct-30 07:51 UTC
[PATCH 20/29] xl: check for restore file open failure in create_domain
...otherwise you get a less helpful error message.
Coverity-ID: 1055569
Signed-off-by: Matthew Daley <mattjd@gmail.com>
---
tools/libxl/xl_cmdimpl.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 43d1519..a935a18 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1919,6 +1919,10 @@ static uint32_t create_domain(struct domain_create
*dom_info)
} else {
restore_source = restore_file;
restore_fd = open(restore_file, O_RDONLY);
+ if (restore_fd == -1) {
+ fprintf(stderr, "Can''t open restore file:
%s\n", strerror(errno));
+ return ERROR_INVAL;
+ }
rc = libxl_fd_set_cloexec(ctx, restore_fd, 1);
if (rc) return rc;
}
--
1.7.10.4
Matthew Daley
2013-Oct-30 07:51 UTC
[PATCH 21/29] xl: remove needless infinite-loop construct in create_domain
Use a simple if condition instead.
Coverity-ID: 1056150
Signed-off-by: Matthew Daley <mattjd@gmail.com>
---
tools/libxl/xl_cmdimpl.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index a935a18..d8f9aba 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -2144,9 +2144,8 @@ start:
child1 = xl_fork(child_waitdaemon);
if (child1) {
- for (;;) {
- got_child = xl_waitpid(child_waitdaemon, &status, 0);
- if (got_child == child1) break;
+ got_child = xl_waitpid(child_waitdaemon, &status, 0);
+ if (got_child != child1) {
assert(got_child == -1);
perror("failed to wait for daemonizing child");
ret = ERROR_FAIL;
--
1.7.10.4
Matthew Daley
2013-Oct-30 07:51 UTC
[PATCH 22/29] xl: correct references to long-removed function in error messages
Signed-off-by: Matthew Daley <mattjd@gmail.com>
---
tools/libxl/xl_cmdimpl.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index d8f9aba..40feb7d 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -3251,7 +3251,7 @@ static void list_vm(void)
info = libxl_list_vm(ctx, &nb_vm);
if (!info) {
- fprintf(stderr, "libxl_domain_infolist failed.\n");
+ fprintf(stderr, "libxl_list_vm failed.\n");
exit(1);
}
printf("UUID ID name\n");
@@ -4147,7 +4147,7 @@ int main_list(int argc, char **argv)
if (optind >= argc) {
info = libxl_list_domain(ctx, &nb_domain);
if (!info) {
- fprintf(stderr, "libxl_domain_infolist failed.\n");
+ fprintf(stderr, "libxl_list_domain failed.\n");
return 1;
}
info_free = info;
@@ -4856,7 +4856,7 @@ int main_sharing(int argc, char **argv)
if (optind >= argc) {
info = libxl_list_domain(ctx, &nb_domain);
if (!info) {
- fprintf(stderr, "libxl_domain_infolist failed.\n");
+ fprintf(stderr, "libxl_list_domain failed.\n");
return 1;
}
info_free = info;
@@ -5070,7 +5070,7 @@ static int sched_domain_output(libxl_scheduler sched, int
(*output)(int),
info = libxl_list_domain(ctx, &nb_domain);
if (!info) {
- fprintf(stderr, "libxl_domain_infolist failed.\n");
+ fprintf(stderr, "libxl_list_domain failed.\n");
return 1;
}
poolinfo = libxl_list_cpupool(ctx, &n_pools);
@@ -6025,7 +6025,7 @@ int main_claims(int argc, char **argv)
info = libxl_list_domain(ctx, &nb_domain);
if (!info) {
- fprintf(stderr, "libxl_domain_infolist failed.\n");
+ fprintf(stderr, "libxl_list_domain failed.\n");
return 1;
}
--
1.7.10.4
Matthew Daley
2013-Oct-30 07:51 UTC
[PATCH 23/29] docs: make `xl vcpu-list` example use verbatim formatting
Signed-off-by: Matthew Daley <mattjd@gmail.com> --- docs/man/xl.pod.1 | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1 index 5975d7b..e7b9de2 100644 --- a/docs/man/xl.pod.1 +++ b/docs/man/xl.pod.1 @@ -639,9 +639,9 @@ B<EXAMPLE> An example format for the list is as follows: -UUID ID name -59e1cf6c-6ab9-4879-90e7-adc8d1c63bf5 2 win -50bc8f75-81d0-4d53-b2e6-95cb44e2682e 3 linux + UUID ID name + 59e1cf6c-6ab9-4879-90e7-adc8d1c63bf5 2 win + 50bc8f75-81d0-4d53-b2e6-95cb44e2682e 3 linux =item B<vncviewer> [I<OPTIONS>] I<domain-id> -- 1.7.10.4
Matthew Daley
2013-Oct-30 07:52 UTC
[PATCH 24/29] libvchan: handle libxc evtchn failures properly in init functions
Also, remove now-unnecessary check from close function.
Coverity-ID: 1055609
Coverity-ID: 1055610
Coverity-ID: 1055611
Signed-off-by: Matthew Daley <mattjd@gmail.com>
---
tools/libvchan/init.c | 47 +++++++++++++++++++++++++++++++++++++++--------
tools/libvchan/io.c | 2 +-
2 files changed, 40 insertions(+), 9 deletions(-)
diff --git a/tools/libvchan/init.c b/tools/libvchan/init.c
index 0c7cff6..314b1f6 100644
--- a/tools/libvchan/init.c
+++ b/tools/libvchan/init.c
@@ -215,15 +215,30 @@ static int init_gnt_cli(struct libxenvchan *ctrl, int
domain, uint32_t ring_ref)
static int init_evt_srv(struct libxenvchan *ctrl, int domain, xentoollog_logger
*logger)
{
+ int port;
+
ctrl->event = xc_evtchn_open(logger, 0);
if (!ctrl->event)
return -1;
- ctrl->event_port = xc_evtchn_bind_unbound_port(ctrl->event, domain);
- if (ctrl->event_port < 0)
- return -1;
+
+ port = xc_evtchn_bind_unbound_port(ctrl->event, domain);
+ if (port < 0)
+ goto fail;
+ ctrl->event_port = port;
+
if (xc_evtchn_unmask(ctrl->event, ctrl->event_port))
- return -1;
+ goto fail;
+
return 0;
+
+fail:
+ if (port >= 0)
+ xc_evtchn_unbind(ctrl->event, port);
+
+ xc_evtchn_close(ctrl->event);
+ ctrl->event = NULL;
+
+ return -1;
}
static int init_xs_srv(struct libxenvchan *ctrl, int domain, const char*
xs_base, int ring_ref)
@@ -330,15 +345,31 @@ out:
static int init_evt_cli(struct libxenvchan *ctrl, int domain, xentoollog_logger
*logger)
{
+ int port;
+
ctrl->event = xc_evtchn_open(logger, 0);
if (!ctrl->event)
return -1;
- ctrl->event_port = xc_evtchn_bind_interdomain(ctrl->event,
+
+ port = xc_evtchn_bind_interdomain(ctrl->event,
domain, ctrl->event_port);
- if (ctrl->event_port < 0)
- return -1;
- xc_evtchn_unmask(ctrl->event, ctrl->event_port);
+ if (port < 0)
+ goto fail;
+ ctrl->event_port = port;
+
+ if (xc_evtchn_unmask(ctrl->event, ctrl->event_port))
+ goto fail;
+
return 0;
+
+fail:
+ if (port >= 0)
+ xc_evtchn_unbind(ctrl->event, port);
+
+ xc_evtchn_close(ctrl->event);
+ ctrl->event = NULL;
+
+ return -1;
}
diff --git a/tools/libvchan/io.c b/tools/libvchan/io.c
index 3c8d236..2383364 100644
--- a/tools/libvchan/io.c
+++ b/tools/libvchan/io.c
@@ -337,7 +337,7 @@ void libxenvchan_close(struct libxenvchan *ctrl)
}
}
if (ctrl->event) {
- if (ctrl->event_port >= 0 && ctrl->ring)
+ if (ctrl->ring)
xc_evtchn_notify(ctrl->event, ctrl->event_port);
xc_evtchn_close(ctrl->event);
}
--
1.7.10.4
Matthew Daley
2013-Oct-30 07:52 UTC
[PATCH 25/29] libvchan: check for fcntl failures in select-type sample application
Coverity-ID: 1055041
Signed-off-by: Matthew Daley <mattjd@gmail.com>
---
tools/libvchan/node-select.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/tools/libvchan/node-select.c b/tools/libvchan/node-select.c
index 6c6c19e..c6914ab 100644
--- a/tools/libvchan/node-select.c
+++ b/tools/libvchan/node-select.c
@@ -105,8 +105,10 @@ int main(int argc, char **argv)
exit(1);
}
- fcntl(0, F_SETFL, O_NONBLOCK);
- fcntl(1, F_SETFL, O_NONBLOCK);
+ if (fcntl(0, F_SETFL, O_NONBLOCK) == -1 || fcntl(1, F_SETFL, O_NONBLOCK) ==
-1) {
+ perror("fcntl");
+ exit(1);
+ }
libxenvchan_fd = libxenvchan_fd_for_select(ctrl);
for (;;) {
--
1.7.10.4
Matthew Daley
2013-Oct-30 07:52 UTC
[PATCH 26/29] xenctx: only alloc symbol if we are inserting it into the symbol table
...otherwise it''s pointless, and will leak.
Coverity-ID: 1055936
Signed-off-by: Matthew Daley <mattjd@gmail.com>
---
tools/xentrace/xenctx.c | 37 +++++++++++++++++++++++--------------
1 file changed, 23 insertions(+), 14 deletions(-)
diff --git a/tools/xentrace/xenctx.c b/tools/xentrace/xenctx.c
index 4b774af..a86d512 100644
--- a/tools/xentrace/xenctx.c
+++ b/tools/xentrace/xenctx.c
@@ -167,6 +167,7 @@ static void read_symbol_table(const char *symtab)
char *p;
struct symbol *symbol;
FILE *f;
+ guest_word_t address;
f = fopen(symtab, "r");
if(f == NULL) {
@@ -178,10 +179,8 @@ static void read_symbol_table(const char *symtab)
if(fgets(line,256,f)==NULL)
break;
- symbol = malloc(sizeof(*symbol));
-
/* need more checks for syntax here... */
- symbol->address = strtoull(line, &p, 16);
+ address = strtoull(line, &p, 16);
if (!isspace((uint8_t)*p++))
continue;
type = *p++;
@@ -196,7 +195,6 @@ static void read_symbol_table(const char *symtab)
*/
if (p[strlen(p)-1] == ''\n'')
p[strlen(p)-1] = ''\0'';
- symbol->name = strdup(p);
switch (type) {
case ''A'': /* global absolute */
@@ -207,20 +205,31 @@ static void read_symbol_table(const char *symtab)
case ''w'': /* undefined weak function */
continue;
default:
+ symbol = malloc(sizeof(*symbol));
+ if (symbol == NULL)
+ return;
+
+ symbol->address = address;
+ symbol->name = strdup(p);
+ if (symbol->name == NULL) {
+ free(symbol);
+ return;
+ }
+
insert_symbol(symbol);
break;
}
- if (strcmp(symbol->name, "_stext") == 0)
- kernel_stext = symbol->address;
- else if (strcmp(symbol->name, "_etext") == 0)
- kernel_etext = symbol->address;
- else if (strcmp(symbol->name, "_sinittext") == 0)
- kernel_sinittext = symbol->address;
- else if (strcmp(symbol->name, "_einittext") == 0)
- kernel_einittext = symbol->address;
- else if (strcmp(symbol->name, "hypercall_page") == 0)
- kernel_hypercallpage = symbol->address;
+ if (strcmp(p, "_stext") == 0)
+ kernel_stext = address;
+ else if (strcmp(p, "_etext") == 0)
+ kernel_etext = address;
+ else if (strcmp(p, "_sinittext") == 0)
+ kernel_sinittext = address;
+ else if (strcmp(p, "_einittext") == 0)
+ kernel_einittext = address;
+ else if (strcmp(p, "hypercall_page") == 0)
+ kernel_hypercallpage = address;
}
fclose(f);
--
1.7.10.4
Matthew Daley
2013-Oct-30 07:52 UTC
[PATCH 27/29] xenctx: correct error check when opening libxc
Coverity-ID: 1054979
Coverity-ID: 1055238
Signed-off-by: Matthew Daley <mattjd@gmail.com>
---
tools/xentrace/xenctx.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/xentrace/xenctx.c b/tools/xentrace/xenctx.c
index a86d512..c4b7912 100644
--- a/tools/xentrace/xenctx.c
+++ b/tools/xentrace/xenctx.c
@@ -897,7 +897,7 @@ int main(int argc, char **argv)
read_symbol_table(symbol_table);
xenctx.xc_handle = xc_interface_open(0,0,0); /* for accessing control
interface */
- if (xenctx.xc_handle < 0) {
+ if (xenctx.xc_handle == NULL) {
perror("xc_interface_open");
exit(-1);
}
--
1.7.10.4
Matthew Daley
2013-Oct-30 07:52 UTC
[PATCH 28/29] xentrace: don''t try to close null libxc handle in disable_tbufs
While at it, simplify the function.
Coverity-ID: 1055316
Signed-off-by: Matthew Daley <mattjd@gmail.com>
---
tools/xentrace/xentrace.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/tools/xentrace/xentrace.c b/tools/xentrace/xentrace.c
index 622bac8..504763d 100644
--- a/tools/xentrace/xentrace.c
+++ b/tools/xentrace/xentrace.c
@@ -425,22 +425,18 @@ fail:
static void disable_tbufs(void)
{
xc_interface *xc_handle = xc_interface_open(0,0,0);
- int ret;
if ( !xc_handle )
{
perror("Couldn''t open xc handle to disable tbufs.");
- goto out;
+ return;
}
- ret = xc_tbuf_disable(xc_handle);
-
- if ( ret != 0 )
+ if ( xc_tbuf_disable(xc_handle) != 0 )
{
perror("Couldn''t disable trace buffers");
}
-out:
xc_interface_close(xc_handle);
}
--
1.7.10.4
Matthew Daley
2013-Oct-30 07:52 UTC
[PATCH 29/29] xentrace: undeadify invalid option argument handling
Apparently it''s always been like this.
Coverity-ID: 1056153
Signed-off-by: Matthew Daley <mattjd@gmail.com>
---
tools/xentrace/xentrace.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tools/xentrace/xentrace.c b/tools/xentrace/xentrace.c
index 504763d..8a38e32 100644
--- a/tools/xentrace/xentrace.c
+++ b/tools/xentrace/xentrace.c
@@ -858,10 +858,11 @@ long sargtol(const char *restrict arg, int base)
return val;
+
invalid:
- return 0;
fprintf(stderr, "Invalid option argument: %s\n\n", arg);
usage();
+ return 0; /* not actually reached */
}
/* convert the argument string pointed to by arg to a long int representation
*/
--
1.7.10.4
Roger Pau Monné
2013-Oct-30 08:58 UTC
Re: [PATCH 14/29] libxl: don''t leak xs_read results in libxl__device_nic_from_xs_be
On 30/10/13 08:51, Matthew Daley wrote:> Coverity-ID: 1055866 > Signed-off-by: Matthew Daley <mattjd@gmail.com> > --- > tools/libxl/libxl.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c > index 0b29f32..234f3c1 100644 > --- a/tools/libxl/libxl.c > +++ b/tools/libxl/libxl.c > @@ -2975,9 +2975,10 @@ static void libxl__device_nic_from_xs_be(libxl__gc *gc, > > tmp = xs_read(ctx->xsh, XBT_NULL, > libxl__sprintf(gc, "%s/handle", be_path), &len); > - if ( tmp ) > + if ( tmp ) { > nic->devid = atoi(tmp); > - else > + free(tmp); > + } else > nic->devid = 0; > > /* nic->mtu = */ > @@ -2987,6 +2988,7 @@ static void libxl__device_nic_from_xs_be(libxl__gc *gc, > rc = libxl__parse_mac(tmp, nic->mac); > if (rc) > memset(nic->mac, 0, sizeof(nic->mac)); > + free(tmp); > > nic->ip = xs_read(ctx->xsh, XBT_NULL, > libxl__sprintf(gc, "%s/ip", be_path), &len);I think the proper way of dealing with this is to use libxl__xs_read_checked instead of xs_read, that adds the results to the gc.
Jan Beulich
2013-Oct-30 08:59 UTC
Re: [PATCH 26/29] xenctx: only alloc symbol if we are inserting it into the symbol table
>>> On 30.10.13 at 08:52, Matthew Daley <mattjd@gmail.com> wrote: > ...otherwise it''s pointless, and will leak. > > Coverity-ID: 1055936 > Signed-off-by: Matthew Daley <mattjd@gmail.com>Reviewed-by: Jan Beulich <jbeulich@suse.com>> --- > tools/xentrace/xenctx.c | 37 +++++++++++++++++++++++-------------- > 1 file changed, 23 insertions(+), 14 deletions(-) > > diff --git a/tools/xentrace/xenctx.c b/tools/xentrace/xenctx.c > index 4b774af..a86d512 100644 > --- a/tools/xentrace/xenctx.c > +++ b/tools/xentrace/xenctx.c > @@ -167,6 +167,7 @@ static void read_symbol_table(const char *symtab) > char *p; > struct symbol *symbol; > FILE *f; > + guest_word_t address; > > f = fopen(symtab, "r"); > if(f == NULL) { > @@ -178,10 +179,8 @@ static void read_symbol_table(const char *symtab) > if(fgets(line,256,f)==NULL) > break; > > - symbol = malloc(sizeof(*symbol)); > - > /* need more checks for syntax here... */ > - symbol->address = strtoull(line, &p, 16); > + address = strtoull(line, &p, 16); > if (!isspace((uint8_t)*p++)) > continue; > type = *p++; > @@ -196,7 +195,6 @@ static void read_symbol_table(const char *symtab) > */ > if (p[strlen(p)-1] == ''\n'') > p[strlen(p)-1] = ''\0''; > - symbol->name = strdup(p); > > switch (type) { > case ''A'': /* global absolute */ > @@ -207,20 +205,31 @@ static void read_symbol_table(const char *symtab) > case ''w'': /* undefined weak function */ > continue; > default: > + symbol = malloc(sizeof(*symbol)); > + if (symbol == NULL) > + return; > + > + symbol->address = address; > + symbol->name = strdup(p); > + if (symbol->name == NULL) { > + free(symbol); > + return; > + } > + > insert_symbol(symbol); > break; > } > > - if (strcmp(symbol->name, "_stext") == 0) > - kernel_stext = symbol->address; > - else if (strcmp(symbol->name, "_etext") == 0) > - kernel_etext = symbol->address; > - else if (strcmp(symbol->name, "_sinittext") == 0) > - kernel_sinittext = symbol->address; > - else if (strcmp(symbol->name, "_einittext") == 0) > - kernel_einittext = symbol->address; > - else if (strcmp(symbol->name, "hypercall_page") == 0) > - kernel_hypercallpage = symbol->address; > + if (strcmp(p, "_stext") == 0) > + kernel_stext = address; > + else if (strcmp(p, "_etext") == 0) > + kernel_etext = address; > + else if (strcmp(p, "_sinittext") == 0) > + kernel_sinittext = address; > + else if (strcmp(p, "_einittext") == 0) > + kernel_einittext = address; > + else if (strcmp(p, "hypercall_page") == 0) > + kernel_hypercallpage = address; > } > > fclose(f); > -- > 1.7.10.4 > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
Matthew Daley
2013-Oct-30 09:51 UTC
Re: [PATCH 14/29] libxl: don''t leak xs_read results in libxl__device_nic_from_xs_be
On Wed, Oct 30, 2013 at 9:58 PM, Roger Pau Monné <roger.pau@citrix.com> wrote:> On 30/10/13 08:51, Matthew Daley wrote: >> Coverity-ID: 1055866 >> Signed-off-by: Matthew Daley <mattjd@gmail.com> >> --- >> tools/libxl/libxl.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c >> index 0b29f32..234f3c1 100644 >> --- a/tools/libxl/libxl.c >> +++ b/tools/libxl/libxl.c >> @@ -2975,9 +2975,10 @@ static void libxl__device_nic_from_xs_be(libxl__gc *gc, >> >> tmp = xs_read(ctx->xsh, XBT_NULL, >> libxl__sprintf(gc, "%s/handle", be_path), &len); >> - if ( tmp ) >> + if ( tmp ) { >> nic->devid = atoi(tmp); >> - else >> + free(tmp); >> + } else >> nic->devid = 0; >> >> /* nic->mtu = */ >> @@ -2987,6 +2988,7 @@ static void libxl__device_nic_from_xs_be(libxl__gc *gc, >> rc = libxl__parse_mac(tmp, nic->mac); >> if (rc) >> memset(nic->mac, 0, sizeof(nic->mac)); >> + free(tmp); >> >> nic->ip = xs_read(ctx->xsh, XBT_NULL, >> libxl__sprintf(gc, "%s/ip", be_path), &len); > > I think the proper way of dealing with this is to use > libxl__xs_read_checked instead of xs_read, that adds the results to the gc. >Indeed, that seems the better way. I''ll try to provide a patch which does this consistently across libxl. This current patch can be considered dropped if so wanted. - Matthew
Matthew Daley
2013-Oct-30 10:12 UTC
Re: [PATCH 24/29] libvchan: handle libxc evtchn failures properly in init functions
On Wed, Oct 30, 2013 at 8:52 PM, Matthew Daley <mattjd@gmail.com> wrote:> Also, remove now-unnecessary check from close function. > > Coverity-ID: 1055609 > Coverity-ID: 1055610 > Coverity-ID: 1055611 > Signed-off-by: Matthew Daley <mattjd@gmail.com>I should clarify: the base reason for this patch is that ctrl->event_port is a uint32_t (ie. unsigned), so the current checks on it for negative error results, non-negative port presence etc. are incorrect. I can spin a v2 with this mentioned if desired. - Matthew> --- > tools/libvchan/init.c | 47 +++++++++++++++++++++++++++++++++++++++-------- > tools/libvchan/io.c | 2 +- > 2 files changed, 40 insertions(+), 9 deletions(-) > > diff --git a/tools/libvchan/init.c b/tools/libvchan/init.c > index 0c7cff6..314b1f6 100644 > --- a/tools/libvchan/init.c > +++ b/tools/libvchan/init.c > @@ -215,15 +215,30 @@ static int init_gnt_cli(struct libxenvchan *ctrl, int domain, uint32_t ring_ref) > > static int init_evt_srv(struct libxenvchan *ctrl, int domain, xentoollog_logger *logger) > { > + int port; > + > ctrl->event = xc_evtchn_open(logger, 0); > if (!ctrl->event) > return -1; > - ctrl->event_port = xc_evtchn_bind_unbound_port(ctrl->event, domain); > - if (ctrl->event_port < 0) > - return -1; > + > + port = xc_evtchn_bind_unbound_port(ctrl->event, domain); > + if (port < 0) > + goto fail; > + ctrl->event_port = port; > + > if (xc_evtchn_unmask(ctrl->event, ctrl->event_port)) > - return -1; > + goto fail; > + > return 0; > + > +fail: > + if (port >= 0) > + xc_evtchn_unbind(ctrl->event, port); > + > + xc_evtchn_close(ctrl->event); > + ctrl->event = NULL; > + > + return -1; > } > > static int init_xs_srv(struct libxenvchan *ctrl, int domain, const char* xs_base, int ring_ref) > @@ -330,15 +345,31 @@ out: > > static int init_evt_cli(struct libxenvchan *ctrl, int domain, xentoollog_logger *logger) > { > + int port; > + > ctrl->event = xc_evtchn_open(logger, 0); > if (!ctrl->event) > return -1; > - ctrl->event_port = xc_evtchn_bind_interdomain(ctrl->event, > + > + port = xc_evtchn_bind_interdomain(ctrl->event, > domain, ctrl->event_port); > - if (ctrl->event_port < 0) > - return -1; > - xc_evtchn_unmask(ctrl->event, ctrl->event_port); > + if (port < 0) > + goto fail; > + ctrl->event_port = port; > + > + if (xc_evtchn_unmask(ctrl->event, ctrl->event_port)) > + goto fail; > + > return 0; > + > +fail: > + if (port >= 0) > + xc_evtchn_unbind(ctrl->event, port); > + > + xc_evtchn_close(ctrl->event); > + ctrl->event = NULL; > + > + return -1; > } > > > diff --git a/tools/libvchan/io.c b/tools/libvchan/io.c > index 3c8d236..2383364 100644 > --- a/tools/libvchan/io.c > +++ b/tools/libvchan/io.c > @@ -337,7 +337,7 @@ void libxenvchan_close(struct libxenvchan *ctrl) > } > } > if (ctrl->event) { > - if (ctrl->event_port >= 0 && ctrl->ring) > + if (ctrl->ring) > xc_evtchn_notify(ctrl->event, ctrl->event_port); > xc_evtchn_close(ctrl->event); > } > -- > 1.7.10.4 >
Andrew Cooper
2013-Oct-30 10:50 UTC
Re: [PATCH 02/29] libxc: fix retrieval of and remove pointless check on gzip size
On 30/10/13 07:51, Matthew Daley wrote:> Coverity-ID: 1055587 > Coverity-ID: 1055963 > Signed-off-by: Matthew Daley <mattjd@gmail.com> > --- > tools/libxc/xc_dom_core.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/tools/libxc/xc_dom_core.c b/tools/libxc/xc_dom_core.c > index 0f367f6..c9366a9 100644 > --- a/tools/libxc/xc_dom_core.c > +++ b/tools/libxc/xc_dom_core.c > @@ -294,8 +294,8 @@ size_t xc_dom_check_gzip(xc_interface *xch, void *blob, size_t ziplen) > return 0; > > gzlen = blob + ziplen - 4; > - unziplen = gzlen[3] << 24 | gzlen[2] << 16 | gzlen[1] << 8 | gzlen[0]; > - if ( (unziplen < 0) || (unziplen > XC_DOM_DECOMPRESS_MAX) ) > + unziplen = (unsigned int)gzlen[3] << 24 | gzlen[2] << 16 | gzlen[1] << 8 | gzlen[0];This is very minor, but might it be better to cast to size_t, to match unziplen ? Either way, the result will now be correct. ~Andrew> + if ( unziplen > XC_DOM_DECOMPRESS_MAX ) > { > xc_dom_printf > (xch,
Matthew Daley
2013-Oct-30 10:53 UTC
Re: [PATCH 23/29] docs: make `xl vcpu-list` example use verbatim formatting
On Wed, Oct 30, 2013 at 8:51 PM, Matthew Daley <mattjd@gmail.com> wrote:> Signed-off-by: Matthew Daley <mattjd@gmail.com>Er, this patch''s title should refer to `xl vm-list`, not `xl vcpu-list`, sorry.
George Dunlap
2013-Oct-30 12:47 UTC
Re: [PATCH 29/29] xentrace: undeadify invalid option argument handling
On 30/10/13 07:52, Matthew Daley wrote:> Apparently it''s always been like this. > > Coverity-ID: 1056153 > Signed-off-by: Matthew Daley <mattjd@gmail.com>Weird! Reviewed-by: George Dunlap <george.dunlap@eu.citrix.com>> --- > tools/xentrace/xentrace.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/tools/xentrace/xentrace.c b/tools/xentrace/xentrace.c > index 504763d..8a38e32 100644 > --- a/tools/xentrace/xentrace.c > +++ b/tools/xentrace/xentrace.c > @@ -858,10 +858,11 @@ long sargtol(const char *restrict arg, int base) > > > return val; > + > invalid: > - return 0; > fprintf(stderr, "Invalid option argument: %s\n\n", arg); > usage(); > + return 0; /* not actually reached */ > } > > /* convert the argument string pointed to by arg to a long int representation */
George Dunlap
2013-Oct-30 12:47 UTC
Re: [PATCH 28/29] xentrace: don''t try to close null libxc handle in disable_tbufs
On 30/10/13 07:52, Matthew Daley wrote:> While at it, simplify the function. > > Coverity-ID: 1055316 > Signed-off-by: Matthew Daley <mattjd@gmail.com>Reviewed-by: George Dunlap <george.dunlap@eu.citrix.com>> --- > tools/xentrace/xentrace.c | 8 ++------ > 1 file changed, 2 insertions(+), 6 deletions(-) > > diff --git a/tools/xentrace/xentrace.c b/tools/xentrace/xentrace.c > index 622bac8..504763d 100644 > --- a/tools/xentrace/xentrace.c > +++ b/tools/xentrace/xentrace.c > @@ -425,22 +425,18 @@ fail: > static void disable_tbufs(void) > { > xc_interface *xc_handle = xc_interface_open(0,0,0); > - int ret; > > if ( !xc_handle ) > { > perror("Couldn''t open xc handle to disable tbufs."); > - goto out; > + return; > } > > - ret = xc_tbuf_disable(xc_handle); > - > - if ( ret != 0 ) > + if ( xc_tbuf_disable(xc_handle) != 0 ) > { > perror("Couldn''t disable trace buffers"); > } > > -out: > xc_interface_close(xc_handle); > } >
George Dunlap
2013-Oct-30 12:48 UTC
Re: [PATCH 27/29] xenctx: correct error check when opening libxc
On 30/10/13 07:52, Matthew Daley wrote:> Coverity-ID: 1054979 > Coverity-ID: 1055238 > Signed-off-by: Matthew Daley <mattjd@gmail.com>Reviewed-by: George Dunlap <george.dunlap@eu.citrix.com>> --- > tools/xentrace/xenctx.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/xentrace/xenctx.c b/tools/xentrace/xenctx.c > index a86d512..c4b7912 100644 > --- a/tools/xentrace/xenctx.c > +++ b/tools/xentrace/xenctx.c > @@ -897,7 +897,7 @@ int main(int argc, char **argv) > read_symbol_table(symbol_table); > > xenctx.xc_handle = xc_interface_open(0,0,0); /* for accessing control interface */ > - if (xenctx.xc_handle < 0) { > + if (xenctx.xc_handle == NULL) { > perror("xc_interface_open"); > exit(-1); > }
Daniel De Graaf
2013-Oct-30 14:52 UTC
Re: [PATCH 24/29] libvchan: handle libxc evtchn failures properly in init functions
On 10/30/2013 06:12 AM, Matthew Daley wrote:> On Wed, Oct 30, 2013 at 8:52 PM, Matthew Daley <mattjd@gmail.com> wrote: >> Also, remove now-unnecessary check from close function. >> >> Coverity-ID: 1055609 >> Coverity-ID: 1055610 >> Coverity-ID: 1055611 >> Signed-off-by: Matthew Daley <mattjd@gmail.com> > > I should clarify: the base reason for this patch is that > ctrl->event_port is a uint32_t (ie. unsigned), so the current checks > on it for negative error results, non-negative port presence etc. are > incorrect. > > I can spin a v2 with this mentioned if desired. > > - MatthewI agree the clarification would be useful in the commit message. If you''re spinning a v2, you may want to use evtchn_port_or_error_t in place of int since that is the actual typedef used in the return. Either way, Reviewed-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>> >> --- >> tools/libvchan/init.c | 47 +++++++++++++++++++++++++++++++++++++++-------- >> tools/libvchan/io.c | 2 +- >> 2 files changed, 40 insertions(+), 9 deletions(-) >> >> diff --git a/tools/libvchan/init.c b/tools/libvchan/init.c >> index 0c7cff6..314b1f6 100644 >> --- a/tools/libvchan/init.c >> +++ b/tools/libvchan/init.c >> @@ -215,15 +215,30 @@ static int init_gnt_cli(struct libxenvchan *ctrl, int domain, uint32_t ring_ref) >> >> static int init_evt_srv(struct libxenvchan *ctrl, int domain, xentoollog_logger *logger) >> { >> + int port; >> + >> ctrl->event = xc_evtchn_open(logger, 0); >> if (!ctrl->event) >> return -1; >> - ctrl->event_port = xc_evtchn_bind_unbound_port(ctrl->event, domain); >> - if (ctrl->event_port < 0) >> - return -1; >> + >> + port = xc_evtchn_bind_unbound_port(ctrl->event, domain); >> + if (port < 0) >> + goto fail; >> + ctrl->event_port = port; >> + >> if (xc_evtchn_unmask(ctrl->event, ctrl->event_port)) >> - return -1; >> + goto fail; >> + >> return 0; >> + >> +fail: >> + if (port >= 0) >> + xc_evtchn_unbind(ctrl->event, port); >> + >> + xc_evtchn_close(ctrl->event); >> + ctrl->event = NULL; >> + >> + return -1; >> } >> >> static int init_xs_srv(struct libxenvchan *ctrl, int domain, const char* xs_base, int ring_ref) >> @@ -330,15 +345,31 @@ out: >> >> static int init_evt_cli(struct libxenvchan *ctrl, int domain, xentoollog_logger *logger) >> { >> + int port; >> + >> ctrl->event = xc_evtchn_open(logger, 0); >> if (!ctrl->event) >> return -1; >> - ctrl->event_port = xc_evtchn_bind_interdomain(ctrl->event, >> + >> + port = xc_evtchn_bind_interdomain(ctrl->event, >> domain, ctrl->event_port); >> - if (ctrl->event_port < 0) >> - return -1; >> - xc_evtchn_unmask(ctrl->event, ctrl->event_port); >> + if (port < 0) >> + goto fail; >> + ctrl->event_port = port; >> + >> + if (xc_evtchn_unmask(ctrl->event, ctrl->event_port)) >> + goto fail; >> + >> return 0; >> + >> +fail: >> + if (port >= 0) >> + xc_evtchn_unbind(ctrl->event, port); >> + >> + xc_evtchn_close(ctrl->event); >> + ctrl->event = NULL; >> + >> + return -1; >> } >> >> >> diff --git a/tools/libvchan/io.c b/tools/libvchan/io.c >> index 3c8d236..2383364 100644 >> --- a/tools/libvchan/io.c >> +++ b/tools/libvchan/io.c >> @@ -337,7 +337,7 @@ void libxenvchan_close(struct libxenvchan *ctrl) >> } >> } >> if (ctrl->event) { >> - if (ctrl->event_port >= 0 && ctrl->ring) >> + if (ctrl->ring) >> xc_evtchn_notify(ctrl->event, ctrl->event_port); >> xc_evtchn_close(ctrl->event); >> } >> -- >> 1.7.10.4 >> >-- Daniel De Graaf National Security Agency
Daniel De Graaf
2013-Oct-30 17:06 UTC
Re: [PATCH 25/29] libvchan: check for fcntl failures in select-type sample application
On 10/30/2013 03:52 AM, Matthew Daley wrote:> Coverity-ID: 1055041 > Signed-off-by: Matthew Daley <mattjd@gmail.com> > --- > tools/libvchan/node-select.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/tools/libvchan/node-select.c b/tools/libvchan/node-select.c > index 6c6c19e..c6914ab 100644 > --- a/tools/libvchan/node-select.c > +++ b/tools/libvchan/node-select.c > @@ -105,8 +105,10 @@ int main(int argc, char **argv) > exit(1); > } > > - fcntl(0, F_SETFL, O_NONBLOCK); > - fcntl(1, F_SETFL, O_NONBLOCK); > + if (fcntl(0, F_SETFL, O_NONBLOCK) == -1 || fcntl(1, F_SETFL, O_NONBLOCK) == -1) { > + perror("fcntl"); > + exit(1); > + } > > libxenvchan_fd = libxenvchan_fd_for_select(ctrl); > for (;;) { >To be completely correct, a call to F_GETFL would be required first, with the result ORed with O_NONBLOCK and passed to F_SETFL. That is a separate existing bug in the code, however, so this patch is still an improvement as-is. Is the fcntl on line 156 different in some way that does not trigger this Coverity check? -- Daniel De Graaf National Security Agency
Andrew Cooper
2013-Oct-30 17:13 UTC
Re: [PATCH 25/29] libvchan: check for fcntl failures in select-type sample application
On 30/10/13 17:06, Daniel De Graaf wrote:> On 10/30/2013 03:52 AM, Matthew Daley wrote: >> Coverity-ID: 1055041 >> Signed-off-by: Matthew Daley <mattjd@gmail.com> >> --- >> tools/libvchan/node-select.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/tools/libvchan/node-select.c b/tools/libvchan/node-select.c >> index 6c6c19e..c6914ab 100644 >> --- a/tools/libvchan/node-select.c >> +++ b/tools/libvchan/node-select.c >> @@ -105,8 +105,10 @@ int main(int argc, char **argv) >> exit(1); >> } >> >> - fcntl(0, F_SETFL, O_NONBLOCK); >> - fcntl(1, F_SETFL, O_NONBLOCK); >> + if (fcntl(0, F_SETFL, O_NONBLOCK) == -1 || fcntl(1, F_SETFL, >> O_NONBLOCK) == -1) { >> + perror("fcntl"); >> + exit(1); >> + } >> >> libxenvchan_fd = libxenvchan_fd_for_select(ctrl); >> for (;;) { >> > > To be completely correct, a call to F_GETFL would be required first, with > the result ORed with O_NONBLOCK and passed to F_SETFL. That is a separate > existing bug in the code, however, so this patch is still an improvement > as-is. > > Is the fcntl on line 156 different in some way that does not trigger this > Coverity check? >Hmm - that error wasn''t flagged in the slightest by Coverity. I would agree that it suffers from the same problem and needs appropriately-similar fixing. It is possible Coverity only flagged the first instance of all ignored return values from fcntl(1,...) library calls. Some of the checkers seem to have logic which decides that something consistently wrong/questionable might be by design. I suspect that if the above were fixed, then the latter would identified in the next scan. ~Andrew
Matthew Daley
2013-Oct-30 22:04 UTC
Re: [PATCH 25/29] libvchan: check for fcntl failures in select-type sample application
On Thu, Oct 31, 2013 at 6:06 AM, Daniel De Graaf <dgdegra@tycho.nsa.gov> wrote:> On 10/30/2013 03:52 AM, Matthew Daley wrote: >> >> Coverity-ID: 1055041 >> Signed-off-by: Matthew Daley <mattjd@gmail.com> >> --- >> tools/libvchan/node-select.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/tools/libvchan/node-select.c b/tools/libvchan/node-select.c >> index 6c6c19e..c6914ab 100644 >> --- a/tools/libvchan/node-select.c >> +++ b/tools/libvchan/node-select.c >> @@ -105,8 +105,10 @@ int main(int argc, char **argv) >> exit(1); >> } >> >> - fcntl(0, F_SETFL, O_NONBLOCK); >> - fcntl(1, F_SETFL, O_NONBLOCK); >> + if (fcntl(0, F_SETFL, O_NONBLOCK) == -1 || fcntl(1, F_SETFL, >> O_NONBLOCK) == -1) { >> + perror("fcntl"); >> + exit(1); >> + } >> >> libxenvchan_fd = libxenvchan_fd_for_select(ctrl); >> for (;;) { >> > > To be completely correct, a call to F_GETFL would be required first, with > the result ORed with O_NONBLOCK and passed to F_SETFL. That is a separate > existing bug in the code, however, so this patch is still an improvement > as-is.Ah, yes. v2 coming along, also with changes for...> > Is the fcntl on line 156 different in some way that does not trigger this > Coverity check?...this fcntl. As Andrew replied, sometimes Coverity likes to ignore repeated issues in a single file until the balance between fixed and unfixed tips in the former''s favour (also, one tends to get tunnel vision when looking at a issue in Coverity, especially when moving across a range of subsystems. I need to work on avoiding this). - Matthew
Matthew Daley
2013-Oct-30 22:07 UTC
Re: [PATCH 24/29] libvchan: handle libxc evtchn failures properly in init functions
On Thu, Oct 31, 2013 at 3:52 AM, Daniel De Graaf <dgdegra@tycho.nsa.gov> wrote:> On 10/30/2013 06:12 AM, Matthew Daley wrote: >> >> On Wed, Oct 30, 2013 at 8:52 PM, Matthew Daley <mattjd@gmail.com> wrote: >>> >>> Also, remove now-unnecessary check from close function. >>> >>> Coverity-ID: 1055609 >>> Coverity-ID: 1055610 >>> Coverity-ID: 1055611 >>> Signed-off-by: Matthew Daley <mattjd@gmail.com> >> >> >> I should clarify: the base reason for this patch is that >> ctrl->event_port is a uint32_t (ie. unsigned), so the current checks >> on it for negative error results, non-negative port presence etc. are >> incorrect. >> >> I can spin a v2 with this mentioned if desired. >> >> - Matthew > > > I agree the clarification would be useful in the commit message. If you''re > spinning a v2, you may want to use evtchn_port_or_error_t in place of int > since that is the actual typedef used in the return.Hmm, OK. I was thinking that it may not make sense to carry a evtchn_port_or_error_t type outside of the init functions and into the library''s state (where it could be assumed that if there is a valid evtchn xc handle, there is a valid port too), but I suppose it simplifies the code, and allows that check in the close function to remain. v2 on its way... - Matthew> > Either way, > Reviewed-by: Daniel De Graaf <dgdegra@tycho.nsa.gov> > >
Matthew Daley
2013-Oct-30 22:08 UTC
Re: [PATCH 02/29] libxc: fix retrieval of and remove pointless check on gzip size
On Wed, Oct 30, 2013 at 11:50 PM, Andrew Cooper <andrew.cooper3@citrix.com> wrote:> On 30/10/13 07:51, Matthew Daley wrote: >> Coverity-ID: 1055587 >> Coverity-ID: 1055963 >> Signed-off-by: Matthew Daley <mattjd@gmail.com> >> --- >> tools/libxc/xc_dom_core.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/tools/libxc/xc_dom_core.c b/tools/libxc/xc_dom_core.c >> index 0f367f6..c9366a9 100644 >> --- a/tools/libxc/xc_dom_core.c >> +++ b/tools/libxc/xc_dom_core.c >> @@ -294,8 +294,8 @@ size_t xc_dom_check_gzip(xc_interface *xch, void *blob, size_t ziplen) >> return 0; >> >> gzlen = blob + ziplen - 4; >> - unziplen = gzlen[3] << 24 | gzlen[2] << 16 | gzlen[1] << 8 | gzlen[0]; >> - if ( (unziplen < 0) || (unziplen > XC_DOM_DECOMPRESS_MAX) ) >> + unziplen = (unsigned int)gzlen[3] << 24 | gzlen[2] << 16 | gzlen[1] << 8 | gzlen[0]; > > This is very minor, but might it be better to cast to size_t, to match > unziplen ? Either way, the result will now be correct.Might as well. v2 coming...> > ~Andrew > >> + if ( unziplen > XC_DOM_DECOMPRESS_MAX ) >> { >> xc_dom_printf >> (xch, >
Matthew Daley
2013-Oct-30 22:17 UTC
Re: [PATCH 24/29] libvchan: handle libxc evtchn failures properly in init functions
On Thu, Oct 31, 2013 at 11:07 AM, Matthew Daley <mattjd@gmail.com> wrote:> On Thu, Oct 31, 2013 at 3:52 AM, Daniel De Graaf <dgdegra@tycho.nsa.gov> wrote: >> On 10/30/2013 06:12 AM, Matthew Daley wrote: >>> >>> On Wed, Oct 30, 2013 at 8:52 PM, Matthew Daley <mattjd@gmail.com> wrote: >>>> >>>> Also, remove now-unnecessary check from close function. >>>> >>>> Coverity-ID: 1055609 >>>> Coverity-ID: 1055610 >>>> Coverity-ID: 1055611 >>>> Signed-off-by: Matthew Daley <mattjd@gmail.com> >>> >>> >>> I should clarify: the base reason for this patch is that >>> ctrl->event_port is a uint32_t (ie. unsigned), so the current checks >>> on it for negative error results, non-negative port presence etc. are >>> incorrect. >>> >>> I can spin a v2 with this mentioned if desired. >>> >>> - Matthew >> >> >> I agree the clarification would be useful in the commit message. If you''re >> spinning a v2, you may want to use evtchn_port_or_error_t in place of int >> since that is the actual typedef used in the return. > > Hmm, OK. I was thinking that it may not make sense to carry a > evtchn_port_or_error_t type outside of the init functions and into the > library''s state (where it could be assumed that if there is a valid > evtchn xc handle, there is a valid port too), but I suppose it > simplifies the code, and allows that check in the close function to > remain.Sorry, I missed the "int" in your reply. Yes, changing that type makes even more sense.> > v2 on its way... > > - Matthew > >> >> Either way, >> Reviewed-by: Daniel De Graaf <dgdegra@tycho.nsa.gov> >> >>
Matthew Daley
2013-Oct-31 02:58 UTC
[PATCH v2] libxc: fix retrieval of and remove pointless check on gzip size
Coverity-ID: 1055587
Coverity-ID: 1055963
Signed-off-by: Matthew Daley <mattjd@gmail.com>
---
v2: Cast to size_t instead of unsigned int, matching the type of
unziplen. Suggested by Andrew Cooper.
tools/libxc/xc_dom_core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/libxc/xc_dom_core.c b/tools/libxc/xc_dom_core.c
index 0f367f6..3bf51ef 100644
--- a/tools/libxc/xc_dom_core.c
+++ b/tools/libxc/xc_dom_core.c
@@ -294,8 +294,8 @@ size_t xc_dom_check_gzip(xc_interface *xch, void *blob,
size_t ziplen)
return 0;
gzlen = blob + ziplen - 4;
- unziplen = gzlen[3] << 24 | gzlen[2] << 16 | gzlen[1] <<
8 | gzlen[0];
- if ( (unziplen < 0) || (unziplen > XC_DOM_DECOMPRESS_MAX) )
+ unziplen = (size_t)gzlen[3] << 24 | gzlen[2] << 16 | gzlen[1]
<< 8 | gzlen[0];
+ if ( unziplen > XC_DOM_DECOMPRESS_MAX )
{
xc_dom_printf
(xch,
--
1.7.10.4
Matthew Daley
2013-Oct-31 03:02 UTC
[PATCH v2] docs: make `xl vm-list` example use verbatim formatting
Signed-off-by: Matthew Daley <mattjd@gmail.com> --- v2: Fix patch title (vcpu-list -> vm-list) docs/man/xl.pod.1 | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1 index 5975d7b..e7b9de2 100644 --- a/docs/man/xl.pod.1 +++ b/docs/man/xl.pod.1 @@ -639,9 +639,9 @@ B<EXAMPLE> An example format for the list is as follows: -UUID ID name -59e1cf6c-6ab9-4879-90e7-adc8d1c63bf5 2 win -50bc8f75-81d0-4d53-b2e6-95cb44e2682e 3 linux + UUID ID name + 59e1cf6c-6ab9-4879-90e7-adc8d1c63bf5 2 win + 50bc8f75-81d0-4d53-b2e6-95cb44e2682e 3 linux =item B<vncviewer> [I<OPTIONS>] I<domain-id> -- 1.7.10.4
Matthew Daley
2013-Oct-31 03:41 UTC
[PATCH v2] libvchan: handle libxc evtchn failures properly in init functions
The reasoning behind this patch is that ctrl->event_port is a uint32_t
(ie. unsigned), so the current checks on it for negative error results,
non-negative port presence etc. are incorrect.
Fix by using evtchn_port_or_error_t in the init functions instead,
adjusting the error handling, and removing the now-unnecessary check
from the close function.
Coverity-ID: 1055609
Coverity-ID: 1055610
Coverity-ID: 1055611
Signed-off-by: Matthew Daley <mattjd@gmail.com>
Reviewed-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
---
v2: Elaborate patch purpose in commit message. Change from int to
evtchn_port_or_error_t, suggested by Daniel De Graaf.
tools/libvchan/init.c | 47 +++++++++++++++++++++++++++++++++++++++--------
tools/libvchan/io.c | 2 +-
2 files changed, 40 insertions(+), 9 deletions(-)
diff --git a/tools/libvchan/init.c b/tools/libvchan/init.c
index 0c7cff6..c080a1c 100644
--- a/tools/libvchan/init.c
+++ b/tools/libvchan/init.c
@@ -215,15 +215,30 @@ static int init_gnt_cli(struct libxenvchan *ctrl, int
domain, uint32_t ring_ref)
static int init_evt_srv(struct libxenvchan *ctrl, int domain, xentoollog_logger
*logger)
{
+ evtchn_port_or_error_t port;
+
ctrl->event = xc_evtchn_open(logger, 0);
if (!ctrl->event)
return -1;
- ctrl->event_port = xc_evtchn_bind_unbound_port(ctrl->event, domain);
- if (ctrl->event_port < 0)
- return -1;
+
+ port = xc_evtchn_bind_unbound_port(ctrl->event, domain);
+ if (port < 0)
+ goto fail;
+ ctrl->event_port = port;
+
if (xc_evtchn_unmask(ctrl->event, ctrl->event_port))
- return -1;
+ goto fail;
+
return 0;
+
+fail:
+ if (port >= 0)
+ xc_evtchn_unbind(ctrl->event, port);
+
+ xc_evtchn_close(ctrl->event);
+ ctrl->event = NULL;
+
+ return -1;
}
static int init_xs_srv(struct libxenvchan *ctrl, int domain, const char*
xs_base, int ring_ref)
@@ -330,15 +345,31 @@ out:
static int init_evt_cli(struct libxenvchan *ctrl, int domain, xentoollog_logger
*logger)
{
+ evtchn_port_or_error_t port;
+
ctrl->event = xc_evtchn_open(logger, 0);
if (!ctrl->event)
return -1;
- ctrl->event_port = xc_evtchn_bind_interdomain(ctrl->event,
+
+ port = xc_evtchn_bind_interdomain(ctrl->event,
domain, ctrl->event_port);
- if (ctrl->event_port < 0)
- return -1;
- xc_evtchn_unmask(ctrl->event, ctrl->event_port);
+ if (port < 0)
+ goto fail;
+ ctrl->event_port = port;
+
+ if (xc_evtchn_unmask(ctrl->event, ctrl->event_port))
+ goto fail;
+
return 0;
+
+fail:
+ if (port >= 0)
+ xc_evtchn_unbind(ctrl->event, port);
+
+ xc_evtchn_close(ctrl->event);
+ ctrl->event = NULL;
+
+ return -1;
}
diff --git a/tools/libvchan/io.c b/tools/libvchan/io.c
index 3c8d236..2383364 100644
--- a/tools/libvchan/io.c
+++ b/tools/libvchan/io.c
@@ -337,7 +337,7 @@ void libxenvchan_close(struct libxenvchan *ctrl)
}
}
if (ctrl->event) {
- if (ctrl->event_port >= 0 && ctrl->ring)
+ if (ctrl->ring)
xc_evtchn_notify(ctrl->event, ctrl->event_port);
xc_evtchn_close(ctrl->event);
}
--
1.7.10.4
Matthew Daley
2013-Oct-31 03:44 UTC
[PATCH v2] libvchan: tidy up usages of fcntl in select-type sample application
Namely, don''t overwrite all the other flags when twiddling O_NONBLOCK,
and add basic error handling.
Coverity-ID: 1055041
Signed-off-by: Matthew Daley <mattjd@gmail.com>
---
v2: Adjust other fcntl usage further down as well. Use F_GETFL to modify
only the O_NONBLOCK flag. Both suggested by Daniel De Graaf.
Improve commit message.
tools/libvchan/node-select.c | 16 +++++++++++++---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/tools/libvchan/node-select.c b/tools/libvchan/node-select.c
index 6c6c19e..acd9bae 100644
--- a/tools/libvchan/node-select.c
+++ b/tools/libvchan/node-select.c
@@ -90,6 +90,7 @@ int main(int argc, char **argv)
{
int ret;
int libxenvchan_fd;
+ int flags;
if (argc < 4 || argv[3][0] != ''/'')
usage(argv);
if (!strcmp(argv[1], "server")) {
@@ -105,8 +106,13 @@ int main(int argc, char **argv)
exit(1);
}
- fcntl(0, F_SETFL, O_NONBLOCK);
- fcntl(1, F_SETFL, O_NONBLOCK);
+ if ((flags = fcntl(0, F_GETFL)) == -1 ||
+ fcntl(0, F_SETFL, flags | O_NONBLOCK) == -1 ||
+ (flags = fcntl(1, F_GETFL)) == -1 ||
+ fcntl(1, F_SETFL, flags | O_NONBLOCK) == -1) {
+ perror("fcntl");
+ exit(1);
+ }
libxenvchan_fd = libxenvchan_fd_for_select(ctrl);
for (;;) {
@@ -153,7 +159,11 @@ int main(int argc, char **argv)
stdout_wr();
}
if (!libxenvchan_is_open(ctrl)) {
- fcntl(1, F_SETFL, 0);
+ if ((flags = fcntl(1, F_GETFL)) == -1 ||
+ fcntl(1, F_SETFL, flags & ~O_NONBLOCK) == -1) {
+ perror("fcntl");
+ exit(1);
+ }
while (outsiz)
stdout_wr();
return 0;
--
1.7.10.4
Andrew Cooper
2013-Oct-31 10:22 UTC
Re: [PATCH v2] libxc: fix retrieval of and remove pointless check on gzip size
On 31/10/13 02:58, Matthew Daley wrote:> Coverity-ID: 1055587 > Coverity-ID: 1055963 > Signed-off-by: Matthew Daley <mattjd@gmail.com>Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>> --- > v2: Cast to size_t instead of unsigned int, matching the type of > unziplen. Suggested by Andrew Cooper. > > tools/libxc/xc_dom_core.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/tools/libxc/xc_dom_core.c b/tools/libxc/xc_dom_core.c > index 0f367f6..3bf51ef 100644 > --- a/tools/libxc/xc_dom_core.c > +++ b/tools/libxc/xc_dom_core.c > @@ -294,8 +294,8 @@ size_t xc_dom_check_gzip(xc_interface *xch, void *blob, size_t ziplen) > return 0; > > gzlen = blob + ziplen - 4; > - unziplen = gzlen[3] << 24 | gzlen[2] << 16 | gzlen[1] << 8 | gzlen[0]; > - if ( (unziplen < 0) || (unziplen > XC_DOM_DECOMPRESS_MAX) ) > + unziplen = (size_t)gzlen[3] << 24 | gzlen[2] << 16 | gzlen[1] << 8 | gzlen[0]; > + if ( unziplen > XC_DOM_DECOMPRESS_MAX ) > { > xc_dom_printf > (xch,
On 30/10/13 07:51, Matthew Daley wrote:> This also includes a couple of non-Coverity-spotted issues which were found > while working on said Coverity-spotted issues. > > Tested by make installing the entire tree, creating a few PV domains w/ xl, > using vchan-node2 (libvchan) and xenctx. >Having now had an opportunity to look at all of the patches, everything appears to be in order Therefore, all (including v2''s where appropriate) Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> This should hopefully make a very large dent in our bug/issue statistics.> _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
Ian Campbell
2013-Oct-31 21:03 UTC
Re: [PATCH 01/29] libxc: check for xc_domain_get_guest_width failure and pass it down
On Wed, 2013-10-30 at 20:51 +1300, Matthew Daley wrote:> @@ -443,7 +443,8 @@ static void xc_cpuid_pv_policy( > > xc_cpuid_brand_get(brand); > > - xc_domain_get_guest_width(xch, domid, &guest_width); > + if ( xc_domain_get_guest_width(xch, domid, &guest_width) != 0 ) > + return 1;Not the actual error from xc_domain_get_guest_width?
Ian Campbell
2013-Oct-31 21:22 UTC
Re: [PATCH 06/29] libxc: don''t read uninitialized size value in xc_read_image
On Wed, 2013-10-30 at 20:51 +1300, Matthew Daley wrote:> This error case can only be triggered by gzread returning 0 (and having > not read anything), so move it there. > > Coverity-ID: 1056076Is this right? It seems to correspond to an issue in xc_hvm_build -- which doesn''t look related.> Signed-off-by: Matthew Daley <mattjd@gmail.com> > --- > tools/libxc/xg_private.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/tools/libxc/xg_private.c b/tools/libxc/xg_private.c > index 8fa068e..a914068 100644 > --- a/tools/libxc/xg_private.c > +++ b/tools/libxc/xg_private.c > @@ -71,6 +71,12 @@ char *xc_read_image(xc_interface *xch, > image = NULL; > goto out; > case 0: /* EOF */ > + if ( *size == 0 ) > + { > + PERROR("Could not read kernel image"); > + free(image); > + image = NULL; > + } > goto out; > default: > *size += bytes; > @@ -80,13 +86,7 @@ char *xc_read_image(xc_interface *xch, > #undef CHUNK > > out: > - if ( *size == 0 ) > - { > - PERROR("Could not read kernel image"); > - free(image); > - image = NULL; > - } > - else if ( image ) > + if ( image ) > { > /* Shrink allocation to fit image. */ > tmp = realloc(image, *size);
Ian Campbell
2013-Oct-31 21:29 UTC
Re: [PATCH 13/29] libxl: check for xc_domain_max_vcpus failure in libxl__build_pre
On Wed, 2013-10-30 at 20:51 +1300, Matthew Daley wrote:> Coverity-ID: 1055047 > Signed-off-by: Matthew Daley <mattjd@gmail.com> > --- > tools/libxl/libxl_dom.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c > index 1873459..5b9fd27 100644 > --- a/tools/libxl/libxl_dom.c > +++ b/tools/libxl/libxl_dom.c > @@ -209,7 +209,10 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid, > char *xs_domid, *con_domid; > int rc; > > - xc_domain_max_vcpus(ctx->xch, domid, info->max_vcpus); > + if (xc_domain_max_vcpus(ctx->xch, domid, info->max_vcpus) != 0) { > + LOG(ERROR, "Couldn''t get max vcpu count");This function is a setter, isn''t it?> + return ERROR_FAIL; > + } > > /* > * Check if the domain has any CPU affinity. If not, try to build
Ian Campbell
2013-Oct-31 21:31 UTC
Re: [PATCH 14/29] libxl: don''t leak xs_read results in libxl__device_nic_from_xs_be
On Wed, 2013-10-30 at 22:51 +1300, Matthew Daley wrote:> On Wed, Oct 30, 2013 at 9:58 PM, Roger Pau Monné <roger.pau@citrix.com> wrote: > > I think the proper way of dealing with this is to use > > libxl__xs_read_checked instead of xs_read, that adds the results to the gc. > > > > Indeed, that seems the better way. I''ll try to provide a patch which > does this consistently across libxl.Thanks.> This current patch can be > considered dropped if so wanted.Done.
Ian Campbell
2013-Oct-31 21:36 UTC
Re: [PATCH 16/29] libxl: check for xc_domain_shutdown failure in libxl__domain_suspend_common_callback
On Wed, 2013-10-30 at 20:51 +1300, Matthew Daley wrote:> Coverity-ID: 1055048 > Signed-off-by: Matthew Daley <mattjd@gmail.com> > --- > tools/libxl/libxl_dom.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c > index 5b9fd27..4606a12 100644 > --- a/tools/libxl/libxl_dom.c > +++ b/tools/libxl/libxl_dom.c > @@ -1041,7 +1041,11 @@ int libxl__domain_suspend_common_callback(void *user) > > if (dss->hvm && (!hvm_pvdrv || hvm_s_state)) { > LOG(DEBUG, "Calling xc_domain_shutdown on HVM domain"); > - xc_domain_shutdown(CTX->xch, domid, SHUTDOWN_suspend); > + ret = xc_domain_shutdown(CTX->xch, domid, SHUTDOWN_suspend); > + if (ret < 0) { > + LOG(ERROR, "xc_domain_shutdown failed ret=%d", ret);This returns -1 and sets errno I think, if so then ret is largely uninteresting. If you want to print errno you can use LOGE() and if ret really was an errno val then LOGEV() will do the right thing, I think.> + return 0; > + } > /* The guest does not (need to) respond to this sort of request. */ > dss->guest_responded = 1; > } else {
Ian Campbell
2013-Oct-31 21:47 UTC
Re: [PATCH v2] libvchan: tidy up usages of fcntl in select-type sample application
On Thu, 2013-10-31 at 16:44 +1300, Matthew Daley wrote:> Namely, don''t overwrite all the other flags when twiddling O_NONBLOCK, > and add basic error handling. > > Coverity-ID: 1055041 > Signed-off-by: Matthew Daley <mattjd@gmail.com> > --- > v2: Adjust other fcntl usage further down as well. Use F_GETFL to modify > only the O_NONBLOCK flag. Both suggested by Daniel De Graaf. > Improve commit message. > > tools/libvchan/node-select.c | 16 +++++++++++++--- > 1 file changed, 13 insertions(+), 3 deletions(-) > > diff --git a/tools/libvchan/node-select.c b/tools/libvchan/node-select.c > index 6c6c19e..acd9bae 100644 > --- a/tools/libvchan/node-select.c > +++ b/tools/libvchan/node-select.c > @@ -90,6 +90,7 @@ int main(int argc, char **argv) > { > int ret; > int libxenvchan_fd; > + int flags; > if (argc < 4 || argv[3][0] != ''/'') > usage(argv); > if (!strcmp(argv[1], "server")) { > @@ -105,8 +106,13 @@ int main(int argc, char **argv) > exit(1); > } > > - fcntl(0, F_SETFL, O_NONBLOCK); > - fcntl(1, F_SETFL, O_NONBLOCK); > + if ((flags = fcntl(0, F_GETFL)) == -1 || > + fcntl(0, F_SETFL, flags | O_NONBLOCK) == -1 || > + (flags = fcntl(1, F_GETFL)) == -1 || > + fcntl(1, F_SETFL, flags | O_NONBLOCK) == -1) { > + perror("fcntl"); > + exit(1); > + }I''m sure this is correct but it feels like it could be written in a more straightforward/readable way as a series of ifs rather than bundling them all together. Since you do the same thing twice to two file descs it might even be a candidate for a small helper function?> > libxenvchan_fd = libxenvchan_fd_for_select(ctrl); > for (;;) { > @@ -153,7 +159,11 @@ int main(int argc, char **argv) > stdout_wr(); > } > if (!libxenvchan_is_open(ctrl)) { > - fcntl(1, F_SETFL, 0); > + if ((flags = fcntl(1, F_GETFL)) == -1 || > + fcntl(1, F_SETFL, flags & ~O_NONBLOCK) == -1) { > + perror("fcntl"); > + exit(1); > + } > while (outsiz) > stdout_wr(); > return 0;
On Fri, Nov 1, 2013 at 1:28 AM, Andrew Cooper <andrew.cooper3@citrix.com> wrote:> On 30/10/13 07:51, Matthew Daley wrote: >> This also includes a couple of non-Coverity-spotted issues which were found >> while working on said Coverity-spotted issues. >> >> Tested by make installing the entire tree, creating a few PV domains w/ xl, >> using vchan-node2 (libvchan) and xenctx. >> > > Having now had an opportunity to look at all of the patches, everything > appears to be in order > > Therefore, all (including v2''s where appropriate) > > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> > > This should hopefully make a very large dent in our bug/issue statistics.I don''t know about "very large" ;), but it should be virtually all of the libxc ones, and another third of the libxl ones. libxl should be done in one more "volume" (I think...) - Matthew
Ian Campbell
2013-Oct-31 21:56 UTC
Re: [PATCH 09/29] libxc: check for xc_vcpu_setcontext failure in xc_domain_resume_any
On Wed, 2013-10-30 at 20:51 +1300, Matthew Daley wrote:> Coverity-ID: 1090352 > Signed-off-by: Matthew Daley <mattjd@gmail.com> > --- > tools/libxc/xc_resume.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/tools/libxc/xc_resume.c b/tools/libxc/xc_resume.c > index 50724f2..a4c0f53 100644 > --- a/tools/libxc/xc_resume.c > +++ b/tools/libxc/xc_resume.c > @@ -211,7 +211,11 @@ static int xc_domain_resume_any(xc_interface *xch, uint32_t domid) > > /* Reset all secondary CPU states. */ > for ( i = 1; i <= info.max_vcpu_id; i++ ) > - xc_vcpu_setcontext(xch, domid, i, NULL); > + if ( xc_vcpu_setcontext(xch, domid, i, NULL) != 0 ) > + { > + ERROR("Couldn''t reset vcpu state"); > + goto out;The label is inside an x86 specific ifdef and therefore this breaks on ARM.> + } > > /* Ready to resume domain execution now. */ > domctl.cmd = XEN_DOMCTL_resumedomain;
Ian Campbell
2013-Oct-31 22:07 UTC
Re: [PATCH v2] libxc: fix retrieval of and remove pointless check on gzip size
On Thu, 2013-10-31 at 10:22 +0000, Andrew Cooper wrote:> On 31/10/13 02:58, Matthew Daley wrote: > > Coverity-ID: 1055587 > > Coverity-ID: 1055963 > > Signed-off-by: Matthew Daley <mattjd@gmail.com> > > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>I''d have used a few more ()s for clarity, but acked + applied.> > > --- > > v2: Cast to size_t instead of unsigned int, matching the type of > > unziplen. Suggested by Andrew Cooper. > > > > tools/libxc/xc_dom_core.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/tools/libxc/xc_dom_core.c b/tools/libxc/xc_dom_core.c > > index 0f367f6..3bf51ef 100644 > > --- a/tools/libxc/xc_dom_core.c > > +++ b/tools/libxc/xc_dom_core.c > > @@ -294,8 +294,8 @@ size_t xc_dom_check_gzip(xc_interface *xch, void *blob, size_t ziplen) > > return 0; > > > > gzlen = blob + ziplen - 4; > > - unziplen = gzlen[3] << 24 | gzlen[2] << 16 | gzlen[1] << 8 | gzlen[0]; > > - if ( (unziplen < 0) || (unziplen > XC_DOM_DECOMPRESS_MAX) ) > > + unziplen = (size_t)gzlen[3] << 24 | gzlen[2] << 16 | gzlen[1] << 8 | gzlen[0]; > > + if ( unziplen > XC_DOM_DECOMPRESS_MAX ) > > { > > xc_dom_printf > > (xch, >
On Thu, 2013-10-31 at 12:28 +0000, Andrew Cooper wrote:> On 30/10/13 07:51, Matthew Daley wrote: > > This also includes a couple of non-Coverity-spotted issues which were found > > while working on said Coverity-spotted issues. > > > > Tested by make installing the entire tree, creating a few PV domains w/ xl, > > using vchan-node2 (libvchan) and xenctx. > > > > Having now had an opportunity to look at all of the patches, everything > appears to be in order > > Therefore, all (including v2''s where appropriate) > > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>Thanks. I''ve applied the bulk of this, picking up v2 (hopefully) as appropriate: 96be5ea xentrace: undeadify invalid option argument handling 7343db9 xentrace: don''t try to close null libxc handle in disable_tbufs 1924ed3 xenctx: correct error check when opening libxc 33d3716 xenctx: only alloc symbol if we are inserting it into the symbol table f2f06a2 libvchan: handle libxc evtchn failures properly in init functions 93700cb docs: make `xl vm-list` example use verbatim formatting 9d801e2 xl: correct references to long-removed function in error messages 50b9bdf xl: remove needless infinite-loop construct in create_domain 77d8f86 xl: check for restore file open failure in create_domain 235f16a xl: libxl_list_vm returns a pointer, not a handle 2ffc14e libxl: only close fds which successfully opened in libxl__spawn_local_dm 1edd6d8 libxl: don''t leak memory in libxl__poller_get failure case 4b62556 libxl: correct file open success check in libxl__device_pci_reset 5bc3a84 libxl: check for transaction abortion failure in libxl_set_memory_target d6dea2f libxc: remove null pointer checks in xc_pm_get_{c, p}xstat 492da27 libxc: initalize stdio loggers'' progress_last_percent values to 0 e1fde2a libxc: check for xc_domain_get_guest_width failure in xc_domain_resume_any 6b02836 libxc: remove pointless null pointer check in xc_interface_open_common fcaacad libxc: tidy up loop construct in write_compressed f46fa80 libxc: don''t ignore fd read failures when restoring a domain 9fae04f libxc: fix memory leak in move_l3_below_4G error handling e9fa4c8 libxc: fix retrieval of and remove pointless check on gzip size I think I replied to everything which I didn''t apply (or else observed that someone else had replied) but I''m afraid I didn''t keep detailed notes, hopefully "git rebase" will make it obvious which ones I missed.> This should hopefully make a very large dent in our bug/issue statistics.Absolutely!> > > _______________________________________________ > > Xen-devel mailing list > > Xen-devel@lists.xen.org > > http://lists.xen.org/xen-devel > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
Matthew Daley
2013-Oct-31 22:12 UTC
Re: [PATCH 06/29] libxc: don''t read uninitialized size value in xc_read_image
On Fri, Nov 1, 2013 at 10:22 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote:> On Wed, 2013-10-30 at 20:51 +1300, Matthew Daley wrote: >> This error case can only be triggered by gzread returning 0 (and having >> not read anything), so move it there. >> >> Coverity-ID: 1056076 > > Is this right? It seems to correspond to an issue in xc_hvm_build -- > which doesn''t look related.xc_hvm_build calls xc_read_image, which is where the issue itself lies (click on "show details" on event 8.)> >> Signed-off-by: Matthew Daley <mattjd@gmail.com> >> --- >> tools/libxc/xg_private.c | 14 +++++++------- >> 1 file changed, 7 insertions(+), 7 deletions(-) >> >> diff --git a/tools/libxc/xg_private.c b/tools/libxc/xg_private.c >> index 8fa068e..a914068 100644 >> --- a/tools/libxc/xg_private.c >> +++ b/tools/libxc/xg_private.c >> @@ -71,6 +71,12 @@ char *xc_read_image(xc_interface *xch, >> image = NULL; >> goto out; >> case 0: /* EOF */ >> + if ( *size == 0 ) >> + { >> + PERROR("Could not read kernel image"); >> + free(image); >> + image = NULL; >> + } >> goto out; >> default: >> *size += bytes; >> @@ -80,13 +86,7 @@ char *xc_read_image(xc_interface *xch, >> #undef CHUNK >> >> out: >> - if ( *size == 0 ) >> - { >> - PERROR("Could not read kernel image"); >> - free(image); >> - image = NULL; >> - } >> - else if ( image ) >> + if ( image ) >> { >> /* Shrink allocation to fit image. */ >> tmp = realloc(image, *size); > >
Ian Campbell
2013-Oct-31 22:26 UTC
Re: [PATCH 06/29] libxc: don''t read uninitialized size value in xc_read_image
On Fri, 2013-11-01 at 11:12 +1300, Matthew Daley wrote:> On Fri, Nov 1, 2013 at 10:22 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote: > > On Wed, 2013-10-30 at 20:51 +1300, Matthew Daley wrote: > >> This error case can only be triggered by gzread returning 0 (and having > >> not read anything), so move it there. > >> > >> Coverity-ID: 1056076 > > > > Is this right? It seems to correspond to an issue in xc_hvm_build -- > > which doesn''t look related. > > xc_hvm_build calls xc_read_image, which is where the issue itself lies > (click on "show details" on event 8.)Thanks, applied.
Matthew Daley
2013-Nov-01 00:24 UTC
[PATCH v2] libxc: check for xc_domain_get_guest_width failure and pass it down
...in xc_cpuid_pv_policy.
Coverity-ID: 1093050
Signed-off-by: Matthew Daley <mattjd@gmail.com>
---
v2: Return actual error from xc_domain_get_guest_width
tools/libxc/xc_cpuid_x86.c | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
index bbbf9b8..328769a 100644
--- a/tools/libxc/xc_cpuid_x86.c
+++ b/tools/libxc/xc_cpuid_x86.c
@@ -431,11 +431,12 @@ static void xc_cpuid_hvm_policy(
}
-static void xc_cpuid_pv_policy(
+static int xc_cpuid_pv_policy(
xc_interface *xch, domid_t domid,
const unsigned int *input, unsigned int *regs)
{
DECLARE_DOMCTL;
+ int rc;
unsigned int guest_width;
int guest_64bit, xen_64bit = hypervisor_is_64bit(xch);
char brand[13];
@@ -443,7 +444,9 @@ static void xc_cpuid_pv_policy(
xc_cpuid_brand_get(brand);
- xc_domain_get_guest_width(xch, domid, &guest_width);
+ rc = xc_domain_get_guest_width(xch, domid, &guest_width);
+ if ( rc != 0 )
+ return rc;
guest_64bit = (guest_width == 8);
/* Detecting Xen''s atitude towards XSAVE */
@@ -547,6 +550,8 @@ static void xc_cpuid_pv_policy(
regs[0] = regs[1] = regs[2] = regs[3] = 0;
break;
}
+
+ return rc;
}
static int xc_cpuid_policy(
@@ -561,7 +566,8 @@ static int xc_cpuid_policy(
if ( info.hvm )
xc_cpuid_hvm_policy(xch, domid, input, regs);
else
- xc_cpuid_pv_policy(xch, domid, input, regs);
+ if ( xc_cpuid_pv_policy(xch, domid, input, regs) != 0 )
+ return -EINVAL;
return 0;
}
@@ -632,7 +638,9 @@ int xc_cpuid_apply_policy(xc_interface *xch, domid_t domid)
for ( ; ; )
{
cpuid(input, regs);
- xc_cpuid_policy(xch, domid, input, regs);
+ rc = xc_cpuid_policy(xch, domid, input, regs);
+ if ( rc )
+ return rc;
if ( regs[0] || regs[1] || regs[2] || regs[3] )
{
--
1.7.10.4
Matthew Daley
2013-Nov-01 00:27 UTC
[PATCH v2] libxc: check for xc_vcpu_setcontext failure in xc_domain_resume_any
Coverity-ID: 1090352
Signed-off-by: Matthew Daley <mattjd@gmail.com>
---
v2: Move ''out'' label out of x86 #ifdef region
tools/libxc/xc_resume.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/tools/libxc/xc_resume.c b/tools/libxc/xc_resume.c
index 50724f2..18b4818 100644
--- a/tools/libxc/xc_resume.c
+++ b/tools/libxc/xc_resume.c
@@ -211,15 +211,19 @@ static int xc_domain_resume_any(xc_interface *xch,
uint32_t domid)
/* Reset all secondary CPU states. */
for ( i = 1; i <= info.max_vcpu_id; i++ )
- xc_vcpu_setcontext(xch, domid, i, NULL);
+ if ( xc_vcpu_setcontext(xch, domid, i, NULL) != 0 )
+ {
+ ERROR("Couldn''t reset vcpu state");
+ goto out;
+ }
/* Ready to resume domain execution now. */
domctl.cmd = XEN_DOMCTL_resumedomain;
domctl.domain = domid;
rc = do_domctl(xch, &domctl);
+out:
#if defined(__i386__) || defined(__x86_64__)
- out:
if (p2m)
munmap(p2m, P2M_FL_ENTRIES*PAGE_SIZE);
if (p2m_frame_list)
--
1.7.10.4
Matthew Daley
2013-Nov-01 00:29 UTC
[PATCH v2] libxl: check for xc_domain_max_vcpus failure in libxl__build_pre
Coverity-ID: 1055047
Signed-off-by: Matthew Daley <mattjd@gmail.com>
---
v2: Correct error message (get -> set)
tools/libxl/libxl_dom.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 1873459..c164469 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -209,7 +209,10 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
char *xs_domid, *con_domid;
int rc;
- xc_domain_max_vcpus(ctx->xch, domid, info->max_vcpus);
+ if (xc_domain_max_vcpus(ctx->xch, domid, info->max_vcpus) != 0) {
+ LOG(ERROR, "Couldn''t set max vcpu count");
+ return ERROR_FAIL;
+ }
/*
* Check if the domain has any CPU affinity. If not, try to build
--
1.7.10.4
Matthew Daley
2013-Nov-01 00:30 UTC
[PATCH v2] libxl: check for xc_domain_shutdown failure in libxl__domain_suspend_common_callback
Coverity-ID: 1055048
Signed-off-by: Matthew Daley <mattjd@gmail.com>
---
v2: Don''t log ret in error case, log errno instead
tools/libxl/libxl_dom.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index c164469..1812bdc 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1041,7 +1041,11 @@ int libxl__domain_suspend_common_callback(void *user)
if (dss->hvm && (!hvm_pvdrv || hvm_s_state)) {
LOG(DEBUG, "Calling xc_domain_shutdown on HVM domain");
- xc_domain_shutdown(CTX->xch, domid, SHUTDOWN_suspend);
+ ret = xc_domain_shutdown(CTX->xch, domid, SHUTDOWN_suspend);
+ if (ret < 0) {
+ LOGE(ERROR, "xc_domain_shutdown failed");
+ return 0;
+ }
/* The guest does not (need to) respond to this sort of request. */
dss->guest_responded = 1;
} else {
--
1.7.10.4
Matthew Daley
2013-Nov-01 00:33 UTC
[PATCH v3] libvchan: tidy up usages of fcntl in select-type sample application
Namely, don''t overwrite all the other flags when twiddling O_NONBLOCK,
and add basic error handling.
Coverity-ID: 1055041
Signed-off-by: Matthew Daley <mattjd@gmail.com>
---
v2: Adjust other fcntl usage further down as well. Use F_GETFL to modify
only the O_NONBLOCK flag. Both suggested by Daniel De Graaf.
Improve commit message.
v3: Make a new function, set_nonblocking, to do the twiddling
tools/libvchan/node-select.c | 27 ++++++++++++++++++++++++---
1 file changed, 24 insertions(+), 3 deletions(-)
diff --git a/tools/libvchan/node-select.c b/tools/libvchan/node-select.c
index 6c6c19e..13c5822 100644
--- a/tools/libvchan/node-select.c
+++ b/tools/libvchan/node-select.c
@@ -80,6 +80,22 @@ void stdout_wr() {
}
}
+static int set_nonblocking(int fd, int nonblocking) {
+ int flags = fcntl(fd, F_GETFL);
+ if (flags == -1)
+ return -1;
+
+ if (nonblocking)
+ flags |= O_NONBLOCK;
+ else
+ flags &= ~O_NONBLOCK;
+
+ if (fcntl(fd, F_SETFL, flags) == -1)
+ return -1;
+
+ return 0;
+}
+
/**
Simple libxenvchan application, both client and server.
Both sides may write and read, both from the libxenvchan and from
@@ -105,8 +121,10 @@ int main(int argc, char **argv)
exit(1);
}
- fcntl(0, F_SETFL, O_NONBLOCK);
- fcntl(1, F_SETFL, O_NONBLOCK);
+ if (set_nonblocking(0, 1) || set_nonblocking(1, 1)) {
+ perror("set_nonblocking");
+ exit(1);
+ }
libxenvchan_fd = libxenvchan_fd_for_select(ctrl);
for (;;) {
@@ -153,7 +171,10 @@ int main(int argc, char **argv)
stdout_wr();
}
if (!libxenvchan_is_open(ctrl)) {
- fcntl(1, F_SETFL, 0);
+ if (set_nonblocking(1, 0)) {
+ perror("set_nonblocking");
+ exit(1);
+ }
while (outsiz)
stdout_wr();
return 0;
--
1.7.10.4
On Fri, Nov 1, 2013 at 11:10 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote:> On Thu, 2013-10-31 at 12:28 +0000, Andrew Cooper wrote: >> On 30/10/13 07:51, Matthew Daley wrote: >> > This also includes a couple of non-Coverity-spotted issues which were found >> > while working on said Coverity-spotted issues. >> > >> > Tested by make installing the entire tree, creating a few PV domains w/ xl, >> > using vchan-node2 (libvchan) and xenctx. >> > >> >> Having now had an opportunity to look at all of the patches, everything >> appears to be in order >> >> Therefore, all (including v2''s where appropriate) >> >> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> > > Thanks. I''ve applied the bulk of this, picking up v2 (hopefully) as > appropriate: > 96be5ea xentrace: undeadify invalid option argument handling > 7343db9 xentrace: don''t try to close null libxc handle in disable_tbufs > 1924ed3 xenctx: correct error check when opening libxc > 33d3716 xenctx: only alloc symbol if we are inserting it into the symbol table > f2f06a2 libvchan: handle libxc evtchn failures properly in init functions > 93700cb docs: make `xl vm-list` example use verbatim formatting > 9d801e2 xl: correct references to long-removed function in error messages > 50b9bdf xl: remove needless infinite-loop construct in create_domain > 77d8f86 xl: check for restore file open failure in create_domain > 235f16a xl: libxl_list_vm returns a pointer, not a handle > 2ffc14e libxl: only close fds which successfully opened in libxl__spawn_local_dm > 1edd6d8 libxl: don''t leak memory in libxl__poller_get failure case > 4b62556 libxl: correct file open success check in libxl__device_pci_reset > 5bc3a84 libxl: check for transaction abortion failure in libxl_set_memory_target > d6dea2f libxc: remove null pointer checks in xc_pm_get_{c, p}xstat > 492da27 libxc: initalize stdio loggers'' progress_last_percent values to 0 > e1fde2a libxc: check for xc_domain_get_guest_width failure in xc_domain_resume_any > 6b02836 libxc: remove pointless null pointer check in xc_interface_open_common > fcaacad libxc: tidy up loop construct in write_compressed > f46fa80 libxc: don''t ignore fd read failures when restoring a domain > 9fae04f libxc: fix memory leak in move_l3_below_4G error handling > e9fa4c8 libxc: fix retrieval of and remove pointless check on gzip size > > I think I replied to everything which I didn''t apply (or else observed > that someone else had replied) but I''m afraid I didn''t keep detailed > notes, hopefully "git rebase" will make it obvious which ones I missed.Thanks, and indeed it did. The remaining 5 patches have new versions posted.> >> This should hopefully make a very large dent in our bug/issue statistics. > > Absolutely! > >> >> > _______________________________________________ >> > Xen-devel mailing list >> > Xen-devel@lists.xen.org >> > http://lists.xen.org/xen-devel >> >> >> _______________________________________________ >> Xen-devel mailing list >> Xen-devel@lists.xen.org >> http://lists.xen.org/xen-devel > >
Ian Campbell
2013-Nov-01 13:12 UTC
Re: [PATCH v2] libxc: check for xc_domain_get_guest_width failure and pass it down
On Fri, 2013-11-01 at 13:24 +1300, Matthew Daley wrote:> ...in xc_cpuid_pv_policy. > > Coverity-ID: 1093050 > Signed-off-by: Matthew Daley <mattjd@gmail.com> > --- > v2: Return actual error from xc_domain_get_guest_width[...]> - xc_cpuid_pv_policy(xch, domid, input, regs); > + if ( xc_cpuid_pv_policy(xch, domid, input, regs) != 0 ) > + return -EINVAL;But then you throw it away again one step further up the chain? Ian.
Ian Campbell
2013-Nov-01 13:26 UTC
Re: [PATCH v2] libxc: check for xc_vcpu_setcontext failure in xc_domain_resume_any
On Fri, 2013-11-01 at 13:27 +1300, Matthew Daley wrote:> Coverity-ID: 1090352 > Signed-off-by: Matthew Daley <mattjd@gmail.com> > --- > v2: Move ''out'' label out of x86 #ifdef regionthanks, akced+ applied.
Ian Campbell
2013-Nov-01 13:27 UTC
Re: [PATCH v2] libxl: check for xc_domain_max_vcpus failure in libxl__build_pre
On Fri, 2013-11-01 at 13:29 +1300, Matthew Daley wrote:> Coverity-ID: 1055047 > Signed-off-by: Matthew Daley <mattjd@gmail.com>akced + applied, thanks.
Ian Campbell
2013-Nov-01 13:27 UTC
Re: [PATCH v2] libxl: check for xc_domain_shutdown failure in libxl__domain_suspend_common_callback
On Fri, 2013-11-01 at 13:30 +1300, Matthew Daley wrote:> Coverity-ID: 1055048 > Signed-off-by: Matthew Daley <mattjd@gmail.com> > --- > v2: Don''t log ret in error case, log errno insteadacked + applied.
Ian Campbell
2013-Nov-01 13:28 UTC
Re: [PATCH v3] libvchan: tidy up usages of fcntl in select-type sample application
On Fri, 2013-11-01 at 13:33 +1300, Matthew Daley wrote:> Namely, don''t overwrite all the other flags when twiddling O_NONBLOCK, > and add basic error handling. > > Coverity-ID: 1055041 > Signed-off-by: Matthew Daley <mattjd@gmail.com>Acked-by: Ian Campbell <ian.campbell@citrix.com> I''ll give Daniel a chance to comment before applying.> --- > v2: Adjust other fcntl usage further down as well. Use F_GETFL to modify > only the O_NONBLOCK flag. Both suggested by Daniel De Graaf. > Improve commit message. > > v3: Make a new function, set_nonblocking, to do the twiddling > > tools/libvchan/node-select.c | 27 ++++++++++++++++++++++++--- > 1 file changed, 24 insertions(+), 3 deletions(-) > > diff --git a/tools/libvchan/node-select.c b/tools/libvchan/node-select.c > index 6c6c19e..13c5822 100644 > --- a/tools/libvchan/node-select.c > +++ b/tools/libvchan/node-select.c > @@ -80,6 +80,22 @@ void stdout_wr() { > } > } > > +static int set_nonblocking(int fd, int nonblocking) { > + int flags = fcntl(fd, F_GETFL); > + if (flags == -1) > + return -1; > + > + if (nonblocking) > + flags |= O_NONBLOCK; > + else > + flags &= ~O_NONBLOCK; > + > + if (fcntl(fd, F_SETFL, flags) == -1) > + return -1; > + > + return 0; > +} > + > /** > Simple libxenvchan application, both client and server. > Both sides may write and read, both from the libxenvchan and from > @@ -105,8 +121,10 @@ int main(int argc, char **argv) > exit(1); > } > > - fcntl(0, F_SETFL, O_NONBLOCK); > - fcntl(1, F_SETFL, O_NONBLOCK); > + if (set_nonblocking(0, 1) || set_nonblocking(1, 1)) { > + perror("set_nonblocking"); > + exit(1); > + } > > libxenvchan_fd = libxenvchan_fd_for_select(ctrl); > for (;;) { > @@ -153,7 +171,10 @@ int main(int argc, char **argv) > stdout_wr(); > } > if (!libxenvchan_is_open(ctrl)) { > - fcntl(1, F_SETFL, 0); > + if (set_nonblocking(1, 0)) { > + perror("set_nonblocking"); > + exit(1); > + } > while (outsiz) > stdout_wr(); > return 0;
Daniel De Graaf
2013-Nov-01 14:11 UTC
Re: [PATCH v3] libvchan: tidy up usages of fcntl in select-type sample application
On 11/01/2013 09:28 AM, Ian Campbell wrote:> On Fri, 2013-11-01 at 13:33 +1300, Matthew Daley wrote: >> Namely, don''t overwrite all the other flags when twiddling O_NONBLOCK, >> and add basic error handling. >> >> Coverity-ID: 1055041 >> Signed-off-by: Matthew Daley <mattjd@gmail.com> > > Acked-by: Ian Campbell <ian.campbell@citrix.com> > > I''ll give Daniel a chance to comment before applying.Looks good to me. Reviewed-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>> >> --- >> v2: Adjust other fcntl usage further down as well. Use F_GETFL to modify >> only the O_NONBLOCK flag. Both suggested by Daniel De Graaf. >> Improve commit message. >> >> v3: Make a new function, set_nonblocking, to do the twiddling >> >> tools/libvchan/node-select.c | 27 ++++++++++++++++++++++++--- >> 1 file changed, 24 insertions(+), 3 deletions(-) >> >> diff --git a/tools/libvchan/node-select.c b/tools/libvchan/node-select.c >> index 6c6c19e..13c5822 100644 >> --- a/tools/libvchan/node-select.c >> +++ b/tools/libvchan/node-select.c >> @@ -80,6 +80,22 @@ void stdout_wr() { >> } >> } >> >> +static int set_nonblocking(int fd, int nonblocking) { >> + int flags = fcntl(fd, F_GETFL); >> + if (flags == -1) >> + return -1; >> + >> + if (nonblocking) >> + flags |= O_NONBLOCK; >> + else >> + flags &= ~O_NONBLOCK; >> + >> + if (fcntl(fd, F_SETFL, flags) == -1) >> + return -1; >> + >> + return 0; >> +} >> + >> /** >> Simple libxenvchan application, both client and server. >> Both sides may write and read, both from the libxenvchan and from >> @@ -105,8 +121,10 @@ int main(int argc, char **argv) >> exit(1); >> } >> >> - fcntl(0, F_SETFL, O_NONBLOCK); >> - fcntl(1, F_SETFL, O_NONBLOCK); >> + if (set_nonblocking(0, 1) || set_nonblocking(1, 1)) { >> + perror("set_nonblocking"); >> + exit(1); >> + } >> >> libxenvchan_fd = libxenvchan_fd_for_select(ctrl); >> for (;;) { >> @@ -153,7 +171,10 @@ int main(int argc, char **argv) >> stdout_wr(); >> } >> if (!libxenvchan_is_open(ctrl)) { >> - fcntl(1, F_SETFL, 0); >> + if (set_nonblocking(1, 0)) { >> + perror("set_nonblocking"); >> + exit(1); >> + } >> while (outsiz) >> stdout_wr(); >> return 0; > >-- Daniel De Graaf National Security Agency
Ian Jackson
2013-Nov-01 16:34 UTC
Re: [PATCH 09/29] libxc: check for xc_vcpu_setcontext failure in xc_domain_resume_any
Ian Campbell writes ("Re: [PATCH 09/29] libxc: check for xc_vcpu_setcontext
failure in xc_domain_resume_any"):> On Wed, 2013-10-30 at 20:51 +1300, Matthew Daley wrote:
> > Coverity-ID: 1090352
> > Signed-off-by: Matthew Daley <mattjd@gmail.com>
> > ---
> > tools/libxc/xc_resume.c | 6 +++++-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/libxc/xc_resume.c b/tools/libxc/xc_resume.c
> > index 50724f2..a4c0f53 100644
> > --- a/tools/libxc/xc_resume.c
> > +++ b/tools/libxc/xc_resume.c
> > @@ -211,7 +211,11 @@ static int xc_domain_resume_any(xc_interface
*xch, uint32_t domid)
> >
> > /* Reset all secondary CPU states. */
> > for ( i = 1; i <= info.max_vcpu_id; i++ )
> > - xc_vcpu_setcontext(xch, domid, i, NULL);
> > + if ( xc_vcpu_setcontext(xch, domid, i, NULL) != 0 )
> > + {
> > + ERROR("Couldn''t reset vcpu state");
> > + goto out;
>
> The label is inside an x86 specific ifdef and therefore this breaks on
> ARM.
Surely this is a mistake. I mean, the label should be outside the
ifdef. Anything else is too tangled.
Ian.
Matthew Daley
2013-Nov-02 22:57 UTC
[PATCH v3] libxc: check for various libxc failures and pass them down
...in xc_cpuid_{pv,hvm}_policy.
Coverity-ID: 1093050
Signed-off-by: Matthew Daley <mattjd@gmail.com>
---
tools/libxc/xc_cpuid_x86.c | 41 ++++++++++++++++++++++++++++++-----------
1 file changed, 30 insertions(+), 11 deletions(-)
diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
index bbbf9b8..92b9d3e 100644
--- a/tools/libxc/xc_cpuid_x86.c
+++ b/tools/libxc/xc_cpuid_x86.c
@@ -268,28 +268,35 @@ static void xc_cpuid_config_xsave(
}
}
-static void xc_cpuid_hvm_policy(
+static int xc_cpuid_hvm_policy(
xc_interface *xch, domid_t domid,
const unsigned int *input, unsigned int *regs)
{
DECLARE_DOMCTL;
+ int rc;
char brand[13];
unsigned long nestedhvm;
unsigned long pae;
int is_pae, is_nestedhvm;
uint64_t xfeature_mask;
- xc_get_hvm_param(xch, domid, HVM_PARAM_PAE_ENABLED, &pae);
+ rc = xc_get_hvm_param(xch, domid, HVM_PARAM_PAE_ENABLED, &pae);
+ if ( rc )
+ return rc;
is_pae = !!pae;
/* Detecting Xen''s atitude towards XSAVE */
memset(&domctl, 0, sizeof(domctl));
domctl.cmd = XEN_DOMCTL_getvcpuextstate;
domctl.domain = domid;
- do_domctl(xch, &domctl);
+ rc = do_domctl(xch, &domctl);
+ if ( rc )
+ return rc;
xfeature_mask = domctl.u.vcpuextstate.xfeature_mask;
- xc_get_hvm_param(xch, domid, HVM_PARAM_NESTEDHVM, &nestedhvm);
+ rc = xc_get_hvm_param(xch, domid, HVM_PARAM_NESTEDHVM, &nestedhvm);
+ if ( rc )
+ return rc;
is_nestedhvm = !!nestedhvm;
switch ( input[0] )
@@ -429,13 +436,15 @@ static void xc_cpuid_hvm_policy(
else
intel_xc_cpuid_policy(xch, domid, input, regs, is_pae, is_nestedhvm);
+ return rc;
}
-static void xc_cpuid_pv_policy(
+static int xc_cpuid_pv_policy(
xc_interface *xch, domid_t domid,
const unsigned int *input, unsigned int *regs)
{
DECLARE_DOMCTL;
+ int rc;
unsigned int guest_width;
int guest_64bit, xen_64bit = hypervisor_is_64bit(xch);
char brand[13];
@@ -443,7 +452,9 @@ static void xc_cpuid_pv_policy(
xc_cpuid_brand_get(brand);
- xc_domain_get_guest_width(xch, domid, &guest_width);
+ rc = xc_domain_get_guest_width(xch, domid, &guest_width);
+ if ( rc != 0 )
+ return rc;
guest_64bit = (guest_width == 8);
/* Detecting Xen''s atitude towards XSAVE */
@@ -547,23 +558,29 @@ static void xc_cpuid_pv_policy(
regs[0] = regs[1] = regs[2] = regs[3] = 0;
break;
}
+
+ return rc;
}
static int xc_cpuid_policy(
xc_interface *xch, domid_t domid,
const unsigned int *input, unsigned int *regs)
{
+ int rc = 0;
xc_dominfo_t info;
- if ( xc_domain_getinfo(xch, domid, 1, &info) == 0 )
+ rc = xc_domain_getinfo(xch, domid, 1, &info);
+ if ( rc < 0 )
+ return rc;
+ if ( rc == 0 )
return -EINVAL;
if ( info.hvm )
- xc_cpuid_hvm_policy(xch, domid, input, regs);
+ rc = xc_cpuid_hvm_policy(xch, domid, input, regs);
else
- xc_cpuid_pv_policy(xch, domid, input, regs);
+ rc = xc_cpuid_pv_policy(xch, domid, input, regs);
- return 0;
+ return rc;
}
static int xc_cpuid_do_domctl(
@@ -632,7 +649,9 @@ int xc_cpuid_apply_policy(xc_interface *xch, domid_t domid)
for ( ; ; )
{
cpuid(input, regs);
- xc_cpuid_policy(xch, domid, input, regs);
+ rc = xc_cpuid_policy(xch, domid, input, regs);
+ if ( rc )
+ return rc;
if ( regs[0] || regs[1] || regs[2] || regs[3] )
{
--
1.7.10.4
Matthew Daley
2013-Nov-02 23:08 UTC
[PATCH v4] libxc: check for various libxc failures and pass them down
...in xc_cpuid_{pv,hvm}_policy.
Coverity-ID: 1093050
Signed-off-by: Matthew Daley <mattjd@gmail.com>
---
v2: Return actual error from xc_domain_get_guest_width
v3: Return actual error from xc_cpuid_pv_policy as well, and add error
checking to libxc calls in xc_cpuid_hvm_param as well
v4: Use ''rc != 0'' instead of just ''rc'' in if
checks
tools/libxc/xc_cpuid_x86.c | 43 +++++++++++++++++++++++++++++++------------
1 file changed, 31 insertions(+), 12 deletions(-)
diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
index bbbf9b8..8373e73 100644
--- a/tools/libxc/xc_cpuid_x86.c
+++ b/tools/libxc/xc_cpuid_x86.c
@@ -268,28 +268,35 @@ static void xc_cpuid_config_xsave(
}
}
-static void xc_cpuid_hvm_policy(
+static int xc_cpuid_hvm_policy(
xc_interface *xch, domid_t domid,
const unsigned int *input, unsigned int *regs)
{
DECLARE_DOMCTL;
+ int rc;
char brand[13];
unsigned long nestedhvm;
unsigned long pae;
int is_pae, is_nestedhvm;
uint64_t xfeature_mask;
- xc_get_hvm_param(xch, domid, HVM_PARAM_PAE_ENABLED, &pae);
+ rc = xc_get_hvm_param(xch, domid, HVM_PARAM_PAE_ENABLED, &pae);
+ if ( rc != 0 )
+ return rc;
is_pae = !!pae;
/* Detecting Xen''s atitude towards XSAVE */
memset(&domctl, 0, sizeof(domctl));
domctl.cmd = XEN_DOMCTL_getvcpuextstate;
domctl.domain = domid;
- do_domctl(xch, &domctl);
+ rc = do_domctl(xch, &domctl);
+ if ( rc != 0 )
+ return rc;
xfeature_mask = domctl.u.vcpuextstate.xfeature_mask;
- xc_get_hvm_param(xch, domid, HVM_PARAM_NESTEDHVM, &nestedhvm);
+ rc = xc_get_hvm_param(xch, domid, HVM_PARAM_NESTEDHVM, &nestedhvm);
+ if ( rc != 0 )
+ return rc;
is_nestedhvm = !!nestedhvm;
switch ( input[0] )
@@ -429,13 +436,15 @@ static void xc_cpuid_hvm_policy(
else
intel_xc_cpuid_policy(xch, domid, input, regs, is_pae, is_nestedhvm);
+ return rc;
}
-static void xc_cpuid_pv_policy(
+static int xc_cpuid_pv_policy(
xc_interface *xch, domid_t domid,
const unsigned int *input, unsigned int *regs)
{
DECLARE_DOMCTL;
+ int rc;
unsigned int guest_width;
int guest_64bit, xen_64bit = hypervisor_is_64bit(xch);
char brand[13];
@@ -443,7 +452,9 @@ static void xc_cpuid_pv_policy(
xc_cpuid_brand_get(brand);
- xc_domain_get_guest_width(xch, domid, &guest_width);
+ rc = xc_domain_get_guest_width(xch, domid, &guest_width);
+ if ( rc != 0 )
+ return rc;
guest_64bit = (guest_width == 8);
/* Detecting Xen''s atitude towards XSAVE */
@@ -547,23 +558,29 @@ static void xc_cpuid_pv_policy(
regs[0] = regs[1] = regs[2] = regs[3] = 0;
break;
}
+
+ return rc;
}
static int xc_cpuid_policy(
xc_interface *xch, domid_t domid,
const unsigned int *input, unsigned int *regs)
{
+ int rc;
xc_dominfo_t info;
- if ( xc_domain_getinfo(xch, domid, 1, &info) == 0 )
+ rc = xc_domain_getinfo(xch, domid, 1, &info);
+ if ( rc < 0 )
+ return rc;
+ if ( rc == 0 )
return -EINVAL;
if ( info.hvm )
- xc_cpuid_hvm_policy(xch, domid, input, regs);
+ rc = xc_cpuid_hvm_policy(xch, domid, input, regs);
else
- xc_cpuid_pv_policy(xch, domid, input, regs);
+ rc = xc_cpuid_pv_policy(xch, domid, input, regs);
- return 0;
+ return rc;
}
static int xc_cpuid_do_domctl(
@@ -632,12 +649,14 @@ int xc_cpuid_apply_policy(xc_interface *xch, domid_t
domid)
for ( ; ; )
{
cpuid(input, regs);
- xc_cpuid_policy(xch, domid, input, regs);
+ rc = xc_cpuid_policy(xch, domid, input, regs);
+ if ( rc != 0 )
+ return rc;
if ( regs[0] || regs[1] || regs[2] || regs[3] )
{
rc = xc_cpuid_do_domctl(xch, domid, input, regs);
- if ( rc )
+ if ( rc != 0 )
return rc;
}
--
1.7.10.4
Ian Campbell
2013-Nov-04 10:29 UTC
Re: [PATCH 09/29] libxc: check for xc_vcpu_setcontext failure in xc_domain_resume_any
On Fri, 2013-11-01 at 16:34 +0000, Ian Jackson wrote:> Ian Campbell writes ("Re: [PATCH 09/29] libxc: check for xc_vcpu_setcontext failure in xc_domain_resume_any"): > > On Wed, 2013-10-30 at 20:51 +1300, Matthew Daley wrote: > > > Coverity-ID: 1090352 > > > Signed-off-by: Matthew Daley <mattjd@gmail.com> > > > --- > > > tools/libxc/xc_resume.c | 6 +++++- > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > > > diff --git a/tools/libxc/xc_resume.c b/tools/libxc/xc_resume.c > > > index 50724f2..a4c0f53 100644 > > > --- a/tools/libxc/xc_resume.c > > > +++ b/tools/libxc/xc_resume.c > > > @@ -211,7 +211,11 @@ static int xc_domain_resume_any(xc_interface *xch, uint32_t domid) > > > > > > /* Reset all secondary CPU states. */ > > > for ( i = 1; i <= info.max_vcpu_id; i++ ) > > > - xc_vcpu_setcontext(xch, domid, i, NULL); > > > + if ( xc_vcpu_setcontext(xch, domid, i, NULL) != 0 ) > > > + { > > > + ERROR("Couldn''t reset vcpu state"); > > > + goto out; > > > > The label is inside an x86 specific ifdef and therefore this breaks on > > ARM. > > Surely this is a mistake. I mean, the label should be outside the > ifdef. Anything else is too tangled.Yes, which is what Matthew did in v2, which I already applied. Ian.
Ian Campbell
2013-Nov-04 17:31 UTC
Re: [PATCH v4] libxc: check for various libxc failures and pass them down
On Sun, 2013-11-03 at 12:08 +1300, Matthew Daley wrote:> static int xc_cpuid_policy( > xc_interface *xch, domid_t domid, > const unsigned int *input, unsigned int *regs) > { > + int rc; > xc_dominfo_t info; > > - if ( xc_domain_getinfo(xch, domid, 1, &info) == 0 ) > + rc = xc_domain_getinfo(xch, domid, 1, &info); > + if ( rc < 0 ) > + return rc; > + if ( rc == 0 ) > return -EINVAL;I think this means that this function now returns a mixture of -1 (with errno set) and -errno depending on the failure mode. libxc really is a horrible chuffing mess in this regard, sorry. Luckily nothing right now looks at the return value (you are adding those checks to the caller in this patch) so I think you can just convert this into { errno=EINVAL; return -1; } Then again, it looks like xc_cpuid_set (the aforementioned called) already returns both styles (explicit rc = -EINVAL, vs returning the result of do_domctl which is -1 and set errno). Gah! Hah, but no one looks at the specific value of the result of xc_cpuid_set on failure: libxl doesn''t check it at all, Python and ocaml bindings turn it into a generic exception using xc_get_last_error. WTF, xc_get_last_error is a *third* error handling mechanism in libxc, apparently built into the logging infrastructure. Which isn''t used at all on this code path AFAICT. What a terrible mess. I think I should just apply this patch and putthe rug back over the rest of it :-( Ian.
Ian Campbell
2013-Nov-04 17:51 UTC
Re: [PATCH v3] libvchan: tidy up usages of fcntl in select-type sample application
On Fri, 2013-11-01 at 10:11 -0400, Daniel De Graaf wrote:> On 11/01/2013 09:28 AM, Ian Campbell wrote: > > On Fri, 2013-11-01 at 13:33 +1300, Matthew Daley wrote: > >> Namely, don''t overwrite all the other flags when twiddling O_NONBLOCK, > >> and add basic error handling. > >> > >> Coverity-ID: 1055041 > >> Signed-off-by: Matthew Daley <mattjd@gmail.com> > > > > Acked-by: Ian Campbell <ian.campbell@citrix.com> > > > > I''ll give Daniel a chance to comment before applying. > > Looks good to me. > > Reviewed-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>thanks, applied.
Matthew Daley
2013-Nov-05 04:58 UTC
Re: [PATCH v4] libxc: check for various libxc failures and pass them down
On Tue, Nov 5, 2013 at 6:31 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote:> On Sun, 2013-11-03 at 12:08 +1300, Matthew Daley wrote: >> static int xc_cpuid_policy( >> xc_interface *xch, domid_t domid, >> const unsigned int *input, unsigned int *regs) >> { >> + int rc; >> xc_dominfo_t info; >> >> - if ( xc_domain_getinfo(xch, domid, 1, &info) == 0 ) >> + rc = xc_domain_getinfo(xch, domid, 1, &info); >> + if ( rc < 0 ) >> + return rc; >> + if ( rc == 0 ) >> return -EINVAL; > > I think this means that this function now returns a mixture of -1 (with > errno set) and -errno depending on the failure mode. libxc really is a > horrible chuffing mess in this regard, sorry.I noticed :)> > Luckily nothing right now looks at the return value (you are adding > those checks to the caller in this patch) so I think you can just > convert this into { errno=EINVAL; return -1; } > > Then again, it looks like xc_cpuid_set (the aforementioned called) > already returns both styles (explicit rc = -EINVAL, vs returning the > result of do_domctl which is -1 and set errno). Gah! > > Hah, but no one looks at the specific value of the result of > xc_cpuid_set on failure: libxl doesn''t check it at all, Python and ocaml > bindings turn it into a generic exception using xc_get_last_error. > > WTF, xc_get_last_error is a *third* error handling mechanism in libxc, > apparently built into the logging infrastructure. Which isn''t used at > all on this code path AFAICT. > > What a terrible mess. I think I should just apply this patch and putthe > rug back over the rest of it :-(Perhaps this patch should be dropped and the issues put in a "take a proper look at later" pile? - Matthew> > Ian. > >
Ian Campbell
2013-Nov-05 10:10 UTC
Re: [PATCH v4] libxc: check for various libxc failures and pass them down
On Tue, 2013-11-05 at 17:58 +1300, Matthew Daley wrote:> On Tue, Nov 5, 2013 at 6:31 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote: > > On Sun, 2013-11-03 at 12:08 +1300, Matthew Daley wrote: > >> static int xc_cpuid_policy( > >> xc_interface *xch, domid_t domid, > >> const unsigned int *input, unsigned int *regs) > >> { > >> + int rc; > >> xc_dominfo_t info; > >> > >> - if ( xc_domain_getinfo(xch, domid, 1, &info) == 0 ) > >> + rc = xc_domain_getinfo(xch, domid, 1, &info); > >> + if ( rc < 0 ) > >> + return rc; > >> + if ( rc == 0 ) > >> return -EINVAL; > > > > I think this means that this function now returns a mixture of -1 (with > > errno set) and -errno depending on the failure mode. libxc really is a > > horrible chuffing mess in this regard, sorry. > > I noticed :) > > > > > Luckily nothing right now looks at the return value (you are adding > > those checks to the caller in this patch) so I think you can just > > convert this into { errno=EINVAL; return -1; } > > > > Then again, it looks like xc_cpuid_set (the aforementioned called) > > already returns both styles (explicit rc = -EINVAL, vs returning the > > result of do_domctl which is -1 and set errno). Gah! > > > > Hah, but no one looks at the specific value of the result of > > xc_cpuid_set on failure: libxl doesn''t check it at all, Python and ocaml > > bindings turn it into a generic exception using xc_get_last_error. > > > > WTF, xc_get_last_error is a *third* error handling mechanism in libxc, > > apparently built into the logging infrastructure. Which isn''t used at > > all on this code path AFAICT. > > > > What a terrible mess. I think I should just apply this patch and putthe > > rug back over the rest of it :-( > > Perhaps this patch should be dropped and the issues put in a "take a > proper look at later" pile?I''m ok with that. Sorry it took me until v4 to realise what a rats nest it would be to fix. Ian.