qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [PATCH v2 3/7] qcow2: Make perform_cow() call do_perfor


From: Alberto Garcia
Subject: Re: [Qemu-block] [PATCH v2 3/7] qcow2: Make perform_cow() call do_perform_cow() twice
Date: Thu, 08 Jun 2017 09:09:00 +0200
User-agent: Notmuch/0.18.2 (http://notmuchmail.org) Emacs/24.4.1 (i586-pc-linux-gnu)

On Wed 07 Jun 2017 11:43:34 PM CEST, Eric Blake wrote:
>>  block/qcow2-cluster.c | 38 +++++++++++++++++++++++---------------
>>  1 file changed, 23 insertions(+), 15 deletions(-)
>
>>      qemu_co_mutex_unlock(&s->lock);
>> -    ret = do_perform_cow(bs, m->offset, m->alloc_offset, r->offset, 
>> r->nb_bytes);
>> +    ret = do_perform_cow(bs, m->offset, m->alloc_offset,
>> +                         start->offset, start->nb_bytes);
>> +    if (ret < 0) {
>> +        goto fail;
>> +    }
>> +
>> +    ret = do_perform_cow(bs, m->offset, m->alloc_offset,
>> +                         end->offset, end->nb_bytes);
>> +
>> +fail:
>
> Since you reach this label even on success, it feels a bit awkward. I
> probably would have avoided the goto and used fewer lines by
> refactoring the logic as:
>
> ret = do_perform_cow(..., start->...);
> if (ret >= 0) {
>     ret = do_perform_cow(..., end->...);
> }

I actually wrote this code without any gotos the way you mention, and it
works fine in this patch, but as I was making more changes I ended up
with a quite large piece of code that needed to add "if (!ret)" to
almost every if statement, so I didn't quite like the final result.

I'm open to reconsider it, but take a look first at the code after the
last patch and you'll see that it would not be as neat as it appears
now.

Berto



reply via email to

[Prev in Thread] Current Thread [Next in Thread]