Re: [PULL 28/34] nbd/client-connection: return only one io channel - Vladimir Sementsov-Ogievskiy

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
To: Eric Blake <eblake@redhat.com>, qemu-devel@nongnu.org
Cc: Kevin Wolf <kwolf@redhat.com>, Max Reitz <mreitz@redhat.com>,
	"open list:Network Block Dev..." <qemu-block@nongnu.org>
Subject: Re: [PULL 28/34] nbd/client-connection: return only one io channel
Date: Thu, 17 Jun 2021 21:32:02 +0300	[thread overview]
Message-ID: <4a756fb6-fb78-7420-482d-a529480bb3f2@virtuozzo.com> (raw)
In-Reply-To: <20210615204756.281505-29-eblake@redhat.com>

15.06.2021 23:47, Eric Blake wrote:
> From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> block/nbd doesn't need underlying sioc channel anymore. So, we can
> update nbd/client-connection interface to return only one top-most io
> channel, which is more straight forward.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Message-Id: <20210610100802.5888-27-vsementsov@virtuozzo.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>   include/block/nbd.h     |  4 ++--
>   block/nbd.c             | 13 ++-----------
>   nbd/client-connection.c | 33 +++++++++++++++++++++++++--------
>   3 files changed, 29 insertions(+), 21 deletions(-)
> 
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index 5bb54d831c8a..10c8a0bcca80 100644
> --- a/include/block/nbd.h
> +++ b/include/block/nbd.h
> @@ -418,9 +418,9 @@ NBDClientConnection *nbd_client_connection_new(const SocketAddress *saddr,
>                                                  QCryptoTLSCreds *tlscreds);
>   void nbd_client_connection_release(NBDClientConnection *conn);
> 
> -QIOChannelSocket *coroutine_fn
> +QIOChannel *coroutine_fn
>   nbd_co_establish_connection(NBDClientConnection *conn, NBDExportInfo *info,
> -                            QIOChannel **ioc, Error **errp);
> +                            Error **errp);
> 
>   void coroutine_fn nbd_co_establish_connection_cancel(NBDClientConnection *conn);
> 
> diff --git a/block/nbd.c b/block/nbd.c
> index 9f193d130bcd..411435c1559e 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -360,7 +360,6 @@ static coroutine_fn void nbd_reconnect_attempt(BDRVNBDState *s)
>   {
>       int ret;
>       AioContext *aio_context = bdrv_get_aio_context(s->bs);
> -    QIOChannelSocket *sioc;
> 
>       if (!nbd_client_connecting(s)) {
>           return;
> @@ -399,20 +398,12 @@ static coroutine_fn void nbd_reconnect_attempt(BDRVNBDState *s)
>           s->ioc = NULL;
>       }
> 
> -    sioc = nbd_co_establish_connection(s->conn, &s->info, &s->ioc, NULL);
> -    if (!sioc) {
> +    s->ioc = nbd_co_establish_connection(s->conn, &s->info, NULL);
> +    if (!s->ioc) {
>           ret = -ECONNREFUSED;
>           goto out;
>       }
> 
> -    if (s->ioc) {
> -        /* sioc is referenced by s->ioc */
> -        object_unref(OBJECT(sioc));
> -    } else {
> -        s->ioc = QIO_CHANNEL(sioc);
> -    }
> -    sioc = NULL;
> -
>       qio_channel_set_blocking(QIO_CHANNEL(s->ioc), false, NULL);
>       qio_channel_attach_aio_context(QIO_CHANNEL(s->ioc), aio_context);
> 
> diff --git a/nbd/client-connection.c b/nbd/client-connection.c
> index 883f9cf158cb..72138a5ff74a 100644
> --- a/nbd/client-connection.c
> +++ b/nbd/client-connection.c
> @@ -272,15 +272,15 @@ void nbd_client_connection_release(NBDClientConnection *conn)
>    * nbd_receive_export_list() would be zero (see description of NBDExportInfo in
>    * include/block/nbd.h).
>    */
> -QIOChannelSocket *coroutine_fn
> +QIOChannel *coroutine_fn
>   nbd_co_establish_connection(NBDClientConnection *conn, NBDExportInfo *info,
> -                            QIOChannel **ioc, Error **errp)
> +                            Error **errp)
>   {
> +    QIOChannel *ioc;
>       QemuThread thread;
> 
>       if (conn->do_negotiation) {
>           assert(info);
> -        assert(ioc);
>       }
> 
>       WITH_QEMU_LOCK_GUARD(&conn->mutex) {
> @@ -294,10 +294,17 @@ nbd_co_establish_connection(NBDClientConnection *conn, NBDExportInfo *info,
>               if (conn->sioc) {
>                   /* Previous attempt finally succeeded in background */
>                   if (conn->do_negotiation) {
> -                    *ioc = g_steal_pointer(&conn->ioc);
> +                    ioc = g_steal_pointer(&conn->ioc);
>                       memcpy(info, &conn->updated_info, sizeof(*info));
>                   }
> -                return g_steal_pointer(&conn->sioc);
> +                if (ioc) {
> +                    /* TLS channel now has own reference to parent */
> +                    object_unref(OBJECT(conn->sioc));
> +                } else {
> +                    ioc = QIO_CHANNEL(conn->sioc);
> +                }
> +                conn->sioc = NULL;
> +                return ioc;
>               }
> 
>               conn->running = true;
> @@ -329,11 +336,21 @@ nbd_co_establish_connection(NBDClientConnection *conn, NBDExportInfo *info,
>           } else {
>               error_propagate(errp, conn->err);
>               conn->err = NULL;
> -            if (conn->sioc && conn->do_negotiation) {
> -                *ioc = g_steal_pointer(&conn->ioc);
> +            if (!conn->sioc) {
> +                return NULL;
> +            }
> +            if (conn->do_negotiation) {
> +                ioc = g_steal_pointer(&conn->ioc);
>                   memcpy(info, &conn->updated_info, sizeof(*info));
>               }
> -            return g_steal_pointer(&conn->sioc);
> +            if (ioc) {
> +                /* TLS channel now has own reference to parent */
> +                object_unref(OBJECT(conn->sioc));
> +            } else {
> +                ioc = QIO_CHANNEL(conn->sioc);
> +            }
> +            conn->sioc = NULL;
> +            return ioc;
>           }
>       }
> 

Logic is wrong and uninitialized use of ioc really possible, as Peter (and clang) reports.

So, I propose the following squash-in. It doesn't conflict with following patches.

squash-in:

diff --git a/nbd/client-connection.c b/nbd/client-connection.c
index 500b8591e8..396d7f17f0 100644
--- a/nbd/client-connection.c
+++ b/nbd/client-connection.c
@@ -275,7 +275,6 @@ QIOChannel *coroutine_fn
  nbd_co_establish_connection(NBDClientConnection *conn, NBDExportInfo *info,
                              Error **errp)
  {
-    QIOChannel *ioc;
      QemuThread thread;
  
      if (conn->do_negotiation) {
@@ -293,17 +292,19 @@ nbd_co_establish_connection(NBDClientConnection *conn, NBDExportInfo *info,
              if (conn->sioc) {
                  /* Previous attempt finally succeeded in background */
                  if (conn->do_negotiation) {
-                    ioc = g_steal_pointer(&conn->ioc);
                      memcpy(info, &conn->updated_info, sizeof(*info));
+                    if (conn->ioc) {
+                        /* TLS channel now has own reference to parent */
+                        object_unref(OBJECT(conn->sioc));
+                        conn->sioc = NULL;
+
+                        return g_steal_pointer(&conn->ioc);
+                    }
                  }
-                if (ioc) {
-                    /* TLS channel now has own reference to parent */
-                    object_unref(OBJECT(conn->sioc));
-                } else {
-                    ioc = QIO_CHANNEL(conn->sioc);
-                }
-                conn->sioc = NULL;
-                return ioc;
+
+                assert(!conn->ioc);
+
+                return QIO_CHANNEL(g_steal_pointer(&conn->sioc));
              }
  
              conn->running = true;
@@ -339,17 +340,19 @@ nbd_co_establish_connection(NBDClientConnection *conn, NBDExportInfo *info,
                  return NULL;
              }
              if (conn->do_negotiation) {
-                ioc = g_steal_pointer(&conn->ioc);
                  memcpy(info, &conn->updated_info, sizeof(*info));
+                if (conn->ioc) {
+                    /* TLS channel now has own reference to parent */
+                    object_unref(OBJECT(conn->sioc));
+                    conn->sioc = NULL;
+
+                    return g_steal_pointer(&conn->ioc);
+                }
              }
-            if (ioc) {
-                /* TLS channel now has own reference to parent */
-                object_unref(OBJECT(conn->sioc));
-            } else {
-                ioc = QIO_CHANNEL(conn->sioc);
-            }
-            conn->sioc = NULL;
-            return ioc;
+
+            assert(!conn->ioc);
+
+            return QIO_CHANNEL(g_steal_pointer(&conn->sioc));
          }
      }
  




-- 
Best regards,
Vladimir

