Re: [PATCH v3 14/33] nbd: move connection code from block/nbd to nbd/client-connection

2021-06-09 Thread Vladimir Sementsov-Ogievskiy 

28.04.2021 11:14, Vladimir Sementsov-Ogievskiy wrote:

+struct NBDClientConnection {
+    /* Initialization constants */
+    SocketAddress *saddr; /* address to connect to */
+
+    /*
+ * Result of last attempt. Valid in FAIL and SUCCESS states.
+ * If you want to steal error, don't forget to set pointer to NULL.
+ */
+    QIOChannelSocket *sioc;
+    Error *err;


These two are also manipulated under the mutex.  Consider also updating
the comment: both these pointers are to be "stolen" by the caller, with
the former being valid when the connection succeeds and the latter
otherwise.



Hmm. I should move mutex and "All further" comment above these two fields.

Ok, I'll think on updating the comment (probably as an additional patch, to 
keep this as a simple movement). I don't like to document that they are stolen 
by caller(). For me it sounds like caller is user of the interface. And caller 
of nbd_co_establish_connection() doesn't stole anything: the structure is 
private now..


Finally, I decided to improve the comment as part of "[PATCH v3 08/33] block/nbd: drop 
thr->state" commit, as "FAIL and SUCCESS states" string becomes outdated when we 
drop these states.

--
Best regards,
Vladimir

Re: [PATCH v3 14/33] nbd: move connection code from block/nbd to nbd/client-connection

2021-06-03 Thread Eric Blake 
On Fri, Apr 16, 2021 at 11:08:52AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> We now have bs-independent connection API, which consists of four
> functions:
> 
>   nbd_client_connection_new()
>   nbd_client_connection_unref()
>   nbd_co_establish_connection()
>   nbd_co_establish_connection_cancel()
> 
> Move them to a separate file together with NBDClientConnection
> structure which becomes private to the new API.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  include/block/nbd.h |  11 +++
>  block/nbd.c | 187 ---
>  nbd/client-connection.c | 212 
>  nbd/meson.build |   1 +

> +++ b/include/block/nbd.h
> @@ -406,4 +406,15 @@ const char *nbd_info_lookup(uint16_t info);
>  const char *nbd_cmd_lookup(uint16_t info);
>  const char *nbd_err_lookup(int err);
>  
> +/* nbd/client-connection.c */
> +typedef struct NBDClientConnection NBDClientConnection;
> +
> +NBDClientConnection *nbd_client_connection_new(const SocketAddress *saddr);
> +void nbd_client_connection_release(NBDClientConnection *conn);
> +
> +QIOChannelSocket *coroutine_fn
> +nbd_co_establish_connection(NBDClientConnection *conn, Error **errp);

To me, the placement of coroutine_fn looks a bit odd here, like it is
the name of a pointer declaration.  But I see that we have precedence
for doing it that way (such as block.c:bdrv_co_enter()); the
difference being that none of the other locations split the return
type on one line and the function name on another.  I don't really
have any changes to suggest, though, so I'm fine keeping it the way
you wrote.

> +++ b/nbd/client-connection.c

There may be fallout in v4 based on what you tweak in the code that
got moved here, but the split to a new file looks sane to me.

Reviewed-by: Eric Blake 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v3 14/33] nbd: move connection code from block/nbd to nbd/client-connection

2021-04-28 Thread Vladimir Sementsov-Ogievskiy 

28.04.2021 01:45, Roman Kagan wrote:

On Fri, Apr 16, 2021 at 11:08:52AM +0300, Vladimir Sementsov-Ogievskiy wrote:

We now have bs-independent connection API, which consists of four
functions:

   nbd_client_connection_new()
   nbd_client_connection_unref()
   nbd_co_establish_connection()
   nbd_co_establish_connection_cancel()

Move them to a separate file together with NBDClientConnection
structure which becomes private to the new API.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  include/block/nbd.h |  11 +++
  block/nbd.c | 187 ---
  nbd/client-connection.c | 212 
  nbd/meson.build |   1 +
  4 files changed, 224 insertions(+), 187 deletions(-)
  create mode 100644 nbd/client-connection.c

diff --git a/include/block/nbd.h b/include/block/nbd.h
index 5f34d23bb0..57381be76f 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -406,4 +406,15 @@ const char *nbd_info_lookup(uint16_t info);
  const char *nbd_cmd_lookup(uint16_t info);
  const char *nbd_err_lookup(int err);
  
+/* nbd/client-connection.c */

+typedef struct NBDClientConnection NBDClientConnection;
+
+NBDClientConnection *nbd_client_connection_new(const SocketAddress *saddr);
+void nbd_client_connection_release(NBDClientConnection *conn);
+
+QIOChannelSocket *coroutine_fn
+nbd_co_establish_connection(NBDClientConnection *conn, Error **errp);
+
+void coroutine_fn nbd_co_establish_connection_cancel(NBDClientConnection 
*conn);
+
  #endif
diff --git a/block/nbd.c b/block/nbd.c
index 8531d019b2..9bd68dcf10 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -66,24 +66,6 @@ typedef enum NBDClientState {
  NBD_CLIENT_QUIT
  } NBDClientState;
  
-typedef struct NBDClientConnection {

-/* Initialization constants */
-SocketAddress *saddr; /* address to connect to */
-
-/*
- * Result of last attempt. Valid in FAIL and SUCCESS states.
- * If you want to steal error, don't forget to set pointer to NULL.
- */
-QIOChannelSocket *sioc;
-Error *err;
-
-QemuMutex mutex;
-/* All further fields are protected by mutex */
-bool running; /* thread is running now */
-bool detached; /* thread is detached and should cleanup the state */
-Coroutine *wait_co; /* nbd_co_establish_connection() wait in yield() */
-} NBDClientConnection;
-
  typedef struct BDRVNBDState {
  QIOChannelSocket *sioc; /* The master data channel */
  QIOChannel *ioc; /* The current I/O channel which may differ (eg TLS) */
@@ -118,12 +100,8 @@ typedef struct BDRVNBDState {
  NBDClientConnection *conn;
  } BDRVNBDState;
  
-static void nbd_client_connection_release(NBDClientConnection *conn);

  static int nbd_establish_connection(BlockDriverState *bs, SocketAddress 
*saddr,
  Error **errp);
-static coroutine_fn QIOChannelSocket *
-nbd_co_establish_connection(NBDClientConnection *conn, Error **errp);
-static void nbd_co_establish_connection_cancel(NBDClientConnection *conn);
  static int nbd_client_handshake(BlockDriverState *bs, Error **errp);
  static void nbd_yank(void *opaque);
  
@@ -340,171 +318,6 @@ static bool nbd_client_connecting_wait(BDRVNBDState *s)

  return qatomic_load_acquire(&s->state) == NBD_CLIENT_CONNECTING_WAIT;
  }
  
-static NBDClientConnection *

-nbd_client_connection_new(const SocketAddress *saddr)
-{
-NBDClientConnection *conn = g_new(NBDClientConnection, 1);
-
-*conn = (NBDClientConnection) {
-.saddr = QAPI_CLONE(SocketAddress, saddr),
-};
-
-qemu_mutex_init(&conn->mutex);
-
-return conn;
-}
-
-static void nbd_client_connection_do_free(NBDClientConnection *conn)
-{
-if (conn->sioc) {
-qio_channel_close(QIO_CHANNEL(conn->sioc), NULL);
-object_unref(OBJECT(conn->sioc));
-}
-error_free(conn->err);
-qapi_free_SocketAddress(conn->saddr);
-g_free(conn);
-}
-
-static void *connect_thread_func(void *opaque)
-{
-NBDClientConnection *conn = opaque;
-bool do_free;
-int ret;
-
-conn->sioc = qio_channel_socket_new();
-
-error_free(conn->err);
-conn->err = NULL;
-ret = qio_channel_socket_connect_sync(conn->sioc, conn->saddr, &conn->err);
-if (ret < 0) {
-object_unref(OBJECT(conn->sioc));
-conn->sioc = NULL;
-}
-
-qemu_mutex_lock(&conn->mutex);
-
-assert(conn->running);
-conn->running = false;
-if (conn->wait_co) {
-aio_co_schedule(NULL, conn->wait_co);
-conn->wait_co = NULL;
-}
-do_free = conn->detached;
-
-qemu_mutex_unlock(&conn->mutex);
-
-if (do_free) {
-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->deta

Re: [PATCH v3 14/33] nbd: move connection code from block/nbd to nbd/client-connection

2021-04-27 Thread Roman Kagan 
On Fri, Apr 16, 2021 at 11:08:52AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> We now have bs-independent connection API, which consists of four
> functions:
> 
>   nbd_client_connection_new()
>   nbd_client_connection_unref()
>   nbd_co_establish_connection()
>   nbd_co_establish_connection_cancel()
> 
> Move them to a separate file together with NBDClientConnection
> structure which becomes private to the new API.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  include/block/nbd.h |  11 +++
>  block/nbd.c | 187 ---
>  nbd/client-connection.c | 212 
>  nbd/meson.build |   1 +
>  4 files changed, 224 insertions(+), 187 deletions(-)
>  create mode 100644 nbd/client-connection.c
> 
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index 5f34d23bb0..57381be76f 100644
> --- a/include/block/nbd.h
> +++ b/include/block/nbd.h
> @@ -406,4 +406,15 @@ const char *nbd_info_lookup(uint16_t info);
>  const char *nbd_cmd_lookup(uint16_t info);
>  const char *nbd_err_lookup(int err);
>  
> +/* nbd/client-connection.c */
> +typedef struct NBDClientConnection NBDClientConnection;
> +
> +NBDClientConnection *nbd_client_connection_new(const SocketAddress *saddr);
> +void nbd_client_connection_release(NBDClientConnection *conn);
> +
> +QIOChannelSocket *coroutine_fn
> +nbd_co_establish_connection(NBDClientConnection *conn, Error **errp);
> +
> +void coroutine_fn nbd_co_establish_connection_cancel(NBDClientConnection 
> *conn);
> +
>  #endif
> diff --git a/block/nbd.c b/block/nbd.c
> index 8531d019b2..9bd68dcf10 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -66,24 +66,6 @@ typedef enum NBDClientState {
>  NBD_CLIENT_QUIT
>  } NBDClientState;
>  
> -typedef struct NBDClientConnection {
> -/* Initialization constants */
> -SocketAddress *saddr; /* address to connect to */
> -
> -/*
> - * Result of last attempt. Valid in FAIL and SUCCESS states.
> - * If you want to steal error, don't forget to set pointer to NULL.
> - */
> -QIOChannelSocket *sioc;
> -Error *err;
> -
> -QemuMutex mutex;
> -/* All further fields are protected by mutex */
> -bool running; /* thread is running now */
> -bool detached; /* thread is detached and should cleanup the state */
> -Coroutine *wait_co; /* nbd_co_establish_connection() wait in yield() */
> -} NBDClientConnection;
> -
>  typedef struct BDRVNBDState {
>  QIOChannelSocket *sioc; /* The master data channel */
>  QIOChannel *ioc; /* The current I/O channel which may differ (eg TLS) */
> @@ -118,12 +100,8 @@ typedef struct BDRVNBDState {
>  NBDClientConnection *conn;
>  } BDRVNBDState;
>  
> -static void nbd_client_connection_release(NBDClientConnection *conn);
>  static int nbd_establish_connection(BlockDriverState *bs, SocketAddress 
> *saddr,
>  Error **errp);
> -static coroutine_fn QIOChannelSocket *
> -nbd_co_establish_connection(NBDClientConnection *conn, Error **errp);
> -static void nbd_co_establish_connection_cancel(NBDClientConnection *conn);
>  static int nbd_client_handshake(BlockDriverState *bs, Error **errp);
>  static void nbd_yank(void *opaque);
>  
> @@ -340,171 +318,6 @@ static bool nbd_client_connecting_wait(BDRVNBDState *s)
>  return qatomic_load_acquire(&s->state) == NBD_CLIENT_CONNECTING_WAIT;
>  }
>  
> -static NBDClientConnection *
> -nbd_client_connection_new(const SocketAddress *saddr)
> -{
> -NBDClientConnection *conn = g_new(NBDClientConnection, 1);
> -
> -*conn = (NBDClientConnection) {
> -.saddr = QAPI_CLONE(SocketAddress, saddr),
> -};
> -
> -qemu_mutex_init(&conn->mutex);
> -
> -return conn;
> -}
> -
> -static void nbd_client_connection_do_free(NBDClientConnection *conn)
> -{
> -if (conn->sioc) {
> -qio_channel_close(QIO_CHANNEL(conn->sioc), NULL);
> -object_unref(OBJECT(conn->sioc));
> -}
> -error_free(conn->err);
> -qapi_free_SocketAddress(conn->saddr);
> -g_free(conn);
> -}
> -
> -static void *connect_thread_func(void *opaque)
> -{
> -NBDClientConnection *conn = opaque;
> -bool do_free;
> -int ret;
> -
> -conn->sioc = qio_channel_socket_new();
> -
> -error_free(conn->err);
> -conn->err = NULL;
> -ret = qio_channel_socket_connect_sync(conn->sioc, conn->saddr, 
> &conn->err);
> -if (ret < 0) {
> -object_unref(OBJECT(conn->sioc));
> -conn->sioc = NULL;
> -}
> -
> -qemu_mutex_lock(&conn->mutex);
> -
> -assert(conn->running);
> -conn->running = false;
> -if (conn->wait_co) {
> -aio_co_schedule(NULL, conn->wait_co);
> -conn->wait_co = NULL;
> -}
> -do_free = conn->detached;
> -
> -qemu_mutex_unlock(&conn->mutex);
> -
> -if (do_free) {
> -nbd_client_connection_do_free(conn);
> -}
> -
> -return NULL;
> -}
> -
> -static void nbd_client_connection_release(NBDClientConnec

[PATCH v3 14/33] nbd: move connection code from block/nbd to nbd/client-connection

2021-04-16 Thread Vladimir Sementsov-Ogievskiy 
We now have bs-independent connection API, which consists of four
functions:

  nbd_client_connection_new()
  nbd_client_connection_unref()
  nbd_co_establish_connection()
  nbd_co_establish_connection_cancel()

Move them to a separate file together with NBDClientConnection
structure which becomes private to the new API.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 include/block/nbd.h |  11 +++
 block/nbd.c | 187 ---
 nbd/client-connection.c | 212 
 nbd/meson.build |   1 +
 4 files changed, 224 insertions(+), 187 deletions(-)
 create mode 100644 nbd/client-connection.c

diff --git a/include/block/nbd.h b/include/block/nbd.h
index 5f34d23bb0..57381be76f 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -406,4 +406,15 @@ const char *nbd_info_lookup(uint16_t info);
 const char *nbd_cmd_lookup(uint16_t info);
 const char *nbd_err_lookup(int err);
 
+/* nbd/client-connection.c */
+typedef struct NBDClientConnection NBDClientConnection;
+
+NBDClientConnection *nbd_client_connection_new(const SocketAddress *saddr);
+void nbd_client_connection_release(NBDClientConnection *conn);
+
+QIOChannelSocket *coroutine_fn
+nbd_co_establish_connection(NBDClientConnection *conn, Error **errp);
+
+void coroutine_fn nbd_co_establish_connection_cancel(NBDClientConnection 
*conn);
+
 #endif
diff --git a/block/nbd.c b/block/nbd.c
index 8531d019b2..9bd68dcf10 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -66,24 +66,6 @@ typedef enum NBDClientState {
 NBD_CLIENT_QUIT
 } NBDClientState;
 
-typedef struct NBDClientConnection {
-/* Initialization constants */
-SocketAddress *saddr; /* address to connect to */
-
-/*
- * Result of last attempt. Valid in FAIL and SUCCESS states.
- * If you want to steal error, don't forget to set pointer to NULL.
- */
-QIOChannelSocket *sioc;
-Error *err;
-
-QemuMutex mutex;
-/* All further fields are protected by mutex */
-bool running; /* thread is running now */
-bool detached; /* thread is detached and should cleanup the state */
-Coroutine *wait_co; /* nbd_co_establish_connection() wait in yield() */
-} NBDClientConnection;
-
 typedef struct BDRVNBDState {
 QIOChannelSocket *sioc; /* The master data channel */
 QIOChannel *ioc; /* The current I/O channel which may differ (eg TLS) */
@@ -118,12 +100,8 @@ typedef struct BDRVNBDState {
 NBDClientConnection *conn;
 } BDRVNBDState;
 
-static void nbd_client_connection_release(NBDClientConnection *conn);
 static int nbd_establish_connection(BlockDriverState *bs, SocketAddress *saddr,
 Error **errp);
-static coroutine_fn QIOChannelSocket *
-nbd_co_establish_connection(NBDClientConnection *conn, Error **errp);
-static void nbd_co_establish_connection_cancel(NBDClientConnection *conn);
 static int nbd_client_handshake(BlockDriverState *bs, Error **errp);
 static void nbd_yank(void *opaque);
 
@@ -340,171 +318,6 @@ static bool nbd_client_connecting_wait(BDRVNBDState *s)
 return qatomic_load_acquire(&s->state) == NBD_CLIENT_CONNECTING_WAIT;
 }
 
-static NBDClientConnection *
-nbd_client_connection_new(const SocketAddress *saddr)
-{
-NBDClientConnection *conn = g_new(NBDClientConnection, 1);
-
-*conn = (NBDClientConnection) {
-.saddr = QAPI_CLONE(SocketAddress, saddr),
-};
-
-qemu_mutex_init(&conn->mutex);
-
-return conn;
-}
-
-static void nbd_client_connection_do_free(NBDClientConnection *conn)
-{
-if (conn->sioc) {
-qio_channel_close(QIO_CHANNEL(conn->sioc), NULL);
-object_unref(OBJECT(conn->sioc));
-}
-error_free(conn->err);
-qapi_free_SocketAddress(conn->saddr);
-g_free(conn);
-}
-
-static void *connect_thread_func(void *opaque)
-{
-NBDClientConnection *conn = opaque;
-bool do_free;
-int ret;
-
-conn->sioc = qio_channel_socket_new();
-
-error_free(conn->err);
-conn->err = NULL;
-ret = qio_channel_socket_connect_sync(conn->sioc, conn->saddr, &conn->err);
-if (ret < 0) {
-object_unref(OBJECT(conn->sioc));
-conn->sioc = NULL;
-}
-
-qemu_mutex_lock(&conn->mutex);
-
-assert(conn->running);
-conn->running = false;
-if (conn->wait_co) {
-aio_co_schedule(NULL, conn->wait_co);
-conn->wait_co = NULL;
-}
-do_free = conn->detached;
-
-qemu_mutex_unlock(&conn->mutex);
-
-if (do_free) {
-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