From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:52237)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <den@openvz.org>) id 1ZyH1a-0003bo-Ea
	for qemu-devel@nongnu.org; Mon, 16 Nov 2015 05:28:39 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <den@openvz.org>) id 1ZyH1X-0002in-7o
	for qemu-devel@nongnu.org; Mon, 16 Nov 2015 05:28:38 -0500
Received: from relay.parallels.com ([195.214.232.42]:38981)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <den@openvz.org>) id 1ZyH1W-0002iJ-VE
	for qemu-devel@nongnu.org; Mon, 16 Nov 2015 05:28:35 -0500
References: <1447165535-31263-1-git-send-email-den@openvz.org>
	<1447165535-31263-6-git-send-email-den@openvz.org>
	<20151116093100.GA16152@stefanha-x1.localdomain>
From: "Denis V. Lunev" <den@openvz.org>
Message-ID: <5649AFC1.3090009@openvz.org>
Date: Mon, 16 Nov 2015 13:28:17 +0300
MIME-Version: 1.0
In-Reply-To: <20151116093100.GA16152@stefanha-x1.localdomain>
Content-Type: text/plain; charset="windows-1252"; format=flowed
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH 05/10] snapshot: create
 bdrv_all_find_snapshot helper
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: Stefan Hajnoczi <stefanha@gmail.com>
Cc: Kevin Wolf <kwolf@redhat.com>, qemu-devel@nongnu.org, Stefan Hajnoczi <stefanha@redhat.com>, Juan Quintela <quintela@redhat.com>

On 11/16/2015 12:31 PM, Stefan Hajnoczi wrote:
> On Tue, Nov 10, 2015 at 05:25:30PM +0300, Denis V. Lunev wrote:
>> +int bdrv_all_find_snapshot(const char *name, bool read_only,
>> +                           BlockDriverState **first_bad_bs)
>> +{
>> +    QEMUSnapshotInfo sn;
>> +    int err = 0;
>> +    BlockDriverState *bs = NULL;
>> +
>> +    while (err == 0 && (bs = bdrv_next(bs))) {
>> +        AioContext *ctx = bdrv_get_aio_context(bs);
>> +
>> +        aio_context_acquire(ctx);
>> +        if (read_only || (bdrv_is_inserted(bs) && !bdrv_is_read_only(bs))) {
>> +            err = bdrv_snapshot_find(bs, &sn, name);
>> +        }
>> +        aio_context_release(ctx);
>> +    }
>> +
>> +    *first_bad_bs = bs;
>> +    return err;
>> +}
> It's difficult to see how bdrv_all_find_snapshot(read_only=true) is
> equivalent to what you replaced below:
>
>> @@ -1500,21 +1489,7 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
>>       available_snapshots = g_new0(int, nb_sns);
>>       total = 0;
>>       for (i = 0; i < nb_sns; i++) {
>> -        sn = &sn_tab[i];
>> -        available = 1;
>> -        bs1 = NULL;
>> -
>> -        while ((bs1 = bdrv_next(bs1))) {
>> -            if (bdrv_can_snapshot(bs1) && bs1 != bs) {
>> -                ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str);
>> -                if (ret < 0) {
>> -                    available = 0;
>> -                    break;
>> -                }
>> -            }
>> -        }
>> -
>> -        if (available) {
>> +        if (bdrv_all_find_snapshot(sn_tab[i].id_str, true, &bs1) == 0) {
> The original loop skips drives that cannot snapshot and the vmstate
> drive.  The new code tries to find a snapshot all devices.
>
> To me it seems the semantics are changed.  Can you explain why this
> change is safe?
argh... This code is used for 'virsh qemu-monitor-command  --hmp rhel7 
info snapshots'
and NOT used for 'virsh snapshot-list rhel7' or used in a very different 
way.

This is the root of the problem.

You are right, the command is broken completely. Some refactoring is 
necessary
here.

Den