[PATCH v3 13/33] block/nbd: introduce nbd_client_connection_release()
Signed-off-by: Vladimir Sementsov-Ogievskiy --- block/nbd.c | 43 ++- 1 file changed, 26 insertions(+), 17 deletions(-) diff --git a/block/nbd.c b/block/nbd.c index 21a4039359..8531d019b2 100644 --- a/block/nbd.c +++ b/block/nbd.c @@ -118,7 +118,7 @@ typedef struct BDRVNBDState { NBDClientConnection *conn; } BDRVNBDState; -static void nbd_free_connect_thread(NBDClientConnection *conn); +static void nbd_client_connection_release(NBDClientConnection *conn); static int nbd_establish_connection(BlockDriverState *bs, SocketAddress *saddr, Error **errp); static coroutine_fn QIOChannelSocket * @@ -130,20 +130,9 @@ static void nbd_yank(void *opaque); static void nbd_clear_bdrvstate(BlockDriverState *bs) { BDRVNBDState *s = (BDRVNBDState *)bs->opaque; -NBDClientConnection *conn = s->conn; -bool do_free; - -qemu_mutex_lock(&conn->mutex); -if (conn->running) { -conn->detached = true; -} -do_free = !conn->running && !conn->detached; -qemu_mutex_unlock(&conn->mutex); -/* the runaway thread will clean it up itself */ -if (do_free) { -nbd_free_connect_thread(conn); -} +nbd_client_connection_release(s->conn); +s->conn = NULL; yank_unregister_instance(BLOCKDEV_YANK_INSTANCE(bs->node_name)); @@ -365,7 +354,7 @@ nbd_client_connection_new(const SocketAddress *saddr) return conn; } -static void nbd_free_connect_thread(NBDClientConnection *conn) +static void nbd_client_connection_do_free(NBDClientConnection *conn) { if (conn->sioc) { qio_channel_close(QIO_CHANNEL(conn->sioc), NULL); @@ -379,8 +368,8 @@ static void nbd_free_connect_thread(NBDClientConnection *conn) static void *connect_thread_func(void *opaque) { NBDClientConnection *conn = opaque; +bool do_free; int ret; -bool do_free = false; conn->sioc = qio_channel_socket_new(); @@ -405,12 +394,32 @@ static void *connect_thread_func(void *opaque) qemu_mutex_unlock(&conn->mutex); if (do_free) { -nbd_free_connect_thread(conn); +nbd_client_connection_do_free(conn); } return NULL; } +static void nbd_client_connection_release(NBDClientConnection *conn) +{ +bool do_free; + +if (!conn) { +return; +} + +qemu_mutex_lock(&conn->mutex); +if (conn->running) { +conn->detached = true; +} +do_free = !conn->running && !conn->detached; +qemu_mutex_unlock(&conn->mutex); + +if (do_free) { +nbd_client_connection_do_free(conn); +} +} + /* * Get a new connection in context of @conn: * if thread is running, wait for completion -- 2.29.2
Re: [PATCH v3 13/33] block/nbd: introduce nbd_client_connection_release()
On Fri, Apr 16, 2021 at 11:08:51AM +0300, Vladimir Sementsov-Ogievskiy wrote: > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > block/nbd.c | 43 ++- > 1 file changed, 26 insertions(+), 17 deletions(-) > > diff --git a/block/nbd.c b/block/nbd.c > index 21a4039359..8531d019b2 100644 > --- a/block/nbd.c > +++ b/block/nbd.c > @@ -118,7 +118,7 @@ typedef struct BDRVNBDState { > NBDClientConnection *conn; > } BDRVNBDState; > > -static void nbd_free_connect_thread(NBDClientConnection *conn); > +static void nbd_client_connection_release(NBDClientConnection *conn); > static int nbd_establish_connection(BlockDriverState *bs, SocketAddress > *saddr, > Error **errp); > static coroutine_fn QIOChannelSocket * > @@ -130,20 +130,9 @@ static void nbd_yank(void *opaque); > static void nbd_clear_bdrvstate(BlockDriverState *bs) > { > BDRVNBDState *s = (BDRVNBDState *)bs->opaque; > -NBDClientConnection *conn = s->conn; > -bool do_free; > - > -qemu_mutex_lock(&conn->mutex); > -if (conn->running) { > -conn->detached = true; > -} > -do_free = !conn->running && !conn->detached; > -qemu_mutex_unlock(&conn->mutex); > > -/* the runaway thread will clean it up itself */ > -if (do_free) { > -nbd_free_connect_thread(conn); > -} > +nbd_client_connection_release(s->conn); > +s->conn = NULL; > > yank_unregister_instance(BLOCKDEV_YANK_INSTANCE(bs->node_name)); > > @@ -365,7 +354,7 @@ nbd_client_connection_new(const SocketAddress *saddr) > return conn; > } > > -static void nbd_free_connect_thread(NBDClientConnection *conn) > +static void nbd_client_connection_do_free(NBDClientConnection *conn) > { > if (conn->sioc) { > qio_channel_close(QIO_CHANNEL(conn->sioc), NULL); > @@ -379,8 +368,8 @@ static void nbd_free_connect_thread(NBDClientConnection > *conn) > static void *connect_thread_func(void *opaque) > { > NBDClientConnection *conn = opaque; > +bool do_free; > int ret; > -bool do_free = false; > This hunk belongs in patch 8. Roman. > conn->sioc = qio_channel_socket_new(); > > @@ -405,12 +394,32 @@ static void *connect_thread_func(void *opaque) > qemu_mutex_unlock(&conn->mutex); > > if (do_free) { > -nbd_free_connect_thread(conn); > +nbd_client_connection_do_free(conn); > } > > return NULL; > } > > +static void nbd_client_connection_release(NBDClientConnection *conn) > +{ > +bool do_free; > + > +if (!conn) { > +return; > +} > + > +qemu_mutex_lock(&conn->mutex); > +if (conn->running) { > +conn->detached = true; > +} > +do_free = !conn->running && !conn->detached; > +qemu_mutex_unlock(&conn->mutex); > + > +if (do_free) { > +nbd_client_connection_do_free(conn); > +} > +} > + > /* > * Get a new connection in context of @conn: > * if thread is running, wait for completion > -- > 2.29.2 >
Re: [PATCH v3 13/33] block/nbd: introduce nbd_client_connection_release()
28.04.2021 01:35, Roman Kagan wrote: On Fri, Apr 16, 2021 at 11:08:51AM +0300, Vladimir Sementsov-Ogievskiy wrote: Signed-off-by: Vladimir Sementsov-Ogievskiy --- block/nbd.c | 43 ++- 1 file changed, 26 insertions(+), 17 deletions(-) diff --git a/block/nbd.c b/block/nbd.c index 21a4039359..8531d019b2 100644 --- a/block/nbd.c +++ b/block/nbd.c @@ -118,7 +118,7 @@ typedef struct BDRVNBDState { NBDClientConnection *conn; } BDRVNBDState; -static void nbd_free_connect_thread(NBDClientConnection *conn); +static void nbd_client_connection_release(NBDClientConnection *conn); static int nbd_establish_connection(BlockDriverState *bs, SocketAddress *saddr, Error **errp); static coroutine_fn QIOChannelSocket * @@ -130,20 +130,9 @@ static void nbd_yank(void *opaque); static void nbd_clear_bdrvstate(BlockDriverState *bs) { BDRVNBDState *s = (BDRVNBDState *)bs->opaque; -NBDClientConnection *conn = s->conn; -bool do_free; - -qemu_mutex_lock(&conn->mutex); -if (conn->running) { -conn->detached = true; -} -do_free = !conn->running && !conn->detached; -qemu_mutex_unlock(&conn->mutex); -/* the runaway thread will clean it up itself */ -if (do_free) { -nbd_free_connect_thread(conn); -} +nbd_client_connection_release(s->conn); +s->conn = NULL; yank_unregister_instance(BLOCKDEV_YANK_INSTANCE(bs->node_name)); @@ -365,7 +354,7 @@ nbd_client_connection_new(const SocketAddress *saddr) return conn; } -static void nbd_free_connect_thread(NBDClientConnection *conn) +static void nbd_client_connection_do_free(NBDClientConnection *conn) { if (conn->sioc) { qio_channel_close(QIO_CHANNEL(conn->sioc), NULL); @@ -379,8 +368,8 @@ static void nbd_free_connect_thread(NBDClientConnection *conn) static void *connect_thread_func(void *opaque) { NBDClientConnection *conn = opaque; +bool do_free; int ret; -bool do_free = false; This hunk belongs in patch 8. Agree conn->sioc = qio_channel_socket_new(); @@ -405,12 +394,32 @@ static void *connect_thread_func(void *opaque) qemu_mutex_unlock(&conn->mutex); if (do_free) { -nbd_free_connect_thread(conn); +nbd_client_connection_do_free(conn); } return NULL; } +static void nbd_client_connection_release(NBDClientConnection *conn) +{ +bool do_free; + +if (!conn) { +return; +} + +qemu_mutex_lock(&conn->mutex); +if (conn->running) { +conn->detached = true; +} +do_free = !conn->running && !conn->detached; +qemu_mutex_unlock(&conn->mutex); + +if (do_free) { +nbd_client_connection_do_free(conn); +} +} + /* * Get a new connection in context of @conn: * if thread is running, wait for completion -- 2.29.2 -- Best regards, Vladimir
Re: [PATCH v3 13/33] block/nbd: introduce nbd_client_connection_release()
On Fri, Apr 16, 2021 at 11:08:51AM +0300, Vladimir Sementsov-Ogievskiy wrote: > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > block/nbd.c | 43 ++- > 1 file changed, 26 insertions(+), 17 deletions(-) Commit message said what, but not why. Presumably this is one more bit of refactoring to make the upcoming file split in a later patch easier. But patch 12/33 said it was the last step before a new file, and this patch isn't yet at a new file. Missing some continuity in your commit messages? > > diff --git a/block/nbd.c b/block/nbd.c > index 21a4039359..8531d019b2 100644 > --- a/block/nbd.c > +++ b/block/nbd.c > @@ -118,7 +118,7 @@ typedef struct BDRVNBDState { > NBDClientConnection *conn; > } BDRVNBDState; > > -static void nbd_free_connect_thread(NBDClientConnection *conn); > +static void nbd_client_connection_release(NBDClientConnection *conn); Is it necessary for a forward declaration, or can you just implement the new function prior to its users? At any rate, the refactoring looks sane. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [PATCH v3 13/33] block/nbd: introduce nbd_client_connection_release()
03.06.2021 00:27, Eric Blake wrote: On Fri, Apr 16, 2021 at 11:08:51AM +0300, Vladimir Sementsov-Ogievskiy wrote: Signed-off-by: Vladimir Sementsov-Ogievskiy --- block/nbd.c | 43 ++- 1 file changed, 26 insertions(+), 17 deletions(-) Commit message said what, but not why. Presumably this is one more bit of refactoring to make the upcoming file split in a later patch easier. But patch 12/33 said it was the last step before a new file, and this patch isn't yet at a new file. Missing some continuity in your commit messages? Seems like one more small additional step after last step ) diff --git a/block/nbd.c b/block/nbd.c index 21a4039359..8531d019b2 100644 --- a/block/nbd.c +++ b/block/nbd.c @@ -118,7 +118,7 @@ typedef struct BDRVNBDState { NBDClientConnection *conn; } BDRVNBDState; -static void nbd_free_connect_thread(NBDClientConnection *conn); +static void nbd_client_connection_release(NBDClientConnection *conn); Is it necessary for a forward declaration, or can you just implement the new function prior to its users? Hmm, seems it could be easily moved. At any rate, the refactoring looks sane. -- Best regards, Vladimir
Re: [PATCH v3 13/33] block/nbd: introduce nbd_client_connection_release()
03.06.2021 00:27, Eric Blake wrote: On Fri, Apr 16, 2021 at 11:08:51AM +0300, Vladimir Sementsov-Ogievskiy wrote: Signed-off-by: Vladimir Sementsov-Ogievskiy --- block/nbd.c | 43 ++- 1 file changed, 26 insertions(+), 17 deletions(-) Commit message said what, but not why. Presumably this is one more bit of refactoring to make the upcoming file split in a later patch easier. But patch 12/33 said it was the last step before a new file, and this patch isn't yet at a new file. Missing some continuity in your commit messages? diff --git a/block/nbd.c b/block/nbd.c index 21a4039359..8531d019b2 100644 --- a/block/nbd.c +++ b/block/nbd.c @@ -118,7 +118,7 @@ typedef struct BDRVNBDState { NBDClientConnection *conn; } BDRVNBDState; -static void nbd_free_connect_thread(NBDClientConnection *conn); +static void nbd_client_connection_release(NBDClientConnection *conn); Is it necessary for a forward declaration, or can you just implement the new function prior to its users? Actually, otherwise we'll need a forward declaration for nbd_client_connection_do_free(). Anyway, this all doesn't make real sense before moving to separate file -- Best regards, Vladimir
Re: [PATCH v3 13/33] block/nbd: introduce nbd_client_connection_release()
On Tue, Jun 08, 2021 at 01:00:08PM +0300, Vladimir Sementsov-Ogievskiy wrote: > 03.06.2021 00:27, Eric Blake wrote: > > On Fri, Apr 16, 2021 at 11:08:51AM +0300, Vladimir Sementsov-Ogievskiy > > wrote: > > > Signed-off-by: Vladimir Sementsov-Ogievskiy > > > --- > > > block/nbd.c | 43 ++- > > > 1 file changed, 26 insertions(+), 17 deletions(-) > > > > Commit message said what, but not why. Presumably this is one more > > bit of refactoring to make the upcoming file split in a later patch > > easier. But patch 12/33 said it was the last step before a new file, > > and this patch isn't yet at a new file. Missing some continuity in > > your commit messages? > > > > > > > > diff --git a/block/nbd.c b/block/nbd.c > > > index 21a4039359..8531d019b2 100644 > > > --- a/block/nbd.c > > > +++ b/block/nbd.c > > > @@ -118,7 +118,7 @@ typedef struct BDRVNBDState { > > > NBDClientConnection *conn; > > > } BDRVNBDState; > > > -static void nbd_free_connect_thread(NBDClientConnection *conn); > > > +static void nbd_client_connection_release(NBDClientConnection *conn); > > > > Is it necessary for a forward declaration, or can you just implement > > the new function prior to its users? > > > > Actually, otherwise we'll need a forward declaration for > nbd_client_connection_do_free(). Anyway, this all doesn't make real sense > before moving to separate file Fair enough. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org