Re: [qemu-s390x] [Qemu-devel] [PATCH 02/17] block: Pass local error object pointer to error_append_hint()

[Greg Kurz]

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