On Tue, 17 Sep 2019 13:25:03 +0000
Vladimir Sementsov-Ogievskiy <vsement...@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 <gr...@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.
