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

[Max Reitz]

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;
}

Or:

ret = foo(..., errp);
if (ret < 0) {
    return ret;
}

Instead of:

foo(..., &local_err);
if (local_err) {
    error_propagate(errp, local_err);
    return;
}


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

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

>> I can agree that it's not
>> nice within a single driver. But I think it's fine to have some drivers
>> that generate an Error object and others that do not, as long as the
>> generic interface (bdrv_truncate()) masks this behavior.
> 
> I agree that as an interim thing, having some drivers that aren't
> converted yet is the best way to make progress. But it's still best if
> every driver is switched on an all-or-none basis, so that we don't
> forget to go back and re-fix drivers that half-heartedly set errp if we
> eventually reach the point where all other drivers have been fixed.
> 
> 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.

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

>>> know you aren't comfortable converting all drivers, but for the drivers
>>> you do convert, I'd rather guarantee that ALL errors set errp.
>>
>> Can do, I just felt bad that it be would exactly the same as the message
>> that would be generated in bdrv_truncate() anyway.
> 
> There may be some duplication of error messages, but it still seems
> cleaner to consistently set the error within the driver than to rely on
> the generic crutch.

As I said, I don't necessarily think it's a crutch. :-)

Max

signature.asc
Description: OpenPGP digital signature