Re: [PATCH v4 1/5] define snapshot API

[Jim Fehlig <jfehlig@suse.com>]

Chun Yan Liu wrote:
>   
>>>> On 6/23/2014 at 07:25 PM, in message
>>>>         
> <1403522755-6894-2-git-send-email-bjzhang@suse.com>, Bamvor Jian Zhang
> <bjzhang@suse.com> wrote: 
>   
>> it includes two parts APIs: domain snapshot configuration file operation 
>> (load, store, delete, it base on Wei Liu's libxl-json api) and disk 
>> snapshot operation(create, delete, revert, including implementation 
>> details: choose qmp or qemu-img command). 
>>  
>> about xl and libvirt cooperation. currently, libvirt use xml for description 
>> domain snapshot for both user interface and store the snapshot information 
>> on disks. if libvirt libxl driver could use libxl-json format in load/store 
>> domain snapshot configuration, it would be easier for the user who may be 
>> switch xl and libvirt. this will not affect the libvirt user experience. 
>>  
>> Signed-off-by: Bamvor Jian Zhang <bjzhang@suse.com> 
>> --- 
>>  tools/libxl/libxl.h | 37 +++++++++++++++++++++++++++++++++++++ 
>>  1 file changed, 37 insertions(+) 
>>  
>> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h 
>> index be722b6..8106f4b 100644 
>> --- a/tools/libxl/libxl.h 
>> +++ b/tools/libxl/libxl.h 
>> @@ -1261,6 +1261,43 @@ int libxl_load_domain_configuration(libxl_ctx *ctx,  
>> uint32_t domid, 
>>  int libxl_store_domain_configuration(libxl_ctx *ctx, uint32_t domid, 
>>                                       libxl_domain_config *d_config); 
>>   
>> +/* snapshot relative APIs */ 
>> + 
>> +/* management functions for domain snapshot configuration */ 
>> + 
>> +/* Load, save, delete domain snapshot configuration file. */ 
>> +int libxl_load_dom_snapshot_conf(libxl_ctx *ctx, uint32_t domid, 
>> +                                    libxl_domain_snapshot *snapshot); 
>> +int libxl_store_dom_snapshot_conf(libxl_ctx *ctx, uint32_t domid, 
>> +                                     libxl_domain_snapshot *snapshot); 
>> +int libxl_delete_dom_snapshot_conf(libxl_ctx *ctx, uint32_t domid, 
>> +                                   libxl_domain_snapshot *snapshot); 
>> + 
>> +/* retrieve all the snapshot information from disk, put the number of it to  
>> num. 
>> + * caller is responsible for free the libxl_domain_snapshot array. 
>> + */ 
>> +libxl_domain_snapshot *libxl_domain_snapshot_list(libxl_ctx *ctx, 
>> +                                                  uint32_t domid, int  
>> *num); 
>> + 
>> +/* functions for disk snapshot operations */ 
>> +/* create disk snapshot through qmp transaction */ 
>> +int libxl_disk_snapshot_create(libxl_ctx *ctx, int domid, 
>> +                               libxl_disk_snapshot *snapshot, int nb); 
>> + 
>> +/* delete disk snapshot through qmp delete */ 
>> +int libxl_disk_snapshot_delete(libxl_ctx *ctx, int domid, 
>> +                               libxl_disk_snapshot *snapshot, int nb); 
>> + 
>> +/* revert disk snapshot through qemu-img snapshot apply command */ 
>> +int libxl_disk_snapshot_revert(libxl_ctx *ctx, uint32_t domid, 
>> +                               libxl_disk_snapshot *snapshot, int nb); 
>> + 
>> +/* create libxl_disk_snapshot from libxl_device_disk 
>> + * will alloc disks if disks empty 
>> + */ 
>> +int libxl_disk_to_snapshot(libxl_ctx *ctx, uint32_t domid, 
>> +                           libxl_domain_snapshot *snapshot); 
>> + 
>>  #include <libxl_event.h> 
>>   
>>  #endif /* LIBXL_H */ 
>>     
>
> I didn't see the details of structure libxl_disk_snapshot. Miss it?
>
> And generally, I think it would be helpful to roughly introduce your ideas in
> this patch, like:
> a) User interface, how would you expect user to use vm snapshot functionality?
>      xl snapshot-create xxx xxx xxx
>      xl snapshot-delete xxx xxx xxx
>   

Agreed.  Here you could also provide some user stories.  E.g. "As an
admin, I want to snapshot the system before applying the latest kernel
updates, so I can revert to the known working state if the update
introduces regressions."  And then describe how this is done with the
user interface.  IMO, all of this, along with the information in the
cover letter, should be in a new 'docs/misc/snapshot-HOTOW.txt' or such
file.

> b) To each operation, rough ideas on how you would implement. That may help
>     to understand why you introduce new structures and new APIs.
> c) New structures
> d) New APIs (would be helpful if there is description on function, parameters,
>      return value.)
>   

Yep, this sounds like a reasonable approach to me.

Regards,
Jim