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 17:40:11 +0000
Vladimir Sementsov-Ogievskiy <address@hidden> wrote:

> 17.09.2019 18:37, Greg Kurz wrote:
> > 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.
> > 
> 
> 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() ?