Re: [Xen-devel] [PATCH v11 14/27] secondary vm suspend/resume/checkpoint code

[Wen Congyang <wency@xxxxxxxxxxxxxx>]

On 03/05/2016 01:11 AM, Ian Jackson wrote:
> Changlong Xie writes ("[PATCH v11 14/27] secondary vm 
> suspend/resume/checkpoint code"):
>> From: Wen Congyang <wency@xxxxxxxxxxxxxx>
>>
>> Secondary vm is running in colo mode. So we will do
>> the following things again and again:
> 
> I don't propose to review this in detail.  Skimreading it, it looks
> plausible.  I don't think a detailed review is needed.
> 
> I will review the changes to the core code.
> 
>> diff --git a/tools/libxc/xc_sr_common.h b/tools/libxc/xc_sr_common.h
>> index 5d9f497..2bfed64 100644
>> --- a/tools/libxc/xc_sr_common.h
>> +++ b/tools/libxc/xc_sr_common.h
>> @@ -184,10 +184,12 @@ struct xc_sr_context
>>       * migration stream
>>       * 0: Plain VM
>>       * 1: Remus
>> +     * 2: COLO
>>       */
>>      enum {
>>          MIG_STREAM_NONE, /* plain stream */
>>          MIG_STREAM_REMUS,
>> +        MIG_STREAM_COLO,
> 
> I think this shows that the duplicated list (in the comment, above the
> enum) is a mistake.  I would prefer it to be removed.

Do you mean remove the comments?

> 
>> +/* ===================== colo: common functions ===================== */

Add a blank line here? Will fix it in the next version.

>> +static void colo_enable_logdirty(libxl__colo_restore_state *crs, libxl__egc 
>> *egc)
> 
> Here's another missing blank line.  This seems to be a general theme:
> can you change this everywhere ?  Thanks.
> 
>> @@ -994,6 +1011,8 @@ static void domcreate_bootloader_done(libxl__egc *egc,
>>      const int restore_fd = dcs->restore_fd;
>>      libxl__domain_build_state *const state = &dcs->build_state;
>>      const int checkpointed_stream = dcs->restore_params.checkpointed_stream;
>> +    libxl__colo_restore_state *const crs = &dcs->crs;
>> +    libxl_domain_build_info *const info = &d_config->b_info;
>>  
>>      if (rc) {
>>          domcreate_rebuild_done(egc, dcs, rc);
>> @@ -1022,6 +1041,13 @@ static void domcreate_bootloader_done(libxl__egc *egc,
>>  
>>      /* Restore */
>>  
>> +    /* COLO only supports HVM now */
>> +    if (info->type != LIBXL_DOMAIN_TYPE_HVM &&
>> +        checkpointed_stream == LIBXL_CHECKPOINTED_STREAM_COLO) {
>> +        rc = ERROR_FAIL;
>> +        goto out;
> 
> Please log something here, or it may be very mysterious.

OK. Will add some comments to explain why only support pure HVM now.

> 
>> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
>> index 6307b71..48b4858 100644
>> --- a/tools/libxl/libxl_internal.h
>> +++ b/tools/libxl/libxl_internal.h
>> @@ -87,6 +87,8 @@
>> @@ -3468,7 +3464,6 @@ libxl__stream_read_inuse(const 
>> libxl__stream_read_state *stream)
>>      return stream->running;
>>  }
>>  
>> -
>>  struct libxl__domain_create_state {
>>      /* filled in by user */
>>      libxl__ao *ao;
>> @@ -3484,6 +3479,8 @@ struct libxl__domain_create_state {
> 
> Unintentional whitespace change.

Sorry for the mistake. Will fix it in the next version.

Thanks
Wen Congyang

> 
> 
> Ian.
> 
> 
> .
> 




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