Re: [Xen-devel] [PATCH: Lazy Restore] xl restore: Add a new "-l" lazy switch.

[Dushyant Behl <myselfdushyantbehl@xxxxxxxxx>]

Thank you very much for your feedback. I have mentioned the comments
inline, Please have a look.
If something is not in place or anything else is missing from my side
please mention.

On Mon, Jun 2, 2014 at 9:34 PM, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> On Thu, 2014-05-29 at 00:34 +0530, Dushyant Behl wrote:
>> @@ -1470,6 +1470,15 @@ int xc_domain_restore(xc_interface *xch, int io_fd, 
>> uint32_t dom,
>>
>>      DPRINTF("%s: starting restore of new domid %u", __func__, dom);
>>
>> +    if ( lazy ) {
>
> Coding style for libxc (see toplevel CODING_STYLE doc) has { on the next
> line under the if.

I checked with the CODING_STYLE doc and the block structure mentioned is
the same followed by me ( opening bracket on the same line as the statement).
The only exception that is mentioned to have opening bracket for a block on
a different line is a function.

>
> Eventually you will need to target this work on top of the migration v2
> work being done by Andrew Cooper, see
> http://lists.xen.org/archives/html/xen-devel/2014-04/msg01377.html
>
> You might find it easier to start on top of those patches anyway -- they
> are a far cleaner base to work from.

Sure, I'll try to work on top of those changes.

>
>> +        DPRINTF("xc: lazy switch enabled for restore\n");
>> +
>> +        if ( !hvm ) {
>> +            PERROR("xc: lazy restore called for non HVM guest");
>> +            return 1;
>> +        }
>> +    }
>> +
>>      pagebuf_init(&pagebuf);
>>      memset(&tailbuf, 0, sizeof(tailbuf));
>>      tailbuf.ishvm = hvm;
>> diff --git a/tools/libxc/xenguest.h b/tools/libxc/xenguest.h
>> index 1f216cd..269275a 100644
>> --- a/tools/libxc/xenguest.h
>> +++ b/tools/libxc/xenguest.h
>> @@ -126,7 +126,7 @@ int xc_domain_restore(xc_interface *xch, int io_fd, 
>> uint32_t dom,
>>                        unsigned long *console_mfn, domid_t console_domid,
>>                        unsigned int hvm, unsigned int pae, int superpages,
>>                        int no_incr_generationid, int checkpointed_stream,
>> -                      unsigned long *vm_generationid_addr,
>> +                      int lazy, unsigned long *vm_generationid_addr,
>>                        struct restore_callbacks *callbacks);
>>  /**
>>   * xc_domain_restore writes a file to disk that contains the device
>> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
>> index d015cf4..f599280 100644
>> --- a/tools/libxl/libxl_create.c
>> +++ b/tools/libxl/libxl_create.c
>> @@ -1348,8 +1348,8 @@ static void domain_create_cb(libxl__egc *egc,
>>                               int rc, uint32_t domid);
>>
>>  static int do_domain_create(libxl_ctx *ctx, libxl_domain_config *d_config,
>> -                            uint32_t *domid,
>> -                            int restore_fd, int checkpointed_stream,
>> +                            uint32_t *domid, int restore_fd,
>> +                            const libxl_domain_restore_params 
>> *restore_params,
>>                              const libxl_asyncop_how *ao_how,
>>                              const libxl_asyncprogress_how *aop_console_how)
>>  {
>> @@ -1361,10 +1361,14 @@ static int do_domain_create(libxl_ctx *ctx, 
>> libxl_domain_config *d_config,
>>      cdcs->dcs.guest_config = d_config;
>>      cdcs->dcs.restore_fd = restore_fd;
>>      cdcs->dcs.callback = domain_create_cb;
>> -    cdcs->dcs.checkpointed_stream = checkpointed_stream;
>>      libxl__ao_progress_gethow(&cdcs->dcs.aop_console_how, aop_console_how);
>>      cdcs->domid_out = domid;
>>
>> +    if (restore_params) {
>> +        cdcs->dcs.checkpointed_stream = restore_params->checkpointed_stream;
>
> If !restore_params then cdcs->dcs.checkpointed_stream is no longer
> explicitly initialised. Have you confirmed that it is zeroed somewhere?

Actually the struct libxl__app_domain_create_state 'cdcs' is initialized
using the macro GCNEW which calls libxl__zalloc to allocate and zero out
the memory contents of the structure, so the variables would be
initialized to zero.

>> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
>> index 52f1aa9..6c8355c 100644
>> --- a/tools/libxl/libxl_types.idl
>> +++ b/tools/libxl/libxl_types.idl
>> @@ -282,6 +282,7 @@ libxl_domain_create_info = Struct("domain_create_info",[
>>
>>  libxl_domain_restore_params = Struct("domain_restore_params", [
>>      ("checkpointed_stream", integer),
>> +    ("lazy", integer),
>
> Changes to the libxl interface require a LIBXL_HAVE #define in libxl.h,
> see the comment regarding API compatibility in that file.
>

I have created a new macro LIBXL_HAVE_RESTORE_PARAMS which is
set when the structure libxl_domain_restore_params has lazy as an attribute.

>> @@ -3957,6 +3966,8 @@ int main_restore(int argc, char **argv)
>>      case 'A':
>>          vnc = vncautopass = 1;
>>          break;
>> +    case 'l':
>> +        lazy = 1;
>
> missing "break;"

I missed it the first time, added now.

>
>> diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
>> index 4279b9f..0405c6e 100644
>> --- a/tools/libxl/xl_cmdtable.c
>> +++ b/tools/libxl/xl_cmdtable.c
>> @@ -167,6 +167,7 @@ struct cmd_spec cmd_table[] = {
>>        "Restore a domain from a saved state",
>>        "[options] [<ConfigFile>] <CheckpointFile>",
>>        "-h                       Print this help.\n"
>> +      "-l                       Enable lazy restore.\n"
>
> As Konrad says this also needs documentation, at least in the xl man
> page.

Sure, I'll change the documentation accordingly.

Thanks,
Dushyant Behl

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