Re: [PATCH v3 1/3] block: Add blk_new_with_bs() helper

[Eric Blake]

On 4/27/20 5:00 AM, Max Reitz wrote:
> On 24.04.20 21:09, Eric Blake wrote:
> > There are several callers that need to create a new block backend from
> > an existing BDS; make the task slightly easier with a common helper
> > routine.
> >
> > Suggested-by: Max Reitz <address@hidden>
> > Signed-off-by: Eric Blake <address@hidden>
> > ---

> > +++ b/block/crypto.c
> > @@ -256,16 +256,14 @@ static int 
> > block_crypto_co_create_generic(BlockDriverState *bs,
> >                                             PreallocMode prealloc,
> >                                             Error **errp)
> >   {
> > -    int ret;
> > +    int ret = -EPERM;
>
> I’m not sure I’m a fan of this, because I feel like it makes the code
> harder to read, due to having to look in three places (here, around the
> blk_new_with_bs() call, and under the cleanup label) instead of in two
> (not here) to verify that the error handling code is correct.
>
> There’s also the fact that this is not really a default return value,
> but one very specific error code for if one very specific function call
> fails.
>
> I suppose it comes down to whether one considers LoC a complexity
> problem.  (I don’t, necessarily.)
>
> (Also I realize it seems rather common in the kernel to set error return
> variables before the function call, but I think the more common pattern
> in qemu is to set it in the error path.)

I'm fine with either style.  Setting it up front is handy if that 
particular error makes a good default, but in many of the functions I 
touched, we were returning a variety of errors (-EIO, -EINVAL, -EPERM, 
etc) such that there was no good default, and thus no reason to set a 
default up front.  Is this something that would go through your tree, 
and if so, are you okay making that tweak, or do I need to send v4?

> But it does look like you got all cases covered this time.

Good to hear.

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org