[Qemu-devel] [PATCH 00/19] nbd errors and traces refactoring

[Qemu-devel] [PATCH 00/19] nbd errors and traces refactoring

[Vladimir Sementsov-Ogievskiy]

This is based on my "nbd: error path refactoring" series.

These series bring errp instead of LOG to the server too, making
some related refactorings.

nbd_negotiate_read and friends a dropped as was discussed in
"nbd: error path refactoring" thread.

finally - refactor traces to modern way too, to get rid of LOG()
macro at all.

Patches subgroup 01-x, where x <= 10 may be pushed as separate series,
as they are general enough.

11-14 needed to not create error-output on non-error disconnects in 15
16 is good addition for 15
17 is addition for 15 and in the same time prerequisite for 19

patches 18-19 may be pushed separately (but of course after 01-17 and
19 after 18)

Vladimir Sementsov-Ogievskiy (19):
  nbd/server: get rid of nbd_negotiate_read and friends
  nbd/server: get rid of ssize_t
  nbd/server: refactor nbd_co_send_reply
  nbd/server: get rid of EAGAIN dead code
  nbd/server: refactor nbd_co_receive_request
  nbd/server: remove NBDClientNewData
  nbd/server: nbd_negotiate: fix error path
  nbd/server: get rid of fail: return rc
  nbd/server: rename rc to ret
  nbd/server: refactor nbd_trip
  io/channel-socket: qio_channel_socket_writev handle EPIPE
  nbd/common: nbd_wr_syncv handle QIO_CHANNEL_ERR_EPIPE
  nbd/server: return original error codes
  nbd/server: nbd_negotiate: return 1 on NBD_OPT_ABORT
  nbd/server: use errp instead of LOG
  nbd/server: add errp to nbd_send_reply()
  nbd/common: nbd_tls_handshake: use error_reportf_err instead of TRACE
  nbd/client: refactor TRACE of NBD_MAGIC
  nbd: use generic trace subsystem instead of TRACE macro

 Makefile.objs        |   1 +
 include/io/channel.h |   1 +
 io/channel-socket.c  |   2 +-
 nbd/client.c         | 127 ++++------
 nbd/common.c         |  30 ++-
 nbd/nbd-internal.h   |  21 +-
 nbd/server.c         | 670 ++++++++++++++++++++++++---------------------------
 nbd/trace-events     |  67 ++++++
 8 files changed, 471 insertions(+), 448 deletions(-)
 create mode 100644 nbd/trace-events

-- 
2.11.1

[Qemu-devel] [PATCH 01/19] nbd/server: get rid of nbd_negotiate_read and friends

[Vladimir Sementsov-Ogievskiy]

Functions nbd_negotiate_{read,write,drop_sync} were introduced in
1a6245a5b, when nbd_wr_syncv was working through qemu_co_sendv_recvv,
which just yields, without setting any handlers. But now, nbd_wr_syncv
works through qio_channel_yield() which sets handlers, so watchers
are redundant in nbd_negotiate_{read,write,drop_sync}, then, let's just
use {read,write,drop}_sync functions.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 nbd/client.c       |  26 -------------
 nbd/common.c       |  26 +++++++++++++
 nbd/nbd-internal.h |   2 +
 nbd/server.c       | 107 +++++++++++------------------------------------------
 4 files changed, 50 insertions(+), 111 deletions(-)

diff --git a/nbd/client.c b/nbd/client.c
index f9e1d75be4..3d15596120 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -86,32 +86,6 @@ static QTAILQ_HEAD(, NBDExport) exports = QTAILQ_HEAD_INITIALIZER(exports);
 
 */
 
-/* Discard length bytes from channel.  Return -errno on failure and 0 on
- * success*/
-static int drop_sync(QIOChannel *ioc, size_t size, Error **errp)
-{
-    ssize_t ret = 0;
-    char small[1024];
-    char *buffer;
-
-    buffer = sizeof(small) >= size ? small : g_malloc(MIN(65536, size));
-    while (size > 0) {
-        ssize_t count = MIN(65536, size);
-        ret = read_sync(ioc, buffer, MIN(65536, size), errp);
-
-        if (ret < 0) {
-            goto cleanup;
-        }
-        size -= count;
-    }
-
- cleanup:
-    if (buffer != small) {
-        g_free(buffer);
-    }
-    return ret;
-}
-
 /* Send an option request.
  *
  * The request is for option @opt, with @data containing @len bytes of
diff --git a/nbd/common.c b/nbd/common.c
index bd81637ab9..e520aae741 100644
--- a/nbd/common.c
+++ b/nbd/common.c
@@ -69,6 +69,32 @@ ssize_t nbd_wr_syncv(QIOChannel *ioc,
     return done;
 }
 
+/* Discard length bytes from channel.  Return -errno on failure and 0 on
+ * success*/
+int drop_sync(QIOChannel *ioc, size_t size, Error **errp)
+{
+    ssize_t ret = 0;
+    char small[1024];
+    char *buffer;
+
+    buffer = sizeof(small) >= size ? small : g_malloc(MIN(65536, size));
+    while (size > 0) {
+        ssize_t count = MIN(65536, size);
+        ret = read_sync(ioc, buffer, MIN(65536, size), errp);
+
+        if (ret < 0) {
+            goto cleanup;
+        }
+        size -= count;
+    }
+
+ cleanup:
+    if (buffer != small) {
+        g_free(buffer);
+    }
+    return ret;
+}
+
 
 void nbd_tls_handshake(QIOTask *task,
                        void *opaque)
diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h
index d6071640a0..01d5778679 100644
--- a/nbd/nbd-internal.h
+++ b/nbd/nbd-internal.h
@@ -153,4 +153,6 @@ struct NBDTLSHandshakeData {
 void nbd_tls_handshake(QIOTask *task,
                        void *opaque);
 
+int drop_sync(QIOChannel *ioc, size_t size, Error **errp);
+
 #endif
diff --git a/nbd/server.c b/nbd/server.c
index ee59e5d234..26096f14e1 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -104,69 +104,6 @@ struct NBDClient {
 
 static void nbd_client_receive_next_request(NBDClient *client);
 
-static gboolean nbd_negotiate_continue(QIOChannel *ioc,
-                                       GIOCondition condition,
-                                       void *opaque)
-{
-    qemu_coroutine_enter(opaque);
-    return TRUE;
-}
-
-static int nbd_negotiate_read(QIOChannel *ioc, void *buffer, size_t size)
-{
-    ssize_t ret;
-    guint watch;
-
-    assert(qemu_in_coroutine());
-    /* Negotiation are always in main loop. */
-    watch = qio_channel_add_watch(ioc,
-                                  G_IO_IN,
-                                  nbd_negotiate_continue,
-                                  qemu_coroutine_self(),
-                                  NULL);
-    ret = read_sync(ioc, buffer, size, NULL);
-    g_source_remove(watch);
-    return ret;
-
-}
-
-static int nbd_negotiate_write(QIOChannel *ioc, const void *buffer, size_t size)
-{
-    ssize_t ret;
-    guint watch;
-
-    assert(qemu_in_coroutine());
-    /* Negotiation are always in main loop. */
-    watch = qio_channel_add_watch(ioc,
-                                  G_IO_OUT,
-                                  nbd_negotiate_continue,
-                                  qemu_coroutine_self(),
-                                  NULL);
-    ret = write_sync(ioc, buffer, size, NULL);
-    g_source_remove(watch);
-    return ret;
-}
-
-static int nbd_negotiate_drop_sync(QIOChannel *ioc, size_t size)
-{
-    ssize_t ret;
-    uint8_t *buffer = g_malloc(MIN(65536, size));
-
-    while (size > 0) {
-        size_t count = MIN(65536, size);
-        ret = nbd_negotiate_read(ioc, buffer, count);
-        if (ret < 0) {
-            g_free(buffer);
-            return ret;
-        }
-
-        size -= count;
-    }
-
-    g_free(buffer);
-    return 0;
-}
-
 /* Basic flow for negotiation
 
    Server         Client
@@ -205,22 +142,22 @@ static int nbd_negotiate_send_rep_len(QIOChannel *ioc, uint32_t type,
           type, opt, len);
 
     magic = cpu_to_be64(NBD_REP_MAGIC);
-    if (nbd_negotiate_write(ioc, &magic, sizeof(magic)) < 0) {
+    if (write_sync(ioc, &magic, sizeof(magic), NULL) < 0) {
         LOG("write failed (rep magic)");
         return -EINVAL;
     }
     opt = cpu_to_be32(opt);
-    if (nbd_negotiate_write(ioc, &opt, sizeof(opt)) < 0) {
+    if (write_sync(ioc, &opt, sizeof(opt), NULL) < 0) {
         LOG("write failed (rep opt)");
         return -EINVAL;
     }
     type = cpu_to_be32(type);
-    if (nbd_negotiate_write(ioc, &type, sizeof(type)) < 0) {
+    if (write_sync(ioc, &type, sizeof(type), NULL) < 0) {
         LOG("write failed (rep type)");
         return -EINVAL;
     }
     len = cpu_to_be32(len);
-    if (nbd_negotiate_write(ioc, &len, sizeof(len)) < 0) {
+    if (write_sync(ioc, &len, sizeof(len), NULL) < 0) {
         LOG("write failed (rep data length)");
         return -EINVAL;
     }
@@ -255,7 +192,7 @@ nbd_negotiate_send_rep_err(QIOChannel *ioc, uint32_t type,
     if (ret < 0) {
         goto out;
     }
-    if (nbd_negotiate_write(ioc, msg, len) < 0) {
+    if (write_sync(ioc, msg, len, NULL) < 0) {
         LOG("write failed (error message)");
         ret = -EIO;
     } else {
@@ -286,15 +223,15 @@ static int nbd_negotiate_send_rep_list(QIOChannel *ioc, NBDExport *exp)
     }
 
     len = cpu_to_be32(name_len);
-    if (nbd_negotiate_write(ioc, &len, sizeof(len)) < 0) {
+    if (write_sync(ioc, &len, sizeof(len), NULL) < 0) {
         LOG("write failed (name length)");
         return -EINVAL;
     }
-    if (nbd_negotiate_write(ioc, name, name_len) < 0) {
+    if (write_sync(ioc, name, name_len, NULL) < 0) {
         LOG("write failed (name buffer)");
         return -EINVAL;
     }
-    if (nbd_negotiate_write(ioc, desc, desc_len) < 0) {
+    if (write_sync(ioc, desc, desc_len, NULL) < 0) {
         LOG("write failed (description buffer)");
         return -EINVAL;
     }
@@ -308,7 +245,7 @@ static int nbd_negotiate_handle_list(NBDClient *client, uint32_t length)
     NBDExport *exp;
 
     if (length) {
-        if (nbd_negotiate_drop_sync(client->ioc, length) < 0) {
+        if (drop_sync(client->ioc, length, NULL) < 0) {
             return -EIO;
         }
         return nbd_negotiate_send_rep_err(client->ioc,
@@ -339,7 +276,7 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, uint32_t length)
         LOG("Bad length received");
         goto fail;
     }
-    if (nbd_negotiate_read(client->ioc, name, length) < 0) {
+    if (read_sync(client->ioc, name, length, NULL) < 0) {
         LOG("read failed");
         goto fail;
     }
@@ -372,7 +309,7 @@ static QIOChannel *nbd_negotiate_handle_starttls(NBDClient *client,
     TRACE("Setting up TLS");
     ioc = client->ioc;
     if (length) {
-        if (nbd_negotiate_drop_sync(ioc, length) < 0) {
+        if (drop_sync(ioc, length, NULL) < 0) {
             return NULL;
         }
         nbd_negotiate_send_rep_err(ioc, NBD_REP_ERR_INVALID, NBD_OPT_STARTTLS,
@@ -436,7 +373,7 @@ static int nbd_negotiate_options(NBDClient *client)
         ...           Rest of request
     */
 
-    if (nbd_negotiate_read(client->ioc, &flags, sizeof(flags)) < 0) {
+    if (read_sync(client->ioc, &flags, sizeof(flags), NULL) < 0) {
         LOG("read failed");
         return -EIO;
     }
@@ -462,7 +399,7 @@ static int nbd_negotiate_options(NBDClient *client)
         uint32_t clientflags, length;
         uint64_t magic;
 
-        if (nbd_negotiate_read(client->ioc, &magic, sizeof(magic)) < 0) {
+        if (read_sync(client->ioc, &magic, sizeof(magic), NULL) < 0) {
             LOG("read failed");
             return -EINVAL;
         }
@@ -472,15 +409,15 @@ static int nbd_negotiate_options(NBDClient *client)
             return -EINVAL;
         }
 
-        if (nbd_negotiate_read(client->ioc, &clientflags,
-                               sizeof(clientflags)) < 0)
+        if (read_sync(client->ioc, &clientflags,
+                      sizeof(clientflags), NULL) < 0)
         {
             LOG("read failed");
             return -EINVAL;
         }
         clientflags = be32_to_cpu(clientflags);
 
-        if (nbd_negotiate_read(client->ioc, &length, sizeof(length)) < 0) {
+        if (read_sync(client->ioc, &length, sizeof(length), NULL) < 0) {
             LOG("read failed");
             return -EINVAL;
         }
@@ -510,7 +447,7 @@ static int nbd_negotiate_options(NBDClient *client)
                 return -EINVAL;
 
             default:
-                if (nbd_negotiate_drop_sync(client->ioc, length) < 0) {
+                if (drop_sync(client->ioc, length, NULL) < 0) {
                     return -EIO;
                 }
                 ret = nbd_negotiate_send_rep_err(client->ioc,
@@ -548,7 +485,7 @@ static int nbd_negotiate_options(NBDClient *client)
                 return nbd_negotiate_handle_export_name(client, length);
 
             case NBD_OPT_STARTTLS:
-                if (nbd_negotiate_drop_sync(client->ioc, length) < 0) {
+                if (drop_sync(client->ioc, length, NULL) < 0) {
                     return -EIO;
                 }
                 if (client->tlscreds) {
@@ -567,7 +504,7 @@ static int nbd_negotiate_options(NBDClient *client)
                 }
                 break;
             default:
-                if (nbd_negotiate_drop_sync(client->ioc, length) < 0) {
+                if (drop_sync(client->ioc, length, NULL) < 0) {
                     return -EIO;
                 }
                 ret = nbd_negotiate_send_rep_err(client->ioc,
@@ -656,12 +593,12 @@ static coroutine_fn int nbd_negotiate(NBDClientNewData *data)
             TRACE("TLS cannot be enabled with oldstyle protocol");
             goto fail;
         }
-        if (nbd_negotiate_write(client->ioc, buf, sizeof(buf)) < 0) {
+        if (write_sync(client->ioc, buf, sizeof(buf), NULL) < 0) {
             LOG("write failed");
             goto fail;
         }
     } else {
-        if (nbd_negotiate_write(client->ioc, buf, 18) < 0) {
+        if (write_sync(client->ioc, buf, 18, NULL) < 0) {
             LOG("write failed");
             goto fail;
         }
@@ -676,7 +613,7 @@ static coroutine_fn int nbd_negotiate(NBDClientNewData *data)
         stq_be_p(buf + 18, client->exp->size);
         stw_be_p(buf + 26, client->exp->nbdflags | myflags);
         len = client->no_zeroes ? 10 : sizeof(buf) - 18;
-        if (nbd_negotiate_write(client->ioc, buf + 18, len) < 0) {
+        if (write_sync(client->ioc, buf + 18, len, NULL) < 0) {
             LOG("write failed");
             goto fail;
         }
-- 
2.11.1

Re: [Qemu-devel] [PATCH 01/19] nbd/server: get rid of nbd_negotiate_read and friends

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2044 bytes --]

On 05/30/2017 09:30 AM, Vladimir Sementsov-Ogievskiy wrote:
> Functions nbd_negotiate_{read,write,drop_sync} were introduced in
> 1a6245a5b, when nbd_wr_syncv was working through qemu_co_sendv_recvv,

There is no qemu_co_sendv_recvv. Did you mean qemu_co_recv/qemu_co_send?

> which just yields, without setting any handlers. But now, nbd_wr_syncv
> works through qio_channel_yield() which sets handlers, so watchers
> are redundant in nbd_negotiate_{read,write,drop_sync}, then, let's just
> use {read,write,drop}_sync functions.

Makes sense to me.  Note that I have a pending patch that was also
related to 1a6245a5, where we introduced an assertion failure (which
later morphed into a segfault) if a client hangs up really early during
negotiation:

https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg06240.html

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---

> +++ b/nbd/common.c
> @@ -69,6 +69,32 @@ ssize_t nbd_wr_syncv(QIOChannel *ioc,
>      return done;
>  }
>  
> +/* Discard length bytes from channel.  Return -errno on failure and 0 on
> + * success*/

Space before */, if you don't mind (yes, I know this was just code
motion, and that the old spot was wrong).

> +int drop_sync(QIOChannel *ioc, size_t size, Error **errp)

As part of moving it into nbd/common.c, please rename this function to
an nbd_ prefix, since non-static functions are more likely to collide
with the rest of the code base if not properly named.  Better yet: do it
in multiple patches:
- rename the static functions and fallout to callers (all of
nbd_drop_sync, nbd_read_sync, and nbd_write_sync)
- code motion (promote nbd_drop_sync from static in client.c to exported
in common.c)
- drop nbd_negotiate_ functions in favor of common nbd_*_sync functions

The idea makes sense, but I think there's too much going on in this one
commit.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 01/19] nbd/server: get rid of nbd_negotiate_read and friends

[Vladimir Sementsov-Ogievskiy]

30.05.2017 23:10, Eric Blake wrote:
> On 05/30/2017 09:30 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Functions nbd_negotiate_{read,write,drop_sync} were introduced in
>> 1a6245a5b, when nbd_wr_syncv was working through qemu_co_sendv_recvv,
> There is no qemu_co_sendv_recvv. Did you mean qemu_co_recv/qemu_co_send?

qemu_co_recv is a macro, qemu_co_sendv_recvv - is an actual function, 
which do something.

#define qemu_co_recv(sockfd, buf, bytes) \
   qemu_co_send_recv(sockfd, buf, bytes, false)

ssize_t coroutine_fn
qemu_co_send_recv(int sockfd, void *buf, size_t bytes, bool do_send)
{
     struct iovec iov = { .iov_base = buf, .iov_len = bytes };
     return qemu_co_sendv_recvv(sockfd, &iov, 1, 0, bytes, do_send);
}


>
>> which just yields, without setting any handlers. But now, nbd_wr_syncv
>> works through qio_channel_yield() which sets handlers, so watchers
>> are redundant in nbd_negotiate_{read,write,drop_sync}, then, let's just
>> use {read,write,drop}_sync functions.
> Makes sense to me.  Note that I have a pending patch that was also
> related to 1a6245a5, where we introduced an assertion failure (which
> later morphed into a segfault) if a client hangs up really early during
> negotiation:
>
> https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg06240.html
>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>> +++ b/nbd/common.c
>> @@ -69,6 +69,32 @@ ssize_t nbd_wr_syncv(QIOChannel *ioc,
>>       return done;
>>   }
>>   
>> +/* Discard length bytes from channel.  Return -errno on failure and 0 on
>> + * success*/
> Space before */, if you don't mind (yes, I know this was just code
> motion, and that the old spot was wrong).
>
>> +int drop_sync(QIOChannel *ioc, size_t size, Error **errp)
> As part of moving it into nbd/common.c, please rename this function to
> an nbd_ prefix, since non-static functions are more likely to collide
> with the rest of the code base if not properly named.  Better yet: do it
> in multiple patches:
> - rename the static functions and fallout to callers (all of
> nbd_drop_sync, nbd_read_sync, and nbd_write_sync)
> - code motion (promote nbd_drop_sync from static in client.c to exported
> in common.c)
> - drop nbd_negotiate_ functions in favor of common nbd_*_sync functions

OK

>
> The idea makes sense, but I think there's too much going on in this one
> commit.
>

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 01/19] nbd/server: get rid of nbd_negotiate_read and friends

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2364 bytes --]

On 05/31/2017 08:12 AM, Vladimir Sementsov-Ogievskiy wrote:
> 30.05.2017 23:10, Eric Blake wrote:
>> On 05/30/2017 09:30 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> Functions nbd_negotiate_{read,write,drop_sync} were introduced in
>>> 1a6245a5b, when nbd_wr_syncv was working through qemu_co_sendv_recvv,
>> There is no qemu_co_sendv_recvv. Did you mean qemu_co_recv/qemu_co_send?
> 
> qemu_co_recv is a macro, qemu_co_sendv_recvv - is an actual function,
> which do something.

Oh. I see where I went wrong - I was grepping for 'nbd_co_sendv' and
coming up short.

> 
> #define qemu_co_recv(sockfd, buf, bytes) \
>   qemu_co_send_recv(sockfd, buf, bytes, false)
> 
> ssize_t coroutine_fn
> qemu_co_send_recv(int sockfd, void *buf, size_t bytes, bool do_send)
> {
>     struct iovec iov = { .iov_base = buf, .iov_len = bytes };
>     return qemu_co_sendv_recvv(sockfd, &iov, 1, 0, bytes, do_send);
> }

The commit message still makes me chase through several layers of
indirection, so I still wonder if referring to qemu_co_recv/qemu_co_send
(which is what is directly used in that older commit for nbd_wr_syncv)
is any better.  It is probably also worth referring back to commit
ff82911cd as the point in time where we switched to the qio_channel
code, thereby no longer needing to manage coroutine handlers quite as
directly as beforehand.


>>> +int drop_sync(QIOChannel *ioc, size_t size, Error **errp)
>> As part of moving it into nbd/common.c, please rename this function to
>> an nbd_ prefix, since non-static functions are more likely to collide
>> with the rest of the code base if not properly named.  Better yet: do it
>> in multiple patches:
>> - rename the static functions and fallout to callers (all of
>> nbd_drop_sync, nbd_read_sync, and nbd_write_sync)

In fact, does the _sync name buy us anything any more, or can we just go
with the shorter nbd_drop(), nbd_read(), and nbd_write()?

>> - code motion (promote nbd_drop_sync from static in client.c to exported
>> in common.c)
>> - drop nbd_negotiate_ functions in favor of common nbd_*_sync functions
> 
> OK
> 
>>
>> The idea makes sense, but I think there's too much going on in this one
>> commit.
>>
> 

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 01/19] nbd/server: get rid of nbd_negotiate_read and friends

[Vladimir Sementsov-Ogievskiy]

31.05.2017 17:39, Eric Blake wrote:
> On 05/31/2017 08:12 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 30.05.2017 23:10, Eric Blake wrote:
>>> On 05/30/2017 09:30 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>> Functions nbd_negotiate_{read,write,drop_sync} were introduced in
>>>> 1a6245a5b, when nbd_wr_syncv was working through qemu_co_sendv_recvv,
>>> There is no qemu_co_sendv_recvv. Did you mean qemu_co_recv/qemu_co_send?
>> qemu_co_recv is a macro, qemu_co_sendv_recvv - is an actual function,
>> which do something.
> Oh. I see where I went wrong - I was grepping for 'nbd_co_sendv' and
> coming up short.
>
>> #define qemu_co_recv(sockfd, buf, bytes) \
>>    qemu_co_send_recv(sockfd, buf, bytes, false)
>>
>> ssize_t coroutine_fn
>> qemu_co_send_recv(int sockfd, void *buf, size_t bytes, bool do_send)
>> {
>>      struct iovec iov = { .iov_base = buf, .iov_len = bytes };
>>      return qemu_co_sendv_recvv(sockfd, &iov, 1, 0, bytes, do_send);
>> }
> The commit message still makes me chase through several layers of
> indirection, so I still wonder if referring to qemu_co_recv/qemu_co_send

I'll add a note like "(the path is nbd_wr_sync -> qemu_co_{recv/send} -> 
qemu_co_send_recv -> qemu_co_sendv_recvv)',
because I writing that qemu_co_recv yields will be confusing too.

> (which is what is directly used in that older commit for nbd_wr_syncv)
> is any better.  It is probably also worth referring back to commit
> ff82911cd as the point in time where we switched to the qio_channel
> code, thereby no longer needing to manage coroutine handlers quite as
> directly as beforehand.

OK

>
>
>>>> +int drop_sync(QIOChannel *ioc, size_t size, Error **errp)
>>> As part of moving it into nbd/common.c, please rename this function to
>>> an nbd_ prefix, since non-static functions are more likely to collide
>>> with the rest of the code base if not properly named.  Better yet: do it
>>> in multiple patches:
>>> - rename the static functions and fallout to callers (all of
>>> nbd_drop_sync, nbd_read_sync, and nbd_write_sync)
> In fact, does the _sync name buy us anything any more, or can we just go
> with the shorter nbd_drop(), nbd_read(), and nbd_write()?

OK, good idea.

>
>>> - code motion (promote nbd_drop_sync from static in client.c to exported
>>> in common.c)
>>> - drop nbd_negotiate_ functions in favor of common nbd_*_sync functions
>> OK
>>
>>> The idea makes sense, but I think there's too much going on in this one
>>> commit.
>>>

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 01/19] nbd/server: get rid of nbd_negotiate_read and friends

[Vladimir Sementsov-Ogievskiy]

31.05.2017 17:56, Vladimir Sementsov-Ogievskiy wrote:
> 31.05.2017 17:39, Eric Blake wrote:
>> On 05/31/2017 08:12 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> 30.05.2017 23:10, Eric Blake wrote:
>>>> On 05/30/2017 09:30 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>>> Functions nbd_negotiate_{read,write,drop_sync} were introduced in
>>>>> 1a6245a5b, when nbd_wr_syncv was working through qemu_co_sendv_recvv,
>>>> There is no qemu_co_sendv_recvv. Did you mean 
>>>> qemu_co_recv/qemu_co_send?
>>> qemu_co_recv is a macro, qemu_co_sendv_recvv - is an actual function,
>>> which do something.
>> Oh. I see where I went wrong - I was grepping for 'nbd_co_sendv' and
>> coming up short.
>>
>>> #define qemu_co_recv(sockfd, buf, bytes) \
>>>    qemu_co_send_recv(sockfd, buf, bytes, false)
>>>
>>> ssize_t coroutine_fn
>>> qemu_co_send_recv(int sockfd, void *buf, size_t bytes, bool do_send)
>>> {
>>>      struct iovec iov = { .iov_base = buf, .iov_len = bytes };
>>>      return qemu_co_sendv_recvv(sockfd, &iov, 1, 0, bytes, do_send);
>>> }
>> The commit message still makes me chase through several layers of
>> indirection, so I still wonder if referring to qemu_co_recv/qemu_co_send
>
> I'll add a note like "(the path is nbd_wr_sync -> qemu_co_{recv/send} 
> -> qemu_co_send_recv -> qemu_co_sendv_recvv)',
> because I writing that qemu_co_recv yields will be confusing too.
>
>> (which is what is directly used in that older commit for nbd_wr_syncv)
>> is any better.  It is probably also worth referring back to commit
>> ff82911cd as the point in time where we switched to the qio_channel
>> code, thereby no longer needing to manage coroutine handlers quite as
>> directly as beforehand.
>
> OK
>
>>
>>
>>>>> +int drop_sync(QIOChannel *ioc, size_t size, Error **errp)
>>>> As part of moving it into nbd/common.c, please rename this function to
>>>> an nbd_ prefix, since non-static functions are more likely to collide
>>>> with the rest of the code base if not properly named. Better yet: 
>>>> do it
>>>> in multiple patches:
>>>> - rename the static functions and fallout to callers (all of
>>>> nbd_drop_sync, nbd_read_sync, and nbd_write_sync)
>> In fact, does the _sync name buy us anything any more, or can we just go
>> with the shorter nbd_drop(), nbd_read(), and nbd_write()?
>
> OK, good idea.

nbd_wr_syncv shoud be renamed then too.

As I understand, sync here is related to the fact that on EAGAIN from 
socket the function doesn't return. now it is also true (but instead of 
looping the function yields)..

On the other hand, it is normal for r/w functions to by synchronous, so 
having additional suffix for it looks redundant (contrariwise, we have
_aio suffix for async functions).

also, _sync suffix in block layer is used when function does flush (so 
using it for other thing is confusing a bit).
>
>>
>>>> - code motion (promote nbd_drop_sync from static in client.c to 
>>>> exported
>>>> in common.c)
>>>> - drop nbd_negotiate_ functions in favor of common nbd_*_sync 
>>>> functions
>>> OK
>>>
>>>> The idea makes sense, but I think there's too much going on in this 
>>>> one
>>>> commit.
>>>>
>

-- 
Best regards,
Vladimir

[Qemu-devel] [PATCH 02/19] nbd/server: get rid of ssize_t

[Vladimir Sementsov-Ogievskiy]

Now read_sync and friends return int, so get rid of ssize_t.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 nbd/server.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 26096f14e1..03880fd501 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -625,11 +625,11 @@ fail:
     return rc;
 }
 
-static ssize_t nbd_receive_request(QIOChannel *ioc, NBDRequest *request)
+static int nbd_receive_request(QIOChannel *ioc, NBDRequest *request)
 {
     uint8_t buf[NBD_REQUEST_SIZE];
     uint32_t magic;
-    ssize_t ret;
+    int ret;
 
     ret = read_sync(ioc, buf, sizeof(buf), NULL);
     if (ret < 0) {
@@ -663,7 +663,7 @@ static ssize_t nbd_receive_request(QIOChannel *ioc, NBDRequest *request)
     return 0;
 }
 
-static ssize_t nbd_send_reply(QIOChannel *ioc, NBDReply *reply)
+static int nbd_send_reply(QIOChannel *ioc, NBDReply *reply)
 {
     uint8_t buf[NBD_REPLY_SIZE];
 
@@ -969,11 +969,10 @@ void nbd_export_close_all(void)
     }
 }
 
-static ssize_t nbd_co_send_reply(NBDRequestData *req, NBDReply *reply,
-                                 int len)
+static int nbd_co_send_reply(NBDRequestData *req, NBDReply *reply, int len)
 {
     NBDClient *client = req->client;
-    ssize_t rc, ret;
+    int rc, ret;
 
     g_assert(qemu_in_coroutine());
     qemu_co_mutex_lock(&client->send_lock);
@@ -1003,11 +1002,10 @@ static ssize_t nbd_co_send_reply(NBDRequestData *req, NBDReply *reply,
  * and any other negative value to report an error to the client
  * (although the caller may still need to disconnect after reporting
  * the error).  */
-static ssize_t nbd_co_receive_request(NBDRequestData *req,
-                                      NBDRequest *request)
+static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request)
 {
     NBDClient *client = req->client;
-    ssize_t rc;
+    int rc;
 
     g_assert(qemu_in_coroutine());
     assert(client->recv_coroutine == qemu_coroutine_self());
@@ -1105,7 +1103,7 @@ static coroutine_fn void nbd_trip(void *opaque)
     NBDRequestData *req;
     NBDRequest request = { 0 };    /* GCC thinks it can be used uninitialized */
     NBDReply reply;
-    ssize_t ret;
+    int ret;
     int flags;
 
     TRACE("Reading request.");
-- 
2.11.1

Re: [Qemu-devel] [PATCH 02/19] nbd/server: get rid of ssize_t

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 758 bytes --]

On 05/30/2017 09:30 AM, Vladimir Sementsov-Ogievskiy wrote:
> Now read_sync and friends return int, so get rid of ssize_t.

In particular, it was your earlier commit "nbd: read_sync and friends:
return 0 on success" that changed the semantics of the return value to
just be 0 instead of a size; and this is just a followup cleanup (now
that we aren't returning a size, ssize_t is overkill).

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  nbd/server.c | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH 03/19] nbd/server: refactor nbd_co_send_reply

[Vladimir Sementsov-Ogievskiy]

As write_sync never returns value > 0, we can get rid of extra ret.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 nbd/server.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 03880fd501..e18eb020a7 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -972,7 +972,7 @@ void nbd_export_close_all(void)
 static int nbd_co_send_reply(NBDRequestData *req, NBDReply *reply, int len)
 {
     NBDClient *client = req->client;
-    int rc, ret;
+    int rc;
 
     g_assert(qemu_in_coroutine());
     qemu_co_mutex_lock(&client->send_lock);
@@ -983,9 +983,9 @@ static int nbd_co_send_reply(NBDRequestData *req, NBDReply *reply, int len)
     } else {
         qio_channel_set_cork(client->ioc, true);
         rc = nbd_send_reply(client->ioc, reply);
-        if (rc >= 0) {
-            ret = write_sync(client->ioc, req->data, len, NULL);
-            if (ret < 0) {
+        if (rc == 0) {
+            rc = write_sync(client->ioc, req->data, len, NULL);
+            if (rc < 0) {
                 rc = -EIO;
             }
         }
-- 
2.11.1

Re: [Qemu-devel] [PATCH 03/19] nbd/server: refactor nbd_co_send_reply

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 478 bytes --]

On 05/30/2017 09:30 AM, Vladimir Sementsov-Ogievskiy wrote:
> As write_sync never returns value > 0, we can get rid of extra ret.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  nbd/server.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH 04/19] nbd/server: get rid of EAGAIN dead code

[Vladimir Sementsov-Ogievskiy]

For now read_sync never returns EAGAIN. So, don't handle it.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 nbd/server.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index e18eb020a7..fcf35f0270 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -997,11 +997,12 @@ static int nbd_co_send_reply(NBDRequestData *req, NBDReply *reply, int len)
     return rc;
 }
 
-/* Collect a client request.  Return 0 if request looks valid, -EAGAIN
- * to keep trying the collection, -EIO to drop connection right away,
- * and any other negative value to report an error to the client
- * (although the caller may still need to disconnect after reporting
- * the error).  */
+/* nbd_co_receive_request
+ * Collect a client request. Return 0 if request looks valid, -EIO to drop
+ * connection right away, and any other negative value to report an error to
+ * the client (although the caller may still need to disconnect after reporting
+ * the error).
+ */
 static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request)
 {
     NBDClient *client = req->client;
@@ -1011,9 +1012,7 @@ static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request)
     assert(client->recv_coroutine == qemu_coroutine_self());
     rc = nbd_receive_request(client->ioc, request);
     if (rc < 0) {
-        if (rc != -EAGAIN) {
-            rc = -EIO;
-        }
+        rc = -EIO;
         goto out;
     }
 
@@ -1114,9 +1113,6 @@ static coroutine_fn void nbd_trip(void *opaque)
 
     req = nbd_request_get(client);
     ret = nbd_co_receive_request(req, &request);
-    if (ret == -EAGAIN) {
-        goto done;
-    }
     if (ret == -EIO) {
         goto out;
     }
-- 
2.11.1

Re: [Qemu-devel] [PATCH 04/19] nbd/server: get rid of EAGAIN dead code

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 692 bytes --]

On 05/30/2017 09:30 AM, Vladimir Sementsov-Ogievskiy wrote:
> For now read_sync never returns EAGAIN. So, don't handle it.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  nbd/server.c | 18 +++++++-----------
>  1 file changed, 7 insertions(+), 11 deletions(-)
> 

I had to go re-read your cover letter, to learn that EAGAIN went away in
your prerequisite series (I see Paolo has a pending PULL request with
your prerequisite in place, but it's not yet on master).

Reviewed-by: Eric Blake <eblake@redhat.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH 05/19] nbd/server: refactor nbd_co_receive_request

[Vladimir Sementsov-Ogievskiy]

Move function tail, about receiving next request out of the function.
Error path is simplified and nbd_co_receive_request becomes more
corresponding to its name.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 nbd/server.c | 41 +++++++++++++----------------------------
 1 file changed, 13 insertions(+), 28 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index fcf35f0270..546bf3ae61 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1006,14 +1006,11 @@ static int nbd_co_send_reply(NBDRequestData *req, NBDReply *reply, int len)
 static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request)
 {
     NBDClient *client = req->client;
-    int rc;
 
     g_assert(qemu_in_coroutine());
     assert(client->recv_coroutine == qemu_coroutine_self());
-    rc = nbd_receive_request(client->ioc, request);
-    if (rc < 0) {
-        rc = -EIO;
-        goto out;
+    if (nbd_receive_request(client->ioc, request) < 0) {
+        return -EIO;
     }
 
     TRACE("Decoding type");
@@ -1027,8 +1024,7 @@ static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request)
         /* Special case: we're going to disconnect without a reply,
          * whether or not flags, from, or len are bogus */
         TRACE("Request type is DISCONNECT");
-        rc = -EIO;
-        goto out;
+        return -EIO;
     }
 
     /* Check for sanity in the parameters, part 1.  Defer as many
@@ -1036,22 +1032,19 @@ static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request)
      * payload, so we can try and keep the connection alive.  */
     if ((request->from + request->len) < request->from) {
         LOG("integer overflow detected, you're probably being attacked");
-        rc = -EINVAL;
-        goto out;
+        return -EINVAL;
     }
 
     if (request->type == NBD_CMD_READ || request->type == NBD_CMD_WRITE) {
         if (request->len > NBD_MAX_BUFFER_SIZE) {
             LOG("len (%" PRIu32" ) is larger than max len (%u)",
                 request->len, NBD_MAX_BUFFER_SIZE);
-            rc = -EINVAL;
-            goto out;
+            return -EINVAL;
         }
 
         req->data = blk_try_blockalign(client->exp->blk, request->len);
         if (req->data == NULL) {
-            rc = -ENOMEM;
-            goto out;
+            return -ENOMEM;
         }
     }
     if (request->type == NBD_CMD_WRITE) {
@@ -1059,8 +1052,7 @@ static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request)
 
         if (read_sync(client->ioc, req->data, request->len, NULL) < 0) {
             LOG("reading from socket failed");
-            rc = -EIO;
-            goto out;
+            return -EIO;
         }
         req->complete = true;
     }
@@ -1070,28 +1062,19 @@ static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request)
         LOG("operation past EOF; From: %" PRIu64 ", Len: %" PRIu32
             ", Size: %" PRIu64, request->from, request->len,
             (uint64_t)client->exp->size);
-        rc = request->type == NBD_CMD_WRITE ? -ENOSPC : -EINVAL;
-        goto out;
+        return request->type == NBD_CMD_WRITE ? -ENOSPC : -EINVAL;
     }
     if (request->flags & ~(NBD_CMD_FLAG_FUA | NBD_CMD_FLAG_NO_HOLE)) {
         LOG("unsupported flags (got 0x%x)", request->flags);
-        rc = -EINVAL;
-        goto out;
+        return -EINVAL;
     }
     if (request->type != NBD_CMD_WRITE_ZEROES &&
         (request->flags & NBD_CMD_FLAG_NO_HOLE)) {
         LOG("unexpected flags (got 0x%x)", request->flags);
-        rc = -EINVAL;
-        goto out;
+        return -EINVAL;
     }
 
-    rc = 0;
-
-out:
-    client->recv_coroutine = NULL;
-    nbd_client_receive_next_request(client);
-
-    return rc;
+    return 0;
 }
 
 /* Owns a reference to the NBDClient passed as opaque.  */
@@ -1113,6 +1096,8 @@ static coroutine_fn void nbd_trip(void *opaque)
 
     req = nbd_request_get(client);
     ret = nbd_co_receive_request(req, &request);
+    client->recv_coroutine = NULL;
+    nbd_client_receive_next_request(client);
     if (ret == -EIO) {
         goto out;
     }
-- 
2.11.1

Re: [Qemu-devel] [PATCH 05/19] nbd/server: refactor nbd_co_receive_request

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 637 bytes --]

On 05/30/2017 09:30 AM, Vladimir Sementsov-Ogievskiy wrote:
> Move function tail, about receiving next request out of the function.
> Error path is simplified and nbd_co_receive_request becomes more
> corresponding to its name.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  nbd/server.c | 41 +++++++++++++----------------------------
>  1 file changed, 13 insertions(+), 28 deletions(-)

Quite the diffstat.

Reviewed-by: Eric Blake <eblake@redhat.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH 06/19] nbd/server: remove NBDClientNewData

[Vladimir Sementsov-Ogievskiy]

"co" field of NBDClientNewData is unused, so let's just use client
pointer instead of extra structure.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 nbd/server.c | 25 +++++++------------------
 1 file changed, 7 insertions(+), 18 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 546bf3ae61..3d4cd3d21c 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -535,14 +535,8 @@ static int nbd_negotiate_options(NBDClient *client)
     }
 }
 
-typedef struct {
-    NBDClient *client;
-    Coroutine *co;
-} NBDClientNewData;
-
-static coroutine_fn int nbd_negotiate(NBDClientNewData *data)
+static coroutine_fn int nbd_negotiate(NBDClient *client)
 {
-    NBDClient *client = data->client;
     char buf[8 + 8 + 8 + 128];
     int rc;
     const uint16_t myflags = (NBD_FLAG_HAS_FLAGS | NBD_FLAG_SEND_TRIM |
@@ -1268,16 +1262,15 @@ static void nbd_client_receive_next_request(NBDClient *client)
 
 static coroutine_fn void nbd_co_client_start(void *opaque)
 {
-    NBDClientNewData *data = opaque;
-    NBDClient *client = data->client;
+    NBDClient *client = opaque;
     NBDExport *exp = client->exp;
 
     if (exp) {
         nbd_export_get(exp);
     }
-    if (nbd_negotiate(data)) {
+    if (nbd_negotiate(client)) {
         client_close(client);
-        goto out;
+        return;
     }
     qemu_co_mutex_init(&client->send_lock);
 
@@ -1286,9 +1279,6 @@ static coroutine_fn void nbd_co_client_start(void *opaque)
     }
 
     nbd_client_receive_next_request(client);
-
-out:
-    g_free(data);
 }
 
 void nbd_client_new(NBDExport *exp,
@@ -1298,7 +1288,7 @@ void nbd_client_new(NBDExport *exp,
                     void (*close_fn)(NBDClient *))
 {
     NBDClient *client;
-    NBDClientNewData *data = g_new(NBDClientNewData, 1);
+    Coroutine *co;
 
     client = g_malloc0(sizeof(NBDClient));
     client->refcount = 1;
@@ -1314,7 +1304,6 @@ void nbd_client_new(NBDExport *exp,
     object_ref(OBJECT(client->ioc));
     client->close = close_fn;
 
-    data->client = client;
-    data->co = qemu_coroutine_create(nbd_co_client_start, data);
-    qemu_coroutine_enter(data->co);
+    co = qemu_coroutine_create(nbd_co_client_start, client);
+    qemu_coroutine_enter(co);
 }
-- 
2.11.1

Re: [Qemu-devel] [PATCH 06/19] nbd/server: remove NBDClientNewData

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 676 bytes --]

On 05/30/2017 09:30 AM, Vladimir Sementsov-Ogievskiy wrote:
> "co" field of NBDClientNewData is unused, so let's just use client
> pointer instead of extra structure.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  nbd/server.c | 25 +++++++------------------
>  1 file changed, 7 insertions(+), 18 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

Might be worth mentioning in the commit message that it has never been
used, all the way back to its declaration in commit 1a6245a5.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH 07/19] nbd/server: nbd_negotiate: fix error path

[Vladimir Sementsov-Ogievskiy]

Current code will return 0 on this write_sync fail, as rc is 0
after successful nbd_negotiate_options. Fix this.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 nbd/server.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/nbd/server.c b/nbd/server.c
index 3d4cd3d21c..984fad4bdb 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -607,7 +607,8 @@ static coroutine_fn int nbd_negotiate(NBDClient *client)
         stq_be_p(buf + 18, client->exp->size);
         stw_be_p(buf + 26, client->exp->nbdflags | myflags);
         len = client->no_zeroes ? 10 : sizeof(buf) - 18;
-        if (write_sync(client->ioc, buf + 18, len, NULL) < 0) {
+        rc = write_sync(client->ioc, buf + 18, len, NULL);
+        if (rc < 0) {
             LOG("write failed");
             goto fail;
         }
-- 
2.11.1

Re: [Qemu-devel] [PATCH 07/19] nbd/server: nbd_negotiate: fix error path

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1149 bytes --]

On 05/30/2017 09:30 AM, Vladimir Sementsov-Ogievskiy wrote:
> Current code will return 0 on this write_sync fail, as rc is 0
> after successful nbd_negotiate_options. Fix this.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  nbd/server.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

> diff --git a/nbd/server.c b/nbd/server.c
> index 3d4cd3d21c..984fad4bdb 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -607,7 +607,8 @@ static coroutine_fn int nbd_negotiate(NBDClient *client)
>          stq_be_p(buf + 18, client->exp->size);
>          stw_be_p(buf + 26, client->exp->nbdflags | myflags);
>          len = client->no_zeroes ? 10 : sizeof(buf) - 18;
> -        if (write_sync(client->ioc, buf + 18, len, NULL) < 0) {
> +        rc = write_sync(client->ioc, buf + 18, len, NULL);
> +        if (rc < 0) {
>              LOG("write failed");
>              goto fail;
>          }
> 

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH 08/19] nbd/server: get rid of fail: return rc

[Vladimir Sementsov-Ogievskiy]

"goto fail" error handling scheme is not needed for just returning
error code. Better is return it immediately.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 nbd/server.c | 28 ++++++++++++----------------
 1 file changed, 12 insertions(+), 16 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 984fad4bdb..4e9d33d83c 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -265,7 +265,6 @@ static int nbd_negotiate_handle_list(NBDClient *client, uint32_t length)
 
 static int nbd_negotiate_handle_export_name(NBDClient *client, uint32_t length)
 {
-    int rc = -EINVAL;
     char name[NBD_MAX_NAME_SIZE + 1];
 
     /* Client sends:
@@ -274,11 +273,11 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, uint32_t length)
     TRACE("Checking length");
     if (length >= sizeof(name)) {
         LOG("Bad length received");
-        goto fail;
+        return -EINVAL;
     }
     if (read_sync(client->ioc, name, length, NULL) < 0) {
         LOG("read failed");
-        goto fail;
+        return -EINVAL;
     }
     name[length] = '\0';
 
@@ -287,14 +286,13 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, uint32_t length)
     client->exp = nbd_export_find(name);
     if (!client->exp) {
         LOG("export not found");
-        goto fail;
+        return -EINVAL;
     }
 
     QTAILQ_INSERT_TAIL(&client->exp->clients, client, next);
     nbd_export_get(client->exp);
-    rc = 0;
-fail:
-    return rc;
+
+    return 0;
 }
 
 /* Handle NBD_OPT_STARTTLS. Return NULL to drop connection, or else the
@@ -564,7 +562,6 @@ static coroutine_fn int nbd_negotiate(NBDClient *client)
      */
 
     qio_channel_set_blocking(client->ioc, false, NULL);
-    rc = -EINVAL;
 
     TRACE("Beginning negotiation.");
     memset(buf, 0, sizeof(buf));
@@ -585,21 +582,21 @@ static coroutine_fn int nbd_negotiate(NBDClient *client)
     if (oldStyle) {
         if (client->tlscreds) {
             TRACE("TLS cannot be enabled with oldstyle protocol");
-            goto fail;
+            return -EINVAL;
         }
         if (write_sync(client->ioc, buf, sizeof(buf), NULL) < 0) {
             LOG("write failed");
-            goto fail;
+            return -EINVAL;
         }
     } else {
         if (write_sync(client->ioc, buf, 18, NULL) < 0) {
             LOG("write failed");
-            goto fail;
+            return -EINVAL;
         }
         rc = nbd_negotiate_options(client);
         if (rc != 0) {
             LOG("option negotiation failed");
-            goto fail;
+            return rc;
         }
 
         TRACE("advertising size %" PRIu64 " and flags %x",
@@ -610,14 +607,13 @@ static coroutine_fn int nbd_negotiate(NBDClient *client)
         rc = write_sync(client->ioc, buf + 18, len, NULL);
         if (rc < 0) {
             LOG("write failed");
-            goto fail;
+            return rc;
         }
     }
 
     TRACE("Negotiation succeeded.");
-    rc = 0;
-fail:
-    return rc;
+
+    return 0;
 }
 
 static int nbd_receive_request(QIOChannel *ioc, NBDRequest *request)
-- 
2.11.1

Re: [Qemu-devel] [PATCH 08/19] nbd/server: get rid of fail: return rc

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 552 bytes --]

On 05/30/2017 09:30 AM, Vladimir Sementsov-Ogievskiy wrote:
> "goto fail" error handling scheme is not needed for just returning
> error code. Better is return it immediately.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  nbd/server.c | 28 ++++++++++++----------------
>  1 file changed, 12 insertions(+), 16 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH 09/19] nbd/server: rename rc to ret

[Vladimir Sementsov-Ogievskiy]

For consistency use 'ret' name for saving return code everywhere
in the file.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 nbd/server.c | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 4e9d33d83c..a76267bea7 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -211,15 +211,15 @@ static int nbd_negotiate_send_rep_list(QIOChannel *ioc, NBDExport *exp)
     uint32_t len;
     const char *name = exp->name ? exp->name : "";
     const char *desc = exp->description ? exp->description : "";
-    int rc;
+    int ret;
 
     TRACE("Advertising export name '%s' description '%s'", name, desc);
     name_len = strlen(name);
     desc_len = strlen(desc);
     len = name_len + desc_len + sizeof(len);
-    rc = nbd_negotiate_send_rep_len(ioc, NBD_REP_SERVER, NBD_OPT_LIST, len);
-    if (rc < 0) {
-        return rc;
+    ret = nbd_negotiate_send_rep_len(ioc, NBD_REP_SERVER, NBD_OPT_LIST, len);
+    if (ret < 0) {
+        return ret;
     }
 
     len = cpu_to_be32(name_len);
@@ -536,7 +536,7 @@ static int nbd_negotiate_options(NBDClient *client)
 static coroutine_fn int nbd_negotiate(NBDClient *client)
 {
     char buf[8 + 8 + 8 + 128];
-    int rc;
+    int ret;
     const uint16_t myflags = (NBD_FLAG_HAS_FLAGS | NBD_FLAG_SEND_TRIM |
                               NBD_FLAG_SEND_FLUSH | NBD_FLAG_SEND_FUA |
                               NBD_FLAG_SEND_WRITE_ZEROES);
@@ -593,10 +593,10 @@ static coroutine_fn int nbd_negotiate(NBDClient *client)
             LOG("write failed");
             return -EINVAL;
         }
-        rc = nbd_negotiate_options(client);
-        if (rc != 0) {
+        ret = nbd_negotiate_options(client);
+        if (ret != 0) {
             LOG("option negotiation failed");
-            return rc;
+            return ret;
         }
 
         TRACE("advertising size %" PRIu64 " and flags %x",
@@ -604,10 +604,10 @@ static coroutine_fn int nbd_negotiate(NBDClient *client)
         stq_be_p(buf + 18, client->exp->size);
         stw_be_p(buf + 26, client->exp->nbdflags | myflags);
         len = client->no_zeroes ? 10 : sizeof(buf) - 18;
-        rc = write_sync(client->ioc, buf + 18, len, NULL);
-        if (rc < 0) {
+        ret = write_sync(client->ioc, buf + 18, len, NULL);
+        if (ret < 0) {
             LOG("write failed");
-            return rc;
+            return ret;
         }
     }
 
@@ -963,21 +963,21 @@ void nbd_export_close_all(void)
 static int nbd_co_send_reply(NBDRequestData *req, NBDReply *reply, int len)
 {
     NBDClient *client = req->client;
-    int rc;
+    int ret;
 
     g_assert(qemu_in_coroutine());
     qemu_co_mutex_lock(&client->send_lock);
     client->send_coroutine = qemu_coroutine_self();
 
     if (!len) {
-        rc = nbd_send_reply(client->ioc, reply);
+        ret = nbd_send_reply(client->ioc, reply);
     } else {
         qio_channel_set_cork(client->ioc, true);
-        rc = nbd_send_reply(client->ioc, reply);
-        if (rc == 0) {
-            rc = write_sync(client->ioc, req->data, len, NULL);
-            if (rc < 0) {
-                rc = -EIO;
+        ret = nbd_send_reply(client->ioc, reply);
+        if (ret == 0) {
+            ret = write_sync(client->ioc, req->data, len, NULL);
+            if (ret < 0) {
+                ret = -EIO;
             }
         }
         qio_channel_set_cork(client->ioc, false);
@@ -985,7 +985,7 @@ static int nbd_co_send_reply(NBDRequestData *req, NBDReply *reply, int len)
 
     client->send_coroutine = NULL;
     qemu_co_mutex_unlock(&client->send_lock);
-    return rc;
+    return ret;
 }
 
 /* nbd_co_receive_request
-- 
2.11.1

Re: [Qemu-devel] [PATCH 09/19] nbd/server: rename rc to ret

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 528 bytes --]

On 05/30/2017 09:30 AM, Vladimir Sementsov-Ogievskiy wrote:
> For consistency use 'ret' name for saving return code everywhere
> in the file.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  nbd/server.c | 38 +++++++++++++++++++-------------------
>  1 file changed, 19 insertions(+), 19 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH 10/19] nbd/server: refactor nbd_trip

[Vladimir Sementsov-Ogievskiy]

- do not goto into switch block from outer block
- reduce code duplications

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 nbd/server.c | 53 ++++++++++++++++++++---------------------------------
 1 file changed, 20 insertions(+), 33 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index a76267bea7..a47f13e4fb 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1078,6 +1078,7 @@ static coroutine_fn void nbd_trip(void *opaque)
     NBDReply reply;
     int ret;
     int flags;
+    int reply_data_len = 0;
 
     TRACE("Reading request.");
     if (client->closing) {
@@ -1090,7 +1091,7 @@ static coroutine_fn void nbd_trip(void *opaque)
     client->recv_coroutine = NULL;
     nbd_client_receive_next_request(client);
     if (ret == -EIO) {
-        goto out;
+        goto disconnect;
     }
 
     reply.handle = request.handle;
@@ -1098,7 +1099,7 @@ static coroutine_fn void nbd_trip(void *opaque)
 
     if (ret < 0) {
         reply.error = -ret;
-        goto error_reply;
+        goto reply;
     }
 
     if (client->closing) {
@@ -1119,7 +1120,7 @@ static coroutine_fn void nbd_trip(void *opaque)
             if (ret < 0) {
                 LOG("flush failed");
                 reply.error = -ret;
-                goto error_reply;
+                break;
             }
         }
 
@@ -1128,12 +1129,12 @@ static coroutine_fn void nbd_trip(void *opaque)
         if (ret < 0) {
             LOG("reading from file failed");
             reply.error = -ret;
-            goto error_reply;
+            break;
         }
 
+        reply_data_len = request.len;
         TRACE("Read %" PRIu32" byte(s)", request.len);
-        if (nbd_co_send_reply(req, &reply, request.len) < 0)
-            goto out;
+
         break;
     case NBD_CMD_WRITE:
         TRACE("Request type is WRITE");
@@ -1141,7 +1142,7 @@ static coroutine_fn void nbd_trip(void *opaque)
         if (exp->nbdflags & NBD_FLAG_READ_ONLY) {
             TRACE("Server is read-only, return error");
             reply.error = EROFS;
-            goto error_reply;
+            break;
         }
 
         TRACE("Writing to device");
@@ -1155,21 +1156,16 @@ static coroutine_fn void nbd_trip(void *opaque)
         if (ret < 0) {
             LOG("writing to file failed");
             reply.error = -ret;
-            goto error_reply;
         }
 
-        if (nbd_co_send_reply(req, &reply, 0) < 0) {
-            goto out;
-        }
         break;
-
     case NBD_CMD_WRITE_ZEROES:
         TRACE("Request type is WRITE_ZEROES");
 
         if (exp->nbdflags & NBD_FLAG_READ_ONLY) {
             TRACE("Server is read-only, return error");
             reply.error = EROFS;
-            goto error_reply;
+            break;
         }
 
         TRACE("Writing to device");
@@ -1186,14 +1182,9 @@ static coroutine_fn void nbd_trip(void *opaque)
         if (ret < 0) {
             LOG("writing to file failed");
             reply.error = -ret;
-            goto error_reply;
         }
 
-        if (nbd_co_send_reply(req, &reply, 0) < 0) {
-            goto out;
-        }
         break;
-
     case NBD_CMD_DISC:
         /* unreachable, thanks to special case in nbd_co_receive_request() */
         abort();
@@ -1206,9 +1197,7 @@ static coroutine_fn void nbd_trip(void *opaque)
             LOG("flush failed");
             reply.error = -ret;
         }
-        if (nbd_co_send_reply(req, &reply, 0) < 0) {
-            goto out;
-        }
+
         break;
     case NBD_CMD_TRIM:
         TRACE("Request type is TRIM");
@@ -1218,21 +1207,19 @@ static coroutine_fn void nbd_trip(void *opaque)
             LOG("discard failed");
             reply.error = -ret;
         }
-        if (nbd_co_send_reply(req, &reply, 0) < 0) {
-            goto out;
-        }
+
         break;
     default:
         LOG("invalid request type (%" PRIu32 ") received", request.type);
         reply.error = EINVAL;
-    error_reply:
-        /* We must disconnect after NBD_CMD_WRITE if we did not
-         * read the payload.
-         */
-        if (nbd_co_send_reply(req, &reply, 0) < 0 || !req->complete) {
-            goto out;
-        }
-        break;
+    }
+
+reply:
+    /* We must disconnect after NBD_CMD_WRITE if we did not
+     * read the payload.
+     */
+    if (nbd_co_send_reply(req, &reply, reply_data_len) < 0 || !req->complete) {
+        goto disconnect;
     }
 
     TRACE("Request/Reply complete");
@@ -1242,7 +1229,7 @@ done:
     nbd_client_put(client);
     return;
 
-out:
+disconnect:
     nbd_request_put(req);
     client_close(client);
     nbd_client_put(client);
-- 
2.11.1

Re: [Qemu-devel] [PATCH 10/19] nbd/server: refactor nbd_trip

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1352 bytes --]

On 05/30/2017 09:30 AM, Vladimir Sementsov-Ogievskiy wrote:
> - do not goto into switch block from outer block

This sentence didn't quite make sense.  Better might be:

- do not use 'goto error_reply' outside a switch to jump into the middle
of the switch's default case label

> - reduce code duplications

s/duplications/duplication/

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  nbd/server.c | 53 ++++++++++++++++++++---------------------------------
>  1 file changed, 20 insertions(+), 33 deletions(-)
> 

> @@ -1098,7 +1099,7 @@ static coroutine_fn void nbd_trip(void *opaque)
>  
>      if (ret < 0) {
>          reply.error = -ret;
> -        goto error_reply;
> +        goto reply;
>      }

But this was indeed a case of jumping from before the switch()...

>      default:
>          LOG("invalid request type (%" PRIu32 ") received", request.type);
>          reply.error = EINVAL;
> -    error_reply:
> -        /* We must disconnect after NBD_CMD_WRITE if we did not
> -         * read the payload.
> -         */

...into the middle, which, although valid C, is indeed unusual.

Reviewed-by: Eric Blake <eblake@redhat.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH 11/19] io/channel-socket: qio_channel_socket_writev handle EPIPE

[Vladimir Sementsov-Ogievskiy]

Return QIO_CHANNEL_ERR_EPIPE on EPIPE, we will need it to improve error
path in nbd server.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/io/channel.h | 1 +
 io/channel-socket.c  | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/io/channel.h b/include/io/channel.h
index 5d48906998..5529c2da31 100644
--- a/include/io/channel.h
+++ b/include/io/channel.h
@@ -38,6 +38,7 @@ typedef struct QIOChannel QIOChannel;
 typedef struct QIOChannelClass QIOChannelClass;
 
 #define QIO_CHANNEL_ERR_BLOCK -2
+#define QIO_CHANNEL_ERR_EPIPE -3
 
 typedef enum QIOChannelFeature QIOChannelFeature;
 
diff --git a/io/channel-socket.c b/io/channel-socket.c
index 53386b7ba3..50f9f966c6 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -542,7 +542,7 @@ static ssize_t qio_channel_socket_writev(QIOChannel *ioc,
         }
         error_setg_errno(errp, errno,
                          "Unable to write to socket");
-        return -1;
+        return errno == EPIPE ? QIO_CHANNEL_ERR_EPIPE : -1;
     }
     return ret;
 }
-- 
2.11.1

Re: [Qemu-devel] [PATCH 11/19] io/channel-socket: qio_channel_socket_writev handle EPIPE

[Daniel P. Berrange]

On Tue, May 30, 2017 at 05:30:44PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Return QIO_CHANNEL_ERR_EPIPE on EPIPE, we will need it to improve error
> path in nbd server.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  include/io/channel.h | 1 +
>  io/channel-socket.c  | 2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/include/io/channel.h b/include/io/channel.h
> index 5d48906998..5529c2da31 100644
> --- a/include/io/channel.h
> +++ b/include/io/channel.h
> @@ -38,6 +38,7 @@ typedef struct QIOChannel QIOChannel;
>  typedef struct QIOChannelClass QIOChannelClass;
>  
>  #define QIO_CHANNEL_ERR_BLOCK -2
> +#define QIO_CHANNEL_ERR_EPIPE -3
>  
>  typedef enum QIOChannelFeature QIOChannelFeature;
>  
> diff --git a/io/channel-socket.c b/io/channel-socket.c
> index 53386b7ba3..50f9f966c6 100644
> --- a/io/channel-socket.c
> +++ b/io/channel-socket.c
> @@ -542,7 +542,7 @@ static ssize_t qio_channel_socket_writev(QIOChannel *ioc,
>          }
>          error_setg_errno(errp, errno,
>                           "Unable to write to socket");
> -        return -1;
> +        return errno == EPIPE ? QIO_CHANNEL_ERR_EPIPE : -1;
>      }
>      return ret;

Ewwww, no. We don't want to go down the road of special casing
further errno values for every error scenario. 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 11/19] io/channel-socket: qio_channel_socket_writev handle EPIPE

[Vladimir Sementsov-Ogievskiy]

30.05.2017 18:04, Daniel P. Berrange wrote:
> On Tue, May 30, 2017 at 05:30:44PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> Return QIO_CHANNEL_ERR_EPIPE on EPIPE, we will need it to improve error
>> path in nbd server.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   include/io/channel.h | 1 +
>>   io/channel-socket.c  | 2 +-
>>   2 files changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/io/channel.h b/include/io/channel.h
>> index 5d48906998..5529c2da31 100644
>> --- a/include/io/channel.h
>> +++ b/include/io/channel.h
>> @@ -38,6 +38,7 @@ typedef struct QIOChannel QIOChannel;
>>   typedef struct QIOChannelClass QIOChannelClass;
>>   
>>   #define QIO_CHANNEL_ERR_BLOCK -2
>> +#define QIO_CHANNEL_ERR_EPIPE -3
>>   
>>   typedef enum QIOChannelFeature QIOChannelFeature;
>>   
>> diff --git a/io/channel-socket.c b/io/channel-socket.c
>> index 53386b7ba3..50f9f966c6 100644
>> --- a/io/channel-socket.c
>> +++ b/io/channel-socket.c
>> @@ -542,7 +542,7 @@ static ssize_t qio_channel_socket_writev(QIOChannel *ioc,
>>           }
>>           error_setg_errno(errp, errno,
>>                            "Unable to write to socket");
>> -        return -1;
>> +        return errno == EPIPE ? QIO_CHANNEL_ERR_EPIPE : -1;
>>       }
>>       return ret;
> Ewwww, no. We don't want to go down the road of special casing
> further errno values for every error scenario.

I need to distinguish client socket closed without reading nbd server 
reply from other errors, because this is a valid behavior accordingly to 
the protocol.
Any way to do this? Hmm, I can examine the contents of errp, of course, 
but I'm afraid it would be even less appropriate. This is funny: user 
has this information in errp message, but the code - doesn't..

Alternatively:
1. not report errors on sending reply after OPT_ABORT at all
2. report all errors, (including this EPIPE, which actually is not a 
failure)

Paolo, what do you think?

(current behavior is (1))

>
> Regards,
> Daniel


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 11/19] io/channel-socket: qio_channel_socket_writev handle EPIPE

[Daniel P. Berrange]

On Tue, May 30, 2017 at 06:15:07PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> 30.05.2017 18:04, Daniel P. Berrange wrote:
> > On Tue, May 30, 2017 at 05:30:44PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > > Return QIO_CHANNEL_ERR_EPIPE on EPIPE, we will need it to improve error
> > > path in nbd server.
> > > 
> > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > > ---
> > >   include/io/channel.h | 1 +
> > >   io/channel-socket.c  | 2 +-
> > >   2 files changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/include/io/channel.h b/include/io/channel.h
> > > index 5d48906998..5529c2da31 100644
> > > --- a/include/io/channel.h
> > > +++ b/include/io/channel.h
> > > @@ -38,6 +38,7 @@ typedef struct QIOChannel QIOChannel;
> > >   typedef struct QIOChannelClass QIOChannelClass;
> > >   #define QIO_CHANNEL_ERR_BLOCK -2
> > > +#define QIO_CHANNEL_ERR_EPIPE -3
> > >   typedef enum QIOChannelFeature QIOChannelFeature;
> > > diff --git a/io/channel-socket.c b/io/channel-socket.c
> > > index 53386b7ba3..50f9f966c6 100644
> > > --- a/io/channel-socket.c
> > > +++ b/io/channel-socket.c
> > > @@ -542,7 +542,7 @@ static ssize_t qio_channel_socket_writev(QIOChannel *ioc,
> > >           }
> > >           error_setg_errno(errp, errno,
> > >                            "Unable to write to socket");
> > > -        return -1;
> > > +        return errno == EPIPE ? QIO_CHANNEL_ERR_EPIPE : -1;
> > >       }
> > >       return ret;
> > Ewwww, no. We don't want to go down the road of special casing
> > further errno values for every error scenario.
> 
> I need to distinguish client socket closed without reading nbd server reply
> from other errors, because this is a valid behavior accordingly to the
> protocol.
> Any way to do this? Hmm, I can examine the contents of errp, of course, but
> I'm afraid it would be even less appropriate. This is funny: user has this
> information in errp message, but the code - doesn't..
> 
> Alternatively:
> 1. not report errors on sending reply after OPT_ABORT at all
> 2. report all errors, (including this EPIPE, which actually is not a
> failure)
> 
> Paolo, what do you think?
> 
> (current behavior is (1))

I'm not seeing any problem with continuing with (1). Once we've received
an OPT_ABORT msg from the client, we know it is going away, so no further
errors we get matter from that point onwards.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

[Qemu-devel] [PATCH 12/19] nbd/common: nbd_wr_syncv handle QIO_CHANNEL_ERR_EPIPE

[Vladimir Sementsov-Ogievskiy]

Return EPIPE in case of QIO_CHANNEL_ERR_EPIPE, we will need it to
improve error path in nbd server.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 nbd/common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/nbd/common.c b/nbd/common.c
index e520aae741..88e0297fb2 100644
--- a/nbd/common.c
+++ b/nbd/common.c
@@ -52,7 +52,7 @@ ssize_t nbd_wr_syncv(QIOChannel *ioc,
             continue;
         }
         if (len < 0) {
-            done = -EIO;
+            done = len == QIO_CHANNEL_ERR_EPIPE ? -EPIPE : -EIO;
             goto cleanup;
         }
 
-- 
2.11.1

Re: [Qemu-devel] [PATCH 12/19] nbd/common: nbd_wr_syncv handle QIO_CHANNEL_ERR_EPIPE

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1015 bytes --]

On 05/30/2017 09:30 AM, Vladimir Sementsov-Ogievskiy wrote:
> Return EPIPE in case of QIO_CHANNEL_ERR_EPIPE, we will need it to
> improve error path in nbd server.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  nbd/common.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

I agree with Daniel's assessment that we probably aren't doing things
right in the previous patch, and therefore probably don't need this one.

> 
> diff --git a/nbd/common.c b/nbd/common.c
> index e520aae741..88e0297fb2 100644
> --- a/nbd/common.c
> +++ b/nbd/common.c
> @@ -52,7 +52,7 @@ ssize_t nbd_wr_syncv(QIOChannel *ioc,
>              continue;
>          }
>          if (len < 0) {
> -            done = -EIO;
> +            done = len == QIO_CHANNEL_ERR_EPIPE ? -EPIPE : -EIO;
>              goto cleanup;
>          }
>  
> 

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH 13/19] nbd/server: return original error codes

[Vladimir Sementsov-Ogievskiy]

The code in many cases return -EINVAL or -EIO instead of original error
code from, for example, write_sync(). Following patch will need EPIPE
handling, so, let's refactor this where possible (the only exclusion
is nbd_co_receive_request, with own return-code convention)

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 nbd/server.c | 124 +++++++++++++++++++++++++++++++++++++----------------------
 1 file changed, 77 insertions(+), 47 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index a47f13e4fb..30dfb81a5c 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -136,30 +136,38 @@ static void nbd_client_receive_next_request(NBDClient *client);
 static int nbd_negotiate_send_rep_len(QIOChannel *ioc, uint32_t type,
                                       uint32_t opt, uint32_t len)
 {
+    int ret;
     uint64_t magic;
 
     TRACE("Reply opt=%" PRIx32 " type=%" PRIx32 " len=%" PRIu32,
           type, opt, len);
 
     magic = cpu_to_be64(NBD_REP_MAGIC);
-    if (write_sync(ioc, &magic, sizeof(magic), NULL) < 0) {
+    ret = write_sync(ioc, &magic, sizeof(magic), NULL);
+    if (ret < 0) {
         LOG("write failed (rep magic)");
-        return -EINVAL;
+        return ret;
     }
+
     opt = cpu_to_be32(opt);
-    if (write_sync(ioc, &opt, sizeof(opt), NULL) < 0) {
+    ret = write_sync(ioc, &opt, sizeof(opt), NULL);
+    if (ret < 0) {
         LOG("write failed (rep opt)");
-        return -EINVAL;
+        return ret;
     }
+
     type = cpu_to_be32(type);
-    if (write_sync(ioc, &type, sizeof(type), NULL) < 0) {
+    ret = write_sync(ioc, &type, sizeof(type), NULL);
+    if (ret < 0) {
         LOG("write failed (rep type)");
-        return -EINVAL;
+        return ret;
     }
+
     len = cpu_to_be32(len);
-    if (write_sync(ioc, &len, sizeof(len), NULL) < 0) {
+    ret = write_sync(ioc, &len, sizeof(len), NULL);
+    if (ret < 0) {
         LOG("write failed (rep data length)");
-        return -EINVAL;
+        return ret;
     }
     return 0;
 }
@@ -192,12 +200,12 @@ nbd_negotiate_send_rep_err(QIOChannel *ioc, uint32_t type,
     if (ret < 0) {
         goto out;
     }
-    if (write_sync(ioc, msg, len, NULL) < 0) {
+
+    ret = write_sync(ioc, msg, len, NULL);
+    if (ret < 0) {
         LOG("write failed (error message)");
-        ret = -EIO;
-    } else {
-        ret = 0;
     }
+
 out:
     g_free(msg);
     return ret;
@@ -223,18 +231,24 @@ static int nbd_negotiate_send_rep_list(QIOChannel *ioc, NBDExport *exp)
     }
 
     len = cpu_to_be32(name_len);
-    if (write_sync(ioc, &len, sizeof(len), NULL) < 0) {
+    ret = write_sync(ioc, &len, sizeof(len), NULL);
+    if (ret < 0) {
         LOG("write failed (name length)");
-        return -EINVAL;
+        return ret;
     }
-    if (write_sync(ioc, name, name_len, NULL) < 0) {
+
+    ret = write_sync(ioc, name, name_len, NULL);
+    if (ret < 0) {
         LOG("write failed (name buffer)");
-        return -EINVAL;
+        return ret;
     }
-    if (write_sync(ioc, desc, desc_len, NULL) < 0) {
+
+    ret = write_sync(ioc, desc, desc_len, NULL);
+    if (ret < 0) {
         LOG("write failed (description buffer)");
-        return -EINVAL;
+        return ret;
     }
+
     return 0;
 }
 
@@ -242,11 +256,13 @@ static int nbd_negotiate_send_rep_list(QIOChannel *ioc, NBDExport *exp)
  * Return -errno on error, 0 on success. */
 static int nbd_negotiate_handle_list(NBDClient *client, uint32_t length)
 {
+    int ret;
     NBDExport *exp;
 
     if (length) {
-        if (drop_sync(client->ioc, length, NULL) < 0) {
-            return -EIO;
+        ret = drop_sync(client->ioc, length, NULL);
+        if (ret < 0) {
+            return ret;
         }
         return nbd_negotiate_send_rep_err(client->ioc,
                                           NBD_REP_ERR_INVALID, NBD_OPT_LIST,
@@ -255,8 +271,9 @@ static int nbd_negotiate_handle_list(NBDClient *client, uint32_t length)
 
     /* For each export, send a NBD_REP_SERVER reply. */
     QTAILQ_FOREACH(exp, &exports, next) {
-        if (nbd_negotiate_send_rep_list(client->ioc, exp)) {
-            return -EINVAL;
+        ret = nbd_negotiate_send_rep_list(client->ioc, exp);
+        if (ret < 0) {
+            return ret;
         }
     }
     /* Finish with a NBD_REP_ACK. */
@@ -265,6 +282,7 @@ static int nbd_negotiate_handle_list(NBDClient *client, uint32_t length)
 
 static int nbd_negotiate_handle_export_name(NBDClient *client, uint32_t length)
 {
+    int ret;
     char name[NBD_MAX_NAME_SIZE + 1];
 
     /* Client sends:
@@ -275,9 +293,11 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, uint32_t length)
         LOG("Bad length received");
         return -EINVAL;
     }
-    if (read_sync(client->ioc, name, length, NULL) < 0) {
+
+    ret = read_sync(client->ioc, name, length, NULL);
+    if (ret < 0) {
         LOG("read failed");
-        return -EINVAL;
+        return ret;
     }
     name[length] = '\0';
 
@@ -354,6 +374,7 @@ static QIOChannel *nbd_negotiate_handle_starttls(NBDClient *client,
  * Return -errno on error, 0 on success. */
 static int nbd_negotiate_options(NBDClient *client)
 {
+    int ret;
     uint32_t flags;
     bool fixedNewstyle = false;
 
@@ -371,9 +392,10 @@ static int nbd_negotiate_options(NBDClient *client)
         ...           Rest of request
     */
 
-    if (read_sync(client->ioc, &flags, sizeof(flags), NULL) < 0) {
+    ret = read_sync(client->ioc, &flags, sizeof(flags), NULL);
+    if (ret < 0) {
         LOG("read failed");
-        return -EIO;
+        return ret;
     }
     TRACE("Checking client flags");
     be32_to_cpus(&flags);
@@ -397,9 +419,10 @@ static int nbd_negotiate_options(NBDClient *client)
         uint32_t clientflags, length;
         uint64_t magic;
 
-        if (read_sync(client->ioc, &magic, sizeof(magic), NULL) < 0) {
+        ret = read_sync(client->ioc, &magic, sizeof(magic), NULL);
+        if (ret < 0) {
             LOG("read failed");
-            return -EINVAL;
+            return ret;
         }
         TRACE("Checking opts magic");
         if (magic != be64_to_cpu(NBD_OPTS_MAGIC)) {
@@ -407,17 +430,17 @@ static int nbd_negotiate_options(NBDClient *client)
             return -EINVAL;
         }
 
-        if (read_sync(client->ioc, &clientflags,
-                      sizeof(clientflags), NULL) < 0)
-        {
+        ret = read_sync(client->ioc, &clientflags, sizeof(clientflags), NULL);
+        if (ret < 0) {
             LOG("read failed");
-            return -EINVAL;
+            return ret;
         }
         clientflags = be32_to_cpu(clientflags);
 
-        if (read_sync(client->ioc, &length, sizeof(length), NULL) < 0) {
+        ret = read_sync(client->ioc, &length, sizeof(length), NULL);
+        if (ret < 0) {
             LOG("read failed");
-            return -EINVAL;
+            return ret;
         }
         length = be32_to_cpu(length);
 
@@ -445,8 +468,9 @@ static int nbd_negotiate_options(NBDClient *client)
                 return -EINVAL;
 
             default:
-                if (drop_sync(client->ioc, length, NULL) < 0) {
-                    return -EIO;
+                ret = drop_sync(client->ioc, length, NULL);
+                if (ret < 0) {
+                    return ret;
                 }
                 ret = nbd_negotiate_send_rep_err(client->ioc,
                                                  NBD_REP_ERR_TLS_REQD,
@@ -476,15 +500,17 @@ static int nbd_negotiate_options(NBDClient *client)
                 /* NBD spec says we must try to reply before
                  * disconnecting, but that we must also tolerate
                  * guests that don't wait for our reply. */
-                nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK, clientflags);
-                return -EINVAL;
+                ret = nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK,
+                                             clientflags);
+                return ret < 0 ? ret : -EINVAL;
 
             case NBD_OPT_EXPORT_NAME:
                 return nbd_negotiate_handle_export_name(client, length);
 
             case NBD_OPT_STARTTLS:
-                if (drop_sync(client->ioc, length, NULL) < 0) {
-                    return -EIO;
+                ret = drop_sync(client->ioc, length, NULL);
+                if (ret < 0) {
+                    return ret;
                 }
                 if (client->tlscreds) {
                     ret = nbd_negotiate_send_rep_err(client->ioc,
@@ -502,8 +528,9 @@ static int nbd_negotiate_options(NBDClient *client)
                 }
                 break;
             default:
-                if (drop_sync(client->ioc, length, NULL) < 0) {
-                    return -EIO;
+                ret = drop_sync(client->ioc, length, NULL);
+                if (ret < 0) {
+                    return ret;
                 }
                 ret = nbd_negotiate_send_rep_err(client->ioc,
                                                  NBD_REP_ERR_UNSUP,
@@ -584,14 +611,17 @@ static coroutine_fn int nbd_negotiate(NBDClient *client)
             TRACE("TLS cannot be enabled with oldstyle protocol");
             return -EINVAL;
         }
-        if (write_sync(client->ioc, buf, sizeof(buf), NULL) < 0) {
+
+        ret = write_sync(client->ioc, buf, sizeof(buf), NULL);
+        if (ret < 0) {
             LOG("write failed");
-            return -EINVAL;
+            return ret;
         }
     } else {
-        if (write_sync(client->ioc, buf, 18, NULL) < 0) {
+        ret = write_sync(client->ioc, buf, 18, NULL);
+        if (ret < 0) {
             LOG("write failed");
-            return -EINVAL;
+            return ret;
         }
         ret = nbd_negotiate_options(client);
         if (ret != 0) {
@@ -977,7 +1007,7 @@ static int nbd_co_send_reply(NBDRequestData *req, NBDReply *reply, int len)
         if (ret == 0) {
             ret = write_sync(client->ioc, req->data, len, NULL);
             if (ret < 0) {
-                ret = -EIO;
+                ret = ret;
             }
         }
         qio_channel_set_cork(client->ioc, false);
-- 
2.11.1

Re: [Qemu-devel] [PATCH 13/19] nbd/server: return original error codes

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2025 bytes --]

On 05/30/2017 09:30 AM, Vladimir Sementsov-Ogievskiy wrote:
> The code in many cases return -EINVAL or -EIO instead of original error
> code from, for example, write_sync(). Following patch will need EPIPE
> handling, so, let's refactor this where possible (the only exclusion
> is nbd_co_receive_request, with own return-code convention)

Do we still want/need EPIPE handling, given the discussion on the
previous two patches?

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  nbd/server.c | 124 +++++++++++++++++++++++++++++++++++++----------------------
>  1 file changed, 77 insertions(+), 47 deletions(-)
> 

Feels weird to have a net gain in code, but maybe worthwhile.

> diff --git a/nbd/server.c b/nbd/server.c
> index a47f13e4fb..30dfb81a5c 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -136,30 +136,38 @@ static void nbd_client_receive_next_request(NBDClient *client);
>  static int nbd_negotiate_send_rep_len(QIOChannel *ioc, uint32_t type,
>                                        uint32_t opt, uint32_t len)
>  {
> +    int ret;
>      uint64_t magic;
>  
>      TRACE("Reply opt=%" PRIx32 " type=%" PRIx32 " len=%" PRIu32,
>            type, opt, len);
>  
>      magic = cpu_to_be64(NBD_REP_MAGIC);
> -    if (write_sync(ioc, &magic, sizeof(magic), NULL) < 0) {
> +    ret = write_sync(ioc, &magic, sizeof(magic), NULL);
> +    if (ret < 0) {
>          LOG("write failed (rep magic)");
> -        return -EINVAL;
> +        return ret;
>      }

Constructs like this should get shorter once we plumb errp all the way
through.  Okay, I can live with the temporary verbosity.

You may still have to make changes due to rebasing (in which case I'll
definitely want to review again); but if this patch doesn't need further
rework, you can add:
Reviewed-by: Eric Blake <eblake@redhat.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 13/19] nbd/server: return original error codes

[Vladimir Sementsov-Ogievskiy]

02.06.2017 01:29, Eric Blake wrote:
> On 05/30/2017 09:30 AM, Vladimir Sementsov-Ogievskiy wrote:
>> The code in many cases return -EINVAL or -EIO instead of original error
>> code from, for example, write_sync(). Following patch will need EPIPE
>> handling, so, let's refactor this where possible (the only exclusion
>> is nbd_co_receive_request, with own return-code convention)
> Do we still want/need EPIPE handling, given the discussion on the
> previous two patches?

Looks like this patch should be dropped. If EPIPE is not accepted 
(previous discussion), so error code is always EIO, no needs to save it 
into a variable..

>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   nbd/server.c | 124 +++++++++++++++++++++++++++++++++++++----------------------
>>   1 file changed, 77 insertions(+), 47 deletions(-)
>>
> Feels weird to have a net gain in code, but maybe worthwhile.
>
>> diff --git a/nbd/server.c b/nbd/server.c
>> index a47f13e4fb..30dfb81a5c 100644
>> --- a/nbd/server.c
>> +++ b/nbd/server.c
>> @@ -136,30 +136,38 @@ static void nbd_client_receive_next_request(NBDClient *client);
>>   static int nbd_negotiate_send_rep_len(QIOChannel *ioc, uint32_t type,
>>                                         uint32_t opt, uint32_t len)
>>   {
>> +    int ret;
>>       uint64_t magic;
>>   
>>       TRACE("Reply opt=%" PRIx32 " type=%" PRIx32 " len=%" PRIu32,
>>             type, opt, len);
>>   
>>       magic = cpu_to_be64(NBD_REP_MAGIC);
>> -    if (write_sync(ioc, &magic, sizeof(magic), NULL) < 0) {
>> +    ret = write_sync(ioc, &magic, sizeof(magic), NULL);
>> +    if (ret < 0) {
>>           LOG("write failed (rep magic)");
>> -        return -EINVAL;
>> +        return ret;
>>       }
> Constructs like this should get shorter once we plumb errp all the way
> through.  Okay, I can live with the temporary verbosity.
>
> You may still have to make changes due to rebasing (in which case I'll
> definitely want to review again); but if this patch doesn't need further
> rework, you can add:
> Reviewed-by: Eric Blake <eblake@redhat.com>
>

-- 
Best regards,
Vladimir

[Qemu-devel] [PATCH 14/19] nbd/server: nbd_negotiate: return 1 on NBD_OPT_ABORT

[Vladimir Sementsov-Ogievskiy]

Separate case when client sent NBD_OPT_ABORT from other errors.
It will be needed for the following patch, where errors will be
reported.
Considered case is not actually the error - it honestly follows NBD
protocol. Therefore it should not be reported like an error.
-EPIPE case means client not read server reply on NBD_OPT_ABORT,
which is also OK.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 nbd/server.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 30dfb81a5c..0e53d3dd91 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -369,9 +369,13 @@ static QIOChannel *nbd_negotiate_handle_starttls(NBDClient *client,
     return QIO_CHANNEL(tioc);
 }
 
-
-/* Process all NBD_OPT_* client option commands.
- * Return -errno on error, 0 on success. */
+/* nbd_negotiate_options
+ * Process all NBD_OPT_* client option commands.
+ * Return:
+ * < 0 on error
+ * 0   on successful negotiation
+ * 1   if client sent NBD_OPT_ABORT, i.e. on legal disconnect
+ */
 static int nbd_negotiate_options(NBDClient *client)
 {
     int ret;
@@ -483,7 +487,7 @@ static int nbd_negotiate_options(NBDClient *client)
                 }
                 /* Let the client keep trying, unless they asked to quit */
                 if (clientflags == NBD_OPT_ABORT) {
-                    return -EINVAL;
+                    return 1;
                 }
                 break;
             }
@@ -502,7 +506,7 @@ static int nbd_negotiate_options(NBDClient *client)
                  * guests that don't wait for our reply. */
                 ret = nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK,
                                              clientflags);
-                return ret < 0 ? ret : -EINVAL;
+                return ret < 0 && ret != -EPIPE ? ret : 1;
 
             case NBD_OPT_EXPORT_NAME:
                 return nbd_negotiate_handle_export_name(client, length);
@@ -560,6 +564,12 @@ static int nbd_negotiate_options(NBDClient *client)
     }
 }
 
+/* nbd_negotiate
+ * Return:
+ * < 0 on error
+ * 0   on successful negotiation
+ * 1   if client sent NBD_OPT_ABORT, i.e. on legal disconnect
+ */
 static coroutine_fn int nbd_negotiate(NBDClient *client)
 {
     char buf[8 + 8 + 8 + 128];
-- 
2.11.1

Re: [Qemu-devel] [PATCH 14/19] nbd/server: nbd_negotiate: return 1 on NBD_OPT_ABORT

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2991 bytes --]

On 05/30/2017 09:30 AM, Vladimir Sementsov-Ogievskiy wrote:
> Separate case when client sent NBD_OPT_ABORT from other errors.
> It will be needed for the following patch, where errors will be
> reported.
> Considered case is not actually the error - it honestly follows NBD
> protocol. Therefore it should not be reported like an error.
> -EPIPE case means client not read server reply on NBD_OPT_ABORT,
> which is also OK.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  nbd/server.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/nbd/server.c b/nbd/server.c
> index 30dfb81a5c..0e53d3dd91 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -369,9 +369,13 @@ static QIOChannel *nbd_negotiate_handle_starttls(NBDClient *client,
>      return QIO_CHANNEL(tioc);
>  }
>  
> -
> -/* Process all NBD_OPT_* client option commands.
> - * Return -errno on error, 0 on success. */
> +/* nbd_negotiate_options
> + * Process all NBD_OPT_* client option commands.
> + * Return:
> + * < 0 on error

Do you want to be specific that this is a negative errno value, or is it
just any negative value with no correlation to errno?

> + * 0   on successful negotiation
> + * 1   if client sent NBD_OPT_ABORT, i.e. on legal disconnect
> + */
>  static int nbd_negotiate_options(NBDClient *client)
>  {
>      int ret;
> @@ -483,7 +487,7 @@ static int nbd_negotiate_options(NBDClient *client)
>                  }
>                  /* Let the client keep trying, unless they asked to quit */
>                  if (clientflags == NBD_OPT_ABORT) {
> -                    return -EINVAL;
> +                    return 1;
>                  }
>                  break;
>              }
> @@ -502,7 +506,7 @@ static int nbd_negotiate_options(NBDClient *client)
>                   * guests that don't wait for our reply. */
>                  ret = nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK,
>                                               clientflags);
> -                return ret < 0 ? ret : -EINVAL;
> +                return ret < 0 && ret != -EPIPE ? ret : 1;

This should just be 'return 1;', which means you don't need to capture
and check 'ret'.

>  
>              case NBD_OPT_EXPORT_NAME:
>                  return nbd_negotiate_handle_export_name(client, length);
> @@ -560,6 +564,12 @@ static int nbd_negotiate_options(NBDClient *client)
>      }
>  }
>  
> +/* nbd_negotiate
> + * Return:
> + * < 0 on error

Again, if this is reliably a negative errno, specifically document that.

> + * 0   on successful negotiation
> + * 1   if client sent NBD_OPT_ABORT, i.e. on legal disconnect
> + */
>  static coroutine_fn int nbd_negotiate(NBDClient *client)
>  {
>      char buf[8 + 8 + 8 + 128];
> 

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 14/19] nbd/server: nbd_negotiate: return 1 on NBD_OPT_ABORT

[Vladimir Sementsov-Ogievskiy]

02.06.2017 01:33, Eric Blake wrote:
> On 05/30/2017 09:30 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Separate case when client sent NBD_OPT_ABORT from other errors.
>> It will be needed for the following patch, where errors will be
>> reported.
>> Considered case is not actually the error - it honestly follows NBD
>> protocol. Therefore it should not be reported like an error.
>> -EPIPE case means client not read server reply on NBD_OPT_ABORT,
>> which is also OK.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   nbd/server.c | 20 +++++++++++++++-----
>>   1 file changed, 15 insertions(+), 5 deletions(-)
>>
>> diff --git a/nbd/server.c b/nbd/server.c
>> index 30dfb81a5c..0e53d3dd91 100644
>> --- a/nbd/server.c
>> +++ b/nbd/server.c
>> @@ -369,9 +369,13 @@ static QIOChannel *nbd_negotiate_handle_starttls(NBDClient *client,
>>       return QIO_CHANNEL(tioc);
>>   }
>>   
>> -
>> -/* Process all NBD_OPT_* client option commands.
>> - * Return -errno on error, 0 on success. */
>> +/* nbd_negotiate_options
>> + * Process all NBD_OPT_* client option commands.
>> + * Return:
>> + * < 0 on error
> Do you want to be specific that this is a negative errno value, or is it
> just any negative value with no correlation to errno?

nothing here (except blk_write and friends, but their errors are not 
returned by any function) correlates with errno, because core function - 
nbd_wr_syncv returns EIO on any error. Underlying io_channel returns -1 
or QIO_CHANNEL_ERR_BLOCK...


>
>> + * 0   on successful negotiation
>> + * 1   if client sent NBD_OPT_ABORT, i.e. on legal disconnect
>> + */
>>   static int nbd_negotiate_options(NBDClient *client)
>>   {
>>       int ret;
>> @@ -483,7 +487,7 @@ static int nbd_negotiate_options(NBDClient *client)
>>                   }
>>                   /* Let the client keep trying, unless they asked to quit */
>>                   if (clientflags == NBD_OPT_ABORT) {
>> -                    return -EINVAL;
>> +                    return 1;
>>                   }
>>                   break;
>>               }
>> @@ -502,7 +506,7 @@ static int nbd_negotiate_options(NBDClient *client)
>>                    * guests that don't wait for our reply. */
>>                   ret = nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK,
>>                                                clientflags);
>> -                return ret < 0 ? ret : -EINVAL;
>> +                return ret < 0 && ret != -EPIPE ? ret : 1;
> This should just be 'return 1;', which means you don't need to capture
> and check 'ret'.
>
>>   
>>               case NBD_OPT_EXPORT_NAME:
>>                   return nbd_negotiate_handle_export_name(client, length);
>> @@ -560,6 +564,12 @@ static int nbd_negotiate_options(NBDClient *client)
>>       }
>>   }
>>   
>> +/* nbd_negotiate
>> + * Return:
>> + * < 0 on error
> Again, if this is reliably a negative errno, specifically document that.
>
>> + * 0   on successful negotiation
>> + * 1   if client sent NBD_OPT_ABORT, i.e. on legal disconnect
>> + */
>>   static coroutine_fn int nbd_negotiate(NBDClient *client)
>>   {
>>       char buf[8 + 8 + 8 + 128];
>>

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 14/19] nbd/server: nbd_negotiate: return 1 on NBD_OPT_ABORT

[Vladimir Sementsov-Ogievskiy]

02.06.2017 15:55, Vladimir Sementsov-Ogievskiy wrote:
> 02.06.2017 01:33, Eric Blake wrote:
>> On 05/30/2017 09:30 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> Separate case when client sent NBD_OPT_ABORT from other errors.
>>> It will be needed for the following patch, where errors will be
>>> reported.
>>> Considered case is not actually the error - it honestly follows NBD
>>> protocol. Therefore it should not be reported like an error.
>>> -EPIPE case means client not read server reply on NBD_OPT_ABORT,
>>> which is also OK.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   nbd/server.c | 20 +++++++++++++++-----
>>>   1 file changed, 15 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/nbd/server.c b/nbd/server.c
>>> index 30dfb81a5c..0e53d3dd91 100644
>>> --- a/nbd/server.c
>>> +++ b/nbd/server.c
>>> @@ -369,9 +369,13 @@ static QIOChannel 
>>> *nbd_negotiate_handle_starttls(NBDClient *client,
>>>       return QIO_CHANNEL(tioc);
>>>   }
>>>   -
>>> -/* Process all NBD_OPT_* client option commands.
>>> - * Return -errno on error, 0 on success. */
>>> +/* nbd_negotiate_options
>>> + * Process all NBD_OPT_* client option commands.
>>> + * Return:
>>> + * < 0 on error
>> Do you want to be specific that this is a negative errno value, or is it
>> just any negative value with no correlation to errno?
>
> nothing here (except blk_write and friends, but their errors are not 
> returned by any function) correlates with errno, because core function 
> - nbd_wr_syncv returns EIO on any error. Underlying io_channel returns 
> -1 or QIO_CHANNEL_ERR_BLOCK...

changed to "-errno  on error"

>
>
>>
>>> + * 0   on successful negotiation
>>> + * 1   if client sent NBD_OPT_ABORT, i.e. on legal disconnect
>>> + */
>>>   static int nbd_negotiate_options(NBDClient *client)
>>>   {
>>>       int ret;
>>> @@ -483,7 +487,7 @@ static int nbd_negotiate_options(NBDClient *client)
>>>                   }
>>>                   /* Let the client keep trying, unless they asked 
>>> to quit */
>>>                   if (clientflags == NBD_OPT_ABORT) {
>>> -                    return -EINVAL;
>>> +                    return 1;
>>>                   }
>>>                   break;
>>>               }
>>> @@ -502,7 +506,7 @@ static int nbd_negotiate_options(NBDClient *client)
>>>                    * guests that don't wait for our reply. */
>>>                   ret = nbd_negotiate_send_rep(client->ioc, 
>>> NBD_REP_ACK,
>>>                                                clientflags);
>>> -                return ret < 0 ? ret : -EINVAL;
>>> +                return ret < 0 && ret != -EPIPE ? ret : 1;
>> This should just be 'return 1;', which means you don't need to capture
>> and check 'ret'.
>>
>>>                 case NBD_OPT_EXPORT_NAME:
>>>                   return nbd_negotiate_handle_export_name(client, 
>>> length);
>>> @@ -560,6 +564,12 @@ static int nbd_negotiate_options(NBDClient 
>>> *client)
>>>       }
>>>   }
>>>   +/* nbd_negotiate
>>> + * Return:
>>> + * < 0 on error
>> Again, if this is reliably a negative errno, specifically document that.
>>
>>> + * 0   on successful negotiation
>>> + * 1   if client sent NBD_OPT_ABORT, i.e. on legal disconnect
>>> + */
>>>   static coroutine_fn int nbd_negotiate(NBDClient *client)
>>>   {
>>>       char buf[8 + 8 + 8 + 128];
>>>
>

-- 
Best regards,
Vladimir

[Qemu-devel] [PATCH 15/19] nbd/server: use errp instead of LOG

[Vladimir Sementsov-Ogievskiy]

Move to modern errp scheme from just LOGging errors.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 nbd/server.c | 257 ++++++++++++++++++++++++++++++++++-------------------------
 1 file changed, 150 insertions(+), 107 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 0e53d3dd91..3035fb6586 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -134,7 +134,7 @@ static void nbd_client_receive_next_request(NBDClient *client);
 /* Send a reply header, including length, but no payload.
  * Return -errno on error, 0 on success. */
 static int nbd_negotiate_send_rep_len(QIOChannel *ioc, uint32_t type,
-                                      uint32_t opt, uint32_t len)
+                                      uint32_t opt, uint32_t len, Error **errp)
 {
     int ret;
     uint64_t magic;
@@ -143,30 +143,30 @@ static int nbd_negotiate_send_rep_len(QIOChannel *ioc, uint32_t type,
           type, opt, len);
 
     magic = cpu_to_be64(NBD_REP_MAGIC);
-    ret = write_sync(ioc, &magic, sizeof(magic), NULL);
+    ret = write_sync(ioc, &magic, sizeof(magic), errp);
     if (ret < 0) {
-        LOG("write failed (rep magic)");
+        error_prepend(errp, "write failed (rep magic): ");
         return ret;
     }
 
     opt = cpu_to_be32(opt);
-    ret = write_sync(ioc, &opt, sizeof(opt), NULL);
+    ret = write_sync(ioc, &opt, sizeof(opt), errp);
     if (ret < 0) {
-        LOG("write failed (rep opt)");
+        error_prepend(errp, "write failed (rep opt): ");
         return ret;
     }
 
     type = cpu_to_be32(type);
-    ret = write_sync(ioc, &type, sizeof(type), NULL);
+    ret = write_sync(ioc, &type, sizeof(type), errp);
     if (ret < 0) {
-        LOG("write failed (rep type)");
+        error_prepend(errp, "write failed (rep type): ");
         return ret;
     }
 
     len = cpu_to_be32(len);
-    ret = write_sync(ioc, &len, sizeof(len), NULL);
+    ret = write_sync(ioc, &len, sizeof(len), errp);
     if (ret < 0) {
-        LOG("write failed (rep data length)");
+        error_prepend(errp, "write failed (rep data length): ");
         return ret;
     }
     return 0;
@@ -174,16 +174,17 @@ static int nbd_negotiate_send_rep_len(QIOChannel *ioc, uint32_t type,
 
 /* Send a reply header with default 0 length.
  * Return -errno on error, 0 on success. */
-static int nbd_negotiate_send_rep(QIOChannel *ioc, uint32_t type, uint32_t opt)
+static int nbd_negotiate_send_rep(QIOChannel *ioc, uint32_t type, uint32_t opt,
+                                  Error **errp)
 {
-    return nbd_negotiate_send_rep_len(ioc, type, opt, 0);
+    return nbd_negotiate_send_rep_len(ioc, type, opt, 0, errp);
 }
 
 /* Send an error reply.
  * Return -errno on error, 0 on success. */
-static int GCC_FMT_ATTR(4, 5)
+static int GCC_FMT_ATTR(5, 6)
 nbd_negotiate_send_rep_err(QIOChannel *ioc, uint32_t type,
-                           uint32_t opt, const char *fmt, ...)
+                           uint32_t opt, Error **errp, const char *fmt, ...)
 {
     va_list va;
     char *msg;
@@ -196,14 +197,14 @@ nbd_negotiate_send_rep_err(QIOChannel *ioc, uint32_t type,
     len = strlen(msg);
     assert(len < 4096);
     TRACE("sending error message \"%s\"", msg);
-    ret = nbd_negotiate_send_rep_len(ioc, type, opt, len);
+    ret = nbd_negotiate_send_rep_len(ioc, type, opt, len, errp);
     if (ret < 0) {
         goto out;
     }
 
-    ret = write_sync(ioc, msg, len, NULL);
+    ret = write_sync(ioc, msg, len, errp);
     if (ret < 0) {
-        LOG("write failed (error message)");
+        error_prepend(errp, "write failed (error message): ");
     }
 
 out:
@@ -213,7 +214,8 @@ out:
 
 /* Send a single NBD_REP_SERVER reply to NBD_OPT_LIST, including payload.
  * Return -errno on error, 0 on success. */
-static int nbd_negotiate_send_rep_list(QIOChannel *ioc, NBDExport *exp)
+static int nbd_negotiate_send_rep_list(QIOChannel *ioc, NBDExport *exp,
+                                       Error **errp)
 {
     size_t name_len, desc_len;
     uint32_t len;
@@ -225,27 +227,28 @@ static int nbd_negotiate_send_rep_list(QIOChannel *ioc, NBDExport *exp)
     name_len = strlen(name);
     desc_len = strlen(desc);
     len = name_len + desc_len + sizeof(len);
-    ret = nbd_negotiate_send_rep_len(ioc, NBD_REP_SERVER, NBD_OPT_LIST, len);
+    ret = nbd_negotiate_send_rep_len(ioc, NBD_REP_SERVER, NBD_OPT_LIST, len,
+                                     errp);
     if (ret < 0) {
         return ret;
     }
 
     len = cpu_to_be32(name_len);
-    ret = write_sync(ioc, &len, sizeof(len), NULL);
+    ret = write_sync(ioc, &len, sizeof(len), errp);
     if (ret < 0) {
-        LOG("write failed (name length)");
+        error_prepend(errp, "write failed (name length): ");
         return ret;
     }
 
-    ret = write_sync(ioc, name, name_len, NULL);
+    ret = write_sync(ioc, name, name_len, errp);
     if (ret < 0) {
-        LOG("write failed (name buffer)");
+        error_prepend(errp, "write failed (name buffer): ");
         return ret;
     }
 
-    ret = write_sync(ioc, desc, desc_len, NULL);
+    ret = write_sync(ioc, desc, desc_len, errp);
     if (ret < 0) {
-        LOG("write failed (description buffer)");
+        error_prepend(errp, "write failed (description buffer): ");
         return ret;
     }
 
@@ -254,33 +257,36 @@ static int nbd_negotiate_send_rep_list(QIOChannel *ioc, NBDExport *exp)
 
 /* Process the NBD_OPT_LIST command, with a potential series of replies.
  * Return -errno on error, 0 on success. */
-static int nbd_negotiate_handle_list(NBDClient *client, uint32_t length)
+static int nbd_negotiate_handle_list(NBDClient *client, uint32_t length,
+                                     Error **errp)
 {
     int ret;
     NBDExport *exp;
 
     if (length) {
-        ret = drop_sync(client->ioc, length, NULL);
+        ret = drop_sync(client->ioc, length, errp);
         if (ret < 0) {
             return ret;
         }
         return nbd_negotiate_send_rep_err(client->ioc,
                                           NBD_REP_ERR_INVALID, NBD_OPT_LIST,
+                                          errp,
                                           "OPT_LIST should not have length");
     }
 
     /* For each export, send a NBD_REP_SERVER reply. */
     QTAILQ_FOREACH(exp, &exports, next) {
-        ret = nbd_negotiate_send_rep_list(client->ioc, exp);
+        ret = nbd_negotiate_send_rep_list(client->ioc, exp, errp);
         if (ret < 0) {
             return ret;
         }
     }
     /* Finish with a NBD_REP_ACK. */
-    return nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK, NBD_OPT_LIST);
+    return nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK, NBD_OPT_LIST, errp);
 }
 
-static int nbd_negotiate_handle_export_name(NBDClient *client, uint32_t length)
+static int nbd_negotiate_handle_export_name(NBDClient *client, uint32_t length,
+                                            Error **errp)
 {
     int ret;
     char name[NBD_MAX_NAME_SIZE + 1];
@@ -290,13 +296,13 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, uint32_t length)
      */
     TRACE("Checking length");
     if (length >= sizeof(name)) {
-        LOG("Bad length received");
+        error_setg(errp, "Bad length received");
         return -EINVAL;
     }
 
-    ret = read_sync(client->ioc, name, length, NULL);
+    ret = read_sync(client->ioc, name, length, errp);
     if (ret < 0) {
-        LOG("read failed");
+        error_setg(errp, "read failed");
         return ret;
     }
     name[length] = '\0';
@@ -305,7 +311,7 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, uint32_t length)
 
     client->exp = nbd_export_find(name);
     if (!client->exp) {
-        LOG("export not found");
+        error_setg(errp, "export not found");
         return -EINVAL;
     }
 
@@ -318,7 +324,8 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, uint32_t length)
 /* Handle NBD_OPT_STARTTLS. Return NULL to drop connection, or else the
  * new channel for all further (now-encrypted) communication. */
 static QIOChannel *nbd_negotiate_handle_starttls(NBDClient *client,
-                                                 uint32_t length)
+                                                 uint32_t length,
+                                                 Error **errp)
 {
     QIOChannel *ioc;
     QIOChannelTLS *tioc;
@@ -327,23 +334,24 @@ static QIOChannel *nbd_negotiate_handle_starttls(NBDClient *client,
     TRACE("Setting up TLS");
     ioc = client->ioc;
     if (length) {
-        if (drop_sync(ioc, length, NULL) < 0) {
+        if (drop_sync(ioc, length, errp) < 0) {
             return NULL;
         }
         nbd_negotiate_send_rep_err(ioc, NBD_REP_ERR_INVALID, NBD_OPT_STARTTLS,
+                                   errp,
                                    "OPT_STARTTLS should not have length");
         return NULL;
     }
 
     if (nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK,
-                               NBD_OPT_STARTTLS) < 0) {
+                               NBD_OPT_STARTTLS, errp) < 0) {
         return NULL;
     }
 
     tioc = qio_channel_tls_new_server(ioc,
                                       client->tlscreds,
                                       client->tlsaclname,
-                                      NULL);
+                                      errp);
     if (!tioc) {
         return NULL;
     }
@@ -362,7 +370,7 @@ static QIOChannel *nbd_negotiate_handle_starttls(NBDClient *client,
     g_main_loop_unref(data.loop);
     if (data.error) {
         object_unref(OBJECT(tioc));
-        error_free(data.error);
+        error_propagate(errp, data.error);
         return NULL;
     }
 
@@ -372,15 +380,16 @@ static QIOChannel *nbd_negotiate_handle_starttls(NBDClient *client,
 /* nbd_negotiate_options
  * Process all NBD_OPT_* client option commands.
  * Return:
- * < 0 on error
- * 0   on successful negotiation
- * 1   if client sent NBD_OPT_ABORT, i.e. on legal disconnect
+ * < 0 on error, errp is set
+ * 0   on successful negotiation, errp is not set
+ * 1   if client sent NBD_OPT_ABORT, i.e. on legal disconnect, errp is not set
  */
-static int nbd_negotiate_options(NBDClient *client)
+static int nbd_negotiate_options(NBDClient *client, Error **errp)
 {
     int ret;
     uint32_t flags;
     bool fixedNewstyle = false;
+    Error *local_err = NULL;
 
     /* Client sends:
         [ 0 ..   3]   client flags
@@ -396,9 +405,9 @@ static int nbd_negotiate_options(NBDClient *client)
         ...           Rest of request
     */
 
-    ret = read_sync(client->ioc, &flags, sizeof(flags), NULL);
+    ret = read_sync(client->ioc, &flags, sizeof(flags), errp);
     if (ret < 0) {
-        LOG("read failed");
+        error_prepend(errp, "read failed: ");
         return ret;
     }
     TRACE("Checking client flags");
@@ -414,7 +423,7 @@ static int nbd_negotiate_options(NBDClient *client)
         flags &= ~NBD_FLAG_C_NO_ZEROES;
     }
     if (flags != 0) {
-        TRACE("Unknown client flags 0x%" PRIx32 " received", flags);
+        error_setg(errp, "Unknown client flags 0x%" PRIx32 " received", flags);
         return -EIO;
     }
 
@@ -423,27 +432,27 @@ static int nbd_negotiate_options(NBDClient *client)
         uint32_t clientflags, length;
         uint64_t magic;
 
-        ret = read_sync(client->ioc, &magic, sizeof(magic), NULL);
+        ret = read_sync(client->ioc, &magic, sizeof(magic), errp);
         if (ret < 0) {
-            LOG("read failed");
+            error_prepend(errp, "read failed: ");
             return ret;
         }
         TRACE("Checking opts magic");
         if (magic != be64_to_cpu(NBD_OPTS_MAGIC)) {
-            LOG("Bad magic received");
+            error_setg(errp, "Bad magic received");
             return -EINVAL;
         }
 
-        ret = read_sync(client->ioc, &clientflags, sizeof(clientflags), NULL);
+        ret = read_sync(client->ioc, &clientflags, sizeof(clientflags), errp);
         if (ret < 0) {
-            LOG("read failed");
+            error_prepend(errp, "read failed: ");
             return ret;
         }
         clientflags = be32_to_cpu(clientflags);
 
-        ret = read_sync(client->ioc, &length, sizeof(length), NULL);
+        ret = read_sync(client->ioc, &length, sizeof(length), errp);
         if (ret < 0) {
-            LOG("read failed");
+            error_prepend(errp, "read failed: ");
             return ret;
         }
         length = be32_to_cpu(length);
@@ -453,12 +462,12 @@ static int nbd_negotiate_options(NBDClient *client)
             client->ioc == (QIOChannel *)client->sioc) {
             QIOChannel *tioc;
             if (!fixedNewstyle) {
-                TRACE("Unsupported option 0x%" PRIx32, clientflags);
+                error_setg(errp, "Unsupported option 0x%" PRIx32, clientflags);
                 return -EINVAL;
             }
             switch (clientflags) {
             case NBD_OPT_STARTTLS:
-                tioc = nbd_negotiate_handle_starttls(client, length);
+                tioc = nbd_negotiate_handle_starttls(client, length, errp);
                 if (!tioc) {
                     return -EIO;
                 }
@@ -468,17 +477,18 @@ static int nbd_negotiate_options(NBDClient *client)
 
             case NBD_OPT_EXPORT_NAME:
                 /* No way to return an error to client, so drop connection */
-                TRACE("Option 0x%x not permitted before TLS", clientflags);
+                error_setg(errp, "Option 0x%x not permitted before TLS",
+                           clientflags);
                 return -EINVAL;
 
             default:
-                ret = drop_sync(client->ioc, length, NULL);
+                ret = drop_sync(client->ioc, length, errp);
                 if (ret < 0) {
                     return ret;
                 }
                 ret = nbd_negotiate_send_rep_err(client->ioc,
                                                  NBD_REP_ERR_TLS_REQD,
-                                                 clientflags,
+                                                 clientflags, errp,
                                                  "Option 0x%" PRIx32
                                                  "not permitted before TLS",
                                                  clientflags);
@@ -494,7 +504,7 @@ static int nbd_negotiate_options(NBDClient *client)
         } else if (fixedNewstyle) {
             switch (clientflags) {
             case NBD_OPT_LIST:
-                ret = nbd_negotiate_handle_list(client, length);
+                ret = nbd_negotiate_handle_list(client, length, errp);
                 if (ret < 0) {
                     return ret;
                 }
@@ -505,26 +515,33 @@ static int nbd_negotiate_options(NBDClient *client)
                  * disconnecting, but that we must also tolerate
                  * guests that don't wait for our reply. */
                 ret = nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK,
-                                             clientflags);
-                return ret < 0 && ret != -EPIPE ? ret : 1;
+                                             clientflags, &local_err);
+
+                if (ret < 0 && ret != -EPIPE) {
+                    error_propagate(errp, local_err);
+                    return ret;
+                }
+
+                error_free(local_err);
+                return 1;
 
             case NBD_OPT_EXPORT_NAME:
-                return nbd_negotiate_handle_export_name(client, length);
+                return nbd_negotiate_handle_export_name(client, length, errp);
 
             case NBD_OPT_STARTTLS:
-                ret = drop_sync(client->ioc, length, NULL);
+                ret = drop_sync(client->ioc, length, errp);
                 if (ret < 0) {
                     return ret;
                 }
                 if (client->tlscreds) {
                     ret = nbd_negotiate_send_rep_err(client->ioc,
                                                      NBD_REP_ERR_INVALID,
-                                                     clientflags,
+                                                     clientflags, errp,
                                                      "TLS already enabled");
                 } else {
                     ret = nbd_negotiate_send_rep_err(client->ioc,
                                                      NBD_REP_ERR_POLICY,
-                                                     clientflags,
+                                                     clientflags, errp,
                                                      "TLS not configured");
                 }
                 if (ret < 0) {
@@ -532,13 +549,13 @@ static int nbd_negotiate_options(NBDClient *client)
                 }
                 break;
             default:
-                ret = drop_sync(client->ioc, length, NULL);
+                ret = drop_sync(client->ioc, length, errp);
                 if (ret < 0) {
                     return ret;
                 }
                 ret = nbd_negotiate_send_rep_err(client->ioc,
                                                  NBD_REP_ERR_UNSUP,
-                                                 clientflags,
+                                                 clientflags, errp,
                                                  "Unsupported option 0x%"
                                                  PRIx32,
                                                  clientflags);
@@ -554,10 +571,10 @@ static int nbd_negotiate_options(NBDClient *client)
              */
             switch (clientflags) {
             case NBD_OPT_EXPORT_NAME:
-                return nbd_negotiate_handle_export_name(client, length);
+                return nbd_negotiate_handle_export_name(client, length, errp);
 
             default:
-                TRACE("Unsupported option 0x%" PRIx32, clientflags);
+                error_setg(errp, "Unsupported option 0x%" PRIx32, clientflags);
                 return -EINVAL;
             }
         }
@@ -566,11 +583,11 @@ static int nbd_negotiate_options(NBDClient *client)
 
 /* nbd_negotiate
  * Return:
- * < 0 on error
- * 0   on successful negotiation
- * 1   if client sent NBD_OPT_ABORT, i.e. on legal disconnect
+ * < 0 on error, errp is set
+ * 0   on successful negotiation, errp is not set
+ * 1   if client sent NBD_OPT_ABORT, i.e. on legal disconnect, errp is not set
  */
-static coroutine_fn int nbd_negotiate(NBDClient *client)
+static coroutine_fn int nbd_negotiate(NBDClient *client, Error **errp)
 {
     char buf[8 + 8 + 8 + 128];
     int ret;
@@ -618,24 +635,26 @@ static coroutine_fn int nbd_negotiate(NBDClient *client)
 
     if (oldStyle) {
         if (client->tlscreds) {
-            TRACE("TLS cannot be enabled with oldstyle protocol");
+            error_setg(errp, "TLS cannot be enabled with oldstyle protocol");
             return -EINVAL;
         }
 
-        ret = write_sync(client->ioc, buf, sizeof(buf), NULL);
+        ret = write_sync(client->ioc, buf, sizeof(buf), errp);
         if (ret < 0) {
-            LOG("write failed");
+            error_prepend(errp, "write failed: ");
             return ret;
         }
     } else {
-        ret = write_sync(client->ioc, buf, 18, NULL);
+        ret = write_sync(client->ioc, buf, 18, errp);
         if (ret < 0) {
-            LOG("write failed");
+            error_prepend(errp, "write failed: ");
             return ret;
         }
-        ret = nbd_negotiate_options(client);
+        ret = nbd_negotiate_options(client, errp);
         if (ret != 0) {
-            LOG("option negotiation failed");
+            if (ret < 0) {
+                error_prepend(errp, "option negotiation failed: ");
+            }
             return ret;
         }
 
@@ -644,9 +663,9 @@ static coroutine_fn int nbd_negotiate(NBDClient *client)
         stq_be_p(buf + 18, client->exp->size);
         stw_be_p(buf + 26, client->exp->nbdflags | myflags);
         len = client->no_zeroes ? 10 : sizeof(buf) - 18;
-        ret = write_sync(client->ioc, buf + 18, len, NULL);
+        ret = write_sync(client->ioc, buf + 18, len, errp);
         if (ret < 0) {
-            LOG("write failed");
+            error_prepend(errp, "write failed: ");
             return ret;
         }
     }
@@ -656,13 +675,14 @@ static coroutine_fn int nbd_negotiate(NBDClient *client)
     return 0;
 }
 
-static int nbd_receive_request(QIOChannel *ioc, NBDRequest *request)
+static int nbd_receive_request(QIOChannel *ioc, NBDRequest *request,
+                               Error **errp)
 {
     uint8_t buf[NBD_REQUEST_SIZE];
     uint32_t magic;
     int ret;
 
-    ret = read_sync(ioc, buf, sizeof(buf), NULL);
+    ret = read_sync(ioc, buf, sizeof(buf), errp);
     if (ret < 0) {
         return ret;
     }
@@ -688,7 +708,7 @@ static int nbd_receive_request(QIOChannel *ioc, NBDRequest *request)
           magic, request->flags, request->type, request->from, request->len);
 
     if (magic != NBD_REQUEST_MAGIC) {
-        LOG("invalid magic (got 0x%" PRIx32 ")", magic);
+        error_setg(errp, "invalid magic (got 0x%" PRIx32 ")", magic);
         return -EINVAL;
     }
     return 0;
@@ -1034,13 +1054,14 @@ static int nbd_co_send_reply(NBDRequestData *req, NBDReply *reply, int len)
  * the client (although the caller may still need to disconnect after reporting
  * the error).
  */
-static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request)
+static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request,
+                                  Error **errp)
 {
     NBDClient *client = req->client;
 
     g_assert(qemu_in_coroutine());
     assert(client->recv_coroutine == qemu_coroutine_self());
-    if (nbd_receive_request(client->ioc, request) < 0) {
+    if (nbd_receive_request(client->ioc, request, errp) < 0) {
         return -EIO;
     }
 
@@ -1062,27 +1083,29 @@ static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request)
      * checks as possible until after reading any NBD_CMD_WRITE
      * payload, so we can try and keep the connection alive.  */
     if ((request->from + request->len) < request->from) {
-        LOG("integer overflow detected, you're probably being attacked");
+        error_setg(errp,
+                   "integer overflow detected, you're probably being attacked");
         return -EINVAL;
     }
 
     if (request->type == NBD_CMD_READ || request->type == NBD_CMD_WRITE) {
         if (request->len > NBD_MAX_BUFFER_SIZE) {
-            LOG("len (%" PRIu32" ) is larger than max len (%u)",
-                request->len, NBD_MAX_BUFFER_SIZE);
+            error_setg(errp, "len (%" PRIu32" ) is larger than max len (%u)",
+                       request->len, NBD_MAX_BUFFER_SIZE);
             return -EINVAL;
         }
 
         req->data = blk_try_blockalign(client->exp->blk, request->len);
         if (req->data == NULL) {
+            error_setg(errp, "No memory");
             return -ENOMEM;
         }
     }
     if (request->type == NBD_CMD_WRITE) {
         TRACE("Reading %" PRIu32 " byte(s)", request->len);
 
-        if (read_sync(client->ioc, req->data, request->len, NULL) < 0) {
-            LOG("reading from socket failed");
+        if (read_sync(client->ioc, req->data, request->len, errp) < 0) {
+            error_prepend(errp, "reading from socket failed: ");
             return -EIO;
         }
         req->complete = true;
@@ -1090,18 +1113,18 @@ static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request)
 
     /* Sanity checks, part 2. */
     if (request->from + request->len > client->exp->size) {
-        LOG("operation past EOF; From: %" PRIu64 ", Len: %" PRIu32
-            ", Size: %" PRIu64, request->from, request->len,
-            (uint64_t)client->exp->size);
+        error_setg(errp, "operation past EOF; From: %" PRIu64 ", Len: %" PRIu32
+                   ", Size: %" PRIu64, request->from, request->len,
+                   (uint64_t)client->exp->size);
         return request->type == NBD_CMD_WRITE ? -ENOSPC : -EINVAL;
     }
     if (request->flags & ~(NBD_CMD_FLAG_FUA | NBD_CMD_FLAG_NO_HOLE)) {
-        LOG("unsupported flags (got 0x%x)", request->flags);
+        error_setg(errp, "unsupported flags (got 0x%x)", request->flags);
         return -EINVAL;
     }
     if (request->type != NBD_CMD_WRITE_ZEROES &&
         (request->flags & NBD_CMD_FLAG_NO_HOLE)) {
-        LOG("unexpected flags (got 0x%x)", request->flags);
+        error_setg(errp, "unexpected flags (got 0x%x)", request->flags);
         return -EINVAL;
     }
 
@@ -1119,6 +1142,7 @@ static coroutine_fn void nbd_trip(void *opaque)
     int ret;
     int flags;
     int reply_data_len = 0;
+    Error *local_err = NULL;
 
     TRACE("Reading request.");
     if (client->closing) {
@@ -1127,7 +1151,7 @@ static coroutine_fn void nbd_trip(void *opaque)
     }
 
     req = nbd_request_get(client);
-    ret = nbd_co_receive_request(req, &request);
+    ret = nbd_co_receive_request(req, &request, &local_err);
     client->recv_coroutine = NULL;
     nbd_client_receive_next_request(client);
     if (ret == -EIO) {
@@ -1158,7 +1182,7 @@ static coroutine_fn void nbd_trip(void *opaque)
         if (request.flags & NBD_CMD_FLAG_FUA) {
             ret = blk_co_flush(exp->blk);
             if (ret < 0) {
-                LOG("flush failed");
+                error_setg_errno(&local_err, -ret, "flush failed");
                 reply.error = -ret;
                 break;
             }
@@ -1167,7 +1191,7 @@ static coroutine_fn void nbd_trip(void *opaque)
         ret = blk_pread(exp->blk, request.from + exp->dev_offset,
                         req->data, request.len);
         if (ret < 0) {
-            LOG("reading from file failed");
+            error_setg_errno(&local_err, -ret, "reading from file failed");
             reply.error = -ret;
             break;
         }
@@ -1194,7 +1218,7 @@ static coroutine_fn void nbd_trip(void *opaque)
         ret = blk_pwrite(exp->blk, request.from + exp->dev_offset,
                          req->data, request.len, flags);
         if (ret < 0) {
-            LOG("writing to file failed");
+            error_setg_errno(&local_err, -ret, "writing to file failed");
             reply.error = -ret;
         }
 
@@ -1203,7 +1227,7 @@ static coroutine_fn void nbd_trip(void *opaque)
         TRACE("Request type is WRITE_ZEROES");
 
         if (exp->nbdflags & NBD_FLAG_READ_ONLY) {
-            TRACE("Server is read-only, return error");
+            error_setg(&local_err, "Server is read-only, return error");
             reply.error = EROFS;
             break;
         }
@@ -1220,7 +1244,7 @@ static coroutine_fn void nbd_trip(void *opaque)
         ret = blk_pwrite_zeroes(exp->blk, request.from + exp->dev_offset,
                                 request.len, flags);
         if (ret < 0) {
-            LOG("writing to file failed");
+            error_setg_errno(&local_err, -ret, "writing to file failed");
             reply.error = -ret;
         }
 
@@ -1234,7 +1258,7 @@ static coroutine_fn void nbd_trip(void *opaque)
 
         ret = blk_co_flush(exp->blk);
         if (ret < 0) {
-            LOG("flush failed");
+            error_setg_errno(&local_err, -ret, "flush failed");
             reply.error = -ret;
         }
 
@@ -1244,21 +1268,33 @@ static coroutine_fn void nbd_trip(void *opaque)
         ret = blk_co_pdiscard(exp->blk, request.from + exp->dev_offset,
                               request.len);
         if (ret < 0) {
-            LOG("discard failed");
+            error_setg_errno(&local_err, -ret, "discard failed");
             reply.error = -ret;
         }
 
         break;
     default:
-        LOG("invalid request type (%" PRIu32 ") received", request.type);
+        error_setg(&local_err, "invalid request type (%" PRIu32 ") received",
+                   request.type);
         reply.error = EINVAL;
     }
 
 reply:
+    if (local_err) {
+        error_report_err(local_err);
+        local_err = NULL;
+    }
+
+    if (nbd_co_send_reply(req, &reply, reply_data_len) < 0) {
+        error_setg(&local_err, "Failed to send reply");
+        goto disconnect;
+    }
+
     /* We must disconnect after NBD_CMD_WRITE if we did not
      * read the payload.
      */
-    if (nbd_co_send_reply(req, &reply, reply_data_len) < 0 || !req->complete) {
+    if (!req->complete) {
+        error_setg(&local_err, "Request handling failed in intermediate state");
         goto disconnect;
     }
 
@@ -1270,6 +1306,9 @@ done:
     return;
 
 disconnect:
+    if (local_err) {
+        error_reportf_err(local_err, "Disconnect client, due to: ");
+    }
     nbd_request_put(req);
     client_close(client);
     nbd_client_put(client);
@@ -1288,11 +1327,15 @@ static coroutine_fn void nbd_co_client_start(void *opaque)
 {
     NBDClient *client = opaque;
     NBDExport *exp = client->exp;
+    Error *local_err = NULL;
 
     if (exp) {
         nbd_export_get(exp);
     }
-    if (nbd_negotiate(client)) {
+    if (nbd_negotiate(client, &local_err)) {
+        if (local_err) {
+            error_report_err(local_err);
+        }
         client_close(client);
         return;
     }
-- 
2.11.1

Re: [Qemu-devel] [PATCH 15/19] nbd/server: use errp instead of LOG

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 4421 bytes --]

On 05/30/2017 09:30 AM, Vladimir Sementsov-Ogievskiy wrote:
> Move to modern errp scheme from just LOGging errors.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  nbd/server.c | 257 ++++++++++++++++++++++++++++++++++-------------------------
>  1 file changed, 150 insertions(+), 107 deletions(-)
> 

> @@ -143,30 +143,30 @@ static int nbd_negotiate_send_rep_len(QIOChannel *ioc, uint32_t type,
>            type, opt, len);
>  
>      magic = cpu_to_be64(NBD_REP_MAGIC);
> -    ret = write_sync(ioc, &magic, sizeof(magic), NULL);
> +    ret = write_sync(ioc, &magic, sizeof(magic), errp);
>      if (ret < 0) {
> -        LOG("write failed (rep magic)");
> +        error_prepend(errp, "write failed (rep magic): ");
>          return ret;
>      }

I'm wondering how much we really want to use error_prepend(), or if we
could just get away with using the original error message unchanged.
I'm not saying to rewrite the patch now that you've done the work, so
much as asking aloud whether the additional information in the error
messages will prove useful.

There are definitely some ripple effects from your v2 posting of the
first half of the series that will require you to rebase this, but the
overall idea is sound.

>  /* Send an error reply.
>   * Return -errno on error, 0 on success. */
> -static int GCC_FMT_ATTR(4, 5)
> +static int GCC_FMT_ATTR(5, 6)
>  nbd_negotiate_send_rep_err(QIOChannel *ioc, uint32_t type,
> -                           uint32_t opt, const char *fmt, ...)
> +                           uint32_t opt, Error **errp, const char *fmt, ...)

Having errp not be the last argument is unusual, but I don't see how you
can do any differently with the var-args nature of the call.

> @@ -505,26 +515,33 @@ static int nbd_negotiate_options(NBDClient *client)
>                   * disconnecting, but that we must also tolerate
>                   * guests that don't wait for our reply. */
>                  ret = nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK,
> -                                             clientflags);
> -                return ret < 0 && ret != -EPIPE ? ret : 1;
> +                                             clientflags, &local_err);
> +
> +                if (ret < 0 && ret != -EPIPE) {
> +                    error_propagate(errp, local_err);
> +                    return ret;
> +                }
> +
> +                error_free(local_err);
> +                return 1;

Of course, you'll have to simplify this portion.  This is probably the
one place where you actually DO want to use:

nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK,
                       clientflags, NULL)

because you truly do not care whether you had an error.

> @@ -1090,18 +1113,18 @@ static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request)
>  
>      /* Sanity checks, part 2. */
>      if (request->from + request->len > client->exp->size) {
> -        LOG("operation past EOF; From: %" PRIu64 ", Len: %" PRIu32
> -            ", Size: %" PRIu64, request->from, request->len,
> -            (uint64_t)client->exp->size);
> +        error_setg(errp, "operation past EOF; From: %" PRIu64 ", Len: %" PRIu32
> +                   ", Size: %" PRIu64, request->from, request->len,
> +                   (uint64_t)client->exp->size);
>          return request->type == NBD_CMD_WRITE ? -ENOSPC : -EINVAL;

Question - now that we are consistently setting a decent errp, do we
have any callers that care about specific -errno return values, or could
we further simplify the functions by just returning -1 (instead of
worrying about -errno) on failures?


> @@ -1244,21 +1268,33 @@ static coroutine_fn void nbd_trip(void *opaque)

>  
>  reply:
> +    if (local_err) {
> +        error_report_err(local_err);
> +        local_err = NULL;
> +    }

This says that after we detect an error, we report it,

> +
> +    if (nbd_co_send_reply(req, &reply, reply_data_len) < 0) {
> +        error_setg(&local_err, "Failed to send reply");
> +        goto disconnect;
> +    }

...but then blindly try to send the reply anyways, forgetting that we
ever detected the original error.  Is that going to bite us?

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 15/19] nbd/server: use errp instead of LOG

[Vladimir Sementsov-Ogievskiy]

On 04.06.2017 00:46, Eric Blake wrote:
> On 05/30/2017 09:30 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Move to modern errp scheme from just LOGging errors.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   nbd/server.c | 257 ++++++++++++++++++++++++++++++++++-------------------------
>>   1 file changed, 150 insertions(+), 107 deletions(-)
>>
>> @@ -143,30 +143,30 @@ static int nbd_negotiate_send_rep_len(QIOChannel *ioc, uint32_t type,
>>             type, opt, len);
>>   
>>       magic = cpu_to_be64(NBD_REP_MAGIC);
>> -    ret = write_sync(ioc, &magic, sizeof(magic), NULL);
>> +    ret = write_sync(ioc, &magic, sizeof(magic), errp);
>>       if (ret < 0) {
>> -        LOG("write failed (rep magic)");
>> +        error_prepend(errp, "write failed (rep magic): ");
>>           return ret;
>>       }
> I'm wondering how much we really want to use error_prepend(), or if we
> could just get away with using the original error message unchanged.
> I'm not saying to rewrite the patch now that you've done the work, so
> much as asking aloud whether the additional information in the error
> messages will prove useful.
>
> There are definitely some ripple effects from your v2 posting of the
> first half of the series that will require you to rebase this, but the
> overall idea is sound.

I've tried to save old messages as is (+ additional info)

>
>>   /* Send an error reply.
>>    * Return -errno on error, 0 on success. */
>> -static int GCC_FMT_ATTR(4, 5)
>> +static int GCC_FMT_ATTR(5, 6)
>>   nbd_negotiate_send_rep_err(QIOChannel *ioc, uint32_t type,
>> -                           uint32_t opt, const char *fmt, ...)
>> +                           uint32_t opt, Error **errp, const char *fmt, ...)
> Having errp not be the last argument is unusual, but I don't see how you
> can do any differently with the var-args nature of the call.
>
>> @@ -505,26 +515,33 @@ static int nbd_negotiate_options(NBDClient *client)
>>                    * disconnecting, but that we must also tolerate
>>                    * guests that don't wait for our reply. */
>>                   ret = nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK,
>> -                                             clientflags);
>> -                return ret < 0 && ret != -EPIPE ? ret : 1;
>> +                                             clientflags, &local_err);
>> +
>> +                if (ret < 0 && ret != -EPIPE) {
>> +                    error_propagate(errp, local_err);
>> +                    return ret;
>> +                }
>> +
>> +                error_free(local_err);
>> +                return 1;
> Of course, you'll have to simplify this portion.  This is probably the
> one place where you actually DO want to use:
>
> nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK,
>                         clientflags, NULL)
>
> because you truly do not care whether you had an error.
>
>> @@ -1090,18 +1113,18 @@ static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request)
>>   
>>       /* Sanity checks, part 2. */
>>       if (request->from + request->len > client->exp->size) {
>> -        LOG("operation past EOF; From: %" PRIu64 ", Len: %" PRIu32
>> -            ", Size: %" PRIu64, request->from, request->len,
>> -            (uint64_t)client->exp->size);
>> +        error_setg(errp, "operation past EOF; From: %" PRIu64 ", Len: %" PRIu32
>> +                   ", Size: %" PRIu64, request->from, request->len,
>> +                   (uint64_t)client->exp->size);
>>           return request->type == NBD_CMD_WRITE ? -ENOSPC : -EINVAL;
> Question - now that we are consistently setting a decent errp, do we
> have any callers that care about specific -errno return values, or could
> we further simplify the functions by just returning -1 (instead of
> worrying about -errno) on failures?

I think it is not bad to have informative error codes.. Example is 
previous discussion about EPIPE.

>
>
>> @@ -1244,21 +1268,33 @@ static coroutine_fn void nbd_trip(void *opaque)
>>   
>>   reply:
>> +    if (local_err) {
>> +        error_report_err(local_err);
>> +        local_err = NULL;
>> +    }
> This says that after we detect an error, we report it,
>
>> +
>> +    if (nbd_co_send_reply(req, &reply, reply_data_len) < 0) {
>> +        error_setg(&local_err, "Failed to send reply");
>> +        goto disconnect;
>> +    }
> ...but then blindly try to send the reply anyways, forgetting that we
> ever detected the original error.  Is that going to bite us?

Reported error is blk_* error, which should be returned to client. But I 
think it is not bad to report is on server too (and this corresponds to 
the old behavior). Creating extra "Error *blk_err" may help.

>

-- 
Best regards,
Vladimir.

[Qemu-devel] [PATCH 16/19] nbd/server: add errp to nbd_send_reply()

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 nbd/server.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 3035fb6586..6c0702287e 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -714,7 +714,7 @@ static int nbd_receive_request(QIOChannel *ioc, NBDRequest *request,
     return 0;
 }
 
-static int nbd_send_reply(QIOChannel *ioc, NBDReply *reply)
+static int nbd_send_reply(QIOChannel *ioc, NBDReply *reply, Error **errp)
 {
     uint8_t buf[NBD_REPLY_SIZE];
 
@@ -733,7 +733,7 @@ static int nbd_send_reply(QIOChannel *ioc, NBDReply *reply)
     stl_be_p(buf + 4, reply->error);
     stq_be_p(buf + 8, reply->handle);
 
-    return write_sync(ioc, buf, sizeof(buf), NULL);
+    return write_sync(ioc, buf, sizeof(buf), errp);
 }
 
 #define MAX_NBD_REQUESTS 16
@@ -1020,7 +1020,8 @@ void nbd_export_close_all(void)
     }
 }
 
-static int nbd_co_send_reply(NBDRequestData *req, NBDReply *reply, int len)
+static int nbd_co_send_reply(NBDRequestData *req, NBDReply *reply, int len,
+                             Error **errp)
 {
     NBDClient *client = req->client;
     int ret;
@@ -1030,12 +1031,12 @@ static int nbd_co_send_reply(NBDRequestData *req, NBDReply *reply, int len)
     client->send_coroutine = qemu_coroutine_self();
 
     if (!len) {
-        ret = nbd_send_reply(client->ioc, reply);
+        ret = nbd_send_reply(client->ioc, reply, errp);
     } else {
         qio_channel_set_cork(client->ioc, true);
-        ret = nbd_send_reply(client->ioc, reply);
+        ret = nbd_send_reply(client->ioc, reply, errp);
         if (ret == 0) {
-            ret = write_sync(client->ioc, req->data, len, NULL);
+            ret = write_sync(client->ioc, req->data, len, errp);
             if (ret < 0) {
                 ret = ret;
             }
@@ -1285,8 +1286,8 @@ reply:
         local_err = NULL;
     }
 
-    if (nbd_co_send_reply(req, &reply, reply_data_len) < 0) {
-        error_setg(&local_err, "Failed to send reply");
+    if (nbd_co_send_reply(req, &reply, reply_data_len, &local_err) < 0) {
+        error_prepend(&local_err, "Failed to send reply");
         goto disconnect;
     }
 
-- 
2.11.1

Re: [Qemu-devel] [PATCH 16/19] nbd/server: add errp to nbd_send_reply()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 856 bytes --]

On 05/30/2017 09:30 AM, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  nbd/server.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 

>  
> -    if (nbd_co_send_reply(req, &reply, reply_data_len) < 0) {
> -        error_setg(&local_err, "Failed to send reply");
> +    if (nbd_co_send_reply(req, &reply, reply_data_len, &local_err) < 0) {
> +        error_prepend(&local_err, "Failed to send reply");

In the previous patch, you had a lot of conversions from LOG("message")
to error_prepend(..., "message: ") (note the addition of the ': '
suffix).  Is this addition of error_prepend going to come out right?

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH 17/19] nbd/common: nbd_tls_handshake: use error_reportf_err instead of TRACE

[Vladimir Sementsov-Ogievskiy]

Use error_reportf_err instead of TRACE in case of fail.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 nbd/common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/nbd/common.c b/nbd/common.c
index 88e0297fb2..574a551abe 100644
--- a/nbd/common.c
+++ b/nbd/common.c
@@ -102,7 +102,7 @@ void nbd_tls_handshake(QIOTask *task,
     struct NBDTLSHandshakeData *data = opaque;
 
     if (qio_task_propagate_error(task, &data->error)) {
-        TRACE("TLS failed %s", error_get_pretty(data->error));
+        error_reportf_err(data->error, "TLS failed");
     }
     data->complete = true;
     g_main_loop_quit(data->loop);
-- 
2.11.1

Re: [Qemu-devel] [PATCH 17/19] nbd/common: nbd_tls_handshake: use error_reportf_err instead of TRACE

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1342 bytes --]

On 05/30/2017 09:30 AM, Vladimir Sementsov-Ogievskiy wrote:
> Use error_reportf_err instead of TRACE in case of fail.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  nbd/common.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/nbd/common.c b/nbd/common.c
> index 88e0297fb2..574a551abe 100644
> --- a/nbd/common.c
> +++ b/nbd/common.c
> @@ -102,7 +102,7 @@ void nbd_tls_handshake(QIOTask *task,
>      struct NBDTLSHandshakeData *data = opaque;
>  
>      if (qio_task_propagate_error(task, &data->error)) {
> -        TRACE("TLS failed %s", error_get_pretty(data->error));
> +        error_reportf_err(data->error, "TLS failed");

I don't think this is right. You already populated &data->error(), which
means you have the error message available to the caller, and should let
the caller handle the message rather than blindly reporting it here
yourself (especially since if the caller also reports it, you've now
doubled up error messages).  Converting this TRACE() into a proper
tracepoint may be okay, but it may also be sufficient to just delete
this TRACE() since the caller should already be handling the failure.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH 18/19] nbd/client: refactor TRACE of NBD_MAGIC

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 nbd/client.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/nbd/client.c b/nbd/client.c
index 3d15596120..52f7981c9c 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -426,6 +426,21 @@ static QIOChannel *nbd_receive_starttls(QIOChannel *ioc,
     return QIO_CHANNEL(tioc);
 }
 
+static const char *nbd_magic_to_string(char *out, const char *in,
+                                       size_t count)
+{
+    size_t i;
+
+    for (i = 0; i < count; ++i) {
+        if (in[i] == '\0') {
+            out[i] = '\0';
+            break;
+        }
+        out[i] = qemu_isprint(in[i]) ? in[i] : '.';
+    }
+
+    return out;
+}
 
 int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
                           QCryptoTLSCreds *tlscreds, const char *hostname,
@@ -433,6 +448,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
                           off_t *size, Error **errp)
 {
     char buf[256];
+    char print_buf[256];
     uint64_t magic, s;
     int rc;
     bool zeroes = true;
@@ -461,15 +477,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
         goto fail;
     }
 
-    TRACE("Magic is %c%c%c%c%c%c%c%c",
-          qemu_isprint(buf[0]) ? buf[0] : '.',
-          qemu_isprint(buf[1]) ? buf[1] : '.',
-          qemu_isprint(buf[2]) ? buf[2] : '.',
-          qemu_isprint(buf[3]) ? buf[3] : '.',
-          qemu_isprint(buf[4]) ? buf[4] : '.',
-          qemu_isprint(buf[5]) ? buf[5] : '.',
-          qemu_isprint(buf[6]) ? buf[6] : '.',
-          qemu_isprint(buf[7]) ? buf[7] : '.');
+    TRACE("Magic is %s", nbd_magic_to_string(print_buf, buf, 9));
 
     if (memcmp(buf, "NBDMAGIC", 8) != 0) {
         error_setg(errp, "Invalid magic received");
-- 
2.11.1

Re: [Qemu-devel] [PATCH 18/19] nbd/client: refactor TRACE of NBD_MAGIC

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1965 bytes --]

On 05/30/2017 09:30 AM, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  nbd/client.c | 26 +++++++++++++++++---------
>  1 file changed, 17 insertions(+), 9 deletions(-)
> 
> diff --git a/nbd/client.c b/nbd/client.c
> index 3d15596120..52f7981c9c 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -426,6 +426,21 @@ static QIOChannel *nbd_receive_starttls(QIOChannel *ioc,
>      return QIO_CHANNEL(tioc);
>  }
>  
> +static const char *nbd_magic_to_string(char *out, const char *in,
> +                                       size_t count)
> +{
> +    size_t i;
> +
> +    for (i = 0; i < count; ++i) {
> +        if (in[i] == '\0') {
> +            out[i] = '\0';
> +            break;
> +        }
> +        out[i] = qemu_isprint(in[i]) ? in[i] : '.';
> +    }
> +
> +    return out;
> +}

Do we really need this?

> -    TRACE("Magic is %c%c%c%c%c%c%c%c",
> -          qemu_isprint(buf[0]) ? buf[0] : '.',
> -          qemu_isprint(buf[1]) ? buf[1] : '.',
> -          qemu_isprint(buf[2]) ? buf[2] : '.',
> -          qemu_isprint(buf[3]) ? buf[3] : '.',
> -          qemu_isprint(buf[4]) ? buf[4] : '.',
> -          qemu_isprint(buf[5]) ? buf[5] : '.',
> -          qemu_isprint(buf[6]) ? buf[6] : '.',
> -          qemu_isprint(buf[7]) ? buf[7] : '.');
> +    TRACE("Magic is %s", nbd_magic_to_string(print_buf, buf, 9));

Would it be any simpler to just print the received magic as a 64-bit
number in hex, instead of trying to string-ize it?  Either the client
sends the right magic (the usual case), so we don't need to waste time
string-izing it, or the client is bogus and sent us garbage (where
printing a hex value is just as efficient as string-izing things to see
what the garbage actually was).

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 18/19] nbd/client: refactor TRACE of NBD_MAGIC

[Vladimir Sementsov-Ogievskiy]

05.06.2017 18:06, Eric Blake wrote:
> On 05/30/2017 09:30 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   nbd/client.c | 26 +++++++++++++++++---------
>>   1 file changed, 17 insertions(+), 9 deletions(-)
>>
>> diff --git a/nbd/client.c b/nbd/client.c
>> index 3d15596120..52f7981c9c 100644
>> --- a/nbd/client.c
>> +++ b/nbd/client.c
>> @@ -426,6 +426,21 @@ static QIOChannel *nbd_receive_starttls(QIOChannel *ioc,
>>       return QIO_CHANNEL(tioc);
>>   }
>>   
>> +static const char *nbd_magic_to_string(char *out, const char *in,
>> +                                       size_t count)
>> +{
>> +    size_t i;
>> +
>> +    for (i = 0; i < count; ++i) {
>> +        if (in[i] == '\0') {
>> +            out[i] = '\0';
>> +            break;
>> +        }
>> +        out[i] = qemu_isprint(in[i]) ? in[i] : '.';
>> +    }
>> +
>> +    return out;
>> +}
> Do we really need this?
>
>> -    TRACE("Magic is %c%c%c%c%c%c%c%c",
>> -          qemu_isprint(buf[0]) ? buf[0] : '.',
>> -          qemu_isprint(buf[1]) ? buf[1] : '.',
>> -          qemu_isprint(buf[2]) ? buf[2] : '.',
>> -          qemu_isprint(buf[3]) ? buf[3] : '.',
>> -          qemu_isprint(buf[4]) ? buf[4] : '.',
>> -          qemu_isprint(buf[5]) ? buf[5] : '.',
>> -          qemu_isprint(buf[6]) ? buf[6] : '.',
>> -          qemu_isprint(buf[7]) ? buf[7] : '.');
>> +    TRACE("Magic is %s", nbd_magic_to_string(print_buf, buf, 9));
> Would it be any simpler to just print the received magic as a 64-bit
> number in hex, instead of trying to string-ize it?  Either the client
> sends the right magic (the usual case), so we don't need to waste time
> string-izing it, or the client is bogus and sent us garbage (where
> printing a hex value is just as efficient as string-izing things to see
> what the garbage actually was).
>
agree.



-- 
Best regards,
Vladimir

[Qemu-devel] [PATCH 19/19] nbd: use generic trace subsystem instead of TRACE macro

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 Makefile.objs      |  1 +
 nbd/client.c       | 77 ++++++++++++++++++++++++----------------------------
 nbd/nbd-internal.h | 19 -------------
 nbd/server.c       | 79 ++++++++++++++++++++++++++----------------------------
 nbd/trace-events   | 67 +++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 141 insertions(+), 102 deletions(-)
 create mode 100644 nbd/trace-events

diff --git a/Makefile.objs b/Makefile.objs
index 6167e7b17d..3288f74af1 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -164,6 +164,7 @@ trace-events-subdirs += target/ppc
 trace-events-subdirs += qom
 trace-events-subdirs += linux-user
 trace-events-subdirs += qapi
+trace-events-subdirs += nbd
 
 trace-events-files = $(SRC_PATH)/trace-events $(trace-events-subdirs:%=$(SRC_PATH)/%/trace-events)
 
diff --git a/nbd/client.c b/nbd/client.c
index 52f7981c9c..91586f17b1 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -19,6 +19,7 @@
 
 #include "qemu/osdep.h"
 #include "qapi/error.h"
+#include "trace.h"
 #include "nbd-internal.h"
 
 static int nbd_errno_to_system_errno(int err)
@@ -44,7 +45,7 @@ static int nbd_errno_to_system_errno(int err)
         ret = ESHUTDOWN;
         break;
     default:
-        TRACE("Squashing unexpected error %d to EINVAL", err);
+        trace_nbd_unknown_error(err);
         /* fallthrough */
     case NBD_EINVAL:
         ret = EINVAL;
@@ -103,7 +104,7 @@ static int nbd_send_option_request(QIOChannel *ioc, uint32_t opt,
     if (len == -1) {
         req.length = len = strlen(data);
     }
-    TRACE("Sending option request %" PRIu32", len %" PRIu32, opt, len);
+    trace_nbd_send_option_request(opt, len);
 
     stq_be_p(&req.magic, NBD_OPTS_MAGIC);
     stl_be_p(&req.option, opt);
@@ -153,8 +154,7 @@ static int nbd_receive_option_reply(QIOChannel *ioc, uint32_t opt,
     be32_to_cpus(&reply->type);
     be32_to_cpus(&reply->length);
 
-    TRACE("Received option reply %" PRIx32", type %" PRIx32", len %" PRIu32,
-          reply->option, reply->type, reply->length);
+    trace_nbd_receive_option_reply(reply->option, reply->type, reply->length);
 
     if (reply->magic != NBD_REP_MAGIC) {
         error_setg(errp, "Unexpected option reply magic");
@@ -201,8 +201,7 @@ static int nbd_handle_reply_err(QIOChannel *ioc, nbd_opt_reply *reply,
 
     switch (reply->type) {
     case NBD_REP_ERR_UNSUP:
-        TRACE("server doesn't understand request %" PRIx32
-              ", attempting fallback", reply->option);
+        trace_nbd_reply_err_unsup(reply->option);
         result = 0;
         goto cleanup;
 
@@ -342,12 +341,12 @@ static int nbd_receive_query_exports(QIOChannel *ioc,
 {
     bool foundExport = false;
 
-    TRACE("Querying export list for '%s'", wantname);
+    trace_nbd_receive_query_exports_start(wantname);
     if (nbd_send_option_request(ioc, NBD_OPT_LIST, 0, NULL, errp) < 0) {
         return -1;
     }
 
-    TRACE("Reading available export names");
+    trace_nbd_receive_query_exports_loop();
     while (1) {
         int ret = nbd_receive_list(ioc, wantname, &foundExport, errp);
 
@@ -362,7 +361,7 @@ static int nbd_receive_query_exports(QIOChannel *ioc,
                 nbd_send_opt_abort(ioc);
                 return -1;
             }
-            TRACE("Found desired export name '%s'", wantname);
+            trace_nbd_receive_query_exports_success(wantname);
             return 0;
         }
     }
@@ -376,12 +375,12 @@ static QIOChannel *nbd_receive_starttls(QIOChannel *ioc,
     QIOChannelTLS *tioc;
     struct NBDTLSHandshakeData data = { 0 };
 
-    TRACE("Requesting TLS from server");
+    trace_nbd_receive_starttls_request();
     if (nbd_send_option_request(ioc, NBD_OPT_STARTTLS, 0, NULL, errp) < 0) {
         return NULL;
     }
 
-    TRACE("Getting TLS reply from server");
+    trace_nbd_receive_starttls_reply();
     if (nbd_receive_option_reply(ioc, NBD_OPT_STARTTLS, &reply, errp) < 0) {
         return NULL;
     }
@@ -400,14 +399,14 @@ static QIOChannel *nbd_receive_starttls(QIOChannel *ioc,
         return NULL;
     }
 
-    TRACE("TLS request approved, setting up TLS");
+    trace_nbd_receive_starttls_new_client();
     tioc = qio_channel_tls_new_client(ioc, tlscreds, hostname, errp);
     if (!tioc) {
         return NULL;
     }
     qio_channel_set_name(QIO_CHANNEL(tioc), "nbd-client-tls");
     data.loop = g_main_loop_new(g_main_context_default(), FALSE);
-    TRACE("Starting TLS handshake");
+    trace_nbd_receive_starttls_tls_handshake();
     qio_channel_tls_handshake(tioc,
                               nbd_tls_handshake,
                               &data,
@@ -453,8 +452,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
     int rc;
     bool zeroes = true;
 
-    TRACE("Receiving negotiation tlscreds=%p hostname=%s.",
-          tlscreds, hostname ? hostname : "<null>");
+    trace_nbd_receive_negotiate(tlscreds, hostname ? hostname : "<null>");
 
     rc = -EINVAL;
 
@@ -477,7 +475,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
         goto fail;
     }
 
-    TRACE("Magic is %s", nbd_magic_to_string(print_buf, buf, 9));
+    trace_nbd_receive_negotiate_magic(nbd_magic_to_string(print_buf, buf, 9));
 
     if (memcmp(buf, "NBDMAGIC", 8) != 0) {
         error_setg(errp, "Invalid magic received");
@@ -489,7 +487,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
         goto fail;
     }
     magic = be64_to_cpu(magic);
-    TRACE("Magic is 0x%" PRIx64, magic);
+    trace_nbd_receive_negotiate_magic2(magic);
 
     if (magic == NBD_OPTS_MAGIC) {
         uint32_t clientflags = 0;
@@ -501,15 +499,16 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
             goto fail;
         }
         globalflags = be16_to_cpu(globalflags);
-        TRACE("Global flags are %" PRIx32, globalflags);
+        trace_nbd_receive_negotiate_server_flags(
+            globalflags,
+            !!(globalflags & NBD_FLAG_FIXED_NEWSTYLE),
+            !!(globalflags & NBD_FLAG_NO_ZEROES));
         if (globalflags & NBD_FLAG_FIXED_NEWSTYLE) {
             fixedNewStyle = true;
-            TRACE("Server supports fixed new style");
             clientflags |= NBD_FLAG_C_FIXED_NEWSTYLE;
         }
         if (globalflags & NBD_FLAG_NO_ZEROES) {
             zeroes = false;
-            TRACE("Server supports no zeroes");
             clientflags |= NBD_FLAG_C_NO_ZEROES;
         }
         /* client requested flags */
@@ -531,7 +530,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
             }
         }
         if (!name) {
-            TRACE("Using default NBD export name \"\"");
+            trace_nbd_receive_negotiate_default_name();
             name = "";
         }
         if (fixedNewStyle) {
@@ -580,7 +579,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
             goto fail;
         }
         *size = be64_to_cpu(s);
-        TRACE("Size is %" PRIu64, *size);
+        trace_nbd_receive_negotiate_export_size(*size);
 
         if (read_sync(ioc, &oldflags, sizeof(oldflags), errp) < 0) {
             error_prepend(errp, "Failed to read export flags");
@@ -597,7 +596,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
         goto fail;
     }
 
-    TRACE("Size is %" PRIu64 ", export flags %" PRIx16, *size, *flags);
+    trace_nbd_receive_negotiate_size_flags(*size, *flags);
     if (zeroes && drop_sync(ioc, 124, errp) < 0) {
         error_prepend(errp, "Failed to read reserved block");
         goto fail;
@@ -619,7 +618,7 @@ int nbd_init(int fd, QIOChannelSocket *sioc, uint16_t flags, off_t size,
         return -E2BIG;
     }
 
-    TRACE("Setting NBD socket");
+    trace_nbd_init_set_socket();
 
     if (ioctl(fd, NBD_SET_SOCK, (unsigned long) sioc->fd) < 0) {
         int serrno = errno;
@@ -627,7 +626,7 @@ int nbd_init(int fd, QIOChannelSocket *sioc, uint16_t flags, off_t size,
         return -serrno;
     }
 
-    TRACE("Setting block size to %lu", (unsigned long)BDRV_SECTOR_SIZE);
+    trace_nbd_init_set_block_size(BDRV_SECTOR_SIZE);
 
     if (ioctl(fd, NBD_SET_BLKSIZE, (unsigned long)BDRV_SECTOR_SIZE) < 0) {
         int serrno = errno;
@@ -635,10 +634,9 @@ int nbd_init(int fd, QIOChannelSocket *sioc, uint16_t flags, off_t size,
         return -serrno;
     }
 
-    TRACE("Setting size to %lu block(s)", sectors);
+    trace_nbd_init_set_size(sectors);
     if (size % BDRV_SECTOR_SIZE) {
-        TRACE("Ignoring trailing %d bytes of export",
-              (int) (size % BDRV_SECTOR_SIZE));
+        trace_nbd_init_trailing_bytes(size % BDRV_SECTOR_SIZE);
     }
 
     if (ioctl(fd, NBD_SET_SIZE_BLOCKS, sectors) < 0) {
@@ -650,7 +648,7 @@ int nbd_init(int fd, QIOChannelSocket *sioc, uint16_t flags, off_t size,
     if (ioctl(fd, NBD_SET_FLAGS, (unsigned long) flags) < 0) {
         if (errno == ENOTTY) {
             int read_only = (flags & NBD_FLAG_READ_ONLY) != 0;
-            TRACE("Setting readonly attribute");
+            trace_nbd_init_set_readonly();
 
             if (ioctl(fd, BLKROSET, (unsigned long) &read_only) < 0) {
                 int serrno = errno;
@@ -664,7 +662,7 @@ int nbd_init(int fd, QIOChannelSocket *sioc, uint16_t flags, off_t size,
         }
     }
 
-    TRACE("Negotiation ended");
+    trace_nbd_init_finish();
 
     return 0;
 }
@@ -674,7 +672,7 @@ int nbd_client(int fd)
     int ret;
     int serrno;
 
-    TRACE("Doing NBD loop");
+    trace_nbd_client_loop();
 
     ret = ioctl(fd, NBD_DO_IT);
     if (ret < 0 && errno == EPIPE) {
@@ -686,12 +684,12 @@ int nbd_client(int fd)
     }
     serrno = errno;
 
-    TRACE("NBD loop returned %d: %s", ret, strerror(serrno));
+    trace_nbd_client_loop_ret(ret, strerror(serrno));
 
-    TRACE("Clearing NBD queue");
+    trace_nbd_client_clear_queue();
     ioctl(fd, NBD_CLEAR_QUE);
 
-    TRACE("Clearing NBD socket");
+    trace_nbd_client_clear_socket();
     ioctl(fd, NBD_CLEAR_SOCK);
 
     errno = serrno;
@@ -728,11 +726,8 @@ ssize_t nbd_send_request(QIOChannel *ioc, NBDRequest *request)
 {
     uint8_t buf[NBD_REQUEST_SIZE];
 
-    TRACE("Sending request to server: "
-          "{ .from = %" PRIu64", .len = %" PRIu32 ", .handle = %" PRIu64
-          ", .flags = %" PRIx16 ", .type = %" PRIu16 " }",
-          request->from, request->len, request->handle,
-          request->flags, request->type);
+    trace_nbd_send_request(request->from, request->len, request->handle,
+                           request->flags, request->type);
 
     stl_be_p(buf, NBD_REQUEST_MAGIC);
     stw_be_p(buf + 4, request->flags);
@@ -777,9 +772,7 @@ ssize_t nbd_receive_reply(QIOChannel *ioc, NBDReply *reply, Error **errp)
         error_setg(errp, "server shutting down");
         return -EINVAL;
     }
-    TRACE("Got reply: { magic = 0x%" PRIx32 ", .error = % " PRId32
-          ", handle = %" PRIu64" }",
-          magic, reply->error, reply->handle);
+    trace_nbd_receive_reply(magic, reply->error, reply->handle);
 
     if (magic != NBD_REPLY_MAGIC) {
         error_setg(errp, "invalid magic (got 0x%" PRIx32 ")", magic);
diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h
index 01d5778679..d2ce14f08d 100644
--- a/nbd/nbd-internal.h
+++ b/nbd/nbd-internal.h
@@ -31,25 +31,6 @@
 #include "qemu/queue.h"
 #include "qemu/main-loop.h"
 
-/* #define DEBUG_NBD */
-
-#ifdef DEBUG_NBD
-#define DEBUG_NBD_PRINT 1
-#else
-#define DEBUG_NBD_PRINT 0
-#endif
-
-#define TRACE(msg, ...) do { \
-    if (DEBUG_NBD_PRINT) { \
-        LOG(msg, ## __VA_ARGS__); \
-    } \
-} while (0)
-
-#define LOG(msg, ...) do { \
-    fprintf(stderr, "%s:%s():L%d: " msg "\n", \
-            __FILE__, __FUNCTION__, __LINE__, ## __VA_ARGS__); \
-} while (0)
-
 /* This is all part of the "official" NBD API.
  *
  * The most up-to-date documentation is available at:
diff --git a/nbd/server.c b/nbd/server.c
index 6c0702287e..64bc577aea 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -19,6 +19,7 @@
 
 #include "qemu/osdep.h"
 #include "qapi/error.h"
+#include "trace.h"
 #include "nbd-internal.h"
 
 static int system_errno_to_nbd_errno(int err)
@@ -139,8 +140,7 @@ static int nbd_negotiate_send_rep_len(QIOChannel *ioc, uint32_t type,
     int ret;
     uint64_t magic;
 
-    TRACE("Reply opt=%" PRIx32 " type=%" PRIx32 " len=%" PRIu32,
-          type, opt, len);
+    trace_nbd_negotiate_send_rep_len(type, opt, len);
 
     magic = cpu_to_be64(NBD_REP_MAGIC);
     ret = write_sync(ioc, &magic, sizeof(magic), errp);
@@ -196,7 +196,7 @@ nbd_negotiate_send_rep_err(QIOChannel *ioc, uint32_t type,
     va_end(va);
     len = strlen(msg);
     assert(len < 4096);
-    TRACE("sending error message \"%s\"", msg);
+    trace_nbd_negotiate_send_rep_err(msg);
     ret = nbd_negotiate_send_rep_len(ioc, type, opt, len, errp);
     if (ret < 0) {
         goto out;
@@ -223,7 +223,7 @@ static int nbd_negotiate_send_rep_list(QIOChannel *ioc, NBDExport *exp,
     const char *desc = exp->description ? exp->description : "";
     int ret;
 
-    TRACE("Advertising export name '%s' description '%s'", name, desc);
+    trace_nbd_negotiate_send_rep_list(name, desc);
     name_len = strlen(name);
     desc_len = strlen(desc);
     len = name_len + desc_len + sizeof(len);
@@ -294,7 +294,7 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, uint32_t length,
     /* Client sends:
         [20 ..  xx]   export name (length bytes)
      */
-    TRACE("Checking length");
+    trace_nbd_negotiate_handle_export_name();
     if (length >= sizeof(name)) {
         error_setg(errp, "Bad length received");
         return -EINVAL;
@@ -307,7 +307,7 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, uint32_t length,
     }
     name[length] = '\0';
 
-    TRACE("Client requested export '%s'", name);
+    trace_nbd_negotiate_handle_export_name_request(name);
 
     client->exp = nbd_export_find(name);
     if (!client->exp) {
@@ -331,7 +331,7 @@ static QIOChannel *nbd_negotiate_handle_starttls(NBDClient *client,
     QIOChannelTLS *tioc;
     struct NBDTLSHandshakeData data = { 0 };
 
-    TRACE("Setting up TLS");
+    trace_nbd_negotiate_handle_starttls();
     ioc = client->ioc;
     if (length) {
         if (drop_sync(ioc, length, errp) < 0) {
@@ -357,7 +357,7 @@ static QIOChannel *nbd_negotiate_handle_starttls(NBDClient *client,
     }
 
     qio_channel_set_name(QIO_CHANNEL(tioc), "nbd-server-tls");
-    TRACE("Starting TLS handshake");
+    trace_nbd_negotiate_handle_starttls_handshake();
     data.loop = g_main_loop_new(g_main_context_default(), FALSE);
     qio_channel_tls_handshake(tioc,
                               nbd_tls_handshake,
@@ -410,15 +410,15 @@ static int nbd_negotiate_options(NBDClient *client, Error **errp)
         error_prepend(errp, "read failed: ");
         return ret;
     }
-    TRACE("Checking client flags");
+    trace_nbd_negotiate_options_flags();
     be32_to_cpus(&flags);
     if (flags & NBD_FLAG_C_FIXED_NEWSTYLE) {
-        TRACE("Client supports fixed newstyle handshake");
+        trace_nbd_negotiate_options_newstyle();
         fixedNewstyle = true;
         flags &= ~NBD_FLAG_C_FIXED_NEWSTYLE;
     }
     if (flags & NBD_FLAG_C_NO_ZEROES) {
-        TRACE("Client supports no zeroes at handshake end");
+        trace_nbd_negotiate_options_no_zeroes();
         client->no_zeroes = true;
         flags &= ~NBD_FLAG_C_NO_ZEROES;
     }
@@ -437,7 +437,7 @@ static int nbd_negotiate_options(NBDClient *client, Error **errp)
             error_prepend(errp, "read failed: ");
             return ret;
         }
-        TRACE("Checking opts magic");
+        trace_nbd_negotiate_options_check_magic();
         if (magic != be64_to_cpu(NBD_OPTS_MAGIC)) {
             error_setg(errp, "Bad magic received");
             return -EINVAL;
@@ -457,7 +457,7 @@ static int nbd_negotiate_options(NBDClient *client, Error **errp)
         }
         length = be32_to_cpu(length);
 
-        TRACE("Checking option 0x%" PRIx32, clientflags);
+        trace_nbd_negotiate_options_check_option(clientflags);
         if (client->tlscreds &&
             client->ioc == (QIOChannel *)client->sioc) {
             QIOChannel *tioc;
@@ -617,14 +617,14 @@ static coroutine_fn int nbd_negotiate(NBDClient *client, Error **errp)
 
     qio_channel_set_blocking(client->ioc, false, NULL);
 
-    TRACE("Beginning negotiation.");
+    trace_nbd_negotiate_begin();
     memset(buf, 0, sizeof(buf));
     memcpy(buf, "NBDMAGIC", 8);
 
     oldStyle = client->exp != NULL && !client->tlscreds;
     if (oldStyle) {
-        TRACE("advertising size %" PRIu64 " and flags %x",
-              client->exp->size, client->exp->nbdflags | myflags);
+        trace_nbd_negotiate_old_style(client->exp->size,
+                                      client->exp->nbdflags | myflags);
         stq_be_p(buf + 8, NBD_CLIENT_MAGIC);
         stq_be_p(buf + 16, client->exp->size);
         stw_be_p(buf + 26, client->exp->nbdflags | myflags);
@@ -658,8 +658,8 @@ static coroutine_fn int nbd_negotiate(NBDClient *client, Error **errp)
             return ret;
         }
 
-        TRACE("advertising size %" PRIu64 " and flags %x",
-              client->exp->size, client->exp->nbdflags | myflags);
+        trace_nbd_negotiate_new_style_size_flags(
+            client->exp->size, client->exp->nbdflags | myflags);
         stq_be_p(buf + 18, client->exp->size);
         stw_be_p(buf + 26, client->exp->nbdflags | myflags);
         len = client->no_zeroes ? 10 : sizeof(buf) - 18;
@@ -670,7 +670,7 @@ static coroutine_fn int nbd_negotiate(NBDClient *client, Error **errp)
         }
     }
 
-    TRACE("Negotiation succeeded.");
+    trace_nbd_negotiate_success();
 
     return 0;
 }
@@ -703,9 +703,8 @@ static int nbd_receive_request(QIOChannel *ioc, NBDRequest *request,
     request->from   = ldq_be_p(buf + 16);
     request->len    = ldl_be_p(buf + 24);
 
-    TRACE("Got request: { magic = 0x%" PRIx32 ", .flags = %" PRIx16
-          ", .type = %" PRIx16 ", from = %" PRIu64 ", len = %" PRIu32 " }",
-          magic, request->flags, request->type, request->from, request->len);
+    trace_nbd_receive_request(magic, request->flags, request->type,
+                              request->from, request->len);
 
     if (magic != NBD_REQUEST_MAGIC) {
         error_setg(errp, "invalid magic (got 0x%" PRIx32 ")", magic);
@@ -720,9 +719,7 @@ static int nbd_send_reply(QIOChannel *ioc, NBDReply *reply, Error **errp)
 
     reply->error = system_errno_to_nbd_errno(reply->error);
 
-    TRACE("Sending response to client: { .error = %" PRId32
-          ", handle = %" PRIu64 " }",
-          reply->error, reply->handle);
+    trace_nbd_send_reply(reply->error, reply->handle);
 
     /* Reply
        [ 0 ..  3]    magic   (NBD_REPLY_MAGIC)
@@ -819,7 +816,7 @@ static void blk_aio_attached(AioContext *ctx, void *opaque)
     NBDExport *exp = opaque;
     NBDClient *client;
 
-    TRACE("Export %s: Attaching clients to AIO context %p\n", exp->name, ctx);
+    trace_blk_aio_attached(exp->name, ctx);
 
     exp->ctx = ctx;
 
@@ -839,7 +836,7 @@ static void blk_aio_detach(void *opaque)
     NBDExport *exp = opaque;
     NBDClient *client;
 
-    TRACE("Export %s: Detaching clients from AIO context %p\n", exp->name, exp->ctx);
+    trace_blk_aio_detach(exp->name, exp->ctx);
 
     QTAILQ_FOREACH(client, &exp->clients, next) {
         qio_channel_detach_aio_context(client->ioc);
@@ -1066,7 +1063,7 @@ static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request,
         return -EIO;
     }
 
-    TRACE("Decoding type");
+    trace_nbd_co_receive_request_decode_type();
 
     if (request->type != NBD_CMD_WRITE) {
         /* No payload, we are ready to read the next request.  */
@@ -1076,7 +1073,7 @@ static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request,
     if (request->type == NBD_CMD_DISC) {
         /* Special case: we're going to disconnect without a reply,
          * whether or not flags, from, or len are bogus */
-        TRACE("Request type is DISCONNECT");
+        trace_nbd_co_receive_request_disconnect();
         return -EIO;
     }
 
@@ -1103,7 +1100,7 @@ static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request,
         }
     }
     if (request->type == NBD_CMD_WRITE) {
-        TRACE("Reading %" PRIu32 " byte(s)", request->len);
+        trace_nbd_co_receive_request_cmd_write(request->len);
 
         if (read_sync(client->ioc, req->data, request->len, errp) < 0) {
             error_prepend(errp, "reading from socket failed: ");
@@ -1145,7 +1142,7 @@ static coroutine_fn void nbd_trip(void *opaque)
     int reply_data_len = 0;
     Error *local_err = NULL;
 
-    TRACE("Reading request.");
+    trace_do_nbd_trip();
     if (client->closing) {
         nbd_client_put(client);
         return;
@@ -1177,7 +1174,7 @@ static coroutine_fn void nbd_trip(void *opaque)
 
     switch (request.type) {
     case NBD_CMD_READ:
-        TRACE("Request type is READ");
+        trace_do_nbd_trip_cmd_read();
 
         /* XXX: NBD Protocol only documents use of FUA with WRITE */
         if (request.flags & NBD_CMD_FLAG_FUA) {
@@ -1198,19 +1195,19 @@ static coroutine_fn void nbd_trip(void *opaque)
         }
 
         reply_data_len = request.len;
-        TRACE("Read %" PRIu32" byte(s)", request.len);
+        trace_do_nbd_trip_read(request.len);
 
         break;
     case NBD_CMD_WRITE:
-        TRACE("Request type is WRITE");
+        trace_do_nbd_trip_cmd_write();
 
         if (exp->nbdflags & NBD_FLAG_READ_ONLY) {
-            TRACE("Server is read-only, return error");
+            trace_do_nbd_trip_cmd_write_readonly();
             reply.error = EROFS;
             break;
         }
 
-        TRACE("Writing to device");
+        trace_do_nbd_trip_write();
 
         flags = 0;
         if (request.flags & NBD_CMD_FLAG_FUA) {
@@ -1225,7 +1222,7 @@ static coroutine_fn void nbd_trip(void *opaque)
 
         break;
     case NBD_CMD_WRITE_ZEROES:
-        TRACE("Request type is WRITE_ZEROES");
+        trace_do_nbd_trip_cmd_write_zeroes();
 
         if (exp->nbdflags & NBD_FLAG_READ_ONLY) {
             error_setg(&local_err, "Server is read-only, return error");
@@ -1233,7 +1230,7 @@ static coroutine_fn void nbd_trip(void *opaque)
             break;
         }
 
-        TRACE("Writing to device");
+        trace_do_nbd_trip_write_zeroes();
 
         flags = 0;
         if (request.flags & NBD_CMD_FLAG_FUA) {
@@ -1255,7 +1252,7 @@ static coroutine_fn void nbd_trip(void *opaque)
         abort();
 
     case NBD_CMD_FLUSH:
-        TRACE("Request type is FLUSH");
+        trace_do_nbd_trip_cmd_flush();
 
         ret = blk_co_flush(exp->blk);
         if (ret < 0) {
@@ -1265,7 +1262,7 @@ static coroutine_fn void nbd_trip(void *opaque)
 
         break;
     case NBD_CMD_TRIM:
-        TRACE("Request type is TRIM");
+        trace_do_nbd_trip_cmd_trim();
         ret = blk_co_pdiscard(exp->blk, request.from + exp->dev_offset,
                               request.len);
         if (ret < 0) {
@@ -1299,7 +1296,7 @@ reply:
         goto disconnect;
     }
 
-    TRACE("Request/Reply complete");
+    trace_do_nbd_trip_complete();
 
 done:
     nbd_request_put(req);
diff --git a/nbd/trace-events b/nbd/trace-events
new file mode 100644
index 0000000000..82e45a1a27
--- /dev/null
+++ b/nbd/trace-events
@@ -0,0 +1,67 @@
+# nbd/client.c
+nbd_unknown_error(int err) "Squashing unexpected error %d to EINVAL"
+nbd_send_option_request(uint32_t opt, uint32_t len) "Sending option request %" PRIu32", len %" PRIu32
+nbd_receive_option_reply(uint32_t option, uint32_t type, uint32_t length) "Received option reply %" PRIx32", type %" PRIx32", len %" PRIu32
+nbd_reply_err_unsup(uint32_t option) "server doesn't understand request %" PRIx32 ", attempting fallback"
+nbd_receive_query_exports_start(const char *wantname) "Querying export list for '%s'"
+nbd_receive_query_exports_loop(void) "Reading available export names"
+nbd_receive_query_exports_success(const char *wantname) "Found desired export name '%s'"
+nbd_receive_starttls_request(void) "Requesting TLS from server"
+nbd_receive_starttls_reply(void) "Getting TLS reply from server"
+nbd_receive_starttls_new_client(void) "TLS request approved, setting up TLS"
+nbd_receive_starttls_tls_handshake(void) "Starting TLS handshake"
+nbd_receive_negotiate(void *tlscreds, const char *hostname) "Receiving negotiation tlscreds=%p hostname=%s."
+nbd_receive_negotiate_magic(const char *magic) "Magic is %s"
+nbd_receive_negotiate_magic2(uint64_t magic) "Magic is 0x%" PRIx64
+nbd_receive_negotiate_server_flags(uint32_t globalflags, int fixed_newstyle, int no_zeroes) "Global flags are %d" PRIx32 " fixed_newstyle %d no_zeroes %d"
+nbd_receive_negotiate_default_name(void) "Using default NBD export name \"\""
+nbd_receive_negotiate_export_size(uint64_t size) "Size is %" PRIu64
+nbd_receive_negotiate_size_flags(uint64_t size, uint16_t flags) "Size is %" PRIu64 ", export flags %" PRIx16
+nbd_init_set_socket(void) "Setting NBD socket"
+nbd_init_set_block_size(unsigned long block_size) "Setting block size to %lu"
+nbd_init_set_size(unsigned long sectors) "Setting size to %lu block(s)"
+nbd_init_trailing_bytes(int ignored_bytes) "Ignoring trailing %d bytes of export"
+nbd_init_set_readonly(void) "Setting readonly attribute"
+nbd_init_finish(void) "Negotiation ended"
+nbd_client_loop(void) "Doing NBD loop"
+nbd_client_loop_ret(int ret, const char *error) "NBD loop returned %d: %s"
+nbd_client_clear_queue(void) "Clearing NBD queue"
+nbd_client_clear_socket(void) "Clearing NBD socket"
+nbd_send_request(uint64_t from, uint32_t len, uint64_t handle, uint16_t flags, uint16_t type) "Sending request to server: { .from = %" PRIu64", .len = %" PRIu32 ", .handle = %" PRIu64 ", .flags = %" PRIx16 ", .type = %" PRIu16 " }"
+nbd_receive_reply(uint32_t magic, int32_t error, uint64_t handle) "Got reply: { magic = 0x%" PRIx32 ", .error = % " PRId32 ", handle = %" PRIu64" }"
+
+# nbd/server.c
+nbd_negotiate_send_rep_len(uint32_t type, uint32_t opt, uint32_t len) "Reply opt=%" PRIx32 " type=%" PRIx32 " len=%" PRIu32
+nbd_negotiate_send_rep_err(const char *msg) "sending error message \"%s\""
+nbd_negotiate_send_rep_list(const char *name, const char *desc) "Advertising export name '%s' description '%s'"
+nbd_negotiate_handle_export_name(void) "Checking length"
+nbd_negotiate_handle_export_name_request(const char *name) "Client requested export '%s'"
+nbd_negotiate_handle_starttls(void) "Setting up TLS"
+nbd_negotiate_handle_starttls_handshake(void) "Starting TLS handshake"
+nbd_negotiate_options_flags(void) "Checking client flags"
+nbd_negotiate_options_newstyle(void) "Client supports fixed newstyle handshake"
+nbd_negotiate_options_no_zeroes(void) "Client supports no zeroes at handshake end"
+nbd_negotiate_options_check_magic(void) "Checking opts magic"
+nbd_negotiate_options_check_option(uint32_t clientflags) "Checking option 0x%" PRIx32
+nbd_negotiate_begin(void) "Beginning negotiation."
+nbd_negotiate_old_style(uint64_t size, unsigned flags) "advertising size %" PRIu64 " and flags %x"
+nbd_negotiate_new_style_size_flags(uint64_t size, unsigned flags) "advertising size %" PRIu64 " and flags %x"
+nbd_negotiate_success(void) "Negotiation succeeded."
+nbd_receive_request(uint32_t magic, uint16_t flags, uint16_t type, uint64_t from, uint32_t len) "Got request: { magic = 0x%" PRIx32 ", .flags = %" PRIx16 ", .type = %" PRIx16 ", from = %" PRIu64 ", len = %" PRIu32 " }"
+nbd_send_reply(int32_t error, uint64_t handle) "Sending response to client: { .error = %" PRId32 ", handle = %" PRIu64 " }"
+blk_aio_attached(const char *name, void *ctx) "Export %s: Attaching clients to AIO context %p\n"
+blk_aio_detach(const char *name, void *ctx) "Export %s: Detaching clients from AIO context %p\n"
+nbd_co_receive_request_decode_type(void) "Decoding type"
+nbd_co_receive_request_disconnect(void) "Request type is DISCONNECT"
+nbd_co_receive_request_cmd_write(uint32_t len) "Reading %" PRIu32 " byte(s)"
+do_nbd_trip(void) "Reading request."
+do_nbd_trip_cmd_read(void) "Request type is READ"
+do_nbd_trip_read(uint32_t len) "Read %" PRIu32" byte(s)"
+do_nbd_trip_cmd_write(void) "Request type is WRITE"
+do_nbd_trip_cmd_write_readonly(void) "Server is read-only, return error"
+do_nbd_trip_write(void) "Writing to device"
+do_nbd_trip_cmd_write_zeroes(void) "Request type is WRITE_ZEROES"
+do_nbd_trip_write_zeroes(void) "Writing to device"
+do_nbd_trip_cmd_flush(void) "Request type is FLUSH"
+do_nbd_trip_cmd_trim(void) "Request type is TRIM"
+do_nbd_trip_complete(void) "Request/Reply complete"
-- 
2.11.1

Re: [Qemu-devel] [PATCH 19/19] nbd: use generic trace subsystem instead of TRACE macro

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 4791 bytes --]

[adding Stefan as trace maintainer]

On 05/30/2017 09:30 AM, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  Makefile.objs      |  1 +
>  nbd/client.c       | 77 ++++++++++++++++++++++++----------------------------
>  nbd/nbd-internal.h | 19 -------------
>  nbd/server.c       | 79 ++++++++++++++++++++++++++----------------------------
>  nbd/trace-events   | 67 +++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 141 insertions(+), 102 deletions(-)
>  create mode 100644 nbd/trace-events

I like where you're headed with this one.

Can you please include an example command line usage in the commit
message of how you are enabling the traces (the old way was to recompile
with CFLAGS=-DDEBUG_NBD; the new way no longer requires recompilation,
but listing a formula for easy tracing as part of the commit message
can't hurt).  And if I'm correct, we already have the same command-line
tracing framework hooked up for all of qemu-nbd, qemu-io, and qemu
proper, right?

> @@ -453,8 +452,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
>      int rc;
>      bool zeroes = true;
>  
> -    TRACE("Receiving negotiation tlscreds=%p hostname=%s.",

Most trace messages don't have a trailing '.'; you can remove this one
as part of moving it to trace-events.

> @@ -477,7 +475,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
>          goto fail;
>      }
>  
> -    TRACE("Magic is %s", nbd_magic_to_string(print_buf, buf, 9));
> +    trace_nbd_receive_negotiate_magic(nbd_magic_to_string(print_buf, buf, 9));
>  
>      if (memcmp(buf, "NBDMAGIC", 8) != 0) {

See my review on 18/19; I think it is sufficient to trace an 8-byte hex
number, instead of trying to string-ize things.  Yeah, the magic number
happens to be an ASCII string, but treating it as a number is no real
loss of information.

>          error_setg(errp, "Invalid magic received");
> @@ -489,7 +487,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
>          goto fail;
>      }
>      magic = be64_to_cpu(magic);
> -    TRACE("Magic is 0x%" PRIx64, magic);
> +    trace_nbd_receive_negotiate_magic2(magic);

In fact, I think you only need one trace function for tracing an 8-byte
magic number, that can be called from more than one spot.

> @@ -501,15 +499,16 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
>              goto fail;
>          }
>          globalflags = be16_to_cpu(globalflags);
> -        TRACE("Global flags are %" PRIx32, globalflags);
> +        trace_nbd_receive_negotiate_server_flags(
> +            globalflags,
> +            !!(globalflags & NBD_FLAG_FIXED_NEWSTYLE),
> +            !!(globalflags & NBD_FLAG_NO_ZEROES));

Why do we have to trace particular bits within the flags, when we are
already tracing the flags as a whole?  It is just redundant information
(yes, it makes it a bit harder to read the trace to learn whether a
given flag is set when you only have the hex number, but I don't think
that is a real loss).

>          if (globalflags & NBD_FLAG_FIXED_NEWSTYLE) {
>              fixedNewStyle = true;
> -            TRACE("Server supports fixed new style");
>              clientflags |= NBD_FLAG_C_FIXED_NEWSTYLE;
>          }
>          if (globalflags & NBD_FLAG_NO_ZEROES) {
>              zeroes = false;
> -            TRACE("Server supports no zeroes");
>              clientflags |= NBD_FLAG_C_NO_ZEROES;
>          }

I agree with dropping these two TRACE() in favor of the overall trace of
the flags above (this exercise is a good place to figure out where
TRACE() was too chatty).

> @@ -580,7 +579,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
>              goto fail;
>          }
>          *size = be64_to_cpu(s);
> -        TRACE("Size is %" PRIu64, *size);
> +        trace_nbd_receive_negotiate_export_size(*size);
>  
>          if (read_sync(ioc, &oldflags, sizeof(oldflags), errp) < 0) {
>              error_prepend(errp, "Failed to read export flags");
> @@ -597,7 +596,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
>          goto fail;
>      }
>  
> -    TRACE("Size is %" PRIu64 ", export flags %" PRIx16, *size, *flags);
> +    trace_nbd_receive_negotiate_size_flags(*size, *flags);

Can we share the same trace for these two callers, by having the first
one use trace_nbd_receive_negotiate_size_flags(*size, 0)?

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 19/19] nbd: use generic trace subsystem instead of TRACE macro

[Vladimir Sementsov-Ogievskiy]

05.06.2017 18:23, Eric Blake wrote:
> [adding Stefan as trace maintainer]
>
> On 05/30/2017 09:30 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   Makefile.objs      |  1 +
>>   nbd/client.c       | 77 ++++++++++++++++++++++++----------------------------
>>   nbd/nbd-internal.h | 19 -------------
>>   nbd/server.c       | 79 ++++++++++++++++++++++++++----------------------------
>>   nbd/trace-events   | 67 +++++++++++++++++++++++++++++++++++++++++++++
>>   5 files changed, 141 insertions(+), 102 deletions(-)

[...]

>>           goto fail;
>>       }
>>       magic = be64_to_cpu(magic);
>> -    TRACE("Magic is 0x%" PRIx64, magic);
>> +    trace_nbd_receive_negotiate_magic2(magic);
> In fact, I think you only need one trace function for tracing an 8-byte
> magic number, that can be called from more than one spot.

You've touched interesting point: is it normal to create 
trace-functions, used as different trace-points? So should 
trace-point-name describe point in the program or not? I've thought it 
should.

>
>> @@ -501,15 +499,16 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
>>               goto fail;
>>           }
>>           globalflags = be16_to_cpu(globalflags);
>> -        TRACE("Global flags are %" PRIx32, globalflags);
>> +        trace_nbd_receive_negotiate_server_flags(
>> +            globalflags,
>> +            !!(globalflags & NBD_FLAG_FIXED_NEWSTYLE),
>> +            !!(globalflags & NBD_FLAG_NO_ZEROES));
> Why do we have to trace particular b


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 19/19] nbd: use generic trace subsystem instead of TRACE macro

[Vladimir Sementsov-Ogievskiy]

I'll update this (with removed 11-13) after "nbd refactoring part 1" 
pushed (actually, the first part of these series).

05.06.2017 18:23, Eric Blake wrote:
> [adding Stefan as trace maintainer]
>
> On 05/30/2017 09:30 AM, Vladimir Sementsov-Ogievskiy wrote:
[...]


-- 
Best regards,
Vladimir