[Qemu-commits] [qemu/qemu] 46321d: nbd/server: fix nbd_negotiate_handle_info

[GitHub]

  Branch: refs/heads/master
  Home:   https://github.com/qemu/qemu
  Commit: 46321d6b5f8c880932a6b3d07bd0ff6f892e665c
      
https://github.com/qemu/qemu/commit/46321d6b5f8c880932a6b3d07bd0ff6f892e665c
  Author: Vladimir Sementsov-Ogievskiy <address@hidden>
  Date:   2017-11-08 (Wed, 08 Nov 2017)

  Changed paths:
    M nbd/server.c

  Log Message:
  -----------
  nbd/server: fix nbd_negotiate_handle_info

namelen should be here, length is unrelated, and always 0 at this
point.  Broken in introduction in commit f37708f6, but mostly
harmless (replying with '' as the name does not violate protocol,
and does not confuse qemu as the nbd client since our implementation
does not ask for the name; but might confuse some other client that
does ask for the name especially if the default export is different
than the export name being queried).

Adding an assert makes it obvious that we are not skipping any bytes
in the client's message, as well as making it obvious that we were
using the wrong variable.

Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
CC: address@hidden
Message-Id: <address@hidden>
[eblake: improve commit message, squash in assert addition]
Signed-off-by: Eric Blake <address@hidden>


  Commit: e659fb3b9904b89f817e5f1d158f247be1bcc862
      
https://github.com/qemu/qemu/commit/e659fb3b9904b89f817e5f1d158f247be1bcc862
  Author: Eric Blake <address@hidden>
  Date:   2017-11-09 (Thu, 09 Nov 2017)

  Changed paths:
    M block/nbd-client.c

  Log Message:
  -----------
  nbd-client: Fix error message typos

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

Signed-off-by: Eric Blake <address@hidden>
Message-Id: <address@hidden>
Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>


  Commit: 1104d83c726d2b20f9cec7b99ab3570a2fdbd46d
      
https://github.com/qemu/qemu/commit/1104d83c726d2b20f9cec7b99ab3570a2fdbd46d
  Author: Eric Blake <address@hidden>
  Date:   2017-11-09 (Thu, 09 Nov 2017)

  Changed paths:
    M block/nbd-client.c
    M tests/qemu-iotests/058
    M tests/qemu-iotests/140
    M tests/qemu-iotests/147

  Log Message:
  -----------
  nbd-client: Refuse read-only client with BDRV_O_RDWR

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 check that, and would allow the block layer to use NBD with
BDRV_O_RDWR even when the server is read-only, which meant we
were depending on the server sending a proper EPERM failure for
various commands, and also exposes a leaky abstraction: using
qemu-io in read-write mode would succeed on 'w -z 0 0' because
of local short-circuiting logic, but 'w 0 0' would send a
request over the wire (where it then depends on the server, and
fails at least for qemu-nbd but might pass for other NBD
implementations).

With this patch, a client MUST request read-only mode to access
a server that is doing a read-only export, or else it will get
a message like:

can't open device nbd://localhost:10809/foo: request for write access conflicts 
with read-only export

It is no longer possible to even attempt writes over the wire
(including the corner case of 0-length writes), because the block
layer enforces the explicit read-only request; this matches the
behavior of qcow2 when backed by a read-only POSIX file.

Fix several iotests to comply with the new behavior (since
qemu-nbd of an internal snapshot, as well as nbd-server-add over QMP,
default to a read-only export, we must tell blockdev-add/qemu-io to
set up a read-only client).

CC: address@hidden
Signed-off-by: Eric Blake <address@hidden>
Message-Id: <address@hidden>
Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>


  Commit: 079d3266c79a41b2b00780f74d1c5e0b1756be95
      
https://github.com/qemu/qemu/commit/079d3266c79a41b2b00780f74d1c5e0b1756be95
  Author: Eric Blake <address@hidden>
  Date:   2017-11-09 (Thu, 09 Nov 2017)

  Changed paths:
    M nbd/client.c
    M nbd/trace-events

  Log Message:
  -----------
  nbd/client: Nicer trace of structured reply

It's useful to know which structured reply chunk is being processed.
Missed in commit d2febedb.

Signed-off-by: Eric Blake <address@hidden>
Message-Id: <address@hidden>
Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>


  Commit: efdc0c103d7919e5cabfc0b12a5699c1f8e482db
      
https://github.com/qemu/qemu/commit/efdc0c103d7919e5cabfc0b12a5699c1f8e482db
  Author: Eric Blake <address@hidden>
  Date:   2017-11-09 (Thu, 09 Nov 2017)

  Changed paths:
    M include/block/nbd.h
    M nbd/server.c

  Log Message:
  -----------
  nbd: Fix struct name for structured reads

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.  Messed up in commit bae245d1.

Signed-off-by: Eric Blake <address@hidden>
Message-Id: <address@hidden>
Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>


  Commit: 9d8f818cdee83e726a5dd14b645738ec632d2577
      
https://github.com/qemu/qemu/commit/9d8f818cdee83e726a5dd14b645738ec632d2577
  Author: Eric Blake <address@hidden>
  Date:   2017-11-09 (Thu, 09 Nov 2017)

  Changed paths:
    M block/nbd-client.c

  Log Message:
  -----------
  nbd-client: Short-circuit 0-length operations

The NBD spec was recently clarified to state that clients should
not send 0-length requests to the server, as the server behavior
is undefined [1].  We know that qemu-nbd's behavior is a successful
no-op (once it has filtered for read-only exports), but other NBD
implementations might return an error.  To avoid any questionable
server implementations, it is better to just short-circuit such
requests on the client side (we are relying on the block layer to
already filter out requests such as invalid offset, write to a
read-only volume, and so forth); do the short-circuit as late as
possible to still benefit from protections from assertions that
the block layer is not violating our assumptions.

[1] https://github.com/NetworkBlockDevice/nbd/commit/ee926037

Signed-off-by: Eric Blake <address@hidden>
Message-Id: <address@hidden>
Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>


  Commit: b4176cb314995ad225d6c2b531568801feb04f3f
      
https://github.com/qemu/qemu/commit/b4176cb314995ad225d6c2b531568801feb04f3f
  Author: Eric Blake <address@hidden>
  Date:   2017-11-09 (Thu, 09 Nov 2017)

  Changed paths:
    M block/nbd-client.c

  Log Message:
  -----------
  nbd-client: Stricter enforcing of structured reply spec

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 as
broken that responds to a zero-length read with an OFFSET_DATA
(what our server currently does, but that's about to be fixed)
or with OFFSET_HOLE, even though we previously fixed our client
to never be able to send such a request over the wire.

Signed-off-by: Eric Blake <address@hidden>
Message-Id: <address@hidden>
Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>


  Commit: ef8c887ee01a4e4c8c5c28c86ea5b45162c51bcd
      
https://github.com/qemu/qemu/commit/ef8c887ee01a4e4c8c5c28c86ea5b45162c51bcd
  Author: Eric Blake <address@hidden>
  Date:   2017-11-09 (Thu, 09 Nov 2017)

  Changed paths:
    M nbd/server.c
    M nbd/trace-events

  Log Message:
  -----------
  nbd/server: Fix structured read of length 0

The NBD spec was recently clarified to state that a read of length 0
should not be attempted by a compliant client; but that a server must
still handle it correctly in an unspecified manner (that is, either
a successful no-op or an error reply, but not a crash) [1].  However,
it also implies that NBD_REPLY_TYPE_OFFSET_DATA must have a non-zero
payload length, but our existing code was replying with a chunk
that a picky client could reject as invalid because it was missing
a payload (our own client implementation was recently patched to be
that picky, after first fixing it to not send 0-length requests).

We are already doing successful no-ops for 0-length writes and for
non-structured reads; so for consistency, we want structured reply
reads to also be a no-op.  The easiest way to do this is to return
a NBD_REPLY_TYPE_NONE chunk; this is best done via a new helper
function (especially since future patches for other structured
replies may benefit from using the same helper).

[1] https://github.com/NetworkBlockDevice/nbd/commit/ee926037

Signed-off-by: Eric Blake <address@hidden>
Message-Id: <address@hidden>
Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>


  Commit: f291910db61b5812e68f1e76afb3ade41d567bea
      
https://github.com/qemu/qemu/commit/f291910db61b5812e68f1e76afb3ade41d567bea
  Author: Peter Maydell <address@hidden>
  Date:   2017-11-13 (Mon, 13 Nov 2017)

  Changed paths:
    M block/nbd-client.c
    M include/block/nbd.h
    M nbd/client.c
    M nbd/server.c
    M nbd/trace-events
    M tests/qemu-iotests/058
    M tests/qemu-iotests/140
    M tests/qemu-iotests/147

  Log Message:
  -----------
  Merge remote-tracking branch 'remotes/ericb/tags/pull-nbd-2017-11-09' into 
staging

nbd patches for 2017-11-09

- Vladimir Sementsov-Ogievskiy: nbd/server: fix nbd_negotiate_handle_info
- Eric Blake: 0/7 various NBD fixes for 2.11

# gpg: Signature made Thu 09 Nov 2017 16:56:58 GMT
# gpg:                using RSA key 0xA7A16B4A2527436A
# gpg: Good signature from "Eric Blake <address@hidden>"
# gpg:                 aka "Eric Blake (Free Software Programmer) 
<address@hidden>"
# gpg:                 aka "[jpeg image of size 6874]"
# Primary key fingerprint: 71C2 CC22 B1C4 6029 27D2  F3AA A7A1 6B4A 2527 436A

* remotes/ericb/tags/pull-nbd-2017-11-09:
  nbd/server: Fix structured read of length 0
  nbd-client: Stricter enforcing of structured reply spec
  nbd-client: Short-circuit 0-length operations
  nbd: Fix struct name for structured reads
  nbd/client: Nicer trace of structured reply
  nbd-client: Refuse read-only client with BDRV_O_RDWR
  nbd-client: Fix error message typos
  nbd/server: fix nbd_negotiate_handle_info

Signed-off-by: Peter Maydell <address@hidden>


Compare: https://github.com/qemu/qemu/compare/508ba0f7e209...f291910db61b