[Xen-devel] [PATCH] tools/save: Drop unused parameters from xc_domain_save()

Andrew Cooper posted 1 patch 37 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/xen tags/patchew/20200106170352.28582-1-andrew.cooper3@citrix.com
tools/libxc/include/xenguest.h   | 15 +++++++--------
tools/libxc/xc_nomigrate.c       |  2 +-
tools/libxc/xc_sr_save.c         | 31 ++++++++++++++++---------------
tools/libxl/libxl_dom_save.c     | 21 +--------------------
tools/libxl/libxl_internal.h     |  1 -
tools/libxl/libxl_save_callout.c |  2 +-
tools/libxl/libxl_save_helper.c  |  3 +--
7 files changed, 27 insertions(+), 48 deletions(-)

[Xen-devel] [PATCH] tools/save: Drop unused parameters from xc_domain_save()

Posted by Andrew Cooper 37 weeks ago
XCFLAGS_CHECKPOINT_COMPRESS has been unused since c/s b15bc4345 (2015),
XCFLAGS_HVM since c/s 9e8672f1c (2013), and XCFLAGS_STDVGA since c/s
087d43326 (2007).  Drop the constants, and code which sets them.

The separate hvm parameter (appeared in c/s d11bec8a1, 2007 and ultimately
redundant with XCFLAGS_HVM), is used for sanity checking and debug printing,
then discarded and replaced with Xen's idea of whether the domain is PV or
HVM.

Rearrange the logic in xc_domain_save() to ask Xen sightly earlier, and use a
consistent idea of 'hvm' throughout.  Removing this parameter removes the
final user of libxl's dss->hvm, so drop that field as well.

Update the doxygen comment to be accurate.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/libxc/include/xenguest.h   | 15 +++++++--------
 tools/libxc/xc_nomigrate.c       |  2 +-
 tools/libxc/xc_sr_save.c         | 31 ++++++++++++++++---------------
 tools/libxl/libxl_dom_save.c     | 21 +--------------------
 tools/libxl/libxl_internal.h     |  1 -
 tools/libxl/libxl_save_callout.c |  2 +-
 tools/libxl/libxl_save_helper.c  |  3 +--
 7 files changed, 27 insertions(+), 48 deletions(-)

diff --git a/tools/libxc/include/xenguest.h b/tools/libxc/include/xenguest.h
index fdb03e32da..1c358a0577 100644
--- a/tools/libxc/include/xenguest.h
+++ b/tools/libxc/include/xenguest.h
@@ -26,9 +26,6 @@
 
 #define XCFLAGS_LIVE      (1 << 0)
 #define XCFLAGS_DEBUG     (1 << 1)
-#define XCFLAGS_HVM       (1 << 2)
-#define XCFLAGS_STDVGA    (1 << 3)
-#define XCFLAGS_CHECKPOINT_COMPRESS    (1 << 4)
 
 #define X86_64_B_SIZE   64 
 #define X86_32_B_SIZE   32
@@ -124,16 +121,18 @@ typedef enum {
 /**
  * This function will save a running domain.
  *
- * @parm xch a handle to an open hypervisor interface
- * @parm fd the file descriptor to save a domain to
- * @parm dom the id of the domain
+ * @param xch a handle to an open hypervisor interface
+ * @param io_fd the file descriptor to save a domain to
+ * @param dom the id of the domain
+ * @param flags XCFLAGS_xxx
  * @param stream_type XC_MIG_STREAM_NONE if the far end of the stream
  *        doesn't use checkpointing
+ * @param recv_fd Only used for XC_MIG_STREAM_COLO.  Contains backchannel from
+ *        the destination side.
  * @return 0 on success, -1 on failure
  */
 int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom,
-                   uint32_t flags /* XCFLAGS_xxx */,
-                   struct save_callbacks* callbacks, int hvm,
+                   uint32_t flags, struct save_callbacks *callbacks,
                    xc_migration_stream_t stream_type, int recv_fd);
 
 /* callbacks provided by xc_domain_restore */
diff --git a/tools/libxc/xc_nomigrate.c b/tools/libxc/xc_nomigrate.c
index c4dca88eb0..5a1d7e46f9 100644
--- a/tools/libxc/xc_nomigrate.c
+++ b/tools/libxc/xc_nomigrate.c
@@ -21,7 +21,7 @@
 #include <xenguest.h>
 
 int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom, uint32_t flags,
-                   struct save_callbacks* callbacks, int hvm,
+                   struct save_callbacks *callbacks,
                    xc_migration_stream_t stream_type, int recv_fd)
 {
     errno = ENOSYS;
diff --git a/tools/libxc/xc_sr_save.c b/tools/libxc/xc_sr_save.c
index 5a40e588e0..6f61f85ee0 100644
--- a/tools/libxc/xc_sr_save.c
+++ b/tools/libxc/xc_sr_save.c
@@ -967,7 +967,7 @@ static int save(struct xc_sr_context *ctx, uint16_t guest_type)
 
 int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom,
                    uint32_t flags, struct save_callbacks* callbacks,
-                   int hvm, xc_migration_stream_t stream_type, int recv_fd)
+                   xc_migration_stream_t stream_type, int recv_fd)
 {
     struct xc_sr_context ctx =
         {
@@ -982,32 +982,33 @@ int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom,
     ctx.save.checkpointed = stream_type;
     ctx.save.recv_fd = recv_fd;
 
+    if ( xc_domain_getinfo(xch, dom, 1, &ctx.dominfo) != 1 )
+    {
+        PERROR("Failed to get domain info");
+        return -1;
+    }
+
+    if ( ctx.dominfo.domid != dom )
+    {
+        ERROR("Domain %u does not exist", dom);
+        return -1;
+    }
+
     /* If altering migration_stream update this assert too. */
     assert(stream_type == XC_MIG_STREAM_NONE ||
            stream_type == XC_MIG_STREAM_REMUS ||
            stream_type == XC_MIG_STREAM_COLO);
 
     /* Sanity checks for callbacks. */
-    if ( hvm )
+    if ( ctx.dominfo.hvm )
         assert(callbacks->switch_qemu_logdirty);
     if ( ctx.save.checkpointed )
         assert(callbacks->checkpoint && callbacks->postcopy);
     if ( ctx.save.checkpointed == XC_MIG_STREAM_COLO )
         assert(callbacks->wait_checkpoint);
 
-    DPRINTF("fd %d, dom %u, flags %u, hvm %d", io_fd, dom, flags, hvm);
-
-    if ( xc_domain_getinfo(xch, dom, 1, &ctx.dominfo) != 1 )
-    {
-        PERROR("Failed to get domain info");
-        return -1;
-    }
-
-    if ( ctx.dominfo.domid != dom )
-    {
-        ERROR("Domain %u does not exist", dom);
-        return -1;
-    }
+    DPRINTF("fd %d, dom %u, flags %u, hvm %d",
+            io_fd, dom, flags, ctx.dominfo.hvm);
 
     ctx.domid = dom;
 
diff --git a/tools/libxl/libxl_dom_save.c b/tools/libxl/libxl_dom_save.c
index 65610e6055..32e3cb5a13 100644
--- a/tools/libxl/libxl_dom_save.c
+++ b/tools/libxl/libxl_dom_save.c
@@ -408,22 +408,8 @@ void libxl__domain_save(libxl__egc *egc, libxl__domain_save_state *dss)
     rc = libxl__domain_suspend_init(egc, dsps, type);
     if (rc) goto out;
 
-    switch (type) {
-    case LIBXL_DOMAIN_TYPE_PVH:
-    case LIBXL_DOMAIN_TYPE_HVM: {
-        dss->hvm = 1;
-        break;
-    }
-    case LIBXL_DOMAIN_TYPE_PV:
-        dss->hvm = 0;
-        break;
-    default:
-        abort();
-    }
-
     dss->xcflags = (live ? XCFLAGS_LIVE : 0)
-          | (debug ? XCFLAGS_DEBUG : 0)
-          | (dss->hvm ? XCFLAGS_HVM : 0);
+          | (debug ? XCFLAGS_DEBUG : 0);
 
     /* Disallow saving a guest with vNUMA configured because migration
      * stream does not preserve node information.
@@ -440,11 +426,6 @@ void libxl__domain_save(libxl__egc *egc, libxl__domain_save_state *dss)
         goto out;
     }
 
-    if (dss->checkpointed_stream == LIBXL_CHECKPOINTED_STREAM_REMUS) {
-        if (libxl_defbool_val(r_info->compression))
-            dss->xcflags |= XCFLAGS_CHECKPOINT_COMPRESS;
-    }
-
     if (dss->checkpointed_stream == LIBXL_CHECKPOINTED_STREAM_NONE)
         callbacks->suspend = libxl__domain_suspend_callback;
 
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index a99f3627e4..ba8c9b41ab 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -3591,7 +3591,6 @@ struct libxl__domain_save_state {
     const libxl_domain_remus_info *remus;
     /* private */
     int rc;
-    int hvm;
     int xcflags;
     libxl__domain_suspend_state dsps;
     union {
diff --git a/tools/libxl/libxl_save_callout.c b/tools/libxl/libxl_save_callout.c
index caa1396813..0b11495f9b 100644
--- a/tools/libxl/libxl_save_callout.c
+++ b/tools/libxl/libxl_save_callout.c
@@ -87,7 +87,7 @@ void libxl__xc_domain_save(libxl__egc *egc, libxl__domain_save_state *dss,
         libxl__srm_callout_enumcallbacks_save(&shs->callbacks.save.a);
 
     const unsigned long argnums[] = {
-        dss->domid, dss->xcflags, dss->hvm, cbflags,
+        dss->domid, dss->xcflags, cbflags,
         dss->checkpointed_stream,
     };
 
diff --git a/tools/libxl/libxl_save_helper.c b/tools/libxl/libxl_save_helper.c
index cdad40be4f..0f52930c45 100644
--- a/tools/libxl/libxl_save_helper.c
+++ b/tools/libxl/libxl_save_helper.c
@@ -252,7 +252,6 @@ int main(int argc, char **argv)
         recv_fd =                           atoi(NEXTARG);
         uint32_t dom =                      strtoul(NEXTARG,0,10);
         uint32_t flags =                    strtoul(NEXTARG,0,10);
-        int hvm =                           atoi(NEXTARG);
         unsigned cbflags =                  strtoul(NEXTARG,0,10);
         xc_migration_stream_t stream_type = strtoul(NEXTARG,0,10);
         assert(!*++argv);
@@ -263,7 +262,7 @@ int main(int argc, char **argv)
         setup_signals(save_signal_handler);
 
         r = xc_domain_save(xch, io_fd, dom, flags, &helper_save_callbacks,
-                           hvm, stream_type, recv_fd);
+                           stream_type, recv_fd);
         complete(r);
 
     } else if (!strcmp(mode,"--restore-domain")) {
-- 
2.11.0


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

Re: [Xen-devel] [PATCH] tools/save: Drop unused parameters from xc_domain_save()

Posted by Wei Liu 37 weeks ago
On Mon, Jan 06, 2020 at 05:03:52PM +0000, Andrew Cooper wrote:
> XCFLAGS_CHECKPOINT_COMPRESS has been unused since c/s b15bc4345 (2015),
> XCFLAGS_HVM since c/s 9e8672f1c (2013), and XCFLAGS_STDVGA since c/s
> 087d43326 (2007).  Drop the constants, and code which sets them.
> 
> The separate hvm parameter (appeared in c/s d11bec8a1, 2007 and ultimately
> redundant with XCFLAGS_HVM), is used for sanity checking and debug printing,
> then discarded and replaced with Xen's idea of whether the domain is PV or
> HVM.
> 
> Rearrange the logic in xc_domain_save() to ask Xen sightly earlier, and use a
> consistent idea of 'hvm' throughout.  Removing this parameter removes the
> final user of libxl's dss->hvm, so drop that field as well.
> 
> Update the doxygen comment to be accurate.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Ian Jackson <Ian.Jackson@citrix.com>

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

Re: [Xen-devel] [PATCH] tools/save: Drop unused parameters from xc_domain_save()

Posted by Wei Liu 35 weeks ago
On Tue, Jan 07, 2020 at 12:19:36PM +0000, Wei Liu wrote:
> On Mon, Jan 06, 2020 at 05:03:52PM +0000, Andrew Cooper wrote:
> > XCFLAGS_CHECKPOINT_COMPRESS has been unused since c/s b15bc4345 (2015),
> > XCFLAGS_HVM since c/s 9e8672f1c (2013), and XCFLAGS_STDVGA since c/s
> > 087d43326 (2007).  Drop the constants, and code which sets them.
> > 
> > The separate hvm parameter (appeared in c/s d11bec8a1, 2007 and ultimately
> > redundant with XCFLAGS_HVM), is used for sanity checking and debug printing,
> > then discarded and replaced with Xen's idea of whether the domain is PV or
> > HVM.
> > 
> > Rearrange the logic in xc_domain_save() to ask Xen sightly earlier, and use a
> > consistent idea of 'hvm' throughout.  Removing this parameter removes the
> > final user of libxl's dss->hvm, so drop that field as well.
> > 
> > Update the doxygen comment to be accurate.
> > 
> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> Acked-by: Ian Jackson <Ian.Jackson@citrix.com>

This is a mistake. I obviously shouldn't use Ian's name and address to
ack a patch.

Acked-by: Wei Liu <wl@xen.org>

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

Re: [Xen-devel] [PATCH] tools/save: Drop unused parameters from xc_domain_save()

Posted by Andrew Cooper 35 weeks ago
On 22/01/2020 14:08, Wei Liu wrote:
> On Tue, Jan 07, 2020 at 12:19:36PM +0000, Wei Liu wrote:
>> On Mon, Jan 06, 2020 at 05:03:52PM +0000, Andrew Cooper wrote:
>>> XCFLAGS_CHECKPOINT_COMPRESS has been unused since c/s b15bc4345 (2015),
>>> XCFLAGS_HVM since c/s 9e8672f1c (2013), and XCFLAGS_STDVGA since c/s
>>> 087d43326 (2007).  Drop the constants, and code which sets them.
>>>
>>> The separate hvm parameter (appeared in c/s d11bec8a1, 2007 and ultimately
>>> redundant with XCFLAGS_HVM), is used for sanity checking and debug printing,
>>> then discarded and replaced with Xen's idea of whether the domain is PV or
>>> HVM.
>>>
>>> Rearrange the logic in xc_domain_save() to ask Xen sightly earlier, and use a
>>> consistent idea of 'hvm' throughout.  Removing this parameter removes the
>>> final user of libxl's dss->hvm, so drop that field as well.
>>>
>>> Update the doxygen comment to be accurate.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Acked-by: Ian Jackson <Ian.Jackson@citrix.com>
> This is a mistake. I obviously shouldn't use Ian's name and address to
> ack a patch.
>
> Acked-by: Wei Liu <wl@xen.org>

Erm, oops...  I already committed it with "Ian's" ack.

I was talking to him about it on IRC at around this time (in relation to
its companion patch on the restore side, which he really did ack), so I
didn't think twice.

Overall, no harm done, but I will try to be more vigilant in the future.

~Andrew

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