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
