[PATCH] xl save but leave domain paused

[PATCH] xl save but leave domain paused

[Ian Murray]

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@yahoo.co.uk>
---
 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)
 {
     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);
+    }
     else
         libxl_domain_destroy(ctx, domid, 0);
 
@@ -3838,12 +3842,16 @@ int main_save(int argc, char **argv)
     const char *filename;
     const char *config_filename = NULL;
     int checkpoint = 0;
+    int leavepaused = 0;
     int opt;
 
-    SWITCH_FOREACH_OPT(opt, "c", NULL, "save", 2) {
+    SWITCH_FOREACH_OPT(opt, "cp", NULL, "save", 2) {
     case 'c':
         checkpoint = 1;
         break;
+    case 'p':
+        leavepaused = 1;
+        break;
     }
 
     if (argc-optind > 3) {
@@ -3856,7 +3864,7 @@ int main_save(int argc, char **argv)
     if ( argc - optind >= 3 )
         config_filename = argv[optind + 2];
 
-    save_domain(domid, filename, checkpoint, config_filename);
+    save_domain(domid, filename, checkpoint, leavepaused, config_filename);
     return 0;
 }
 
diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
index 44b42b0..c429cea 100644
--- a/tools/libxl/xl_cmdtable.c
+++ b/tools/libxl/xl_cmdtable.c
@@ -142,7 +142,9 @@ struct cmd_spec cmd_table[] = {
       "Save a domain state to restore later",
       "[options] <Domain> <CheckpointFile> [<ConfigFile>]",
       "-h  Print this help.\n"
-      "-c  Leave domain running after creating the snapshot."
+      "-c  Leave domain running after creating the snapshot.\n"
+      "-p  Leave domain paused after creating the snapshot."
+
     },
     { "migrate",
       &main_migrate, 0, 1,
-- 
1.7.9.5

Re: [PATCH] xl save but leave domain paused

[Andrew Cooper]

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@yahoo.co.uk>
> ---
>  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

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

>      else
>          libxl_domain_destroy(ctx, domid, 0);
>  
> @@ -3838,12 +3842,16 @@ int main_save(int argc, char **argv)
>      const char *filename;
>      const char *config_filename = NULL;
>      int checkpoint = 0;
> +    int leavepaused = 0;
>      int opt;
>  
> -    SWITCH_FOREACH_OPT(opt, "c", NULL, "save", 2) {
> +    SWITCH_FOREACH_OPT(opt, "cp", NULL, "save", 2) {
>      case 'c':
>          checkpoint = 1;
>          break;
> +    case 'p':
> +        leavepaused = 1;
> +        break;
>      }
>  
>      if (argc-optind > 3) {
> @@ -3856,7 +3864,7 @@ int main_save(int argc, char **argv)
>      if ( argc - optind >= 3 )
>          config_filename = argv[optind + 2];
>  
> -    save_domain(domid, filename, checkpoint, config_filename);
> +    save_domain(domid, filename, checkpoint, leavepaused, config_filename);
>      return 0;
>  }
>  
> diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
> index 44b42b0..c429cea 100644
> --- a/tools/libxl/xl_cmdtable.c
> +++ b/tools/libxl/xl_cmdtable.c
> @@ -142,7 +142,9 @@ struct cmd_spec cmd_table[] = {
>        "Save a domain state to restore later",
>        "[options] <Domain> <CheckpointFile> [<ConfigFile>]",
>        "-h  Print this help.\n"
> -      "-c  Leave domain running after creating the snapshot."
> +      "-c  Leave domain running after creating the snapshot.\n"
> +      "-p  Leave domain paused after creating the snapshot."
> +

Needless whitespace

>      },
>      { "migrate",
>        &main_migrate, 0, 1,

Can you also patch the manpage, docs/man/xl.pod.1 to the same effect

~Andrew

Re: [PATCH] xl save but leave domain paused

[Ian Murray]

>________________________________
> From: Andrew Cooper <andrew.cooper3@citrix.com>
>To: Ian Murray <murrayie@yahoo.co.uk> 
>Cc: Ian.Campbell@citrix.com; xen-devel@lists.xen.org 
>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@yahoo.co.uk>
>> ---
>>  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.

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. :)


>
>>  {
>>      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() 

}

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

I used the !(rc<0) because I didn't want to make assumptions about the possible value of rc, given the rest of the code uses rc<0



>
>>      else
>>          libxl_domain_destroy(ctx, domid, 0);
>>  
>> @@ -3838,12 +3842,16 @@ int main_save(int argc, char **argv)
>>      const char *filename;
>>      const char *config_filename = NULL;
>>      int checkpoint = 0;
>> +    int leavepaused = 0;
>>      int opt;
>>  
>> -    SWITCH_FOREACH_OPT(opt, "c", NULL, "save", 2) {
>> +    SWITCH_FOREACH_OPT(opt, "cp", NULL, "save", 2) {
>>      case 'c':
>>          checkpoint = 1;
>>          break;
>> +    case 'p':
>> +        leavepaused = 1;
>> +        break;
>>      }
>>  
>>      if (argc-optind > 3) {
>> @@ -3856,7 +3864,7 @@ int main_save(int argc, char **argv)
>>      if ( argc - optind >= 3 )
>>          config_filename = argv[optind + 2];
>>  
>> -    save_domain(domid, filename, checkpoint, config_filename);
>> +    save_domain(domid, filename, checkpoint, leavepaused, config_filename);
>>      return 0;
>>  }
>>  
>> diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
>> index 44b42b0..c429cea 100644
>> --- a/tools/libxl/xl_cmdtable.c
>> +++ b/tools/libxl/xl_cmdtable.c
>> @@ -142,7 +142,9 @@ struct cmd_spec cmd_table[] = {
>>        "Save a domain state to restore later",
>>        "[options] <Domain> <CheckpointFile> [<ConfigFile>]",
>>        "-h  Print this help.\n"
>> -      "-c  Leave domain running after creating the snapshot."
>> +      "-c  Leave domain running after creating the snapshot.\n"
>> +      "-p  Leave domain paused after creating the snapshot."
>> +
>
>Needless whitespace

Sure. Slipped in by accident.



>
>>      },
>>      { "migrate",
>>        &main_migrate, 0, 1,
>
>Can you also patch the manpage, docs/man/xl.pod.1 to the same effect

Yeah, I forgot about that. Thanks.

Thanks again for your response.



>
>~Andrew
>
>_______________________________________________
>Xen-devel mailing list
>Xen-devel@lists.xen.org
>http://lists.xen.org/xen-devel
>
>
>

Re: [PATCH] xl save but leave domain paused

[Ian Campbell]

On Wed, 2013-07-03 at 09:08 +0100, Ian Murray wrote:
> 
> 
> 
> 
> 
> 
> >________________________________
> > From: Andrew Cooper <andrew.cooper3@citrix.com>
> >To: Ian Murray <murrayie@yahoo.co.uk> 
> >Cc: Ian.Campbell@citrix.com; xen-devel@lists.xen.org 
> >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@yahoo.co.uk>
> >> ---
> >>  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.

Re: [PATCH] xl save but leave domain paused

[Ian Murray]

----- Original Message -----
> From: Ian Campbell <Ian.Campbell@citrix.com>
> To: Ian Murray <murrayie@yahoo.co.uk>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>; "xen-devel@lists.xen.org" <xen-devel@lists.xen.org>
> 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@citrix.com>
>>  >To: Ian Murray <murrayie@yahoo.co.uk> 
>>  >Cc: Ian.Campbell@citrix.com; xen-devel@lists.xen.org 
>>  >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@yahoo.co.uk>
>>  >> ---
>>  >>  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@lists.xen.org
> http://lists.xen.org/xen-devel
>