diff mbox

[v5,06/14] nbd: Send message along with server NBD_REP_ERR errors

Message ID 1468901281-22858-7-git-send-email-eblake@redhat.com
State New
Headers show

Commit Message

Eric Blake July 19, 2016, 4:07 a.m. UTC 
The NBD Protocol allows us to send human-readable messages
along with any NBD_REP_ERR error during option negotiation;
make use of this fact for clients that know what to do with
our message.

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

---
v5: don't leak 'msg'
v4: new patch
---
 nbd/server.c | 78 +++++++++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 59 insertions(+), 19 deletions(-)

Comments

Fam Zheng July 19, 2016, 5:15 a.m. UTC | #1 
On Mon, 07/18 22:07, Eric Blake wrote:
>  nbd/server.c | 78 +++++++++++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 59 insertions(+), 19 deletions(-)
> 
> diff --git a/nbd/server.c b/nbd/server.c
> index c8716f1..ad31c4a 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -235,6 +235,38 @@ static int nbd_negotiate_send_rep(QIOChannel *ioc, uint32_t type, uint32_t opt)
>      return nbd_negotiate_send_rep_len(ioc, type, opt, 0);
>  }
> 
> +/* Send an error reply.
> + * Return -errno to kill connection, 0 to continue negotiation. */
> +static int GCC_FMT_ATTR(4, 5)
> +    nbd_negotiate_send_rep_err(QIOChannel *ioc, uint32_t type,

Isn't the function name supposed to be place at col 0?

> +                               uint32_t opt, const char *fmt, ...)
Eric Blake Oct. 11, 2016, 3:12 p.m. UTC | #2 
On 07/19/2016 12:15 AM, Fam Zheng wrote:
> On Mon, 07/18 22:07, Eric Blake wrote:
>>  nbd/server.c | 78 +++++++++++++++++++++++++++++++++++++++++++++---------------
>>  1 file changed, 59 insertions(+), 19 deletions(-)
>>
>> diff --git a/nbd/server.c b/nbd/server.c
>> index c8716f1..ad31c4a 100644
>> --- a/nbd/server.c
>> +++ b/nbd/server.c
>> @@ -235,6 +235,38 @@ static int nbd_negotiate_send_rep(QIOChannel *ioc, uint32_t type, uint32_t opt)
>>      return nbd_negotiate_send_rep_len(ioc, type, opt, 0);
>>  }
>>
>> +/* Send an error reply.
>> + * Return -errno to kill connection, 0 to continue negotiation. */
>> +static int GCC_FMT_ATTR(4, 5)
>> +    nbd_negotiate_send_rep_err(QIOChannel *ioc, uint32_t type,
> 
> Isn't the function name supposed to be place at col 0?

I blame emacs for that one.  I'm (finally) back to working on this
series, and will have a v6 posted soon addressing comments here and
elsewhere.
diff mbox

Patch

diff --git a/nbd/server.c b/nbd/server.c
index c8716f1..ad31c4a 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -235,6 +235,38 @@  static int nbd_negotiate_send_rep(QIOChannel *ioc, uint32_t type, uint32_t opt)
     return nbd_negotiate_send_rep_len(ioc, type, opt, 0);
 }

+/* Send an error reply.
+ * Return -errno to kill connection, 0 to continue negotiation. */
+static int GCC_FMT_ATTR(4, 5)
+    nbd_negotiate_send_rep_err(QIOChannel *ioc, uint32_t type,
+                               uint32_t opt, const char *fmt, ...)
+{
+    va_list va;
+    char *msg;
+    int ret;
+    size_t len;
+
+    va_start(va, fmt);
+    msg = g_strdup_vprintf(fmt, va);
+    va_end(va);
+    len = strlen(msg);
+    assert(len < 4096);
+    TRACE("sending error message \"%s\"", msg);
+    ret = nbd_negotiate_send_rep_len(ioc, type, opt, len);
+    if (ret < 0) {
+        goto out;
+    }
+    if (nbd_negotiate_write(ioc, msg, len) != len) {
+        LOG("write failed (error message)");
+        ret = -EIO;
+    } else {
+        ret = 0;
+    }
+out:
+    g_free(msg);
+    return ret;
+}
+
 /* Send an NBD_REP_SERVER reply to NBD_OPT_LIST, including payload.
  * Return -errno to kill connection, 0 to continue negotiation. */
 static int nbd_negotiate_send_rep_list(QIOChannel *ioc, NBDExport *exp)
@@ -278,8 +310,9 @@  static int nbd_negotiate_handle_list(NBDClient *client, uint32_t length)
         if (nbd_negotiate_drop_sync(client->ioc, length) != length) {
             return -EIO;
         }
-        return nbd_negotiate_send_rep(client->ioc,
-                                      NBD_REP_ERR_INVALID, NBD_OPT_LIST);
+        return nbd_negotiate_send_rep_err(client->ioc,
+                                          NBD_REP_ERR_INVALID, NBD_OPT_LIST,
+                                          "OPT_LIST should not have length");
     }

     /* For each export, send a NBD_REP_SERVER reply. */
@@ -326,7 +359,8 @@  fail:
     return rc;
 }

-
+/* 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)
 {
@@ -340,7 +374,8 @@  static QIOChannel *nbd_negotiate_handle_starttls(NBDClient *client,
         if (nbd_negotiate_drop_sync(ioc, length) != length) {
             return NULL;
         }
-        nbd_negotiate_send_rep(ioc, NBD_REP_ERR_INVALID, NBD_OPT_STARTTLS);
+        nbd_negotiate_send_rep_err(ioc, NBD_REP_ERR_INVALID, NBD_OPT_STARTTLS,
+                                   "OPT_STARTTLS should not have length");
         return NULL;
     }

@@ -468,13 +503,15 @@  static int nbd_negotiate_options(NBDClient *client)
                 return -EINVAL;

             default:
-                TRACE("Option 0x%" PRIx32 " not permitted before TLS",
-                      clientflags);
                 if (nbd_negotiate_drop_sync(client->ioc, length) != length) {
                     return -EIO;
                 }
-                ret = nbd_negotiate_send_rep(client->ioc, NBD_REP_ERR_TLS_REQD,
-                                             clientflags);
+                ret = nbd_negotiate_send_rep_err(client->ioc,
+                                                 NBD_REP_ERR_TLS_REQD,
+                                                 clientflags,
+                                                 "Option 0x%" PRIx32
+                                                 "not permitted before TLS",
+                                                 clientflags);
                 if (ret < 0) {
                     return ret;
                 }
@@ -500,27 +537,30 @@  static int nbd_negotiate_options(NBDClient *client)
                     return -EIO;
                 }
                 if (client->tlscreds) {
-                    TRACE("TLS already enabled");
-                    ret = nbd_negotiate_send_rep(client->ioc,
-                                                 NBD_REP_ERR_INVALID,
-                                                 clientflags);
+                    ret = nbd_negotiate_send_rep_err(client->ioc,
+                                                     NBD_REP_ERR_INVALID,
+                                                     clientflags,
+                                                     "TLS already enabled");
                 } else {
-                    TRACE("TLS not configured");
-                    ret = nbd_negotiate_send_rep(client->ioc,
-                                                 NBD_REP_ERR_POLICY,
-                                                 clientflags);
+                    ret = nbd_negotiate_send_rep_err(client->ioc,
+                                                     NBD_REP_ERR_POLICY,
+                                                     clientflags,
+                                                     "TLS not configured");
                 }
                 if (ret < 0) {
                     return ret;
                 }
                 break;
             default:
-                TRACE("Unsupported option 0x%" PRIx32, clientflags);
                 if (nbd_negotiate_drop_sync(client->ioc, length) != length) {
                     return -EIO;
                 }
-                ret = nbd_negotiate_send_rep(client->ioc, NBD_REP_ERR_UNSUP,
-                                             clientflags);
+                ret = nbd_negotiate_send_rep_err(client->ioc,
+                                                 NBD_REP_ERR_UNSUP,
+                                                 clientflags,
+                                                 "Unsupported option 0x%"
+                                                 PRIx32,
+                                                 clientflags);
                 if (ret < 0) {
                     return ret;
                 }