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);
--
Best regards,
Vladimir
