Re: [Xen-devel] [PATCH v7 08/18] tools/libxl: introduce libxl__domain_restore_device_model to load qemu state

[Wen Congyang <wency@xxxxxxxxxxxxxx>]

On 02/04/2016 03:40 AM, Wei Liu wrote:
> On Fri, Jan 29, 2016 at 01:27:24PM +0800, Wen Congyang wrote:
>> In normal migration, the qemu state is passed to qemu as a parameter.
>> With COLO, secondary vm is running. So we will do the following steps
>> at every checkpoint:
>> 1. suspend both primary vm and secondary vm
>> 2. sync the state
>> 3. resume both primary vm and secondary vm
>> Primary will send qemu's state in step2, and secondary's qemu should
>> read it and restore the state before it is resumed. We can not pass
>> the state to qemu as a parameter because secondary QEMU already started
>> at this point, so we introduce libxl__domain_restore_device_model() to
>> do it. This API MUST be called before resuming secondary vm.
>>
>> Signed-off-by: Yang Hongyang <hongyang.yang@xxxxxxxxxxxx>
>> Signed-off-by: Wen Congyang <wency@xxxxxxxxxxxxxx>
>> Cc: Anthony Perard <anthony.perard@xxxxxxxxxx>
>> ---
>>  tools/libxl/libxl_dom_save.c | 20 ++++++++++++++++++++
>>  tools/libxl/libxl_internal.h |  4 ++++
>>  tools/libxl/libxl_qmp.c      | 10 ++++++++++
>>  3 files changed, 34 insertions(+)
>>
>> diff --git a/tools/libxl/libxl_dom_save.c b/tools/libxl/libxl_dom_save.c
>> index cd2e7de..7383d2d 100644
>> --- a/tools/libxl/libxl_dom_save.c
>> +++ b/tools/libxl/libxl_dom_save.c
>> @@ -518,6 +518,26 @@ int 
>> libxl__restore_emulator_xenstore_data(libxl__domain_create_state *dcs,
>>      return rc;
>>  }
>>  
>> +int libxl__domain_restore_device_model(libxl__gc *gc, uint32_t domid,
>> +                                       const char *restore_file)
>> +{
>> +    int rc;
>> +
>> +    switch (libxl__device_model_version_running(gc, domid)) {
>> +    case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
>> +        /* Will never be supported. */
>> +        rc = ERROR_INVAL;
>> +        break;
> 
> I'm not entirely sure if this statement would be true. The function name
> is generic enough to indicate this case should be supported.
> 
> However, this function is not used anywhere in this series, so I don't
> know whether my comment makes sense.
> 
> One way of moving forward is to stick this patch to COLO series itself.
> Let's skip this in this prerequisite series.

OK, I will put it in the COLO series itself.
This API is used for COLO, and COLO requries the newest qemu with block 
replication.
The block replication is still in the way. The tranditional qemu doesn't support
block replication and it is hard to backport it.

> 
>> +    case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
>> +        rc = libxl__qmp_restore(gc, domid, restore_file);
>> +        break;
>> +    default:
>> +        rc = ERROR_INVAL;
>> +    }
>> +
>> +    return rc;
>> +}
>> +
>>  /*
>>   * Local variables:
>>   * mode: C
>> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
>> index fbd1acb..896c119 100644
>> --- a/tools/libxl/libxl_internal.h
>> +++ b/tools/libxl/libxl_internal.h
>> @@ -1117,6 +1117,8 @@ _hidden int libxl__domain_rename(libxl__gc *gc, 
>> uint32_t domid,
>>                                   const char *old_name, const char *new_name,
>>                                   xs_transaction_t trans);
>>  
>> +_hidden int libxl__domain_restore_device_model(libxl__gc *gc, uint32_t 
>> domid,
>> +                                               const char *restore_file);
>>  _hidden int libxl__domain_resume_device_model(libxl__gc *gc, uint32_t 
>> domid);
>>  
>>  _hidden const char *libxl__userdata_path(libxl__gc *gc, uint32_t domid,
>> @@ -1760,6 +1762,8 @@ _hidden int libxl__qmp_stop(libxl__gc *gc, int domid);
>>  _hidden int libxl__qmp_resume(libxl__gc *gc, int domid);
>>  /* Save current QEMU state into fd. */
>>  _hidden int libxl__qmp_save(libxl__gc *gc, int domid, const char *filename);
>> +/* Load current QEMU state from fd. */
> 
> This comment is wrong, it loads QEMU state from file, not fd.

will fix it in the next version.

Thanks
Wen Congyang

> 
>> +_hidden int libxl__qmp_restore(libxl__gc *gc, int domid, const char 
>> *filename);
>>  /* Set dirty bitmap logging status */
>>  _hidden int libxl__qmp_set_global_dirty_log(libxl__gc *gc, int domid, bool 
>> enable);
>>  _hidden int libxl__qmp_insert_cdrom(libxl__gc *gc, int domid, const 
>> libxl_device_disk *disk);
>> diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
>> index 714038b..eec8a44 100644
>> --- a/tools/libxl/libxl_qmp.c
>> +++ b/tools/libxl/libxl_qmp.c
>> @@ -905,6 +905,16 @@ int libxl__qmp_save(libxl__gc *gc, int domid, const 
>> char *filename)
>>                             NULL, NULL);
>>  }
>>  
>> +int libxl__qmp_restore(libxl__gc *gc, int domid, const char *state_file)
>> +{
>> +    libxl__json_object *args = NULL;
>> +
>> +    qmp_parameters_add_string(gc, &args, "filename", state_file);
>> +
>> +    return qmp_run_command(gc, domid, "xen-load-devices-state", args,
>> +                           NULL, NULL);
>> +}
>> +
> 
> This looks correct FWIW.
> 
>>  static int qmp_change(libxl__gc *gc, libxl__qmp_handler *qmp,
>>                        char *device, char *target, char *arg)
>>  {
>> -- 
>> 2.5.0
>>
>>
>>
>>
>> _______________________________________________
>> 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