15.06.2021 23:47, Eric Blake wrote:
From: Vladimir Sementsov-Ogievskiy <vsement...@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 <vsement...@virtuozzo.com>
Reviewed-by: Eric Blake <ebl...@redhat.com>
Message-Id: <20210610100802.5888-27-vsement...@virtuozzo.com>
Signed-off-by: Eric Blake <ebl...@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
