[PATCH] libxl: add a dummy ao_how to libxl_domain_core_dump

[PATCH] libxl: add a dummy ao_how to libxl_domain_core_dump

[Ian Campbell]

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1334587303 -3600
# Node ID a20c43492fbff622aa9746404981628b498e0711
# Parent  f95c8fe372d35fdf8937750e9271ea76ff991488
libxl: add a dummy ao_how to libxl_domain_core_dump

Although this function is not currently slow it may become so in the future
(this also depends somewhat on the size of the guest).  Therefore arrange for
it to take an ao_how which it completes immediately.  This will allow us to
make it asynchronous in the future without breaking API compatibility.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---

Requires Ian Jackson's "libxl: Allow AO_GC and EGC_GC even if not used"

diff -r f95c8fe372d3 -r a20c43492fbf tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Fri Apr 13 19:40:07 2012 +0100
+++ b/tools/libxl/libxl.c	Mon Apr 16 15:41:43 2012 +0100
@@ -627,16 +627,22 @@ int libxl_domain_pause(libxl_ctx *ctx, u
 }
 
 int libxl_domain_core_dump(libxl_ctx *ctx, uint32_t domid,
-                           const char *filename)
+                           const char *filename,
+                           const libxl_asyncop_how *ao_how)
 {
-    int ret;
+    AO_CREATE(ctx, domid, ao_how);
+    int ret, rc = 0;
+
     ret = xc_domain_dumpcore(ctx->xch, domid, filename);
     if (ret<0) {
         LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "core dumping domain %d to %s",
                      domid, filename);
-        return ERROR_FAIL;
+        rc = ERROR_FAIL;
     }
-    return 0;
+
+    libxl__ao_complete(egc, ao, rc);
+
+    return AO_INPROGRESS;
 }
 
 int libxl_domain_unpause(libxl_ctx *ctx, uint32_t domid)
diff -r f95c8fe372d3 -r a20c43492fbf tools/libxl/libxl.h
--- a/tools/libxl/libxl.h	Fri Apr 13 19:40:07 2012 +0100
+++ b/tools/libxl/libxl.h	Mon Apr 16 15:41:43 2012 +0100
@@ -509,7 +509,9 @@ int libxl_domain_rename(libxl_ctx *ctx, 
 int libxl_domain_pause(libxl_ctx *ctx, uint32_t domid);
 int libxl_domain_unpause(libxl_ctx *ctx, uint32_t domid);
 
-int libxl_domain_core_dump(libxl_ctx *ctx, uint32_t domid, const char *filename);
+int libxl_domain_core_dump(libxl_ctx *ctx, uint32_t domid,
+                           const char *filename,
+                           const libxl_asyncop_how *ao_how);
 
 int libxl_domain_setmaxmem(libxl_ctx *ctx, uint32_t domid, uint32_t target_memkb);
 int libxl_set_memory_target(libxl_ctx *ctx, uint32_t domid, int32_t target_memkb, int relative, int enforce);
diff -r f95c8fe372d3 -r a20c43492fbf tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c	Fri Apr 13 19:40:07 2012 +0100
+++ b/tools/libxl/xl_cmdimpl.c	Mon Apr 16 15:41:43 2012 +0100
@@ -1276,7 +1276,7 @@ static int handle_domain_death(libxl_ctx
             LOG("failed to construct core dump path");
         } else {
             LOG("dumping core to %s", corefile);
-            rc=libxl_domain_core_dump(ctx, domid, corefile);
+            rc=libxl_domain_core_dump(ctx, domid, corefile, NULL);
             if (rc) LOG("core dump failed (rc=%d).", rc);
         }
         /* No point crying over spilled milk, continue on failure. */
@@ -2893,7 +2893,7 @@ static void core_dump_domain(const char 
 {
     int rc;
     find_domain(domain_spec);
-    rc=libxl_domain_core_dump(ctx, domid, filename);
+    rc=libxl_domain_core_dump(ctx, domid, filename, NULL);
     if (rc) { fprintf(stderr,"core dump failed (rc=%d)\n",rc);exit(-1); }
 }

Re: [PATCH] libxl: add a dummy ao_how to libxl_domain_core_dump

[Ian Jackson]

Ian Campbell writes ("[PATCH] libxl: add a dummy ao_how to libxl_domain_core_dump"):
> libxl: add a dummy ao_how to libxl_domain_core_dump
> 
> Although this function is not currently slow it may become so in the future
> (this also depends somewhat on the size of the guest).  Therefore arrange for
> it to take an ao_how which it completes immediately.  This will allow us to
> make it asynchronous in the future without breaking API compatibility.
...
>      ret = xc_domain_dumpcore(ctx->xch, domid, filename);
>      if (ret<0) {
>          LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "core dumping domain %d to %s",
>                       domid, filename);
> -        return ERROR_FAIL;
> +        rc = ERROR_FAIL;

While you're doing that, did you want to change it to use "goto out" ?

>      }
> -    return 0;
> +
> +    libxl__ao_complete(egc, ao, rc);
> +
> +    return AO_INPROGRESS;

Ian.

Re: [PATCH] libxl: add a dummy ao_how to libxl_domain_core_dump

[Ian Campbell]

On Mon, 2012-04-16 at 16:50 +0100, Ian Jackson wrote:
> Ian Campbell writes ("[PATCH] libxl: add a dummy ao_how to libxl_domain_core_dump"):
> > libxl: add a dummy ao_how to libxl_domain_core_dump
> > 
> > Although this function is not currently slow it may become so in the future
> > (this also depends somewhat on the size of the guest).  Therefore arrange for
> > it to take an ao_how which it completes immediately.  This will allow us to
> > make it asynchronous in the future without breaking API compatibility.
> ...
> >      ret = xc_domain_dumpcore(ctx->xch, domid, filename);
> >      if (ret<0) {
> >          LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "core dumping domain %d to %s",
> >                       domid, filename);
> > -        return ERROR_FAIL;
> > +        rc = ERROR_FAIL;
> 
> While you're doing that, did you want to change it to use "goto out" ?

wouldn't the "out:" be immediately after this immediately following
closing brace?

> >      }
> > -    return 0;
> > +
> > +    libxl__ao_complete(egc, ao, rc);
> > +
> > +    return AO_INPROGRESS;
> 
> Ian.

Re: [PATCH] libxl: add a dummy ao_how to libxl_domain_core_dump

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH] libxl: add a dummy ao_how to libxl_domain_core_dump"):
> On Mon, 2012-04-16 at 16:50 +0100, Ian Jackson wrote:
> > Ian Campbell writes ("[PATCH] libxl: add a dummy ao_how to libxl_domain_core_dump"):
> > > libxl: add a dummy ao_how to libxl_domain_core_dump
> > > -        return ERROR_FAIL;
> > > +        rc = ERROR_FAIL;
> > 
> > While you're doing that, did you want to change it to use "goto out" ?
> 
> wouldn't the "out:" be immediately after this immediately following
> closing brace?

I think it would look like this:

            
> > > +        rc = ERROR_FAIL;
               goto out;
> > >      }
> > > -    return 0;
> > > +
           rc = 0;
        out:
> > > +    libxl__ao_complete(egc, ao, rc);
> > > +
> > > +    return AO_INPROGRESS;

And removing the initialisation of rc at the top of the function.

This pattern is the usual one when we have an operation which should
fail, and stop executing, when any sub-operations fail.

Setting rc=0 at the top and setting it to ERROR_FAIL as we go, without
an error exit, is what we do if we want to blunder on after errors (eg
the remove files patch).

Ian.

Re: [PATCH] libxl: add a dummy ao_how to libxl_domain_core_dump

[Ian Campbell]

On Mon, 2012-04-16 at 17:31 +0100, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH] libxl: add a dummy ao_how to libxl_domain_core_dump"):
> > On Mon, 2012-04-16 at 16:50 +0100, Ian Jackson wrote:
> > > Ian Campbell writes ("[PATCH] libxl: add a dummy ao_how to libxl_domain_core_dump"):
> > > > libxl: add a dummy ao_how to libxl_domain_core_dump
> > > > -        return ERROR_FAIL;
> > > > +        rc = ERROR_FAIL;
> > > 
> > > While you're doing that, did you want to change it to use "goto out" ?
> > 
> > wouldn't the "out:" be immediately after this immediately following
> > closing brace?
> 
> I think it would look like this:

Make sense.

8<--------------------------------

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1334595420 -3600
# Node ID 8d92d1f34921c8675d85c74aa36e319c9451f68f
# Parent  f95c8fe372d35fdf8937750e9271ea76ff991488
libxl: add a dummy ao_how to libxl_domain_core_dump

Although this function is not currently slow it may become so in the future
(this also depends somewhat on the size of the guest).  Therefore arrange for
it to take an ao_how which it completes immediately.  This will allow us to
make it asynchronous in the future without breaking API compatibility.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---

Requires Ian Jackson's "libxl: Allow AO_GC and EGC_GC even if not used"

diff -r f95c8fe372d3 -r 8d92d1f34921 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Fri Apr 13 19:40:07 2012 +0100
+++ b/tools/libxl/libxl.c	Mon Apr 16 17:57:00 2012 +0100
@@ -627,16 +627,26 @@ int libxl_domain_pause(libxl_ctx *ctx, u
 }
 
 int libxl_domain_core_dump(libxl_ctx *ctx, uint32_t domid,
-                           const char *filename)
+                           const char *filename,
+                           const libxl_asyncop_how *ao_how)
 {
-    int ret;
+    AO_CREATE(ctx, domid, ao_how);
+    int ret, rc;
+
     ret = xc_domain_dumpcore(ctx->xch, domid, filename);
     if (ret<0) {
         LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "core dumping domain %d to %s",
                      domid, filename);
-        return ERROR_FAIL;
+        rc = ERROR_FAIL;
+        goto out;
     }
-    return 0;
+
+    rc = 0;
+out:
+
+    libxl__ao_complete(egc, ao, rc);
+
+    return AO_INPROGRESS;
 }
 
 int libxl_domain_unpause(libxl_ctx *ctx, uint32_t domid)
diff -r f95c8fe372d3 -r 8d92d1f34921 tools/libxl/libxl.h
--- a/tools/libxl/libxl.h	Fri Apr 13 19:40:07 2012 +0100
+++ b/tools/libxl/libxl.h	Mon Apr 16 17:57:00 2012 +0100
@@ -509,7 +509,9 @@ int libxl_domain_rename(libxl_ctx *ctx, 
 int libxl_domain_pause(libxl_ctx *ctx, uint32_t domid);
 int libxl_domain_unpause(libxl_ctx *ctx, uint32_t domid);
 
-int libxl_domain_core_dump(libxl_ctx *ctx, uint32_t domid, const char *filename);
+int libxl_domain_core_dump(libxl_ctx *ctx, uint32_t domid,
+                           const char *filename,
+                           const libxl_asyncop_how *ao_how);
 
 int libxl_domain_setmaxmem(libxl_ctx *ctx, uint32_t domid, uint32_t target_memkb);
 int libxl_set_memory_target(libxl_ctx *ctx, uint32_t domid, int32_t target_memkb, int relative, int enforce);
diff -r f95c8fe372d3 -r 8d92d1f34921 tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c	Fri Apr 13 19:40:07 2012 +0100
+++ b/tools/libxl/xl_cmdimpl.c	Mon Apr 16 17:57:00 2012 +0100
@@ -1276,7 +1276,7 @@ static int handle_domain_death(libxl_ctx
             LOG("failed to construct core dump path");
         } else {
             LOG("dumping core to %s", corefile);
-            rc=libxl_domain_core_dump(ctx, domid, corefile);
+            rc=libxl_domain_core_dump(ctx, domid, corefile, NULL);
             if (rc) LOG("core dump failed (rc=%d).", rc);
         }
         /* No point crying over spilled milk, continue on failure. */
@@ -2893,7 +2893,7 @@ static void core_dump_domain(const char 
 {
     int rc;
     find_domain(domain_spec);
-    rc=libxl_domain_core_dump(ctx, domid, filename);
+    rc=libxl_domain_core_dump(ctx, domid, filename, NULL);
     if (rc) { fprintf(stderr,"core dump failed (rc=%d)\n",rc);exit(-1); }
 }

Re: [PATCH] libxl: add a dummy ao_how to libxl_domain_core_dump

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] [PATCH] libxl: add a dummy ao_how to libxl_domain_core_dump"):
> libxl: add a dummy ao_how to libxl_domain_core_dump

Unfortunately this depends on my __attribute__((unused)) patch which
isn't in tree yet...

Ian.

Re: [PATCH] libxl: add a dummy ao_how to libxl_domain_core_dump

[Ian Campbell]

On Tue, 2012-04-24 at 17:32 +0100, Ian Jackson wrote:
> Ian Campbell writes ("Re: [Xen-devel] [PATCH] libxl: add a dummy ao_how to libxl_domain_core_dump"):
> > libxl: add a dummy ao_how to libxl_domain_core_dump
> 
> Unfortunately this depends on my __attribute__((unused)) patch which
> isn't in tree yet...

Right, I said just below the commit message:
> Requires Ian Jackson's "libxl: Allow AO_GC and EGC_GC even if not
> used"

;-)

Re: [PATCH] libxl: add a dummy ao_how to libxl_domain_core_dump

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] [PATCH] libxl: add a dummy ao_how to libxl_domain_core_dump"):
> libxl: add a dummy ao_how to libxl_domain_core_dump

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

I have added this to my child process handling series.  It will go in
with the rest of those.

Ian.