Re: [PATCH] xl save but leave domain paused

[Andrew Cooper <andrew.cooper3@citrix.com>]

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