Re: [Qemu-devel] [PATCH for-2.10 3/3] block: Add some bdrv_truncate() error messages

[Eric Blake]

On 03/06/2017 02:48 PM, Max Reitz wrote:
> On 06.03.2017 21:21, Eric Blake wrote:
>> On 03/06/2017 02:14 PM, Max Reitz wrote:
>>
>>>>>      if (S_ISREG(st.st_mode)) {
>>>>>          if (ftruncate(s->fd, offset) < 0) {
>>>>> +            /* The generic error message will be fine */
>>>>>              return -errno;
>>>>
>>>> Relying on a generic error message in the caller is awkward. I see it as
>>>> evidence of a partial conversion if we have an interface that requires a
>>>> return of a negative errno value to make up for when errp is not set.
>>>
>>> I'm not sure what you mean by this exactly.
>>
>> The ideal conversion would be having .bdrv_truncate switch to a void
>> return; then no caller is messing with negative errno values (especially
>> since some of them are just synthesizing errno values, rather than
>> actually encountering one, and since some of them are probably using -1
>> when they should be using errno). But such a conversion requires that
>> all drivers be updated to properly set errp.
> 
> Not sure if that is an ideal conversion. I very much prefer negative
> return value + error object because that allows constructs like
> 
> if (foo(..., errp) < 0) {
>     return;
> }

Fair point - Markus has argued that we should convert functions from
void to easy-to-spot return sentinels for easier logic (less boilerplate
in creating a local error, checking it, and propagating it).

But the point remains that returning -1 is simpler to code than
returning negative errno, when some of the existing drivers are already
synthesizing errno.  And (temporarily) forcing a void return would make
it easy to spot who still needs conversion, even if we then go back to
returning int (but this time, with the simpler -1 for error, rather than
negative errno).

> For me, the most important thing is that errp will always be set if the
> function fails.

Yes, that's what you are guaranteed with a void return, and what we
would also have with a (sane) integer return.

> 
> (Of course, I'd be just fine with a boolean return value, too.)

boolean works too, except it's a little harder to remember when 'true'
means success, compared to other code where '0' is success and negative
is failure.

>> In other words, I view the generic bdrv_truncate() supplying an error
>> based on negative errno is a crutch, and not something that we should
>> rely on, at least not for the drivers that we fix to set errp directly.
> 
> Well, the more detailed the error messages are the better, but I don't
> see any real improvement over a generic message supplied by
> bdrv_truncate() if that is good enough.

Maybe Markus has some opinions, since error reporting is his area.

> 
> Of course, I can easily be convinced that the generic message is in fact
> not good enough.

Maybe it's hard to argue that from the quality-of-message standpoint,
but that's exactly what I'm arguing from the ease-of-maintenance
standpoint.  Relying on the generic message is harder to maintain.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature