Re: [Qemu-devel] [PATCH 13/17] nbd: Pass local error object pointer to error_append_hint()

From: Greg Kurz <groug@kaod.org>
To: Eric Blake <eblake@redhat.com>
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>,
	qemu-devel@nongnu.org, "Michael Roth" <mdroth@linux.vnet.ibm.com>,
	"Gerd Hoffmann" <kraxel@redhat.com>,
	"Subbaraya Sundeep" <sundeep.lkml@gmail.com>,
	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>,
	"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, "John Snow" <jsnow@redhat.com>,
	"Richard Henderson" <rth@twiddle.net>,
	"Kevin Wolf" <kwolf@redhat.com>,
	"Daniel P. Berrangé" <berrange@redhat.com>,
	"Cornelia Huck" <cohuck@redhat.com>,
	qemu-s390x@nongnu.org, "Max Reitz" <mreitz@redhat.com>,
	qemu-ppc@nongnu.org, "Paolo Bonzini" <pbonzini@redhat.com>
Subject: Re: [Qemu-devel] [PATCH 13/17] nbd: Pass local error object pointer to error_append_hint()
Date: Tue, 17 Sep 2019 18:26:56 +0200	[thread overview]
Message-ID: <20190917182656.1db277b1@bahia.lan> (raw)
In-Reply-To: <de1d7856-3c59-0e94-2ba8-d0438e9d0ff6@redhat.com>

On Tue, 17 Sep 2019 10:15:07 -0500
Eric Blake <eblake@redhat.com> wrote:

> On 9/17/19 5:21 AM, 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>
> > ---
> >  nbd/client.c |   24 +++++++++++++-----------
> >  nbd/server.c |    7 +++++--
> >  2 files changed, 18 insertions(+), 13 deletions(-)
> > 
> > diff --git a/nbd/client.c b/nbd/client.c
> > index b9dc829175f9..c6e6e4046fd5 100644
> > --- a/nbd/client.c
> > +++ b/nbd/client.c
> > @@ -154,6 +154,7 @@ static int nbd_handle_reply_err(QIOChannel *ioc, NBDOptionReply *reply,
> >                                  bool strict, Error **errp)
> >  {
> >      g_autofree char *msg = NULL;
> > +    Error *local_err = NULL;
> 
> I'd prefer 'err' here...
> 
> >  
> >      if (!(reply->type & (1 << 31))) {
> >          return 1;
> > @@ -161,14 +162,14 @@ static int nbd_handle_reply_err(QIOChannel *ioc, NBDOptionReply *reply,
> >  
> >      if (reply->length) {
> >          if (reply->length > NBD_MAX_BUFFER_SIZE) {
> > -            error_setg(errp, "server error %" PRIu32
> > +            error_setg(&local_err, "server error %" PRIu32
> 
> so that &err doesn't change line lengths.
> 
> >      case NBD_REP_ERR_SHUTDOWN:
> > -        error_setg(errp, "Server shutting down before option %" PRIu32 " (%s)",
> > +        error_setg(&local_err, "Server shutting down before option %" PRIu32 " (%s)",
> 
> For example, here, you went beyond 80 columns.
> 
> At any rate, I'm assuming this will probably go in through Markus' error
> tree as part of the whole series, rather than me needing to pick just
> this patch through my NBD tree.
> 
> Whether or not you shorten the local variable name,
> 

Regardless of which tree this goes through, it will be code for
which you're the official maintainer in the end. I'll gladly fix
it to meet your needs :)

> Reviewed-by: Eric Blake <eblake@redhat.com>
> 