[sheepdog] [PATCH v3 1/3] block: Add blk_new_with_bs() helper
eblake at redhat.com
Tue Apr 28 14:47:05 CEST 2020
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
>>> 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
More information about the sheepdog