Re: [Qemu-block] [PATCH v7 5/9] block/nbd: refactor nbd connection parameters

[Eric Blake]

On 6/18/19 6:43 AM, Vladimir Sementsov-Ogievskiy wrote:
> We'll need some connection parameters to be available all the time to
> implement nbd reconnect. So, let's refactor them: define additional
> parameters in BDRVNBDState, drop them from function parameters, drop
> nbd_client_init and separate options parsing instead from nbd_open.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/nbd.c | 125 +++-
>  1 file changed, 64 insertions(+), 61 deletions(-)
> 

> @@ -1659,20 +1630,19 @@ static int nbd_open(BlockDriverState *bs, QDict 
> *options, int flags,
>  error_setg(errp, "TLS only supported over IP sockets");
>  goto error;
>  }
> -hostname = s->saddr->u.inet.host;
> +s->hostname = s->saddr->u.inet.host;
>  }
>  
> -/* NBD handshake */
> -ret = nbd_client_init(bs, s->saddr, s->export, tlscreds, hostname,
> -  qemu_opt_get(opts, "x-dirty-bitmap"),
> -  qemu_opt_get_number(opts, "reconnect-delay", 0),
> -  errp);
> +s->x_dirty_bitmap = g_strdup(qemu_opt_get(opts, "x-dirty-bitmap"));
> +s->reconnect_delay = qemu_opt_get_number(opts, "reconnect-delay", 0);
> +
> +ret = 0;
>  
>   error:
> -if (tlscreds) {
> -object_unref(OBJECT(tlscreds));
> -}
>  if (ret < 0) {
> +if (s->tlscreds) {
> +object_unref(OBJECT(s->tlscreds));
> +}

Pre-existing, but object_unref(NULL) is safe, making this a useless 'if'.


> @@ -1726,9 +1725,13 @@ static void nbd_close(BlockDriverState *bs)
>  
>  nbd_client_close(bs);
>  
> +if (s->tlscreds) {
> +object_unref(OBJECT(s->tlscreds));
> +}

and copied here.

>  qapi_free_SocketAddress(s->saddr);
>  g_free(s->export);
>  g_free(s->tlscredsid);
> +g_free(s->x_dirty_bitmap);

Do we need to duplicate s->x_dirty_bitmap with s->info.x_dirty_bitmap,
or make the latter be const char * and reuse the pointer from the former
rather than strdup'ing it? (I don't think it affects the refactoring
done in this patch, but is possibly worth a separate cleanup patch).

I can fix up the two useless 'if's.

Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature

[Qemu-block] [PATCH v7 5/9] block/nbd: refactor nbd connection parameters

[Vladimir Sementsov-Ogievskiy]

We'll need some connection parameters to be available all the time to
implement nbd reconnect. So, let's refactor them: define additional
parameters in BDRVNBDState, drop them from function parameters, drop
nbd_client_init and separate options parsing instead from nbd_open.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/nbd.c | 125 +++-
 1 file changed, 64 insertions(+), 61 deletions(-)

diff --git a/block/nbd.c b/block/nbd.c
index df9b0a2c8a..68e0e168e8 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -73,9 +73,13 @@ typedef struct BDRVNBDState {
 NBDReply reply;
 BlockDriverState *bs;
 
-/* For nbd_refresh_filename() */
+/* Connection parameters */
+uint32_t reconnect_delay;
 SocketAddress *saddr;
 char *export, *tlscredsid;
+QCryptoTLSCreds *tlscreds;
+const char *hostname;
+char *x_dirty_bitmap;
 } BDRVNBDState;
 
 /* @ret will be used for reconnect in future */
@@ -1183,13 +1187,7 @@ static QIOChannelSocket 
*nbd_establish_connection(SocketAddress *saddr,
 return sioc;
 }
 
-static int nbd_client_connect(BlockDriverState *bs,
-  SocketAddress *saddr,
-  const char *export,
-  QCryptoTLSCreds *tlscreds,
-  const char *hostname,
-  const char *x_dirty_bitmap,
-  Error **errp)
+static int nbd_client_connect(BlockDriverState *bs, Error **errp)
 {
 BDRVNBDState *s = (BDRVNBDState *)bs->opaque;
 AioContext *aio_context = bdrv_get_aio_context(bs);
@@ -1199,33 +1197,33 @@ static int nbd_client_connect(BlockDriverState *bs,
  * establish TCP connection, return error if it fails
  * TODO: Configurable retry-until-timeout behaviour.
  */
-QIOChannelSocket *sioc = nbd_establish_connection(saddr, errp);
+QIOChannelSocket *sioc = nbd_establish_connection(s->saddr, errp);
 
 if (!sioc) {
 return -ECONNREFUSED;
 }
 
 /* NBD handshake */
-trace_nbd_client_connect(export);
+trace_nbd_client_connect(s->export);
 qio_channel_set_blocking(QIO_CHANNEL(sioc), false, NULL);
 qio_channel_attach_aio_context(QIO_CHANNEL(sioc), aio_context);
 
 s->info.request_sizes = true;
 s->info.structured_reply = true;
 s->info.base_allocation = true;
-s->info.x_dirty_bitmap = g_strdup(x_dirty_bitmap);
-s->info.name = g_strdup(export ?: "");
-ret = nbd_receive_negotiate(aio_context, QIO_CHANNEL(sioc), tlscreds,
-hostname, &s->ioc, &s->info, errp);
+s->info.x_dirty_bitmap = g_strdup(s->x_dirty_bitmap);
+s->info.name = g_strdup(s->export ?: "");
+ret = nbd_receive_negotiate(aio_context, QIO_CHANNEL(sioc), s->tlscreds,
+s->hostname, &s->ioc, &s->info, errp);
 g_free(s->info.x_dirty_bitmap);
 g_free(s->info.name);
 if (ret < 0) {
 object_unref(OBJECT(sioc));
 return ret;
 }
-if (x_dirty_bitmap && !s->info.base_allocation) {
+if (s->x_dirty_bitmap && !s->info.base_allocation) {
 error_setg(errp, "requested x-dirty-bitmap %s not found",
-   x_dirty_bitmap);
+   s->x_dirty_bitmap);
 ret = -EINVAL;
 goto fail;
 }
@@ -1250,7 +1248,7 @@ static int nbd_client_connect(BlockDriverState *bs,
 object_ref(OBJECT(s->ioc));
 }
 
-trace_nbd_client_connect_success(export);
+trace_nbd_client_connect_success(s->export);
 
 return 0;
 
@@ -1270,34 +1268,9 @@ static int nbd_client_connect(BlockDriverState *bs,
 }
 }
 
-static int nbd_client_init(BlockDriverState *bs,
-   SocketAddress *saddr,
-   const char *export,
-   QCryptoTLSCreds *tlscreds,
-   const char *hostname,
-   const char *x_dirty_bitmap,
-   uint32_t reconnect_delay,
-   Error **errp)
-{
-int ret;
-BDRVNBDState *s = (BDRVNBDState *)bs->opaque;
-
-s->bs = bs;
-qemu_co_mutex_init(&s->send_mutex);
-qemu_co_queue_init(&s->free_sema);
-
-ret = nbd_client_connect(bs, saddr, export, tlscreds, hostname,
- x_dirty_bitmap, errp);
-if (ret < 0) {
-return ret;
-}
-
-s->connection_co = qemu_coroutine_create(nbd_connection_entry, s);
-bdrv_inc_in_flight(bs);
-aio_co_schedule(bdrv_get_aio_context(bs), s->connection_co);
-
-return 0;
-}
+/*
+ * Parse nbd_open options
+ */
 
 static int nbd_parse_uri(const char *filename, QDict *options)
 {
@@ -1617,14 +1590,12 @@ static QemuOptsList nbd_runtime_opts = {
 },
 };
 
-static int nbd_open(BlockDriverState *bs, QDict *options, int flags,
-Error **errp)
+static int nbd_process_options(BlockDriverState *bs, QDict *options,
+