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

[Ian Murray <murrayie@xxxxxxxxxxx>]

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

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@xxxxxxxxxxxxx
>http://lists.xen.org/xen-devel
>
>
>

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