Am 06.03.2017 um 21:53 hat Eric Blake geschrieben:
> 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).Note that bdrv_truncate() is a bit special because it is called in some paths that only have a -errno return and no Error** parameter, like bdrv_co_pwritev() implementations, and I don't think we intend to convert those to Error**. Sharing some code between bdrv_truncate() and write after EOF is something that I'd like to have anyway. Some of the things that bdrv_truncate() does are missing in bdrv_aligned_pwritev(), and in the age of -blockdev where you can use any node for anything, this is a bug. Kevin
pgpA55UuAh5oa.pgp
Description: PGP signature
