[01/35] libxl: Make libxl_domain_unpause async

Commit Message

Anthony PERARD Aug. 2, 2019, 3:35 p.m. UTC

libxl_domain_unpause needs to make QMP calls, which are asynchronous,
change the API to reflect that.

Do the same with libxl_domain_pause async, even if it will keep
completing synchronously.

Also fix some coding style issue in those functions.

Signed-off-by: Anthony PERARD <[e-mail]>
---
 tools/libxl/libxl.h              | 26 +++++++++++++++--
 tools/libxl/libxl_colo_restore.c |  2 +-
 tools/libxl/libxl_colo_save.c    |  2 +-
 tools/libxl/libxl_dm.c           |  2 +-
 tools/libxl/libxl_domain.c       | 48 +++++++++++++++++++++-----------
 tools/libxl/libxl_internal.h     |  1 +
 tools/xl/xl_migrate.c            |  4 +--
 tools/xl/xl_saverestore.c        |  2 +-
 tools/xl/xl_vmcontrol.c          |  6 ++--
 9 files changed, 65 insertions(+), 28 deletions(-)

Comments

Ian Jackson Sept. 17, 2019, 4:50 p.m. UTC | #1

Anthony PERARD writes ("[PATCH 01/35] libxl: Make libxl_domain_unpause async"):
> libxl_domain_unpause needs to make QMP calls, which are asynchronous,
> change the API to reflect that.
> 
> Do the same with libxl_domain_pause async, even if it will keep
> completing synchronously.

Jolly good.  I think, though, that there should be a HAVE_...  for
this set of API changes.  I don't mind if it goes into the last patch
or the first one.  Assuming you add that to the series in v2:

Acked-by: Ian Jackson <[e-mail]>

Thanks,
Ian.

Anthony PERARD Sept. 18, 2019, 2:05 p.m. UTC | #2

On Tue, Sep 17, 2019 at 05:50:05PM +0100, Ian Jackson wrote:
> Anthony PERARD writes ("[PATCH 01/35] libxl: Make libxl_domain_unpause async"):
> > libxl_domain_unpause needs to make QMP calls, which are asynchronous,
> > change the API to reflect that.
> > 
> > Do the same with libxl_domain_pause async, even if it will keep
> > completing synchronously.
> 
> Jolly good.  I think, though, that there should be a HAVE_...  for
> this set of API changes.  I don't mind if it goes into the last patch

I thought that HAVE_* wasn't needed when the API version is bumped. But
now I guess that the HAVE_* macro are the only way for an application
to build against old version of libxl since the version number isn't
exposed.

The question is, how many macro should there be? As many macro as there
are function changed? Or just one?

For many, I can add that:
    /*
     * LIBXL_HAVE_*_ASYNC
     *
     * Those defines indicates that the function libxl_*()'s API has changed
     * and have an extra parameter "ao_how" which means that the function can be
     * executed asynchronously. Those functions are:
     *   libxl_domain_pause()
     *   libxl_domain_unpause()
     *   libxl_send_trigger()
     *   libxl_set_vcpuonline()
     *   libxl_retrieve_domain_configuration()
     *   libxl_qemu_monitor_command()
     */
    #define LIBXL_HAVE_DOMAIN_PAUSE_ASYNC 1
    #define LIBXL_HAVE_DOMAIN_UNPAUSE_ASYNC 1
    #define LIBXL_HAVE_SEND_TRIGGER_ASYNC 1
    #define LIBXL_HAVE_SET_VCPUONLINE_ASYNC 1
    #define LIBXL_HAVE_RETRIEVE_DOMAIN_CONFIGURATION_ASYNC 1
    #define LIBXL_HAVE_QEMU_MONITOR_COMMAND_ASYNC 1

If only one, I don't know how to call it, maybe on of or related:
    LIBXL_HAVE_QMP_FN_ASYNC
    LIBXL_HAVE_FN_USING_QMP_ASYNC
with the same comment listing all affected functions.

> or the first one.  Assuming you add that to the series in v2:
> 
> Acked-by: Ian Jackson <[e-mail]>

Thanks,

Ian Jackson Sept. 18, 2019, 3:44 p.m. UTC | #3

Anthony PERARD writes ("Re: [PATCH 01/35] libxl: Make libxl_domain_unpause async"):
> I thought that HAVE_* wasn't needed when the API version is bumped. But
> now I guess that the HAVE_* macro are the only way for an application
> to build against old version of libxl since the version number isn't
> exposed.

The application can #define the version number of the API it wants.

The HAVE is for an application that *doesn't* do that and wants
instead to adapt continually to our API changes.

> The question is, how many macro should there be? As many macro as there
> are function changed? Or just one?

One will do.

> If only one, I don't know how to call it, maybe on of or related:
>     LIBXL_HAVE_QMP_FN_ASYNC
>     LIBXL_HAVE_FN_USING_QMP_ASYNC
> with the same comment listing all affected functions.

Either of those is fine.  I will leave it to your judgement.

Thanks,
Ian.

Patch

diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index e40546c23a..7a31169611 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -619,7 +619,8 @@  typedef struct libxl__ctx libxl_ctx;
 /* API compatibility. */
 #ifdef LIBXL_API_VERSION
 #if LIBXL_API_VERSION != 0x040200 && LIBXL_API_VERSION != 0x040300 && \
-    LIBXL_API_VERSION != 0x040400 && LIBXL_API_VERSION != 0x040500
+    LIBXL_API_VERSION != 0x040400 && LIBXL_API_VERSION != 0x040500 && \
+    LIBXL_API_VERSION != 0x041300
 #error Unknown LIBXL_API_VERSION
 #endif
 #endif
@@ -1595,8 +1596,27 @@  int libxl_domain_rename(libxl_ctx *ctx, uint32_t domid,
    * transactionally that the domain has the old old name; if
    * trans is not 0 we use caller's transaction and caller must do retries */
 
-int libxl_domain_pause(libxl_ctx *ctx, uint32_t domid);
-int libxl_domain_unpause(libxl_ctx *ctx, uint32_t domid);
+int libxl_domain_pause(libxl_ctx *ctx, uint32_t domid,
+                       const libxl_asyncop_how *ao_how)
+                       LIBXL_EXTERNAL_CALLERS_ONLY;
+int libxl_domain_unpause(libxl_ctx *ctx, uint32_t domid,
+                         const libxl_asyncop_how *ao_how)
+                         LIBXL_EXTERNAL_CALLERS_ONLY;
+#if defined(LIBXL_API_VERSION) && LIBXL_API_VERSION < 0x041300
+static inline int libxl_domain_pause_0x041200(
+    libxl_ctx *ctx, uint32_t domid)
+{
+    return libxl_domain_pause(ctx, domid, NULL);
+}
+static inline int libxl_domain_unpause_0x041200(
+    libxl_ctx *ctx, uint32_t domid)
+{
+    return libxl_domain_unpause(ctx, domid, NULL);
+}
+#define libxl_domain_pause libxl_domain_pause_0x041200
+#define libxl_domain_unpause libxl_domain_unpause_0x041200
+#endif
+
 
 int libxl_domain_core_dump(libxl_ctx *ctx, uint32_t domid,
                            const char *filename,
diff --git a/tools/libxl/libxl_colo_restore.c b/tools/libxl/libxl_colo_restore.c
index 0c535bd95d..aaa70552b8 100644
--- a/tools/libxl/libxl_colo_restore.c
+++ b/tools/libxl/libxl_colo_restore.c
@@ -853,7 +853,7 @@  static void colo_unpause_svm(libxl__egc *egc,
     EGC_GC;
 
     /* We have enabled secondary vm's logdirty, so we can unpause it now */
-    rc = libxl_domain_unpause(CTX, domid);
+    rc = libxl__domain_unpause(gc, domid);
     if (rc) {
         LOGD(ERROR, domid, "cannot unpause secondary vm");
         goto out;
diff --git a/tools/libxl/libxl_colo_save.c b/tools/libxl/libxl_colo_save.c
index 3247cce3a7..1d261a1639 100644
--- a/tools/libxl/libxl_colo_save.c
+++ b/tools/libxl/libxl_colo_save.c
@@ -480,7 +480,7 @@  static void colo_preresume_cb(libxl__egc *egc,
      * no disk migration.
      */
     if (css->paused) {
-        rc = libxl_domain_unpause(CTX, dss->domid);
+        rc = libxl__domain_unpause(gc, dss->domid);
         if (rc) {
             LOGD(ERROR, dss->domid, "cannot unpause primary vm");
             goto out;
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 5fe25b56f5..00da59153d 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -2402,7 +2402,7 @@  static void stubdom_pvqemu_cb(libxl__egc *egc,
         goto out;
     }
 
-    rc = libxl_domain_unpause(CTX, dm_domid);
+    rc = libxl__domain_unpause(gc, dm_domid);
     if (rc) goto out;
 
     sdss->xswait.ao = ao;
diff --git a/tools/libxl/libxl_domain.c b/tools/libxl/libxl_domain.c
index 11a29b235b..1c313005db 100644
--- a/tools/libxl/libxl_domain.c
+++ b/tools/libxl/libxl_domain.c
@@ -557,18 +557,18 @@  int libxl_domain_suspend_only(libxl_ctx *ctx, uint32_t domid,
     return AO_CREATE_FAIL(rc);
 }
 
-int libxl_domain_pause(libxl_ctx *ctx, uint32_t domid)
+int libxl_domain_pause(libxl_ctx *ctx, uint32_t domid,
+                       const libxl_asyncop_how *ao_how)
 {
-    int ret;
-    GC_INIT(ctx);
-    ret = xc_domain_pause(ctx->xch, domid);
-    if (ret<0) {
+    AO_CREATE(ctx, domid, ao_how);
+    int r;
+    r = xc_domain_pause(ctx->xch, domid);
+    if (r < 0) {
         LOGED(ERROR, domid, "Pausing domain");
-        GC_FREE;
-        return ERROR_FAIL;
+        return AO_CREATE_FAIL(ERROR_FAIL);
     }
-    GC_FREE;
-    return 0;
+    libxl__ao_complete(egc, ao, 0);
+    return AO_INPROGRESS;
 }
 
 int libxl_domain_core_dump(libxl_ctx *ctx, uint32_t domid,
@@ -593,10 +593,9 @@  int libxl_domain_core_dump(libxl_ctx *ctx, uint32_t domid,
     return AO_INPROGRESS;
 }
 
-int libxl_domain_unpause(libxl_ctx *ctx, uint32_t domid)
+int libxl__domain_unpause(libxl__gc *gc, libxl_domid domid)
 {
-    GC_INIT(ctx);
-    int ret, rc = 0;
+    int r, rc;
 
     libxl_domain_type type = libxl__domain_type(gc, domid);
     if (type == LIBXL_DOMAIN_TYPE_INVALID) {
@@ -612,16 +611,33 @@  int libxl_domain_unpause(libxl_ctx *ctx, uint32_t domid)
             goto out;
         }
     }
-    ret = xc_domain_unpause(ctx->xch, domid);
-    if (ret<0) {
+    r = xc_domain_unpause(CTX->xch, domid);
+    if (r < 0) {
         LOGED(ERROR, domid, "Unpausing domain");
         rc = ERROR_FAIL;
+        goto out;
     }
- out:
-    GC_FREE;
+    rc = 0;
+out:
     return rc;
 }
 
+int libxl_domain_unpause(libxl_ctx *ctx, uint32_t domid,
+                         const libxl_asyncop_how *ao_how)
+{
+    AO_CREATE(ctx, domid, ao_how);
+    int rc = 0;
+
+    rc = libxl__domain_unpause(gc, domid);
+    if (rc) goto out;
+
+    libxl__ao_complete(egc, ao, rc);
+    return AO_INPROGRESS;
+
+ out:
+    return AO_CREATE_FAIL(rc);
+}
+
 int libxl__domain_pvcontrol_available(libxl__gc *gc, uint32_t domid)
 {
     libxl_ctx *ctx = libxl__gc_owner(gc);
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index cf9287c488..7bd08032cf 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -4108,6 +4108,7 @@  _hidden void libxl__remus_teardown(libxl__egc *egc,
                                    int rc);
 _hidden void libxl__remus_restore_setup(libxl__egc *egc,
                                         libxl__domain_create_state *dcs);
+_hidden int libxl__domain_unpause(libxl__gc *, libxl_domid domid);
 
 
 /*
diff --git a/tools/xl/xl_migrate.c b/tools/xl/xl_migrate.c
index 1f0e87df50..22f0429b84 100644
--- a/tools/xl/xl_migrate.c
+++ b/tools/xl/xl_migrate.c
@@ -394,7 +394,7 @@  static void migrate_receive(int debug, int daemonize, int monitor,
             /* The guest is running after failover in COLO mode */
             exit(rc ? -ERROR_FAIL: 0);
 
-        rc = libxl_domain_unpause(ctx, domid);
+        rc = libxl_domain_unpause(ctx, domid, NULL);
         if (rc)
             fprintf(stderr, "migration target (%s): "
                     "Failed to unpause domain %s (id: %u):%d\n",
@@ -429,7 +429,7 @@  static void migrate_receive(int debug, int daemonize, int monitor,
     }
 
     if (!pause_after_migration) {
-        rc = libxl_domain_unpause(ctx, domid);
+        rc = libxl_domain_unpause(ctx, domid, NULL);
         if (rc) goto perhaps_destroy_notify_rc;
     }
 
diff --git a/tools/xl/xl_saverestore.c b/tools/xl/xl_saverestore.c
index 9afeadeeb2..5c70e2e874 100644
--- a/tools/xl/xl_saverestore.c
+++ b/tools/xl/xl_saverestore.c
@@ -150,7 +150,7 @@  static int save_domain(uint32_t domid, const char *filename, int checkpoint,
     }
     else if (leavepaused || checkpoint) {
         if (leavepaused)
-            libxl_domain_pause(ctx, domid);
+            libxl_domain_pause(ctx, domid, NULL);
         libxl_domain_resume(ctx, domid, 1, 0);
     }
     else
diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c
index a1d633795c..419bf780a4 100644
--- a/tools/xl/xl_vmcontrol.c
+++ b/tools/xl/xl_vmcontrol.c
@@ -34,12 +34,12 @@  static int fd_lock = -1;
 
 static void pause_domain(uint32_t domid)
 {
-    libxl_domain_pause(ctx, domid);
+    libxl_domain_pause(ctx, domid, NULL);
 }
 
 static void unpause_domain(uint32_t domid)
 {
-    libxl_domain_unpause(ctx, domid);
+    libxl_domain_unpause(ctx, domid, NULL);
 }
 
 static void destroy_domain(uint32_t domid, int force)
@@ -972,7 +972,7 @@  int create_domain(struct domain_create *dom_info)
     }
 
     if (!paused)
-        libxl_domain_unpause(ctx, domid);
+        libxl_domain_unpause(ctx, domid, NULL);
 
     ret = domid; /* caller gets success in parent */
     if (!daemonize && !monitor)