* [Qemu-devel] [PATCH 00/25] nbd: Several fixes
@ 2015-02-25 18:08 Max Reitz
2015-02-25 18:08 ` [Qemu-devel] [PATCH 01/25] util/uri: Add overflow check to rfc3986_parse_port Max Reitz
` (26 more replies)
0 siblings, 27 replies; 46+ messages in thread
From: Max Reitz @ 2015-02-25 18:08 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Paolo Bonzini, qemu-devel, Stefan Hajnoczi, 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
^ permalink raw reply [flat|nested] 46+ messages in thread
* [Qemu-devel] [PATCH 01/25] util/uri: Add overflow check to rfc3986_parse_port
2015-02-25 18:08 [Qemu-devel] [PATCH 00/25] nbd: Several fixes Max Reitz
@ 2015-02-25 18:08 ` Max Reitz
2015-02-25 18:08 ` [Qemu-devel] [PATCH 02/25] qemu-nbd: Detect unused partitions by system == 0 Max Reitz
` (25 subsequent siblings)
26 siblings, 0 replies; 46+ messages in thread
From: Max Reitz @ 2015-02-25 18:08 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Paolo Bonzini, qemu-devel, Stefan Hajnoczi, 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
^ permalink raw reply related [flat|nested] 46+ messages in thread
* [Qemu-devel] [PATCH 02/25] qemu-nbd: Detect unused partitions by system == 0
2015-02-25 18:08 [Qemu-devel] [PATCH 00/25] nbd: Several fixes Max Reitz
2015-02-25 18:08 ` [Qemu-devel] [PATCH 01/25] util/uri: Add overflow check to rfc3986_parse_port Max Reitz
@ 2015-02-25 18:08 ` Max Reitz
2015-02-25 18:08 ` [Qemu-devel] [PATCH 03/25] nbd: Fix nbd_establish_connection()'s return value Max Reitz
` (24 subsequent siblings)
26 siblings, 0 replies; 46+ messages in thread
From: Max Reitz @ 2015-02-25 18:08 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Paolo Bonzini, qemu-devel, Stefan Hajnoczi, 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
^ permalink raw reply related [flat|nested] 46+ messages in thread
* [Qemu-devel] [PATCH 03/25] nbd: Fix nbd_establish_connection()'s return value
2015-02-25 18:08 [Qemu-devel] [PATCH 00/25] nbd: Several fixes Max Reitz
2015-02-25 18:08 ` [Qemu-devel] [PATCH 01/25] util/uri: Add overflow check to rfc3986_parse_port Max Reitz
2015-02-25 18:08 ` [Qemu-devel] [PATCH 02/25] qemu-nbd: Detect unused partitions by system == 0 Max Reitz
@ 2015-02-25 18:08 ` Max Reitz
2015-02-25 18:08 ` [Qemu-devel] [PATCH 04/25] nbd: Fix response to invalid requests Max Reitz
` (23 subsequent siblings)
26 siblings, 0 replies; 46+ messages in thread
From: Max Reitz @ 2015-02-25 18:08 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Paolo Bonzini, qemu-devel, Stefan Hajnoczi, 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
^ permalink raw reply related [flat|nested] 46+ messages in thread
* [Qemu-devel] [PATCH 04/25] nbd: Fix response to invalid requests
2015-02-25 18:08 [Qemu-devel] [PATCH 00/25] nbd: Several fixes Max Reitz
` (2 preceding siblings ...)
2015-02-25 18:08 ` [Qemu-devel] [PATCH 03/25] nbd: Fix nbd_establish_connection()'s return value Max Reitz
@ 2015-02-25 18:08 ` Max Reitz
2015-03-02 16:52 ` Max Reitz
2015-02-25 18:08 ` [Qemu-devel] [PATCH 05/25] nbd: Avoid generic -EINVAL Max Reitz
` (22 subsequent siblings)
26 siblings, 1 reply; 46+ messages in thread
From: Max Reitz @ 2015-02-25 18:08 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Paolo Bonzini, qemu-devel, Stefan Hajnoczi, 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
^ permalink raw reply related [flat|nested] 46+ messages in thread
* [Qemu-devel] [PATCH 05/25] nbd: Avoid generic -EINVAL
2015-02-25 18:08 [Qemu-devel] [PATCH 00/25] nbd: Several fixes Max Reitz
` (3 preceding siblings ...)
2015-02-25 18:08 ` [Qemu-devel] [PATCH 04/25] nbd: Fix response to invalid requests Max Reitz
@ 2015-02-25 18:08 ` Max Reitz
2015-03-11 11:22 ` Paolo Bonzini
2015-02-25 18:08 ` [Qemu-devel] [PATCH 06/25] nbd: Pass return value from nbd_handle_list() Max Reitz
` (21 subsequent siblings)
26 siblings, 1 reply; 46+ messages in thread
From: Max Reitz @ 2015-02-25 18:08 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Paolo Bonzini, qemu-devel, Stefan Hajnoczi, 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
^ permalink raw reply related [flat|nested] 46+ messages in thread
* [Qemu-devel] [PATCH 06/25] nbd: Pass return value from nbd_handle_list()
2015-02-25 18:08 [Qemu-devel] [PATCH 00/25] nbd: Several fixes Max Reitz
` (4 preceding siblings ...)
2015-02-25 18:08 ` [Qemu-devel] [PATCH 05/25] nbd: Avoid generic -EINVAL Max Reitz
@ 2015-02-25 18:08 ` Max Reitz
2015-02-25 18:08 ` [Qemu-devel] [PATCH 07/25] nbd: Add "failed to open export" error message Max Reitz
` (20 subsequent siblings)
26 siblings, 0 replies; 46+ messages in thread
From: Max Reitz @ 2015-02-25 18:08 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Paolo Bonzini, qemu-devel, Stefan Hajnoczi, 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
^ permalink raw reply related [flat|nested] 46+ messages in thread
* [Qemu-devel] [PATCH 07/25] nbd: Add "failed to open export" error message
2015-02-25 18:08 [Qemu-devel] [PATCH 00/25] nbd: Several fixes Max Reitz
` (5 preceding siblings ...)
2015-02-25 18:08 ` [Qemu-devel] [PATCH 06/25] nbd: Pass return value from nbd_handle_list() Max Reitz
@ 2015-02-25 18:08 ` Max Reitz
2015-03-11 11:24 ` Paolo Bonzini
2015-02-25 18:08 ` [Qemu-devel] [PATCH 08/25] nbd: Handle blk_getlength() failure Max Reitz
` (19 subsequent siblings)
26 siblings, 1 reply; 46+ messages in thread
From: Max Reitz @ 2015-02-25 18:08 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Paolo Bonzini, qemu-devel, Stefan Hajnoczi, 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
^ permalink raw reply related [flat|nested] 46+ messages in thread
* [Qemu-devel] [PATCH 08/25] nbd: Handle blk_getlength() failure
2015-02-25 18:08 [Qemu-devel] [PATCH 00/25] nbd: Several fixes Max Reitz
` (6 preceding siblings ...)
2015-02-25 18:08 ` [Qemu-devel] [PATCH 07/25] nbd: Add "failed to open export" error message Max Reitz
@ 2015-02-25 18:08 ` Max Reitz
2015-03-11 11:26 ` Paolo Bonzini
2015-02-25 18:08 ` [Qemu-devel] [PATCH 09/25] qemu-nbd: fork() can fail Max Reitz
` (18 subsequent siblings)
26 siblings, 1 reply; 46+ messages in thread
From: Max Reitz @ 2015-02-25 18:08 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Paolo Bonzini, qemu-devel, Stefan Hajnoczi, 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
^ permalink raw reply related [flat|nested] 46+ messages in thread
* [Qemu-devel] [PATCH 09/25] qemu-nbd: fork() can fail
2015-02-25 18:08 [Qemu-devel] [PATCH 00/25] nbd: Several fixes Max Reitz
` (7 preceding siblings ...)
2015-02-25 18:08 ` [Qemu-devel] [PATCH 08/25] nbd: Handle blk_getlength() failure Max Reitz
@ 2015-02-25 18:08 ` Max Reitz
2015-02-25 18:08 ` [Qemu-devel] [PATCH 10/25] nbd: Fix potential signed overflow issues Max Reitz
` (17 subsequent siblings)
26 siblings, 0 replies; 46+ messages in thread
From: Max Reitz @ 2015-02-25 18:08 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Paolo Bonzini, qemu-devel, Stefan Hajnoczi, 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
^ permalink raw reply related [flat|nested] 46+ messages in thread
* [Qemu-devel] [PATCH 10/25] nbd: Fix potential signed overflow issues
2015-02-25 18:08 [Qemu-devel] [PATCH 00/25] nbd: Several fixes Max Reitz
` (8 preceding siblings ...)
2015-02-25 18:08 ` [Qemu-devel] [PATCH 09/25] qemu-nbd: fork() can fail Max Reitz
@ 2015-02-25 18:08 ` Max Reitz
2015-03-11 11:28 ` Paolo Bonzini
2015-02-25 18:08 ` [Qemu-devel] [PATCH 11/25] qemu-nbd: Fix and improve input verification Max Reitz
` (16 subsequent siblings)
26 siblings, 1 reply; 46+ messages in thread
From: Max Reitz @ 2015-02-25 18:08 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Paolo Bonzini, qemu-devel, Stefan Hajnoczi, 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
^ permalink raw reply related [flat|nested] 46+ messages in thread
* [Qemu-devel] [PATCH 11/25] qemu-nbd: Fix and improve input verification
2015-02-25 18:08 [Qemu-devel] [PATCH 00/25] nbd: Several fixes Max Reitz
` (9 preceding siblings ...)
2015-02-25 18:08 ` [Qemu-devel] [PATCH 10/25] nbd: Fix potential signed overflow issues Max Reitz
@ 2015-02-25 18:08 ` Max Reitz
2015-03-11 11:30 ` Paolo Bonzini
2015-02-25 18:08 ` [Qemu-devel] [PATCH 12/25] nbd: Set block size to BDRV_SECTOR_SIZE Max Reitz
` (15 subsequent siblings)
26 siblings, 1 reply; 46+ messages in thread
From: Max Reitz @ 2015-02-25 18:08 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Paolo Bonzini, qemu-devel, Stefan Hajnoczi, 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
^ permalink raw reply related [flat|nested] 46+ messages in thread
* [Qemu-devel] [PATCH 12/25] nbd: Set block size to BDRV_SECTOR_SIZE
2015-02-25 18:08 [Qemu-devel] [PATCH 00/25] nbd: Several fixes Max Reitz
` (10 preceding siblings ...)
2015-02-25 18:08 ` [Qemu-devel] [PATCH 11/25] qemu-nbd: Fix and improve input verification Max Reitz
@ 2015-02-25 18:08 ` Max Reitz
2015-02-25 18:08 ` [Qemu-devel] [PATCH 13/25] nbd: Enforce sector alignment Max Reitz
` (14 subsequent siblings)
26 siblings, 0 replies; 46+ messages in thread
From: Max Reitz @ 2015-02-25 18:08 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Paolo Bonzini, qemu-devel, Stefan Hajnoczi, 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
^ permalink raw reply related [flat|nested] 46+ messages in thread
* [Qemu-devel] [PATCH 13/25] nbd: Enforce sector alignment
2015-02-25 18:08 [Qemu-devel] [PATCH 00/25] nbd: Several fixes Max Reitz
` (11 preceding siblings ...)
2015-02-25 18:08 ` [Qemu-devel] [PATCH 12/25] nbd: Set block size to BDRV_SECTOR_SIZE Max Reitz
@ 2015-02-25 18:08 ` Max Reitz
2015-02-25 18:08 ` [Qemu-devel] [PATCH 14/25] coroutine: Add co_yield_timeout() Max Reitz
` (13 subsequent siblings)
26 siblings, 0 replies; 46+ messages in thread
From: Max Reitz @ 2015-02-25 18:08 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Paolo Bonzini, qemu-devel, Stefan Hajnoczi, 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
^ permalink raw reply related [flat|nested] 46+ messages in thread
* [Qemu-devel] [PATCH 14/25] coroutine: Add co_yield_timeout()
2015-02-25 18:08 [Qemu-devel] [PATCH 00/25] nbd: Several fixes Max Reitz
` (12 preceding siblings ...)
2015-02-25 18:08 ` [Qemu-devel] [PATCH 13/25] nbd: Enforce sector alignment Max Reitz
@ 2015-02-25 18:08 ` Max Reitz
2015-02-25 18:08 ` [Qemu-devel] [PATCH 15/25] coroutine-io: Return -errno in case of error Max Reitz
` (12 subsequent siblings)
26 siblings, 0 replies; 46+ messages in thread
From: Max Reitz @ 2015-02-25 18:08 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Paolo Bonzini, qemu-devel, Stefan Hajnoczi, 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
^ permalink raw reply related [flat|nested] 46+ messages in thread
* [Qemu-devel] [PATCH 15/25] coroutine-io: Return -errno in case of error
2015-02-25 18:08 [Qemu-devel] [PATCH 00/25] nbd: Several fixes Max Reitz
` (13 preceding siblings ...)
2015-02-25 18:08 ` [Qemu-devel] [PATCH 14/25] coroutine: Add co_yield_timeout() Max Reitz
@ 2015-02-25 18:08 ` Max Reitz
2015-02-25 18:08 ` [Qemu-devel] [PATCH 16/25] coroutine-io: Add I/O functions with timeout Max Reitz
` (11 subsequent siblings)
26 siblings, 0 replies; 46+ messages in thread
From: Max Reitz @ 2015-02-25 18:08 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Paolo Bonzini, qemu-devel, Stefan Hajnoczi, 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
^ permalink raw reply related [flat|nested] 46+ messages in thread
* [Qemu-devel] [PATCH 16/25] coroutine-io: Add I/O functions with timeout
2015-02-25 18:08 [Qemu-devel] [PATCH 00/25] nbd: Several fixes Max Reitz
` (14 preceding siblings ...)
2015-02-25 18:08 ` [Qemu-devel] [PATCH 15/25] coroutine-io: Return -errno in case of error Max Reitz
@ 2015-02-25 18:08 ` Max Reitz
2015-02-25 18:08 ` [Qemu-devel] [PATCH 17/25] nbd: Employ timeouts Max Reitz
` (10 subsequent siblings)
26 siblings, 0 replies; 46+ messages in thread
From: Max Reitz @ 2015-02-25 18:08 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Paolo Bonzini, qemu-devel, Stefan Hajnoczi, 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
^ permalink raw reply related [flat|nested] 46+ messages in thread
* [Qemu-devel] [PATCH 17/25] nbd: Employ timeouts
2015-02-25 18:08 [Qemu-devel] [PATCH 00/25] nbd: Several fixes Max Reitz
` (15 preceding siblings ...)
2015-02-25 18:08 ` [Qemu-devel] [PATCH 16/25] coroutine-io: Add I/O functions with timeout Max Reitz
@ 2015-02-25 18:08 ` Max Reitz
2015-02-25 18:08 ` [Qemu-devel] [PATCH 18/25] nbd: Fix nbd_receive_options() Max Reitz
` (9 subsequent siblings)
26 siblings, 0 replies; 46+ messages in thread
From: Max Reitz @ 2015-02-25 18:08 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Paolo Bonzini, qemu-devel, Stefan Hajnoczi, 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
^ permalink raw reply related [flat|nested] 46+ messages in thread
* [Qemu-devel] [PATCH 18/25] nbd: Fix nbd_receive_options()
2015-02-25 18:08 [Qemu-devel] [PATCH 00/25] nbd: Several fixes Max Reitz
` (16 preceding siblings ...)
2015-02-25 18:08 ` [Qemu-devel] [PATCH 17/25] nbd: Employ timeouts Max Reitz
@ 2015-02-25 18:08 ` Max Reitz
2015-02-25 18:08 ` [Qemu-devel] [PATCH 19/25] nbd: Fix interpretation of the export flags Max Reitz
` (8 subsequent siblings)
26 siblings, 0 replies; 46+ messages in thread
From: Max Reitz @ 2015-02-25 18:08 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Paolo Bonzini, qemu-devel, Stefan Hajnoczi, 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
^ permalink raw reply related [flat|nested] 46+ messages in thread
* [Qemu-devel] [PATCH 19/25] nbd: Fix interpretation of the export flags
2015-02-25 18:08 [Qemu-devel] [PATCH 00/25] nbd: Several fixes Max Reitz
` (17 preceding siblings ...)
2015-02-25 18:08 ` [Qemu-devel] [PATCH 18/25] nbd: Fix nbd_receive_options() Max Reitz
@ 2015-02-25 18:08 ` Max Reitz
2015-02-25 18:08 ` [Qemu-devel] [PATCH 20/25] block/nbd: Comment on discard/flush silently failing Max Reitz
` (7 subsequent siblings)
26 siblings, 0 replies; 46+ messages in thread
From: Max Reitz @ 2015-02-25 18:08 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Paolo Bonzini, qemu-devel, Stefan Hajnoczi, 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
^ permalink raw reply related [flat|nested] 46+ messages in thread
* [Qemu-devel] [PATCH 20/25] block/nbd: Comment on discard/flush silently failing
2015-02-25 18:08 [Qemu-devel] [PATCH 00/25] nbd: Several fixes Max Reitz
` (18 preceding siblings ...)
2015-02-25 18:08 ` [Qemu-devel] [PATCH 19/25] nbd: Fix interpretation of the export flags Max Reitz
@ 2015-02-25 18:08 ` Max Reitz
2015-03-11 11:31 ` Paolo Bonzini
2015-02-25 18:08 ` [Qemu-devel] [PATCH 21/25] nbd: Drop unexpected data for NBD_OPT_LIST Max Reitz
` (6 subsequent siblings)
26 siblings, 1 reply; 46+ messages in thread
From: Max Reitz @ 2015-02-25 18:08 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Paolo Bonzini, qemu-devel, Stefan Hajnoczi, 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
^ permalink raw reply related [flat|nested] 46+ messages in thread
* [Qemu-devel] [PATCH 21/25] nbd: Drop unexpected data for NBD_OPT_LIST
2015-02-25 18:08 [Qemu-devel] [PATCH 00/25] nbd: Several fixes Max Reitz
` (19 preceding siblings ...)
2015-02-25 18:08 ` [Qemu-devel] [PATCH 20/25] block/nbd: Comment on discard/flush silently failing Max Reitz
@ 2015-02-25 18:08 ` Max Reitz
2015-02-25 18:08 ` [Qemu-devel] [PATCH 22/25] iotests: Add _timeout function Max Reitz
` (5 subsequent siblings)
26 siblings, 0 replies; 46+ messages in thread
From: Max Reitz @ 2015-02-25 18:08 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Paolo Bonzini, qemu-devel, Stefan Hajnoczi, 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
^ permalink raw reply related [flat|nested] 46+ messages in thread
* [Qemu-devel] [PATCH 22/25] iotests: Add _timeout function
2015-02-25 18:08 [Qemu-devel] [PATCH 00/25] nbd: Several fixes Max Reitz
` (20 preceding siblings ...)
2015-02-25 18:08 ` [Qemu-devel] [PATCH 21/25] nbd: Drop unexpected data for NBD_OPT_LIST Max Reitz
@ 2015-02-25 18:08 ` Max Reitz
2015-02-25 18:08 ` [Qemu-devel] [PATCH 23/25] iotests: Add test for invalid qemu-nbd parameters Max Reitz
` (4 subsequent siblings)
26 siblings, 0 replies; 46+ messages in thread
From: Max Reitz @ 2015-02-25 18:08 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Paolo Bonzini, qemu-devel, Stefan Hajnoczi, 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
^ permalink raw reply related [flat|nested] 46+ messages in thread
* [Qemu-devel] [PATCH 23/25] iotests: Add test for invalid qemu-nbd parameters
2015-02-25 18:08 [Qemu-devel] [PATCH 00/25] nbd: Several fixes Max Reitz
` (21 preceding siblings ...)
2015-02-25 18:08 ` [Qemu-devel] [PATCH 22/25] iotests: Add _timeout function Max Reitz
@ 2015-02-25 18:08 ` Max Reitz
2015-02-25 18:08 ` [Qemu-devel] [PATCH 24/25] iotests: Add test for issuing discard over NBD Max Reitz
` (3 subsequent siblings)
26 siblings, 0 replies; 46+ messages in thread
From: Max Reitz @ 2015-02-25 18:08 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Paolo Bonzini, qemu-devel, Stefan Hajnoczi, 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
^ permalink raw reply related [flat|nested] 46+ messages in thread
* [Qemu-devel] [PATCH 24/25] iotests: Add test for issuing discard over NBD
2015-02-25 18:08 [Qemu-devel] [PATCH 00/25] nbd: Several fixes Max Reitz
` (22 preceding siblings ...)
2015-02-25 18:08 ` [Qemu-devel] [PATCH 23/25] iotests: Add test for invalid qemu-nbd parameters Max Reitz
@ 2015-02-25 18:08 ` Max Reitz
2015-02-25 18:08 ` [Qemu-devel] [PATCH 25/25] iotests: Add test for a non-existing NBD export Max Reitz
` (2 subsequent siblings)
26 siblings, 0 replies; 46+ messages in thread
From: Max Reitz @ 2015-02-25 18:08 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Paolo Bonzini, qemu-devel, Stefan Hajnoczi, 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
^ permalink raw reply related [flat|nested] 46+ messages in thread
* [Qemu-devel] [PATCH 25/25] iotests: Add test for a non-existing NBD export
2015-02-25 18:08 [Qemu-devel] [PATCH 00/25] nbd: Several fixes Max Reitz
` (23 preceding siblings ...)
2015-02-25 18:08 ` [Qemu-devel] [PATCH 24/25] iotests: Add test for issuing discard over NBD Max Reitz
@ 2015-02-25 18:08 ` Max Reitz
2015-02-25 18:11 ` [Qemu-devel] [PATCH 00/25] nbd: Several fixes Max Reitz
2015-03-11 11:36 ` Paolo Bonzini
26 siblings, 0 replies; 46+ messages in thread
From: Max Reitz @ 2015-02-25 18:08 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Paolo Bonzini, qemu-devel, Stefan Hajnoczi, 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
^ permalink raw reply related [flat|nested] 46+ messages in thread
* Re: [Qemu-devel] [PATCH 00/25] nbd: Several fixes
2015-02-25 18:08 [Qemu-devel] [PATCH 00/25] nbd: Several fixes Max Reitz
` (24 preceding siblings ...)
2015-02-25 18:08 ` [Qemu-devel] [PATCH 25/25] iotests: Add test for a non-existing NBD export Max Reitz
@ 2015-02-25 18:11 ` Max Reitz
2015-03-11 11:36 ` Paolo Bonzini
26 siblings, 0 replies; 46+ messages in thread
From: Max Reitz @ 2015-02-25 18:11 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, Paolo Bonzini, qemu-devel, Stefan Hajnoczi
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
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [Qemu-devel] [PATCH 04/25] nbd: Fix response to invalid requests
2015-02-25 18:08 ` [Qemu-devel] [PATCH 04/25] nbd: Fix response to invalid requests Max Reitz
@ 2015-03-02 16:52 ` Max Reitz
0 siblings, 0 replies; 46+ messages in thread
From: Max Reitz @ 2015-03-02 16:52 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, Paolo Bonzini, qemu-devel, Stefan Hajnoczi
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
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [Qemu-devel] [PATCH 05/25] nbd: Avoid generic -EINVAL
2015-02-25 18:08 ` [Qemu-devel] [PATCH 05/25] nbd: Avoid generic -EINVAL Max Reitz
@ 2015-03-11 11:22 ` Paolo Bonzini
2015-03-16 13:51 ` Max Reitz
0 siblings, 1 reply; 46+ messages in thread
From: Paolo Bonzini @ 2015-03-11 11:22 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
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;
> }
>
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [Qemu-devel] [PATCH 07/25] nbd: Add "failed to open export" error message
2015-02-25 18:08 ` [Qemu-devel] [PATCH 07/25] nbd: Add "failed to open export" error message Max Reitz
@ 2015-03-11 11:24 ` Paolo Bonzini
2015-03-16 13:55 ` Max Reitz
0 siblings, 1 reply; 46+ messages in thread
From: Paolo Bonzini @ 2015-03-11 11:24 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
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"}
>
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [Qemu-devel] [PATCH 08/25] nbd: Handle blk_getlength() failure
2015-02-25 18:08 ` [Qemu-devel] [PATCH 08/25] nbd: Handle blk_getlength() failure Max Reitz
@ 2015-03-11 11:26 ` Paolo Bonzini
0 siblings, 0 replies; 46+ messages in thread
From: Paolo Bonzini @ 2015-03-11 11:26 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
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);
>
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [Qemu-devel] [PATCH 10/25] nbd: Fix potential signed overflow issues
2015-02-25 18:08 ` [Qemu-devel] [PATCH 10/25] nbd: Fix potential signed overflow issues Max Reitz
@ 2015-03-11 11:28 ` Paolo Bonzini
0 siblings, 0 replies; 46+ messages in thread
From: Paolo Bonzini @ 2015-03-11 11:28 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
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,
>
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [Qemu-devel] [PATCH 11/25] qemu-nbd: Fix and improve input verification
2015-02-25 18:08 ` [Qemu-devel] [PATCH 11/25] qemu-nbd: Fix and improve input verification Max Reitz
@ 2015-03-11 11:30 ` Paolo Bonzini
2015-03-16 13:56 ` Max Reitz
0 siblings, 1 reply; 46+ messages in thread
From: Paolo Bonzini @ 2015-03-11 11:30 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
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);
> }
> }
>
>
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [Qemu-devel] [PATCH 20/25] block/nbd: Comment on discard/flush silently failing
2015-02-25 18:08 ` [Qemu-devel] [PATCH 20/25] block/nbd: Comment on discard/flush silently failing Max Reitz
@ 2015-03-11 11:31 ` Paolo Bonzini
2015-03-16 13:58 ` Max Reitz
0 siblings, 1 reply; 46+ messages in thread
From: Paolo Bonzini @ 2015-03-11 11:31 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
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;
>
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [Qemu-devel] [PATCH 00/25] nbd: Several fixes
2015-02-25 18:08 [Qemu-devel] [PATCH 00/25] nbd: Several fixes Max Reitz
` (25 preceding siblings ...)
2015-02-25 18:11 ` [Qemu-devel] [PATCH 00/25] nbd: Several fixes Max Reitz
@ 2015-03-11 11:36 ` Paolo Bonzini
26 siblings, 0 replies; 46+ messages in thread
From: Paolo Bonzini @ 2015-03-11 11:36 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
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
>
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [Qemu-devel] [PATCH 05/25] nbd: Avoid generic -EINVAL
2015-03-11 11:22 ` Paolo Bonzini
@ 2015-03-16 13:51 ` Max Reitz
2015-03-16 14:42 ` Paolo Bonzini
0 siblings, 1 reply; 46+ messages in thread
From: Max Reitz @ 2015-03-16 13:51 UTC (permalink / raw)
To: Paolo Bonzini, qemu-block; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
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;
>> }
>>
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [Qemu-devel] [PATCH 07/25] nbd: Add "failed to open export" error message
2015-03-11 11:24 ` Paolo Bonzini
@ 2015-03-16 13:55 ` Max Reitz
0 siblings, 0 replies; 46+ messages in thread
From: Max Reitz @ 2015-03-16 13:55 UTC (permalink / raw)
To: Paolo Bonzini, qemu-block; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
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
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [Qemu-devel] [PATCH 11/25] qemu-nbd: Fix and improve input verification
2015-03-11 11:30 ` Paolo Bonzini
@ 2015-03-16 13:56 ` Max Reitz
0 siblings, 0 replies; 46+ messages in thread
From: Max Reitz @ 2015-03-16 13:56 UTC (permalink / raw)
To: Paolo Bonzini, qemu-block; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
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);
>> }
>> }
>>
>>
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [Qemu-devel] [PATCH 20/25] block/nbd: Comment on discard/flush silently failing
2015-03-11 11:31 ` Paolo Bonzini
@ 2015-03-16 13:58 ` Max Reitz
2015-03-16 14:44 ` Paolo Bonzini
0 siblings, 1 reply; 46+ messages in thread
From: Max Reitz @ 2015-03-16 13:58 UTC (permalink / raw)
To: Paolo Bonzini, qemu-block; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
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;
>>
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [Qemu-devel] [PATCH 05/25] nbd: Avoid generic -EINVAL
2015-03-16 13:51 ` Max Reitz
@ 2015-03-16 14:42 ` Paolo Bonzini
2015-03-16 14:48 ` Max Reitz
0 siblings, 1 reply; 46+ messages in thread
From: Paolo Bonzini @ 2015-03-16 14:42 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
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
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [Qemu-devel] [PATCH 20/25] block/nbd: Comment on discard/flush silently failing
2015-03-16 13:58 ` Max Reitz
@ 2015-03-16 14:44 ` Paolo Bonzini
2015-03-16 14:49 ` Max Reitz
0 siblings, 1 reply; 46+ messages in thread
From: Paolo Bonzini @ 2015-03-16 14:44 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
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
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [Qemu-devel] [PATCH 05/25] nbd: Avoid generic -EINVAL
2015-03-16 14:42 ` Paolo Bonzini
@ 2015-03-16 14:48 ` Max Reitz
2015-03-16 14:49 ` Paolo Bonzini
0 siblings, 1 reply; 46+ messages in thread
From: Max Reitz @ 2015-03-16 14:48 UTC (permalink / raw)
To: Paolo Bonzini, qemu-block; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
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
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [Qemu-devel] [PATCH 20/25] block/nbd: Comment on discard/flush silently failing
2015-03-16 14:44 ` Paolo Bonzini
@ 2015-03-16 14:49 ` Max Reitz
2015-03-16 14:51 ` Paolo Bonzini
0 siblings, 1 reply; 46+ messages in thread
From: Max Reitz @ 2015-03-16 14:49 UTC (permalink / raw)
To: Paolo Bonzini, qemu-block; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
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
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [Qemu-devel] [PATCH 05/25] nbd: Avoid generic -EINVAL
2015-03-16 14:48 ` Max Reitz
@ 2015-03-16 14:49 ` Paolo Bonzini
0 siblings, 0 replies; 46+ messages in thread
From: Paolo Bonzini @ 2015-03-16 14:49 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
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
>
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [Qemu-devel] [PATCH 20/25] block/nbd: Comment on discard/flush silently failing
2015-03-16 14:49 ` Max Reitz
@ 2015-03-16 14:51 ` Paolo Bonzini
2015-03-16 14:52 ` Max Reitz
0 siblings, 1 reply; 46+ messages in thread
From: Paolo Bonzini @ 2015-03-16 14:51 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
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
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [Qemu-devel] [PATCH 20/25] block/nbd: Comment on discard/flush silently failing
2015-03-16 14:51 ` Paolo Bonzini
@ 2015-03-16 14:52 ` Max Reitz
0 siblings, 0 replies; 46+ messages in thread
From: Max Reitz @ 2015-03-16 14:52 UTC (permalink / raw)
To: Paolo Bonzini, qemu-block; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
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
^ permalink raw reply [flat|nested] 46+ messages in thread
end of thread, other threads:[~2015-03-16 14:52 UTC | newest]
Thread overview: 46+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-02-25 18:08 [Qemu-devel] [PATCH 00/25] nbd: Several fixes Max Reitz
2015-02-25 18:08 ` [Qemu-devel] [PATCH 01/25] util/uri: Add overflow check to rfc3986_parse_port Max Reitz
2015-02-25 18:08 ` [Qemu-devel] [PATCH 02/25] qemu-nbd: Detect unused partitions by system == 0 Max Reitz
2015-02-25 18:08 ` [Qemu-devel] [PATCH 03/25] nbd: Fix nbd_establish_connection()'s return value Max Reitz
2015-02-25 18:08 ` [Qemu-devel] [PATCH 04/25] nbd: Fix response to invalid requests Max Reitz
2015-03-02 16:52 ` Max Reitz
2015-02-25 18:08 ` [Qemu-devel] [PATCH 05/25] nbd: Avoid generic -EINVAL Max Reitz
2015-03-11 11:22 ` Paolo Bonzini
2015-03-16 13:51 ` Max Reitz
2015-03-16 14:42 ` Paolo Bonzini
2015-03-16 14:48 ` Max Reitz
2015-03-16 14:49 ` Paolo Bonzini
2015-02-25 18:08 ` [Qemu-devel] [PATCH 06/25] nbd: Pass return value from nbd_handle_list() Max Reitz
2015-02-25 18:08 ` [Qemu-devel] [PATCH 07/25] nbd: Add "failed to open export" error message Max Reitz
2015-03-11 11:24 ` Paolo Bonzini
2015-03-16 13:55 ` Max Reitz
2015-02-25 18:08 ` [Qemu-devel] [PATCH 08/25] nbd: Handle blk_getlength() failure Max Reitz
2015-03-11 11:26 ` Paolo Bonzini
2015-02-25 18:08 ` [Qemu-devel] [PATCH 09/25] qemu-nbd: fork() can fail Max Reitz
2015-02-25 18:08 ` [Qemu-devel] [PATCH 10/25] nbd: Fix potential signed overflow issues Max Reitz
2015-03-11 11:28 ` Paolo Bonzini
2015-02-25 18:08 ` [Qemu-devel] [PATCH 11/25] qemu-nbd: Fix and improve input verification Max Reitz
2015-03-11 11:30 ` Paolo Bonzini
2015-03-16 13:56 ` Max Reitz
2015-02-25 18:08 ` [Qemu-devel] [PATCH 12/25] nbd: Set block size to BDRV_SECTOR_SIZE Max Reitz
2015-02-25 18:08 ` [Qemu-devel] [PATCH 13/25] nbd: Enforce sector alignment Max Reitz
2015-02-25 18:08 ` [Qemu-devel] [PATCH 14/25] coroutine: Add co_yield_timeout() Max Reitz
2015-02-25 18:08 ` [Qemu-devel] [PATCH 15/25] coroutine-io: Return -errno in case of error Max Reitz
2015-02-25 18:08 ` [Qemu-devel] [PATCH 16/25] coroutine-io: Add I/O functions with timeout Max Reitz
2015-02-25 18:08 ` [Qemu-devel] [PATCH 17/25] nbd: Employ timeouts Max Reitz
2015-02-25 18:08 ` [Qemu-devel] [PATCH 18/25] nbd: Fix nbd_receive_options() Max Reitz
2015-02-25 18:08 ` [Qemu-devel] [PATCH 19/25] nbd: Fix interpretation of the export flags Max Reitz
2015-02-25 18:08 ` [Qemu-devel] [PATCH 20/25] block/nbd: Comment on discard/flush silently failing Max Reitz
2015-03-11 11:31 ` Paolo Bonzini
2015-03-16 13:58 ` Max Reitz
2015-03-16 14:44 ` Paolo Bonzini
2015-03-16 14:49 ` Max Reitz
2015-03-16 14:51 ` Paolo Bonzini
2015-03-16 14:52 ` Max Reitz
2015-02-25 18:08 ` [Qemu-devel] [PATCH 21/25] nbd: Drop unexpected data for NBD_OPT_LIST Max Reitz
2015-02-25 18:08 ` [Qemu-devel] [PATCH 22/25] iotests: Add _timeout function Max Reitz
2015-02-25 18:08 ` [Qemu-devel] [PATCH 23/25] iotests: Add test for invalid qemu-nbd parameters Max Reitz
2015-02-25 18:08 ` [Qemu-devel] [PATCH 24/25] iotests: Add test for issuing discard over NBD Max Reitz
2015-02-25 18:08 ` [Qemu-devel] [PATCH 25/25] iotests: Add test for a non-existing NBD export Max Reitz
2015-02-25 18:11 ` [Qemu-devel] [PATCH 00/25] nbd: Several fixes Max Reitz
2015-03-11 11:36 ` Paolo Bonzini
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).