Re: [Qemu-devel] [PATCH 11/18] nbd: BLOCK_STATUS for bitmap export: server part
On 02/03/2017 06:47 PM, Vladimir Sementsov-Ogievskiy wrote:
> Only one meta context type is defined: qemu-bitmap:.
> Maximum one query is allowed for NBD_OPT_{SET,LIST}_META_CONTEXT,
> NBD_REP_ERR_TOO_BIG is returned otherwise.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
...
> +static int nbd_negotiate_opt_meta_context_start(NBDClient *client, uint32_t
> opt,
> +uint32_t length,
> +uint32_t *nb_queries,
> +BlockDriverState **bs)
> +{
> +int ret;
> +NBDExport *exp;
> +char *export_name;
> +int nb_read = 0;
> +
> +if (!client->structured_reply) {
> +uint32_t tail = length - nb_read;
> +LOG("Structured reply is not negotiated");
> +
> +if (nbd_negotiate_drop_sync(client->ioc, tail) != tail) {
> +return -EIO;
> +}
> +ret = nbd_negotiate_send_rep_err(client->ioc, NBD_REP_ERR_INVALID,
> opt,
> + "Structured reply is not
> negotiated");
> +g_free(export_name);
export_name is not initialized here! for me there is no need to free
anything here
> +
> +if (ret < 0) {
> +return ret;
> +} else {
> +*bs = NULL;
> +*nb_queries = 0;
> +return length;
> +}
> +}
> +
> +nb_read = nbd_negotiate_read_size_string(client->ioc, _name,
> + NBD_MAX_NAME_SIZE);
> +if (nb_read < 0) {
> +return nb_read;
> +}
> +
> +exp = nbd_export_find(export_name);
> +if (exp == NULL) {
> +uint32_t tail = length - nb_read;
> +LOG("export '%s' is not found", export_name);
> +
> +if (nbd_negotiate_drop_sync(client->ioc, tail) != tail) {
export_name is leaked on this path
> +return -EIO;
> +}
> +ret = nbd_negotiate_send_rep_err(client->ioc, NBD_REP_ERR_INVALID,
> opt,
> + "export '%s' is not found",
> + export_name);
> +g_free(export_name);
> +
Re: [Qemu-devel] [PATCH 11/18] nbd: BLOCK_STATUS for bitmap export: server part
On 02/03/2017 09:47 AM, Vladimir Sementsov-Ogievskiy wrote:
> Only one meta context type is defined: qemu-bitmap:.
Why 'qemu-bitmap:' instead of 'qemu:' for our namespace? I guess it's
okay, though.
> Maximum one query is allowed for NBD_OPT_{SET,LIST}_META_CONTEXT,
> NBD_REP_ERR_TOO_BIG is returned otherwise.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
This is only a high-level review; I may come back and look more closely
at details (and compare against the proposed spec) when I have more time.
> /* Request flags, sent from client to server during transmission phase */
> #define NBD_CMD_FLAG_FUA(1 << 0) /* 'force unit access' during write
> */
> @@ -142,6 +155,7 @@ enum {
> NBD_CMD_TRIM = 4,
> /* 5 reserved for failed experiment NBD_CMD_CACHE */
> NBD_CMD_WRITE_ZEROES = 6,
> +NBD_CMD_BLOCK_STATUS = 7
> };
Please keep the trailing comma (it makes it easier for later patches to
add new values without having to modify existing lines).
>
> #define NBD_DEFAULT_PORT 10809
> @@ -163,6 +177,7 @@ enum {
> #define NBD_REPLY_TYPE_NONE 0
> #define NBD_REPLY_TYPE_OFFSET_DATA 1
> #define NBD_REPLY_TYPE_OFFSET_HOLE 2
> +#define NBD_REPLY_TYPE_BLOCK_STATUS 5
> #define NBD_REPLY_TYPE_ERROR ((1 << 15) + 1)
> #define NBD_REPLY_TYPE_ERROR_OFFSET ((1 << 15) + 2)
Might be worth formatting these so that the definitions all start in the
same column (if so, it affects an earlier patch in the series as well).
>
> diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h
> index 3284bfc85a..fbbcf69925 100644
> --- a/nbd/nbd-internal.h
> +++ b/nbd/nbd-internal.h
> @@ -83,6 +83,10 @@
> #define NBD_OPT_PEEK_EXPORT (4)
> #define NBD_OPT_STARTTLS(5)
> #define NBD_OPT_STRUCTURED_REPLY (8)
> +#define NBD_OPT_LIST_META_CONTEXT (9)
> +#define NBD_OPT_SET_META_CONTEXT (10)
> +
> +#define NBD_META_NS_BITMAPS "qemu-dirty-bitmap"
I still find it odd that we've split our #defines across two different
headers, not your fault.
This name does not match your commit message. Why can't we use just
'qemu:' as our namespace?
>
> /* NBD errors are based on errno numbers, so there is a 1:1 mapping,
> * but only a limited set of errno values is specified in the protocol.
> @@ -105,6 +109,8 @@ static inline const char *nbd_opt_name(int opt)
> case NBD_OPT_PEEK_EXPORT: return "peek_export";
> case NBD_OPT_STARTTLS: return "tls";
> case NBD_OPT_STRUCTURED_REPLY: return "structured_reply";
> +case NBD_OPT_LIST_META_CONTEXT: return "list_meta_context";
> +case NBD_OPT_SET_META_CONTEXT: return "set_meta_context";
Same question as earlier as to whether this violates checkpatch
formatting, and whether it belongs in a .c instead of inline in the header.
>
>
> +static int nbd_negotiate_read_size_string(QIOChannel *ioc, char **str,
> + uint32_t max_len)
I probably would have split the creation of this helper function into
its own patch. Also, can it be utilized in any of the existing code, or
is the new code the only client?
> +
> +/* start handle LIST_META_CONTEXT and SET_META_CONTEXT requests
> + * @opt should be NBD_OPT_LIST_META_CONTEXT or
> NBD_OPT_SET_META_CONTEXT
> + * @length related option data to read
> + * @nb_queries out parameter, number of queries specified by client
> + * @bs out parameter, bs for export, selected by client
> + * will be zero if some not critical error occured and error
> reply
> + * was sent.
Wrong spelling of 'occurred', and awkward grammar. How about:
will be zero if an error reply is successfully sent
> + *
> + * Returns:
> + * Err. code < 0 on critical error
s/Err./Error/
> + * Number of bytes read otherwise (will be equal to length on non critical
> + * error or if there no queries in request)
will be equal to length if there were no errors, or no queries in the
request
> + */
> +static int nbd_negotiate_opt_meta_context_start(NBDClient *client, uint32_t
> opt,
> +uint32_t length,
> +uint32_t *nb_queries,
> +BlockDriverState **bs)
> +{
> +int ret;
> +NBDExport *exp;
> +char *export_name;
Uninitialized...[1]
> +int nb_read = 0;
> +
> +if (!client->structured_reply) {
> +uint32_t tail = length - nb_read;
> +LOG("Structured reply is not negotiated");
This LOG() is redundant...[2]
> +
> +if (nbd_negotiate_drop_sync(client->ioc, tail) != tail) {
> +return -EIO;
> +}
> +ret = nbd_negotiate_send_rep_err(client->ioc, NBD_REP_ERR_INVALID,
> opt,
> + "Structured reply is not
> negotiated");
[2]...because this function also calls LOG().
> +g_free(export_name);
[1]...so this crashes. Oops.
> +
> +if (ret < 0)
[Qemu-devel] [PATCH 11/18] nbd: BLOCK_STATUS for bitmap export: server part
Only one meta context type is defined: qemu-bitmap:.
Maximum one query is allowed for NBD_OPT_{SET,LIST}_META_CONTEXT,
NBD_REP_ERR_TOO_BIG is returned otherwise.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
include/block/nbd.h | 15 ++
nbd/nbd-internal.h | 6 +
nbd/server.c| 445
3 files changed, 466 insertions(+)
diff --git a/include/block/nbd.h b/include/block/nbd.h
index dae2e4bd03..516a24765c 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -94,6 +94,16 @@ typedef struct NBDStructuredError {
uint16_t message_length;
} QEMU_PACKED NBDStructuredError;
+typedef struct NBDStructuredMeta {
+NBDStructuredReplyChunk h;
+uint32_t context_id;
+} QEMU_PACKED NBDStructuredMeta;
+
+typedef struct NBDExtent {
+uint32_t length;
+uint32_t flags;
+} QEMU_PACKED NBDExtent;
+
/* Transmission (export) flags: sent from server to client during handshake,
but describe what will happen during transmission */
#define NBD_FLAG_HAS_FLAGS (1 << 0)/* Flags are there */
@@ -120,6 +130,7 @@ typedef struct NBDStructuredError {
#define NBD_REP_ACK (1) /* Data sending finished. */
#define NBD_REP_SERVER (2) /* Export description. */
+#define NBD_REP_META_CONTEXT(4)
#define NBD_REP_ERR_UNSUP NBD_REP_ERR(1) /* Unknown option */
#define NBD_REP_ERR_POLICY NBD_REP_ERR(2) /* Server denied */
@@ -127,6 +138,8 @@ typedef struct NBDStructuredError {
#define NBD_REP_ERR_PLATFORMNBD_REP_ERR(4) /* Not compiled in */
#define NBD_REP_ERR_TLS_REQDNBD_REP_ERR(5) /* TLS required */
#define NBD_REP_ERR_SHUTDOWNNBD_REP_ERR(7) /* Server shutting down */
+#define NBD_REP_ERR_TOO_BIG NBD_REP_ERR(9) /* The request or the reply is
+ too large to process */
/* Request flags, sent from client to server during transmission phase */
#define NBD_CMD_FLAG_FUA(1 << 0) /* 'force unit access' during write */
@@ -142,6 +155,7 @@ enum {
NBD_CMD_TRIM = 4,
/* 5 reserved for failed experiment NBD_CMD_CACHE */
NBD_CMD_WRITE_ZEROES = 6,
+NBD_CMD_BLOCK_STATUS = 7
};
#define NBD_DEFAULT_PORT 10809
@@ -163,6 +177,7 @@ enum {
#define NBD_REPLY_TYPE_NONE 0
#define NBD_REPLY_TYPE_OFFSET_DATA 1
#define NBD_REPLY_TYPE_OFFSET_HOLE 2
+#define NBD_REPLY_TYPE_BLOCK_STATUS 5
#define NBD_REPLY_TYPE_ERROR ((1 << 15) + 1)
#define NBD_REPLY_TYPE_ERROR_OFFSET ((1 << 15) + 2)
diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h
index 3284bfc85a..fbbcf69925 100644
--- a/nbd/nbd-internal.h
+++ b/nbd/nbd-internal.h
@@ -83,6 +83,10 @@
#define NBD_OPT_PEEK_EXPORT (4)
#define NBD_OPT_STARTTLS(5)
#define NBD_OPT_STRUCTURED_REPLY (8)
+#define NBD_OPT_LIST_META_CONTEXT (9)
+#define NBD_OPT_SET_META_CONTEXT (10)
+
+#define NBD_META_NS_BITMAPS "qemu-dirty-bitmap"
/* NBD errors are based on errno numbers, so there is a 1:1 mapping,
* but only a limited set of errno values is specified in the protocol.
@@ -105,6 +109,8 @@ static inline const char *nbd_opt_name(int opt)
case NBD_OPT_PEEK_EXPORT: return "peek_export";
case NBD_OPT_STARTTLS: return "tls";
case NBD_OPT_STRUCTURED_REPLY: return "structured_reply";
+case NBD_OPT_LIST_META_CONTEXT: return "list_meta_context";
+case NBD_OPT_SET_META_CONTEXT: return "set_meta_context";
}
return "";
diff --git a/nbd/server.c b/nbd/server.c
index cb79a93c87..0b7b7230df 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -21,6 +21,8 @@
#include "qapi/error.h"
#include "nbd-internal.h"
+#define NBD_MAX_BITMAP_EXTENTS (0x10 / 8) /* 1 mb of extents data */
+
static int system_errno_to_nbd_errno(int err)
{
switch (err) {
@@ -102,6 +104,7 @@ struct NBDClient {
bool closing;
bool structured_reply;
+BdrvDirtyBitmap *export_bitmap;
};
/* That's all folks */
@@ -421,7 +424,304 @@ static QIOChannel
*nbd_negotiate_handle_starttls(NBDClient *client,
return QIO_CHANNEL(tioc);
}
+static int nbd_negotiate_read_size_string(QIOChannel *ioc, char **str,
+ uint32_t max_len)
+{
+uint32_t len;
+
+if (nbd_negotiate_read(ioc, , sizeof(len)) != sizeof(len)) {
+LOG("read failed");
+return -EIO;
+}
+
+cpu_to_be32s();
+
+if (max_len > 0 && len > max_len) {
+LOG("Bad length received");
+return -EINVAL;
+}
+
+*str = g_malloc(len + 1);
+
+if (nbd_negotiate_read(ioc, *str, len) != len) {
+LOG("read failed");
+g_free(str);
+return -EIO;
+}
+(*str)[len] = '\0';
+
+return sizeof(len) + len;
+}
+
+static int nbd_negotiate_send_meta_context(QIOChannel *ioc,
+ const char *context,
+ uint32_t opt)
+{
+int ret;
+size_t len = strlen(context);
