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
next prev parent reply other threads:[~2021-06-17 18:33 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-15 20:47 [PULL 00/34] NBD patches for 2021-06-15 Eric Blake
2021-06-15 20:47 ` [PULL 01/34] async: the main AioContext is only "current" if under the BQL Eric Blake
2021-06-15 20:47 ` [PULL 02/34] tests: cover aio_co_enter from a worker thread without BQL taken Eric Blake
2021-06-15 20:47 ` [PULL 03/34] co-queue: drop extra coroutine_fn marks Eric Blake
2021-06-15 20:47 ` [PULL 04/34] block/nbd: fix channel object leak Eric Blake
2021-06-15 20:47 ` [PULL 05/34] block/nbd: fix how state is cleared on nbd_open() failure paths Eric Blake
2021-06-15 20:47 ` [PULL 06/34] block/nbd: connect_thread_func(): do qio_channel_set_delay(false) Eric Blake
2021-06-15 20:47 ` [PULL 07/34] qemu-sockets: introduce socket_address_parse_named_fd() Eric Blake
2021-06-15 20:47 ` [PULL 08/34] block/nbd: call socket_address_parse_named_fd() in advance Eric Blake
2021-06-15 20:47 ` [PULL 09/34] block/nbd: ensure ->connection_thread is always valid Eric Blake
2021-06-15 20:47 ` [PULL 10/34] block/nbd: nbd_client_handshake(): fix leak of s->ioc Eric Blake
2021-06-15 20:47 ` [PULL 11/34] block/nbd: BDRVNBDState: drop unused connect_err and connect_status Eric Blake
2021-06-15 20:47 ` [PULL 12/34] block/nbd: simplify waking of nbd_co_establish_connection() Eric Blake
2021-06-15 20:47 ` [PULL 13/34] block/nbd: drop thr->state Eric Blake
2021-06-15 20:47 ` [PULL 14/34] block/nbd: bs-independent interface for nbd_co_establish_connection() Eric Blake
2021-06-15 20:47 ` [PULL 15/34] block/nbd: make nbd_co_establish_connection_cancel() bs-independent Eric Blake
2021-06-15 20:47 ` [PULL 16/34] block/nbd: rename NBDConnectThread to NBDClientConnection Eric Blake
2021-06-15 20:47 ` [PULL 17/34] block/nbd: introduce nbd_client_connection_new() Eric Blake
2021-06-15 20:47 ` [PULL 18/34] block/nbd: introduce nbd_client_connection_release() Eric Blake
2021-06-15 20:47 ` [PULL 19/34] nbd: move connection code from block/nbd to nbd/client-connection Eric Blake
2021-06-15 20:47 ` [PULL 20/34] nbd/client-connection: use QEMU_LOCK_GUARD Eric Blake
2021-06-15 20:47 ` [PULL 21/34] nbd/client-connection: add possibility of negotiation Eric Blake
2021-06-15 20:47 ` [PULL 22/34] nbd/client-connection: implement connection retry Eric Blake
2021-06-15 20:47 ` [PULL 23/34] nbd/client-connection: shutdown connection on release Eric Blake
2021-06-15 20:47 ` [PULL 24/34] block/nbd: split nbd_handle_updated_info out of nbd_client_handshake() Eric Blake
2021-06-15 20:47 ` [PULL 25/34] block/nbd: use negotiation of NBDClientConnection Eric Blake
2021-06-15 20:47 ` [PULL 26/34] block/nbd: don't touch s->sioc in nbd_teardown_connection() Eric Blake
2021-06-15 20:47 ` [PULL 27/34] block/nbd: drop BDRVNBDState::sioc Eric Blake
2021-06-15 20:47 ` [PULL 28/34] nbd/client-connection: return only one io channel Eric Blake
2021-06-17 18:32 ` Vladimir Sementsov-Ogievskiy [this message]
2021-06-18 15:55 ` Eric Blake
2021-06-15 20:47 ` [PULL 29/34] block-coroutine-wrapper: allow non bdrv_ prefix Eric Blake
2021-06-15 20:47 ` [PULL 30/34] block/nbd: split nbd_co_do_establish_connection out of nbd_reconnect_attempt Eric Blake
2021-06-15 20:47 ` [PULL 31/34] nbd/client-connection: add option for non-blocking connection attempt Eric Blake
2021-06-15 20:47 ` [PULL 32/34] block/nbd: reuse nbd_co_do_establish_connection() in nbd_open() Eric Blake
2021-06-15 20:47 ` [PULL 33/34] block/nbd: add nbd_client_connected() helper Eric Blake
2021-06-15 20:47 ` [PULL 34/34] block/nbd: safer transition to receiving request Eric Blake
2021-06-17 9:42 ` [PULL 00/34] NBD patches for 2021-06-15 Peter Maydell
2021-06-17 18:35 ` Vladimir Sementsov-Ogievskiy
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4a756fb6-fb78-7420-482d-a529480bb3f2@virtuozzo.com \
--to=vsementsov@virtuozzo.com \
--cc=eblake@redhat.com \
--cc=kwolf@redhat.com \
--cc=mreitz@redhat.com \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).