diff mbox series

[13/17] nbd: Pass local error object pointer to error_append_hint()

Message ID 156871571297.196432.545961868971946073.stgit@bahia.lan
State New
Headers show
Series Fix usage of error_append_hint() | expand

Commit Message

Greg Kurz Sept. 17, 2019, 10:21 a.m. UTC 
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(-)

Comments

Eric Blake Sept. 17, 2019, 3:15 p.m. UTC | #1 
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,

Reviewed-by: Eric Blake <eblake@redhat.com>
Greg Kurz Sept. 17, 2019, 4:26 p.m. UTC | #2 
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>
>
diff mbox series

Patch

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;
 
     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
                        " (%s) message is too long",
                        reply->type, nbd_rep_lookup(reply->type));
             goto err;
         }
         msg = g_malloc(reply->length + 1);
         if (nbd_read(ioc, msg, reply->length, NULL, errp) < 0) {
-            error_prepend(errp, "Failed to read option error %" PRIu32
+            error_prepend(&local_err, "Failed to read option error %" PRIu32
                           " (%s) message: ",
                           reply->type, nbd_rep_lookup(reply->type));
             goto err;
@@ -187,50 +188,51 @@  static int nbd_handle_reply_err(QIOChannel *ioc, NBDOptionReply *reply,
 
     switch (reply->type) {
     case NBD_REP_ERR_POLICY:
-        error_setg(errp, "Denied by server for option %" PRIu32 " (%s)",
+        error_setg(&local_err, "Denied by server for option %" PRIu32 " (%s)",
                    reply->option, nbd_opt_lookup(reply->option));
         break;
 
     case NBD_REP_ERR_INVALID:
-        error_setg(errp, "Invalid parameters for option %" PRIu32 " (%s)",
+        error_setg(&local_err, "Invalid parameters for option %" PRIu32 " (%s)",
                    reply->option, nbd_opt_lookup(reply->option));
         break;
 
     case NBD_REP_ERR_PLATFORM:
-        error_setg(errp, "Server lacks support for option %" PRIu32 " (%s)",
+        error_setg(&local_err, "Server lacks support for option %" PRIu32 " (%s)",
                    reply->option, nbd_opt_lookup(reply->option));
         break;
 
     case NBD_REP_ERR_TLS_REQD:
-        error_setg(errp, "TLS negotiation required before option %" PRIu32
+        error_setg(&local_err, "TLS negotiation required before option %" PRIu32
                    " (%s)", reply->option, nbd_opt_lookup(reply->option));
         break;
 
     case NBD_REP_ERR_UNKNOWN:
-        error_setg(errp, "Requested export not available");
+        error_setg(&local_err, "Requested export not available");
         break;
 
     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)",
                    reply->option, nbd_opt_lookup(reply->option));
         break;
 
     case NBD_REP_ERR_BLOCK_SIZE_REQD:
-        error_setg(errp, "Server requires INFO_BLOCK_SIZE for option %" PRIu32
+        error_setg(&local_err, "Server requires INFO_BLOCK_SIZE for option %" PRIu32
                    " (%s)", reply->option, nbd_opt_lookup(reply->option));
         break;
 
     default:
-        error_setg(errp, "Unknown error code when asking for option %" PRIu32
+        error_setg(&local_err, "Unknown error code when asking for option %" PRIu32
                    " (%s)", reply->option, nbd_opt_lookup(reply->option));
         break;
     }
 
     if (msg) {
-        error_append_hint(errp, "server reported: %s\n", msg);
+        error_append_hint(&local_err, "server reported: %s\n", msg);
     }
 
  err:
+    error_propagate(errp, local_err);
     nbd_send_opt_abort(ioc);
     return -1;
 }
diff --git a/nbd/server.c b/nbd/server.c
index 28c3c8be854c..6d9ca2563cce 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1616,6 +1616,8 @@  void nbd_export_close(NBDExport *exp)
 
 void nbd_export_remove(NBDExport *exp, NbdServerRemoveMode mode, Error **errp)
 {
+    Error *local_err = NULL;
+
     if (mode == NBD_SERVER_REMOVE_MODE_HARD || QTAILQ_EMPTY(&exp->clients)) {
         nbd_export_close(exp);
         return;
@@ -1623,8 +1625,9 @@  void nbd_export_remove(NBDExport *exp, NbdServerRemoveMode mode, Error **errp)
 
     assert(mode == NBD_SERVER_REMOVE_MODE_SAFE);
 
-    error_setg(errp, "export '%s' still in use", exp->name);
-    error_append_hint(errp, "Use mode='hard' to force client disconnect\n");
+    error_setg(&local_err, "export '%s' still in use", exp->name);
+    error_append_hint(&local_err, "Use mode='hard' to force client disconnect\n");
+    error_propagate(errp, local_err);
 }
 
 void nbd_export_get(NBDExport *exp)