[PATCH 0/4] tools/libxc: various errno changes

[PATCH 0/4] tools/libxc: various errno changes

[Olaf Hering]

Some changes related to pass errno to callers of xc_domain_save.
Compile and runtime tested with my migration constraint changes.

Olaf Hering (4):
  tools/libxc: remove double inclusion of errno.h
  tools/libxc: assign positive values to errno
  tools/xc: preserve errno in ERROR and DRINTF macros
  tools/xc: pass errno to callers of xc_domain_save

 tools/libxc/xc_compression.c    |  1 -
 tools/libxc/xc_domain_save.c    | 33 ++++++++++++++++++++++-----------
 tools/libxc/xc_flask.c          |  1 -
 tools/libxc/xc_foreign_memory.c |  2 +-
 tools/libxc/xc_gnttab.c         |  1 -
 tools/libxc/xc_mem_paging.c     |  2 +-
 tools/libxc/xc_minios.c         |  3 +--
 tools/libxc/xc_pm.c             |  1 -
 tools/libxc/xc_private.h        | 29 ++++++++++++++++++++++-------
 tools/misc/xen-hptool.c         |  1 -
 tools/misc/xen-mfndump.c        |  1 -
 11 files changed, 47 insertions(+), 28 deletions(-)

[PATCH 1/4] tools/libxc: remove double inclusion of errno.h

[Olaf Hering]

xc_private.h includes errno.h already

Signed-off-by: Olaf Hering <olaf@aepfle.de>
---
 tools/libxc/xc_compression.c | 1 -
 tools/libxc/xc_flask.c       | 1 -
 tools/libxc/xc_gnttab.c      | 1 -
 tools/libxc/xc_minios.c      | 1 -
 tools/libxc/xc_pm.c          | 1 -
 tools/misc/xen-hptool.c      | 1 -
 tools/misc/xen-mfndump.c     | 1 -
 7 files changed, 7 deletions(-)

diff --git a/tools/libxc/xc_compression.c b/tools/libxc/xc_compression.c
index 8f0b89d..d42e651 100644
--- a/tools/libxc/xc_compression.c
+++ b/tools/libxc/xc_compression.c
@@ -32,7 +32,6 @@
 #include <unistd.h>
 #include <sys/types.h>
 #include <inttypes.h>
-#include <errno.h>
 #include "xc_private.h"
 #include "xenctrl.h"
 #include "xg_save_restore.h"
diff --git a/tools/libxc/xc_flask.c b/tools/libxc/xc_flask.c
index 4294a25..255e4b0 100644
--- a/tools/libxc/xc_flask.c
+++ b/tools/libxc/xc_flask.c
@@ -19,7 +19,6 @@
 #include "xc_private.h"
 #include <unistd.h>
 #include <stdio.h>
-#include <errno.h>
 #include <fcntl.h>
 #include <string.h>
 #include <sys/mman.h>
diff --git a/tools/libxc/xc_gnttab.c b/tools/libxc/xc_gnttab.c
index 79dab40..3b6058c 100644
--- a/tools/libxc/xc_gnttab.c
+++ b/tools/libxc/xc_gnttab.c
@@ -18,7 +18,6 @@
  */
 
 #include "xc_private.h"
-#include <errno.h>
 
 int xc_gnttab_op(xc_interface *xch, int cmd, void * op, int op_size, int count)
 {
diff --git a/tools/libxc/xc_minios.c b/tools/libxc/xc_minios.c
index dec4d73..a48f0de 100644
--- a/tools/libxc/xc_minios.c
+++ b/tools/libxc/xc_minios.c
@@ -29,7 +29,6 @@
 #include <mini-os/events.h>
 #include <mini-os/wait.h>
 #include <sys/mman.h>
-#include <errno.h>
 
 #include <xen/memory.h>
 #include <unistd.h>
diff --git a/tools/libxc/xc_pm.c b/tools/libxc/xc_pm.c
index 0da2d89..f7ac40c 100644
--- a/tools/libxc/xc_pm.c
+++ b/tools/libxc/xc_pm.c
@@ -19,7 +19,6 @@
  *
  */
 
-#include <errno.h>
 #include <stdbool.h>
 #include "xc_private.h"
 
diff --git a/tools/misc/xen-hptool.c b/tools/misc/xen-hptool.c
index 24c3e95..f8570f2 100644
--- a/tools/misc/xen-hptool.c
+++ b/tools/misc/xen-hptool.c
@@ -1,7 +1,6 @@
 #include <xenctrl.h>
 #include <xc_private.h>
 #include <xc_core.h>
-#include <errno.h>
 #include <unistd.h>
 
 #define ARRAY_SIZE(a) (sizeof (a) / sizeof ((a)[0]))
diff --git a/tools/misc/xen-mfndump.c b/tools/misc/xen-mfndump.c
index dea9fa9..8064527 100644
--- a/tools/misc/xen-mfndump.c
+++ b/tools/misc/xen-mfndump.c
@@ -1,7 +1,6 @@
 #include <xenctrl.h>
 #include <xc_private.h>
 #include <xc_core.h>
-#include <errno.h>
 #include <unistd.h>
 #include <inttypes.h>

Re: [PATCH 1/4] tools/libxc: remove double inclusion of errno.h

[Ian Jackson]

Olaf Hering writes ("[PATCH 1/4] tools/libxc: remove double inclusion of errno.h"):
> xc_private.h includes errno.h already

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>

[PATCH 2/4] tools/libxc: assign positive values to errno

[Olaf Hering]

Signed-off-by: Olaf Hering <olaf@aepfle.de>
---
 tools/libxc/xc_foreign_memory.c | 2 +-
 tools/libxc/xc_mem_paging.c     | 2 +-
 tools/libxc/xc_minios.c         | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/libxc/xc_foreign_memory.c b/tools/libxc/xc_foreign_memory.c
index 7dfc817..43abf01 100644
--- a/tools/libxc/xc_foreign_memory.c
+++ b/tools/libxc/xc_foreign_memory.c
@@ -27,7 +27,7 @@ void *xc_map_foreign_pages(xc_interface *xch, uint32_t dom, int prot,
     int i, *err;
 
     if (num < 0) {
-        errno = -EINVAL;
+        errno = EINVAL;
         return NULL;
     }
 
diff --git a/tools/libxc/xc_mem_paging.c b/tools/libxc/xc_mem_paging.c
index 269cbdd..8aa7d4d 100644
--- a/tools/libxc/xc_mem_paging.c
+++ b/tools/libxc/xc_mem_paging.c
@@ -76,7 +76,7 @@ int xc_mem_paging_load(xc_interface *xch, domid_t domain_id,
 {
     int rc, old_errno;
 
-    errno = -EINVAL;
+    errno = EINVAL;
 
     if ( !buffer )
         return -1;
diff --git a/tools/libxc/xc_minios.c b/tools/libxc/xc_minios.c
index a48f0de..e621417 100644
--- a/tools/libxc/xc_minios.c
+++ b/tools/libxc/xc_minios.c
@@ -349,7 +349,7 @@ static int minios_evtchn_unbind(xc_evtchn *xce, xc_osdep_handle h, evtchn_port_t
         }
     }
     printf("Warning: couldn't find port %"PRId32" for xc handle %x\n", port, fd);
-    errno = -EINVAL;
+    errno = EINVAL;
     return -1;
 }

Re: [PATCH 2/4] tools/libxc: assign positive values to errno

[Ian Jackson]

Olaf Hering writes ("[PATCH 2/4] tools/libxc: assign positive values to errno"):
> Signed-off-by: Olaf Hering <olaf@aepfle.de>

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>

Re: [PATCH 2/4] tools/libxc: assign positive values to errno

[Ian Campbell]

On Mon, 2014-03-10 at 16:51 +0000, Ian Jackson wrote:
> Olaf Hering writes ("[PATCH 2/4] tools/libxc: assign positive values to errno"):
> > Signed-off-by: Olaf Hering <olaf@aepfle.de>
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>

Are you still planning to do a more comprehensive sweep of libxc's error
reporting for 4.5?

Ian.

Re: [PATCH 2/4] tools/libxc: assign positive values to errno

[Olaf Hering]

On Tue, Mar 11, Ian Campbell wrote:

> On Mon, 2014-03-10 at 16:51 +0000, Ian Jackson wrote:
> > Olaf Hering writes ("[PATCH 2/4] tools/libxc: assign positive values to errno"):
> > > Signed-off-by: Olaf Hering <olaf@aepfle.de>
> > 
> > Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> > Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> Are you still planning to do a more comprehensive sweep of libxc's error
> reporting for 4.5?

I will at least update suspend/resume code paths while working on the
migration constrains patch. While doing that I could also poke around in
other areas.

Olaf

Re: [PATCH 2/4] tools/libxc: assign positive values to errno

[Ian Campbell]

On Tue, 2014-03-11 at 15:35 +0100, Olaf Hering wrote:
> On Tue, Mar 11, Ian Campbell wrote:
> 
> > On Mon, 2014-03-10 at 16:51 +0000, Ian Jackson wrote:
> > > Olaf Hering writes ("[PATCH 2/4] tools/libxc: assign positive values to errno"):
> > > > Signed-off-by: Olaf Hering <olaf@aepfle.de>
> > > 
> > > Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> > > Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>
> > 
> > Are you still planning to do a more comprehensive sweep of libxc's error
> > reporting for 4.5?
> 
> I will at least update suspend/resume code paths while working on the
> migration constrains patch. While doing that I could also poke around in
> other areas.

I was actually talking to Ian J, but if you are doing some then tHat's
great too!

Thanks,
Ian.

Re: [PATCH 2/4] tools/libxc: assign positive values to errno

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH 2/4] tools/libxc: assign positive values to errno"):
> Are you still planning to do a more comprehensive sweep of libxc's error
> reporting for 4.5?

Ideally, yes.

Ian.

[PATCH 3/4] tools/xc: preserve errno in ERROR and DRINTF macros

[Olaf Hering]

This simplifies a changes made in a follow-up patch.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
---
 tools/libxc/xc_private.h | 29 ++++++++++++++++++++++-------
 1 file changed, 22 insertions(+), 7 deletions(-)

diff --git a/tools/libxc/xc_private.h b/tools/libxc/xc_private.h
index a610f0c..b3e0130 100644
--- a/tools/libxc/xc_private.h
+++ b/tools/libxc/xc_private.h
@@ -119,13 +119,28 @@ void xc_report_progress_step(xc_interface *xch,
 
 /* anamorphic macros:  struct xc_interface *xch  must be in scope */
 
-#define IPRINTF(_f, _a...) xc_report(xch, xch->error_handler, XTL_INFO,0, _f , ## _a)
-#define DPRINTF(_f, _a...) xc_report(xch, xch->error_handler, XTL_DETAIL,0, _f , ## _a)
-#define DBGPRINTF(_f, _a...) xc_report(xch, xch->error_handler, XTL_DEBUG,0, _f , ## _a)
-
-#define ERROR(_m, _a...)  xc_report_error(xch,XC_INTERNAL_ERROR,_m , ## _a )
-#define PERROR(_m, _a...) xc_report_error(xch,XC_INTERNAL_ERROR,_m \
-                  " (%d = %s)", ## _a , errno, xc_strerror(xch, errno))
+#define IPRINTF(_f, _a...)  do { int __errno = errno; \
+        xc_report(xch, xch->error_handler, XTL_INFO,0, _f , ## _a); \
+        errno = __errno; \
+        } while (0)
+#define DPRINTF(_f, _a...) do { int __errno = errno; \
+        xc_report(xch, xch->error_handler, XTL_DETAIL,0, _f , ## _a); \
+        errno = __errno; \
+        } while (0)
+#define DBGPRINTF(_f, _a...)  do { int __errno = errno; \
+        xc_report(xch, xch->error_handler, XTL_DEBUG,0, _f , ## _a); \
+        errno = __errno; \
+        } while (0)
+
+#define ERROR(_m, _a...)  do { int __errno = errno; \
+        xc_report_error(xch,XC_INTERNAL_ERROR,_m , ## _a ); \
+        errno = __errno; \
+        } while (0)
+#define PERROR(_m, _a...) do { int __errno = errno; \
+        xc_report_error(xch,XC_INTERNAL_ERROR,_m " (%d = %s)", \
+        ## _a , errno, xc_strerror(xch, errno)); \
+        errno = __errno; \
+        } while (0)
 
 /*
  * HYPERCALL ARGUMENT BUFFERS

Re: [PATCH 3/4] tools/xc: preserve errno in ERROR and DRINTF macros

[Ian Jackson]

Olaf Hering writes ("[PATCH 3/4] tools/xc: preserve errno in ERROR and DRINTF macros"):
> This simplifies a changes made in a follow-up patch.

Thanks.  See my comments in my other email...

Ian.

[PATCH 4/4 v2] tools/xc: pass errno to callers of xc_domain_save

[Olaf Hering]

Callers of xc_domain_save use errno to print diagnostics if the call
fails. But xc_domain_save does not preserve the actual errno in case of
a failure.

This change preserves errno in all cases where code jumps to the label
"out". In addition a new label "exit" is added to catch also code which
used to do just "return 1".

Now libxl_save_helper:complete can print the actual error string.

v2:
- preserve errno during inital jump to label "out"
- use errno as success indicator

Signed-off-by: Olaf Hering <olaf@aepfle.de>
---
 tools/libxc/xc_domain_save.c | 33 ++++++++++++++++++++++-----------
 1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/tools/libxc/xc_domain_save.c b/tools/libxc/xc_domain_save.c
index 42c4752..3aef0d2 100644
--- a/tools/libxc/xc_domain_save.c
+++ b/tools/libxc/xc_domain_save.c
@@ -150,6 +150,7 @@ static inline int outbuf_write(xc_interface *xch,
                                struct outbuf* ob, void* buf, size_t len)
 {
     if ( len > ob->size - ob->pos ) {
+        errno = ERANGE;
         DBGPRINTF("outbuf_write: %zu > %zu@%zu\n", len, ob->size - ob->pos, ob->pos);
         return -1;
     }
@@ -806,7 +807,7 @@ int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom, uint32_t max_iter
     xc_dominfo_t info;
     DECLARE_DOMCTL;
 
-    int rc = 1, frc, i, j, last_iter = 0, iter = 0;
+    int rc, frc, i, j, last_iter = 0, iter = 0;
     int live  = (flags & XCFLAGS_LIVE);
     int debug = (flags & XCFLAGS_DEBUG);
     int superpages = !!hvm;
@@ -899,7 +900,7 @@ int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom, uint32_t max_iter
     {
         ERROR("No switch_qemu_logdirty callback provided.");
         errno = EINVAL;
-        return 1;
+        goto exit;
     }
 
     outbuf_init(xch, &ob_pagebuf, OUTBUF_SIZE);
@@ -914,13 +915,13 @@ int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom, uint32_t max_iter
                             &ctx->max_mfn, &ctx->hvirt_start, &ctx->pt_levels, &dinfo->guest_width) )
     {
         ERROR("Unable to get platform info.");
-        return 1;
+        goto exit;
     }
 
     if ( xc_domain_getinfo(xch, dom, 1, &info) != 1 )
     {
         PERROR("Could not get domain info");
-        return 1;
+        goto exit;
     }
 
     shared_info_frame = info.shared_info_frame;
@@ -942,6 +943,7 @@ int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom, uint32_t max_iter
 
     if ( dinfo->p2m_size > ~XEN_DOMCTL_PFINFO_LTAB_MASK )
     {
+        errno = E2BIG;
         ERROR("Cannot save this big a guest");
         goto out;
     }
@@ -1012,6 +1014,7 @@ int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom, uint32_t max_iter
 
     if ( !to_send || !to_fix || !to_skip )
     {
+        errno = ENOMEM;
         ERROR("Couldn't allocate to_send array");
         goto out;
     }
@@ -1030,6 +1033,7 @@ int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom, uint32_t max_iter
         hvm_buf = malloc(hvm_buf_size);
         if ( !hvm_buf )
         {
+            errno = ENOMEM;
             ERROR("Couldn't allocate memory");
             goto out;
         }
@@ -1598,6 +1602,7 @@ int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom, uint32_t max_iter
 
         if ( info.max_vcpu_id >= XC_SR_MAX_VCPUS )
         {
+            errno = E2BIG;
             ERROR("Too many VCPUS in guest!");
             goto out;
         }
@@ -1830,7 +1835,7 @@ int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom, uint32_t max_iter
         }
         
         /* HVM guests are done now */
-        rc = 0;
+        errno = 0;
         goto out;
     }
 
@@ -1888,6 +1893,7 @@ int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom, uint32_t max_iter
     mfn = GET_FIELD(&ctxt, user_regs.edx);
     if ( !MFN_IS_IN_PSEUDOPHYS_MAP(mfn) )
     {
+        errno = ERANGE;
         ERROR("Suspend record is not in range of pseudophys map");
         goto out;
     }
@@ -1910,6 +1916,7 @@ int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom, uint32_t max_iter
             mfn = GET_FIELD(&ctxt, gdt_frames[j]);
             if ( !MFN_IS_IN_PSEUDOPHYS_MAP(mfn) )
             {
+                errno = ERANGE;
                 ERROR("GDT frame is not in range of pseudophys map");
                 goto out;
             }
@@ -1920,6 +1927,7 @@ int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom, uint32_t max_iter
         if ( !MFN_IS_IN_PSEUDOPHYS_MAP(UNFOLD_CR3(
                                            GET_FIELD(&ctxt, ctrlreg[3]))) )
         {
+            errno = ERANGE;
             ERROR("PT base is not in range of pseudophys map");
             goto out;
         }
@@ -1931,6 +1939,7 @@ int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom, uint32_t max_iter
         {
             if ( !MFN_IS_IN_PSEUDOPHYS_MAP(UNFOLD_CR3(ctxt.x64.ctrlreg[1])) )
             {
+                errno = ERANGE;
                 ERROR("PT base is not in range of pseudophys map");
                 goto out;
             }
@@ -2027,9 +2036,10 @@ int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom, uint32_t max_iter
     }
 
     /* Success! */
-    rc = 0;
+    errno = 0;
 
  out:
+    rc = errno;
     completed = 1;
 
     if ( !rc && callbacks->postcopy )
@@ -2044,13 +2054,11 @@ int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom, uint32_t max_iter
         if (wrcompressed(io_fd) < 0)
         {
             ERROR("Error when writing compressed data, after postcopy\n");
-            rc = 1;
             goto out;
         }
         /* Append the tailbuf data to the main outbuf */
         if ( wrexact(io_fd, ob_tailbuf.buf, ob_tailbuf.pos) )
         {
-            rc = 1;
             PERROR("Error when copying tailbuf into outbuf");
             goto out;
         }
@@ -2059,7 +2067,8 @@ int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom, uint32_t max_iter
     /* Flush last write and discard cache for file. */
     if ( ob && outbuf_flush(xch, ob, io_fd) < 0 ) {
         PERROR("Error when flushing output buffer");
-        rc = 1;
+        if (!rc)
+            rc = errno;
     }
 
     discard_file_cache(xch, io_fd, 1 /* flush */);
@@ -2130,9 +2139,11 @@ int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom, uint32_t max_iter
     free(hvm_buf);
     outbuf_free(&ob_pagebuf);
 
-    DPRINTF("Save exit of domid %u with rc=%d\n", dom, rc);
+    errno = rc;
+exit:
+    DPRINTF("Save exit of domid %u with errno=%d\n", dom, errno);
 
-    return !!rc;
+    return !!errno;
 }
 
 /*

Re: [PATCH 4/4 v2] tools/xc: pass errno to callers of xc_domain_save

[Ian Jackson]

Olaf Hering writes ("[PATCH 4/4 v2] tools/xc: pass errno to callers of xc_domain_save"):
> @@ -914,13 +915,13 @@ int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom, uint32_t max_iter
>                              &ctx->max_mfn, &ctx->hvirt_start, &ctx->pt_levels, &dinfo->guest_width) )
>      {
>          ERROR("Unable to get platform info.");
> -        return 1;
> +        goto exit;

This all looks pretty good, thanks.

Can I suggest that these cases could probably benefit from
    assert(errno)
?

That way there is no risk of thinking we have succeeded because some
subfunction with poor error handling has failed but not set errno.

One option for doing centrally would be:

>      /* Success! */
- -    rc = 0;
- +    errno = 0;
  +    goto out_rc;


>   out:
> +    rc = errno;
  +    assert(rc);
  + out_rc:
>      completed = 1;

Thanks,
Ian.

Re: [PATCH 4/4 v2] tools/xc: pass errno to callers of xc_domain_save

[Olaf Hering]

On Mon, Mar 10, Ian Jackson wrote:

> Can I suggest that these cases could probably benefit from
>     assert(errno)

I think that verifying errno in that case is a good thing. But:
Wouldnt that abort() applications using libxc, including libvirtd?

Olaf

Re: [PATCH 4/4 v2] tools/xc: pass errno to callers of xc_domain_save

[Olaf Hering]

On Mon, Mar 10, Olaf Hering wrote:

> On Mon, Mar 10, Ian Jackson wrote:
> 
> > Can I suggest that these cases could probably benefit from
> >     assert(errno)
> 
> I think that verifying errno in that case is a good thing. But:
> Wouldnt that abort() applications using libxc, including libvirtd?

I see that libxl contains many assert(), so one more would probably not
hurt.

Olaf