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

[Vladimir Sementsov-Ogievskiy]

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);


-- 
Best regards,
Vladimir