[Xen-devel] Re: [PATCH] tools: cleanup domain save switch_qemu_logdirty callback

[Brendan Cully <brendan@xxxxxxxxx>]

On Tuesday, 19 October 2010 at 09:21, Ian Campbell wrote:
> # HG changeset patch
> # User Ian Campbell <ian.campbell@xxxxxxxxxx>
> # Date 1287476238 -3600
> # Node ID 0b5d85ea10f8fff3f654c564c0e66900e83e8012
> # Parent  ca91b58834a35c70f33f6ceb28c02c60f190b6ed
> tools: cleanup domain save switch_qemu_logdirty callback
> 
> Move the function into struct save_callbacks with the others and add
> the void *closure to the callback arguments.
> 
> Add and propagate an error return code from the callback.
> 
> Use this in libxl to pass the save context to
> libxl__domain_suspend_common_switch_qemu_logdirty allowing us to reuse
> the parent's xenstore handle, gc context etc.
> 
> Also add an apparently missing libxl__free_all to
> libxl__domain_suspend_common.
> 
> Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> ---
> Changes from v1:
>   Compile test from missed call to xc_domain_save in
>   tools/python/xen/lowlevel/checkpoint/libcheckpoint.c
> Changes from v2:
>   Check for missing callback and return error.
>   Similarly update tools/libxc/ia64/xc_ia64_linux_save.c
>   Add return value to callback and propagate failure.
>   Update comment to indicate that closure is last argument to callback
>     (all existing callbacks only had the one argument so the first/last
>      distinction is moot)

This reads well, but I don't understand the purpose of all the return
code juggling you've done in libcheckpoint.c here:

> diff -r ca91b58834a3 -r 0b5d85ea10f8 
> tools/python/xen/lowlevel/checkpoint/libcheckpoint.c
> --- a/tools/python/xen/lowlevel/checkpoint/libcheckpoint.c    Tue Oct 19 
> 09:07:47 2010 +0100
> +++ b/tools/python/xen/lowlevel/checkpoint/libcheckpoint.c    Tue Oct 19 
> 09:17:18 2010 +0100
> @@ -164,9 +164,9 @@ void checkpoint_close(checkpoint_state* 
>  
>  /* we toggle logdirty ourselves around the xc_domain_save call --
>   * it avoids having to pass around checkpoint_state */
> -static void noop_switch_logdirty(int domid, unsigned enable)
> +static int noop_switch_logdirty(int domid, unsigned enable, void *data)
>  {
> -    return;
> +    return 0;
>  }
>  
>  int checkpoint_start(checkpoint_state* s, int fd,
> @@ -185,12 +185,13 @@ int checkpoint_start(checkpoint_state* s
>      hvm = s->domtype > dt_pv;
>      if (hvm) {
>         flags |= XCFLAGS_HVM;
> -       if ((rc = switch_qemu_logdirty(s, 1)))
> -           return rc;
> +       if (!switch_qemu_logdirty(s, 1))
> +           return -1;
>      }
>  
> -    rc = xc_domain_save(s->xch, fd, s->domid, 0, 0, flags, callbacks, hvm,
> -       noop_switch_logdirty);
> +    callbacks->switch_qemu_logdirty = noop_switch_logdirty;
> +
> +    rc = xc_domain_save(s->xch, fd, s->domid, 0, 0, flags, callbacks, hvm);
>  
>      if (hvm)
>         switch_qemu_logdirty(s, 0);
> @@ -540,7 +541,7 @@ static int switch_qemu_logdirty(checkpoi
>      strcpy(tail, "ret");
>      if (!xs_watch(s->xsh, path, "qemu-logdirty-ret")) {
>         s->errstr = "error watching qemu logdirty return";
> -       return -1;
> +       return 1;
>      }
>      /* null fire. XXX unify with shutdown watch! */
>      vec = xs_read_watch(s->xsh, &len);
> @@ -550,7 +551,7 @@ static int switch_qemu_logdirty(checkpoi
>      cmd = enable ? "enable" : "disable";
>      if (!xs_write(s->xsh, XBT_NULL, path, cmd, strlen(cmd))) {
>         s->errstr = "error signalling qemu logdirty";
> -       return -1;
> +       return 1;
>      }
>  
>      vec = xs_read_watch(s->xsh, &len);
> @@ -564,7 +565,7 @@ static int switch_qemu_logdirty(checkpoi
>         if (len)
>             free(response);
>         s->errstr = "qemu logdirty command failed";
> -       return -1;
> +       return 1;
>      }
>      free(response);
>      fprintf(stderr, "qemu logdirty mode: %s\n", cmd);

I haven't compiled or run this, I'll wait for it to hit the tree.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel