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

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
To: Greg Kurz <groug@kaod.org>,
	"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>
Cc: "Fam Zheng" <fam@euphon.net>,
	"Peter Maydell" <peter.maydell@linaro.org>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	"Jeff Cody" <codyprime@gmail.com>,
	"Mark Cave-Ayland" <mark.cave-ayland@ilande.co.uk>,
	"Michael Roth" <mdroth@linux.vnet.ibm.com>,
	"Gerd Hoffmann" <kraxel@redhat.com>,
	"Max Reitz" <mreitz@redhat.com>,
	"qemu-block@nongnu.org" <qemu-block@nongnu.org>,
	"Juan Quintela" <quintela@redhat.com>,
	"David Hildenbrand" <david@redhat.com>,
	"Markus Armbruster" <armbru@redhat.com>,
	"Halil Pasic" <pasic@linux.ibm.com>,
	"Christian Borntraeger" <borntraeger@de.ibm.com>,
	"Marc-André Lureau" <marcandre.lureau@redhat.com>,
	"David Gibson" <david@gibson.dropbear.id.au>,
	"\"qemu-s390x@nongnu.org\\\"\"@d06av22.portsmouth.uk.ibm.com"
	<"qemu-s390x@nongnu.org\""@d06av22.portsmouth.uk.ibm.com>,
	"Eric Farman" <farman@linux.ibm.com>,
	"Dr. David Alan Gilbert" <dgilbert@redhat.com>,
	"Yuval Shaia" <yuval.shaia@oracle.com>,
	"Alex Williamson" <alex.williamson@redhat.com>,
	"qemu-arm@nongnu.org" <qemu-arm@nongnu.org>,
	"John Snow" <jsnow@redhat.com>,
	"Richard Henderson" <rth@twiddle.net>,
	"Kevin Wolf" <kwolf@redhat.com>,
	"berrange@redhat.com" <berrange@redhat.com>,
	"Cornelia Huck" <cohuck@redhat.com>,
	"Subbaraya Sundeep" <sundeep.lkml@gmail.com>,
	"qemu-ppc@nongnu.org" <qemu-ppc@nongnu.org>,
	"Paolo Bonzini" <pbonzini@redhat.com>
Subject: Re: [Qemu-devel] [PATCH 02/17] block: Pass local error object pointer to error_append_hint()
Date: Tue, 17 Sep 2019 13:25:03 +0000	[thread overview]
Message-ID: <5dba090e-8a59-6f42-a93a-eb676422211e@virtuozzo.com> (raw)
In-Reply-To: <156871564329.196432.5930574495661947805.stgit@bahia.lan>

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 <groug@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?

At least for cases like this, we can create helper function

error_setg_errno_hint(..., error, hint)

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

Hmmm..

Can it look like

ret = f(..., hint_helper(errp, hint))

?

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

===

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...

-- 
Best regards,
Vladimir