[02/14] block/nbd: nbd_co_establish_connection(): drop unused errp

Commit Message

Vladimir Sementsov-Ogievskiy April 7, 2021, 10:46 a.m. UTC

We are going to refactor connection logic to make it more
understandable. Every bit that we can simplify in advance will help.
Drop errp for now, it's unused anyway. We'll probably reimplement it in
future.

Signed-off-by: Vladimir Sementsov-Ogievskiy <[e-mail]>
---
 block/nbd.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

Comments

Roman Kagan April 7, 2021, 11:28 a.m. UTC | #1

On Wed, Apr 07, 2021 at 01:46:25PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> We are going to refactor connection logic to make it more
> understandable. Every bit that we can simplify in advance will help.
> Drop errp for now, it's unused anyway. We'll probably reimplement it in
> future.

Although I agree that this passing errors around is a bit of an
overkill, my problem with NBD client is that it's notoriously silent
about problems it expeirences, and those errors never pop up in logs.

Given that these errors are not guest-triggerable, and probably indicate
serious problems at the infrastructure level, instead of endlessly
passing them around (as in the code ATM) or dropping them on the floor
(as you propose in the patch) I'd much rather log them immediately when
encountering.

I have a patch to that end, I'll try to port it on top of your series.

Thanks,
Roman.

Patch

diff --git a/block/nbd.c b/block/nbd.c
index a47d6cfea3..29c33338bf 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -145,7 +145,7 @@  typedef struct BDRVNBDState {
 
 static int nbd_establish_connection(BlockDriverState *bs, SocketAddress *saddr,
                                     Error **errp);
-static int nbd_co_establish_connection(BlockDriverState *bs, Error **errp);
+static int nbd_co_establish_connection(BlockDriverState *bs);
 static void nbd_co_establish_connection_cancel(BlockDriverState *bs,
                                                bool detach);
 static int nbd_client_handshake(BlockDriverState *bs, Error **errp);
@@ -435,7 +435,7 @@  static void *connect_thread_func(void *opaque)
 }
 
 static int coroutine_fn
-nbd_co_establish_connection(BlockDriverState *bs, Error **errp)
+nbd_co_establish_connection(BlockDriverState *bs)
 {
     int ret;
     QemuThread thread;
@@ -491,7 +491,7 @@  nbd_co_establish_connection(BlockDriverState *bs, Error **errp)
     case CONNECT_THREAD_SUCCESS:
     case CONNECT_THREAD_FAIL:
         thr->state = CONNECT_THREAD_NONE;
-        error_propagate(errp, thr->err);
+        error_free(thr->err);
         thr->err = NULL;
         s->sioc = thr->sioc;
         thr->sioc = NULL;
@@ -509,7 +509,6 @@  nbd_co_establish_connection(BlockDriverState *bs, Error **errp)
          * result may be used for next connection attempt.
          */
         ret = -1;
-        error_setg(errp, "Connection attempt cancelled by other operation");
         break;
 
     case CONNECT_THREAD_NONE:
@@ -617,7 +616,7 @@  static coroutine_fn void nbd_reconnect_attempt(BDRVNBDState *s)
         s->ioc = NULL;
     }
 
-    if (nbd_co_establish_connection(s->bs, NULL) < 0) {
+    if (nbd_co_establish_connection(s->bs) < 0) {
         ret = -ECONNREFUSED;
         goto out;
     }