Re: [PATCH v3 1/5] block-copy: streamline choice of copy_range vs. read/write

[Vladimir Sementsov-Ogievskiy]

09.06.2021 12:33, Paolo Bonzini wrote:
> On 09/06/21 10:51, Vladimir Sementsov-Ogievskiy wrote:
> > > +    default:
> > > [...]
> > > +        bounce_buffer = qemu_blockalign(s->source->bs, nbytes);
> > > +        ret = bdrv_co_pread(s->source, offset, nbytes, bounce_buffer, 0);
> > > +        if (ret < 0) {
> > > +            trace_block_copy_read_fail(s, offset, ret);
> > > +            *error_is_read = true;
> > > +            goto out;
> > > +        }
> > > +        ret = bdrv_co_pwrite(s->target, offset, nbytes, bounce_buffer,
> > > +                            s->write_flags);
> > > +        if (ret < 0) {
> > > +            trace_block_copy_write_fail(s, offset, ret);
> > > +            *error_is_read = false;
> > > +            goto out;
> > > +        }
> > > +out:
> > > +        qemu_vfree(bounce_buffer);
> >
> > label inside switch operator? Rather unusual. Please, let's avoid it and just 
> > keep out: after switch operator.
>
> Agreed with all comments except this one, the bounce_buffer doesn't exist in 
> the other cases.

Hmm, as well as "return ret" actually is used only for this "default:" case, 
other paths returns earlier :) Also, bounce_buffer is defined in outer scope anyway. So I don't 
think that overall picture becomes better from isolation point of view with this change. Maybe good 
refactoring is moving default branch to a separate helper function together with bounce_buffer 
local variable.

Still, I don't care too much, keep it as is if you want, that's works for me.

The thing that comes to my mind not the first time: how to make something 
similar with g_autofree for qemu_blockalign()?

I can imagine how to implement a macro like ERRP_GUARD, which will work like

void *bounce_buffer = qemu_blockalign(...);
QEMU_AUTO_VFREE(bounce_buffer);

...

> > > +    ret = block_copy_do_copy(s, t->offset, t->bytes, &method, &error_is_read);
> > > +    if (s->method == t->method) {
> > > +        s->method = method;
> >
> > you leave another t->s occurrences in the function untouched. It's somehow inconsistent. Could we just use t->s in this patch, and refactor with a follow-up patch (or as preparing patch)? 
>
> Maybe as a first patch, yes.
>
> Paolo


--
Best regards,
Vladimir