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