From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([208.118.235.92]:50024)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1Tr8aG-0000Bd-7m
	for qemu-devel@nongnu.org; Fri, 04 Jan 2013 09:49:21 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1Tr8aF-0007fq-A6
	for qemu-devel@nongnu.org; Fri, 04 Jan 2013 09:49:20 -0500
Received: from mail-bk0-f49.google.com ([209.85.214.49]:54998)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1Tr8aF-0007fj-2s
	for qemu-devel@nongnu.org; Fri, 04 Jan 2013 09:49:19 -0500
Received: by mail-bk0-f49.google.com with SMTP id jm19so7237527bkc.8
	for <qemu-devel@nongnu.org>; Fri, 04 Jan 2013 06:49:18 -0800 (PST)
Date: Fri, 4 Jan 2013 15:49:15 +0100
From: Stefan Hajnoczi <stefanha@gmail.com>
Message-ID: <20130104144915.GA6310@stefanha-thinkpad.hitronhub.home>
References: <1355725509-5429-1-git-send-email-xiawenc@linux.vnet.ibm.com>
	<1355725509-5429-2-git-send-email-xiawenc@linux.vnet.ibm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1355725509-5429-2-git-send-email-xiawenc@linux.vnet.ibm.com>
Subject: Re: [Qemu-devel] [PATCH 1/6] snapshot: export function in block.c
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Cc: kwolf@redhat.com, pbonzini@redhat.com, aliguori@us.ibm.com, qemu-devel@nongnu.org, blauwirbel@gmail.com

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