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

[Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>]

On Tue, 2010-10-19 at 22:12 +0100, Brendan Cully wrote: 
> 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:

It makes the callback return code match the convention used by
xc_domain_save itself. It isn't necessarily the best convention but I
thought the overall consistency was more important.

Ian.

> 
> > 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