Re: [Qemu-devel] [PATCH 00/25] nbd: Several fixes

[Paolo Bonzini <pbonzini@redhat.com>]

On 25/02/2015 19:08, Max Reitz wrote:
> This series contains a variety of fixes for qemu's NBD code, with the
> most interesting (and probably most prone to be wrong) thing being a
> timeout of ten seconds for NBD connections.

As a first step, I am applying these patches:

Max Reitz (12):
      util/uri: Add overflow check to rfc3986_parse_port
      qemu-nbd: Detect unused partitions by system == 0
      nbd: Fix nbd_establish_connection()'s return value
      nbd: Fix response to invalid requests
      nbd: Pass return value from nbd_handle_list()
      nbd: Handle blk_getlength() failure
      qemu-nbd: fork() can fail
      nbd: Fix potential signed overflow issues
      nbd: Set block size to BDRV_SECTOR_SIZE
      nbd: Fix nbd_receive_options()
      nbd: Fix interpretation of the export flags
      nbd: Drop unexpected data for NBD_OPT_LIST
      coroutine-io: Return -errno in case of error

I disagree on the timeouts, because they can cause reordering of
operations (where you write A, get a timeout, write B, succeed, but then
A succeeds too and the application is confused).  The only correct
application of timeouts is to tear down the connection, reconnect _and_
resubmit pending I/O operations.

For all other patches, either I replied directly to the messages, or
they dependent on previous ones that are not included.

Thanks!

Paolo

> 
> 
> Max Reitz (25):
>   util/uri: Add overflow check to rfc3986_parse_port
>   qemu-nbd: Detect unused partitions by system == 0
>   nbd: Fix nbd_establish_connection()'s return value
>   nbd: Fix response to invalid requests
>   nbd: Avoid generic -EINVAL
>   nbd: Pass return value from nbd_handle_list()
>   nbd: Add "failed to open export" error message
>   nbd: Handle blk_getlength() failure
>   qemu-nbd: fork() can fail
>   nbd: Fix potential signed overflow issues
>   qemu-nbd: Fix and improve input verification
>   nbd: Set block size to BDRV_SECTOR_SIZE
>   nbd: Enforce sector alignment
>   coroutine: Add co_yield_timeout()
>   coroutine-io: Return -errno in case of error
>   coroutine-io: Add I/O functions with timeout
>   nbd: Employ timeouts
>   nbd: Fix nbd_receive_options()
>   nbd: Fix interpretation of the export flags
>   block/nbd: Comment on discard/flush silently failing
>   nbd: Drop unexpected data for NBD_OPT_LIST
>   iotests: Add _timeout function
>   iotests: Add test for invalid qemu-nbd parameters
>   iotests: Add test for issuing discard over NBD
>   iotests: Add test for a non-existing NBD export
> 
>  block/nbd-client.c           |  40 +++++++--
>  block/nbd-client.h           |   1 -
>  block/nbd.c                  |   2 +-
>  blockdev-nbd.c               |   6 +-
>  include/block/coroutine.h    |   6 ++
>  include/block/nbd.h          |  13 +--
>  include/qemu-common.h        |  45 ++++++++--
>  nbd.c                        | 203 ++++++++++++++++++++++++++++++-------------
>  qemu-coroutine-io.c          |  25 ++++--
>  qemu-coroutine-sleep.c       |  34 ++++++++
>  qemu-nbd.c                   |  74 +++++++++++-----
>  tests/qemu-iotests/096.out   |   2 +-
>  tests/qemu-iotests/125       |  69 +++++++++++++++
>  tests/qemu-iotests/125.out   |  21 +++++
>  tests/qemu-iotests/126       | 105 ++++++++++++++++++++++
>  tests/qemu-iotests/126.out   |  28 ++++++
>  tests/qemu-iotests/127       |  80 +++++++++++++++++
>  tests/qemu-iotests/127.out   |  14 +++
>  tests/qemu-iotests/common.rc |  16 ++++
>  tests/qemu-iotests/group     |   3 +
>  util/uri.c                   |  24 ++---
>  21 files changed, 686 insertions(+), 125 deletions(-)
>  create mode 100755 tests/qemu-iotests/125
>  create mode 100644 tests/qemu-iotests/125.out
>  create mode 100755 tests/qemu-iotests/126
>  create mode 100644 tests/qemu-iotests/126.out
>  create mode 100755 tests/qemu-iotests/127
>  create mode 100644 tests/qemu-iotests/127.out
>