Re: [Xen-devel] [PATCH] xl save but leave domain paused

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2013-07-03 at 09:08 +0100, Ian Murray wrote:
> 
> 
> 
> 
> 
> 
> >________________________________
> > From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> >To: Ian Murray <murrayie@xxxxxxxxxxx> 
> >Cc: Ian.Campbell@xxxxxxxxxx; xen-devel@xxxxxxxxxxxxx 
> >Sent: Wednesday, 3 July 2013, 0:55
> >Subject: Re: [Xen-devel] [PATCH] xl save but leave domain paused
> > 
> >
> 
> Thanks for your feedback.
> 
> 
> 
> >On 03/07/2013 00:34, Ian Murray wrote:
> >> New feature to allow xl save to leave a domain paused after its
> >> memory has been saved. This is to allow disk snapshots of domU
> >> to be taken that exactly correspond to the memory state at save time.
> >> Once the snapshot(s) have been taken or whatever, the domain can be
> >> unpaused in the usual manner.
> >>
> >> Usage:
> >>   xl save -p <domid> <filespec>
> >>
> >> Signed-off-by: Ian Murray <murrayie@xxxxxxxxxxx>
> >> ---
> >>  tools/libxl/xl_cmdimpl.c  |   16 ++++++++++++----
> >>  tools/libxl/xl_cmdtable.c |    4 +++-
> >>  2 files changed, 15 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> >> index 8a478ba..670620b 100644
> >> --- a/tools/libxl/xl_cmdimpl.c
> >> +++ b/tools/libxl/xl_cmdimpl.c
> >> @@ -3266,7 +3266,7 @@ static void save_domain_core_writeconfig(int fd, 
> >> const char *source,
> >>  }
> >>  
> >>  static int save_domain(uint32_t domid, const char *filename, int 
> >> checkpoint,
> >> -                const char *override_config_file)
> >> +                int leavepaused, const char *override_config_file)
> >
> >#include <stdbool.h> and use bool rather than int.  Also, re-align the
> >second line arguments which was misaligned when the function was made static
> 
> I used an int for consistency, as this was how the checkpoint was
> done. I can change it if you like, but it is going to be inconsistent
> with checkpoint. I could do checkpoint as well but that would be
> fixing code that isn't broken, which is a bit out of scope and makes
> the patch footprint bigger.

I don't think this is necessary unless you really want to. If
xl_cmdimpl.c was already using bool in some places it might be more
worthwhile, but currently it doesn't use it at all.

IMHO any overall change to use bool should be in a separate patch, but
there is no onus on you to make this change unless you want to.

> As for the misalignment, is there a style guide or should I just look
> round some more? I just re-used what was there, which wasn't helpful
> given it was already broke. I don't want to copy any more OP's
> mis-alignment mistakes. :)

tools/libxl/CODING_STYLE covers this code. 

There is also a top-level CODING_STYLE which sometimes applies when
there is no more specific CODING_STYLE documented (although see the
caveats at the top of the file)

> >>  {
> >>      int fd;
> >>      uint8_t *config_data;
> >> @@ -3293,8 +3293,12 @@ static int save_domain(uint32_t domid, const char 
> >> *filename, int checkpoint,
> >>      if (rc < 0)
> >>          fprintf(stderr, "Failed to save domain, resuming domain\n");
> >>  
> >> -    if (checkpoint || rc < 0)
> >> +    if (leavepaused || checkpoint || rc < 0) {
> >> +        if (leavepaused && !(rc < 0)) {
> >> +            libxl_domain_pause(ctx, domid);
> >> +        }
> >>          libxl_domain_resume(ctx, domid, 1, 0);
> >> +    }
> >
> >This logic is quite opaque.  It would be clearer to have
> >
> >if (leavepaused && rc == 0)
> >  pause()
> >else if (checkpoint ...
> 
> In itself, this isn't correct logic because the resume has to occur for both 
> checkpointing or pausing.
> 
> You would end up with
> 
> if (leavepaused && ...) {
> 
>     pause()
>     resume()
> 
> } else if (checkpoint && ...) {
>     resume() 
> 
> }
    else if ( rc < 0 ) {
      resume()
too...

> Is this preferable? I tend to avoid repeating code if I can help it
> esp in if statements, although perhaps it does read easier.

Both variants have their upside and downside IMHO.

What about refactoring the failure case (rc < 0) out from the success
cases?
    if (rc < 0) {
        resume(...)
    } else if (leavepaused || checkpoint) {
        if (leavepaused) pause(...)
        resume(...)
    }

It's still a repetitive but "error" and "not error" are somewhat
different cases IYSWIM.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel