Re: [PATCH v2 10/10] nbd: move connection code from block/nbd to nbd/client-connection

2021-06-02 Thread Eric Blake
On Thu, Apr 08, 2021 at 05:08:27PM +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 
> ---
> 
> Hmm. I keep only Virtuozzo's copyright in a new file, as actually I've
> developed nbd-reconnection code. Still probably safer to save all
> copyrights. Let me now if you think so and I'll add them.

I trust git's version log better than what the file header itself
states.  But in this particular case (the new file has only functions
that you demonstrably contributed), I think you are okay in listing
only your own copyright, instead of carrying along everything else
from the file you split out of.  If anyone objects, we can always add
the details back in.  However, it may be good to include a disclaimer
in your commit message proper mentioning your choice in copyright on
the new file.


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




Re: [PATCH v2 10/10] nbd: move connection code from block/nbd to nbd/client-connection

2021-04-08 Thread Vladimir Sementsov-Ogievskiy

08.04.2021 20:04, Roman Kagan wrote:

On Thu, Apr 08, 2021 at 05:08:27PM +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 
---

Hmm. I keep only Virtuozzo's copyright in a new file, as actually I've
developed nbd-reconnection code. Still probably safer to save all
copyrights. Let me now if you think so and I'll add them.


Not my call.


  include/block/nbd.h |  11 +++
  block/nbd.c | 167 --
  nbd/client-connection.c | 192 
  nbd/meson.build |   1 +
  4 files changed, 204 insertions(+), 167 deletions(-)
  create mode 100644 nbd/client-connection.c


Reviewed-by: Roman Kagan 



Thanks a lot for reviewing!

--
Best regards,
Vladimir



Re: [PATCH v2 10/10] nbd: move connection code from block/nbd to nbd/client-connection

2021-04-08 Thread Roman Kagan
On Thu, Apr 08, 2021 at 05:08:27PM +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 
> ---
> 
> Hmm. I keep only Virtuozzo's copyright in a new file, as actually I've
> developed nbd-reconnection code. Still probably safer to save all
> copyrights. Let me now if you think so and I'll add them.

Not my call.

>  include/block/nbd.h |  11 +++
>  block/nbd.c | 167 --
>  nbd/client-connection.c | 192 
>  nbd/meson.build |   1 +
>  4 files changed, 204 insertions(+), 167 deletions(-)
>  create mode 100644 nbd/client-connection.c

Reviewed-by: Roman Kagan 



[PATCH v2 10/10] nbd: move connection code from block/nbd to nbd/client-connection

2021-04-08 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 
---

Hmm. I keep only Virtuozzo's copyright in a new file, as actually I've
developed nbd-reconnection code. Still probably safer to save all
copyrights. Let me now if you think so and I'll add them.

 include/block/nbd.h |  11 +++
 block/nbd.c | 167 --
 nbd/client-connection.c | 192 
 nbd/meson.build |   1 +
 4 files changed, 204 insertions(+), 167 deletions(-)
 create mode 100644 nbd/client-connection.c

diff --git a/include/block/nbd.h b/include/block/nbd.h
index 5f34d23bb0..d4666b105e 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_unref(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 376ab9f92d..1db86b7340 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -66,25 +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;
-
-int refcnt; /* atomic access */
-
-QemuMutex mutex;
-/* All further fields are protected by mutex */
-bool running; /* thread is running now */
-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) */
@@ -119,12 +100,8 @@ typedef struct BDRVNBDState {
 NBDClientConnection *conn;
 } BDRVNBDState;
 
-static void nbd_client_connection_unref(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);
 
@@ -336,150 +313,6 @@ static bool nbd_client_connecting_wait(BDRVNBDState *s)
 return qatomic_load_acquire(>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),
-.refcnt = 1,
-};
-
-qemu_mutex_init(>mutex);
-
-return conn;
-}
-
-static void nbd_client_connection_unref(NBDClientConnection *conn)
-{
-if (qatomic_dec_fetch(>refcnt) == 0) {
-if (conn->sioc) {
-qio_channel_close(QIO_CHANNEL(conn->sioc), NULL);
-}
-error_free(conn->err);
-qapi_free_SocketAddress(conn->saddr);
-g_free(conn);
-}
-}
-
-static void *connect_thread_func(void *opaque)
-{
-NBDClientConnection *conn = opaque;
-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, >err);
-if (ret < 0) {
-object_unref(OBJECT(conn->sioc));
-conn->sioc = NULL;
-}
-
-qemu_mutex_lock(>mutex);
-
-assert(conn->running);
-conn->running = false;
-if (conn->wait_co) {
-aio_co_wake(conn->wait_co);
-conn->wait_co = NULL;
-}
-
-qemu_mutex_unlock(>mutex);
-
-nbd_client_connection_unref(conn);
-
-return NULL;
-}
-
-/*
- * Get a new connection in context of @conn:
- *   if thread is running, wait for completion
- *   if thread is already succeeded in background, and user didn't get the
- * result, just return it now
- *   otherwise if thread is not running, start a thread and wait for completion
- */
-static coroutine_fn QIOChannelSocket *