Re: [PATCH 07/14] block/blklogwrites: drop error propagation
Ari Sundholm writes:
> Hi,
>
> On 9/11/20 11:03 AM, Markus Armbruster wrote:
>> Ari Sundholm writes:
>>
>>> Hi Vladimir!
>>>
>>> Thank you for working on this. My comments below.
>>>
>>> On 9/9/20 9:59 PM, Vladimir Sementsov-Ogievskiy wrote:
It's simple to avoid error propagation in blk_log_writes_open(), we
just need to refactor blk_log_writes_find_cur_log_sector() a bit.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block/blklogwrites.c | 23 +++
1 file changed, 11 insertions(+), 12 deletions(-)
diff --git a/block/blklogwrites.c b/block/blklogwrites.c
index 7ef046cee9..c7da507b2d 100644
--- a/block/blklogwrites.c
+++ b/block/blklogwrites.c
@@ -96,10 +96,10 @@ static inline bool
blk_log_writes_sector_size_valid(uint32_t sector_size)
sector_size < (1ull << 24);
}
-static uint64_t blk_log_writes_find_cur_log_sector(BdrvChild
*log,
- uint32_t sector_size,
- uint64_t nr_entries,
- Error **errp)
+static int64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
>>>
>>> I'd rather not change the return type for reasons detailed below.
>>>
+ uint32_t sector_size,
+ uint64_t nr_entries,
+ Error **errp)
{
uint64_t cur_sector = 1;
uint64_t cur_idx = 0;
@@ -112,13 +112,13 @@ static uint64_t
blk_log_writes_find_cur_log_sector(BdrvChild *log,
if (read_ret < 0) {
error_setg_errno(errp, -read_ret,
"Failed to read log entry %"PRIu64,
cur_idx);
-return (uint64_t)-1ull;
+return read_ret;
>>>
>>> This is OK, provided the change in return type signedness is necessary
>>> in the first place.
>>>
}
if (cur_entry.flags & ~cpu_to_le64(LOG_FLAG_MASK)) {
error_setg(errp, "Invalid flags 0x%"PRIx64" in log entry
%"PRIu64,
le64_to_cpu(cur_entry.flags), cur_idx);
-return (uint64_t)-1ull;
+return -EINVAL;
>>>
>>> This is OK, provided the return type signedness change is necessary,
>>> although we already do have errp to indicate any errors.
>>>
}
/* Account for the sector of the entry itself */
@@ -143,7 +143,6 @@ static int blk_log_writes_open(BlockDriverState *bs,
QDict *options, int flags,
{
BDRVBlkLogWritesState *s = bs->opaque;
QemuOpts *opts;
-Error *local_err = NULL;
int ret;
uint64_t log_sector_size;
bool log_append;
@@ -215,15 +214,15 @@ static int blk_log_writes_open(BlockDriverState *bs,
QDict *options, int flags,
s->nr_entries = 0;
if (blk_log_writes_sector_size_valid(log_sector_size)) {
-s->cur_log_sector =
+int64_t cur_log_sector =
blk_log_writes_find_cur_log_sector(s->log_file,
log_sector_size,
-le64_to_cpu(log_sb.nr_entries),
&local_err);
-if (local_err) {
-ret = -EINVAL;
-error_propagate(errp, local_err);
+le64_to_cpu(log_sb.nr_entries), errp);
+if (cur_log_sector < 0) {
+ret = cur_log_sector;
>>>
>>> This changes the semantics slightly. Changing the return type to int64
>>> may theoretically cause valid return values to be interpreted as
>>> errors. Since we already do have a dedicated out pointer for the error
>>> value (errp), why not use it?
>> Error.h's big comment:
>> * Error reporting system loosely patterned after Glib's GError.
>> *
>> * = Rules =
>> [...]
>> * - Whenever practical, also return a value that indicates success /
>> * failure. This can make the error checking more concise, and can
>> * avoid useless error object creation and destruction. Note that
>> * we still have many functions returning void. We recommend
>> * • bool-valued functions return true on success / false on failure,
>> * • pointer-valued functions return non-null / null pointer, and
>> * • integer-valued functions return non-negative / negative.
>> blk_log_writes_find_cur_log_sector() does return such an error value
>> before the patch: (uint64_t)-1.
>> The caller does not use it to check for errors. It uses @err
>> instead.
>> Awkward, has to error_propagate().
>> Avoiding error_propagate() reduces the error handling boileplate.
>> It
>> also improves b
Re: [PATCH 07/14] block/blklogwrites: drop error propagation
Hi,
On 9/11/20 11:03 AM, Markus Armbruster wrote:
Ari Sundholm writes:
Hi Vladimir!
Thank you for working on this. My comments below.
On 9/9/20 9:59 PM, Vladimir Sementsov-Ogievskiy wrote:
It's simple to avoid error propagation in blk_log_writes_open(), we
just need to refactor blk_log_writes_find_cur_log_sector() a bit.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block/blklogwrites.c | 23 +++
1 file changed, 11 insertions(+), 12 deletions(-)
diff --git a/block/blklogwrites.c b/block/blklogwrites.c
index 7ef046cee9..c7da507b2d 100644
--- a/block/blklogwrites.c
+++ b/block/blklogwrites.c
@@ -96,10 +96,10 @@ static inline bool
blk_log_writes_sector_size_valid(uint32_t sector_size)
sector_size < (1ull << 24);
}
-static uint64_t blk_log_writes_find_cur_log_sector(BdrvChild
*log,
- uint32_t sector_size,
- uint64_t nr_entries,
- Error **errp)
+static int64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
I'd rather not change the return type for reasons detailed below.
+ uint32_t sector_size,
+ uint64_t nr_entries,
+ Error **errp)
{
uint64_t cur_sector = 1;
uint64_t cur_idx = 0;
@@ -112,13 +112,13 @@ static uint64_t
blk_log_writes_find_cur_log_sector(BdrvChild *log,
if (read_ret < 0) {
error_setg_errno(errp, -read_ret,
"Failed to read log entry %"PRIu64, cur_idx);
-return (uint64_t)-1ull;
+return read_ret;
This is OK, provided the change in return type signedness is necessary
in the first place.
}
if (cur_entry.flags & ~cpu_to_le64(LOG_FLAG_MASK)) {
error_setg(errp, "Invalid flags 0x%"PRIx64" in log entry
%"PRIu64,
le64_to_cpu(cur_entry.flags), cur_idx);
-return (uint64_t)-1ull;
+return -EINVAL;
This is OK, provided the return type signedness change is necessary,
although we already do have errp to indicate any errors.
}
/* Account for the sector of the entry itself */
@@ -143,7 +143,6 @@ static int blk_log_writes_open(BlockDriverState *bs, QDict
*options, int flags,
{
BDRVBlkLogWritesState *s = bs->opaque;
QemuOpts *opts;
-Error *local_err = NULL;
int ret;
uint64_t log_sector_size;
bool log_append;
@@ -215,15 +214,15 @@ static int blk_log_writes_open(BlockDriverState *bs,
QDict *options, int flags,
s->nr_entries = 0;
if (blk_log_writes_sector_size_valid(log_sector_size)) {
-s->cur_log_sector =
+int64_t cur_log_sector =
blk_log_writes_find_cur_log_sector(s->log_file,
log_sector_size,
-le64_to_cpu(log_sb.nr_entries),
&local_err);
-if (local_err) {
-ret = -EINVAL;
-error_propagate(errp, local_err);
+le64_to_cpu(log_sb.nr_entries), errp);
+if (cur_log_sector < 0) {
+ret = cur_log_sector;
This changes the semantics slightly. Changing the return type to int64
may theoretically cause valid return values to be interpreted as
errors. Since we already do have a dedicated out pointer for the error
value (errp), why not use it?
Error.h's big comment:
* Error reporting system loosely patterned after Glib's GError.
*
* = Rules =
[...]
* - Whenever practical, also return a value that indicates success /
* failure. This can make the error checking more concise, and can
* avoid useless error object creation and destruction. Note that
* we still have many functions returning void. We recommend
* • bool-valued functions return true on success / false on failure,
* • pointer-valued functions return non-null / null pointer, and
* • integer-valued functions return non-negative / negative.
blk_log_writes_find_cur_log_sector() does return such an error value
before the patch: (uint64_t)-1.
The caller does not use it to check for errors. It uses @err instead.
Awkward, has to error_propagate().
Avoiding error_propagate() reduces the error handling boileplate. It
also improves behavior when @errp is &error_abort: we get the abort
right where the error happens instead of where we propagate it.
Furthermore, caller has to make an error code (-EINVAL), because
returning (uint64_t)-1 throws it away. Yes, a detailed error is stored
into @err, but you can't cleanly extract the error code.
Using a signed integer for returning "non-negative offset or negative
errno code" is pervasive, starting with read() and write(). It hasn't
been a problem there, and it hasn't been a
Re: [PATCH 07/14] block/blklogwrites: drop error propagation
Greg Kurz writes:
> On Fri, 11 Sep 2020 07:30:37 +0200
> Markus Armbruster wrote:
[...]
>> No, the patch is okay as is.
>>
>> Dumbing it down to keep it simple would also be okay.
>>
>
> What about Ari's proposal to add ERRP_GUARD() and check errors
> with "if (*errp)" like we do for void functions ?
Up to the maintainer. I prefer this patch.
>> Regarding the proposed assertion: do we protect similar conversions from
>> over-wide negative errno int elsewhere?
>>
>
> We do protect int64_t->int conversions in a few places, eg.
> qcow2_write_snapshots() or qemu_spice_create_host_primary(),
> but I couldn't find one about a negarive errno.
Then I'd not protect it here, either.
>> >>> goto fail_log;
>> >>> }
>> >>>
>> >>> +s->cur_log_sector = cur_log_sector;
>> >>> s->nr_entries = le64_to_cpu(log_sb.nr_entries);
>> >>> }
>> >>> } else {
>>
Re: [PATCH 07/14] block/blklogwrites: drop error propagation
Ari Sundholm writes:
> Hi Vladimir!
>
> Thank you for working on this. My comments below.
>
> On 9/9/20 9:59 PM, Vladimir Sementsov-Ogievskiy wrote:
>> It's simple to avoid error propagation in blk_log_writes_open(), we
>> just need to refactor blk_log_writes_find_cur_log_sector() a bit.
>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>>
>> ---
>> block/blklogwrites.c | 23 +++
>> 1 file changed, 11 insertions(+), 12 deletions(-)
>> diff --git a/block/blklogwrites.c b/block/blklogwrites.c
>> index 7ef046cee9..c7da507b2d 100644
>> --- a/block/blklogwrites.c
>> +++ b/block/blklogwrites.c
>> @@ -96,10 +96,10 @@ static inline bool
>> blk_log_writes_sector_size_valid(uint32_t sector_size)
>> sector_size < (1ull << 24);
>> }
>> -static uint64_t blk_log_writes_find_cur_log_sector(BdrvChild
>> *log,
>> - uint32_t sector_size,
>> - uint64_t nr_entries,
>> - Error **errp)
>> +static int64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
>
> I'd rather not change the return type for reasons detailed below.
>
>> + uint32_t sector_size,
>> + uint64_t nr_entries,
>> + Error **errp)
>> {
>> uint64_t cur_sector = 1;
>> uint64_t cur_idx = 0;
>> @@ -112,13 +112,13 @@ static uint64_t
>> blk_log_writes_find_cur_log_sector(BdrvChild *log,
>> if (read_ret < 0) {
>> error_setg_errno(errp, -read_ret,
>>"Failed to read log entry %"PRIu64, cur_idx);
>> -return (uint64_t)-1ull;
>> +return read_ret;
>
> This is OK, provided the change in return type signedness is necessary
> in the first place.
>
>> }
>> if (cur_entry.flags & ~cpu_to_le64(LOG_FLAG_MASK)) {
>> error_setg(errp, "Invalid flags 0x%"PRIx64" in log entry
>> %"PRIu64,
>> le64_to_cpu(cur_entry.flags), cur_idx);
>> -return (uint64_t)-1ull;
>> +return -EINVAL;
>
> This is OK, provided the return type signedness change is necessary,
> although we already do have errp to indicate any errors.
>
>> }
>> /* Account for the sector of the entry itself */
>> @@ -143,7 +143,6 @@ static int blk_log_writes_open(BlockDriverState *bs,
>> QDict *options, int flags,
>> {
>> BDRVBlkLogWritesState *s = bs->opaque;
>> QemuOpts *opts;
>> -Error *local_err = NULL;
>> int ret;
>> uint64_t log_sector_size;
>> bool log_append;
>> @@ -215,15 +214,15 @@ static int blk_log_writes_open(BlockDriverState *bs,
>> QDict *options, int flags,
>> s->nr_entries = 0;
>> if (blk_log_writes_sector_size_valid(log_sector_size)) {
>> -s->cur_log_sector =
>> +int64_t cur_log_sector =
>> blk_log_writes_find_cur_log_sector(s->log_file,
>> log_sector_size,
>> -le64_to_cpu(log_sb.nr_entries),
>> &local_err);
>> -if (local_err) {
>> -ret = -EINVAL;
>> -error_propagate(errp, local_err);
>> +le64_to_cpu(log_sb.nr_entries), errp);
>> +if (cur_log_sector < 0) {
>> +ret = cur_log_sector;
>
> This changes the semantics slightly. Changing the return type to int64
> may theoretically cause valid return values to be interpreted as
> errors. Since we already do have a dedicated out pointer for the error
> value (errp), why not use it?
Error.h's big comment:
* Error reporting system loosely patterned after Glib's GError.
*
* = Rules =
[...]
* - Whenever practical, also return a value that indicates success /
* failure. This can make the error checking more concise, and can
* avoid useless error object creation and destruction. Note that
* we still have many functions returning void. We recommend
* • bool-valued functions return true on success / false on failure,
* • pointer-valued functions return non-null / null pointer, and
* • integer-valued functions return non-negative / negative.
blk_log_writes_find_cur_log_sector() does return such an error value
before the patch: (uint64_t)-1.
The caller does not use it to check for errors. It uses @err instead.
Awkward, has to error_propagate().
Avoiding error_propagate() reduces the error handling boileplate. It
also improves behavior when @errp is &error_abort: we get the abort
right where the error happens instead of where we propagate it.
Furthermore, caller has to make an error code (-EINVAL), because
returning (uint64_t)-1 throws it away. Yes, a detailed error is stored
into @err, but you can't cleanly extract the error code.
Using a signed integer for returning "non-negat
Re: [PATCH 07/14] block/blklogwrites: drop error propagation
On Fri, 11 Sep 2020 07:30:37 +0200
Markus Armbruster wrote:
> Markus Armbruster writes:
>
> > Greg Kurz writes:
> >
> >> On Wed, 9 Sep 2020 21:59:23 +0300
> >> Vladimir Sementsov-Ogievskiy wrote:
> >>
> >>> It's simple to avoid error propagation in blk_log_writes_open(), we
> >>> just need to refactor blk_log_writes_find_cur_log_sector() a bit.
> >>>
> >>> Signed-off-by: Vladimir Sementsov-Ogievskiy
> >>> ---
> >>> block/blklogwrites.c | 23 +++
> >>> 1 file changed, 11 insertions(+), 12 deletions(-)
> >>>
> >>> diff --git a/block/blklogwrites.c b/block/blklogwrites.c
> >>> index 7ef046cee9..c7da507b2d 100644
> >>> --- a/block/blklogwrites.c
> >>> +++ b/block/blklogwrites.c
> >>> @@ -96,10 +96,10 @@ static inline bool
> >>> blk_log_writes_sector_size_valid(uint32_t sector_size)
> >>> sector_size < (1ull << 24);
> >>> }
> >>>
> >>> -static uint64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
> >>> - uint32_t sector_size,
> >>> - uint64_t nr_entries,
> >>> - Error **errp)
> >>> +static int64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
> >>> + uint32_t sector_size,
> >>> + uint64_t nr_entries,
> >>> + Error **errp)
> >>> {
> >>> uint64_t cur_sector = 1;
> >>> uint64_t cur_idx = 0;
> >>> @@ -112,13 +112,13 @@ static uint64_t
> >>> blk_log_writes_find_cur_log_sector(BdrvChild *log,
> >>> if (read_ret < 0) {
> >>> error_setg_errno(errp, -read_ret,
> >>> "Failed to read log entry %"PRIu64,
> >>> cur_idx);
> >>> -return (uint64_t)-1ull;
> >>> +return read_ret;
> >>
> >> This changes the error status of blk_log_writes_open() from -EINVAL to
> >> whatever is returned by bdrv_pread(). I guess this is an improvement
> >> worth to be mentioned in the changelog.
> >>
> >>> }
> >>>
> >>> if (cur_entry.flags & ~cpu_to_le64(LOG_FLAG_MASK)) {
> >>> error_setg(errp, "Invalid flags 0x%"PRIx64" in log entry
> >>> %"PRIu64,
> >>> le64_to_cpu(cur_entry.flags), cur_idx);
> >>> -return (uint64_t)-1ull;
> >>> +return -EINVAL;
> >>> }
> >>>
> >>> /* Account for the sector of the entry itself */
> >>> @@ -143,7 +143,6 @@ static int blk_log_writes_open(BlockDriverState *bs,
> >>> QDict *options, int flags,
> >>> {
> >>> BDRVBlkLogWritesState *s = bs->opaque;
> >>> QemuOpts *opts;
> >>> -Error *local_err = NULL;
> >>> int ret;
> >>> uint64_t log_sector_size;
> >>> bool log_append;
> >>> @@ -215,15 +214,15 @@ static int blk_log_writes_open(BlockDriverState
> >>> *bs, QDict *options, int flags,
> >>> s->nr_entries = 0;
> >>>
> >>> if (blk_log_writes_sector_size_valid(log_sector_size)) {
> >>> -s->cur_log_sector =
> >>> +int64_t cur_log_sector =
> >>> blk_log_writes_find_cur_log_sector(s->log_file,
> >>> log_sector_size,
> >>> -le64_to_cpu(log_sb.nr_entries),
> >>> &local_err);
> >>> -if (local_err) {
> >>> -ret = -EINVAL;
> >>> -error_propagate(errp, local_err);
> >>> +le64_to_cpu(log_sb.nr_entries),
> >>> errp);
> >>> +if (cur_log_sector < 0) {
> >>> +ret = cur_log_sector;
> >>
> >> This is converting int64_t to int, which is usually not recommended.
> >> In practice this is probably okay because cur_log_sector is supposed
> >> to be a negative errno (ie. an int) in this case.
> >
> > It isn't: blk_log_writes_find_cur_log_sector() returns (uint64_t)-1ull
> > on error.
> >
> > Aside: returning uint64_t is awkward. We commonly use a signed integer
> > for non-negative offset or negative error.
> >
> >> Maybe make this clear with a an assert(cur_log_sector >= INT_MIN) ?
> >
> > Unless we change blk_log_writes_find_cur_log_sector() to return a
> > negative errno code,
>
> Which the patch does. I shouldn't review patches before breakfast.
>
:)
> > negative errno code, we need to ret = -EINVAL here. Let's keep this
> > series simple.
>
> No, the patch is okay as is.
>
> Dumbing it down to keep it simple would also be okay.
>
What about Ari's proposal to add ERRP_GUARD() and check errors
with "if (*errp)" like we do for void functions ?
> Regarding the proposed assertion: do we protect similar conversions from
> over-wide negative errno int elsewhere?
>
We do protect int64_t->int conversions in a few places, eg.
qcow2_write_snapshots() or qemu_spice_create_host_primary(),
but I couldn't find one about a negarive errno.
> >>> goto fail_log;
Re: [PATCH 07/14] block/blklogwrites: drop error propagation
Markus Armbruster writes:
> Greg Kurz writes:
>
>> On Wed, 9 Sep 2020 21:59:23 +0300
>> Vladimir Sementsov-Ogievskiy wrote:
>>
>>> It's simple to avoid error propagation in blk_log_writes_open(), we
>>> just need to refactor blk_log_writes_find_cur_log_sector() a bit.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>>> ---
>>> block/blklogwrites.c | 23 +++
>>> 1 file changed, 11 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/block/blklogwrites.c b/block/blklogwrites.c
>>> index 7ef046cee9..c7da507b2d 100644
>>> --- a/block/blklogwrites.c
>>> +++ b/block/blklogwrites.c
>>> @@ -96,10 +96,10 @@ static inline bool
>>> blk_log_writes_sector_size_valid(uint32_t sector_size)
>>> sector_size < (1ull << 24);
>>> }
>>>
>>> -static uint64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
>>> - uint32_t sector_size,
>>> - uint64_t nr_entries,
>>> - Error **errp)
>>> +static int64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
>>> + uint32_t sector_size,
>>> + uint64_t nr_entries,
>>> + Error **errp)
>>> {
>>> uint64_t cur_sector = 1;
>>> uint64_t cur_idx = 0;
>>> @@ -112,13 +112,13 @@ static uint64_t
>>> blk_log_writes_find_cur_log_sector(BdrvChild *log,
>>> if (read_ret < 0) {
>>> error_setg_errno(errp, -read_ret,
>>> "Failed to read log entry %"PRIu64, cur_idx);
>>> -return (uint64_t)-1ull;
>>> +return read_ret;
>>
>> This changes the error status of blk_log_writes_open() from -EINVAL to
>> whatever is returned by bdrv_pread(). I guess this is an improvement
>> worth to be mentioned in the changelog.
>>
>>> }
>>>
>>> if (cur_entry.flags & ~cpu_to_le64(LOG_FLAG_MASK)) {
>>> error_setg(errp, "Invalid flags 0x%"PRIx64" in log entry
>>> %"PRIu64,
>>> le64_to_cpu(cur_entry.flags), cur_idx);
>>> -return (uint64_t)-1ull;
>>> +return -EINVAL;
>>> }
>>>
>>> /* Account for the sector of the entry itself */
>>> @@ -143,7 +143,6 @@ static int blk_log_writes_open(BlockDriverState *bs,
>>> QDict *options, int flags,
>>> {
>>> BDRVBlkLogWritesState *s = bs->opaque;
>>> QemuOpts *opts;
>>> -Error *local_err = NULL;
>>> int ret;
>>> uint64_t log_sector_size;
>>> bool log_append;
>>> @@ -215,15 +214,15 @@ static int blk_log_writes_open(BlockDriverState *bs,
>>> QDict *options, int flags,
>>> s->nr_entries = 0;
>>>
>>> if (blk_log_writes_sector_size_valid(log_sector_size)) {
>>> -s->cur_log_sector =
>>> +int64_t cur_log_sector =
>>> blk_log_writes_find_cur_log_sector(s->log_file,
>>> log_sector_size,
>>> -le64_to_cpu(log_sb.nr_entries),
>>> &local_err);
>>> -if (local_err) {
>>> -ret = -EINVAL;
>>> -error_propagate(errp, local_err);
>>> +le64_to_cpu(log_sb.nr_entries), errp);
>>> +if (cur_log_sector < 0) {
>>> +ret = cur_log_sector;
>>
>> This is converting int64_t to int, which is usually not recommended.
>> In practice this is probably okay because cur_log_sector is supposed
>> to be a negative errno (ie. an int) in this case.
>
> It isn't: blk_log_writes_find_cur_log_sector() returns (uint64_t)-1ull
> on error.
>
> Aside: returning uint64_t is awkward. We commonly use a signed integer
> for non-negative offset or negative error.
>
>> Maybe make this clear with a an assert(cur_log_sector >= INT_MIN) ?
>
> Unless we change blk_log_writes_find_cur_log_sector() to return a
> negative errno code,
Which the patch does. I shouldn't review patches before breakfast.
> negative errno code, we need to ret = -EINVAL here. Let's keep this
> series simple.
No, the patch is okay as is.
Dumbing it down to keep it simple would also be okay.
Regarding the proposed assertion: do we protect similar conversions from
over-wide negative errno int elsewhere?
>>> goto fail_log;
>>> }
>>>
>>> +s->cur_log_sector = cur_log_sector;
>>> s->nr_entries = le64_to_cpu(log_sb.nr_entries);
>>> }
>>> } else {
Re: [PATCH 07/14] block/blklogwrites: drop error propagation
Greg Kurz writes:
> On Wed, 9 Sep 2020 21:59:23 +0300
> Vladimir Sementsov-Ogievskiy wrote:
>
>> It's simple to avoid error propagation in blk_log_writes_open(), we
>> just need to refactor blk_log_writes_find_cur_log_sector() a bit.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>> ---
>> block/blklogwrites.c | 23 +++
>> 1 file changed, 11 insertions(+), 12 deletions(-)
>>
>> diff --git a/block/blklogwrites.c b/block/blklogwrites.c
>> index 7ef046cee9..c7da507b2d 100644
>> --- a/block/blklogwrites.c
>> +++ b/block/blklogwrites.c
>> @@ -96,10 +96,10 @@ static inline bool
>> blk_log_writes_sector_size_valid(uint32_t sector_size)
>> sector_size < (1ull << 24);
>> }
>>
>> -static uint64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
>> - uint32_t sector_size,
>> - uint64_t nr_entries,
>> - Error **errp)
>> +static int64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
>> + uint32_t sector_size,
>> + uint64_t nr_entries,
>> + Error **errp)
>> {
>> uint64_t cur_sector = 1;
>> uint64_t cur_idx = 0;
>> @@ -112,13 +112,13 @@ static uint64_t
>> blk_log_writes_find_cur_log_sector(BdrvChild *log,
>> if (read_ret < 0) {
>> error_setg_errno(errp, -read_ret,
>> "Failed to read log entry %"PRIu64, cur_idx);
>> -return (uint64_t)-1ull;
>> +return read_ret;
>
> This changes the error status of blk_log_writes_open() from -EINVAL to
> whatever is returned by bdrv_pread(). I guess this is an improvement
> worth to be mentioned in the changelog.
>
>> }
>>
>> if (cur_entry.flags & ~cpu_to_le64(LOG_FLAG_MASK)) {
>> error_setg(errp, "Invalid flags 0x%"PRIx64" in log entry
>> %"PRIu64,
>> le64_to_cpu(cur_entry.flags), cur_idx);
>> -return (uint64_t)-1ull;
>> +return -EINVAL;
>> }
>>
>> /* Account for the sector of the entry itself */
>> @@ -143,7 +143,6 @@ static int blk_log_writes_open(BlockDriverState *bs,
>> QDict *options, int flags,
>> {
>> BDRVBlkLogWritesState *s = bs->opaque;
>> QemuOpts *opts;
>> -Error *local_err = NULL;
>> int ret;
>> uint64_t log_sector_size;
>> bool log_append;
>> @@ -215,15 +214,15 @@ static int blk_log_writes_open(BlockDriverState *bs,
>> QDict *options, int flags,
>> s->nr_entries = 0;
>>
>> if (blk_log_writes_sector_size_valid(log_sector_size)) {
>> -s->cur_log_sector =
>> +int64_t cur_log_sector =
>> blk_log_writes_find_cur_log_sector(s->log_file,
>> log_sector_size,
>> -le64_to_cpu(log_sb.nr_entries),
>> &local_err);
>> -if (local_err) {
>> -ret = -EINVAL;
>> -error_propagate(errp, local_err);
>> +le64_to_cpu(log_sb.nr_entries), errp);
>> +if (cur_log_sector < 0) {
>> +ret = cur_log_sector;
>
> This is converting int64_t to int, which is usually not recommended.
> In practice this is probably okay because cur_log_sector is supposed
> to be a negative errno (ie. an int) in this case.
It isn't: blk_log_writes_find_cur_log_sector() returns (uint64_t)-1ull
on error.
Aside: returning uint64_t is awkward. We commonly use a signed integer
for non-negative offset or negative error.
> Maybe make this clear with a an assert(cur_log_sector >= INT_MIN) ?
Unless we change blk_log_writes_find_cur_log_sector() to return a
negative errno code, we need to ret = -EINVAL here. Let's keep this
series simple.
>> goto fail_log;
>> }
>>
>> +s->cur_log_sector = cur_log_sector;
>> s->nr_entries = le64_to_cpu(log_sb.nr_entries);
>> }
>> } else {
Re: [PATCH 07/14] block/blklogwrites: drop error propagation
Hi Vladimir!
Thank you for working on this. My comments below.
On 9/9/20 9:59 PM, Vladimir Sementsov-Ogievskiy wrote:
It's simple to avoid error propagation in blk_log_writes_open(), we
just need to refactor blk_log_writes_find_cur_log_sector() a bit.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block/blklogwrites.c | 23 +++
1 file changed, 11 insertions(+), 12 deletions(-)
diff --git a/block/blklogwrites.c b/block/blklogwrites.c
index 7ef046cee9..c7da507b2d 100644
--- a/block/blklogwrites.c
+++ b/block/blklogwrites.c
@@ -96,10 +96,10 @@ static inline bool
blk_log_writes_sector_size_valid(uint32_t sector_size)
sector_size < (1ull << 24);
}
-static uint64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
- uint32_t sector_size,
- uint64_t nr_entries,
- Error **errp)
+static int64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
I'd rather not change the return type for reasons detailed below.
+ uint32_t sector_size,
+ uint64_t nr_entries,
+ Error **errp)
{
uint64_t cur_sector = 1;
uint64_t cur_idx = 0;
@@ -112,13 +112,13 @@ static uint64_t
blk_log_writes_find_cur_log_sector(BdrvChild *log,
if (read_ret < 0) {
error_setg_errno(errp, -read_ret,
"Failed to read log entry %"PRIu64, cur_idx);
-return (uint64_t)-1ull;
+return read_ret;
This is OK, provided the change in return type signedness is necessary
in the first place.
}
if (cur_entry.flags & ~cpu_to_le64(LOG_FLAG_MASK)) {
error_setg(errp, "Invalid flags 0x%"PRIx64" in log entry %"PRIu64,
le64_to_cpu(cur_entry.flags), cur_idx);
-return (uint64_t)-1ull;
+return -EINVAL;
This is OK, provided the return type signedness change is necessary,
although we already do have errp to indicate any errors.
}
/* Account for the sector of the entry itself */
@@ -143,7 +143,6 @@ static int blk_log_writes_open(BlockDriverState *bs, QDict
*options, int flags,
{
BDRVBlkLogWritesState *s = bs->opaque;
QemuOpts *opts;
-Error *local_err = NULL;
int ret;
uint64_t log_sector_size;
bool log_append;
@@ -215,15 +214,15 @@ static int blk_log_writes_open(BlockDriverState *bs,
QDict *options, int flags,
s->nr_entries = 0;
if (blk_log_writes_sector_size_valid(log_sector_size)) {
-s->cur_log_sector =
+int64_t cur_log_sector =
blk_log_writes_find_cur_log_sector(s->log_file,
log_sector_size,
-le64_to_cpu(log_sb.nr_entries),
&local_err);
-if (local_err) {
-ret = -EINVAL;
-error_propagate(errp, local_err);
+le64_to_cpu(log_sb.nr_entries), errp);
+if (cur_log_sector < 0) {
+ret = cur_log_sector;
This changes the semantics slightly. Changing the return type to int64
may theoretically cause valid return values to be interpreted as errors.
Since we already do have a dedicated out pointer for the error value
(errp), why not use it?
Assigning cur_log_sector to ret looks a bit odd to me. Why not use the
original -EINVAL - or maybe some more appropriate errno value than -EINVAL?
I think the desired effect of this change could be achieved with less
churn. How about just making just the following change in addition to
adding ERRP_GUARD() to the beginning of the function and getting rid of
local_err:
-le64_to_cpu(log_sb.nr_entries),
&local_err);
+le64_to_cpu(log_sb.nr_entries), errp);
-if (local_err) {
+if (*errp) {
ret = -EINVAL;
-error_propagate(errp, local_err);
goto fail_log;
}
+s->cur_log_sector = cur_log_sector;
s->nr_entries = le64_to_cpu(log_sb.nr_entries);
}
} else {
Best regards,
Ari Sundholm
a...@tuxera.com
Re: [PATCH 07/14] block/blklogwrites: drop error propagation
On Wed, 9 Sep 2020 21:59:23 +0300
Vladimir Sementsov-Ogievskiy wrote:
> It's simple to avoid error propagation in blk_log_writes_open(), we
> just need to refactor blk_log_writes_find_cur_log_sector() a bit.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> block/blklogwrites.c | 23 +++
> 1 file changed, 11 insertions(+), 12 deletions(-)
>
> diff --git a/block/blklogwrites.c b/block/blklogwrites.c
> index 7ef046cee9..c7da507b2d 100644
> --- a/block/blklogwrites.c
> +++ b/block/blklogwrites.c
> @@ -96,10 +96,10 @@ static inline bool
> blk_log_writes_sector_size_valid(uint32_t sector_size)
> sector_size < (1ull << 24);
> }
>
> -static uint64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
> - uint32_t sector_size,
> - uint64_t nr_entries,
> - Error **errp)
> +static int64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
> + uint32_t sector_size,
> + uint64_t nr_entries,
> + Error **errp)
> {
> uint64_t cur_sector = 1;
> uint64_t cur_idx = 0;
> @@ -112,13 +112,13 @@ static uint64_t
> blk_log_writes_find_cur_log_sector(BdrvChild *log,
> if (read_ret < 0) {
> error_setg_errno(errp, -read_ret,
> "Failed to read log entry %"PRIu64, cur_idx);
> -return (uint64_t)-1ull;
> +return read_ret;
This changes the error status of blk_log_writes_open() from -EINVAL to
whatever is returned by bdrv_pread(). I guess this is an improvement
worth to be mentioned in the changelog.
> }
>
> if (cur_entry.flags & ~cpu_to_le64(LOG_FLAG_MASK)) {
> error_setg(errp, "Invalid flags 0x%"PRIx64" in log entry
> %"PRIu64,
> le64_to_cpu(cur_entry.flags), cur_idx);
> -return (uint64_t)-1ull;
> +return -EINVAL;
> }
>
> /* Account for the sector of the entry itself */
> @@ -143,7 +143,6 @@ static int blk_log_writes_open(BlockDriverState *bs,
> QDict *options, int flags,
> {
> BDRVBlkLogWritesState *s = bs->opaque;
> QemuOpts *opts;
> -Error *local_err = NULL;
> int ret;
> uint64_t log_sector_size;
> bool log_append;
> @@ -215,15 +214,15 @@ static int blk_log_writes_open(BlockDriverState *bs,
> QDict *options, int flags,
> s->nr_entries = 0;
>
> if (blk_log_writes_sector_size_valid(log_sector_size)) {
> -s->cur_log_sector =
> +int64_t cur_log_sector =
> blk_log_writes_find_cur_log_sector(s->log_file,
> log_sector_size,
> -le64_to_cpu(log_sb.nr_entries),
> &local_err);
> -if (local_err) {
> -ret = -EINVAL;
> -error_propagate(errp, local_err);
> +le64_to_cpu(log_sb.nr_entries), errp);
> +if (cur_log_sector < 0) {
> +ret = cur_log_sector;
This is converting int64_t to int, which is usually not recommended.
In practice this is probably okay because cur_log_sector is supposed
to be a negative errno (ie. an int) in this case.
Maybe make this clear with a an assert(cur_log_sector >= INT_MIN) ?
> goto fail_log;
> }
>
> +s->cur_log_sector = cur_log_sector;
> s->nr_entries = le64_to_cpu(log_sb.nr_entries);
> }
> } else {
