18.09.2019 10:58, Greg Kurz wrote: > 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() ? >
Yes, I'll try -- Best regards, Vladimir