From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([208.118.235.92]:44452)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <paolo.bonzini@gmail.com>) id 1TCA5U-0002uX-LD
	for qemu-devel@nongnu.org; Thu, 13 Sep 2012 10:08:18 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <paolo.bonzini@gmail.com>) id 1TCA5L-0000uD-Mq
	for qemu-devel@nongnu.org; Thu, 13 Sep 2012 10:08:12 -0400
Received: from mail-pb0-f45.google.com ([209.85.160.45]:57749)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <paolo.bonzini@gmail.com>) id 1TCA5L-0000u3-GV
	for qemu-devel@nongnu.org; Thu, 13 Sep 2012 10:08:03 -0400
Received: by pbbrp12 with SMTP id rp12so4210305pbb.4
	for <qemu-devel@nongnu.org>; Thu, 13 Sep 2012 07:08:02 -0700 (PDT)
Sender: Paolo Bonzini <paolo.bonzini@gmail.com>
Message-ID: <5051E8B7.2040307@redhat.com>
Date: Thu, 13 Sep 2012 16:07:51 +0200
From: Paolo Bonzini <pbonzini@redhat.com>
MIME-Version: 1.0
References: <1343127865-16608-1-git-send-email-pbonzini@redhat.com>
	<1343127865-16608-28-git-send-email-pbonzini@redhat.com>
	<5051D78F.90104@redhat.com>
In-Reply-To: <5051D78F.90104@redhat.com>
Content-Type: text/plain; charset=ISO-8859-15
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH 27/47] block: introduce mirror job
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: Kevin Wolf <kwolf@redhat.com>
Cc: jcody@redhat.com, eblake@redhat.com, qemu-devel@nongnu.org, stefanha@linux.vnet.ibm.com

Il 13/09/2012 14:54, Kevin Wolf ha scritto:
>> > +            ret = bdrv_co_is_allocated_above(bs, base,
>> > +                                             sector_num, next - sector_num, &n);
>> > +
>> > +            if (ret < 0) {
>> > +                break;
>> > +            } else if (ret == 1) {
>> > +                bdrv_set_dirty(bs, sector_num, n);
>> > +                sector_num = next;
>> > +            } else {
>> > +                sector_num += n;
>> > +            }
> Maybe it would be worth checking for n == 0 and returning an error in
> that case. One example where this happens is when asking for the
> allocation status after EOF. It shouldn't happen as long as
> bdrv_truncate() is forbidden while the job runs, but an extra check
> rarely hurts.

This is just an initialization loop, so I'll add an assertion instead.

>> > +        }
>> > +    }
>> > +
>> > +    if (ret < 0) {
>> > +        goto immediate_exit;
>> > +    }
> Why not do that directly instead of having a break; first just to get here?

Good idea.

>> > +
>> > +    s->sector_num = -1;
>> > +    for (;;) {
>> > +        uint64_t delay_ns;
>> > +        int64_t cnt;
>> > +        bool should_complete;
>> > +
>> > +        cnt = bdrv_get_dirty_count(bs);
>> > +        if (cnt != 0) {
>> > +            ret = mirror_iteration(s);
>> > +            if (ret < 0) {
>> > +                break;
> goto immediate_exit? It's the same now, but code after the loop may be
> added in the future.

That's why there is a break. :)  There will be code added later before
immediate_exit.  But it is just an if statement that will never be true
if mirroring hasn't started ye, so it can also go after.  I'll change to
goto and move that if statement after the label.

>> > +            }
>> > +            cnt = bdrv_get_dirty_count(bs);
>> > +        }
>> > +
>> > +        if (cnt != 0) {
>> > +            should_complete = false;
>> > +        } else {
>> > +            trace_mirror_before_flush(s);
>> > +            bdrv_flush(s->target);
> No error handling?

Will add.

Paolo