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

[Ian Murray <murrayie@xxxxxxxxxxx>]

----- Original Message -----
> From: Ian Campbell <Ian.Campbell@xxxxxxxxxx>
> To: Ian Murray <murrayie@xxxxxxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>; "xen-devel@xxxxxxxxxxxxx" 
> <xen-devel@xxxxxxxxxxxxx>
> Sent: Wednesday, 3 July 2013, 9:38
> Subject: Re: [Xen-devel] [PATCH] xl save but leave domain paused
> 
> 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.
> 

No, I will leave as is, if it's all the same.


>>  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)
) 
Thanks, I will take a look. 

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

Possibly an exercise in cat skinning, but I do see the logic of your logic ( ;) 
), so am happy to go with it.

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

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