[Qemu-devel] [PATCH 0/8] various NBD fixes for 2.11

[Qemu-devel] [PATCH 0/8] various NBD fixes for 2.11

[Eric Blake]

Sparked by my question on whether we were handling zero-length
reads correctly according to the NBD spec, but grew to fix
other things as well such as some typos, tracing weaknesses,
and better handling of read-only exports.

The order these patches are listed here is bisection-friendly,
but applying the patches out of order (for example, applying
7/8 before any short-circuiting patches stop the client from
sending 0-length requests, and before the server is fixed to
stop botching replies to 0-length requests) proved important
to my ability to test that the patches are needed.

One of the patches is not strictly NBD, and begs the question
of whether other filter drivers (mirror, throttle, etc) should
also be reflecting the read-only status of their underlying
BDS.  Some of my testing included setting up a read-only NBD
server and a read-write qemu-io session, then attempting
'w 0 0', 'w -z 0 0', and 'd 0 0'; these should not succeed if
the corresponding 'w 0 1' fails, but the end goal is that the
server shouldn't even need to be involved in these corner cases.

Eric Blake (8):
  nbd-client: Fix error message typos
  nbd/client: Nicer trace of structured reply
  raw: Reflect read-only protocol layer
  nbd-client: Honor server read-only advertisement
  nbd: Fix struct name for structured reads
  nbd-client: Short-circuit 0-length operations
  nbd-client: Stricter enforcing of structured reply spec
  nbd/server: Fix structured read of length 0

 include/block/nbd.h | 18 +++++++++++++-----
 block/nbd-client.c  | 38 +++++++++++++++++++++++++++++++-------
 block/raw-format.c  |  6 ++++++
 nbd/client.c        |  4 +++-
 nbd/server.c        | 23 +++++++++++++++++++++--
 nbd/trace-events    |  3 ++-
 6 files changed, 76 insertions(+), 16 deletions(-)

-- 
2.13.6

[Qemu-devel] [PATCH 1/8] nbd-client: Fix error message typos

[Eric Blake]

Provide missing spaces that are required when using string
concatenation to break error messages across source lines.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 block/nbd-client.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/block/nbd-client.c b/block/nbd-client.c
index b44d4d4a01..de6c153328 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -248,7 +248,7 @@ static int nbd_parse_error_payload(NBDStructuredReplyChunk *chunk,

     error = nbd_errno_to_system_errno(payload_advance32(&payload));
     if (error == 0) {
-        error_setg(errp, "Protocol error: server sent structured error chunk"
+        error_setg(errp, "Protocol error: server sent structured error chunk "
                          "with error = 0");
         return -EINVAL;
     }
@@ -257,7 +257,7 @@ static int nbd_parse_error_payload(NBDStructuredReplyChunk *chunk,
     message_size = payload_advance16(&payload);

     if (message_size > chunk->length - sizeof(error) - sizeof(message_size)) {
-        error_setg(errp, "Protocol error: server sent structured error chunk"
+        error_setg(errp, "Protocol error: server sent structured error chunk "
                          "with incorrect message size");
         return -EINVAL;
     }
@@ -408,7 +408,7 @@ static coroutine_fn int nbd_co_do_receive_one_chunk(
     if (chunk->type == NBD_REPLY_TYPE_NONE) {
         if (!(chunk->flags & NBD_REPLY_FLAG_DONE)) {
             error_setg(errp, "Protocol error: NBD_REPLY_TYPE_NONE chunk without"
-                             "NBD_REPLY_FLAG_DONE flag set");
+                       " NBD_REPLY_FLAG_DONE flag set");
             return -EINVAL;
         }
         return 0;
-- 
2.13.6

[Qemu-devel] [PATCH 2/8] nbd/client: Nicer trace of structured reply

[Eric Blake]

It's useful to know which structured reply chunk is being processed.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 nbd/client.c     | 4 +++-
 nbd/trace-events | 2 +-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/nbd/client.c b/nbd/client.c
index 3d680e63e1..1880103d2a 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -979,6 +979,7 @@ static int nbd_receive_structured_reply_chunk(QIOChannel *ioc,
 int nbd_receive_reply(QIOChannel *ioc, NBDReply *reply, Error **errp)
 {
     int ret;
+    const char *type;

     ret = nbd_read_eof(ioc, &reply->magic, sizeof(reply->magic), errp);
     if (ret <= 0) {
@@ -1008,8 +1009,9 @@ int nbd_receive_reply(QIOChannel *ioc, NBDReply *reply, Error **errp)
         if (ret < 0) {
             break;
         }
+        type = nbd_reply_type_lookup(reply->structured.type);
         trace_nbd_receive_structured_reply_chunk(reply->structured.flags,
-                                                 reply->structured.type,
+                                                 reply->structured.type, type,
                                                  reply->structured.handle,
                                                  reply->structured.length);
         break;
diff --git a/nbd/trace-events b/nbd/trace-events
index 4a13757524..bbc75f6414 100644
--- a/nbd/trace-events
+++ b/nbd/trace-events
@@ -27,7 +27,7 @@ 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, const char *name) "Sending request to server: { .from = %" PRIu64", .len = %" PRIu32 ", .handle = %" PRIu64 ", .flags = 0x%" PRIx16 ", .type = %" PRIu16 " (%s) }"
 nbd_receive_simple_reply(int32_t error, const char *errname, uint64_t handle) "Got simple reply: { .error = %" PRId32 " (%s), handle = %" PRIu64" }"
-nbd_receive_structured_reply_chunk(uint16_t flags, uint16_t type, uint64_t handle, uint32_t length) "Got structured reply chunk: { flags = 0x%" PRIx16 ", type = %d, handle = %" PRIu64 ", length = %" PRIu32 " }"
+nbd_receive_structured_reply_chunk(uint16_t flags, uint16_t type, const char *name, uint64_t handle, uint32_t length) "Got structured reply chunk: { flags = 0x%" PRIx16 ", type = %d (%s), handle = %" PRIu64 ", length = %" PRIu32 " }"

 # nbd/common.c
 nbd_unknown_error(int err) "Squashing unexpected error %d to EINVAL"
-- 
2.13.6

[Qemu-devel] [PATCH 3/8] raw: Reflect read-only protocol layer

[Eric Blake]

We forbid operations like a zero-length write zero or a discard
at the protocol layer when it is marked read-only, but those
same operations were succeeding at the format layer because the
raw format was not reflecting the underlying read-only status
to the block layer, which then took short circuit paths on
zero-length operations.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 block/raw-format.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/block/raw-format.c b/block/raw-format.c
index 830243a8e4..717b8eff65 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -418,6 +418,12 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags,
         bs->file->bs->supported_write_flags;
     bs->supported_zero_flags = (BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP) &
         bs->file->bs->supported_zero_flags;
+    if (bdrv_is_read_only(bs->file->bs)) {
+        ret = bdrv_set_read_only(bs, true, errp);
+        if (ret < 0) {
+            return ret;
+        }
+    }

     if (bs->probed && !bdrv_is_read_only(bs)) {
         fprintf(stderr,
-- 
2.13.6

[Qemu-devel] [PATCH 4/8] nbd-client: Honor server read-only advertisement

[Eric Blake]

The NBD spec says that clients should not try to write/trim to
an export advertised as read-only by the server.  But we failed
to inform the block layer of what we learned from the server,
meaning that we were depending on the server sending a proper
EPERM failure for various commands.  In fact, this was noticeable
in the case of a 0-length write: we had a difference in behavior
depending on whether we were writing zero (silent success
locally, because the driver is not invoked from the block layer)
or normal writes (noisy failure, because we sent a 0-length
request across the wire and the server flagged it as invalid);
now we always fail locally without sending anything over the wire.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 block/nbd-client.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/block/nbd-client.c b/block/nbd-client.c
index de6c153328..c35aad59b0 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -697,6 +697,7 @@ int nbd_client_co_pwritev(BlockDriverState *bs, uint64_t offset,
         .len = bytes,
     };

+    assert(!(client->info.flags & NBD_FLAG_READ_ONLY));
     if (flags & BDRV_REQ_FUA) {
         assert(client->info.flags & NBD_FLAG_SEND_FUA);
         request.flags |= NBD_CMD_FLAG_FUA;
@@ -717,6 +718,7 @@ int nbd_client_co_pwrite_zeroes(BlockDriverState *bs, int64_t offset,
         .len = bytes,
     };

+    assert(!(client->info.flags & NBD_FLAG_READ_ONLY));
     if (!(client->info.flags & NBD_FLAG_SEND_WRITE_ZEROES)) {
         return -ENOTSUP;
     }
@@ -756,6 +758,7 @@ int nbd_client_co_pdiscard(BlockDriverState *bs, int64_t offset, int bytes)
         .len = bytes,
     };

+    assert(!(client->info.flags & NBD_FLAG_READ_ONLY));
     if (!(client->info.flags & NBD_FLAG_SEND_TRIM)) {
         return 0;
     }
@@ -814,6 +817,12 @@ int nbd_client_init(BlockDriverState *bs,
         logout("Failed to negotiate with the NBD server\n");
         return ret;
     }
+    if (client->info.flags & NBD_FLAG_READ_ONLY) {
+        ret = bdrv_set_read_only(bs, true, errp);
+        if (ret < 0) {
+            return ret;
+        }
+    }
     if (client->info.flags & NBD_FLAG_SEND_FUA) {
         bs->supported_write_flags = BDRV_REQ_FUA;
         bs->supported_zero_flags |= BDRV_REQ_FUA;
-- 
2.13.6

[Qemu-devel] [PATCH 5/8] nbd: Fix struct name for structured reads

[Eric Blake]

A closer read of the NBD spec shows that a structured reply chunk
for a hole is not quite identical to the prefix of a data chunk,
because the hole has to also send a 32-bit size field.  Although
we do not yet send holes, we should fix the misleading information
in our header and make it easier for a future patch to support
sparse reads.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 include/block/nbd.h | 18 +++++++++++++-----
 nbd/server.c        |  2 +-
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/include/block/nbd.h b/include/block/nbd.h
index 92d1723d7c..113c707a5e 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -86,15 +86,23 @@ typedef union NBDReply {
     } QEMU_PACKED;
 } NBDReply;

-/* Header of NBD_REPLY_TYPE_OFFSET_DATA, complete NBD_REPLY_TYPE_OFFSET_HOLE */
-typedef struct NBDStructuredRead {
-    NBDStructuredReplyChunk h;
+/* Header of chunk for NBD_REPLY_TYPE_OFFSET_DATA */
+typedef struct NBDStructuredReadData {
+    NBDStructuredReplyChunk h; /* h.length >= 9 */
     uint64_t offset;
-} QEMU_PACKED NBDStructuredRead;
+    /* At least one byte of data payload follows, calculated from h.length */
+} QEMU_PACKED NBDStructuredReadData;
+
+/* Complete chunk for NBD_REPLY_TYPE_OFFSET_HOLE */
+typedef struct NBDStructuredReadHole {
+    NBDStructuredReplyChunk h; /* h.length == 12 */
+    uint64_t offset;
+    uint32_t length;
+} QEMU_PACKED NBDStructuredReadHole;

 /* Header of all NBD_REPLY_TYPE_ERROR* errors */
 typedef struct NBDStructuredError {
-    NBDStructuredReplyChunk h;
+    NBDStructuredReplyChunk h; /* h.length >= 6 */
     uint32_t error;
     uint16_t message_length;
 } QEMU_PACKED NBDStructuredError;
diff --git a/nbd/server.c b/nbd/server.c
index bcf0cdb47c..6ebb7d9c2e 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1280,7 +1280,7 @@ static int coroutine_fn nbd_co_send_structured_read(NBDClient *client,
                                                     size_t size,
                                                     Error **errp)
 {
-    NBDStructuredRead chunk;
+    NBDStructuredReadData chunk;
     struct iovec iov[] = {
         {.iov_base = &chunk, .iov_len = sizeof(chunk)},
         {.iov_base = data, .iov_len = size}
-- 
2.13.6

[Qemu-devel] [PATCH 6/8] nbd-client: Short-circuit 0-length operations

[Eric Blake]

The NBD spec is being clarified to state that clients should
not send 0-length requests to the server, as the server behavior
is undefined.  While such requests are unlikely to occur from
real guest behavior, qemu-io can force the driver to perform them;
avoid any questionable server implementations by short-circuiting
such operations as a no-op success (we are relying on the block
layer for already filtering out things like invalid offset, write
to a read-only volume, and so forth).

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 block/nbd-client.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/block/nbd-client.c b/block/nbd-client.c
index c35aad59b0..d8ad2ba57c 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -673,6 +673,9 @@ int nbd_client_co_preadv(BlockDriverState *bs, uint64_t offset,

     assert(bytes <= NBD_MAX_BUFFER_SIZE);
     assert(!flags);
+    if (!bytes) {
+        return 0;
+    }

     ret = nbd_co_send_request(bs, &request, NULL);
     if (ret < 0) {
@@ -705,6 +708,9 @@ int nbd_client_co_pwritev(BlockDriverState *bs, uint64_t offset,

     assert(bytes <= NBD_MAX_BUFFER_SIZE);

+    if (!bytes) {
+        return 0;
+    }
     return nbd_co_request(bs, &request, qiov);
 }

@@ -731,6 +737,9 @@ int nbd_client_co_pwrite_zeroes(BlockDriverState *bs, int64_t offset,
         request.flags |= NBD_CMD_FLAG_NO_HOLE;
     }

+    if (!bytes) {
+        return 0;
+    }
     return nbd_co_request(bs, &request, NULL);
 }

@@ -759,7 +768,7 @@ int nbd_client_co_pdiscard(BlockDriverState *bs, int64_t offset, int bytes)
     };

     assert(!(client->info.flags & NBD_FLAG_READ_ONLY));
-    if (!(client->info.flags & NBD_FLAG_SEND_TRIM)) {
+    if (!(client->info.flags & NBD_FLAG_SEND_TRIM) || !bytes) {
         return 0;
     }

-- 
2.13.6

[Qemu-devel] [PATCH 7/8] nbd-client: Stricter enforcing of structured reply spec

[Eric Blake]

Ensure that the server is not sending unexpected chunk lengths
for either the NONE or the OFFSET_DATA chunk, nor unexpected
hole length for OFFSET_HOLE.  This will flag any server that
responds to a zero-length read with an OFFSET_DATA as broken, but
we just fixed things to never send that request.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 block/nbd-client.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/block/nbd-client.c b/block/nbd-client.c
index d8ad2ba57c..992c6e1493 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -216,7 +216,7 @@ static int nbd_parse_offset_hole_payload(NBDStructuredReplyChunk *chunk,
     offset = payload_advance64(&payload);
     hole_size = payload_advance32(&payload);

-    if (offset < orig_offset || hole_size > qiov->size ||
+    if (!hole_size || offset < orig_offset || hole_size > qiov->size ||
         offset > orig_offset + qiov->size - hole_size) {
         error_setg(errp, "Protocol error: server sent chunk exceeding requested"
                          " region");
@@ -281,7 +281,8 @@ static int nbd_co_receive_offset_data_payload(NBDClientSession *s,

     assert(nbd_reply_is_structured(&s->reply));

-    if (chunk->length < sizeof(offset)) {
+    /* The NBD spec requires at least one byte of payload */
+    if (chunk->length <= sizeof(offset)) {
         error_setg(errp, "Protocol error: invalid payload for "
                          "NBD_REPLY_TYPE_OFFSET_DATA");
         return -EINVAL;
@@ -293,7 +294,7 @@ static int nbd_co_receive_offset_data_payload(NBDClientSession *s,
     be64_to_cpus(&offset);

     data_size = chunk->length - sizeof(offset);
-    if (offset < orig_offset || data_size > qiov->size ||
+    if (!data_size || offset < orig_offset || data_size > qiov->size ||
         offset > orig_offset + qiov->size - data_size) {
         error_setg(errp, "Protocol error: server sent chunk exceeding requested"
                          " region");
@@ -411,6 +412,11 @@ static coroutine_fn int nbd_co_do_receive_one_chunk(
                        " NBD_REPLY_FLAG_DONE flag set");
             return -EINVAL;
         }
+        if (chunk->length) {
+            error_setg(errp, "Protocol error: NBD_REPLY_TYPE_NONE chunk with"
+                       " nonzero length");
+            return -EINVAL;
+        }
         return 0;
     }

-- 
2.13.6

[Qemu-devel] [PATCH 8/8] nbd/server: Fix structured read of length 0

[Eric Blake]

The NBD spec is being clarified that a read of length 0 should not
be attempted by a compliant client; but that a server must handle
it correctly.  However, it also implies that NBD_REPLY_TYPE_OFFSET_DATA
must have a non-zero payload length, which means that we must either
give an error or use a no-op REPLY_TYPE_NONE in response to a
zero-length read, so that we don't trip up any clients that might
kill the connection if we send a data chunk without data.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 nbd/server.c     | 21 ++++++++++++++++++++-
 nbd/trace-events |  1 +
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/nbd/server.c b/nbd/server.c
index 6ebb7d9c2e..df771fd42f 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1273,6 +1273,21 @@ static inline void set_be_chunk(NBDStructuredReplyChunk *chunk, uint16_t flags,
     stl_be_p(&chunk->length, length);
 }

+static int coroutine_fn nbd_co_send_structured_done(NBDClient *client,
+                                                    uint64_t handle,
+                                                    Error **errp)
+{
+    NBDStructuredReplyChunk chunk;
+    struct iovec iov[] = {
+        {.iov_base = &chunk, .iov_len = sizeof(chunk)},
+    };
+
+    trace_nbd_co_send_structured_done(handle);
+    set_be_chunk(&chunk, NBD_REPLY_FLAG_DONE, NBD_REPLY_TYPE_NONE, handle, 0);
+
+    return nbd_co_send_iov(client, iov, 1, errp);
+}
+
 static int coroutine_fn nbd_co_send_structured_read(NBDClient *client,
                                                     uint64_t handle,
                                                     uint64_t offset,
@@ -1286,6 +1301,7 @@ static int coroutine_fn nbd_co_send_structured_read(NBDClient *client,
         {.iov_base = data, .iov_len = size}
     };

+    assert(size);
     trace_nbd_co_send_structured_read(handle, offset, data, size);
     set_be_chunk(&chunk.h, NBD_REPLY_FLAG_DONE, NBD_REPLY_TYPE_OFFSET_DATA,
                  handle, sizeof(chunk) - sizeof(chunk.h) + size);
@@ -1544,10 +1560,13 @@ reply:
         if (ret < 0) {
             ret = nbd_co_send_structured_error(req->client, request.handle,
                                                -ret, msg, &local_err);
-        } else {
+        } else if (reply_data_len) {
             ret = nbd_co_send_structured_read(req->client, request.handle,
                                               request.from, req->data,
                                               reply_data_len, &local_err);
+        } else {
+            ret = nbd_co_send_structured_done(req->client, request.handle,
+                                              &local_err);
         }
     } else {
         ret = nbd_co_send_simple_reply(req->client, request.handle,
diff --git a/nbd/trace-events b/nbd/trace-events
index bbc75f6414..92568edce5 100644
--- a/nbd/trace-events
+++ b/nbd/trace-events
@@ -55,6 +55,7 @@ nbd_receive_request(uint32_t magic, uint16_t flags, uint16_t type, uint64_t from
 nbd_blk_aio_attached(const char *name, void *ctx) "Export %s: Attaching clients to AIO context %p\n"
 nbd_blk_aio_detach(const char *name, void *ctx) "Export %s: Detaching clients from AIO context %p\n"
 nbd_co_send_simple_reply(uint64_t handle, uint32_t error, const char *errname, int len) "Send simple reply: handle = %" PRIu64 ", error = %" PRIu32 " (%s), len = %d"
+nbd_co_send_structured_done(uint64_t handle) "Send structured reply done: handle = %" PRIu64
 nbd_co_send_structured_read(uint64_t handle, uint64_t offset, void *data, size_t size) "Send structured read data reply: handle = %" PRIu64 ", offset = %" PRIu64 ", data = %p, len = %zu"
 nbd_co_send_structured_error(uint64_t handle, int err, const char *errname, const char *msg) "Send structured error reply: handle = %" PRIu64 ", error = %d (%s), msg = '%s'"
 nbd_co_receive_request_decode_type(uint64_t handle, uint16_t type, const char *name) "Decoding type: handle = %" PRIu64 ", type = %" PRIu16 " (%s)"
-- 
2.13.6

Re: [Qemu-devel] [PATCH 3/8] raw: Reflect read-only protocol layer

[Paolo Bonzini]

On 07/11/2017 04:02, Eric Blake wrote:
> We forbid operations like a zero-length write zero or a discard
> at the protocol layer when it is marked read-only, but those
> same operations were succeeding at the format layer because the
> raw format was not reflecting the underlying read-only status
> to the block layer, which then took short circuit paths on
> zero-length operations.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  block/raw-format.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/block/raw-format.c b/block/raw-format.c
> index 830243a8e4..717b8eff65 100644
> --- a/block/raw-format.c
> +++ b/block/raw-format.c
> @@ -418,6 +418,12 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags,
>          bs->file->bs->supported_write_flags;
>      bs->supported_zero_flags = (BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP) &
>          bs->file->bs->supported_zero_flags;
> +    if (bdrv_is_read_only(bs->file->bs)) {
> +        ret = bdrv_set_read_only(bs, true, errp);
> +        if (ret < 0) {
> +            return ret;
> +        }
> +    }
> 
>      if (bs->probed && !bdrv_is_read_only(bs)) {
>          fprintf(stderr,
> 

Kevin, perhaps this should be done straight in block.c?

Thanks,

Paolo

Re: [Qemu-devel] [PATCH 3/8] raw: Reflect read-only protocol layer

[Kevin Wolf]

Am 07.11.2017 um 12:00 hat Paolo Bonzini geschrieben:
> On 07/11/2017 04:02, Eric Blake wrote:
> > We forbid operations like a zero-length write zero or a discard
> > at the protocol layer when it is marked read-only, but those
> > same operations were succeeding at the format layer because the
> > raw format was not reflecting the underlying read-only status
> > to the block layer, which then took short circuit paths on
> > zero-length operations.
> > 
> > Signed-off-by: Eric Blake <eblake@redhat.com>
> > ---
> >  block/raw-format.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/block/raw-format.c b/block/raw-format.c
> > index 830243a8e4..717b8eff65 100644
> > --- a/block/raw-format.c
> > +++ b/block/raw-format.c
> > @@ -418,6 +418,12 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags,
> >          bs->file->bs->supported_write_flags;
> >      bs->supported_zero_flags = (BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP) &
> >          bs->file->bs->supported_zero_flags;
> > +    if (bdrv_is_read_only(bs->file->bs)) {
> > +        ret = bdrv_set_read_only(bs, true, errp);
> > +        if (ret < 0) {
> > +            return ret;
> > +        }
> > +    }
> > 
> >      if (bs->probed && !bdrv_is_read_only(bs)) {
> >          fprintf(stderr,
> > 
> 
> Kevin, perhaps this should be done straight in block.c?

No, I just discussed this with Eric on IRC, and it shouldn't be done
anywhere.

Basically, the way qemu works with read-only images is that you need to
be explicit about it and specify read-only=on. Drivers are not supposed
to magically set the read-only flag if the user didn't request it.

So what NBD needs to do is to error out if you try to open a read-write
connection to a read-only NBD server.


There's a second part, related specifically to this patch, that was a
bit surprising to me at first, but it actually makes sense. I can
successfully create a read-write raw node on top of a read-only
file-posix node:

    -blockdev driver=file,filename=/tmp/test.qcow2,node-name=proto,read-only=on
    -blockdev driver=raw,file=proto,node-name=format

This is because in this state without a user, the raw node doesn't
actually want to write to the file-posix node. However, if I add a disk:

    -device ide-hd,drive=format

That will actually request write permissions throughout the whole chain
and cause an error:

    qemu-system-x86_64: -device ide-hd,drive=format: Block node is read-only

Somewhat surprising at first, but it does make sense.

Kevin