Fixes for various minor Coverity issues, volume 3

Fixes for various minor Coverity issues, volume 3

[Matthew Daley]

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.

[PATCH 01/29] libxc: check for xc_domain_get_guest_width failure and pass it down

[Matthew Daley]

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

[PATCH 02/29] libxc: fix retrieval of and remove pointless check on gzip size

[Matthew Daley]

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

[PATCH 03/29] libxc: fix memory leak in move_l3_below_4G error handling

[Matthew Daley]

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

[PATCH 04/29] libxc: don't ignore fd read failures when restoring a domain

[Matthew Daley]

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

[PATCH 05/29] libxc: tidy up loop construct in write_compressed

[Matthew Daley]

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

[PATCH 06/29] libxc: don't read uninitialized size value in xc_read_image

[Matthew Daley]

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

[PATCH 07/29] libxc: remove pointless null pointer check in xc_interface_open_common

[Matthew Daley]

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

[PATCH 08/29] libxc: check for xc_domain_get_guest_width failure in xc_domain_resume_any

[Matthew Daley]

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

[PATCH 09/29] libxc: check for xc_vcpu_setcontext failure in xc_domain_resume_any

[Matthew Daley]

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

[PATCH 10/29] libxc: initalize stdio loggers' progress_last_percent values to 0

[Matthew Daley]

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

[PATCH 11/29] libxc: remove null pointer checks in xc_pm_get_{c, p}xstat

[Matthew Daley]

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

[PATCH 12/29] libxl: check for transaction abortion failure in libxl_set_memory_target

[Matthew Daley]

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, &current_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

[PATCH 13/29] libxl: check for xc_domain_max_vcpus failure in libxl__build_pre

[Matthew Daley]

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

[PATCH 14/29] libxl: don't leak xs_read results in libxl__device_nic_from_xs_be

[Matthew Daley]

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

[PATCH 15/29] libxl: correct file open success check in libxl__device_pci_reset

[Matthew Daley]

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

[PATCH 16/29] libxl: check for xc_domain_shutdown failure in libxl__domain_suspend_common_callback

[Matthew Daley]

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

[PATCH 17/29] libxl: don't leak memory in libxl__poller_get failure case

[Matthew Daley]

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

[PATCH 18/29] libxl: only close fds which successfully opened in libxl__spawn_local_dm

[Matthew Daley]

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

[PATCH 19/29] xl: libxl_list_vm returns a pointer, not a handle

[Matthew Daley]

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

[PATCH 20/29] xl: check for restore file open failure in create_domain

[Matthew Daley]

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

[PATCH 21/29] xl: remove needless infinite-loop construct in create_domain

[Matthew Daley]

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

[PATCH 22/29] xl: correct references to long-removed function in error messages

[Matthew Daley]

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

[PATCH 23/29] docs: make `xl vcpu-list` example use verbatim formatting

[Matthew Daley]

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

[PATCH 24/29] libvchan: handle libxc evtchn failures properly in init functions

[Matthew Daley]

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

[PATCH 25/29] libvchan: check for fcntl failures in select-type sample application

[Matthew Daley]

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

[PATCH 26/29] xenctx: only alloc symbol if we are inserting it into the symbol table

[Matthew Daley]

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

[PATCH 27/29] xenctx: correct error check when opening libxc

[Matthew Daley]

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

[PATCH 28/29] xentrace: don't try to close null libxc handle in disable_tbufs

[Matthew Daley]

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

[PATCH 29/29] xentrace: undeadify invalid option argument handling

[Matthew Daley]

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

Re: [PATCH 14/29] libxl: don't leak xs_read results in libxl__device_nic_from_xs_be

[Roger Pau Monné]

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.

Re: [PATCH 26/29] xenctx: only alloc symbol if we are inserting it into the symbol table

[Jan Beulich]

>>> 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 

Re: [PATCH 14/29] libxl: don't leak xs_read results in libxl__device_nic_from_xs_be

[Matthew Daley]

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

Re: [PATCH 24/29] libvchan: handle libxc evtchn failures properly in init functions

[Matthew Daley]

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
>

Re: [PATCH 02/29] libxc: fix retrieval of and remove pointless check on gzip size

[Andrew Cooper]

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,

Re: [PATCH 23/29] docs: make `xl vcpu-list` example use verbatim formatting

[Matthew Daley]

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.

Re: [PATCH 29/29] xentrace: undeadify invalid option argument handling

[George Dunlap]

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

Re: [PATCH 28/29] xentrace: don't try to close null libxc handle in disable_tbufs

[George Dunlap]

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);
>   }
>   

Re: [PATCH 27/29] xenctx: correct error check when opening libxc

[George Dunlap]

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);
>       }

Re: [PATCH 24/29] libvchan: handle libxc evtchn failures properly in init functions

[Daniel De Graaf]

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.

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

Re: [PATCH 25/29] libvchan: check for fcntl failures in select-type sample application

[Daniel De Graaf]

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

Re: [PATCH 25/29] libvchan: check for fcntl failures in select-type sample application

[Andrew Cooper]

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

Re: [PATCH 25/29] libvchan: check for fcntl failures in select-type sample application

[Matthew Daley]

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

Re: [PATCH 24/29] libvchan: handle libxc evtchn failures properly in init functions

[Matthew Daley]

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>
>
>

Re: [PATCH 02/29] libxc: fix retrieval of and remove pointless check on gzip size

[Matthew Daley]

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,
>

Re: [PATCH 24/29] libvchan: handle libxc evtchn failures properly in init functions

[Matthew Daley]

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>
>>
>>

[PATCH v2] libxc: fix retrieval of and remove pointless check on gzip size

[Matthew Daley]

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

[PATCH v2] docs: make `xl vm-list` example use verbatim formatting

[Matthew Daley]

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

[PATCH v2] libvchan: handle libxc evtchn failures properly in init functions

[Matthew Daley]

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

[PATCH v2] libvchan: tidy up usages of fcntl in select-type sample application

[Matthew Daley]

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

Re: [PATCH v2] libxc: fix retrieval of and remove pointless check on gzip size

[Andrew Cooper]

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,

Re: Fixes for various minor Coverity issues, volume 3

[Andrew Cooper]

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

Re: [PATCH 01/29] libxc: check for xc_domain_get_guest_width failure and pass it down

[Ian Campbell]

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?

Re: [PATCH 06/29] libxc: don't read uninitialized size value in xc_read_image

[Ian Campbell]

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. 

> 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);

Re: [PATCH 13/29] libxl: check for xc_domain_max_vcpus failure in libxl__build_pre

[Ian Campbell]

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

Re: [PATCH 14/29] libxl: don't leak xs_read results in libxl__device_nic_from_xs_be

[Ian Campbell]

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.

Re: [PATCH 16/29] libxl: check for xc_domain_shutdown failure in libxl__domain_suspend_common_callback

[Ian Campbell]

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 {

Re: [PATCH v2] libvchan: tidy up usages of fcntl in select-type sample application

[Ian Campbell]

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;

Re: Fixes for various minor Coverity issues, volume 3

[Matthew Daley]

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

Re: [PATCH 09/29] libxc: check for xc_vcpu_setcontext failure in xc_domain_resume_any

[Ian Campbell]

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;

Re: [PATCH v2] libxc: fix retrieval of and remove pointless check on gzip size

[Ian Campbell]

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,
> 

Re: Fixes for various minor Coverity issues, volume 3

[Ian Campbell]

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

Re: [PATCH 06/29] libxc: don't read uninitialized size value in xc_read_image

[Matthew Daley]

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);
>
>

Re: [PATCH 06/29] libxc: don't read uninitialized size value in xc_read_image

[Ian Campbell]

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.

[PATCH v2] libxc: check for xc_domain_get_guest_width failure and pass it down

[Matthew Daley]

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

[PATCH v2] libxc: check for xc_vcpu_setcontext failure in xc_domain_resume_any

[Matthew Daley]

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

[PATCH v2] libxl: check for xc_domain_max_vcpus failure in libxl__build_pre

[Matthew Daley]

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

[PATCH v2] libxl: check for xc_domain_shutdown failure in libxl__domain_suspend_common_callback

[Matthew Daley]

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

[PATCH v3] libvchan: tidy up usages of fcntl in select-type sample application

[Matthew Daley]

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

Re: Fixes for various minor Coverity issues, volume 3

[Matthew Daley]

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
>
>

Re: [PATCH v2] libxc: check for xc_domain_get_guest_width failure and pass it down

[Ian Campbell]

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.

Re: [PATCH v2] libxc: check for xc_vcpu_setcontext failure in xc_domain_resume_any

[Ian Campbell]

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 region

thanks, akced+ applied.

Re: [PATCH v2] libxl: check for xc_domain_max_vcpus failure in libxl__build_pre

[Ian Campbell]

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.

Re: [PATCH v2] libxl: check for xc_domain_shutdown failure in libxl__domain_suspend_common_callback

[Ian Campbell]

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 instead

acked + applied.

Re: [PATCH v3] libvchan: tidy up usages of fcntl in select-type sample application

[Ian Campbell]

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;

Re: [PATCH v3] libvchan: tidy up usages of fcntl in select-type sample application

[Daniel De Graaf]

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

Re: [PATCH 09/29] libxc: check for xc_vcpu_setcontext failure in xc_domain_resume_any

[Ian Jackson]

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.

[PATCH v3] libxc: check for various libxc failures and pass them down

[Matthew Daley]

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

[PATCH v4] libxc: check for various libxc failures and pass them down

[Matthew Daley]

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

Re: [PATCH 09/29] libxc: check for xc_vcpu_setcontext failure in xc_domain_resume_any

[Ian Campbell]

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.

Re: [PATCH v4] libxc: check for various libxc failures and pass them down

[Ian Campbell]

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.

Re: [PATCH v3] libvchan: tidy up usages of fcntl in select-type sample application

[Ian Campbell]

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.

Re: [PATCH v4] libxc: check for various libxc failures and pass them down

[Matthew Daley]

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

Re: [PATCH v4] libxc: check for various libxc failures and pass them down

[Ian Campbell]

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.