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

[Vladimir Sementsov-Ogievskiy]

18.09.2019 10:58, Greg Kurz wrote:
> 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() ?
> 

Yes, I'll try

-- 
Best regards,
Vladimir