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

[Kevin Wolf]

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

pgpLo78gEAcdz.pgp
Description: PGP signature