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

[Eric Blake <eblake@redhat.com>]

On 4/28/20 1:34 AM, Max Reitz wrote:

>>>> 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?
> 
> I suppose I can do that, this is what I’d squash in, OK?

Yes, that change looks correct to me.

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