Re: [Qemu-devel] [PATCH 27/47] block: introduce mirror job

[Paolo Bonzini]

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