On Tue, 17 Sep 2019 17:40:11 +0000
Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> wrote:
> 17.09.2019 18:37, Greg Kurz wrote:
> > 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.
> >
>
> 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() ?
