diff mbox series

[PULL,28/34] nbd/client-connection: return only one io channel

Message ID 20210615204756.281505-29-eblake@redhat.com (mailing list archive)
State New, archived
Headers show
Series [PULL,01/34] async: the main AioContext is only "current" if under the BQL | expand

Commit Message

Eric Blake June 15, 2021, 8:47 p.m. UTC
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(-)

Comments

Vladimir Sementsov-Ogievskiy June 17, 2021, 6:32 p.m. UTC | #1
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));
          }
      }
Eric Blake June 18, 2021, 3:55 p.m. UTC | #2
On Thu, Jun 17, 2021 at 09:32:02PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> 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));

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

I'll squash this in and send v2 of the pull request
diff mbox series

Patch

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