Re: [PATCH v2 1/5] block/nbd: split nbd_establish_connection out of nbd_client_connect

2020-07-27 Thread Eric Blake

On 7/27/20 1:47 PM, Vladimir Sementsov-Ogievskiy wrote:

We are going to implement non-blocking version of
nbd_establish_connection, which for a while will be used only for
nbd_reconnect_attempt, not for nbd_open, so we need to call it
separately.

Refactor nbd_reconnect_attempt in a way which makes next commit
simpler.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  block/nbd.c| 60 +++---
  block/trace-events |  4 ++--
  2 files changed, 38 insertions(+), 26 deletions(-)



Reviewed-by: Eric Blake 

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




[PATCH v2 1/5] block/nbd: split nbd_establish_connection out of nbd_client_connect

2020-07-27 Thread Vladimir Sementsov-Ogievskiy
We are going to implement non-blocking version of
nbd_establish_connection, which for a while will be used only for
nbd_reconnect_attempt, not for nbd_open, so we need to call it
separately.

Refactor nbd_reconnect_attempt in a way which makes next commit
simpler.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/nbd.c| 60 +++---
 block/trace-events |  4 ++--
 2 files changed, 38 insertions(+), 26 deletions(-)

diff --git a/block/nbd.c b/block/nbd.c
index 65a4f56924..2ec6623c18 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -93,7 +93,10 @@ typedef struct BDRVNBDState {
 char *x_dirty_bitmap;
 } BDRVNBDState;
 
-static int nbd_client_connect(BlockDriverState *bs, Error **errp);
+static QIOChannelSocket *nbd_establish_connection(SocketAddress *saddr,
+  Error **errp);
+static int nbd_client_handshake(BlockDriverState *bs, QIOChannelSocket *sioc,
+Error **errp);
 
 static void nbd_clear_bdrvstate(BDRVNBDState *s)
 {
@@ -241,7 +244,9 @@ static bool nbd_client_connecting_wait(BDRVNBDState *s)
 
 static coroutine_fn void nbd_reconnect_attempt(BDRVNBDState *s)
 {
+int ret;
 Error *local_err = NULL;
+QIOChannelSocket *sioc;
 
 if (!nbd_client_connecting(s)) {
 return;
@@ -280,19 +285,25 @@ static coroutine_fn void 
nbd_reconnect_attempt(BDRVNBDState *s)
 s->ioc = NULL;
 }
 
-s->connect_status = nbd_client_connect(s->bs, _err);
+sioc = nbd_establish_connection(s->saddr, _err);
+if (!sioc) {
+ret = -ECONNREFUSED;
+goto out;
+}
+
+ret = nbd_client_handshake(s->bs, sioc, _err);
+
+out:
+s->connect_status = ret;
 error_free(s->connect_err);
 s->connect_err = NULL;
 error_propagate(>connect_err, local_err);
 
-if (s->connect_status < 0) {
-/* failed attempt */
-return;
+if (ret >= 0) {
+/* successfully connected */
+s->state = NBD_CLIENT_CONNECTED;
+qemu_co_queue_restart_all(>free_sema);
 }
-
-/* successfully connected */
-s->state = NBD_CLIENT_CONNECTED;
-qemu_co_queue_restart_all(>free_sema);
 }
 
 static coroutine_fn void nbd_co_reconnect_loop(BDRVNBDState *s)
@@ -1425,24 +1436,15 @@ static QIOChannelSocket 
*nbd_establish_connection(SocketAddress *saddr,
 return sioc;
 }
 
-static int nbd_client_connect(BlockDriverState *bs, Error **errp)
+/* nbd_client_handshake takes ownership on sioc. On failure it is unref'ed. */
+static int nbd_client_handshake(BlockDriverState *bs, QIOChannelSocket *sioc,
+Error **errp)
 {
 BDRVNBDState *s = (BDRVNBDState *)bs->opaque;
 AioContext *aio_context = bdrv_get_aio_context(bs);
 int ret;
 
-/*
- * establish TCP connection, return error if it fails
- * TODO: Configurable retry-until-timeout behaviour.
- */
-QIOChannelSocket *sioc = nbd_establish_connection(s->saddr, errp);
-
-if (!sioc) {
-return -ECONNREFUSED;
-}
-
-/* NBD handshake */
-trace_nbd_client_connect(s->export);
+trace_nbd_client_handshake(s->export);
 qio_channel_set_blocking(QIO_CHANNEL(sioc), false, NULL);
 qio_channel_attach_aio_context(QIO_CHANNEL(sioc), aio_context);
 
@@ -1489,7 +1491,7 @@ static int nbd_client_connect(BlockDriverState *bs, Error 
**errp)
 object_ref(OBJECT(s->ioc));
 }
 
-trace_nbd_client_connect_success(s->export);
+trace_nbd_client_handshake_success(s->export);
 
 return 0;
 
@@ -1894,6 +1896,7 @@ static int nbd_open(BlockDriverState *bs, QDict *options, 
int flags,
 {
 int ret;
 BDRVNBDState *s = (BDRVNBDState *)bs->opaque;
+QIOChannelSocket *sioc;
 
 ret = nbd_process_options(bs, options, errp);
 if (ret < 0) {
@@ -1904,7 +1907,16 @@ static int nbd_open(BlockDriverState *bs, QDict 
*options, int flags,
 qemu_co_mutex_init(>send_mutex);
 qemu_co_queue_init(>free_sema);
 
-ret = nbd_client_connect(bs, errp);
+/*
+ * establish TCP connection, return error if it fails
+ * TODO: Configurable retry-until-timeout behaviour.
+ */
+sioc = nbd_establish_connection(s->saddr, errp);
+if (!sioc) {
+return -ECONNREFUSED;
+}
+
+ret = nbd_client_handshake(bs, sioc, errp);
 if (ret < 0) {
 nbd_clear_bdrvstate(s);
 return ret;
diff --git a/block/trace-events b/block/trace-events
index d3533ca896..9158335061 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -168,8 +168,8 @@ nbd_parse_blockstatus_compliance(const char *err) "ignoring 
extra data from non-
 nbd_structured_read_compliance(const char *type) "server sent non-compliant 
unaligned read %s chunk"
 nbd_read_reply_entry_fail(int ret, const char *err) "ret = %d, err: %s"
 nbd_co_request_fail(uint64_t from, uint32_t len, uint64_t handle, uint16_t 
flags, uint16_t type, const char *name, int ret, const char *err) "Request 
failed { .from = %"