Re: [Qemu-devel] [PATCH 1/6] snapshot: export function in block.c

[Wenchao Xia <xiawenc@linux.vnet.ibm.com>]

   Thanks for reviewing carefully, this patch would be split to two
patches.

> On Mon, Dec 17, 2012 at 02:25:04PM +0800, Wenchao Xia wrote:
>>    This patch moves bdrv_snapshotfind from savevm.c to block.c and export
>> it, also added bdrv_deappend in block.c.
>
> I suggest naming the patch "block: export bdrv_snapshot_find()" because
> it is more specific than "snapshot: export function in block.c".
>
>> +/* revert the action */
>> +void bdrv_deappend(BlockDriverState *bs_new, BlockDriverState *bs_top)
>> +{
>> +    bdrv_swap(bs_new, bs_top);
>> +    /* this is enough? */
>> +}
>
> Please document this function in the same level of detail as
> bdrv_append().  These functions are tricky and you must explain the
> semantics so anyone else reading the code can safely use or modify this
> function.
>
> We also need to resolve the "this is enough?" comment.  I'll think it
> through when I see how bdrv_deappend() is used later.
>
> Stefan
>


-- 
Best Regards

Wenchao Xia