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

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

[Max Reitz]

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.


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

-- 
2.1.0

[Qemu-devel] [PATCH 01/25] util/uri: Add overflow check to rfc3986_parse_port

[Max Reitz]

And while at it, replace tabs by eight spaces in this function.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 util/uri.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/util/uri.c b/util/uri.c
index 1cfd78b..550b984 100644
--- a/util/uri.c
+++ b/util/uri.c
@@ -320,19 +320,23 @@ static int
 rfc3986_parse_port(URI *uri, const char **str)
 {
     const char *cur = *str;
+    int port = 0;
 
     if (ISA_DIGIT(cur)) {
-	if (uri != NULL)
-	    uri->port = 0;
-	while (ISA_DIGIT(cur)) {
-	    if (uri != NULL)
-		uri->port = uri->port * 10 + (*cur - '0');
-	    cur++;
-	}
-	*str = cur;
-	return(0);
+        while (ISA_DIGIT(cur)) {
+            port = port * 10 + (*cur - '0');
+            if (port > 65535) {
+                return 1;
+            }
+            cur++;
+        }
+        if (uri) {
+            uri->port = port;
+        }
+        *str = cur;
+        return 0;
     }
-    return(1);
+    return 1;
 }
 
 /**
-- 
2.1.0

[Qemu-devel] [PATCH 02/25] qemu-nbd: Detect unused partitions by system == 0

[Max Reitz]

Unused partitions do not necessarily have a total sector count of 0
(although they should have), but they always do have the system field
set to 0, so use that for testing whether a partition is in use rather
than the sector count field alone.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 qemu-nbd.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index ac1e5d6..cfdc4dc 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -167,8 +167,9 @@ static int find_partition(BlockBackend *blk, int partition,
     for (i = 0; i < 4; i++) {
         read_partition(&data[446 + 16 * i], &mbr[i]);
 
-        if (!mbr[i].nb_sectors_abs)
+        if (!mbr[i].system || !mbr[i].nb_sectors_abs) {
             continue;
+        }
 
         if (mbr[i].system == 0xF || mbr[i].system == 0x5) {
             struct partition_record ext[4];
@@ -182,8 +183,9 @@ static int find_partition(BlockBackend *blk, int partition,
 
             for (j = 0; j < 4; j++) {
                 read_partition(&data1[446 + 16 * j], &ext[j]);
-                if (!ext[j].nb_sectors_abs)
+                if (!ext[j].system || !ext[j].nb_sectors_abs) {
                     continue;
+                }
 
                 if ((ext_partnum + j + 1) == partition) {
                     *offset = (uint64_t)ext[j].start_sector_abs << 9;
-- 
2.1.0

[Qemu-devel] [PATCH 03/25] nbd: Fix nbd_establish_connection()'s return value

[Max Reitz]

unix_connect_opts() and inet_connect_opts() do not necessarily set errno
(if at all); therefore, nbd_establish_connection() should not literally
return -errno on error.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/nbd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/nbd.c b/block/nbd.c
index 2f3b9ad..4a6cf9d 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -247,7 +247,7 @@ static int nbd_establish_connection(BlockDriverState *bs, Error **errp)
     /* Failed to establish connection */
     if (sock < 0) {
         logout("Failed to establish connection to NBD server\n");
-        return -errno;
+        return -EIO;
     }
 
     return sock;
-- 
2.1.0

[Qemu-devel] [PATCH 04/25] nbd: Fix response to invalid requests

[Max Reitz]

reply.error is a positive errno value, not a negative one.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 nbd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/nbd.c b/nbd.c
index 5d50f22..b4776ce 100644
--- a/nbd.c
+++ b/nbd.c
@@ -1330,7 +1330,7 @@ static void nbd_trip(void *opaque)
     default:
         LOG("invalid request type (%u) received", request.type);
     invalid_request:
-        reply.error = -EINVAL;
+        reply.error = EINVAL;
     error_reply:
         if (nbd_co_send_reply(req, &reply, 0) < 0) {
             goto out;
-- 
2.1.0

Re: [Qemu-devel] [PATCH 04/25] nbd: Fix response to invalid requests

[Max Reitz]

On 2015-02-25 at 13:08, Max Reitz wrote:
> reply.error is a positive errno value, not a negative one.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   nbd.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

This is (functionally) the same patch as 
http://lists.nongnu.org/archive/html/qemu-devel/2015-02/msg02258.html, 
so this should be dropped if that one is applied before.

Max

[Qemu-devel] [PATCH 05/25] nbd: Avoid generic -EINVAL

[Max Reitz]

Just returning -EINVAL for everything is bad. -EIO is often better, and
sometimes there is an even more fitting value.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 nbd.c | 49 +++++++++++++++++++++++++------------------------
 1 file changed, 25 insertions(+), 24 deletions(-)

diff --git a/nbd.c b/nbd.c
index b4776ce..d2eb1c5 100644
--- a/nbd.c
+++ b/nbd.c
@@ -238,22 +238,22 @@ static int nbd_send_rep(int csock, uint32_t type, uint32_t opt)
     magic = cpu_to_be64(NBD_REP_MAGIC);
     if (write_sync(csock, &magic, sizeof(magic)) != sizeof(magic)) {
         LOG("write failed (rep magic)");
-        return -EINVAL;
+        return -EIO;
     }
     opt = cpu_to_be32(opt);
     if (write_sync(csock, &opt, sizeof(opt)) != sizeof(opt)) {
         LOG("write failed (rep opt)");
-        return -EINVAL;
+        return -EIO;
     }
     type = cpu_to_be32(type);
     if (write_sync(csock, &type, sizeof(type)) != sizeof(type)) {
         LOG("write failed (rep type)");
-        return -EINVAL;
+        return -EIO;
     }
     len = cpu_to_be32(0);
     if (write_sync(csock, &len, sizeof(len)) != sizeof(len)) {
         LOG("write failed (rep data length)");
-        return -EINVAL;
+        return -EIO;
     }
     return 0;
 }
@@ -267,31 +267,31 @@ static int nbd_send_rep_list(int csock, NBDExport *exp)
     magic = cpu_to_be64(NBD_REP_MAGIC);
     if (write_sync(csock, &magic, sizeof(magic)) != sizeof(magic)) {
         LOG("write failed (magic)");
-        return -EINVAL;
+        return -EIO;
      }
     opt = cpu_to_be32(NBD_OPT_LIST);
     if (write_sync(csock, &opt, sizeof(opt)) != sizeof(opt)) {
         LOG("write failed (opt)");
-        return -EINVAL;
+        return -EIO;
     }
     type = cpu_to_be32(NBD_REP_SERVER);
     if (write_sync(csock, &type, sizeof(type)) != sizeof(type)) {
         LOG("write failed (reply type)");
-        return -EINVAL;
+        return -EIO;
     }
     len = cpu_to_be32(name_len + sizeof(len));
     if (write_sync(csock, &len, sizeof(len)) != sizeof(len)) {
         LOG("write failed (length)");
-        return -EINVAL;
+        return -EIO;
     }
     len = cpu_to_be32(name_len);
     if (write_sync(csock, &len, sizeof(len)) != sizeof(len)) {
         LOG("write failed (length)");
-        return -EINVAL;
+        return -EIO;
     }
     if (write_sync(csock, exp->name, name_len) != name_len) {
         LOG("write failed (buffer)");
-        return -EINVAL;
+        return -EIO;
     }
     return 0;
 }
@@ -309,7 +309,7 @@ static int nbd_handle_list(NBDClient *client, uint32_t length)
     /* For each export, send a NBD_REP_SERVER reply. */
     QTAILQ_FOREACH(exp, &exports, next) {
         if (nbd_send_rep_list(csock, exp)) {
-            return -EINVAL;
+            return -EIO;
         }
     }
     /* Finish with a NBD_REP_ACK. */
@@ -331,6 +331,7 @@ static int nbd_handle_export_name(NBDClient *client, uint32_t length)
     }
     if (read_sync(csock, name, length) != length) {
         LOG("read failed");
+        rc = -EIO;
         goto fail;
     }
     name[length] = '\0';
@@ -376,22 +377,22 @@ static int nbd_receive_options(NBDClient *client)
 
         if (read_sync(csock, &magic, sizeof(magic)) != sizeof(magic)) {
             LOG("read failed");
-            return -EINVAL;
+            return -EIO;
         }
         TRACE("Checking opts magic");
         if (magic != be64_to_cpu(NBD_OPTS_MAGIC)) {
             LOG("Bad magic received");
-            return -EINVAL;
+            return -EIO;
         }
 
         if (read_sync(csock, &tmp, sizeof(tmp)) != sizeof(tmp)) {
             LOG("read failed");
-            return -EINVAL;
+            return -EIO;
         }
 
         if (read_sync(csock, &length, sizeof(length)) != sizeof(length)) {
             LOG("read failed");
-            return -EINVAL;
+            return -EIO;
         }
         length = be32_to_cpu(length);
 
@@ -404,7 +405,7 @@ static int nbd_receive_options(NBDClient *client)
             break;
 
         case NBD_OPT_ABORT:
-            return -EINVAL;
+            return -ECONNABORTED;
 
         case NBD_OPT_EXPORT_NAME:
             return nbd_handle_export_name(client, length);
@@ -446,7 +447,7 @@ static int nbd_send_negotiate(NBDClient *client)
      */
 
     qemu_set_block(csock);
-    rc = -EINVAL;
+    rc = -EIO;
 
     TRACE("Beginning negotiation.");
     memset(buf, 0, sizeof(buf));
@@ -503,7 +504,7 @@ int nbd_receive_negotiate(int csock, const char *name, uint32_t *flags,
 
     TRACE("Receiving negotiation.");
 
-    rc = -EINVAL;
+    rc = -EIO;
 
     if (read_sync(csock, buf, 8) != 8) {
         error_setg(errp, "Failed to read data");
@@ -752,7 +753,7 @@ ssize_t nbd_send_request(int csock, struct nbd_request *request)
 
     if (ret != sizeof(buf)) {
         LOG("writing to socket failed");
-        return -EINVAL;
+        return -EIO;
     }
     return 0;
 }
@@ -770,7 +771,7 @@ static ssize_t nbd_receive_request(int csock, struct nbd_request *request)
 
     if (ret != sizeof(buf)) {
         LOG("read failed");
-        return -EINVAL;
+        return -EIO;
     }
 
     /* Request
@@ -793,7 +794,7 @@ static ssize_t nbd_receive_request(int csock, struct nbd_request *request)
 
     if (magic != NBD_REQUEST_MAGIC) {
         LOG("invalid magic (got 0x%x)", magic);
-        return -EINVAL;
+        return -EIO;
     }
     return 0;
 }
@@ -811,7 +812,7 @@ ssize_t nbd_receive_reply(int csock, struct nbd_reply *reply)
 
     if (ret != sizeof(buf)) {
         LOG("read failed");
-        return -EINVAL;
+        return -EIO;
     }
 
     /* Reply
@@ -830,7 +831,7 @@ ssize_t nbd_receive_reply(int csock, struct nbd_reply *reply)
 
     if (magic != NBD_REPLY_MAGIC) {
         LOG("invalid magic (got 0x%x)", magic);
-        return -EINVAL;
+        return -EIO;
     }
     return 0;
 }
@@ -858,7 +859,7 @@ static ssize_t nbd_send_reply(int csock, struct nbd_reply *reply)
 
     if (ret != sizeof(buf)) {
         LOG("writing to socket failed");
-        return -EINVAL;
+        return -EIO;
     }
     return 0;
 }
-- 
2.1.0

Re: [Qemu-devel] [PATCH 05/25] nbd: Avoid generic -EINVAL

[Paolo Bonzini]

On 25/02/2015 19:08, Max Reitz wrote:
> Just returning -EINVAL for everything is bad. -EIO is often better, and
> sometimes there is an even more fitting value.

Propagating the return value from write_sync is uglier, but it is even
better in terms of returned value.

Paolo

> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  nbd.c | 49 +++++++++++++++++++++++++------------------------
>  1 file changed, 25 insertions(+), 24 deletions(-)
> 
> diff --git a/nbd.c b/nbd.c
> index b4776ce..d2eb1c5 100644
> --- a/nbd.c
> +++ b/nbd.c
> @@ -238,22 +238,22 @@ static int nbd_send_rep(int csock, uint32_t type, uint32_t opt)
>      magic = cpu_to_be64(NBD_REP_MAGIC);
>      if (write_sync(csock, &magic, sizeof(magic)) != sizeof(magic)) {
>          LOG("write failed (rep magic)");
> -        return -EINVAL;
> +        return -EIO;
>      }
>      opt = cpu_to_be32(opt);
>      if (write_sync(csock, &opt, sizeof(opt)) != sizeof(opt)) {
>          LOG("write failed (rep opt)");
> -        return -EINVAL;
> +        return -EIO;
>      }
>      type = cpu_to_be32(type);
>      if (write_sync(csock, &type, sizeof(type)) != sizeof(type)) {
>          LOG("write failed (rep type)");
> -        return -EINVAL;
> +        return -EIO;
>      }
>      len = cpu_to_be32(0);
>      if (write_sync(csock, &len, sizeof(len)) != sizeof(len)) {
>          LOG("write failed (rep data length)");
> -        return -EINVAL;
> +        return -EIO;
>      }
>      return 0;
>  }
> @@ -267,31 +267,31 @@ static int nbd_send_rep_list(int csock, NBDExport *exp)
>      magic = cpu_to_be64(NBD_REP_MAGIC);
>      if (write_sync(csock, &magic, sizeof(magic)) != sizeof(magic)) {
>          LOG("write failed (magic)");
> -        return -EINVAL;
> +        return -EIO;
>       }
>      opt = cpu_to_be32(NBD_OPT_LIST);
>      if (write_sync(csock, &opt, sizeof(opt)) != sizeof(opt)) {
>          LOG("write failed (opt)");
> -        return -EINVAL;
> +        return -EIO;
>      }
>      type = cpu_to_be32(NBD_REP_SERVER);
>      if (write_sync(csock, &type, sizeof(type)) != sizeof(type)) {
>          LOG("write failed (reply type)");
> -        return -EINVAL;
> +        return -EIO;
>      }
>      len = cpu_to_be32(name_len + sizeof(len));
>      if (write_sync(csock, &len, sizeof(len)) != sizeof(len)) {
>          LOG("write failed (length)");
> -        return -EINVAL;
> +        return -EIO;
>      }
>      len = cpu_to_be32(name_len);
>      if (write_sync(csock, &len, sizeof(len)) != sizeof(len)) {
>          LOG("write failed (length)");
> -        return -EINVAL;
> +        return -EIO;
>      }
>      if (write_sync(csock, exp->name, name_len) != name_len) {
>          LOG("write failed (buffer)");
> -        return -EINVAL;
> +        return -EIO;
>      }
>      return 0;
>  }
> @@ -309,7 +309,7 @@ static int nbd_handle_list(NBDClient *client, uint32_t length)
>      /* For each export, send a NBD_REP_SERVER reply. */
>      QTAILQ_FOREACH(exp, &exports, next) {
>          if (nbd_send_rep_list(csock, exp)) {
> -            return -EINVAL;
> +            return -EIO;
>          }
>      }
>      /* Finish with a NBD_REP_ACK. */
> @@ -331,6 +331,7 @@ static int nbd_handle_export_name(NBDClient *client, uint32_t length)
>      }
>      if (read_sync(csock, name, length) != length) {
>          LOG("read failed");
> +        rc = -EIO;
>          goto fail;
>      }
>      name[length] = '\0';
> @@ -376,22 +377,22 @@ static int nbd_receive_options(NBDClient *client)
>  
>          if (read_sync(csock, &magic, sizeof(magic)) != sizeof(magic)) {
>              LOG("read failed");
> -            return -EINVAL;
> +            return -EIO;
>          }
>          TRACE("Checking opts magic");
>          if (magic != be64_to_cpu(NBD_OPTS_MAGIC)) {
>              LOG("Bad magic received");
> -            return -EINVAL;
> +            return -EIO;
>          }
>  
>          if (read_sync(csock, &tmp, sizeof(tmp)) != sizeof(tmp)) {
>              LOG("read failed");
> -            return -EINVAL;
> +            return -EIO;
>          }
>  
>          if (read_sync(csock, &length, sizeof(length)) != sizeof(length)) {
>              LOG("read failed");
> -            return -EINVAL;
> +            return -EIO;
>          }
>          length = be32_to_cpu(length);
>  
> @@ -404,7 +405,7 @@ static int nbd_receive_options(NBDClient *client)
>              break;
>  
>          case NBD_OPT_ABORT:
> -            return -EINVAL;
> +            return -ECONNABORTED;
>  
>          case NBD_OPT_EXPORT_NAME:
>              return nbd_handle_export_name(client, length);
> @@ -446,7 +447,7 @@ static int nbd_send_negotiate(NBDClient *client)
>       */
>  
>      qemu_set_block(csock);
> -    rc = -EINVAL;
> +    rc = -EIO;
>  
>      TRACE("Beginning negotiation.");
>      memset(buf, 0, sizeof(buf));
> @@ -503,7 +504,7 @@ int nbd_receive_negotiate(int csock, const char *name, uint32_t *flags,
>  
>      TRACE("Receiving negotiation.");
>  
> -    rc = -EINVAL;
> +    rc = -EIO;
>  
>      if (read_sync(csock, buf, 8) != 8) {
>          error_setg(errp, "Failed to read data");
> @@ -752,7 +753,7 @@ ssize_t nbd_send_request(int csock, struct nbd_request *request)
>  
>      if (ret != sizeof(buf)) {
>          LOG("writing to socket failed");
> -        return -EINVAL;
> +        return -EIO;
>      }
>      return 0;
>  }
> @@ -770,7 +771,7 @@ static ssize_t nbd_receive_request(int csock, struct nbd_request *request)
>  
>      if (ret != sizeof(buf)) {
>          LOG("read failed");
> -        return -EINVAL;
> +        return -EIO;
>      }
>  
>      /* Request
> @@ -793,7 +794,7 @@ static ssize_t nbd_receive_request(int csock, struct nbd_request *request)
>  
>      if (magic != NBD_REQUEST_MAGIC) {
>          LOG("invalid magic (got 0x%x)", magic);
> -        return -EINVAL;
> +        return -EIO;
>      }
>      return 0;
>  }
> @@ -811,7 +812,7 @@ ssize_t nbd_receive_reply(int csock, struct nbd_reply *reply)
>  
>      if (ret != sizeof(buf)) {
>          LOG("read failed");
> -        return -EINVAL;
> +        return -EIO;
>      }
>  
>      /* Reply
> @@ -830,7 +831,7 @@ ssize_t nbd_receive_reply(int csock, struct nbd_reply *reply)
>  
>      if (magic != NBD_REPLY_MAGIC) {
>          LOG("invalid magic (got 0x%x)", magic);
> -        return -EINVAL;
> +        return -EIO;
>      }
>      return 0;
>  }
> @@ -858,7 +859,7 @@ static ssize_t nbd_send_reply(int csock, struct nbd_reply *reply)
>  
>      if (ret != sizeof(buf)) {
>          LOG("writing to socket failed");
> -        return -EINVAL;
> +        return -EIO;
>      }
>      return 0;
>  }
> 

Re: [Qemu-devel] [PATCH 05/25] nbd: Avoid generic -EINVAL

[Max Reitz]

On 2015-03-11 at 07:22, Paolo Bonzini wrote:
>
> On 25/02/2015 19:08, Max Reitz wrote:
>> Just returning -EINVAL for everything is bad. -EIO is often better, and
>> sometimes there is an even more fitting value.
> Propagating the return value from write_sync is uglier, but it is even
> better in terms of returned value.

We can only return -errno values, but write_sync() may do partial writes 
so it may return non-negative values which still indicate an error. So 
we'd have to check whether the return value is negative, if it is, 
return that, if it isn't but if it's still below what we wanted to 
write, return a fixed error (such as -EIO). I'd rather just return -EIO 
and be done with it, but if you really want me to, I can of course do it 
differently.

Max

> Paolo
>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   nbd.c | 49 +++++++++++++++++++++++++------------------------
>>   1 file changed, 25 insertions(+), 24 deletions(-)
>>
>> diff --git a/nbd.c b/nbd.c
>> index b4776ce..d2eb1c5 100644
>> --- a/nbd.c
>> +++ b/nbd.c
>> @@ -238,22 +238,22 @@ static int nbd_send_rep(int csock, uint32_t type, uint32_t opt)
>>       magic = cpu_to_be64(NBD_REP_MAGIC);
>>       if (write_sync(csock, &magic, sizeof(magic)) != sizeof(magic)) {
>>           LOG("write failed (rep magic)");
>> -        return -EINVAL;
>> +        return -EIO;
>>       }
>>       opt = cpu_to_be32(opt);
>>       if (write_sync(csock, &opt, sizeof(opt)) != sizeof(opt)) {
>>           LOG("write failed (rep opt)");
>> -        return -EINVAL;
>> +        return -EIO;
>>       }
>>       type = cpu_to_be32(type);
>>       if (write_sync(csock, &type, sizeof(type)) != sizeof(type)) {
>>           LOG("write failed (rep type)");
>> -        return -EINVAL;
>> +        return -EIO;
>>       }
>>       len = cpu_to_be32(0);
>>       if (write_sync(csock, &len, sizeof(len)) != sizeof(len)) {
>>           LOG("write failed (rep data length)");
>> -        return -EINVAL;
>> +        return -EIO;
>>       }
>>       return 0;
>>   }
>> @@ -267,31 +267,31 @@ static int nbd_send_rep_list(int csock, NBDExport *exp)
>>       magic = cpu_to_be64(NBD_REP_MAGIC);
>>       if (write_sync(csock, &magic, sizeof(magic)) != sizeof(magic)) {
>>           LOG("write failed (magic)");
>> -        return -EINVAL;
>> +        return -EIO;
>>        }
>>       opt = cpu_to_be32(NBD_OPT_LIST);
>>       if (write_sync(csock, &opt, sizeof(opt)) != sizeof(opt)) {
>>           LOG("write failed (opt)");
>> -        return -EINVAL;
>> +        return -EIO;
>>       }
>>       type = cpu_to_be32(NBD_REP_SERVER);
>>       if (write_sync(csock, &type, sizeof(type)) != sizeof(type)) {
>>           LOG("write failed (reply type)");
>> -        return -EINVAL;
>> +        return -EIO;
>>       }
>>       len = cpu_to_be32(name_len + sizeof(len));
>>       if (write_sync(csock, &len, sizeof(len)) != sizeof(len)) {
>>           LOG("write failed (length)");
>> -        return -EINVAL;
>> +        return -EIO;
>>       }
>>       len = cpu_to_be32(name_len);
>>       if (write_sync(csock, &len, sizeof(len)) != sizeof(len)) {
>>           LOG("write failed (length)");
>> -        return -EINVAL;
>> +        return -EIO;
>>       }
>>       if (write_sync(csock, exp->name, name_len) != name_len) {
>>           LOG("write failed (buffer)");
>> -        return -EINVAL;
>> +        return -EIO;
>>       }
>>       return 0;
>>   }
>> @@ -309,7 +309,7 @@ static int nbd_handle_list(NBDClient *client, uint32_t length)
>>       /* For each export, send a NBD_REP_SERVER reply. */
>>       QTAILQ_FOREACH(exp, &exports, next) {
>>           if (nbd_send_rep_list(csock, exp)) {
>> -            return -EINVAL;
>> +            return -EIO;
>>           }
>>       }
>>       /* Finish with a NBD_REP_ACK. */
>> @@ -331,6 +331,7 @@ static int nbd_handle_export_name(NBDClient *client, uint32_t length)
>>       }
>>       if (read_sync(csock, name, length) != length) {
>>           LOG("read failed");
>> +        rc = -EIO;
>>           goto fail;
>>       }
>>       name[length] = '\0';
>> @@ -376,22 +377,22 @@ static int nbd_receive_options(NBDClient *client)
>>   
>>           if (read_sync(csock, &magic, sizeof(magic)) != sizeof(magic)) {
>>               LOG("read failed");
>> -            return -EINVAL;
>> +            return -EIO;
>>           }
>>           TRACE("Checking opts magic");
>>           if (magic != be64_to_cpu(NBD_OPTS_MAGIC)) {
>>               LOG("Bad magic received");
>> -            return -EINVAL;
>> +            return -EIO;
>>           }
>>   
>>           if (read_sync(csock, &tmp, sizeof(tmp)) != sizeof(tmp)) {
>>               LOG("read failed");
>> -            return -EINVAL;
>> +            return -EIO;
>>           }
>>   
>>           if (read_sync(csock, &length, sizeof(length)) != sizeof(length)) {
>>               LOG("read failed");
>> -            return -EINVAL;
>> +            return -EIO;
>>           }
>>           length = be32_to_cpu(length);
>>   
>> @@ -404,7 +405,7 @@ static int nbd_receive_options(NBDClient *client)
>>               break;
>>   
>>           case NBD_OPT_ABORT:
>> -            return -EINVAL;
>> +            return -ECONNABORTED;
>>   
>>           case NBD_OPT_EXPORT_NAME:
>>               return nbd_handle_export_name(client, length);
>> @@ -446,7 +447,7 @@ static int nbd_send_negotiate(NBDClient *client)
>>        */
>>   
>>       qemu_set_block(csock);
>> -    rc = -EINVAL;
>> +    rc = -EIO;
>>   
>>       TRACE("Beginning negotiation.");
>>       memset(buf, 0, sizeof(buf));
>> @@ -503,7 +504,7 @@ int nbd_receive_negotiate(int csock, const char *name, uint32_t *flags,
>>   
>>       TRACE("Receiving negotiation.");
>>   
>> -    rc = -EINVAL;
>> +    rc = -EIO;
>>   
>>       if (read_sync(csock, buf, 8) != 8) {
>>           error_setg(errp, "Failed to read data");
>> @@ -752,7 +753,7 @@ ssize_t nbd_send_request(int csock, struct nbd_request *request)
>>   
>>       if (ret != sizeof(buf)) {
>>           LOG("writing to socket failed");
>> -        return -EINVAL;
>> +        return -EIO;
>>       }
>>       return 0;
>>   }
>> @@ -770,7 +771,7 @@ static ssize_t nbd_receive_request(int csock, struct nbd_request *request)
>>   
>>       if (ret != sizeof(buf)) {
>>           LOG("read failed");
>> -        return -EINVAL;
>> +        return -EIO;
>>       }
>>   
>>       /* Request
>> @@ -793,7 +794,7 @@ static ssize_t nbd_receive_request(int csock, struct nbd_request *request)
>>   
>>       if (magic != NBD_REQUEST_MAGIC) {
>>           LOG("invalid magic (got 0x%x)", magic);
>> -        return -EINVAL;
>> +        return -EIO;
>>       }
>>       return 0;
>>   }
>> @@ -811,7 +812,7 @@ ssize_t nbd_receive_reply(int csock, struct nbd_reply *reply)
>>   
>>       if (ret != sizeof(buf)) {
>>           LOG("read failed");
>> -        return -EINVAL;
>> +        return -EIO;
>>       }
>>   
>>       /* Reply
>> @@ -830,7 +831,7 @@ ssize_t nbd_receive_reply(int csock, struct nbd_reply *reply)
>>   
>>       if (magic != NBD_REPLY_MAGIC) {
>>           LOG("invalid magic (got 0x%x)", magic);
>> -        return -EINVAL;
>> +        return -EIO;
>>       }
>>       return 0;
>>   }
>> @@ -858,7 +859,7 @@ static ssize_t nbd_send_reply(int csock, struct nbd_reply *reply)
>>   
>>       if (ret != sizeof(buf)) {
>>           LOG("writing to socket failed");
>> -        return -EINVAL;
>> +        return -EIO;
>>       }
>>       return 0;
>>   }
>>

Re: [Qemu-devel] [PATCH 05/25] nbd: Avoid generic -EINVAL

[Paolo Bonzini]

On 16/03/2015 14:51, Max Reitz wrote:
>>>
>> Propagating the return value from write_sync is uglier, but it is even
>> better in terms of returned value.
> 
> We can only return -errno values, but write_sync() may do partial writes
> so it may return non-negative values which still indicate an error. So
> we'd have to check whether the return value is negative, if it is,
> return that, if it isn't but if it's still below what we wanted to
> write, return a fixed error (such as -EIO). I'd rather just return -EIO
> and be done with it, but if you really want me to, I can of course do it
> differently.

nbd_wr_sync doesn't do that, it always returns negative errno for a partial
error:

        if (len < 0) {
            err = socket_error();

            /* recoverable error */
            if (err == EINTR || (offset > 0 && (err == EAGAIN || err == EWOULDBLOCK))) {
                continue;
            }

            /* unrecoverable error */
            return -err;
        }

The precise error can be useful to distinguish a network error from something
else.  I'm just in doubt about partial reads; those can return a positive error,
in which case you can return ESHUTDOWN (in read_sync).

Paolo

Re: [Qemu-devel] [PATCH 05/25] nbd: Avoid generic -EINVAL

[Max Reitz]

On 2015-03-16 at 10:42, Paolo Bonzini wrote:
>
> On 16/03/2015 14:51, Max Reitz wrote:
>>> Propagating the return value from write_sync is uglier, but it is even
>>> better in terms of returned value.
>> We can only return -errno values, but write_sync() may do partial writes
>> so it may return non-negative values which still indicate an error. So
>> we'd have to check whether the return value is negative, if it is,
>> return that, if it isn't but if it's still below what we wanted to
>> write, return a fixed error (such as -EIO). I'd rather just return -EIO
>> and be done with it, but if you really want me to, I can of course do it
>> differently.
> nbd_wr_sync doesn't do that, it always returns negative errno for a partial
> error:

qemu_send() might do a partial send, returning a value smaller than len. 
nbd_wr_sync() will try iterating until everything has been sent, but if 
send() returns 0, the loop is aborted and a value smaller than len may 
be returned.

Maybe send() never returns 0, in which case nbd_wr_sync() will actually 
return either len or -errno, but this isn't clear from the structure of 
nbd_wr_sync(). If you really want to pass the value returned from 
nbd_wr_sync(), I'd rather restructure that function so that it always 
returns either len or -errno.

Max

>
>          if (len < 0) {
>              err = socket_error();
>
>              /* recoverable error */
>              if (err == EINTR || (offset > 0 && (err == EAGAIN || err == EWOULDBLOCK))) {
>                  continue;
>              }
>
>              /* unrecoverable error */
>              return -err;
>          }
>
> The precise error can be useful to distinguish a network error from something
> else.  I'm just in doubt about partial reads; those can return a positive error,
> in which case you can return ESHUTDOWN (in read_sync).
>
> Paolo

Re: [Qemu-devel] [PATCH 05/25] nbd: Avoid generic -EINVAL

[Paolo Bonzini]

On 16/03/2015 15:48, Max Reitz wrote:
> On 2015-03-16 at 10:42, Paolo Bonzini wrote:
>>
>> On 16/03/2015 14:51, Max Reitz wrote:
>>>> Propagating the return value from write_sync is uglier, but it is even
>>>> better in terms of returned value.
>>> We can only return -errno values, but write_sync() may do partial writes
>>> so it may return non-negative values which still indicate an error. So
>>> we'd have to check whether the return value is negative, if it is,
>>> return that, if it isn't but if it's still below what we wanted to
>>> write, return a fixed error (such as -EIO). I'd rather just return -EIO
>>> and be done with it, but if you really want me to, I can of course do it
>>> differently.
>> nbd_wr_sync doesn't do that, it always returns negative errno for a
>> partial
>> error:
> 
> qemu_send() might do a partial send, returning a value smaller than len.
> nbd_wr_sync() will try iterating until everything has been sent, but if
> send() returns 0, the loop is aborted and a value smaller than len may
> be returned.

Indeed, send() will never return 0.  It will return either EPIPE or
EWOULDBLOCK.  recv() on the other hand can return 0 or EWOULDBLOCK.

Paolo

> Maybe send() never returns 0, in which case nbd_wr_sync() will actually
> return either len or -errno, but this isn't clear from the structure of
> nbd_wr_sync(). If you really want to pass the value returned from
> nbd_wr_sync(), I'd rather restructure that function so that it always
> returns either len or -errno.
> 
> Max
> 
>>
>>          if (len < 0) {
>>              err = socket_error();
>>
>>              /* recoverable error */
>>              if (err == EINTR || (offset > 0 && (err == EAGAIN || err
>> == EWOULDBLOCK))) {
>>                  continue;
>>              }
>>
>>              /* unrecoverable error */
>>              return -err;
>>          }
>>
>> The precise error can be useful to distinguish a network error from
>> something
>> else.  I'm just in doubt about partial reads; those can return a
>> positive error,
>> in which case you can return ESHUTDOWN (in read_sync).
>>
>> Paolo
> 

[Qemu-devel] [PATCH 06/25] nbd: Pass return value from nbd_handle_list()

[Max Reitz]

While it does not make a difference in practice, nbd_receive_options()
generally returns -errno, so it should do that here as well; and the
easiest way to achieve this is by passing on the value returned by
nbd_handle_list().

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 nbd.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/nbd.c b/nbd.c
index d2eb1c5..77d1158 100644
--- a/nbd.c
+++ b/nbd.c
@@ -352,7 +352,7 @@ fail:
 static int nbd_receive_options(NBDClient *client)
 {
     while (1) {
-        int csock = client->sock;
+        int csock = client->sock, ret;
         uint32_t tmp, length;
         uint64_t magic;
 
@@ -399,8 +399,9 @@ static int nbd_receive_options(NBDClient *client)
         TRACE("Checking option");
         switch (be32_to_cpu(tmp)) {
         case NBD_OPT_LIST:
-            if (nbd_handle_list(client, length) < 0) {
-                return 1;
+            ret = nbd_handle_list(client, length);
+            if (ret < 0) {
+                return ret;
             }
             break;
 
-- 
2.1.0

[Qemu-devel] [PATCH 07/25] nbd: Add "failed to open export" error message

[Max Reitz]

In case the connection is closed before the export length can be read,
and an export name had been specified, this generally indicates that for
some reason the export could not be opened (e.g. there is no export with
that name). Make the error message reflect this.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 nbd.c                      | 6 +++++-
 tests/qemu-iotests/096.out | 2 +-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/nbd.c b/nbd.c
index 77d1158..ad0948b 100644
--- a/nbd.c
+++ b/nbd.c
@@ -600,7 +600,11 @@ int nbd_receive_negotiate(int csock, const char *name, uint32_t *flags,
     }
 
     if (read_sync(csock, &s, sizeof(s)) != sizeof(s)) {
-        error_setg(errp, "Failed to read export length");
+        if (name) {
+            error_setg(errp, "Failed to open export");
+        } else {
+            error_setg(errp, "Failed to read export length");
+        }
         goto fail;
     }
     *size = be64_to_cpu(s);
diff --git a/tests/qemu-iotests/096.out b/tests/qemu-iotests/096.out
index cc10e51..80d542a 100644
--- a/tests/qemu-iotests/096.out
+++ b/tests/qemu-iotests/096.out
@@ -9,7 +9,7 @@ read 65536/65536 bytes at offset 0
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "drv", "tray-open": true}}
 {"return": {}}
-qemu-io: can't open device nbd://127.0.0.1:PORT/drv: Failed to read export length
+qemu-io: can't open device nbd://127.0.0.1:PORT/drv: Failed to open export
 no file open, try 'help open'
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN"}
-- 
2.1.0

Re: [Qemu-devel] [PATCH 07/25] nbd: Add "failed to open export" error message

[Paolo Bonzini]

On 25/02/2015 19:08, Max Reitz wrote:
> In case the connection is closed before the export length can be read,
> and an export name had been specified, this generally indicates that for
> some reason the export could not be opened (e.g. there is no export with
> that name). Make the error message reflect this.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Which patch introduces tests/qemu-iotests/096.out?

Paolo

> ---
>  nbd.c                      | 6 +++++-
>  tests/qemu-iotests/096.out | 2 +-
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/nbd.c b/nbd.c
> index 77d1158..ad0948b 100644
> --- a/nbd.c
> +++ b/nbd.c
> @@ -600,7 +600,11 @@ int nbd_receive_negotiate(int csock, const char *name, uint32_t *flags,
>      }
>  
>      if (read_sync(csock, &s, sizeof(s)) != sizeof(s)) {
> -        error_setg(errp, "Failed to read export length");
> +        if (name) {
> +            error_setg(errp, "Failed to open export");
> +        } else {
> +            error_setg(errp, "Failed to read export length");
> +        }
>          goto fail;
>      }
>      *size = be64_to_cpu(s);
> diff --git a/tests/qemu-iotests/096.out b/tests/qemu-iotests/096.out
> index cc10e51..80d542a 100644
> --- a/tests/qemu-iotests/096.out
> +++ b/tests/qemu-iotests/096.out
> @@ -9,7 +9,7 @@ read 65536/65536 bytes at offset 0
>  64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>  {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "drv", "tray-open": true}}
>  {"return": {}}
> -qemu-io: can't open device nbd://127.0.0.1:PORT/drv: Failed to read export length
> +qemu-io: can't open device nbd://127.0.0.1:PORT/drv: Failed to open export
>  no file open, try 'help open'
>  {"return": {}}
>  {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN"}
> 

Re: [Qemu-devel] [PATCH 07/25] nbd: Add "failed to open export" error message

[Max Reitz]

On 2015-03-11 at 07:24, Paolo Bonzini wrote:
>
> On 25/02/2015 19:08, Max Reitz wrote:
>> In case the connection is closed before the export length can be read,
>> and an export name had been specified, this generally indicates that for
>> some reason the export could not be opened (e.g. there is no export with
>> that name). Make the error message reflect this.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Which patch introduces tests/qemu-iotests/096.out?

As I forgot to write in the cover letter (and only wrote in a reply to 
it), the series actually 'depends on v3 (or any later version) of my 
series "block: Rework bdrv_close_all()"', so the patch that introduces 
it would be "iotests: Add test for eject under NBD server".

Max

[Qemu-devel] [PATCH 08/25] nbd: Handle blk_getlength() failure

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 blockdev-nbd.c      |  6 +++++-
 include/block/nbd.h |  3 ++-
 nbd.c               | 19 ++++++++++++++++---
 qemu-nbd.c          | 10 +++++++++-
 4 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index eb5f9a0..46482a8 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -80,7 +80,11 @@ void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
         writable = false;
     }
 
-    exp = nbd_export_new(blk, 0, -1, writable ? 0 : NBD_FLAG_READ_ONLY, NULL);
+    exp = nbd_export_new(blk, 0, -1, writable ? 0 : NBD_FLAG_READ_ONLY, NULL,
+                         errp);
+    if (!exp) {
+        return;
+    }
 
     nbd_export_set_name(exp, device);
 }
diff --git a/include/block/nbd.h b/include/block/nbd.h
index ca9a5ac..2c20138 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -86,7 +86,8 @@ typedef struct NBDExport NBDExport;
 typedef struct NBDClient NBDClient;
 
 NBDExport *nbd_export_new(BlockBackend *blk, off_t dev_offset, off_t size,
-                          uint32_t nbdflags, void (*close)(NBDExport *));
+                          uint32_t nbdflags, void (*close)(NBDExport *),
+                          Error **errp);
 void nbd_export_close(NBDExport *exp);
 void nbd_export_get(NBDExport *exp);
 void nbd_export_put(NBDExport *exp);
diff --git a/nbd.c b/nbd.c
index ad0948b..ddc2bd8 100644
--- a/nbd.c
+++ b/nbd.c
@@ -986,21 +986,30 @@ static void nbd_eject_notifier(Notifier *n, void *data)
 }
 
 NBDExport *nbd_export_new(BlockBackend *blk, off_t dev_offset, off_t size,
-                          uint32_t nbdflags, void (*close)(NBDExport *))
+                          uint32_t nbdflags, void (*close)(NBDExport *),
+                          Error **errp)
 {
-    NBDEjectNotifier *n = g_new0(NBDEjectNotifier, 1);
+    NBDEjectNotifier *n;
     NBDExport *exp = g_malloc0(sizeof(NBDExport));
     exp->refcount = 1;
     QTAILQ_INIT(&exp->clients);
     exp->blk = blk;
     exp->dev_offset = dev_offset;
     exp->nbdflags = nbdflags;
-    exp->size = size == -1 ? blk_getlength(blk) : size;
+    exp->size = size < 0 ? blk_getlength(blk) : size;
+    if (exp->size < 0) {
+        error_setg_errno(errp, -exp->size,
+                         "Failed to determine the NBD export's length");
+        goto fail;
+    }
+    exp->size -= exp->size % BDRV_SECTOR_SIZE;
+
     exp->close = close;
     exp->ctx = blk_get_aio_context(blk);
     blk_ref(blk);
     blk_add_aio_context_notifier(blk, blk_aio_attached, blk_aio_detach, exp);
 
+    n = g_new0(NBDEjectNotifier, 1);
     n->n.notify = nbd_eject_notifier;
     n->exp = exp;
     QTAILQ_INSERT_TAIL(&eject_notifiers, n, next);
@@ -1014,6 +1023,10 @@ NBDExport *nbd_export_new(BlockBackend *blk, off_t dev_offset, off_t size,
      */
     blk_invalidate_cache(blk, NULL);
     return exp;
+
+fail:
+    g_free(exp);
+    return NULL;
 }
 
 NBDExport *nbd_export_find(const char *name)
diff --git a/qemu-nbd.c b/qemu-nbd.c
index cfdc4dc..9b9d40d 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -721,6 +721,10 @@ int main(int argc, char **argv)
 
     bs->detect_zeroes = detect_zeroes;
     fd_size = blk_getlength(blk);
+    if (fd_size < 0) {
+        errx(EXIT_FAILURE, "Failed to determine the image length: %s",
+             strerror(-fd_size));
+    }
 
     if (partition != -1) {
         ret = find_partition(blk, partition, &dev_offset, &fd_size);
@@ -730,7 +734,11 @@ int main(int argc, char **argv)
         }
     }
 
-    exp = nbd_export_new(blk, dev_offset, fd_size, nbdflags, nbd_export_closed);
+    exp = nbd_export_new(blk, dev_offset, fd_size, nbdflags, nbd_export_closed,
+                         &local_err);
+    if (!exp) {
+        errx(EXIT_FAILURE, "%s", error_get_pretty(local_err));
+    }
 
     if (sockpath) {
         fd = unix_socket_incoming(sockpath);
-- 
2.1.0

Re: [Qemu-devel] [PATCH 08/25] nbd: Handle blk_getlength() failure

[Paolo Bonzini]

On 25/02/2015 19:08, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  blockdev-nbd.c      |  6 +++++-
>  include/block/nbd.h |  3 ++-
>  nbd.c               | 19 ++++++++++++++++---
>  qemu-nbd.c          | 10 +++++++++-
>  4 files changed, 32 insertions(+), 6 deletions(-)
> 
> diff --git a/blockdev-nbd.c b/blockdev-nbd.c
> index eb5f9a0..46482a8 100644
> --- a/blockdev-nbd.c
> +++ b/blockdev-nbd.c
> @@ -80,7 +80,11 @@ void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
>          writable = false;
>      }
>  
> -    exp = nbd_export_new(blk, 0, -1, writable ? 0 : NBD_FLAG_READ_ONLY, NULL);
> +    exp = nbd_export_new(blk, 0, -1, writable ? 0 : NBD_FLAG_READ_ONLY, NULL,
> +                         errp);
> +    if (!exp) {
> +        return;
> +    }
>  
>      nbd_export_set_name(exp, device);
>  }
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index ca9a5ac..2c20138 100644
> --- a/include/block/nbd.h
> +++ b/include/block/nbd.h
> @@ -86,7 +86,8 @@ typedef struct NBDExport NBDExport;
>  typedef struct NBDClient NBDClient;
>  
>  NBDExport *nbd_export_new(BlockBackend *blk, off_t dev_offset, off_t size,
> -                          uint32_t nbdflags, void (*close)(NBDExport *));
> +                          uint32_t nbdflags, void (*close)(NBDExport *),
> +                          Error **errp);
>  void nbd_export_close(NBDExport *exp);
>  void nbd_export_get(NBDExport *exp);
>  void nbd_export_put(NBDExport *exp);
> diff --git a/nbd.c b/nbd.c
> index ad0948b..ddc2bd8 100644
> --- a/nbd.c
> +++ b/nbd.c
> @@ -986,21 +986,30 @@ static void nbd_eject_notifier(Notifier *n, void *data)
>  }
>  
>  NBDExport *nbd_export_new(BlockBackend *blk, off_t dev_offset, off_t size,
> -                          uint32_t nbdflags, void (*close)(NBDExport *))
> +                          uint32_t nbdflags, void (*close)(NBDExport *),
> +                          Error **errp)
>  {
> -    NBDEjectNotifier *n = g_new0(NBDEjectNotifier, 1);
> +    NBDEjectNotifier *n;

This hunk doesn't apply yet, but I've fixed it up.

Paolo

>      NBDExport *exp = g_malloc0(sizeof(NBDExport));
>      exp->refcount = 1;
>      QTAILQ_INIT(&exp->clients);
>      exp->blk = blk;
>      exp->dev_offset = dev_offset;
>      exp->nbdflags = nbdflags;
> -    exp->size = size == -1 ? blk_getlength(blk) : size;
> +    exp->size = size < 0 ? blk_getlength(blk) : size;
> +    if (exp->size < 0) {
> +        error_setg_errno(errp, -exp->size,
> +                         "Failed to determine the NBD export's length");
> +        goto fail;
> +    }
> +    exp->size -= exp->size % BDRV_SECTOR_SIZE;
> +
>      exp->close = close;
>      exp->ctx = blk_get_aio_context(blk);
>      blk_ref(blk);
>      blk_add_aio_context_notifier(blk, blk_aio_attached, blk_aio_detach, exp);
>  
> +    n = g_new0(NBDEjectNotifier, 1);
>      n->n.notify = nbd_eject_notifier;
>      n->exp = exp;
>      QTAILQ_INSERT_TAIL(&eject_notifiers, n, next);
> @@ -1014,6 +1023,10 @@ NBDExport *nbd_export_new(BlockBackend *blk, off_t dev_offset, off_t size,
>       */
>      blk_invalidate_cache(blk, NULL);
>      return exp;
> +
> +fail:
> +    g_free(exp);
> +    return NULL;
>  }
>  
>  NBDExport *nbd_export_find(const char *name)
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index cfdc4dc..9b9d40d 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -721,6 +721,10 @@ int main(int argc, char **argv)
>  
>      bs->detect_zeroes = detect_zeroes;
>      fd_size = blk_getlength(blk);
> +    if (fd_size < 0) {
> +        errx(EXIT_FAILURE, "Failed to determine the image length: %s",
> +             strerror(-fd_size));
> +    }
>  
>      if (partition != -1) {
>          ret = find_partition(blk, partition, &dev_offset, &fd_size);
> @@ -730,7 +734,11 @@ int main(int argc, char **argv)
>          }
>      }
>  
> -    exp = nbd_export_new(blk, dev_offset, fd_size, nbdflags, nbd_export_closed);
> +    exp = nbd_export_new(blk, dev_offset, fd_size, nbdflags, nbd_export_closed,
> +                         &local_err);
> +    if (!exp) {
> +        errx(EXIT_FAILURE, "%s", error_get_pretty(local_err));
> +    }
>  
>      if (sockpath) {
>          fd = unix_socket_incoming(sockpath);
> 

[Qemu-devel] [PATCH 09/25] qemu-nbd: fork() can fail

[Max Reitz]

It is very unlikely, but it is possible.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 qemu-nbd.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index 9b9d40d..c9ed003 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -638,7 +638,9 @@ int main(int argc, char **argv)
          * print errors and exit with the proper status code.
          */
         pid = fork();
-        if (pid == 0) {
+        if (pid < 0) {
+            err(EXIT_FAILURE, "Failed to fork");
+        } else if (pid == 0) {
             close(stderr_fd[0]);
             ret = qemu_daemon(1, 0);
 
-- 
2.1.0

[Qemu-devel] [PATCH 10/25] nbd: Fix potential signed overflow issues

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 include/block/nbd.h | 4 ++--
 qemu-nbd.c          | 5 +++--
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/include/block/nbd.h b/include/block/nbd.h
index 2c20138..53726e8 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -54,8 +54,8 @@ struct nbd_reply {
 /* Reply types. */
 #define NBD_REP_ACK             (1)             /* Data sending finished. */
 #define NBD_REP_SERVER          (2)             /* Export description. */
-#define NBD_REP_ERR_UNSUP       ((1 << 31) | 1) /* Unknown option. */
-#define NBD_REP_ERR_INVALID     ((1 << 31) | 3) /* Invalid length. */
+#define NBD_REP_ERR_UNSUP       ((UINT32_C(1) << 31) | 1) /* Unknown option. */
+#define NBD_REP_ERR_INVALID     ((UINT32_C(1) << 31) | 3) /* Invalid length. */
 
 #define NBD_CMD_MASK_COMMAND	0x0000ffff
 #define NBD_CMD_FLAG_FUA	(1 << 16)
diff --git a/qemu-nbd.c b/qemu-nbd.c
index c9ed003..fd1e0c8 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -142,8 +142,9 @@ static void read_partition(uint8_t *p, struct partition_record *r)
     r->end_head = p[5];
     r->end_cylinder = p[7] | ((p[6] << 2) & 0x300);
     r->end_sector = p[6] & 0x3f;
-    r->start_sector_abs = p[8] | p[9] << 8 | p[10] << 16 | p[11] << 24;
-    r->nb_sectors_abs = p[12] | p[13] << 8 | p[14] << 16 | p[15] << 24;
+
+    r->start_sector_abs = le32_to_cpup((uint32_t *)(p +  8));
+    r->nb_sectors_abs   = le32_to_cpup((uint32_t *)(p + 12));
 }
 
 static int find_partition(BlockBackend *blk, int partition,
-- 
2.1.0

Re: [Qemu-devel] [PATCH 10/25] nbd: Fix potential signed overflow issues

[Paolo Bonzini]

On 25/02/2015 19:08, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  include/block/nbd.h | 4 ++--
>  qemu-nbd.c          | 5 +++--
>  2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index 2c20138..53726e8 100644
> --- a/include/block/nbd.h
> +++ b/include/block/nbd.h
> @@ -54,8 +54,8 @@ struct nbd_reply {
>  /* Reply types. */
>  #define NBD_REP_ACK             (1)             /* Data sending finished. */
>  #define NBD_REP_SERVER          (2)             /* Export description. */
> -#define NBD_REP_ERR_UNSUP       ((1 << 31) | 1) /* Unknown option. */
> -#define NBD_REP_ERR_INVALID     ((1 << 31) | 3) /* Invalid length. */
> +#define NBD_REP_ERR_UNSUP       ((UINT32_C(1) << 31) | 1) /* Unknown option. */
> +#define NBD_REP_ERR_INVALID     ((UINT32_C(1) << 31) | 3) /* Invalid length. */

Easier to just use 0x80000001u and 0x80000003u; changed locally.

>  
>  #define NBD_CMD_MASK_COMMAND	0x0000ffff
>  #define NBD_CMD_FLAG_FUA	(1 << 16)
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index c9ed003..fd1e0c8 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -142,8 +142,9 @@ static void read_partition(uint8_t *p, struct partition_record *r)
>      r->end_head = p[5];
>      r->end_cylinder = p[7] | ((p[6] << 2) & 0x300);
>      r->end_sector = p[6] & 0x3f;
> -    r->start_sector_abs = p[8] | p[9] << 8 | p[10] << 16 | p[11] << 24;
> -    r->nb_sectors_abs = p[12] | p[13] << 8 | p[14] << 16 | p[15] << 24;
> +
> +    r->start_sector_abs = le32_to_cpup((uint32_t *)(p +  8));
> +    r->nb_sectors_abs   = le32_to_cpup((uint32_t *)(p + 12));

By accepting uint32_t*, le32_to_cpup  is not safe if p is not properly
aligned.  ldl_le_p is better in this case.

Paolo

>  }
>  
>  static int find_partition(BlockBackend *blk, int partition,
> 

[Qemu-devel] [PATCH 11/25] qemu-nbd: Fix and improve input verification

[Max Reitz]

This patch makes sure the result of strtol() does not overflow (by
storing it in long integers instead of plain integers, and by checking
errno), allows the user to specify "--discard on" and
"--detect-zeroes unmap" in any order and strips the trailing \n from two
error messages.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 qemu-nbd.c | 40 +++++++++++++++++++++++++++-------------
 1 file changed, 27 insertions(+), 13 deletions(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index fd1e0c8..7376a35 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -51,7 +51,7 @@ static char *srcpath;
 static char *sockpath;
 static int persistent = 0;
 static enum { RUNNING, TERMINATE, TERMINATING, TERMINATED } state;
-static int shared = 1;
+static long shared = 1;
 static int nb_fds;
 
 static void usage(const char *name)
@@ -432,10 +432,10 @@ int main(int argc, char **argv)
     };
     int ch;
     int opt_ind = 0;
-    int li;
+    long li;
     char *end;
     int flags = BDRV_O_RDWR;
-    int partition = -1;
+    long partition = -1;
     int ret = 0;
     int fd;
     bool seen_cache = false;
@@ -510,11 +510,6 @@ int main(int argc, char **argv)
                 errx(EXIT_FAILURE, "Failed to parse detect_zeroes mode: %s", 
                      error_get_pretty(local_err));
             }
-            if (detect_zeroes == BLOCKDEV_DETECT_ZEROES_OPTIONS_UNMAP &&
-                !(flags & BDRV_O_UNMAP)) {
-                errx(EXIT_FAILURE, "setting detect-zeroes to unmap is not allowed "
-                                   "without setting discard operation to unmap"); 
-            }
             break;
         case 'b':
             bindto = optarg;
@@ -530,13 +525,17 @@ int main(int argc, char **argv)
             port = (uint16_t)li;
             break;
         case 'o':
-                dev_offset = strtoll (optarg, &end, 0);
+            errno = 0;
+            dev_offset = strtoll(optarg, &end, 0);
             if (*end) {
                 errx(EXIT_FAILURE, "Invalid offset `%s'", optarg);
             }
             if (dev_offset < 0) {
                 errx(EXIT_FAILURE, "Offset must be positive `%s'", optarg);
             }
+            if (errno) {
+                err(EXIT_FAILURE, "Invalid offset `%s'", optarg);
+            }
             break;
         case 'l':
             if (strstart(optarg, SNAPSHOT_OPT_BASE, NULL)) {
@@ -559,13 +558,13 @@ int main(int argc, char **argv)
                 errx(EXIT_FAILURE, "Invalid partition `%s'", optarg);
             }
             if (partition < 1 || partition > 8) {
-                errx(EXIT_FAILURE, "Invalid partition %d", partition);
+                errx(EXIT_FAILURE, "Invalid partition %s", optarg);
             }
             break;
         case 'k':
             sockpath = optarg;
             if (sockpath[0] != '/') {
-                errx(EXIT_FAILURE, "socket path must be absolute\n");
+                errx(EXIT_FAILURE, "socket path must be absolute");
             }
             break;
         case 'd':
@@ -580,7 +579,12 @@ int main(int argc, char **argv)
                 errx(EXIT_FAILURE, "Invalid shared device number '%s'", optarg);
             }
             if (shared < 1) {
-                errx(EXIT_FAILURE, "Shared device number must be greater than 0\n");
+                errx(EXIT_FAILURE,
+                     "Shared device number must be greater than 0");
+            }
+            if (shared >= INT_MAX) {
+                errx(EXIT_FAILURE,
+                     "Shared device number must be less than %i", INT_MAX);
             }
             break;
         case 'f':
@@ -606,6 +610,12 @@ int main(int argc, char **argv)
         }
     }
 
+    if (detect_zeroes == BLOCKDEV_DETECT_ZEROES_OPTIONS_UNMAP &&
+        !(flags & BDRV_O_UNMAP)) {
+        errx(EXIT_FAILURE, "Setting detect-zeroes to unmap is not allowed "
+                           "without setting discard operation to unmap");
+    }
+
     if ((argc - optind) != 1) {
         errx(EXIT_FAILURE, "Invalid number of argument.\n"
              "Try `%s --help' for more information.",
@@ -730,10 +740,14 @@ int main(int argc, char **argv)
     }
 
     if (partition != -1) {
+        if (dev_offset) {
+            errx(EXIT_FAILURE, "Cannot use both -o and -P at the same time");
+        }
+
         ret = find_partition(blk, partition, &dev_offset, &fd_size);
         if (ret < 0) {
             errno = -ret;
-            err(EXIT_FAILURE, "Could not find partition %d", partition);
+            err(EXIT_FAILURE, "Could not find partition %ld", partition);
         }
     }
 
-- 
2.1.0

Re: [Qemu-devel] [PATCH 11/25] qemu-nbd: Fix and improve input verification

[Paolo Bonzini]

On 25/02/2015 19:08, Max Reitz wrote:
> This patch makes sure the result of strtol() does not overflow (by
> storing it in long integers instead of plain integers, and by checking
> errno), allows the user to specify "--discard on" and
> "--detect-zeroes unmap" in any order and strips the trailing \n from two
> error messages.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  qemu-nbd.c | 40 +++++++++++++++++++++++++++-------------
>  1 file changed, 27 insertions(+), 13 deletions(-)

Let's introduce a general purpose utility instead of duplicating the 
checks in every strto* caller.

For example, you can ensure that errno is checked and, if NULL is 
passed as the second argument, that the whole string is a number. 
Something like this:

   int qemu_strtol(const char *name, const char **next, int base, long *result)
   {
       char *p;
       errno = 0;
       *result = strtol(name, &p, base);
       if (!next && *p) {
           return -EINVAL;
       }
       if (next) {
           *next = p;
       }
       return -errno;
   }

Paolo

> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index fd1e0c8..7376a35 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -51,7 +51,7 @@ static char *srcpath;
>  static char *sockpath;
>  static int persistent = 0;
>  static enum { RUNNING, TERMINATE, TERMINATING, TERMINATED } state;
> -static int shared = 1;
> +static long shared = 1;
>  static int nb_fds;
>  
>  static void usage(const char *name)
> @@ -432,10 +432,10 @@ int main(int argc, char **argv)
>      };
>      int ch;
>      int opt_ind = 0;
> -    int li;
> +    long li;
>      char *end;
>      int flags = BDRV_O_RDWR;
> -    int partition = -1;
> +    long partition = -1;
>      int ret = 0;
>      int fd;
>      bool seen_cache = false;
> @@ -510,11 +510,6 @@ int main(int argc, char **argv)
>                  errx(EXIT_FAILURE, "Failed to parse detect_zeroes mode: %s", 
>                       error_get_pretty(local_err));
>              }
> -            if (detect_zeroes == BLOCKDEV_DETECT_ZEROES_OPTIONS_UNMAP &&
> -                !(flags & BDRV_O_UNMAP)) {
> -                errx(EXIT_FAILURE, "setting detect-zeroes to unmap is not allowed "
> -                                   "without setting discard operation to unmap"); 
> -            }
>              break;
>          case 'b':
>              bindto = optarg;
> @@ -530,13 +525,17 @@ int main(int argc, char **argv)
>              port = (uint16_t)li;
>              break;
>          case 'o':
> -                dev_offset = strtoll (optarg, &end, 0);
> +            errno = 0;
> +            dev_offset = strtoll(optarg, &end, 0);
>              if (*end) {
>                  errx(EXIT_FAILURE, "Invalid offset `%s'", optarg);
>              }
>              if (dev_offset < 0) {
>                  errx(EXIT_FAILURE, "Offset must be positive `%s'", optarg);
>              }
> +            if (errno) {
> +                err(EXIT_FAILURE, "Invalid offset `%s'", optarg);
> +            }
>              break;
>          case 'l':
>              if (strstart(optarg, SNAPSHOT_OPT_BASE, NULL)) {
> @@ -559,13 +558,13 @@ int main(int argc, char **argv)
>                  errx(EXIT_FAILURE, "Invalid partition `%s'", optarg);
>              }
>              if (partition < 1 || partition > 8) {
> -                errx(EXIT_FAILURE, "Invalid partition %d", partition);
> +                errx(EXIT_FAILURE, "Invalid partition %s", optarg);
>              }
>              break;
>          case 'k':
>              sockpath = optarg;
>              if (sockpath[0] != '/') {
> -                errx(EXIT_FAILURE, "socket path must be absolute\n");
> +                errx(EXIT_FAILURE, "socket path must be absolute");
>              }
>              break;
>          case 'd':
> @@ -580,7 +579,12 @@ int main(int argc, char **argv)
>                  errx(EXIT_FAILURE, "Invalid shared device number '%s'", optarg);
>              }
>              if (shared < 1) {
> -                errx(EXIT_FAILURE, "Shared device number must be greater than 0\n");
> +                errx(EXIT_FAILURE,
> +                     "Shared device number must be greater than 0");
> +            }
> +            if (shared >= INT_MAX) {
> +                errx(EXIT_FAILURE,
> +                     "Shared device number must be less than %i", INT_MAX);
>              }
>              break;
>          case 'f':
> @@ -606,6 +610,12 @@ int main(int argc, char **argv)
>          }
>      }
>  
> +    if (detect_zeroes == BLOCKDEV_DETECT_ZEROES_OPTIONS_UNMAP &&
> +        !(flags & BDRV_O_UNMAP)) {
> +        errx(EXIT_FAILURE, "Setting detect-zeroes to unmap is not allowed "
> +                           "without setting discard operation to unmap");
> +    }
> +
>      if ((argc - optind) != 1) {
>          errx(EXIT_FAILURE, "Invalid number of argument.\n"
>               "Try `%s --help' for more information.",
> @@ -730,10 +740,14 @@ int main(int argc, char **argv)
>      }
>  
>      if (partition != -1) {
> +        if (dev_offset) {
> +            errx(EXIT_FAILURE, "Cannot use both -o and -P at the same time");
> +        }
> +
>          ret = find_partition(blk, partition, &dev_offset, &fd_size);
>          if (ret < 0) {
>              errno = -ret;
> -            err(EXIT_FAILURE, "Could not find partition %d", partition);
> +            err(EXIT_FAILURE, "Could not find partition %ld", partition);
>          }
>      }
>  
> 

Re: [Qemu-devel] [PATCH 11/25] qemu-nbd: Fix and improve input verification

[Max Reitz]

On 2015-03-11 at 07:30, Paolo Bonzini wrote:
>
> On 25/02/2015 19:08, Max Reitz wrote:
>> This patch makes sure the result of strtol() does not overflow (by
>> storing it in long integers instead of plain integers, and by checking
>> errno), allows the user to specify "--discard on" and
>> "--detect-zeroes unmap" in any order and strips the trailing \n from two
>> error messages.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   qemu-nbd.c | 40 +++++++++++++++++++++++++++-------------
>>   1 file changed, 27 insertions(+), 13 deletions(-)
> Let's introduce a general purpose utility instead of duplicating the
> checks in every strto* caller.
>
> For example, you can ensure that errno is checked and, if NULL is
> passed as the second argument, that the whole string is a number.
> Something like this:
>
>     int qemu_strtol(const char *name, const char **next, int base, long *result)
>     {
>         char *p;
>         errno = 0;
>         *result = strtol(name, &p, base);
>         if (!next && *p) {
>             return -EINVAL;
>         }
>         if (next) {
>             *next = p;
>         }
>         return -errno;
>     }

I was afraid you might say this... Will do.


Thank you for reviewing!

Max

> Paolo
>
>> diff --git a/qemu-nbd.c b/qemu-nbd.c
>> index fd1e0c8..7376a35 100644
>> --- a/qemu-nbd.c
>> +++ b/qemu-nbd.c
>> @@ -51,7 +51,7 @@ static char *srcpath;
>>   static char *sockpath;
>>   static int persistent = 0;
>>   static enum { RUNNING, TERMINATE, TERMINATING, TERMINATED } state;
>> -static int shared = 1;
>> +static long shared = 1;
>>   static int nb_fds;
>>   
>>   static void usage(const char *name)
>> @@ -432,10 +432,10 @@ int main(int argc, char **argv)
>>       };
>>       int ch;
>>       int opt_ind = 0;
>> -    int li;
>> +    long li;
>>       char *end;
>>       int flags = BDRV_O_RDWR;
>> -    int partition = -1;
>> +    long partition = -1;
>>       int ret = 0;
>>       int fd;
>>       bool seen_cache = false;
>> @@ -510,11 +510,6 @@ int main(int argc, char **argv)
>>                   errx(EXIT_FAILURE, "Failed to parse detect_zeroes mode: %s",
>>                        error_get_pretty(local_err));
>>               }
>> -            if (detect_zeroes == BLOCKDEV_DETECT_ZEROES_OPTIONS_UNMAP &&
>> -                !(flags & BDRV_O_UNMAP)) {
>> -                errx(EXIT_FAILURE, "setting detect-zeroes to unmap is not allowed "
>> -                                   "without setting discard operation to unmap");
>> -            }
>>               break;
>>           case 'b':
>>               bindto = optarg;
>> @@ -530,13 +525,17 @@ int main(int argc, char **argv)
>>               port = (uint16_t)li;
>>               break;
>>           case 'o':
>> -                dev_offset = strtoll (optarg, &end, 0);
>> +            errno = 0;
>> +            dev_offset = strtoll(optarg, &end, 0);
>>               if (*end) {
>>                   errx(EXIT_FAILURE, "Invalid offset `%s'", optarg);
>>               }
>>               if (dev_offset < 0) {
>>                   errx(EXIT_FAILURE, "Offset must be positive `%s'", optarg);
>>               }
>> +            if (errno) {
>> +                err(EXIT_FAILURE, "Invalid offset `%s'", optarg);
>> +            }
>>               break;
>>           case 'l':
>>               if (strstart(optarg, SNAPSHOT_OPT_BASE, NULL)) {
>> @@ -559,13 +558,13 @@ int main(int argc, char **argv)
>>                   errx(EXIT_FAILURE, "Invalid partition `%s'", optarg);
>>               }
>>               if (partition < 1 || partition > 8) {
>> -                errx(EXIT_FAILURE, "Invalid partition %d", partition);
>> +                errx(EXIT_FAILURE, "Invalid partition %s", optarg);
>>               }
>>               break;
>>           case 'k':
>>               sockpath = optarg;
>>               if (sockpath[0] != '/') {
>> -                errx(EXIT_FAILURE, "socket path must be absolute\n");
>> +                errx(EXIT_FAILURE, "socket path must be absolute");
>>               }
>>               break;
>>           case 'd':
>> @@ -580,7 +579,12 @@ int main(int argc, char **argv)
>>                   errx(EXIT_FAILURE, "Invalid shared device number '%s'", optarg);
>>               }
>>               if (shared < 1) {
>> -                errx(EXIT_FAILURE, "Shared device number must be greater than 0\n");
>> +                errx(EXIT_FAILURE,
>> +                     "Shared device number must be greater than 0");
>> +            }
>> +            if (shared >= INT_MAX) {
>> +                errx(EXIT_FAILURE,
>> +                     "Shared device number must be less than %i", INT_MAX);
>>               }
>>               break;
>>           case 'f':
>> @@ -606,6 +610,12 @@ int main(int argc, char **argv)
>>           }
>>       }
>>   
>> +    if (detect_zeroes == BLOCKDEV_DETECT_ZEROES_OPTIONS_UNMAP &&
>> +        !(flags & BDRV_O_UNMAP)) {
>> +        errx(EXIT_FAILURE, "Setting detect-zeroes to unmap is not allowed "
>> +                           "without setting discard operation to unmap");
>> +    }
>> +
>>       if ((argc - optind) != 1) {
>>           errx(EXIT_FAILURE, "Invalid number of argument.\n"
>>                "Try `%s --help' for more information.",
>> @@ -730,10 +740,14 @@ int main(int argc, char **argv)
>>       }
>>   
>>       if (partition != -1) {
>> +        if (dev_offset) {
>> +            errx(EXIT_FAILURE, "Cannot use both -o and -P at the same time");
>> +        }
>> +
>>           ret = find_partition(blk, partition, &dev_offset, &fd_size);
>>           if (ret < 0) {
>>               errno = -ret;
>> -            err(EXIT_FAILURE, "Could not find partition %d", partition);
>> +            err(EXIT_FAILURE, "Could not find partition %ld", partition);
>>           }
>>       }
>>   
>>

[Qemu-devel] [PATCH 12/25] nbd: Set block size to BDRV_SECTOR_SIZE

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/nbd-client.c  |  3 +--
 block/nbd-client.h  |  1 -
 include/block/nbd.h |  4 ++--
 nbd.c               | 15 +++++++--------
 qemu-nbd.c          |  5 ++---
 5 files changed, 12 insertions(+), 16 deletions(-)

diff --git a/block/nbd-client.c b/block/nbd-client.c
index 259f5a3..e1bb919 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -386,8 +386,7 @@ int nbd_client_init(BlockDriverState *bs, int sock, const char *export,
     logout("session init %s\n", export);
     qemu_set_block(sock);
     ret = nbd_receive_negotiate(sock, export,
-                                &client->nbdflags, &client->size,
-                                &client->blocksize, errp);
+                                &client->nbdflags, &client->size, errp);
     if (ret < 0) {
         logout("Failed to negotiate with the NBD server\n");
         closesocket(sock);
diff --git a/block/nbd-client.h b/block/nbd-client.h
index fa4ff42..e841340 100644
--- a/block/nbd-client.h
+++ b/block/nbd-client.h
@@ -20,7 +20,6 @@ typedef struct NbdClientSession {
     int sock;
     uint32_t nbdflags;
     off_t size;
-    size_t blocksize;
 
     CoMutex send_mutex;
     CoMutex free_sema;
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 53726e8..65f409d 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -75,8 +75,8 @@ enum {
 
 ssize_t nbd_wr_sync(int fd, void *buffer, size_t size, bool do_read);
 int nbd_receive_negotiate(int csock, const char *name, uint32_t *flags,
-                          off_t *size, size_t *blocksize, Error **errp);
-int nbd_init(int fd, int csock, uint32_t flags, off_t size, size_t blocksize);
+                          off_t *size, Error **errp);
+int nbd_init(int fd, int csock, uint32_t flags, off_t size);
 ssize_t nbd_send_request(int csock, struct nbd_request *request);
 ssize_t nbd_receive_reply(int csock, struct nbd_reply *reply);
 int nbd_client(int fd);
diff --git a/nbd.c b/nbd.c
index ddc2bd8..1cd7757 100644
--- a/nbd.c
+++ b/nbd.c
@@ -496,7 +496,7 @@ fail:
 }
 
 int nbd_receive_negotiate(int csock, const char *name, uint32_t *flags,
-                          off_t *size, size_t *blocksize, Error **errp)
+                          off_t *size, Error **errp)
 {
     char buf[256];
     uint64_t magic, s;
@@ -608,7 +608,6 @@ int nbd_receive_negotiate(int csock, const char *name, uint32_t *flags,
         goto fail;
     }
     *size = be64_to_cpu(s);
-    *blocksize = 1024;
     TRACE("Size is %" PRIu64, *size);
 
     if (!name) {
@@ -635,7 +634,7 @@ fail:
 }
 
 #ifdef __linux__
-int nbd_init(int fd, int csock, uint32_t flags, off_t size, size_t blocksize)
+int nbd_init(int fd, int csock, uint32_t flags, off_t size)
 {
     TRACE("Setting NBD socket");
 
@@ -645,17 +644,17 @@ int nbd_init(int fd, int csock, uint32_t flags, off_t size, size_t blocksize)
         return -serrno;
     }
 
-    TRACE("Setting block size to %lu", (unsigned long)blocksize);
+    TRACE("Setting block size to %lu", (unsigned long)BDRV_SECTOR_SIZE);
 
-    if (ioctl(fd, NBD_SET_BLKSIZE, blocksize) < 0) {
+    if (ioctl(fd, NBD_SET_BLKSIZE, (size_t)BDRV_SECTOR_SIZE) < 0) {
         int serrno = errno;
         LOG("Failed setting NBD block size");
         return -serrno;
     }
 
-        TRACE("Setting size to %zd block(s)", (size_t)(size / blocksize));
+    TRACE("Setting size to %zd block(s)", (size_t)(size / BDRV_SECTOR_SIZE));
 
-    if (ioctl(fd, NBD_SET_SIZE_BLOCKS, size / blocksize) < 0) {
+    if (ioctl(fd, NBD_SET_SIZE_BLOCKS, size / (size_t)BDRV_SECTOR_SIZE) < 0) {
         int serrno = errno;
         LOG("Failed setting size (in blocks)");
         return -serrno;
@@ -720,7 +719,7 @@ int nbd_client(int fd)
     return ret;
 }
 #else
-int nbd_init(int fd, int csock, uint32_t flags, off_t size, size_t blocksize)
+int nbd_init(int fd, int csock, uint32_t flags, off_t size)
 {
     return -ENOTSUP;
 }
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 7376a35..511857e 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -282,7 +282,6 @@ static void *nbd_client_thread(void *arg)
 {
     char *device = arg;
     off_t size;
-    size_t blocksize;
     uint32_t nbdflags;
     int fd, sock;
     int ret;
@@ -295,7 +294,7 @@ static void *nbd_client_thread(void *arg)
     }
 
     ret = nbd_receive_negotiate(sock, NULL, &nbdflags,
-                                &size, &blocksize, &local_error);
+                                &size, &local_error);
     if (ret < 0) {
         if (local_error) {
             fprintf(stderr, "%s\n", error_get_pretty(local_error));
@@ -311,7 +310,7 @@ static void *nbd_client_thread(void *arg)
         goto out_socket;
     }
 
-    ret = nbd_init(fd, sock, nbdflags, size, blocksize);
+    ret = nbd_init(fd, sock, nbdflags, size);
     if (ret < 0) {
         goto out_fd;
     }
-- 
2.1.0

[Qemu-devel] [PATCH 13/25] nbd: Enforce sector alignment

[Max Reitz]

Operations on NBDs must be aligned to BDRV_SECTOR_SIZE. Enforce this.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 nbd.c      | 21 +++++++++++++++++++++
 qemu-nbd.c |  4 ++++
 2 files changed, 25 insertions(+)

diff --git a/nbd.c b/nbd.c
index 1cd7757..5764fd1 100644
--- a/nbd.c
+++ b/nbd.c
@@ -990,6 +990,13 @@ NBDExport *nbd_export_new(BlockBackend *blk, off_t dev_offset, off_t size,
 {
     NBDEjectNotifier *n;
     NBDExport *exp = g_malloc0(sizeof(NBDExport));
+
+    if (dev_offset % BDRV_SECTOR_SIZE) {
+        error_setg(errp, "NBD export offset must be a multiple of %i",
+                   (int)BDRV_SECTOR_SIZE);
+        goto fail;
+    }
+
     exp->refcount = 1;
     QTAILQ_INIT(&exp->clients);
     exp->blk = blk;
@@ -1257,6 +1264,20 @@ static void nbd_trip(void *opaque)
         goto invalid_request;
     }
 
+    if (command == NBD_CMD_READ ||
+        command == NBD_CMD_WRITE ||
+        command == NBD_CMD_TRIM)
+    {
+        if (request.from % BDRV_SECTOR_SIZE) {
+            goto invalid_request;
+        }
+        assert(!((request.from + exp->dev_offset) % BDRV_SECTOR_SIZE));
+
+        if (request.len % BDRV_SECTOR_SIZE) {
+            goto invalid_request;
+        }
+    }
+
     switch (command) {
     case NBD_CMD_READ:
         TRACE("Request type is READ");
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 511857e..cc13664 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -535,6 +535,10 @@ int main(int argc, char **argv)
             if (errno) {
                 err(EXIT_FAILURE, "Invalid offset `%s'", optarg);
             }
+            if (dev_offset % BDRV_SECTOR_SIZE) {
+                errx(EXIT_FAILURE, "Offset must be a multiple of %i",
+                     (int)BDRV_SECTOR_SIZE);
+            }
             break;
         case 'l':
             if (strstart(optarg, SNAPSHOT_OPT_BASE, NULL)) {
-- 
2.1.0

[Qemu-devel] [PATCH 14/25] coroutine: Add co_yield_timeout()

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 include/block/coroutine.h |  6 ++++++
 qemu-coroutine-sleep.c    | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+)

diff --git a/include/block/coroutine.h b/include/block/coroutine.h
index 20c027a..a05aefb 100644
--- a/include/block/coroutine.h
+++ b/include/block/coroutine.h
@@ -216,4 +216,10 @@ void coroutine_fn co_aio_sleep_ns(AioContext *ctx, QEMUClockType type,
  */
 void coroutine_fn yield_until_fd_readable(int fd);
 
+/**
+ * Yield with a timeout given in nanoseconds. Returns true if the timeout
+ * expired.
+ */
+bool coroutine_fn co_yield_timeout(QEMUClockType type, int64_t ns);
+
 #endif /* QEMU_COROUTINE_H */
diff --git a/qemu-coroutine-sleep.c b/qemu-coroutine-sleep.c
index 9abb7fd..9b33bf0 100644
--- a/qemu-coroutine-sleep.c
+++ b/qemu-coroutine-sleep.c
@@ -14,6 +14,7 @@
 #include "block/coroutine.h"
 #include "qemu/timer.h"
 #include "block/aio.h"
+#include "qemu/main-loop.h"
 
 typedef struct CoSleepCB {
     QEMUTimer *ts;
@@ -39,3 +40,36 @@ void coroutine_fn co_aio_sleep_ns(AioContext *ctx, QEMUClockType type,
     timer_del(sleep_cb.ts);
     timer_free(sleep_cb.ts);
 }
+
+typedef struct CoTimeoutCB {
+    Coroutine *co;
+    bool expired;
+} CoTimeoutCB;
+
+static void co_timeout_cb(void *opaque)
+{
+    CoTimeoutCB *timeout_cb = opaque;
+
+    timeout_cb->expired = true;
+    qemu_coroutine_enter(timeout_cb->co, NULL);
+}
+
+bool coroutine_fn co_yield_timeout(QEMUClockType type, int64_t ns)
+{
+    QEMUTimer timer;
+    CoTimeoutCB timeout_cb = {
+        .co = qemu_coroutine_self()
+    };
+
+    if (!ns) {
+        return true;
+    }
+
+    aio_timer_init(qemu_get_aio_context(), &timer, type, SCALE_NS,
+                   &co_timeout_cb, &timeout_cb);
+    timer_mod(&timer, qemu_clock_get_ns(type) + ns);
+    qemu_coroutine_yield();
+    timer_del(&timer);
+
+    return timeout_cb.expired;
+}
-- 
2.1.0

[Qemu-devel] [PATCH 15/25] coroutine-io: Return -errno in case of error

[Max Reitz]

In case qemu_co_sendv_recvv() fails without any data read, there is no
reason not to return the perfectly fine error number retrieved from
socket_error().

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 qemu-coroutine-io.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qemu-coroutine-io.c b/qemu-coroutine-io.c
index d404926..28dc735 100644
--- a/qemu-coroutine-io.c
+++ b/qemu-coroutine-io.c
@@ -45,7 +45,7 @@ qemu_co_sendv_recvv(int sockfd, struct iovec *iov, unsigned iov_cnt,
             if (err == EAGAIN || err == EWOULDBLOCK) {
                 qemu_coroutine_yield();
             } else if (done == 0) {
-                return -1;
+                return -err;
             } else {
                 break;
             }
-- 
2.1.0

[Qemu-devel] [PATCH 16/25] coroutine-io: Add I/O functions with timeout

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 include/qemu-common.h | 45 +++++++++++++++++++++++++++++++++++++--------
 qemu-coroutine-io.c   | 23 ++++++++++++++++-------
 2 files changed, 53 insertions(+), 15 deletions(-)

diff --git a/include/qemu-common.h b/include/qemu-common.h
index 644b46d..a268ffc 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -296,23 +296,52 @@ struct qemu_work_item {
  * Receives data into a (part of) iovec from a socket,
  * yielding when there is no data in the socket.
  * The same interface as qemu_sendv_recvv(), with added yielding.
- * XXX should mark these as coroutine_fn
  */
-ssize_t qemu_co_sendv_recvv(int sockfd, struct iovec *iov, unsigned iov_cnt,
-                            size_t offset, size_t bytes, bool do_send);
+#define qemu_co_sendv_recvv(sockfd, iov, iov_cnt, offset, bytes, do_send) \
+     qemu_co_sendv_recvv_timeout(sockfd, iov, iov_cnt, offset, bytes, do_send, \
+                                 -1)
 #define qemu_co_recvv(sockfd, iov, iov_cnt, offset, bytes) \
-  qemu_co_sendv_recvv(sockfd, iov, iov_cnt, offset, bytes, false)
+    qemu_co_sendv_recvv_timeout(sockfd, iov, iov_cnt, offset, bytes, false, -1)
 #define qemu_co_sendv(sockfd, iov, iov_cnt, offset, bytes) \
-  qemu_co_sendv_recvv(sockfd, iov, iov_cnt, offset, bytes, true)
+    qemu_co_sendv_recvv_timeout(sockfd, iov, iov_cnt, offset, bytes, true, -1)
 
 /**
  * The same as above, but with just a single buffer
  */
-ssize_t qemu_co_send_recv(int sockfd, void *buf, size_t bytes, bool do_send);
+#define qemu_co_send_recv(sockfd, buf, bytes, do_send) \
+    qemu_co_send_recv_timeout(sockfd, buf, bytes, do_send, -1)
 #define qemu_co_recv(sockfd, buf, bytes) \
-  qemu_co_send_recv(sockfd, buf, bytes, false)
+    qemu_co_send_recv_timeout(sockfd, buf, bytes, false, -1)
 #define qemu_co_send(sockfd, buf, bytes) \
-  qemu_co_send_recv(sockfd, buf, bytes, true)
+    qemu_co_send_recv_timeout(sockfd, buf, bytes, true, -1)
+
+/**
+ * The same as qemu_co_sendv_recvv(), but allows you to specify a timeout in
+ * nanoseconds.
+ * XXX should mark these as coroutine_fn
+ */
+ssize_t qemu_co_sendv_recvv_timeout(int sockfd, struct iovec *iov,
+                                    unsigned iov_cnt, size_t offset,
+                                    size_t bytes, bool do_send,
+                                    int64_t timeout);
+#define qemu_co_recvv_timeout(sockfd, iov, iov_cnt, offset, bytes, timeout) \
+    qemu_co_sendv_recvv_timeout(sockfd, iov, iov_cnt, offset, bytes, false, \
+                                timeout)
+#define qemu_co_sendv_timeout(sockfd, iov, iov_cnt, offset, bytes, timeout) \
+    qemu_co_sendv_recvv_timeout(sockfd, iov, iov_cnt, offset, bytes, true, \
+                                timeout)
+
+/**
+ * The same as qemu_co_send_recv(), but allows you to specify a timeout in
+ * nanoseconds.
+ */
+ssize_t qemu_co_send_recv_timeout(int sockfd, void *buf, size_t bytes,
+                                  bool do_send, int64_t timeout);
+#define qemu_co_recv_timeout(sockfd, buf, bytes, timeout) \
+    qemu_co_send_recv_timeout(sockfd, buf, bytes, false, timeout)
+#define qemu_co_send_timeout(sockfd, buf, bytes, timeout) \
+    qemu_co_send_recv_timeout(sockfd, buf, bytes, true, timeout)
+
 
 typedef struct QEMUIOVector {
     struct iovec *iov;
diff --git a/qemu-coroutine-io.c b/qemu-coroutine-io.c
index 28dc735..f261a6e 100644
--- a/qemu-coroutine-io.c
+++ b/qemu-coroutine-io.c
@@ -28,9 +28,10 @@
 #include "qemu/iov.h"
 #include "qemu/main-loop.h"
 
-ssize_t coroutine_fn
-qemu_co_sendv_recvv(int sockfd, struct iovec *iov, unsigned iov_cnt,
-                    size_t offset, size_t bytes, bool do_send)
+ssize_t coroutine_fn qemu_co_sendv_recvv_timeout(int sockfd, struct iovec *iov,
+                                                 unsigned iov_cnt,
+                                                 size_t offset, size_t bytes,
+                                                 bool do_send, int64_t timeout)
 {
     size_t done = 0;
     ssize_t ret;
@@ -43,7 +44,13 @@ qemu_co_sendv_recvv(int sockfd, struct iovec *iov, unsigned iov_cnt,
         } else if (ret < 0) {
             err = socket_error();
             if (err == EAGAIN || err == EWOULDBLOCK) {
-                qemu_coroutine_yield();
+                if (timeout < 0) {
+                    qemu_coroutine_yield();
+                } else {
+                    if (co_yield_timeout(QEMU_CLOCK_REALTIME, timeout)) {
+                        return -ETIMEDOUT;
+                    }
+                }
             } else if (done == 0) {
                 return -err;
             } else {
@@ -60,11 +67,13 @@ qemu_co_sendv_recvv(int sockfd, struct iovec *iov, unsigned iov_cnt,
     return done;
 }
 
-ssize_t coroutine_fn
-qemu_co_send_recv(int sockfd, void *buf, size_t bytes, bool do_send)
+ssize_t coroutine_fn qemu_co_send_recv_timeout(int sockfd, void *buf,
+                                               size_t bytes, bool do_send,
+                                               int64_t timeout)
 {
     struct iovec iov = { .iov_base = buf, .iov_len = bytes };
-    return qemu_co_sendv_recvv(sockfd, &iov, 1, 0, bytes, do_send);
+    return qemu_co_sendv_recvv_timeout(sockfd, &iov, 1, 0, bytes, do_send,
+                                       timeout);
 }
 
 typedef struct {
-- 
2.1.0

[Qemu-devel] [PATCH 17/25] nbd: Employ timeouts

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/nbd-client.c  | 35 +++++++++++++++++++++++++++++++----
 include/block/nbd.h |  2 ++
 nbd.c               | 12 ++++++++++--
 3 files changed, 43 insertions(+), 6 deletions(-)

diff --git a/block/nbd-client.c b/block/nbd-client.c
index e1bb919..be6803d 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -28,6 +28,7 @@
 
 #include "nbd-client.h"
 #include "qemu/sockets.h"
+#include "qemu/timer.h"
 
 #define HANDLE_TO_INDEX(bs, handle) ((handle) ^ ((uint64_t)(intptr_t)bs))
 #define INDEX_TO_HANDLE(bs, index)  ((index)  ^ ((uint64_t)(intptr_t)bs))
@@ -158,15 +159,21 @@ static void nbd_co_receive_reply(NbdClientSession *s,
 
     /* Wait until we're woken up by the read handler.  TODO: perhaps
      * peek at the next reply and avoid yielding if it's ours?  */
-    qemu_coroutine_yield();
+    if (co_yield_timeout(QEMU_CLOCK_REALTIME, NBD_TIMEOUT)) {
+        reply->error = ETIMEDOUT;
+        return;
+    }
+
     *reply = s->reply;
     if (reply->handle != request->handle) {
         reply->error = EIO;
     } else {
         if (qiov && reply->error == 0) {
-            ret = qemu_co_recvv(s->sock, qiov->iov, qiov->niov,
-                                offset, request->len);
-            if (ret != request->len) {
+            ret = qemu_co_recvv_timeout(s->sock, qiov->iov, qiov->niov,
+                                        offset, request->len, NBD_TIMEOUT);
+            if (ret < 0) {
+                reply->error = -ret;
+            } else if (ret != request->len) {
                 reply->error = EIO;
             }
         }
@@ -220,6 +227,11 @@ static int nbd_co_readv_1(BlockDriverState *bs, int64_t sector_num,
         nbd_co_receive_reply(client, &request, &reply, qiov, offset);
     }
     nbd_coroutine_end(client, &request);
+
+    if (reply.error == ETIMEDOUT) {
+        nbd_teardown_connection(bs);
+    }
+
     return -reply.error;
 
 }
@@ -249,6 +261,11 @@ static int nbd_co_writev_1(BlockDriverState *bs, int64_t sector_num,
         nbd_co_receive_reply(client, &request, &reply, NULL, 0);
     }
     nbd_coroutine_end(client, &request);
+
+    if (reply.error == ETIMEDOUT) {
+        nbd_teardown_connection(bs);
+    }
+
     return -reply.error;
 }
 
@@ -316,6 +333,11 @@ int nbd_client_co_flush(BlockDriverState *bs)
         nbd_co_receive_reply(client, &request, &reply, NULL, 0);
     }
     nbd_coroutine_end(client, &request);
+
+    if (reply.error == ETIMEDOUT) {
+        nbd_teardown_connection(bs);
+    }
+
     return -reply.error;
 }
 
@@ -341,6 +363,11 @@ int nbd_client_co_discard(BlockDriverState *bs, int64_t sector_num,
         nbd_co_receive_reply(client, &request, &reply, NULL, 0);
     }
     nbd_coroutine_end(client, &request);
+
+    if (reply.error == ETIMEDOUT) {
+        nbd_teardown_connection(bs);
+    }
+
     return -reply.error;
 
 }
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 65f409d..5e31986 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -73,6 +73,8 @@ enum {
 /* Maximum size of a single READ/WRITE data buffer */
 #define NBD_MAX_BUFFER_SIZE (32 * 1024 * 1024)
 
+#define NBD_TIMEOUT (INT64_C(10) * 1000 * 1000 * 1000) /* ns */
+
 ssize_t nbd_wr_sync(int fd, void *buffer, size_t size, bool do_read);
 int nbd_receive_negotiate(int csock, const char *name, uint32_t *flags,
                           off_t *size, Error **errp);
diff --git a/nbd.c b/nbd.c
index 5764fd1..a05fd02 100644
--- a/nbd.c
+++ b/nbd.c
@@ -140,12 +140,13 @@ static void nbd_update_can_read(NBDClient *client);
 
 ssize_t nbd_wr_sync(int fd, void *buffer, size_t size, bool do_read)
 {
+    int64_t deadline = qemu_clock_get_ns(QEMU_CLOCK_REALTIME) + NBD_TIMEOUT;
     size_t offset = 0;
     int err;
 
     if (qemu_in_coroutine()) {
         if (do_read) {
-            return qemu_co_recv(fd, buffer, size);
+            return qemu_co_recv_timeout(fd, buffer, size, NBD_TIMEOUT);
         } else {
             return qemu_co_send(fd, buffer, size);
         }
@@ -154,6 +155,10 @@ ssize_t nbd_wr_sync(int fd, void *buffer, size_t size, bool do_read)
     while (offset < size) {
         ssize_t len;
 
+        if (do_read && qemu_clock_get_ns(QEMU_CLOCK_REALTIME) >= deadline) {
+            return -ETIMEDOUT;
+        }
+
         if (do_read) {
             len = qemu_recv(fd, buffer + offset, size - offset, 0);
         } else {
@@ -178,6 +183,7 @@ ssize_t nbd_wr_sync(int fd, void *buffer, size_t size, bool do_read)
         }
 
         offset += len;
+        deadline = qemu_clock_get_ns(QEMU_CLOCK_REALTIME) + NBD_TIMEOUT;
     }
 
     return offset;
@@ -1208,7 +1214,9 @@ static ssize_t nbd_co_receive_request(NBDRequest *req, struct nbd_request *reque
     if (command == NBD_CMD_WRITE) {
         TRACE("Reading %u byte(s)", request->len);
 
-        if (qemu_co_recv(csock, req->data, request->len) != request->len) {
+        if (qemu_co_recv_timeout(csock, req->data, request->len, NBD_TIMEOUT)
+            != request->len)
+        {
             LOG("reading from socket failed");
             rc = -EIO;
             goto out;
-- 
2.1.0

[Qemu-devel] [PATCH 18/25] nbd: Fix nbd_receive_options()

[Max Reitz]

The client flags are sent exactly once overall, not once per option.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 nbd.c | 49 +++++++++++++++++++++++++++++--------------------
 1 file changed, 29 insertions(+), 20 deletions(-)

diff --git a/nbd.c b/nbd.c
index a05fd02..d94cd88 100644
--- a/nbd.c
+++ b/nbd.c
@@ -357,30 +357,39 @@ fail:
 
 static int nbd_receive_options(NBDClient *client)
 {
+    int csock = client->sock;
+    uint32_t flags;
+
+    /* Client sends:
+        [ 0 ..   3]   client flags
+
+        [ 0 ..   7]   NBD_OPTS_MAGIC
+        [ 8 ..  11]   NBD option
+        [12 ..  15]   Data length
+        ...           Rest of request
+
+        [ 0 ..   7]   NBD_OPTS_MAGIC
+        [ 8 ..  11]   Second NBD option
+        [12 ..  15]   Data length
+        ...           Rest of request
+    */
+
+    if (read_sync(csock, &flags, sizeof(flags)) != sizeof(flags)) {
+        LOG("read failed");
+        return -EIO;
+    }
+    TRACE("Checking client flags");
+    be32_to_cpus(&flags);
+    if (flags != 0 && flags != NBD_FLAG_C_FIXED_NEWSTYLE) {
+        LOG("Bad client flags received");
+        return -EIO;
+    }
+
     while (1) {
-        int csock = client->sock, ret;
+        int ret;
         uint32_t tmp, length;
         uint64_t magic;
 
-        /* Client sends:
-            [ 0 ..   3]   client flags
-            [ 4 ..  11]   NBD_OPTS_MAGIC
-            [12 ..  15]   NBD option
-            [16 ..  19]   length
-            ...           Rest of request
-        */
-
-        if (read_sync(csock, &tmp, sizeof(tmp)) != sizeof(tmp)) {
-            LOG("read failed");
-            return -EINVAL;
-        }
-        TRACE("Checking client flags");
-        tmp = be32_to_cpu(tmp);
-        if (tmp != 0 && tmp != NBD_FLAG_C_FIXED_NEWSTYLE) {
-            LOG("Bad client flags received");
-            return -EINVAL;
-        }
-
         if (read_sync(csock, &magic, sizeof(magic)) != sizeof(magic)) {
             LOG("read failed");
             return -EIO;
-- 
2.1.0

[Qemu-devel] [PATCH 19/25] nbd: Fix interpretation of the export flags

[Max Reitz]

The export flags are a 16 bit value, so be16_to_cpu() has to be used to
interpret them correctly. This makes discard and flush actually work
for named NBD exports (they did not work before, because the client
always assumed them to be unsupported because of the bug fixed by this
patch).

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 nbd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/nbd.c b/nbd.c
index d94cd88..aa0925a 100644
--- a/nbd.c
+++ b/nbd.c
@@ -636,7 +636,7 @@ int nbd_receive_negotiate(int csock, const char *name, uint32_t *flags,
             error_setg(errp, "Failed to read export flags");
             goto fail;
         }
-        *flags |= be32_to_cpu(tmp);
+        *flags |= be16_to_cpu(tmp);
     }
     if (read_sync(csock, &buf, 124) != 124) {
         error_setg(errp, "Failed to read reserved block");
-- 
2.1.0

[Qemu-devel] [PATCH 20/25] block/nbd: Comment on discard/flush silently failing

[Max Reitz]

If some operation cannot be performed by a block driver, it is normally
supposed to return an error. In these cases, however, it is fine to
pretend the operations were carried out successfully because if the NBD
block driver would not implement discard or flush in the first place,
this is exactly what the block layer would do.

Because this may not be obvious, add a comment for it.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/nbd-client.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block/nbd-client.c b/block/nbd-client.c
index be6803d..ab13607 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -315,6 +315,7 @@ int nbd_client_co_flush(BlockDriverState *bs)
     ssize_t ret;
 
     if (!(client->nbdflags & NBD_FLAG_SEND_FLUSH)) {
+        /* This mirrors the behavior of bdrv_co_flush() in block.c */
         return 0;
     }
 
@@ -350,6 +351,7 @@ int nbd_client_co_discard(BlockDriverState *bs, int64_t sector_num,
     ssize_t ret;
 
     if (!(client->nbdflags & NBD_FLAG_SEND_TRIM)) {
+        /* This mirrors the behavior of bdrv_co_discard() in block.c */
         return 0;
     }
     request.from = sector_num * 512;
-- 
2.1.0

Re: [Qemu-devel] [PATCH 20/25] block/nbd: Comment on discard/flush silently failing

[Paolo Bonzini]

On 25/02/2015 19:08, Max Reitz wrote:
> If some operation cannot be performed by a block driver, it is normally
> supposed to return an error. In these cases, however, it is fine to
> pretend the operations were carried out successfully because if the NBD
> block driver would not implement discard or flush in the first place,
> this is exactly what the block layer would do.
> 
> Because this may not be obvious, add a comment for it.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/nbd-client.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/block/nbd-client.c b/block/nbd-client.c
> index be6803d..ab13607 100644
> --- a/block/nbd-client.c
> +++ b/block/nbd-client.c
> @@ -315,6 +315,7 @@ int nbd_client_co_flush(BlockDriverState *bs)
>      ssize_t ret;
>  
>      if (!(client->nbdflags & NBD_FLAG_SEND_FLUSH)) {
> +        /* This mirrors the behavior of bdrv_co_flush() in block.c */
>          return 0;
>      }
>  
> @@ -350,6 +351,7 @@ int nbd_client_co_discard(BlockDriverState *bs, int64_t sector_num,
>      ssize_t ret;
>  
>      if (!(client->nbdflags & NBD_FLAG_SEND_TRIM)) {
> +        /* This mirrors the behavior of bdrv_co_discard() in block.c */
>          return 0;

Should this return -EOPNOTSUPP instead?

Paolo

>      }
>      request.from = sector_num * 512;
> 

Re: [Qemu-devel] [PATCH 20/25] block/nbd: Comment on discard/flush silently failing

[Max Reitz]

On 2015-03-11 at 07:31, Paolo Bonzini wrote:
>
> On 25/02/2015 19:08, Max Reitz wrote:
>> If some operation cannot be performed by a block driver, it is normally
>> supposed to return an error. In these cases, however, it is fine to
>> pretend the operations were carried out successfully because if the NBD
>> block driver would not implement discard or flush in the first place,
>> this is exactly what the block layer would do.
>>
>> Because this may not be obvious, add a comment for it.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   block/nbd-client.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/block/nbd-client.c b/block/nbd-client.c
>> index be6803d..ab13607 100644
>> --- a/block/nbd-client.c
>> +++ b/block/nbd-client.c
>> @@ -315,6 +315,7 @@ int nbd_client_co_flush(BlockDriverState *bs)
>>       ssize_t ret;
>>   
>>       if (!(client->nbdflags & NBD_FLAG_SEND_FLUSH)) {
>> +        /* This mirrors the behavior of bdrv_co_flush() in block.c */
>>           return 0;
>>       }
>>   
>> @@ -350,6 +351,7 @@ int nbd_client_co_discard(BlockDriverState *bs, int64_t sector_num,
>>       ssize_t ret;
>>   
>>       if (!(client->nbdflags & NBD_FLAG_SEND_TRIM)) {
>> +        /* This mirrors the behavior of bdrv_co_discard() in block.c */
>>           return 0;
> Should this return -EOPNOTSUPP instead?

That's what this patch is for. I asked myself the same thing, and it 
turns out, the functions deliberately return 0 because bdrv_co_flush() 
and bdrv_co_discard() do the same if the block driver does not support 
these functions at all, so that's why I'm adding these comments.

Max

> Paolo
>
>>       }
>>       request.from = sector_num * 512;
>>

Re: [Qemu-devel] [PATCH 20/25] block/nbd: Comment on discard/flush silently failing

[Paolo Bonzini]

On 16/03/2015 14:58, Max Reitz wrote:
>>>
>>> diff --git a/block/nbd-client.c b/block/nbd-client.c
>>> index be6803d..ab13607 100644
>>> --- a/block/nbd-client.c
>>> +++ b/block/nbd-client.c
>>> @@ -315,6 +315,7 @@ int nbd_client_co_flush(BlockDriverState *bs)
>>>       ssize_t ret;
>>>         if (!(client->nbdflags & NBD_FLAG_SEND_FLUSH)) {
>>> +        /* This mirrors the behavior of bdrv_co_flush() in block.c */
>>>           return 0;
>>>       }
>>>   @@ -350,6 +351,7 @@ int nbd_client_co_discard(BlockDriverState *bs,
>>> int64_t sector_num,
>>>       ssize_t ret;
>>>         if (!(client->nbdflags & NBD_FLAG_SEND_TRIM)) {
>>> +        /* This mirrors the behavior of bdrv_co_discard() in block.c */
>>>           return 0;
>> Should this return -EOPNOTSUPP instead?
> 
> That's what this patch is for. I asked myself the same thing, and it
> turns out, the functions deliberately return 0 because bdrv_co_flush()
> and bdrv_co_discard() do the same if the block driver does not support
> these functions at all, so that's why I'm adding these comments.

Right, but a better model than block.c should be for example
block/raw-posix.c, which returns ENOTSUP (I checked now...).

Paolo

Re: [Qemu-devel] [PATCH 20/25] block/nbd: Comment on discard/flush silently failing

[Max Reitz]

On 2015-03-16 at 10:44, Paolo Bonzini wrote:
>
> On 16/03/2015 14:58, Max Reitz wrote:
>>>> diff --git a/block/nbd-client.c b/block/nbd-client.c
>>>> index be6803d..ab13607 100644
>>>> --- a/block/nbd-client.c
>>>> +++ b/block/nbd-client.c
>>>> @@ -315,6 +315,7 @@ int nbd_client_co_flush(BlockDriverState *bs)
>>>>        ssize_t ret;
>>>>          if (!(client->nbdflags & NBD_FLAG_SEND_FLUSH)) {
>>>> +        /* This mirrors the behavior of bdrv_co_flush() in block.c */
>>>>            return 0;
>>>>        }
>>>>    @@ -350,6 +351,7 @@ int nbd_client_co_discard(BlockDriverState *bs,
>>>> int64_t sector_num,
>>>>        ssize_t ret;
>>>>          if (!(client->nbdflags & NBD_FLAG_SEND_TRIM)) {
>>>> +        /* This mirrors the behavior of bdrv_co_discard() in block.c */
>>>>            return 0;
>>> Should this return -EOPNOTSUPP instead?
>> That's what this patch is for. I asked myself the same thing, and it
>> turns out, the functions deliberately return 0 because bdrv_co_flush()
>> and bdrv_co_discard() do the same if the block driver does not support
>> these functions at all, so that's why I'm adding these comments.
> Right, but a better model than block.c should be for example
> block/raw-posix.c, which returns ENOTSUP (I checked now...).

Maybe we should catch ENOTSUP in bdrv_co_discard() and override it?

Max

Re: [Qemu-devel] [PATCH 20/25] block/nbd: Comment on discard/flush silently failing

[Paolo Bonzini]

On 16/03/2015 15:49, Max Reitz wrote:
>>>
>> Right, but a better model than block.c should be for example
>> block/raw-posix.c, which returns ENOTSUP (I checked now...).
> 
> Maybe we should catch ENOTSUP in bdrv_co_discard() and override it?

That's what it does:

        if (ret && ret != -ENOTSUP) {
            return ret;
        }

Returning 0 for NBD's flush_to_disk, by the way, is okay.

Paolo

Re: [Qemu-devel] [PATCH 20/25] block/nbd: Comment on discard/flush silently failing

[Max Reitz]

On 2015-03-16 at 10:51, Paolo Bonzini wrote:
>
> On 16/03/2015 15:49, Max Reitz wrote:
>>> Right, but a better model than block.c should be for example
>>> block/raw-posix.c, which returns ENOTSUP (I checked now...).
>> Maybe we should catch ENOTSUP in bdrv_co_discard() and override it?
> That's what it does:
>
>          if (ret && ret != -ENOTSUP) {
>              return ret;
>          }

Oops, thanks for telling me. Good, then I'll do that, thanks.

Max

> Returning 0 for NBD's flush_to_disk, by the way, is okay.
>
> Paolo

[Qemu-devel] [PATCH 21/25] nbd: Drop unexpected data for NBD_OPT_LIST

[Max Reitz]

When requesting the list of exports, no data should be sent. If data is
sent, the NBD server should not just inform the client of the invalid
request, but also drop the data.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 nbd.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/nbd.c b/nbd.c
index aa0925a..72e8243 100644
--- a/nbd.c
+++ b/nbd.c
@@ -199,6 +199,26 @@ static ssize_t read_sync(int fd, void *buffer, size_t size)
     return nbd_wr_sync(fd, buffer, size, true);
 }
 
+static ssize_t drop_sync(int fd, size_t size)
+{
+    ssize_t ret, dropped = size;
+    uint8_t *buffer = g_malloc(MIN(65536, size));
+
+    while (size > 0) {
+        ret = read_sync(fd, buffer, MIN(65536, size));
+        if (ret < 0) {
+            g_free(buffer);
+            return ret;
+        }
+
+        assert(ret <= size);
+        size -= ret;
+    }
+
+    g_free(buffer);
+    return dropped;
+}
+
 static ssize_t write_sync(int fd, void *buffer, size_t size)
 {
     int ret;
@@ -309,6 +329,9 @@ static int nbd_handle_list(NBDClient *client, uint32_t length)
 
     csock = client->sock;
     if (length) {
+        if (drop_sync(csock, length) != length) {
+            return -EIO;
+        }
         return nbd_send_rep(csock, NBD_REP_ERR_INVALID, NBD_OPT_LIST);
     }
 
-- 
2.1.0

[Qemu-devel] [PATCH 22/25] iotests: Add _timeout function

[Max Reitz]

Add a function for executing a command with a timeout.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/common.rc | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 22d3514..bd8453a 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -489,5 +489,21 @@ _die()
         exit 1
 }
 
+_timeout()
+{
+    local timeout=1
+    local pid
+
+    if [ "$1" = "-t" ]; then
+        shift
+        timeout=$1
+        shift
+    fi
+    "$@" &
+    pid=$!
+    (sleep $timeout; kill -9 $pid &> /dev/null) &
+    wait -n $pid $!
+}
+
 # make sure this script returns success
 true
-- 
2.1.0

[Qemu-devel] [PATCH 23/25] iotests: Add test for invalid qemu-nbd parameters

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/125     | 69 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/125.out | 21 ++++++++++++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 91 insertions(+)
 create mode 100755 tests/qemu-iotests/125
 create mode 100644 tests/qemu-iotests/125.out

diff --git a/tests/qemu-iotests/125 b/tests/qemu-iotests/125
new file mode 100755
index 0000000..9fbfb82
--- /dev/null
+++ b/tests/qemu-iotests/125
@@ -0,0 +1,69 @@
+#!/bin/bash
+#
+# Test case for (qemu-)nbd's parameter verification
+#
+# Copyright (C) 2015 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=mreitz@redhat.com
+
+seq="$(basename $0)"
+echo "QA output created by $seq"
+
+here="$PWD"
+tmp=/tmp/$$
+status=1	# failure is the default!
+
+_cleanup()
+{
+    _cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt generic
+_supported_proto file
+_supported_os Linux
+
+_make_test_img 1M
+_timeout $QEMU_NBD -f $IMGFMT -p $((0x100000000 + 10809)) "$TEST_IMG"
+_timeout $QEMU_NBD -f $IMGFMT -p 0x10000000000000000 "$TEST_IMG"
+_timeout $QEMU_NBD -f $IMGFMT -p -10809 "$TEST_IMG"
+_timeout $QEMU_NBD -f $IMGFMT -o 0x10000000000000000 "$TEST_IMG"
+_timeout $QEMU_NBD -f $IMGFMT -o 42 "$TEST_IMG"
+_timeout $QEMU_NBD -f $IMGFMT -o -512 "$TEST_IMG"
+_timeout $QEMU_NBD -f $IMGFMT -P 0x100000001 "$TEST_IMG"
+_timeout $QEMU_NBD -f $IMGFMT -P 0x10000000000000000 "$TEST_IMG"
+_timeout $QEMU_NBD -f $IMGFMT -P 1 -o 512 "$TEST_IMG"
+_timeout $QEMU_NBD -f $IMGFMT -P -1 "$TEST_IMG"
+_timeout $QEMU_NBD -f $IMGFMT -e 0x80000000 "$TEST_IMG"
+_timeout $QEMU_NBD -f $IMGFMT -e 0xffffffff "$TEST_IMG"
+_timeout $QEMU_NBD -f $IMGFMT -e 0x100000001 "$TEST_IMG"
+_timeout $QEMU_NBD -f $IMGFMT -e 0x10000000000000000 "$TEST_IMG"
+_timeout $QEMU_NBD -f $IMGFMT -e -1 "$TEST_IMG"
+
+$QEMU_IMG info "nbd://localhost:$((0x100000000 + 10809))"
+$QEMU_IMG info "nbd://localhost:-10809"
+
+# success, all done
+echo
+echo '*** done'
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/125.out b/tests/qemu-iotests/125.out
new file mode 100644
index 0000000..48d6f8b
--- /dev/null
+++ b/tests/qemu-iotests/125.out
@@ -0,0 +1,21 @@
+QA output created by 125
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
+qemu-nbd: Port out of range `4294978105'
+qemu-nbd: Port out of range `0x10000000000000000'
+qemu-nbd: Port out of range `-10809'
+qemu-nbd: Invalid offset `0x10000000000000000': Numerical result out of range
+qemu-nbd: Offset must be a multiple of 512
+qemu-nbd: Offset must be positive `-512'
+qemu-nbd: Invalid partition 0x100000001
+qemu-nbd: Invalid partition 0x10000000000000000
+qemu-nbd: Cannot use both -o and -P at the same time
+qemu-nbd: Invalid partition -1
+qemu-nbd: Shared device number must be less than 2147483647
+qemu-nbd: Shared device number must be less than 2147483647
+qemu-nbd: Shared device number must be less than 2147483647
+qemu-nbd: Shared device number must be less than 2147483647
+qemu-nbd: Shared device number must be greater than 0
+qemu-img: Could not open 'nbd://localhost:4294978105': No valid URL specified
+qemu-img: Could not open 'nbd://localhost:-10809': No valid URL specified
+
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 7436300..9410b3a 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -124,3 +124,4 @@
 117 rw auto
 118 rw auto
 123 rw auto quick
+125 rw auto quick
-- 
2.1.0

[Qemu-devel] [PATCH 24/25] iotests: Add test for issuing discard over NBD

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/126     | 105 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/126.out |  28 ++++++++++++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 134 insertions(+)
 create mode 100755 tests/qemu-iotests/126
 create mode 100644 tests/qemu-iotests/126.out

diff --git a/tests/qemu-iotests/126 b/tests/qemu-iotests/126
new file mode 100755
index 0000000..efbfba2
--- /dev/null
+++ b/tests/qemu-iotests/126
@@ -0,0 +1,105 @@
+#!/bin/bash
+#
+# Test case for discard over NBD
+#
+# Copyright (C) 2015 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=mreitz@redhat.com
+
+seq="$(basename $0)"
+echo "QA output created by $seq"
+
+here="$PWD"
+tmp=/tmp/$$
+status=1	# failure is the default!
+
+_cleanup()
+{
+    _cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+. ./common.qemu
+
+# must support discard (and discarding must result in reading zeros)
+_supported_fmt raw qcow2
+_supported_proto file
+_supported_os Linux
+
+echo
+echo '=== Testing anonymous export ==='
+echo
+
+_make_test_img 1M
+$QEMU_IO -c 'write -P 42 0 1M' "$TEST_IMG" | _filter_qemu_io
+
+$QEMU_NBD -f $IMGFMT --discard unmap "$TEST_IMG" &
+nbd_pid=$!
+
+# Wait for the NBD server to come up
+sleep 1
+
+$QEMU_IO_PROG -f raw -c 'discard 0 1M' 'nbd://localhost' | _filter_qemu_io
+wait $nbd_pid
+
+$QEMU_IO -c 'read -P 0 0 1M' "$TEST_IMG" | _filter_qemu_io
+
+echo
+echo '=== Testing named export ==='
+echo
+
+_make_test_img 1M
+$QEMU_IO -c 'write -P 42 0 1M' "$TEST_IMG" | _filter_qemu_io
+
+_launch_qemu -drive if=none,id=drv,file="$TEST_IMG",format=$IMGFMT,discard=unmap \
+    2> >(_filter_nbd)
+
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'qmp_capabilities' }" \
+       'return'
+
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'nbd-server-start',
+       'arguments': { 'addr': { 'type': 'inet',
+                                'data': { 'host': '127.0.0.1',
+                                          'port': '10809' }}}}" \
+    'return'
+
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'nbd-server-add',
+       'arguments': { 'device': 'drv' }}" \
+    'return'
+
+$QEMU_IO_PROG -f raw -c 'discard 0 1M' 'nbd://localhost/drv' | _filter_qemu_io
+
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'quit' }" \
+    'return'
+
+wait=1 _cleanup_qemu
+
+$QEMU_IO -c 'read -P 0 0 1M' "$TEST_IMG" | _filter_qemu_io
+
+# success, all done
+echo
+echo '*** done'
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/126.out b/tests/qemu-iotests/126.out
new file mode 100644
index 0000000..1f05f6f
--- /dev/null
+++ b/tests/qemu-iotests/126.out
@@ -0,0 +1,28 @@
+QA output created by 126
+
+=== Testing anonymous export ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
+wrote 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+discard 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+=== Testing named export ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
+wrote 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+{"return": {}}
+{"return": {}}
+{"return": {}}
+discard 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN"}
+read 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 9410b3a..f45a4ec 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -125,3 +125,4 @@
 118 rw auto
 123 rw auto quick
 125 rw auto quick
+126 rw auto quick
-- 
2.1.0

[Qemu-devel] [PATCH 25/25] iotests: Add test for a non-existing NBD export

[Max Reitz]

The error message should be somehow helpful.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/127     | 80 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/127.out | 14 ++++++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 95 insertions(+)
 create mode 100755 tests/qemu-iotests/127
 create mode 100644 tests/qemu-iotests/127.out

diff --git a/tests/qemu-iotests/127 b/tests/qemu-iotests/127
new file mode 100755
index 0000000..8ab86df
--- /dev/null
+++ b/tests/qemu-iotests/127
@@ -0,0 +1,80 @@
+#!/bin/bash
+#
+# Test case for opening a non-existing export
+#
+# Copyright (C) 2015 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=mreitz@redhat.com
+
+seq="$(basename $0)"
+echo "QA output created by $seq"
+
+here="$PWD"
+tmp=/tmp/$$
+status=1	# failure is the default!
+
+_cleanup()
+{
+    _cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+. ./common.qemu
+
+_supported_fmt generic
+_supported_proto file
+_supported_os Linux
+
+_make_test_img 1M
+
+_launch_qemu -drive if=none,id=drv,file="$TEST_IMG",format=$IMGFMT \
+    2> >(_filter_nbd)
+
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'qmp_capabilities' }" \
+    'return'
+
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'nbd-server-start',
+       'arguments': { 'addr': { 'type': 'inet',
+                                'data': { 'host': '127.0.0.1',
+                                          'port': '10809' }}}}" \
+    'return'
+
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'nbd-server-add',
+       'arguments': { 'device': 'drv' }}" \
+    'return'
+
+$QEMU_IMG info 'nbd://localhost/foo'
+$QEMU_IMG info 'nbd://localhost/drv'
+
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'quit' }" \
+    'return'
+
+wait=1 _cleanup_qemu
+
+# success, all done
+echo
+echo '*** done'
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/127.out b/tests/qemu-iotests/127.out
new file mode 100644
index 0000000..0b4dbfa
--- /dev/null
+++ b/tests/qemu-iotests/127.out
@@ -0,0 +1,14 @@
+QA output created by 127
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
+{"return": {}}
+{"return": {}}
+{"return": {}}
+qemu-img: Could not open 'nbd://localhost/foo': Failed to open export
+image: nbd://localhost/drv
+file format: raw
+virtual size: 1.0M (1048576 bytes)
+disk size: unavailable
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN"}
+
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index f45a4ec..e7c410a 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -126,3 +126,4 @@
 123 rw auto quick
 125 rw auto quick
 126 rw auto quick
+127 rw auto quick
-- 
2.1.0

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

[Max Reitz]

On 2015-02-25 at 13: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.

And of course I forgot to add:

This series depends on v3 (or any later version) of my series "block: 
Rework bdrv_close_all()".

Max

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

[Paolo Bonzini]

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
>