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
