| Message ID | 20170306195434.16782-1-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 03/06/2017 01:54 PM, Max Reitz wrote: > Add missing error messages for the drivers I am comfortable to do this > in. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > block/file-posix.c | 8 +++++++- > block/qcow2.c | 2 ++ > block/qed.c | 4 +++- > block/raw-format.c | 2 ++ > 4 files changed, 14 insertions(+), 2 deletions(-) > > diff --git a/block/file-posix.c b/block/file-posix.c > index 9d2bea730d..553213221c 100644 > --- a/block/file-posix.c > +++ b/block/file-posix.c > @@ -1341,20 +1341,26 @@ static int raw_truncate(BlockDriverState *bs, int64_t offset, Error **errp) > { > BDRVRawState *s = bs->opaque; > struct stat st; > + int ret; > > if (fstat(s->fd, &st)) { > - return -errno; > + ret = -errno; > + error_setg_errno(errp, -ret, "Failed to fstat() the file"); > + return ret; > } > > 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 know you aren't comfortable converting all drivers, but for the drivers you do convert, I'd rather guarantee that ALL errors set errp.
On 06.03.2017 21:09, Eric Blake wrote: > On 03/06/2017 01:54 PM, Max Reitz wrote: >> Add missing error messages for the drivers I am comfortable to do this >> in. >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> block/file-posix.c | 8 +++++++- >> block/qcow2.c | 2 ++ >> block/qed.c | 4 +++- >> block/raw-format.c | 2 ++ >> 4 files changed, 14 insertions(+), 2 deletions(-) >> >> diff --git a/block/file-posix.c b/block/file-posix.c >> index 9d2bea730d..553213221c 100644 >> --- a/block/file-posix.c >> +++ b/block/file-posix.c >> @@ -1341,20 +1341,26 @@ static int raw_truncate(BlockDriverState *bs, int64_t offset, Error **errp) >> { >> BDRVRawState *s = bs->opaque; >> struct stat st; >> + int ret; >> >> if (fstat(s->fd, &st)) { >> - return -errno; >> + ret = -errno; >> + error_setg_errno(errp, -ret, "Failed to fstat() the file"); >> + return ret; >> } >> >> 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. 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 > 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. Max
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. > 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. > >> I >> 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.
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
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.
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
diff --git a/block/file-posix.c b/block/file-posix.c index 9d2bea730d..553213221c 100644 --- a/block/file-posix.c +++ b/block/file-posix.c @@ -1341,20 +1341,26 @@ static int raw_truncate(BlockDriverState *bs, int64_t offset, Error **errp) { BDRVRawState *s = bs->opaque; struct stat st; + int ret; if (fstat(s->fd, &st)) { - return -errno; + ret = -errno; + error_setg_errno(errp, -ret, "Failed to fstat() the file"); + return ret; } if (S_ISREG(st.st_mode)) { if (ftruncate(s->fd, offset) < 0) { + /* The generic error message will be fine */ return -errno; } } else if (S_ISCHR(st.st_mode) || S_ISBLK(st.st_mode)) { if (offset > raw_getlength(bs)) { + error_setg(errp, "Cannot grow device files"); return -EINVAL; } } else { + error_setg(errp, "Resizing this file is not supported"); return -ENOTSUP; } diff --git a/block/qcow2.c b/block/qcow2.c index 17585fbb89..53b0bd61a7 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -2550,6 +2550,7 @@ static int qcow2_truncate(BlockDriverState *bs, int64_t offset, Error **errp) new_l1_size = size_to_l1(s, offset); ret = qcow2_grow_l1_table(bs, new_l1_size, true); if (ret < 0) { + error_setg(errp, "Failed to grow the L1 table"); return ret; } @@ -2558,6 +2559,7 @@ static int qcow2_truncate(BlockDriverState *bs, int64_t offset, Error **errp) ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, size), &offset, sizeof(uint64_t)); if (ret < 0) { + error_setg(errp, "Failed to update the image size"); return ret; } diff --git a/block/qed.c b/block/qed.c index fa2aeee471..eb346d645b 100644 --- a/block/qed.c +++ b/block/qed.c @@ -1526,11 +1526,12 @@ static int bdrv_qed_truncate(BlockDriverState *bs, int64_t offset, Error **errp) if (!qed_is_image_size_valid(offset, s->header.cluster_size, s->header.table_size)) { + error_setg(errp, "Invalid image size specified"); return -EINVAL; } - /* Shrinking is currently not supported */ if ((uint64_t)offset < s->header.image_size) { + error_setg(errp, "Shrinking images is currently not supported"); return -ENOTSUP; } @@ -1539,6 +1540,7 @@ static int bdrv_qed_truncate(BlockDriverState *bs, int64_t offset, Error **errp) ret = qed_write_header_sync(s); if (ret < 0) { s->header.image_size = old_image_size; + error_setg(errp, "Failed to update the image size"); } return ret; } diff --git a/block/raw-format.c b/block/raw-format.c index 9761bdaff8..36e65036f0 100644 --- a/block/raw-format.c +++ b/block/raw-format.c @@ -332,10 +332,12 @@ static int raw_truncate(BlockDriverState *bs, int64_t offset, Error **errp) BDRVRawState *s = bs->opaque; if (s->has_size) { + error_setg(errp, "Cannot resize fixed-size raw disks"); return -ENOTSUP; } if (INT64_MAX - offset < s->offset) { + error_setg(errp, "Disk size too large for the chosen offset"); return -EINVAL; }
Add missing error messages for the drivers I am comfortable to do this in. Signed-off-by: Max Reitz <mreitz@redhat.com> --- block/file-posix.c | 8 +++++++- block/qcow2.c | 2 ++ block/qed.c | 4 +++- block/raw-format.c | 2 ++ 4 files changed, 14 insertions(+), 2 deletions(-)