[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [RFC v5 067/126] block: introduce ERRP_AUTO_PROPAGATE
From: |
Eric Blake |
Subject: |
Re: [RFC v5 067/126] block: introduce ERRP_AUTO_PROPAGATE |
Date: |
Fri, 11 Oct 2019 14:15:38 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.1.0 |
On 10/11/19 11:04 AM, Vladimir Sementsov-Ogievskiy wrote:
If we want to add some info to errp (by error_prepend() or
error_append_hint()), we must use the ERRP_AUTO_PROPAGATE macro.
Otherwise, this info will not be added when errp == &fatal_err
(the program will exit prior to the error_append_hint() or
error_prepend() call). Fix such cases.
If we want to check error after errp-function call, we need to
introduce local_err and than propagate it to errp. Instead, use
ERRP_AUTO_PROPAGATE macro, benefits are:
1. No need of explicit error_propagate call
2. No need of explicit local_err variable: use errp directly
3. ERRP_AUTO_PROPAGATE leaves errp as is if it's not NULL or
&error_fatel, this means that we don't break error_abort
(we'll abort on error_set, not on error_propagate)
Reported-by: Kevin Wolf <address@hidden>
Reported-by: Greg Kurz <address@hidden>
Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
---
17 files changed, 287 insertions(+), 381 deletions(-)
A bit large, but still manageable for manually checking.
+++ b/block.c
@@ -1565,6 +1558,7 @@ fail_opts:
static QDict *parse_json_filename(const char *filename, Error **errp)
{
+ ERRP_AUTO_PROPAGATE();
QObject *options_obj;
QDict *options;
int ret;
Not shown in the diff, but this one is correct because of the use of
error_prepend().
It does make me wonder if separating the error_prepend/error_hint vs.
error_propagate into two separate cleanups (by incrementally amending
the .cocci script between two series of generated patches) makes the
overall review easier or harder. But don't respin the series just
because of me - wait for feedback from Markus as well on the best path
forward.
+++ b/block/backup.c
@@ -583,6 +583,7 @@ static const BlockJobDriver backup_job_driver = {
static int64_t backup_calculate_cluster_size(BlockDriverState *target,
Error **errp)
{
+ ERRP_AUTO_PROPAGATE();
int ret;
BlockDriverInfo bdi;
Here, the diff is correct because of error_append_hint.
+++ b/block/dirty-bitmap.c
@@ -237,6 +237,7 @@ static bool bdrv_dirty_bitmap_recording(BdrvDirtyBitmap
*bitmap)
int bdrv_dirty_bitmap_check(const BdrvDirtyBitmap *bitmap, uint32_t flags,
Error **errp)
{
+ ERRP_AUTO_PROPAGATE();
if ((flags & BDRV_BITMAP_BUSY) && bdrv_dirty_bitmap_busy(bitmap)) {
error_setg(errp, "Bitmap '%s' is currently in use by another"
" operation and cannot be used", bitmap->name);
and here
+++ b/block/qapi.c
@@ -44,6 +44,7 @@
BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
BlockDriverState *bs, Error **errp)
{
+ ERRP_AUTO_PROPAGATE();
ImageInfo **p_image_info;
BlockDriverState *bs0;
BlockDeviceInfo *info;
@@ -148,10 +149,8 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
p_image_info = &info->image;
info->backing_file_depth = 0;
while (1) {
- Error *local_err = NULL;
- bdrv_query_image_info(bs0, p_image_info, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ bdrv_query_image_info(bs0, p_image_info, errp);
+ if (*errp) {
qapi_free_BlockDeviceInfo(info);
return NULL;
}
I'm a bit wary of auto-propagate at the top level, but a local variable
declared inside a loop - if the loop repeats more than once, we need to
make sure that no subsequent iteration of the loop gets handed an
already-set errp from an earlier iteration. In this case, we're safe
(the loop exits early due to return), but it's something to look out for
in other patches.
+++ b/hw/block/onenand.c
A bit odd to group this with the others. But not the end of the world.
Reviewed-by: Eric Blake <address@hidden>
So far, however, I have not tried rerunning the Coccinelle script, or
checking (perhaps by sed) whether there are missed opportunities in
these files that Coccinelle missed.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
- [RFC v5 094/126] RBD: introduce ERRP_AUTO_PROPAGATE, (continued)
- [RFC v5 094/126] RBD: introduce ERRP_AUTO_PROPAGATE, Vladimir Sementsov-Ogievskiy, 2019/10/11
- [RFC v5 052/126] vhost: introduce ERRP_AUTO_PROPAGATE, Vladimir Sementsov-Ogievskiy, 2019/10/11
- [RFC v5 060/126] megasas: introduce ERRP_AUTO_PROPAGATE, Vladimir Sementsov-Ogievskiy, 2019/10/11
- [RFC v5 095/126] Sheepdog: introduce ERRP_AUTO_PROPAGATE, Vladimir Sementsov-Ogievskiy, 2019/10/11
- [RFC v5 093/126] VMDK: introduce ERRP_AUTO_PROPAGATE, Vladimir Sementsov-Ogievskiy, 2019/10/11
- [RFC v5 097/126] VDI: introduce ERRP_AUTO_PROPAGATE, Vladimir Sementsov-Ogievskiy, 2019/10/11
- [RFC v5 098/126] iSCSI: introduce ERRP_AUTO_PROPAGATE, Vladimir Sementsov-Ogievskiy, 2019/10/11
- [RFC v5 099/126] nbd: introduce ERRP_AUTO_PROPAGATE, Vladimir Sementsov-Ogievskiy, 2019/10/11
- [RFC v5 067/126] block: introduce ERRP_AUTO_PROPAGATE, Vladimir Sementsov-Ogievskiy, 2019/10/11
- Re: [RFC v5 067/126] block: introduce ERRP_AUTO_PROPAGATE,
Eric Blake <=
- [RFC v5 026/126] python: add commit-per-subsystem.py, Vladimir Sementsov-Ogievskiy, 2019/10/11
- [RFC v5 025/126] scripts: add coccinelle script to use auto propagated errp, Vladimir Sementsov-Ogievskiy, 2019/10/11
- [RFC v5 024/126] error: auto propagated local_err, Vladimir Sementsov-Ogievskiy, 2019/10/11
- [RFC v5 103/126] GLUSTER: introduce ERRP_AUTO_PROPAGATE, Vladimir Sementsov-Ogievskiy, 2019/10/11
- [RFC v5 115/126] vpc: introduce ERRP_AUTO_PROPAGATE, Vladimir Sementsov-Ogievskiy, 2019/10/11
- [RFC v5 116/126] vvfat: introduce ERRP_AUTO_PROPAGATE, Vladimir Sementsov-Ogievskiy, 2019/10/11