[PATCH v2 0/8] xen-livepatch misc fixes/changes

[PATCH v2 0/8] xen-livepatch misc fixes/changes

[Ross Lagerwall]

Hi all,

This series contains a few fixes to the xen-livepatch tool.
It also contains a few changes to make the output more readable.

Changed in v2:
* Fix minor comments.
* Split the last patch into two.

Ross Lagerwall (8):
  tools/livepatch: Show the correct expected state before action
  tools/livepatch: Set stdout and stderr unbuffered
  tools/livepatch: Improve output
  livepatch: Fix documentation of timeout
  tools/livepatch: Remove pointless retry loop
  tools/livepatch: Remove unused struct member
  tools/livepatch: Save errno where needed
  tools/livepatch: Exit with 2 if a timeout occurs

 docs/misc/livepatch.markdown  |  13 +--
 tools/libxc/include/xenctrl.h |   2 +-
 tools/misc/xen-livepatch.c    | 218 ++++++++++++++++++++++++++----------------
 xen/common/livepatch.c        |   4 +-
 xen/include/public/sysctl.h   |   5 +-
 5 files changed, 151 insertions(+), 91 deletions(-)

-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 1/8] tools/livepatch: Show the correct expected state before action

[Ross Lagerwall]

Somewhat confusingly, before the action has been executed the patch is
expected to be in the "allow" state, not the "expected" state.  The
check for this was correct but the subsequent error message was not.
Fix the error message to show this state correctly.

Before:
    $ xen-livepatch unload test
    test: in wrong state (APPLIED), expected (unknown)
After:
    $ xen-livepatch unload test
    test: in wrong state (APPLIED), expected (CHECKED)

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/misc/xen-livepatch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/misc/xen-livepatch.c b/tools/misc/xen-livepatch.c
index 2de04c0..f6c7c8a 100644
--- a/tools/misc/xen-livepatch.c
+++ b/tools/misc/xen-livepatch.c
@@ -308,7 +308,7 @@ int action_func(int argc, char *argv[], unsigned int idx)
     {
         printf("%s: in wrong state (%s), expected (%s)\n",
                name, state2str(status.state),
-               state2str(action_options[idx].expected));
+               state2str(action_options[idx].allow));
         return -1;
     }
 
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 2/8] tools/livepatch: Set stdout and stderr unbuffered

[Ross Lagerwall]

Using both stdout and stderr interleaved without newlines can result in
strange output when using line buffered mode (e.g. a terminal) or when
fully buffered (e.g. redirected to a file). Set stdout to unbuffered mode
to fix this (stderr is always unbuffered by default).

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
 tools/misc/xen-livepatch.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tools/misc/xen-livepatch.c b/tools/misc/xen-livepatch.c
index f6c7c8a..56ae5aa 100644
--- a/tools/misc/xen-livepatch.c
+++ b/tools/misc/xen-livepatch.c
@@ -330,7 +330,6 @@ int action_func(int argc, char *argv[], unsigned int idx)
         }
 
         printf(".");
-        fflush(stdout);
         usleep(DELAY);
     } while ( ++retry < RETRIES );
 
@@ -416,6 +415,12 @@ int main(int argc, char *argv[])
 {
     int i, j = 0, ret;
 
+    /*
+     * Set stdout to be unbuffered to avoid having to fflush when
+     * printing without a newline.
+     */
+    setvbuf(stdout, NULL, _IONBF, 0);
+
     if ( argc  <= 1 )
     {
         show_help();
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 3/8] tools/livepatch: Improve output

[Ross Lagerwall]

Improving the output of xen-livepatch, which is currently hard to read,
especially when an error occurs.

Some examples of the changes:
Before:
    $ xen-livepatch apply test
    Performing apply:. completed
After:
    $ xen-livepatch apply test
    Applying test:. completed

Before:
    $ xen-livepatch apply test2
    test2 failed with 22(Invalid argument)
    Performing apply: (no newline)
After:
    $ xen-livepatch apply test2
    Applying test2: failed
    Error 22: Invalid argument

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/misc/xen-livepatch.c | 75 +++++++++++++++++++++++++++++++---------------
 1 file changed, 51 insertions(+), 24 deletions(-)

diff --git a/tools/misc/xen-livepatch.c b/tools/misc/xen-livepatch.c
index 56ae5aa..6e3fcf2 100644
--- a/tools/misc/xen-livepatch.c
+++ b/tools/misc/xen-livepatch.c
@@ -100,7 +100,8 @@ static int list_func(int argc, char *argv[])
         rc = xc_livepatch_list(xch, MAX_LEN, idx, info, name, len, &done, &left);
         if ( rc )
         {
-            fprintf(stderr, "Failed to list %d/%d: %d(%s)!\n",
+            fprintf(stderr, "Failed to list %d/%d.\n"
+                            "Error %d: %s\n",
                     idx, left, errno, strerror(errno));
             break;
         }
@@ -140,7 +141,8 @@ static int get_name(int argc, char *argv[], char *name)
     ssize_t len = strlen(argv[0]);
     if ( len > XEN_LIVEPATCH_NAME_SIZE )
     {
-        fprintf(stderr, "ID MUST be %d characters!\n", XEN_LIVEPATCH_NAME_SIZE);
+        fprintf(stderr, "ID must be no more than %d characters.\n",
+                XEN_LIVEPATCH_NAME_SIZE);
         errno = EINVAL;
         return errno;
     }
@@ -172,13 +174,15 @@ static int upload_func(int argc, char *argv[])
     fd = open(filename, O_RDONLY);
     if ( fd < 0 )
     {
-        fprintf(stderr, "Could not open %s, error: %d(%s)\n",
+        fprintf(stderr, "Could not open %s.\n"
+                        "Error %d: %s\n",
                 filename, errno, strerror(errno));
         return errno;
     }
     if ( stat(filename, &buf) != 0 )
     {
-        fprintf(stderr, "Could not get right size %s, error: %d(%s)\n",
+        fprintf(stderr, "Could not get size of %s.\n"
+                        "Error %d: %s\n",
                 filename, errno, strerror(errno));
         close(fd);
         return errno;
@@ -188,21 +192,29 @@ static int upload_func(int argc, char *argv[])
     fbuf = mmap(0, len, PROT_READ, MAP_PRIVATE, fd, 0);
     if ( fbuf == MAP_FAILED )
     {
-        fprintf(stderr,"Could not map: %s, error: %d(%s)\n",
+        fprintf(stderr, "Could not map %s.\n"
+                        "Error %d: %s\n",
                 filename, errno, strerror(errno));
         close (fd);
         return errno;
     }
-    printf("Uploading %s (%zu bytes)\n", filename, len);
+    printf("Uploading %s... ", filename);
     rc = xc_livepatch_upload(xch, name, fbuf, len);
     if ( rc )
-        fprintf(stderr, "Upload failed: %s, error: %d(%s)!\n",
-                filename, errno, strerror(errno));
+    {
+        printf("failed\n");
+        fprintf(stderr, "Error %d: %s\n",
+                errno, strerror(errno));
+    }
+    else
+        printf("completed\n");
+
 
     if ( munmap( fbuf, len) )
     {
-        fprintf(stderr, "Could not unmap!? error: %d(%s)!\n",
-                errno, strerror(errno));
+        fprintf(stderr, "Could not unmap %s.\n"
+                        "Error %d: %s\n",
+                filename, errno, strerror(errno));
         if ( !rc )
             rc = errno;
     }
@@ -223,27 +235,32 @@ struct {
     int allow; /* State it must be in to call function. */
     int expected; /* The state to be in after the function. */
     const char *name;
+    const char *verb;
     int (*function)(xc_interface *xch, char *name, uint32_t timeout);
     unsigned int executed; /* Has the function been called?. */
 } action_options[] = {
     {   .allow = LIVEPATCH_STATE_CHECKED,
         .expected = LIVEPATCH_STATE_APPLIED,
         .name = "apply",
+        .verb = "Applying",
         .function = xc_livepatch_apply,
     },
     {   .allow = LIVEPATCH_STATE_APPLIED,
         .expected = LIVEPATCH_STATE_CHECKED,
         .name = "revert",
+        .verb = "Reverting",
         .function = xc_livepatch_revert,
     },
     {   .allow = LIVEPATCH_STATE_CHECKED,
         .expected = -XEN_ENOENT,
         .name = "unload",
+        .verb = "Unloading",
         .function = xc_livepatch_unload,
     },
     {   .allow = LIVEPATCH_STATE_CHECKED,
         .expected = LIVEPATCH_STATE_APPLIED,
         .name = "replace",
+        .verb = "Replacing all live patches with",
         .function = xc_livepatch_replace,
     },
 };
@@ -276,39 +293,44 @@ int action_func(int argc, char *argv[], unsigned int idx)
     rc = xc_livepatch_get(xch, name, &status);
     if ( rc )
     {
-        fprintf(stderr, "%s failed to get status %d(%s)!\n",
+        fprintf(stderr, "Failed to get status of %s.\n"
+                        "Error %d: %s\n",
                 name, errno, strerror(errno));
         return -1;
     }
     if ( status.rc == -XEN_EAGAIN )
     {
-        fprintf(stderr, "%s failed. Operation already in progress\n", name);
+        fprintf(stderr,
+                "Cannot execute %s.\n"
+                "Operation already in progress.\n", action_options[idx].name);
         return -1;
     }
 
     if ( status.state == action_options[idx].expected )
     {
-        printf("No action needed\n");
+        printf("No action needed.\n");
         return 0;
     }
 
     /* Perform action. */
     if ( action_options[idx].allow & status.state )
     {
-        printf("Performing %s:", action_options[idx].name);
+        printf("%s %s:", action_options[idx].verb, name);
         rc = action_options[idx].function(xch, name, 0);
         if ( rc )
         {
-            fprintf(stderr, "%s failed with %d(%s)\n", name, errno,
-                    strerror(errno));
+            printf(" failed\n");
+            fprintf(stderr, "Error %d: %s\n", errno, strerror(errno));
             return -1;
         }
     }
     else
     {
-        printf("%s: in wrong state (%s), expected (%s)\n",
-               name, state2str(status.state),
-               state2str(action_options[idx].allow));
+        fprintf(stderr, "%s is in the wrong state.\n"
+                        "Current state: %s\n"
+                        "Expected state: %s\n",
+                name, state2str(status.state),
+                state2str(action_options[idx].allow));
         return -1;
     }
 
@@ -335,7 +357,8 @@ int action_func(int argc, char *argv[], unsigned int idx)
 
     if ( retry >= RETRIES )
     {
-        fprintf(stderr, "%s: Operation didn't complete after 30 seconds.\n", name);
+        printf(" failed\n");
+        fprintf(stderr, "Operation didn't complete after 30 seconds.\n");
         return -1;
     }
     else
@@ -347,14 +370,18 @@ int action_func(int argc, char *argv[], unsigned int idx)
             printf(" completed\n");
         else if ( rc < 0 )
         {
-            fprintf(stderr, "%s failed with %d(%s)\n", name, -rc, strerror(-rc));
+            printf(" failed\n");
+            fprintf(stderr, "Error %d: %s\n", -rc, strerror(-rc));
             return -1;
         }
         else
         {
-            fprintf(stderr, "%s: in wrong state (%s), expected (%s)\n",
-               name, state2str(rc),
-               state2str(action_options[idx].expected));
+            printf(" failed\n");
+            fprintf(stderr, "%s is in the wrong state.\n"
+                            "Current state: %s\n"
+                            "Expected state: %s\n",
+                    name, state2str(rc),
+                    state2str(action_options[idx].expected));
             return -1;
         }
     }
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 4/8] livepatch: Fix documentation of timeout

[Ross Lagerwall]

The hypervisor expects the timeout from the hypercall to be in
nanoseconds, so document this correctly. Also correctly document
what happens when timeout is set to zero.

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
 docs/misc/livepatch.markdown  | 13 +++++++------
 tools/libxc/include/xenctrl.h |  2 +-
 xen/common/livepatch.c        |  4 ++--
 xen/include/public/sysctl.h   |  5 +++--
 4 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/docs/misc/livepatch.markdown b/docs/misc/livepatch.markdown
index 8f0559f..54a6b85 100644
--- a/docs/misc/livepatch.markdown
+++ b/docs/misc/livepatch.markdown
@@ -689,9 +689,9 @@ The caller provides:
   payload. If the operation takes more time than the upper bound of time
   the `rc` in `xen_livepatch_status' retrieved via **XEN_SYSCTL_LIVEPATCH_GET**
   will be -XEN_EBUSY.
- * `time` the upper bound of time (ms) the cmd should take. Zero means infinite.
-   If within the time the operation does not succeed the operation would go in
-   error state.
+ * `time` the upper bound of time (ns) the cmd should take. Zero means to use
+   the hypervisor default. If within the time the operation does not succeed
+   the operation would go in error state.
  * `pad` - *MUST* be zero.
 
 The return value will be zero unless the provided fields are incorrect.
@@ -706,9 +706,10 @@ The structure is as follow:
 struct xen_sysctl_livepatch_action {  
     xen_livepatch_name_t name;              /* IN, name of the patch. */  
     uint32_t cmd;                           /* IN: LIVEPATCH_ACTION_* */  
-    uint32_t time;                          /* IN: Zero if no timeout. */   
-                                            /* Or upper bound of time (ms) */   
-                                            /* for operation to take. */  
+    uint32_t time;                          /* IN: If zero then uses */
+                                            /* hypervisor default. */
+                                            /* Or upper bound of time (ns) */
+                                            /* for operation to take. */
 };  
 
 </pre>
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 2c83544..4ab0f57 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2703,7 +2703,7 @@ int xc_livepatch_list(xc_interface *xch, unsigned int max, unsigned int start,
  * The operations are asynchronous and the hypervisor may take a while
  * to complete them. The `timeout` offers an option to expire the
  * operation if it could not be completed within the specified time
- * (in ms). Value of 0 means let hypervisor decide the best timeout.
+ * (in ns). Value of 0 means let hypervisor decide the best timeout.
  */
 int xc_livepatch_apply(xc_interface *xch, char *name, uint32_t timeout);
 int xc_livepatch_revert(xc_interface *xch, char *name, uint32_t timeout);
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index fc8ef99..246e673 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -1227,8 +1227,8 @@ static int schedule_work(struct payload *data, uint32_t cmd, uint32_t timeout)
     livepatch_work.data = data;
     livepatch_work.timeout = timeout ?: MILLISECS(30);
 
-    dprintk(XENLOG_DEBUG, LIVEPATCH "%s: timeout is %"PRI_stime"ms\n",
-            data->name, livepatch_work.timeout / MILLISECS(1));
+    dprintk(XENLOG_DEBUG, LIVEPATCH "%s: timeout is %"PRIu32"ns\n",
+            data->name, livepatch_work.timeout);
 
     atomic_set(&livepatch_work.semaphore, -1);
 
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index 28ac56c..00f5e77 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -1067,8 +1067,9 @@ struct xen_sysctl_livepatch_action {
 #define LIVEPATCH_ACTION_APPLY        3
 #define LIVEPATCH_ACTION_REPLACE      4
     uint32_t cmd;                           /* IN: LIVEPATCH_ACTION_*. */
-    uint32_t timeout;                       /* IN: Zero if no timeout. */
-                                            /* Or upper bound of time (ms) */
+    uint32_t timeout;                       /* IN: If zero then uses */
+                                            /* hypervisor default. */
+                                            /* Or upper bound of time (ns) */
                                             /* for operation to take. */
 };
 typedef struct xen_sysctl_livepatch_action xen_sysctl_livepatch_action_t;
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 5/8] tools/livepatch: Remove pointless retry loop

[Ross Lagerwall]

The default timeout in the hypervisor for a livepatch operation is 30 ms,
but xen-livepatch currently waits for up to 30 seconds for the operation
to complete. Instead, remove the retry loop and simply wait for 2 * 30 ms
for the operation to complete. The extra period is to account for the
time to actually start the operation.

Furthermore, have xen-livepatch set the hypervisor timeout rather than
relying on the hypervisor default since the tool doesn't know how long
it will be. Use nanosleep rather than usleep since usleep has been
removed from POSIX.1-2008.

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
 tools/misc/xen-livepatch.c | 102 ++++++++++++++++++++++-----------------------
 1 file changed, 51 insertions(+), 51 deletions(-)

diff --git a/tools/misc/xen-livepatch.c b/tools/misc/xen-livepatch.c
index 6e3fcf2..4ebe95e 100644
--- a/tools/misc/xen-livepatch.c
+++ b/tools/misc/xen-livepatch.c
@@ -9,6 +9,7 @@
 #include <string.h>
 #include <sys/mman.h>
 #include <sys/stat.h>
+#include <time.h>
 #include <unistd.h>
 #include <xenctrl.h>
 #include <xenstore.h>
@@ -265,17 +266,31 @@ struct {
     },
 };
 
-/* Go around 300 * 0.1 seconds = 30 seconds. */
-#define RETRIES 300
-/* aka 0.1 second */
-#define DELAY 100000
+/* The hypervisor timeout for the live patching operation is 30 msec,
+ * but it could take some time for the operation to start, so wait twice
+ * that period. */
+#define HYPERVISOR_TIMEOUT_NS 30000000
+#define DELAY (2 * HYPERVISOR_TIMEOUT_NS)
+
+static void nanosleep_retry(long ns)
+{
+    struct timespec req, rem;
+    int rc;
+
+    rem.tv_sec = 0;
+    rem.tv_nsec = ns;
+
+    do {
+        req = rem;
+        rc = nanosleep(&req, &rem);
+    } while ( rc == -1 && errno == EINTR );
+}
 
 int action_func(int argc, char *argv[], unsigned int idx)
 {
     char name[XEN_LIVEPATCH_NAME_SIZE];
-    int rc, original_state;
+    int rc;
     xen_livepatch_status_t status;
-    unsigned int retry = 0;
 
     if ( argc != 1 )
     {
@@ -315,11 +330,11 @@ int action_func(int argc, char *argv[], unsigned int idx)
     /* Perform action. */
     if ( action_options[idx].allow & status.state )
     {
-        printf("%s %s:", action_options[idx].verb, name);
-        rc = action_options[idx].function(xch, name, 0);
+        printf("%s %s... ", action_options[idx].verb, name);
+        rc = action_options[idx].function(xch, name, HYPERVISOR_TIMEOUT_NS);
         if ( rc )
         {
-            printf(" failed\n");
+            printf("failed\n");
             fprintf(stderr, "Error %d: %s\n", errno, strerror(errno));
             return -1;
         }
@@ -334,56 +349,41 @@ int action_func(int argc, char *argv[], unsigned int idx)
         return -1;
     }
 
-    original_state = status.state;
-    do {
-        rc = xc_livepatch_get(xch, name, &status);
-        if ( rc )
-        {
-            rc = -errno;
-            break;
-        }
+    nanosleep_retry(DELAY);
+    rc = xc_livepatch_get(xch, name, &status);
 
-        if ( status.state != original_state )
-            break;
-        if ( status.rc && status.rc != -XEN_EAGAIN )
-        {
-            rc = status.rc;
-            break;
-        }
+    if ( rc )
+        rc = -errno;
+    else if ( status.rc )
+        rc = status.rc;
 
-        printf(".");
-        usleep(DELAY);
-    } while ( ++retry < RETRIES );
+    if ( rc == -XEN_EAGAIN )
+    {
+        printf("failed\n");
+        fprintf(stderr, "Operation didn't complete.\n");
+        return -1;
+    }
+
+    if ( rc == 0 )
+        rc = status.state;
 
-    if ( retry >= RETRIES )
+    if ( action_options[idx].expected == rc )
+        printf("completed\n");
+    else if ( rc < 0 )
     {
-        printf(" failed\n");
-        fprintf(stderr, "Operation didn't complete after 30 seconds.\n");
+        printf("failed\n");
+        fprintf(stderr, "Error %d: %s\n", -rc, strerror(-rc));
         return -1;
     }
     else
     {
-        if ( rc == 0 )
-            rc = status.state;
-
-        if ( action_options[idx].expected == rc )
-            printf(" completed\n");
-        else if ( rc < 0 )
-        {
-            printf(" failed\n");
-            fprintf(stderr, "Error %d: %s\n", -rc, strerror(-rc));
-            return -1;
-        }
-        else
-        {
-            printf(" failed\n");
-            fprintf(stderr, "%s is in the wrong state.\n"
-                            "Current state: %s\n"
-                            "Expected state: %s\n",
-                    name, state2str(rc),
-                    state2str(action_options[idx].expected));
-            return -1;
-        }
+        printf("failed\n");
+        fprintf(stderr, "%s is in the wrong state.\n"
+                        "Current state: %s\n"
+                        "Expected state: %s\n",
+                name, state2str(rc),
+                state2str(action_options[idx].expected));
+        return -1;
     }
 
     return 0;
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 6/8] tools/livepatch: Remove unused struct member

[Ross Lagerwall]

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 tools/misc/xen-livepatch.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tools/misc/xen-livepatch.c b/tools/misc/xen-livepatch.c
index 4ebe95e..fd2f968 100644
--- a/tools/misc/xen-livepatch.c
+++ b/tools/misc/xen-livepatch.c
@@ -238,7 +238,6 @@ struct {
     const char *name;
     const char *verb;
     int (*function)(xc_interface *xch, char *name, uint32_t timeout);
-    unsigned int executed; /* Has the function been called?. */
 } action_options[] = {
     {   .allow = LIVEPATCH_STATE_CHECKED,
         .expected = LIVEPATCH_STATE_APPLIED,
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 7/8] tools/livepatch: Save errno where needed

[Ross Lagerwall]

Fix a number of incorrect uses of errno after an operation that could
set it (e.g. fprintf, close).

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
 tools/misc/xen-livepatch.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/tools/misc/xen-livepatch.c b/tools/misc/xen-livepatch.c
index fd2f968..140445d 100644
--- a/tools/misc/xen-livepatch.c
+++ b/tools/misc/xen-livepatch.c
@@ -101,9 +101,10 @@ static int list_func(int argc, char *argv[])
         rc = xc_livepatch_list(xch, MAX_LEN, idx, info, name, len, &done, &left);
         if ( rc )
         {
+            rc = errno;
             fprintf(stderr, "Failed to list %d/%d.\n"
                             "Error %d: %s\n",
-                    idx, left, errno, strerror(errno));
+                    idx, left, rc, strerror(rc));
             break;
         }
         if ( !idx )
@@ -175,37 +176,40 @@ static int upload_func(int argc, char *argv[])
     fd = open(filename, O_RDONLY);
     if ( fd < 0 )
     {
+        int saved_errno = errno;
         fprintf(stderr, "Could not open %s.\n"
                         "Error %d: %s\n",
-                filename, errno, strerror(errno));
-        return errno;
+                filename, saved_errno, strerror(saved_errno));
+        return saved_errno;
     }
     if ( stat(filename, &buf) != 0 )
     {
+        int saved_errno = errno;
         fprintf(stderr, "Could not get size of %s.\n"
                         "Error %d: %s\n",
-                filename, errno, strerror(errno));
+                filename, saved_errno, strerror(saved_errno));
         close(fd);
-        return errno;
+        return saved_errno;
     }
 
     len = buf.st_size;
     fbuf = mmap(0, len, PROT_READ, MAP_PRIVATE, fd, 0);
     if ( fbuf == MAP_FAILED )
     {
+        int saved_errno = errno;
         fprintf(stderr, "Could not map %s.\n"
                         "Error %d: %s\n",
-                filename, errno, strerror(errno));
+                filename, saved_errno, strerror(saved_errno));
         close (fd);
-        return errno;
+        return saved_errno;
     }
     printf("Uploading %s... ", filename);
     rc = xc_livepatch_upload(xch, name, fbuf, len);
     if ( rc )
     {
+        rc = errno;
         printf("failed\n");
-        fprintf(stderr, "Error %d: %s\n",
-                errno, strerror(errno));
+        fprintf(stderr, "Error %d: %s\n", rc, strerror(rc));
     }
     else
         printf("completed\n");
@@ -216,8 +220,6 @@ static int upload_func(int argc, char *argv[])
         fprintf(stderr, "Could not unmap %s.\n"
                         "Error %d: %s\n",
                 filename, errno, strerror(errno));
-        if ( !rc )
-            rc = errno;
     }
     close(fd);
 
@@ -333,8 +335,10 @@ int action_func(int argc, char *argv[], unsigned int idx)
         rc = action_options[idx].function(xch, name, HYPERVISOR_TIMEOUT_NS);
         if ( rc )
         {
+            int saved_errno = errno;
             printf("failed\n");
-            fprintf(stderr, "Error %d: %s\n", errno, strerror(errno));
+            fprintf(stderr, "Error %d: %s\n",
+                    saved_errno, strerror(saved_errno));
             return -1;
         }
     }
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 8/8] tools/livepatch: Exit with 2 if a timeout occurs

[Ross Lagerwall]

Exit with 0 for success.
Exit with 1 for an error.
Exit with 2 if the operation should be retried for any reason (e.g. a
timeout or because another operation was in progress).

This allows a program or script driving xen-livepatch to determine if
the operation should be retried without parsing the output.

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
 tools/misc/xen-livepatch.c | 37 ++++++++++++++++++++++++++++++-------
 1 file changed, 30 insertions(+), 7 deletions(-)

diff --git a/tools/misc/xen-livepatch.c b/tools/misc/xen-livepatch.c
index 140445d..af9fcd6 100644
--- a/tools/misc/xen-livepatch.c
+++ b/tools/misc/xen-livepatch.c
@@ -15,6 +15,7 @@
 #include <xenstore.h>
 
 #include <xen/errno.h>
+#include <xen-tools/libs.h>
 
 static xc_interface *xch;
 
@@ -309,17 +310,18 @@ int action_func(int argc, char *argv[], unsigned int idx)
     rc = xc_livepatch_get(xch, name, &status);
     if ( rc )
     {
+        int saved_errno = errno;
         fprintf(stderr, "Failed to get status of %s.\n"
                         "Error %d: %s\n",
-                name, errno, strerror(errno));
-        return -1;
+                name, saved_errno, strerror(saved_errno));
+        return saved_errno;
     }
     if ( status.rc == -XEN_EAGAIN )
     {
         fprintf(stderr,
                 "Cannot execute %s.\n"
                 "Operation already in progress.\n", action_options[idx].name);
-        return -1;
+        return EAGAIN;
     }
 
     if ( status.state == action_options[idx].expected )
@@ -339,7 +341,7 @@ int action_func(int argc, char *argv[], unsigned int idx)
             printf("failed\n");
             fprintf(stderr, "Error %d: %s\n",
                     saved_errno, strerror(saved_errno));
-            return -1;
+            return saved_errno;
         }
     }
     else
@@ -364,7 +366,7 @@ int action_func(int argc, char *argv[], unsigned int idx)
     {
         printf("failed\n");
         fprintf(stderr, "Operation didn't complete.\n");
-        return -1;
+        return EAGAIN;
     }
 
     if ( rc == 0 )
@@ -376,7 +378,7 @@ int action_func(int argc, char *argv[], unsigned int idx)
     {
         printf("failed\n");
         fprintf(stderr, "Error %d: %s\n", -rc, strerror(-rc));
-        return -1;
+        return -rc;
     }
     else
     {
@@ -488,7 +490,28 @@ int main(int argc, char *argv[])
 
     xc_interface_close(xch);
 
-    return !!ret;
+    /*
+     * Exitcode 0 for success.
+     * Exitcode 1 for an error.
+     * Exitcode 2 if the operation should be retried for any reason (e.g. a
+     * timeout or because another operation was in progress).
+     */
+#define EXIT_TIMEOUT (EXIT_FAILURE + 1)
+
+    BUILD_BUG_ON(EXIT_SUCCESS != 0);
+    BUILD_BUG_ON(EXIT_FAILURE != 1);
+    BUILD_BUG_ON(EXIT_TIMEOUT != 2);
+
+    switch ( ret )
+    {
+    case 0:
+        return EXIT_SUCCESS;
+    case EAGAIN:
+    case EBUSY:
+        return EXIT_TIMEOUT;
+    default:
+        return EXIT_FAILURE;
+    }
 }
 
 /*
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 2/8] tools/livepatch: Set stdout and stderr unbuffered

[Wei Liu]

On Wed, Dec 14, 2016 at 07:51:54AM +0000, Ross Lagerwall wrote:
> Using both stdout and stderr interleaved without newlines can result in
> strange output when using line buffered mode (e.g. a terminal) or when
> fully buffered (e.g. redirected to a file). Set stdout to unbuffered mode
> to fix this (stderr is always unbuffered by default).
> 
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

> ---
>  tools/misc/xen-livepatch.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/misc/xen-livepatch.c b/tools/misc/xen-livepatch.c
> index f6c7c8a..56ae5aa 100644
> --- a/tools/misc/xen-livepatch.c
> +++ b/tools/misc/xen-livepatch.c
> @@ -330,7 +330,6 @@ int action_func(int argc, char *argv[], unsigned int idx)
>          }
>  
>          printf(".");
> -        fflush(stdout);
>          usleep(DELAY);
>      } while ( ++retry < RETRIES );
>  
> @@ -416,6 +415,12 @@ int main(int argc, char *argv[])
>  {
>      int i, j = 0, ret;
>  
> +    /*
> +     * Set stdout to be unbuffered to avoid having to fflush when
> +     * printing without a newline.
> +     */
> +    setvbuf(stdout, NULL, _IONBF, 0);
> +
>      if ( argc  <= 1 )
>      {
>          show_help();
> -- 
> 2.7.4
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 4/8] livepatch: Fix documentation of timeout

[Wei Liu]

On Wed, Dec 14, 2016 at 07:51:56AM +0000, Ross Lagerwall wrote:
> The hypervisor expects the timeout from the hypercall to be in
> nanoseconds, so document this correctly. Also correctly document
> what happens when timeout is set to zero.
> 
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 5/8] tools/livepatch: Remove pointless retry loop

[Wei Liu]

On Wed, Dec 14, 2016 at 07:51:57AM +0000, Ross Lagerwall wrote:
> The default timeout in the hypervisor for a livepatch operation is 30 ms,
> but xen-livepatch currently waits for up to 30 seconds for the operation
> to complete. Instead, remove the retry loop and simply wait for 2 * 30 ms
> for the operation to complete. The extra period is to account for the
> time to actually start the operation.
> 
> Furthermore, have xen-livepatch set the hypervisor timeout rather than
> relying on the hypervisor default since the tool doesn't know how long
> it will be. Use nanosleep rather than usleep since usleep has been
> removed from POSIX.1-2008.
> 
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

But I would also like Korand's ack on this one.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 7/8] tools/livepatch: Save errno where needed

[Wei Liu]

On Wed, Dec 14, 2016 at 07:51:59AM +0000, Ross Lagerwall wrote:
> Fix a number of incorrect uses of errno after an operation that could
> set it (e.g. fprintf, close).
> 
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> ---

Acked-by: Wei Liu <wei.liu2@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 8/8] tools/livepatch: Exit with 2 if a timeout occurs

[Wei Liu]

On Wed, Dec 14, 2016 at 07:52:00AM +0000, Ross Lagerwall wrote:
> Exit with 0 for success.
> Exit with 1 for an error.
> Exit with 2 if the operation should be retried for any reason (e.g. a
> timeout or because another operation was in progress).
> 
> This allows a program or script driving xen-livepatch to determine if
> the operation should be retried without parsing the output.
> 
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 0/8] xen-livepatch misc fixes/changes

[Konrad Rzeszutek Wilk]

On Wed, Dec 14, 2016 at 07:51:52AM +0000, Ross Lagerwall wrote:
> Hi all,
> 
> This series contains a few fixes to the xen-livepatch tool.
> It also contains a few changes to make the output more readable.

Hey Ross,

I've applied and reviewed some of the patches. Testing them right now
and if theere are no issues will push to staging shortly.

Thank you for the fixes!
> 
> Changed in v2:
> * Fix minor comments.
> * Split the last patch into two.
> 
> Ross Lagerwall (8):
>   tools/livepatch: Show the correct expected state before action
>   tools/livepatch: Set stdout and stderr unbuffered
>   tools/livepatch: Improve output
>   livepatch: Fix documentation of timeout
>   tools/livepatch: Remove pointless retry loop
>   tools/livepatch: Remove unused struct member
>   tools/livepatch: Save errno where needed
>   tools/livepatch: Exit with 2 if a timeout occurs
> 
>  docs/misc/livepatch.markdown  |  13 +--
>  tools/libxc/include/xenctrl.h |   2 +-
>  tools/misc/xen-livepatch.c    | 218 ++++++++++++++++++++++++++----------------
>  xen/common/livepatch.c        |   4 +-
>  xen/include/public/sysctl.h   |   5 +-
>  5 files changed, 151 insertions(+), 91 deletions(-)
> 
> -- 
> 2.7.4
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 0/8] xen-livepatch misc fixes/changes

[Konrad Rzeszutek Wilk]

On Wed, Dec 14, 2016 at 01:16:00PM -0500, Konrad Rzeszutek Wilk wrote:
> On Wed, Dec 14, 2016 at 07:51:52AM +0000, Ross Lagerwall wrote:
> > Hi all,
> > 
> > This series contains a few fixes to the xen-livepatch tool.
> > It also contains a few changes to make the output more readable.
> 
> Hey Ross,
> 
> I've applied and reviewed some of the patches. Testing them right now
> and if theere are no issues will push to staging shortly.

Completed. Patches pushed in staging. Thank you again!

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel