From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:57816)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <xiawenc@linux.vnet.ibm.com>) id 1VU9PT-0006J3-B8
	for qemu-devel@nongnu.org; Thu, 10 Oct 2013 02:07:52 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <xiawenc@linux.vnet.ibm.com>) id 1VU9PK-0005nk-Cj
	for qemu-devel@nongnu.org; Thu, 10 Oct 2013 02:07:43 -0400
Received: from e28smtp09.in.ibm.com ([122.248.162.9]:46528)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <xiawenc@linux.vnet.ibm.com>) id 1VU9PJ-0005nZ-PP
	for qemu-devel@nongnu.org; Thu, 10 Oct 2013 02:07:34 -0400
Received: from /spool/local
	by e28smtp09.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use
	Only! Violators will be prosecuted
	for <qemu-devel@nongnu.org> from <xiawenc@linux.vnet.ibm.com>;
	Thu, 10 Oct 2013 11:37:32 +0530
Received: from d28relay03.in.ibm.com (d28relay03.in.ibm.com [9.184.220.60])
	by d28dlp03.in.ibm.com (Postfix) with ESMTP id 7436B125805C
	for <qemu-devel@nongnu.org>; Thu, 10 Oct 2013 11:37:54 +0530 (IST)
Received: from d28av04.in.ibm.com (d28av04.in.ibm.com [9.184.220.66])
	by d28relay03.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id
	r9A6A4b637945578
	for <qemu-devel@nongnu.org>; Thu, 10 Oct 2013 11:40:05 +0530
Received: from d28av04.in.ibm.com (localhost [127.0.0.1])
	by d28av04.in.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id
	r9A66Tsr010961
	for <qemu-devel@nongnu.org>; Thu, 10 Oct 2013 11:36:29 +0530
Message-ID: <525643EA.3090007@linux.vnet.ibm.com>
Date: Thu, 10 Oct 2013 14:06:34 +0800
From: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
MIME-Version: 1.0
References: <1380154568-5339-1-git-send-email-xiawenc@linux.vnet.ibm.com>
	<1380154568-5339-5-git-send-email-xiawenc@linux.vnet.ibm.com>
	<524AE1DD.7070606@redhat.com>
In-Reply-To: <524AE1DD.7070606@redhat.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Subject: Re: [Qemu-devel] [PATCH V3 4/7] qemu-iotests: add 058 internal
 snapshot export with qemu-nbd case
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: Eric Blake <eblake@redhat.com>
Cc: kwolf@redhat.com, pbonzini@redhat.com, qemu-devel@nongnu.org, stefanha@gmail.com

于 2013/10/1 22:53, Eric Blake 写道:
> On 09/25/2013 06:16 PM, Wenchao Xia wrote:
>> Signed-off-by: Wenchao Xia<xiawenc@linux.vnet.ibm.com>
>> ---
>> +_export_nbd_snapshot()
>> +{
>> +    eval "$QEMU_NBD -v -t -b 127.0.0.1 -p $nbd_snapshot_port $TEST_IMG -l $1&"
> Uggh.  Why do you need an eval here?  Especially given that there was
> recently a patch to properly quote $TEST_IMG in case the tests are run
> inside a directory whose absolute name included a space.  What's wrong
> with just directly:
>
> $QEMU_NBD -v -t -b 127.0.0.1 -p $nbd_snapshot_port "$TEST_IMG" -l $1 $
>
   Just a copy and paste for "eval", will remove it.

>> +    NBD_SNAPSHOT_PID=$!
>> +    sleep 1
>> +}
>> +
>> +_export_nbd_snapshot1()
>> +{
>> +    eval "$QEMU_NBD -v -t -b 127.0.0.1 -p $nbd_snapshot_port $TEST_IMG -L snapshot.name=$1&"
> Likewise; and given my complaint on 2-3/7, it would be nicer to support
> this with only one option name spelling.
>
>> +_cleanup()
>> +{
>> +    if [ -n "$NBD_SNAPSHOT_PID" ]; then
>> +        kill $NBD_SNAPSHOT_PID
>> +    fi
>> +	_cleanup_test_img
> Kill the TAB, fix the indentation.
>
   Will fix.

>> +}
>> +trap "_cleanup; exit \$status" 0 1 2 3 15
>> +
>> +# get standard environment, filters and checks
>> +. ./common.rc
>> +. ./common.filter
>> +. ./common.pattern
>> +
>> +# Any format supporting intenal snapshots
> s/intenal/internal/
>
  will fix.

>> +_supported_fmt qcow2
>> +_supported_proto generic
>> +_supported_os Linux
> Is this test truly Linux-only?
   I think it is generic, will remove it.