WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

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

To: Ian Campbell <ian.campbell@xxxxxxxxxx>
Subject: [Xen-devel] Re: [PATCH] tools: cleanup domain save switch_qemu_logdirty callback
From: Brendan Cully <brendan@xxxxxxxxx>
Date: Tue, 19 Oct 2010 14:12:30 -0700
Cc: xen-devel@xxxxxxxxxxxxxxxxxxx, Stefano Stabellini <Stefano.Stabellini@xxxxxxxxxxxxx>
Delivery-date: Tue, 19 Oct 2010 14:13:18 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <0b5d85ea10f8fff3f654.1287476519@xxxxxxxxxxxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
Mail-followup-to: Ian Campbell <ian.campbell@xxxxxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxx, Stefano Stabellini <Stefano.Stabellini@xxxxxxxxxxxxx>
References: <1287044260.2003.5491.camel@xxxxxxxxxxxxxxxxxxxxxx> <0b5d85ea10f8fff3f654.1287476519@xxxxxxxxxxxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Mutt/1.5.21 (2010-09-15)
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