Ensure that hints are added even if errp is &error_fatal or &error_abort.
Signed-off-by: Greg Kurz <groug@kaod.org>
---
block/backup.c | 7 +++++--
block/dirty-bitmap.c | 7 +++++--
block/file-posix.c | 20 +++++++++++++-------
block/gluster.c | 23 +++++++++++++++--------
block/qcow.c | 10 ++++++----
block/qcow2.c | 7 +++++--
block/vhdx-log.c | 7 +++++--
block/vpc.c | 7 +++++--
8 files changed, 59 insertions(+), 29 deletions(-)
diff --git a/block/backup.c b/block/backup.c
index 763f0d7ff6db..d8c422a0e3bc 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -602,11 +602,14 @@ static int64_t backup_calculate_cluster_size(BlockDriverState *target,
BACKUP_CLUSTER_SIZE_DEFAULT);
return BACKUP_CLUSTER_SIZE_DEFAULT;
} else if (ret < 0 && !target->backing) {
- error_setg_errno(errp, -ret,
+ Error *local_err = NULL;
+
+ error_setg_errno(&local_err, -ret,
"Couldn't determine the cluster size of the target image, "
"which has no backing file");
- error_append_hint(errp,
+ error_append_hint(&local_err,
"Aborting, since this may create an unusable destination image\n");
+ error_propagate(errp, local_err);
return ret;
} else if (ret < 0 && target->backing) {
/* Not fatal; just trudge on ahead. */
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 134e0c9a0c8f..b31017a77d51 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -251,10 +251,13 @@ int bdrv_dirty_bitmap_check(const BdrvDirtyBitmap *bitmap, uint32_t flags,
if ((flags & BDRV_BITMAP_INCONSISTENT) &&
bdrv_dirty_bitmap_inconsistent(bitmap)) {
- error_setg(errp, "Bitmap '%s' is inconsistent and cannot be used",
+ Error *local_err = NULL;
+
+ error_setg(&local_err, "Bitmap '%s' is inconsistent and cannot be used",
bitmap->name);
- error_append_hint(errp, "Try block-dirty-bitmap-remove to delete"
+ error_append_hint(&local_err, "Try block-dirty-bitmap-remove to delete"
" this bitmap from disk");
+ error_propagate(errp, local_err);
return -1;
}
diff --git a/block/file-posix.c b/block/file-posix.c
index f12c06de2df5..9a95f7e94123 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -389,8 +389,11 @@ static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
}
if (!s->buf_align || !bs->bl.request_alignment) {
- error_setg(errp, "Could not find working O_DIRECT alignment");
- error_append_hint(errp, "Try cache.direct=off\n");
+ Error *local_err = NULL;
+
+ error_setg(&local_err, "Could not find working O_DIRECT alignment");
+ error_append_hint(&local_err, "Try cache.direct=off\n");
+ error_propagate(errp, local_err);
}
}
@@ -845,16 +848,18 @@ static int raw_handle_perm_lock(BlockDriverState *bs,
}
ret = raw_apply_lock_bytes(s, s->fd, s->perm | new_perm,
~s->shared_perm | ~new_shared,
- false, errp);
+ false, &local_err);
if (!ret) {
- ret = raw_check_lock_bytes(s->fd, new_perm, new_shared, errp);
+ ret = raw_check_lock_bytes(s->fd, new_perm, new_shared, &local_err);
if (!ret) {
return 0;
}
- error_append_hint(errp,
+ error_append_hint(&local_err,
"Is another process using the image [%s]?\n",
bs->filename);
}
+ error_propagate(errp, local_err);
+ local_err = NULL; /* for fall through */
op = RAW_PL_ABORT;
/* fall through to unlock bytes. */
case RAW_PL_ABORT:
@@ -2277,11 +2282,12 @@ raw_co_create(BlockdevCreateOptions *options, Error **errp)
}
/* Step two: Check that nobody else has taken conflicting locks */
- result = raw_check_lock_bytes(fd, perm, shared, errp);
+ result = raw_check_lock_bytes(fd, perm, shared, &local_err);
if (result < 0) {
- error_append_hint(errp,
+ error_append_hint(&local_err,
"Is another process using the image [%s]?\n",
file_opts->filename);
+ error_propagate(errp, local_err);
goto out_unlock;
}
diff --git a/block/gluster.c b/block/gluster.c
index 64028b2cba30..dce42884f97c 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -473,26 +473,29 @@ static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
ret = glfs_init(glfs);
if (ret) {
- error_setg(errp, "Gluster connection for volume %s, path %s failed"
+ Error *local_err = NULL;
+
+ error_setg(&local_err, "Gluster connection for volume %s, path %s failed"
" to connect", gconf->volume, gconf->path);
for (server = gconf->server; server; server = server->next) {
if (server->value->type == SOCKET_ADDRESS_TYPE_UNIX) {
- error_append_hint(errp, "hint: failed on socket %s ",
+ error_append_hint(&local_err, "hint: failed on socket %s ",
server->value->u.q_unix.path);
} else {
- error_append_hint(errp, "hint: failed on host %s and port %s ",
+ error_append_hint(&local_err, "hint: failed on host %s and port %s ",
server->value->u.inet.host,
server->value->u.inet.port);
}
}
- error_append_hint(errp, "Please refer to gluster logs for more info\n");
+ error_append_hint(&local_err, "Please refer to gluster logs for more info\n");
/* glfs_init sometimes doesn't set errno although docs suggest that */
if (errno == 0) {
errno = EINVAL;
}
+ error_propagate(errp, local_err);
goto out;
}
return glfs;
@@ -695,20 +698,23 @@ static int qemu_gluster_parse(BlockdevOptionsGluster *gconf,
QDict *options, Error **errp)
{
int ret;
+ Error *local_err = NULL;
+
if (filename) {
ret = qemu_gluster_parse_uri(gconf, filename);
if (ret < 0) {
- error_setg(errp, "invalid URI %s", filename);
- error_append_hint(errp, "Usage: file=gluster[+transport]://"
+ error_setg(&local_err, "invalid URI %s", filename);
+ error_append_hint(&local_err, "Usage: file=gluster[+transport]://"
"[host[:port]]volume/path[?socket=...]"
"[,file.debug=N]"
"[,file.logfile=/path/filename.log]\n");
+ error_propagate(errp, local_err);
return ret;
}
} else {
- ret = qemu_gluster_parse_json(gconf, options, errp);
+ ret = qemu_gluster_parse_json(gconf, options, &local_err);
if (ret < 0) {
- error_append_hint(errp, "Usage: "
+ error_append_hint(&local_err, "Usage: "
"-drive driver=qcow2,file.driver=gluster,"
"file.volume=testvol,file.path=/path/a.qcow2"
"[,file.debug=9]"
@@ -719,6 +725,7 @@ static int qemu_gluster_parse(BlockdevOptionsGluster *gconf,
"file.server.1.transport=unix,"
"file.server.1.path=/var/run/glusterd.socket ..."
"\n");
+ error_propagate(errp, local_err);
return ret;
}
}
diff --git a/block/qcow.c b/block/qcow.c
index 5bdf72ba33ce..9049573b52b2 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -156,11 +156,12 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
goto fail;
}
if (header.version != QCOW_VERSION) {
- error_setg(errp, "qcow (v%d) does not support qcow version %" PRIu32,
+ error_setg(&local_err, "qcow (v%d) does not support qcow version %" PRIu32,
QCOW_VERSION, header.version);
if (header.version == 2 || header.version == 3) {
- error_append_hint(errp, "Try the 'qcow2' driver instead.\n");
+ error_append_hint(&local_err, "Try the 'qcow2' driver instead.\n");
}
+ error_propagate(errp, local_err);
ret = -ENOTSUP;
goto fail;
@@ -189,14 +190,15 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
if (s->crypt_method_header) {
if (bdrv_uses_whitelist() &&
s->crypt_method_header == QCOW_CRYPT_AES) {
- error_setg(errp,
+ error_setg(&local_err,
"Use of AES-CBC encrypted qcow images is no longer "
"supported in system emulators");
- error_append_hint(errp,
+ error_append_hint(&local_err,
"You can use 'qemu-img convert' to convert your "
"image to an alternative supported format, such "
"as unencrypted qcow, or raw with the LUKS "
"format instead.\n");
+ error_propagate(errp, local_err);
ret = -ENOSYS;
goto fail;
}
diff --git a/block/qcow2.c b/block/qcow2.c
index 57734f20cf8d..2c52139b7385 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1357,14 +1357,17 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
if (s->crypt_method_header) {
if (bdrv_uses_whitelist() &&
s->crypt_method_header == QCOW_CRYPT_AES) {
- error_setg(errp,
+ Error *local_err = NULL;
+
+ error_setg(&local_err,
"Use of AES-CBC encrypted qcow2 images is no longer "
"supported in system emulators");
- error_append_hint(errp,
+ error_append_hint(&local_err,
"You can use 'qemu-img convert' to convert your "
"image to an alternative supported format, such "
"as unencrypted qcow2, or raw with the LUKS "
"format instead.\n");
+ error_propagate(errp, local_err);
ret = -ENOSYS;
goto fail;
}
diff --git a/block/vhdx-log.c b/block/vhdx-log.c
index fdd3a7adc378..9f4de9cbcdb6 100644
--- a/block/vhdx-log.c
+++ b/block/vhdx-log.c
@@ -802,15 +802,18 @@ int vhdx_parse_log(BlockDriverState *bs, BDRVVHDXState *s, bool *flushed,
if (logs.valid) {
if (bs->read_only) {
+ Error *local_err = NULL;
+
bdrv_refresh_filename(bs);
ret = -EPERM;
- error_setg(errp,
+ error_setg(&local_err,
"VHDX image file '%s' opened read-only, but "
"contains a log that needs to be replayed",
bs->filename);
- error_append_hint(errp, "To replay the log, run:\n"
+ error_append_hint(&local_err, "To replay the log, run:\n"
"qemu-img check -r all '%s'\n",
bs->filename);
+ error_propagate(errp, local_err);
goto exit;
}
/* now flush the log */
diff --git a/block/vpc.c b/block/vpc.c
index 5cd38907808b..facd7cdb2c98 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -1028,12 +1028,15 @@ static int coroutine_fn vpc_co_create(BlockdevCreateOptions *opts,
}
if (total_size != total_sectors * BDRV_SECTOR_SIZE) {
- error_setg(errp, "The requested image size cannot be represented in "
+ Error *local_err = NULL;
+
+ error_setg(&local_err, "The requested image size cannot be represented in "
"CHS geometry");
- error_append_hint(errp, "Try size=%llu or force-size=on (the "
+ error_append_hint(&local_err, "Try size=%llu or force-size=on (the "
"latter makes the image incompatible with "
"Virtual PC)",
total_sectors * BDRV_SECTOR_SIZE);
+ error_propagate(errp, local_err);
ret = -EINVAL;
goto out;
}
17.09.2019 13:20, Greg Kurz wrote:
> Ensure that hints are added even if errp is &error_fatal or &error_abort.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
> block/backup.c | 7 +++++--
> block/dirty-bitmap.c | 7 +++++--
> block/file-posix.c | 20 +++++++++++++-------
> block/gluster.c | 23 +++++++++++++++--------
> block/qcow.c | 10 ++++++----
> block/qcow2.c | 7 +++++--
> block/vhdx-log.c | 7 +++++--
> block/vpc.c | 7 +++++--
> 8 files changed, 59 insertions(+), 29 deletions(-)
>
> diff --git a/block/backup.c b/block/backup.c
> index 763f0d7ff6db..d8c422a0e3bc 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -602,11 +602,14 @@ static int64_t backup_calculate_cluster_size(BlockDriverState *target,
> BACKUP_CLUSTER_SIZE_DEFAULT);
> return BACKUP_CLUSTER_SIZE_DEFAULT;
> } else if (ret < 0 && !target->backing) {
> - error_setg_errno(errp, -ret,
> + Error *local_err = NULL;
> +
> + error_setg_errno(&local_err, -ret,
> "Couldn't determine the cluster size of the target image, "
> "which has no backing file");
> - error_append_hint(errp,
> + error_append_hint(&local_err,
> "Aborting, since this may create an unusable destination image\n");
> + error_propagate(errp, local_err);
> return ret;
> } else if (ret < 0 && target->backing) {
> /* Not fatal; just trudge on ahead. */
Pain.. Do we need these hints, if they are so painful?
At least for cases like this, we can create helper function
error_setg_errno_hint(..., error, hint)
But what could be done when we call function, which may or may not set errp?
ret = f(errp);
if (ret) {
error_append_hint(errp, hint);
}
Hmmm..
Can it look like
ret = f(..., hint_helper(errp, hint))
?
I can't imagine how to do it, as someone should remove hint from error_abort object on
success path..
But seems, the following is possible, which seems better for me than local-error approach:
error_push_hint(errp, hint);
ret = f(.., errp);
error_pop_hint(errp);
===
Continue thinking on this:
It may look like just
ret = f(..., set_hint(errp, hint));
or (just to split long line):
set_hint(errp, hint);
ret = f(..., errp);
if in each function with errp does error_push_hint(errp) on start and error_pop_hint(errp) on exit,
which may be just one call at function start of macro, which will call error_push_hint(errp) and
define local variable by g_auto, with cleanup which will call error_pop_hint(errp) on function
exit..
Or, may be, more direct way to set cleanup for function exists?
===
Also, we can implement some code generation, to generate for functions with errp argument
wrappers with additional hint parameter, and just use these wrappers..
===
If nobody likes any of my suggestions, then ignore them. I understand that this series fixes
real issue and much effort has already been spent. May be one day I'll try to rewrite it...
--
Best regards,
Vladimir
On Tue, 17 Sep 2019 13:25:03 +0000
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> 17.09.2019 13:20, Greg Kurz wrote:
> > Ensure that hints are added even if errp is &error_fatal or &error_abort.
> >
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> > block/backup.c | 7 +++++--
> > block/dirty-bitmap.c | 7 +++++--
> > block/file-posix.c | 20 +++++++++++++-------
> > block/gluster.c | 23 +++++++++++++++--------
> > block/qcow.c | 10 ++++++----
> > block/qcow2.c | 7 +++++--
> > block/vhdx-log.c | 7 +++++--
> > block/vpc.c | 7 +++++--
> > 8 files changed, 59 insertions(+), 29 deletions(-)
> >
> > diff --git a/block/backup.c b/block/backup.c
> > index 763f0d7ff6db..d8c422a0e3bc 100644
> > --- a/block/backup.c
> > +++ b/block/backup.c
> > @@ -602,11 +602,14 @@ static int64_t backup_calculate_cluster_size(BlockDriverState *target,
> > BACKUP_CLUSTER_SIZE_DEFAULT);
> > return BACKUP_CLUSTER_SIZE_DEFAULT;
> > } else if (ret < 0 && !target->backing) {
> > - error_setg_errno(errp, -ret,
> > + Error *local_err = NULL;
> > +
> > + error_setg_errno(&local_err, -ret,
> > "Couldn't determine the cluster size of the target image, "
> > "which has no backing file");
> > - error_append_hint(errp,
> > + error_append_hint(&local_err,
> > "Aborting, since this may create an unusable destination image\n");
> > + error_propagate(errp, local_err);
> > return ret;
> > } else if (ret < 0 && target->backing) {
> > /* Not fatal; just trudge on ahead. */
>
>
> Pain.. Do we need these hints, if they are so painful?
>
I agree that the one above doesn't qualify as a useful hint.
It just tells that QEMU is giving up and gives no indication
to the user on how to avoid the issue. It should probably be
dropped.
> At least for cases like this, we can create helper function
>
> error_setg_errno_hint(..., error, hint)
Not very useful if hint has to be forged separately with
g_sprintf(), and we can't have such a thing as:
error_setg_errno_hint(errp, err_fmt, ..., hint_fmt, ...)
>
> But what could be done when we call function, which may or may not set errp?
>
> ret = f(errp);
> if (ret) {
> error_append_hint(errp, hint);
> }
>
Same problem. If errp is &error_fatal and f() does errno_setg(errp), it
ends up calling exit().
> Hmmm..
>
> Can it look like
>
> ret = f(..., hint_helper(errp, hint))
>
> ?
>
Nope, hint_helper() will get called before f() and things are worse.
If errp is NULL then error_append_hint() does nothing and if it is
&error_fatal then it aborts.
> I can't imagine how to do it, as someone should remove hint from error_abort object on
> success path..
>
> But seems, the following is possible, which seems better for me than local-error approach:
>
> error_push_hint(errp, hint);
> ret = f(.., errp);
> error_pop_hint(errp);
>
Matter of taste... also, it looks awkward to come up with a hint
before knowing what happened. I mean the appropriate hint could
depend on the value returned by f() and/or errno for example.
> ===
>
> Continue thinking on this:
>
> It may look like just
> ret = f(..., set_hint(errp, hint));
>
> or (just to split long line):
> set_hint(errp, hint);
> ret = f(..., errp);
>
> if in each function with errp does error_push_hint(errp) on start and error_pop_hint(errp) on exit,
> which may be just one call at function start of macro, which will call error_push_hint(errp) and
> define local variable by g_auto, with cleanup which will call error_pop_hint(errp) on function
> exit..
>
> Or, may be, more direct way to set cleanup for function exists?
>
> ===
>
> Also, we can implement some code generation, to generate for functions with errp argument
> wrappers with additional hint parameter, and just use these wrappers..
>
> ===
>
> If nobody likes any of my suggestions, then ignore them. I understand that this series fixes
> real issue and much effort has already been spent. May be one day I'll try to rewrite it...
>
For the reason exposed above, I don't think it makes sense to
build the hint before calling a function that calls error_setg().
I'm afraid we're stuck with local_err... it is then up to the
people to make it as less painful as possible.
17.09.2019 18:37, Greg Kurz wrote:
> On Tue, 17 Sep 2019 13:25:03 +0000
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>
>> 17.09.2019 13:20, Greg Kurz wrote:
>>> Ensure that hints are added even if errp is &error_fatal or &error_abort.
>>>
>>> Signed-off-by: Greg Kurz <groug@kaod.org>
>>> ---
>>> block/backup.c | 7 +++++--
>>> block/dirty-bitmap.c | 7 +++++--
>>> block/file-posix.c | 20 +++++++++++++-------
>>> block/gluster.c | 23 +++++++++++++++--------
>>> block/qcow.c | 10 ++++++----
>>> block/qcow2.c | 7 +++++--
>>> block/vhdx-log.c | 7 +++++--
>>> block/vpc.c | 7 +++++--
>>> 8 files changed, 59 insertions(+), 29 deletions(-)
>>>
>>> diff --git a/block/backup.c b/block/backup.c
>>> index 763f0d7ff6db..d8c422a0e3bc 100644
>>> --- a/block/backup.c
>>> +++ b/block/backup.c
>>> @@ -602,11 +602,14 @@ static int64_t backup_calculate_cluster_size(BlockDriverState *target,
>>> BACKUP_CLUSTER_SIZE_DEFAULT);
>>> return BACKUP_CLUSTER_SIZE_DEFAULT;
>>> } else if (ret < 0 && !target->backing) {
>>> - error_setg_errno(errp, -ret,
>>> + Error *local_err = NULL;
>>> +
>>> + error_setg_errno(&local_err, -ret,
>>> "Couldn't determine the cluster size of the target image, "
>>> "which has no backing file");
>>> - error_append_hint(errp,
>>> + error_append_hint(&local_err,
>>> "Aborting, since this may create an unusable destination image\n");
>>> + error_propagate(errp, local_err);
>>> return ret;
>>> } else if (ret < 0 && target->backing) {
>>> /* Not fatal; just trudge on ahead. */
>>
>>
>> Pain.. Do we need these hints, if they are so painful?
>>
>
> I agree that the one above doesn't qualify as a useful hint.
> It just tells that QEMU is giving up and gives no indication
> to the user on how to avoid the issue. It should probably be
> dropped.
>
>> At least for cases like this, we can create helper function
>>
>> error_setg_errno_hint(..., error, hint)
>
> Not very useful if hint has to be forged separately with
> g_sprintf(), and we can't have such a thing as:
>
> error_setg_errno_hint(errp, err_fmt, ..., hint_fmt, ...)
>
>>
>> But what could be done when we call function, which may or may not set errp?
>>
>> ret = f(errp);
>> if (ret) {
>> error_append_hint(errp, hint);
>> }
>>
>
> Same problem. If errp is &error_fatal and f() does errno_setg(errp), it
> ends up calling exit().
>
>> Hmmm..
>>
>> Can it look like
>>
>> ret = f(..., hint_helper(errp, hint))
>>
>> ?
>>
>
> Nope, hint_helper() will get called before f() and things are worse.
> If errp is NULL then error_append_hint() does nothing and if it is
> &error_fatal then it aborts.
>
>> I can't imagine how to do it, as someone should remove hint from error_abort object on
>> success path..
>>
>> But seems, the following is possible, which seems better for me than local-error approach:
>>
>> error_push_hint(errp, hint);
>> ret = f(.., errp);
>> error_pop_hint(errp);
>>
>
> Matter of taste... also, it looks awkward to come up with a hint
> before knowing what happened. I mean the appropriate hint could
> depend on the value returned by f() and/or errno for example.
>
>> ===
>>
>> Continue thinking on this:
>>
>> It may look like just
>> ret = f(..., set_hint(errp, hint));
>>
>> or (just to split long line):
>> set_hint(errp, hint);
>> ret = f(..., errp);
>>
>> if in each function with errp does error_push_hint(errp) on start and error_pop_hint(errp) on exit,
>> which may be just one call at function start of macro, which will call error_push_hint(errp) and
>> define local variable by g_auto, with cleanup which will call error_pop_hint(errp) on function
>> exit..
>>
>> Or, may be, more direct way to set cleanup for function exists?
>>
>> ===
>>
>> Also, we can implement some code generation, to generate for functions with errp argument
>> wrappers with additional hint parameter, and just use these wrappers..
>>
>> ===
>>
>> If nobody likes any of my suggestions, then ignore them. I understand that this series fixes
>> real issue and much effort has already been spent. May be one day I'll try to rewrite it...
>>
>
> For the reason exposed above, I don't think it makes sense to
> build the hint before calling a function that calls error_setg().
> I'm afraid we're stuck with local_err... it is then up to the
> people to make it as less painful as possible.
>
Hmm. so, seems that in general we need local_err..
Then may be, may can make automated propagation?
It will look like
g_auto(ErrorPropagation) _error_prop = (ErrorPropagation){
.errp = errp,
.local_err = NULL,
}
errp = &_error_prop.local_err;
and this thing may be fully covered into macro,
to look like this at function start (to be honest it should exactly after all
local variable definitions):
MAKE_ERRP_SAFE(_error_prop, errp);
--
Best regards,
Vladimir
On Tue, 17 Sep 2019 17:40:11 +0000
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> 17.09.2019 18:37, Greg Kurz wrote:
> > On Tue, 17 Sep 2019 13:25:03 +0000
> > Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> >
> >> 17.09.2019 13:20, Greg Kurz wrote:
> >>> Ensure that hints are added even if errp is &error_fatal or &error_abort.
> >>>
> >>> Signed-off-by: Greg Kurz <groug@kaod.org>
> >>> ---
> >>> block/backup.c | 7 +++++--
> >>> block/dirty-bitmap.c | 7 +++++--
> >>> block/file-posix.c | 20 +++++++++++++-------
> >>> block/gluster.c | 23 +++++++++++++++--------
> >>> block/qcow.c | 10 ++++++----
> >>> block/qcow2.c | 7 +++++--
> >>> block/vhdx-log.c | 7 +++++--
> >>> block/vpc.c | 7 +++++--
> >>> 8 files changed, 59 insertions(+), 29 deletions(-)
> >>>
> >>> diff --git a/block/backup.c b/block/backup.c
> >>> index 763f0d7ff6db..d8c422a0e3bc 100644
> >>> --- a/block/backup.c
> >>> +++ b/block/backup.c
> >>> @@ -602,11 +602,14 @@ static int64_t backup_calculate_cluster_size(BlockDriverState *target,
> >>> BACKUP_CLUSTER_SIZE_DEFAULT);
> >>> return BACKUP_CLUSTER_SIZE_DEFAULT;
> >>> } else if (ret < 0 && !target->backing) {
> >>> - error_setg_errno(errp, -ret,
> >>> + Error *local_err = NULL;
> >>> +
> >>> + error_setg_errno(&local_err, -ret,
> >>> "Couldn't determine the cluster size of the target image, "
> >>> "which has no backing file");
> >>> - error_append_hint(errp,
> >>> + error_append_hint(&local_err,
> >>> "Aborting, since this may create an unusable destination image\n");
> >>> + error_propagate(errp, local_err);
> >>> return ret;
> >>> } else if (ret < 0 && target->backing) {
> >>> /* Not fatal; just trudge on ahead. */
> >>
> >>
> >> Pain.. Do we need these hints, if they are so painful?
> >>
> >
> > I agree that the one above doesn't qualify as a useful hint.
> > It just tells that QEMU is giving up and gives no indication
> > to the user on how to avoid the issue. It should probably be
> > dropped.
> >
> >> At least for cases like this, we can create helper function
> >>
> >> error_setg_errno_hint(..., error, hint)
> >
> > Not very useful if hint has to be forged separately with
> > g_sprintf(), and we can't have such a thing as:
> >
> > error_setg_errno_hint(errp, err_fmt, ..., hint_fmt, ...)
> >
> >>
> >> But what could be done when we call function, which may or may not set errp?
> >>
> >> ret = f(errp);
> >> if (ret) {
> >> error_append_hint(errp, hint);
> >> }
> >>
> >
> > Same problem. If errp is &error_fatal and f() does errno_setg(errp), it
> > ends up calling exit().
> >
> >> Hmmm..
> >>
> >> Can it look like
> >>
> >> ret = f(..., hint_helper(errp, hint))
> >>
> >> ?
> >>
> >
> > Nope, hint_helper() will get called before f() and things are worse.
> > If errp is NULL then error_append_hint() does nothing and if it is
> > &error_fatal then it aborts.
> >
> >> I can't imagine how to do it, as someone should remove hint from error_abort object on
> >> success path..
> >>
> >> But seems, the following is possible, which seems better for me than local-error approach:
> >>
> >> error_push_hint(errp, hint);
> >> ret = f(.., errp);
> >> error_pop_hint(errp);
> >>
> >
> > Matter of taste... also, it looks awkward to come up with a hint
> > before knowing what happened. I mean the appropriate hint could
> > depend on the value returned by f() and/or errno for example.
> >
> >> ===
> >>
> >> Continue thinking on this:
> >>
> >> It may look like just
> >> ret = f(..., set_hint(errp, hint));
> >>
> >> or (just to split long line):
> >> set_hint(errp, hint);
> >> ret = f(..., errp);
> >>
> >> if in each function with errp does error_push_hint(errp) on start and error_pop_hint(errp) on exit,
> >> which may be just one call at function start of macro, which will call error_push_hint(errp) and
> >> define local variable by g_auto, with cleanup which will call error_pop_hint(errp) on function
> >> exit..
> >>
> >> Or, may be, more direct way to set cleanup for function exists?
> >>
> >> ===
> >>
> >> Also, we can implement some code generation, to generate for functions with errp argument
> >> wrappers with additional hint parameter, and just use these wrappers..
> >>
> >> ===
> >>
> >> If nobody likes any of my suggestions, then ignore them. I understand that this series fixes
> >> real issue and much effort has already been spent. May be one day I'll try to rewrite it...
> >>
> >
> > For the reason exposed above, I don't think it makes sense to
> > build the hint before calling a function that calls error_setg().
> > I'm afraid we're stuck with local_err... it is then up to the
> > people to make it as less painful as possible.
> >
>
> Hmm. so, seems that in general we need local_err..
>
> Then may be, may can make automated propagation?
>
> It will look like
>
> g_auto(ErrorPropagation) _error_prop = (ErrorPropagation){
> .errp = errp,
> .local_err = NULL,
> }
>
> errp = &_error_prop.local_err;
>
> and this thing may be fully covered into macro,
> to look like this at function start (to be honest it should exactly after all
> local variable definitions):
>
> MAKE_ERRP_SAFE(_error_prop, errp);
>
>
Maybe you can send an RFC patch that converts a handful of
local_err users to g_auto() ?
18.09.2019 10:58, Greg Kurz wrote:
> On Tue, 17 Sep 2019 17:40:11 +0000
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>
>> 17.09.2019 18:37, Greg Kurz wrote:
>>> On Tue, 17 Sep 2019 13:25:03 +0000
>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>>>
>>>> 17.09.2019 13:20, Greg Kurz wrote:
>>>>> Ensure that hints are added even if errp is &error_fatal or &error_abort.
>>>>>
>>>>> Signed-off-by: Greg Kurz <groug@kaod.org>
>>>>> ---
>>>>> block/backup.c | 7 +++++--
>>>>> block/dirty-bitmap.c | 7 +++++--
>>>>> block/file-posix.c | 20 +++++++++++++-------
>>>>> block/gluster.c | 23 +++++++++++++++--------
>>>>> block/qcow.c | 10 ++++++----
>>>>> block/qcow2.c | 7 +++++--
>>>>> block/vhdx-log.c | 7 +++++--
>>>>> block/vpc.c | 7 +++++--
>>>>> 8 files changed, 59 insertions(+), 29 deletions(-)
>>>>>
>>>>> diff --git a/block/backup.c b/block/backup.c
>>>>> index 763f0d7ff6db..d8c422a0e3bc 100644
>>>>> --- a/block/backup.c
>>>>> +++ b/block/backup.c
>>>>> @@ -602,11 +602,14 @@ static int64_t backup_calculate_cluster_size(BlockDriverState *target,
>>>>> BACKUP_CLUSTER_SIZE_DEFAULT);
>>>>> return BACKUP_CLUSTER_SIZE_DEFAULT;
>>>>> } else if (ret < 0 && !target->backing) {
>>>>> - error_setg_errno(errp, -ret,
>>>>> + Error *local_err = NULL;
>>>>> +
>>>>> + error_setg_errno(&local_err, -ret,
>>>>> "Couldn't determine the cluster size of the target image, "
>>>>> "which has no backing file");
>>>>> - error_append_hint(errp,
>>>>> + error_append_hint(&local_err,
>>>>> "Aborting, since this may create an unusable destination image\n");
>>>>> + error_propagate(errp, local_err);
>>>>> return ret;
>>>>> } else if (ret < 0 && target->backing) {
>>>>> /* Not fatal; just trudge on ahead. */
>>>>
>>>>
>>>> Pain.. Do we need these hints, if they are so painful?
>>>>
>>>
>>> I agree that the one above doesn't qualify as a useful hint.
>>> It just tells that QEMU is giving up and gives no indication
>>> to the user on how to avoid the issue. It should probably be
>>> dropped.
>>>
>>>> At least for cases like this, we can create helper function
>>>>
>>>> error_setg_errno_hint(..., error, hint)
>>>
>>> Not very useful if hint has to be forged separately with
>>> g_sprintf(), and we can't have such a thing as:
>>>
>>> error_setg_errno_hint(errp, err_fmt, ..., hint_fmt, ...)
>>>
>>>>
>>>> But what could be done when we call function, which may or may not set errp?
>>>>
>>>> ret = f(errp);
>>>> if (ret) {
>>>> error_append_hint(errp, hint);
>>>> }
>>>>
>>>
>>> Same problem. If errp is &error_fatal and f() does errno_setg(errp), it
>>> ends up calling exit().
>>>
>>>> Hmmm..
>>>>
>>>> Can it look like
>>>>
>>>> ret = f(..., hint_helper(errp, hint))
>>>>
>>>> ?
>>>>
>>>
>>> Nope, hint_helper() will get called before f() and things are worse.
>>> If errp is NULL then error_append_hint() does nothing and if it is
>>> &error_fatal then it aborts.
>>>
>>>> I can't imagine how to do it, as someone should remove hint from error_abort object on
>>>> success path..
>>>>
>>>> But seems, the following is possible, which seems better for me than local-error approach:
>>>>
>>>> error_push_hint(errp, hint);
>>>> ret = f(.., errp);
>>>> error_pop_hint(errp);
>>>>
>>>
>>> Matter of taste... also, it looks awkward to come up with a hint
>>> before knowing what happened. I mean the appropriate hint could
>>> depend on the value returned by f() and/or errno for example.
>>>
>>>> ===
>>>>
>>>> Continue thinking on this:
>>>>
>>>> It may look like just
>>>> ret = f(..., set_hint(errp, hint));
>>>>
>>>> or (just to split long line):
>>>> set_hint(errp, hint);
>>>> ret = f(..., errp);
>>>>
>>>> if in each function with errp does error_push_hint(errp) on start and error_pop_hint(errp) on exit,
>>>> which may be just one call at function start of macro, which will call error_push_hint(errp) and
>>>> define local variable by g_auto, with cleanup which will call error_pop_hint(errp) on function
>>>> exit..
>>>>
>>>> Or, may be, more direct way to set cleanup for function exists?
>>>>
>>>> ===
>>>>
>>>> Also, we can implement some code generation, to generate for functions with errp argument
>>>> wrappers with additional hint parameter, and just use these wrappers..
>>>>
>>>> ===
>>>>
>>>> If nobody likes any of my suggestions, then ignore them. I understand that this series fixes
>>>> real issue and much effort has already been spent. May be one day I'll try to rewrite it...
>>>>
>>>
>>> For the reason exposed above, I don't think it makes sense to
>>> build the hint before calling a function that calls error_setg().
>>> I'm afraid we're stuck with local_err... it is then up to the
>>> people to make it as less painful as possible.
>>>
>>
>> Hmm. so, seems that in general we need local_err..
>>
>> Then may be, may can make automated propagation?
>>
>> It will look like
>>
>> g_auto(ErrorPropagation) _error_prop = (ErrorPropagation){
>> .errp = errp,
>> .local_err = NULL,
>> }
>>
>> errp = &_error_prop.local_err;
>>
>> and this thing may be fully covered into macro,
>> to look like this at function start (to be honest it should exactly after all
>> local variable definitions):
>>
>> MAKE_ERRP_SAFE(_error_prop, errp);
>>
>>
>
> Maybe you can send an RFC patch that converts a handful of
> local_err users to g_auto() ?
>
Yes, I'll try
--
Best regards,
Vladimir
On 9/17/19 5:20 AM, Greg Kurz wrote:
> Ensure that hints are added even if errp is &error_fatal or &error_abort.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
> block/backup.c | 7 +++++--
> block/dirty-bitmap.c | 7 +++++--
> block/file-posix.c | 20 +++++++++++++-------
> block/gluster.c | 23 +++++++++++++++--------
> block/qcow.c | 10 ++++++----
> block/qcow2.c | 7 +++++--
> block/vhdx-log.c | 7 +++++--
> block/vpc.c | 7 +++++--
> 8 files changed, 59 insertions(+), 29 deletions(-)
>
> diff --git a/block/backup.c b/block/backup.c
> index 763f0d7ff6db..d8c422a0e3bc 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -602,11 +602,14 @@ static int64_t backup_calculate_cluster_size(BlockDriverState *target,
> BACKUP_CLUSTER_SIZE_DEFAULT);
> return BACKUP_CLUSTER_SIZE_DEFAULT;
> } else if (ret < 0 && !target->backing) {
> - error_setg_errno(errp, -ret,
> + Error *local_err = NULL;
Can we go with the shorter name 'err' instead of 'local_err'? I know,
we aren't consistent (both styles appear throughout the tree), but the
shorter style is more appealing to me.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Am 17.09.2019 um 16:39 hat Eric Blake geschrieben:
> On 9/17/19 5:20 AM, Greg Kurz wrote:
> > Ensure that hints are added even if errp is &error_fatal or &error_abort.
> >
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> > block/backup.c | 7 +++++--
> > block/dirty-bitmap.c | 7 +++++--
> > block/file-posix.c | 20 +++++++++++++-------
> > block/gluster.c | 23 +++++++++++++++--------
> > block/qcow.c | 10 ++++++----
> > block/qcow2.c | 7 +++++--
> > block/vhdx-log.c | 7 +++++--
> > block/vpc.c | 7 +++++--
> > 8 files changed, 59 insertions(+), 29 deletions(-)
> >
> > diff --git a/block/backup.c b/block/backup.c
> > index 763f0d7ff6db..d8c422a0e3bc 100644
> > --- a/block/backup.c
> > +++ b/block/backup.c
> > @@ -602,11 +602,14 @@ static int64_t backup_calculate_cluster_size(BlockDriverState *target,
> > BACKUP_CLUSTER_SIZE_DEFAULT);
> > return BACKUP_CLUSTER_SIZE_DEFAULT;
> > } else if (ret < 0 && !target->backing) {
> > - error_setg_errno(errp, -ret,
> > + Error *local_err = NULL;
>
> Can we go with the shorter name 'err' instead of 'local_err'? I know,
> we aren't consistent (both styles appear throughout the tree), but the
> shorter style is more appealing to me.
I like local_err better because it's easier to distinguish from errp.
The compiler might catch it if you use the wrong one because one is
Error* and the other is Error**, but as a reviewer, I can still get
confused.
Kevin
On Tue, 17 Sep 2019 16:46:31 +0200
Kevin Wolf <kwolf@redhat.com> wrote:
> Am 17.09.2019 um 16:39 hat Eric Blake geschrieben:
> > On 9/17/19 5:20 AM, Greg Kurz wrote:
> > > Ensure that hints are added even if errp is &error_fatal or &error_abort.
> > >
> > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > ---
> > > block/backup.c | 7 +++++--
> > > block/dirty-bitmap.c | 7 +++++--
> > > block/file-posix.c | 20 +++++++++++++-------
> > > block/gluster.c | 23 +++++++++++++++--------
> > > block/qcow.c | 10 ++++++----
> > > block/qcow2.c | 7 +++++--
> > > block/vhdx-log.c | 7 +++++--
> > > block/vpc.c | 7 +++++--
> > > 8 files changed, 59 insertions(+), 29 deletions(-)
> > >
> > > diff --git a/block/backup.c b/block/backup.c
> > > index 763f0d7ff6db..d8c422a0e3bc 100644
> > > --- a/block/backup.c
> > > +++ b/block/backup.c
> > > @@ -602,11 +602,14 @@ static int64_t backup_calculate_cluster_size(BlockDriverState *target,
> > > BACKUP_CLUSTER_SIZE_DEFAULT);
> > > return BACKUP_CLUSTER_SIZE_DEFAULT;
> > > } else if (ret < 0 && !target->backing) {
> > > - error_setg_errno(errp, -ret,
> > > + Error *local_err = NULL;
> >
> > Can we go with the shorter name 'err' instead of 'local_err'? I know,
> > we aren't consistent (both styles appear throughout the tree), but the
> > shorter style is more appealing to me.
>
> I like local_err better because it's easier to distinguish from errp.
> The compiler might catch it if you use the wrong one because one is
> Error* and the other is Error**, but as a reviewer, I can still get
> confused.
>
I'll favor the official maintainer, hence keeping local_err :)
> Kevin
On 9/17/19 10:46 AM, Kevin Wolf wrote:
> Am 17.09.2019 um 16:39 hat Eric Blake geschrieben:
>> On 9/17/19 5:20 AM, Greg Kurz wrote:
>>> Ensure that hints are added even if errp is &error_fatal or &error_abort.
>>>
>>> Signed-off-by: Greg Kurz <groug@kaod.org>
>>> ---
>>> block/backup.c | 7 +++++--
>>> block/dirty-bitmap.c | 7 +++++--
>>> block/file-posix.c | 20 +++++++++++++-------
>>> block/gluster.c | 23 +++++++++++++++--------
>>> block/qcow.c | 10 ++++++----
>>> block/qcow2.c | 7 +++++--
>>> block/vhdx-log.c | 7 +++++--
>>> block/vpc.c | 7 +++++--
>>> 8 files changed, 59 insertions(+), 29 deletions(-)
>>>
>>> diff --git a/block/backup.c b/block/backup.c
>>> index 763f0d7ff6db..d8c422a0e3bc 100644
>>> --- a/block/backup.c
>>> +++ b/block/backup.c
>>> @@ -602,11 +602,14 @@ static int64_t backup_calculate_cluster_size(BlockDriverState *target,
>>> BACKUP_CLUSTER_SIZE_DEFAULT);
>>> return BACKUP_CLUSTER_SIZE_DEFAULT;
>>> } else if (ret < 0 && !target->backing) {
>>> - error_setg_errno(errp, -ret,
>>> + Error *local_err = NULL;
>>
>> Can we go with the shorter name 'err' instead of 'local_err'? I know,
>> we aren't consistent (both styles appear throughout the tree), but the
>> shorter style is more appealing to me.
>
> I like local_err better because it's easier to distinguish from errp.
> The compiler might catch it if you use the wrong one because one is
> Error* and the other is Error**, but as a reviewer, I can still get
> confused.
>
> Kevin
>
Doesn't that sound like a striking reason for condemnation of this
entire model?
What's going to prevent us from regressing on this technicality in the
future?
Am 17.09.2019 um 21:10 hat John Snow geschrieben:
>
>
> On 9/17/19 10:46 AM, Kevin Wolf wrote:
> > Am 17.09.2019 um 16:39 hat Eric Blake geschrieben:
> >> On 9/17/19 5:20 AM, Greg Kurz wrote:
> >>> Ensure that hints are added even if errp is &error_fatal or &error_abort.
> >>>
> >>> Signed-off-by: Greg Kurz <groug@kaod.org>
> >>> ---
> >>> block/backup.c | 7 +++++--
> >>> block/dirty-bitmap.c | 7 +++++--
> >>> block/file-posix.c | 20 +++++++++++++-------
> >>> block/gluster.c | 23 +++++++++++++++--------
> >>> block/qcow.c | 10 ++++++----
> >>> block/qcow2.c | 7 +++++--
> >>> block/vhdx-log.c | 7 +++++--
> >>> block/vpc.c | 7 +++++--
> >>> 8 files changed, 59 insertions(+), 29 deletions(-)
> >>>
> >>> diff --git a/block/backup.c b/block/backup.c
> >>> index 763f0d7ff6db..d8c422a0e3bc 100644
> >>> --- a/block/backup.c
> >>> +++ b/block/backup.c
> >>> @@ -602,11 +602,14 @@ static int64_t backup_calculate_cluster_size(BlockDriverState *target,
> >>> BACKUP_CLUSTER_SIZE_DEFAULT);
> >>> return BACKUP_CLUSTER_SIZE_DEFAULT;
> >>> } else if (ret < 0 && !target->backing) {
> >>> - error_setg_errno(errp, -ret,
> >>> + Error *local_err = NULL;
> >>
> >> Can we go with the shorter name 'err' instead of 'local_err'? I know,
> >> we aren't consistent (both styles appear throughout the tree), but the
> >> shorter style is more appealing to me.
> >
> > I like local_err better because it's easier to distinguish from errp.
> > The compiler might catch it if you use the wrong one because one is
> > Error* and the other is Error**, but as a reviewer, I can still get
> > confused.
>
> Doesn't that sound like a striking reason for condemnation of this
> entire model?
Might be, but do you have a better idea?
Kevin
© 2016 - 2025 Red Hat, Inc.