nbd client refactoring and fixing
cover nbd client refactoring and fixing
A bit more refactoring and fixing before BLOCK_STATUS series. I've tried to make individual patches simple enough, so there are a lot of them.
Vladimir Sementsov-Ogievskiy (17):
nbd/client: fix nbd_opt_go
nbd/client: refactor nbd_read_eof
nbd/client: refactor nbd_receive_reply
nbd/client: fix nbd_send_request to return int block/nbd-client: get rid of ssize_t
block/nbd-client: fix nbd_read_reply_entry block/nbd-client: refactor request send/receive block/nbd-client: rename nbd_recv_coroutines_enter_all block/nbd-client: move nbd_co_receive_reply content into
nbd_co_request
block/nbd-client: move nbd_coroutine_end content into nbd_co_request block/nbd-client: fix nbd_co_request: set s->reply.handle to 0 on
error
block/nbd-client: refactor nbd_co_request block/nbd-client: refactor NBDClientSession.recv_coroutine block/nbd-client: exit reply-reading coroutine on incorrect handle block/nbd-client: refactor reading reply
block/nbd-client: drop reply field from NBDClientSession block/nbd-client: always return EIO on and after the first io channel
error
block/nbd-client.h | 9 ++- include/block/nbd.h | 4 +- nbd/nbd-internal.h | 34 ++++++--- block/nbd-client.c | 173 ++++++++++++++++++--------------------------- nbd/client.c | 21 +++--- tests/qemu-iotests/083.out | 4 +- 6 files changed, 115 insertions(+), 130 deletions(-)
Signature · 1 line
2.11.1
PATCH 1/17 nbd/client: fix nbd_opt_go
Do not send NBD_OPT_ABORT to the broken server. After sending NBD_REP_ACK on NBD_OPT_GO server is most probably in transmission phase, when option sending is finished.
--- nbd/client.c | 2 -- 1 file changed, 2 deletions(-)
nbd/client.c +0 -2
@@ -399,12 +399,10 @@ static int nbd_opt_go(QIOChannel *ioc, const char *wantname,phase, but make sure it sent flags */ if (len) {error_setg (errp,"server sent invalid NBD_REP_ACK" ); -nbd_send_opt_abort (ioc);return -1 ; }if (!info->flags) {error_setg (errp,"broken server omitted NBD_INFO_EXPORT" ); -nbd_send_opt_abort (ioc);return -1 ; }trace_nbd_opt_go_success ();
Signature · 1 line
2.11.1
Re: [Qemu-devel] [PATCH 01/17] nbd/client: fix nbd_opt_go
Quoted 8 lines · Vladimir Sementsov-Ogievskiy
Do not send
NBD_OPT_ABORTto the broken server. After sendingNBD_REP_ACKonNBD_OPT_GOserver is most probably in transmission phase, when option sending is finished.Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>--- nbd/client.c | 2 -- 1 file changed, 2 deletions(-)
But buggy servers aren't a severe enough problem to warrant this being in 2.10. I'll save this for 2.11.
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
PATCH 2/17 nbd/client: refactor nbd_read_eof
In reply to Vladimir Sementsov-OgievskiyRefactor nbd_read_eof to return 1 on success, 0 on eof, when no data was read and <0 for other cases, because returned size of read data is not actually used.
--- nbd/nbd-internal.h | 34 +++++++++++++++++++++++++--------- nbd/client.c | 5 ----- tests/qemu-iotests/083.out | 4 ++-- 3 files changed, 27 insertions(+), 16 deletions(-)
nbd/nbd-internal.h +25 -9
@@ -77,21 +77,37 @@#define NBD_ESHUTDOWN 108/* nbd_read_eof -* Tries to read @size bytes from @ioc. Returns number of bytes actually read. -* May return a value >= 0 and < size only on EOF, i.e. when iteratively called -* qio_channel_readv() returns 0. So, there is no need to call nbd_read_eof -* iteratively. +* Tries to read @size bytes from @ioc. +* Returns 1 on success +* 0 on eof, when no data was read (errp is not set) +* -EINVAL on eof, when some data < @size was read until eof +* < 0 on read fail */ -static inline ssize_t nbd_read_eof (QIOChannel *ioc,void *buffer,size_t size, -Error **errp) +static inline int nbd_read_eof (QIOChannel *ioc,void *buffer,size_t size, +Error **errp) {struct iovec iov = { .iov_base = buffer, .iov_len = size }; +ssize_t ret; +/* Sockets are kept in blocking mode in the negotiation phase. After * that, a non-readable socket simply means that another thread stole * our request/reply. Synchronization is done with recv_coroutine, so * that this is coroutine-safe. */ -return nbd_rwv (ioc, &iov,1 , size, true, errp); + +assert (size >0 ); + + ret =nbd_rwv (ioc, &iov,1 , size, true, errp); +if (ret <=0 ) { +return ret; + } + +if (ret != size) { +error_setg (errp,"End of file" ); +return -EINVAL ; + } + +return 1 ; }/* nbd_read
@@ -100,9 +116,9 @@ static inline ssize_t nbd_read_eof(QIOChannel *ioc, void *buffer, size_t size,static inline int nbd_read (QIOChannel *ioc,void *buffer,size_t size,Error **errp) { -ssize_t ret =nbd_read_eof (ioc, buffer, size, errp); +int ret =nbd_read_eof (ioc, buffer, size, errp); -if (ret >=0 && ret != size) { +if (ret ==0 ) { ret = -EINVAL ;error_setg (errp,"End of file" ); }
nbd/client.c +0 -5
@@ -925,11 +925,6 @@ ssize_t nbd_receive_reply(QIOChannel *ioc, NBDReply *reply, Error **errp)return ret; } -if (ret !=sizeof (buf)) { -error_setg (errp,"read failed" ); -return -EINVAL ; - } -/* Reply [ 0 .. 3] magic (NBD_REPLY_MAGIC) [ 4 .. 7] error (0 == no error)
tests/qemu-iotests/083.out +2 -2
@@ -69,12 +69,12 @@ read failed: Input/output error === Check disconnect 4 reply === -read failed +End of file read failed: Input/output error === Check disconnect 8 reply === -read failed +End of file read failed: Input/output error === Check disconnect before data ===
Signature · 1 line
2.11.1
Re: [Qemu-devel] [PATCH 02/17] nbd/client: refactor nbd_read_eof
Quoted 18 lines · Vladimir Sementsov-Ogievskiy
Refactor
nbd_read_eofto return 1 on success, 0 on eof, when no data was read and <0 for other cases, because returned size of read data is not actually used.Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>--- nbd/nbd-internal.h | 34 +++++++++++++++++++++++++--------- nbd/client.c | 5 ----- tests/qemu-iotests/083.out | 4 ++-- 3 files changed, 27 insertions(+), 16 deletions(-)nbd/nbd-internal.h +0
-0@@ -77,21 +77,37 @@#define NBD_ESHUTDOWN 108
nbd/nbd-internal.h line 84
Quoted 10 lines
nbd/nbd-internal.h +5
-4/* nbd_read_eof -* Tries to read @size bytes from @ioc. Returns number of bytes actually read. -* May return a value >= 0 and < size only on EOF, i.e. when iteratively called -* qio_channel_readv() returns 0. So, there is no need to call nbd_read_eof -* iteratively. +* Tries to read @size bytes from @ioc. +* Returns 1 on success +* 0 on eof, when no data was read (errp is not set) +* -EINVAL on eof, when some data < @size was read until eof +* < 0 on read fail
In general, mixing negative errno value and generic < 0 in the same function is most likely ambiguous.
Quoted 7 lines
nbd/nbd-internal.h +2
-2*/ -static inline ssize_t nbd_read_eof (QIOChannel *ioc,void *buffer,size_t size, -Error **errp) +static inline int nbd_read_eof (QIOChannel *ioc,void *buffer,size_t size, +Error **errp) {struct iovec iov = { .iov_base = buffer, .iov_len = size };
nbd/nbd-internal.h line 98
Quoted 10 lines
nbd/nbd-internal.h +4
-1+ssize_t ret; +/* Sockets are kept in blocking mode in the negotiation phase. After * that, a non-readable socket simply means that another thread stole * our request/reply. Synchronization is done with recv_coroutine, so * that this is coroutine-safe. */ -return nbd_rwv (ioc, &iov,1 , size, true, errp); + +assert (size >0 );
Effectively the same as assert(size != 0).
nbd/nbd-internal.h line 103
Quoted 5 lines
nbd/nbd-internal.h +5
-0+ + ret =nbd_rwv (ioc, &iov,1 , size, true, errp); +if (ret <=0 ) { +return ret; + }
So this is a negative errno (or 0 on EOF),
nbd/nbd-internal.h line 107
Quoted 4 lines
nbd/nbd-internal.h +4
-0+ +if (ret != size) { +error_setg (errp,"End of file" ); +return -EINVAL ;
and so is this. Which makes the function documentation not quite accurate; you aren't mixing a generic < 0.
Quoted 7 lines
nbd/nbd-internal.h +3
-0+ } + +return 1 ; }/* nbd_read
nbd/nbd-internal.h static inline ssize_t nbd_read_eof(QIOChannel *ioc, void *buffer, size_t size, · line 123
Quoted 10 lines
nbd/nbd-internal.h +2
-2@@ -100,9 +116,9 @@ static inline ssize_t nbd_read_eof(QIOChannel *ioc, void *buffer, size_t size,static inline int nbd_read (QIOChannel *ioc,void *buffer,size_t size,Error **errp) { -ssize_t ret =nbd_read_eof (ioc, buffer, size, errp); +int ret =nbd_read_eof (ioc, buffer, size, errp); -if (ret >=0 && ret != size) { +if (ret ==0 ) { ret = -EINVAL ;error_setg (errp,"End of file" );
Why do we have to set errp here instead of in nbd_read_eof()? Is there ever any case where hitting early EOF is not something that should be treated as an error?
Quoted 22 lines
nbd/nbd-internal.h +0
-0}nbd/client.c +0
-5@@ -925,11 +925,6 @@ ssize_t nbd_receive_reply(QIOChannel *ioc, NBDReply *reply, Error **errp)return ret; } -if (ret !=sizeof (buf)) { -error_setg (errp,"read failed" ); -return -EINVAL ; - } -/* Reply [ 0 .. 3] magic (NBD_REPLY_MAGIC) [ 4 .. 7] error (0 == no error) tests/qemu-iotests/083.out +0
-0
tests/qemu-iotests/083.out read failed: Input/output error · line 73
Quoted 6 lines
tests/qemu-iotests/083.out +1
-1@@ -69,12 +69,12 @@ read failed: Input/output error === Check disconnect 4 reply === -read failed +End of file read failed: Input/output error
At least you tracked that your changes tweak the error message. But I'm not yet convinced whether this change simplifies anything. Is there a later patch that is easier to write with the new semantics which was not possible with the pre-patch semantics?
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH 02/17] nbd/client: refactor nbd_read_eof
Quoted 19 lines · Eric Blake
Quoted 18 lines · Vladimir Sementsov-Ogievskiy
Refactor
nbd_read_eofto return 1 on success, 0 on eof, when no data was read and <0 for other cases, because returned size of read data is not actually used.Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>--- nbd/nbd-internal.h | 34 +++++++++++++++++++++++++--------- nbd/client.c | 5 ----- tests/qemu-iotests/083.out | 4 ++-- 3 files changed, 27 insertions(+), 16 deletions(-)nbd/nbd-internal.h +0
-0@@ -77,21 +77,37 @@#define NBD_ESHUTDOWN 108
Quoted 12 lines
nbd/nbd-internal.h line 84
Quoted 10 lines
nbd/nbd-internal.h +5
-4/* nbd_read_eof -* Tries to read @size bytes from @ioc. Returns number of bytes actually read. -* May return a value >= 0 and < size only on EOF, i.e. when iteratively called -* qio_channel_readv() returns 0. So, there is no need to call nbd_read_eof -* iteratively. +* Tries to read @size bytes from @ioc. +* Returns 1 on success +* 0 on eof, when no data was read (errp is not set) +* -EINVAL on eof, when some data < @size was read until eof +* < 0 on read fail In general, mixing negative errno value and generic < 0 in the same function is most likely ambiguous.
Hmm, but this is entirely what we do so often:
if (,,) return -EINVAL;
return some_other_func().
last two lines may be rewritten like this:
+ * < 0 on fail
Quoted 20 lines
Quoted 8 lines
nbd/nbd-internal.h +2
-2*/ -static inline ssize_t nbd_read_eof (QIOChannel *ioc,void *buffer,size_t size, -Error **errp) +static inline int nbd_read_eof (QIOChannel *ioc,void *buffer,size_t size, +Error **errp) {struct iovec iov = { .iov_base = buffer, .iov_len = size };nbd/nbd-internal.h line 98
Quoted 10 lines
nbd/nbd-internal.h +4
-1+ssize_t ret; +/* Sockets are kept in blocking mode in the negotiation phase. After * that, a non-readable socket simply means that another thread stole * our request/reply. Synchronization is done with recv_coroutine, so * that this is coroutine-safe. */ -return nbd_rwv (ioc, &iov,1 , size, true, errp); + +assert (size >0 );Effectively the same as assert(size != 0).
Quoted 6 lines
nbd/nbd-internal.h line 103
Quoted 5 lines
nbd/nbd-internal.h +5
-0+ + ret =nbd_rwv (ioc, &iov,1 , size, true, errp); +if (ret <=0 ) { +return ret; + }So this is a negative errno (or 0 on EOF),
if it is < 0, it can be only -EIO, specified in nbd_rwv "by hand". it is unrelated to read read/write errno's
Quoted 7 lines
nbd/nbd-internal.h line 107
Quoted 5 lines
nbd/nbd-internal.h +4
-0+ +if (ret != size) { +error_setg (errp,"End of file" ); +return -EINVAL ;and so is this. Which makes the function documentation not quite accurate; you aren't mixing a generic < 0.
hmm.. my wordings are weird sometimes, sorry for that :(. and thank you for your patience.
Quoted 14 lines
Quoted 14 lines
nbd/nbd-internal.h +4
-1+ } + +return 1 ; }/* nbd_read @@ -100,9 +116,9 @@ static inline ssize_t nbd_read_eof(QIOChannel *ioc, void *buffer, size_t size,static inline int nbd_read (QIOChannel *ioc,void *buffer,size_t size,Error **errp) { -ssize_t ret =nbd_read_eof (ioc, buffer, size, errp); +int ret =nbd_read_eof (ioc, buffer, size, errp);
Quoted 7 lines
nbd/nbd-internal.h static inline ssize_t nbd_read_eof(QIOChannel *ioc, void *buffer, size_t size, · line 123
Quoted 4 lines
nbd/nbd-internal.h +1
-1-if (ret >=0 && ret != size) { +if (ret ==0 ) { ret = -EINVAL ;error_setg (errp,"End of file" );Why do we have to set errp here instead of in
nbd_read_eof()? Is there ever any case where hitting early EOF is not something that should be treated as an error?
yes. it is the only usage of nbd_read_eof - in nbd_receive_reply. This used to understand that there no more replies to read.
Quoted 26 lines
Quoted 26 lines
nbd/nbd-internal.h +0
-0}nbd/client.c +0
-5@@ -925,11 +925,6 @@ ssize_t nbd_receive_reply(QIOChannel *ioc, NBDReply *reply, Error **errp)return ret; } -if (ret !=sizeof (buf)) { -error_setg (errp,"read failed" ); -return -EINVAL ; - } -/* Reply [ 0 .. 3] magic (NBD_REPLY_MAGIC) [ 4 .. 7] error (0 == no error) tests/qemu-iotests/083.out +0
-0@@ -69,12 +69,12 @@ read failed: Input/output error === Check disconnect 4 reply ===
Quoted 7 lines
tests/qemu-iotests/083.out read failed: Input/output error · line 73
Quoted 3 lines
tests/qemu-iotests/083.out +1
-1-read failed +End of file read failed: Input/output errorAt least you tracked that your changes tweak the error message. But I'm not yet convinced whether this change simplifies anything. Is there a later patch that is easier to write with the new semantics which was not possible with the pre-patch semantics?
This patch just moves error handling one level down (do not propagate unused information). And removes (with the following patch) last remains of ssize_t and returning number of bytes in nbd/client.c - for consistency. Let nbd_rwv to be the only function returning number of bytes.
Quoted 1 line
Signature · 2 lines
Best regards, Vladimir
Eric Blake
Reviewed-byRe: [Qemu-devel] [PATCH 02/17] nbd/client: refactor nbd_read_eof
Quoted 16 lines · Vladimir Sementsov-Ogievskiy
Quoted 8 lines · Eric Blake
Quoted 6 lines · Vladimir Sementsov-Ogievskiy
Refactor
nbd_read_eofto return 1 on success, 0 on eof, when no data was read and <0 for other cases, because returned size of read data is not actually used.Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>---
Quoted 6 lines
Quoted 4 lines
+ * Returns 1 on success
+ * 0 on eof, when no data was read (errp is not set) + * -EINVAL on eof, when some data < @size was read until eof + * < 0 on read failIn general, mixing negative errno value and generic < 0 in the same function is most likely ambiguous.
Quoted 8 lines
Hmm, but this is entirely what we do so often:
if (,,) return
-EINVAL;return
some_other_func().last two lines may be rewritten like this:
+ * < 0 on fail
Or even better as 'negative errno on failure'.
Here's what I'm proposing to squash in, at which point you have:
nbd/nbd-internal.h +1 -2
@@ -80,8 +80,7 @@* Tries to read @size bytes from @ioc. * Returns 1 on success * 0 on eof, when no data was read (errp is not set) -* -EINVAL on eof, when some data < @size was read until eof -* < 0 on read fail +* negative errno on failure (errp is set) */ static inline int nbd_read_eof (QIOChannel *ioc,void *buffer,size_t size,Error **errp)
@@ -95,7 +94,7 @@ static inline int nbd_read_eof(QIOChannel *ioc, void
*buffer, size_t size,
* that this is coroutine-safe.
*/
- assert(size > 0);
+ assert(size);
ret = nbd_rwv(ioc, &iov, 1, size, true, errp);
if (ret <= 0) {Quoted 9 lines
Quoted 6 lines
Review
Quoted 4 lines
(quoted diff) +4
-0+ + if (ret != size) { + error_setg(errp, "End of file"); + return -EINVAL;and so is this. Which makes the function documentation not quite accurate; you aren't mixing a generic < 0.
hmm.. my wordings are weird sometimes, sorry for that :(. and thank you for your patience.
Not a problem - I understand that English is not your native language, so you are already a leg up on me (I'm nowhere near as competent in a second language as you already are on English, even if review still gives you grammar hints and other improvements).
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
PATCH 3/17 nbd/client: refactor nbd_receive_reply
In reply to Vladimir Sementsov-OgievskiyRefactor nbd_receive_reply to return 1 on success, 0 on eof, when no data was read and <0 for other cases, because returned size of read data is not actually used.
--- include/block/nbd.h | 2 +- nbd/client.c | 12 +++++++++--- 2 files changed, 10 insertions(+), 4 deletions(-)
include/block/nbd.h +1 -1
@@ -164,7 +164,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name,int nbd_init (int fd,QIOChannelSocket *sioc,NBDExportInfo *info,Error **errp);ssize_t nbd_send_request (QIOChannel *ioc,NBDRequest *request); -ssize_t nbd_receive_reply (QIOChannel *ioc,NBDReply *reply,Error **errp); +int nbd_receive_reply (QIOChannel *ioc,NBDReply *reply,Error **errp);int nbd_client (int fd);int nbd_disconnect (int fd);
nbd/client.c +9 -3
@@ -914,11 +914,16 @@ ssize_t nbd_send_request(QIOChannel *ioc, NBDRequest *request)return nbd_write (ioc, buf,sizeof (buf),NULL ); } -ssize_t nbd_receive_reply (QIOChannel *ioc,NBDReply *reply,Error **errp) +/* nbd_receive_reply +* Returns 1 on success +* 0 on eof, when no data was read from @ioc (errp is not set) +* < 0 on fail +*/ +int nbd_receive_reply (QIOChannel *ioc,NBDReply *reply,Error **errp) {uint8_t buf[NBD_REPLY_SIZE ];uint32_t magic; -ssize_t ret; +int ret; ret =nbd_read_eof (ioc, buf,sizeof (buf), errp);if (ret <=0 ) {
@@ -948,6 +953,7 @@ ssize_t nbd_receive_reply(QIOChannel *ioc, NBDReply *reply, Error **errp)error_setg (errp,"invalid magic (got 0x%" PRIx32")" , magic);return -EINVAL ; } -return sizeof (buf); + +return 1 ; }
Signature · 1 line
2.11.1
Re: [Qemu-devel] [PATCH 03/17] nbd/client: refactor nbd_receive_reply
Quoted 13 lines · Vladimir Sementsov-Ogievskiy
Refactor
nbd_receive_replyto return 1 on success, 0 on eof, when no data was read and <0 for other cases, because returned size of read data is not actually used.Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>--- include/block/nbd.h | 2 +- nbd/client.c | 12 +++++++++--- 2 files changed, 10 insertions(+), 4 deletions(-)+++
b/nbd/client.c
Review ssize_t nbd_send_request(QIOChannel *ioc, NBDRequest *request) · line 920
Quoted 8 lines
(quoted diff) +4
-1@@ -914,11 +914,16 @@ ssize_t nbd_send_request(QIOChannel *ioc, NBDRequest *request) return nbd_write(ioc, buf, sizeof(buf), NULL); } -ssize_t nbd_receive_reply(QIOChannel *ioc, NBDReply *reply, Error **errp) +/* nbd_receive_reply + * Returns 1 on success + * 0 on eof, when no data was read from @ioc (errp is not set) + * < 0 on fail
Similar to the previous patch, I'd like to squash in:
nbd/client.c +0 -0
@@ -916,8 +916,8 @@ ssize_t nbd_send_request(QIOChannel *ioc, NBDRequest
*request)
/* nbd_receive_reply
* Returns 1 on success
- * 0 on eof, when no data was read from @ioc (errp is not set) - * < 0 on fail + * 0 on eof, when no data was read (errp is not set) + * negative errno on failure (errp is set)
*/
int nbd_receive_reply(QIOChannel *ioc, NBDReply *reply, Error **errp) {
With that,
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
PATCH 4/17 nbd/client: fix nbd_send_request to return int
In reply to Vladimir Sementsov-OgievskiyFix nbd_send_request to return int, as it returns a return value of nbd_write (which is int), and the only user of nbd_send_request's return value (nbd_co_send_request) consider it as int too.
--- include/block/nbd.h | 2 +- nbd/client.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
include/block/nbd.h +1 -1
@@ -163,7 +163,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, Error **errp);int nbd_init (int fd,QIOChannelSocket *sioc,NBDExportInfo *info,Error **errp); -ssize_t nbd_send_request (QIOChannel *ioc,NBDRequest *request); +int nbd_send_request (QIOChannel *ioc,NBDRequest *request);int nbd_receive_reply (QIOChannel *ioc,NBDReply *reply,Error **errp);int nbd_client (int fd);int nbd_disconnect (int fd);
nbd/client.c +1 -1
@@ -896,7 +896,7 @@ int nbd_disconnect(int fd) }#endif -ssize_t nbd_send_request (QIOChannel *ioc,NBDRequest *request) +int nbd_send_request (QIOChannel *ioc,NBDRequest *request) {uint8_t buf[NBD_REQUEST_SIZE ];
Signature · 1 line
2.11.1
Re: [Qemu-devel] [PATCH 04/17] nbd/client: fix nbd_send_request to return int
Quoted 3 lines · Vladimir Sementsov-Ogievskiy
Fix
nbd_send_requestto return int, as it returns a return value ofnbd_write(which is int), and the only user of nbd_send_request's return value (nbd_co_send_request) consider it as int too.
The real problem here is the return value of nbd_write() - it should be a ssize_t, not an int, since it is propagating the return value of nbd_rwv() which is ssize_t. Basically all functions that do I/O and return the number of bytes read/written should be ssize_t - any use of int is a bug.
Regards,
Daniel
Signature · 3 lines
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH 04/17] nbd/client: fix nbd_send_request to return int
Quoted 7 lines · Daniel P. Berrange
Quoted 3 lines · Vladimir Sementsov-Ogievskiy
Fix
nbd_send_requestto return int, as it returns a return value ofnbd_write(which is int), and the only user of nbd_send_request's return value (nbd_co_send_request) consider it as int too.The real problem here is the return value of
nbd_write()- it should be assize_t, not an int, since it is propagating the return value ofnbd_rwv()which isssize_t.
It was changed in f5d406fe86bb (sent in May) The discussion actually was started half a year ago:
https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg00825.html
Quoted 6 lines
Basically all functions that do I/O and
return the number of bytes read/written should bessize_t- any use of int is a bug.Regards,
Daniel
Signature · 2 lines
Best regards, Vladimir
Re: [Qemu-devel] [PATCH 04/17] nbd/client: fix nbd_send_request to return int
Quoted 6 lines · Vladimir Sementsov-Ogievskiy
Quoted 5 lines · Daniel P. Berrange
Quoted 3 lines · Vladimir Sementsov-Ogievskiy
Fix
nbd_send_requestto return int, as it returns a return value ofnbd_write(which is int), and the only user of nbd_send_request's return value (nbd_co_send_request) consider it as int too.
Quoted 12 lines
Quoted 3 lines
The real problem here is the return value of
nbd_write()- it should be assize_t, not an int, since it is propagating the return value ofnbd_rwv()which isssize_t.It was changed in
f5d406fe86bb(sent in May) The discussion actually was started half a year ago:
https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg00825.htmlQuoted 3 lines
Basically all functions that do I/O and
return the number of bytes read/written should bessize_t- any use of int is a bug.
If returning the size matters, then yes, not using ssize_t is a bug. But if all we need is to know whether an entire expected length was read (and treat ANY partial read the same as failure), then a mere int becomes enough to encode the results.
Where I'm less certain is whether this change in semantics simplifies later patches, or loses information that might have been useful. But intuitively, ANY encounter of EOF means that NBD needs to quit trying to talk to the other end of the socket, whether or not the EOF occurred on a nice message boundary; and the rest of the protocol is strongly tied to messages, where we don't act until we have read the entire expected message; so if a later patch is indeed simpler by not returning bytes read, this patch might be okay.
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH 04/17] nbd/client: fix nbd_send_request to return int
In reply to Vladimir Sementsov-OgievskiyQuoted 5 lines · Vladimir Sementsov-Ogievskiy
Quoted 4 lines · Daniel P. Berrange
Quoted 3 lines · Vladimir Sementsov-Ogievskiy
Fix
nbd_send_requestto return int, as it returns a return value ofnbd_write(which is int), and the only user of nbd_send_request's return value (nbd_co_send_request) consider it as int too.
Quoted 7 lines
Quoted 3 lines
The real problem here is the return value of
nbd_write()- it should be assize_t, not an int, since it is propagating the return value ofnbd_rwv()which isssize_t.It was changed in
f5d406fe86bb(sent in May) The discussion actually was started half a year ago:
https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg00825.html
Actually I misread the code. nbd_rwv() is returning 0 on success, not the number of bytes, so int is in fact ok
Regards,
Daniel
Signature · 3 lines
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH 04/17] nbd/client: fix nbd_send_request to return int
In reply to Vladimir Sementsov-OgievskiyQuoted 10 lines · Vladimir Sementsov-Ogievskiy
Fix
nbd_send_requestto return int, as it returns a return value ofnbd_write(which is int), and the only user of nbd_send_request's return value (nbd_co_send_request) consider it as int too.Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>--- include/block/nbd.h | 2 +- nbd/client.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
PATCH 5/17 block/nbd-client: get rid of ssize_t
In reply to Vladimir Sementsov-OgievskiyUse int variable for nbd_co_send_request return value (as nbd_co_send_request returns int).
--- block/nbd-client.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
block/nbd-client.c +5 -5
@@ -214,7 +214,7 @@ int nbd_client_co_preadv(BlockDriverState *bs, uint64_t offset, .len = bytes, };NBDReply reply; -ssize_t ret; +int ret;assert (bytes <=NBD_MAX_BUFFER_SIZE );assert (!flags);
@@ -239,7 +239,7 @@ int nbd_client_co_pwritev(BlockDriverState *bs, uint64_t offset, .len = bytes, };NBDReply reply; -ssize_t ret; +int ret;if (flags &BDRV_REQ_FUA ) {assert (client->info.flags &NBD_FLAG_SEND_FUA );
@@ -261,7 +261,7 @@ int nbd_client_co_pwritev(BlockDriverState *bs, uint64_t offset,int nbd_client_co_pwrite_zeroes (BlockDriverState *bs,int64_t offset,int bytes,BdrvRequestFlags flags) { -ssize_t ret; +int ret;NBDClientSession *client =nbd_get_client_session (bs);NBDRequest request = { .type =NBD_CMD_WRITE_ZEROES ,
@@ -297,7 +297,7 @@ int nbd_client_co_flush(BlockDriverState *bs)NBDClientSession *client =nbd_get_client_session (bs);NBDRequest request = { .type =NBD_CMD_FLUSH };NBDReply reply; -ssize_t ret; +int ret;if (!(client->info.flags &NBD_FLAG_SEND_FLUSH )) {return 0 ;
@@ -325,7 +325,7 @@ int nbd_client_co_pdiscard(BlockDriverState *bs, int64_t offset, int bytes) .len = bytes, };NBDReply reply; -ssize_t ret; +int ret;if (!(client->info.flags &NBD_FLAG_SEND_TRIM )) {return 0 ;
Signature · 1 line
2.11.1
Re: [Qemu-devel] [PATCH 05/17] block/nbd-client: get rid of ssize_t
Quoted 2 lines · Vladimir Sementsov-Ogievskiy
Use int variable for
nbd_co_send_requestreturn value (asnbd_co_send_requestreturns int).
Hmm, nbd_co_send_request() propagates return value of nbd_send_request, which returns ssize_t. IOW, I think we need to fix nbd_co_send_request to also return ssize_t, rather than changing callers to use int.
Regards,
Daniel
Signature · 3 lines
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH 05/17] block/nbd-client: get rid of ssize_t
Quoted 5 lines · Daniel P. Berrange
Quoted 2 lines · Vladimir Sementsov-Ogievskiy
Use int variable for
nbd_co_send_requestreturn value (asnbd_co_send_requestreturns int).Hmm,
nbd_co_send_request()propagates return value ofnbd_send_request, which returnsssize_t.
See patch 04, nbd_send_request fixed in it to return int. It propagates return value of nbd_write which is int.
Quoted 5 lines
to also return
ssize_t, rather than changing callers to use int.Regards,
Daniel
Signature · 2 lines
Best regards, Vladimir
Re: [Qemu-devel] [PATCH 05/17] block/nbd-client: get rid of ssize_t
Quoted 10 lines · Vladimir Sementsov-Ogievskiy
Quoted 5 lines · Daniel P. Berrange
Quoted 2 lines · Vladimir Sementsov-Ogievskiy
Use int variable for
nbd_co_send_requestreturn value (asnbd_co_send_requestreturns int).Hmm,
nbd_co_send_request()propagates return value ofnbd_send_request, which returnsssize_t.See patch 04,
nbd_send_requestfixed in it to return int. It propagates return
value ofnbd_writewhich is int.
That's wrong too - nbd_write() should have been fixed to have ssize_t as its return type, since its returning the value of a ssize_t variable.
Regards,
Daniel
Signature · 3 lines
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH 05/17] block/nbd-client: get rid of ssize_t
In reply to Vladimir Sementsov-OgievskiyQuoted 7 lines · Vladimir Sementsov-Ogievskiy
Use int variable for
nbd_co_send_requestreturn value (asnbd_co_send_requestreturns int).Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>--- block/nbd-client.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
PATCH 6/17 block/nbd-client: fix nbd_read_reply_entry
In reply to Vladimir Sementsov-OgievskiySet reply.handle to 0 on error path to prevent normal path of nbd_co_receive_reply.
--- block/nbd-client.c | 1 + 1 file changed, 1 insertion(+)
block/nbd-client.c +1 -0
@@ -107,6 +107,7 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)qemu_coroutine_yield (); } + s->reply.handle =0 ;nbd_recv_coroutines_enter_all (s); s->read_reply_co =NULL ; }
Signature · 1 line
2.11.1
Re: [Qemu-devel] [PATCH 06/17] block/nbd-client: fix nbd_read_reply_entry
Quoted 7 lines · Vladimir Sementsov-Ogievskiy
Set reply.handle to 0 on error path to prevent normal path of
nbd_co_receive_reply.Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>--- block/nbd-client.c | 1 + 1 file changed, 1 insertion(+)
Can you document a case where not fixing this would be an observable bug (even if it requires using gdb and single-stepping between client and server to make what is otherwise a racy situation easy to see)? I'm trying to figure out if this is 2.10 material.
Quoted 14 lines
block/nbd-client.c +1
-0@@ -107,6 +107,7 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)qemu_coroutine_yield (); } + s->reply.handle =0 ;nbd_recv_coroutines_enter_all (s); s->read_reply_co =NULL ; }
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH 06/17] block/nbd-client: fix nbd_read_reply_entry
Quoted 4 lines · Eric Blake
Quoted 3 lines · Vladimir Sementsov-Ogievskiy
Set reply.handle to 0 on error path to prevent normal path of
nbd_co_receive_reply.
Quoted 8 lines
Quoted 4 lines
Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>--- block/nbd-client.c | 1 + 1 file changed, 1 insertion(+)Can you document a case where not fixing this would be an observable bug (even if it requires using gdb and single-stepping between client and server to make what is otherwise a racy situation easy to see)? I'm trying to figure out if this is 2.10 material.
it is simple enough:
run qemu-nbd in gdb, set break on nbd_send_reply, and when it shoot s, next up to "stl_be_p(buf, NBD_REPLY_MAGIC);" and after it do "call stl_be_p(buf, 1000)"
run qemu-io with some read in gdb, set break on
br block/nbd-client.c:83
( it is break; after failed nbd_receive_reply call)
and on
br block/nbd-client.c:170
(it is in nbd_co_receive_reply after yield)
on first break we will be sure that nbd_receive_reply failed, on second we will be sure by
(gdb) p s->reply
$1 = {handle = 93825000680144, error = 0}
(gdb) p request->handle
$2 = 93825000680144
that we are on normal receiving path.
Quoted 14 lines
Quoted 14 lines
block/nbd-client.c +1
-0@@ -107,6 +107,7 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)qemu_coroutine_yield (); } + s->reply.handle =0 ;nbd_recv_coroutines_enter_all (s); s->read_reply_co =NULL ; }
Signature · 2 lines
Best regards, Vladimir
Re: [Qemu-devel] [PATCH 06/17] block/nbd-client: fix nbd_read_reply_entry
Quoted 4 lines · Vladimir Sementsov-Ogievskiy
Quoted 3 lines · Eric Blake
Quoted 2 lines · Vladimir Sementsov-Ogievskiy
Set reply.handle to 0 on error path to prevent normal path of
nbd_co_receive_reply.
Side note: in general, our server must allow a handle of 0 as valid (as the handle is something chosen by the client); but our particular client always uses non-zero handles (and therefore using 0 as a sentinel for an error path is okay).
Quoted 10 lines
Quoted 9 lines
Quoted 5 lines
Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>--- block/nbd-client.c | 1 + 1 file changed, 1 insertion(+)Can you document a case where not fixing this would be an observable bug (even if it requires using gdb and single-stepping between client and server to make what is otherwise a racy situation easy to see)? I'm trying to figure out if this is 2.10 material.
Quoted 5 lines
it is simple enough:
run qemu-nbd in gdb, set break on
nbd_send_reply, and when it shoot s, next up to "stl_be_p(buf, NBD_REPLY_MAGIC);" and after it do "callstl_be_p(buf, 1000)"
Okay, so you have set up a malicious server intentionally sending bad magic...
Quoted 11 lines
run qemu-io with some read in gdb, set break on
brblock/nbd-client.c:83( it is break; after failed
nbd_receive_replycall)and on
brblock/nbd-client.c:170(it is in
nbd_co_receive_replyafter yield)
Quoted 8 lines
on first break we will be sure that
nbd_receive_replyfailed, on second we will be sure by
(gdb) ps->reply
$1 = {handle = 93825000680144, error = 0}
(gdb) prequest->handle
$2 = 93825000680144that we are on normal receiving path.
...and the client is recognizing that the server sent garbage, but then proceeds to handle the packet anyway. The ideal reaction is that the client should disconnect from the server, rather than handle the packet.
But because it didn't do that, the client is now unable to receive any
future messages from the server. Compare the traces of:
$ ./qemu-io -c 'r 0 1' -f raw nbd://localhost:10809/foo --trace nbd_\*
against a working server:
[email protected]:nbd_opt_go_success Export is good to go [email protected]:nbd_send_request Sending request to server: { .from = 0, .len = 1, .handle = 93860726319200, .flags = 0x0, .type = 0 (read) }
[email protected]:nbd_receive_reply Got reply: { magic = 0x67446698, .error = 0, handle = 93860726319200 }
read 1/1 bytes at offset 0
1 bytes, 1 ops; 0.0003 sec (2.822 KiB/sec and 2890.1734 ops/sec) [email protected]:nbd_send_request Sending request to server: { .from = 0, .len = 0, .handle = 93860726319200, .flags = 0x0, .type = 3 (flush) }
[email protected]:nbd_receive_reply Got reply: { magic = 0x67446698, .error = 0, handle = 93860726319200 } [email protected]:nbd_send_request Sending request to server: { .from = 0, .len = 0, .handle = 93860726319200, .flags = 0x0, .type = 3 (flush) }
[email protected]:nbd_receive_reply Got reply: { magic = 0x67446698, .error = 0, handle = 93860726319200 } [email protected]:nbd_send_request Sending request to server: { .from = 0, .len = 0, .handle = 0, .flags = 0x0, .type = 2 (discard) }
followed by a clean disconnect; vs. the buggy server:
[email protected]:nbd_opt_go_success Export is good to go [email protected]:nbd_send_request Sending request to server: { .from = 0, .len = 1, .handle = 94152262559840, .flags = 0x0, .type = 0 (read) }
[email protected]:nbd_receive_reply Got reply: { magic = 0x1446698, .error = 0, handle = 94152262559840 }
invalid magic (got 0x1446698)
read 1/1 bytes at offset 0
1 bytes, 1 ops; 0:00:12.10 (0.082581 bytes/sec and 0.0826 ops/sec) [email protected]:nbd_send_request Sending request to server: { .from = 0, .len = 0, .handle = 94152262559840, .flags = 0x0, .type = 3 (flush) }
where the client is now hung. Thankfully, the client is blocked in an idle loop (not eating CPU), so I don't know if this counts as the ability for a malicious server to cause a denial of service against qemu as an NBD client (in general, being unable to read further data from the server because client internal state is now botched is not that much different from being unable to read further data from the server because the client hung up on the invalid server), unless there is some way to cause qemu to die from an assertion failure rather than just get stuck.
It also looks like the client acts on at most one bad packet from the server before it gets stuck - but the question is whether a malicious server could, in that one bad packet reply, cause qemu to misbehave in some other way.
I'm adding Prasad, to analyze whether this needs a CVE.
We don't have a good protocol fuzzer to simulate a bad client and/or bad server as the partner over the socket - if you can find any more such interactions where a bad partner can cause a hang or crash, let's get those fixed in 2.10.
Meanwhile, I'll probably include this patch in 2.10 (after first figuring out if it works in isolation or needs other patches from your series).
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [Qemu-block] [PATCH 06/17] block/nbd-client: fix nbd_read_reply_entry
Quoted 29 lines · Eric Blake
Quoted 4 lines · Vladimir Sementsov-Ogievskiy
Quoted 3 lines · Eric Blake
Quoted 2 lines · Vladimir Sementsov-Ogievskiy
Set reply.handle to 0 on error path to prevent normal path of
nbd_co_receive_reply....and the client is recognizing that the server sent garbage, but then proceeds to handle the packet anyway. The ideal reaction is that the client should disconnect from the server, rather than handle the packet.
But because it didn't do that, the client is now unable to receive any
future messages from the server. Compare the traces of:followed by a clean disconnect; vs. the buggy server:
[email protected]:nbd_opt_go_success Export is good to go [email protected]:nbd_send_request Sending request to server: { .from = 0, .len = 1, .handle = 94152262559840, .flags =
0x0, .type = 0 (read) }
[email protected]:nbd_receive_reply Got reply: { magic =0x1446698, .error = 0, handle = 94152262559840 }
invalid magic (got0x1446698)
read 1/1 bytes at offset 0
1 bytes, 1 ops; 0:00:12.10 (0.082581 bytes/sec and 0.0826 ops/sec) [email protected]:nbd_send_request Sending request to server: { .from = 0, .len = 0, .handle = 94152262559840, .flags =0x0, .type = 3 (flush) }
Quoted 8 lines
where the client is now hung. Thankfully, the client is blocked in an idle loop (not eating CPU), so I don't know if this counts as the ability for a malicious server to cause a denial of service against qemu as an NBD client (in general, being unable to read further data from the server because client internal state is now botched is not that much different from being unable to read further data from the server because the client hung up on the invalid server), unless there is some way to cause qemu to die from an assertion failure rather than just get stuck.
With just patch 6/17 applied, the client still hangs, but this time with a different trace:
[email protected]:nbd_opt_go_success Export is good to go [email protected]:nbd_send_request Sending request to server: { .from = 0, .len = 1, .handle = 94716063746144, .flags = 0x0, .type = 0 (read) }
[email protected]:nbd_receive_reply Got reply: { magic = 0x1446698, .error = 0, handle = 94716063746144 }
invalid magic (got 0x1446698)
read failed: Input/output error [email protected]:nbd_send_request Sending request to server: { .from = 0, .len = 0, .handle = 94716063746144, .flags = 0x0, .type = 3 (flush) }
The client still tried to send a flush request to the server, when it should REALLY quit talking to the server at all and just disconnect, because the moment the client recognizes that the server has sent garbage is the moment that the client can no longer assume that the server will react correctly to any further commands.
So I don't think your patch is quite correct, even if you have identified a shortfall in our client code.
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [Qemu-block] [PATCH 06/17] block/nbd-client: fix nbd_read_reply_entry
Quoted 50 lines · Eric Blake
Quoted 26 lines · Eric Blake
Quoted 4 lines · Vladimir Sementsov-Ogievskiy
Quoted 3 lines · Eric Blake
Quoted 2 lines · Vladimir Sementsov-Ogievskiy
Set reply.handle to 0 on error path to prevent normal path of
nbd_co_receive_reply....and the client is recognizing that the server sent garbage, but then proceeds to handle the packet anyway. The ideal reaction is that the client should disconnect from the server, rather than handle the packet.
But because it didn't do that, the client is now unable to receive any
future messages from the server. Compare the traces of:followed by a clean disconnect; vs. the buggy server:
[email protected]:nbd_opt_go_success Export is good to go [email protected]:nbd_send_request Sending request to server: { .from = 0, .len = 1, .handle = 94152262559840, .flags =
0x0, .type = 0 (read) }
[email protected]:nbd_receive_reply Got reply: { magic =0x1446698, .error = 0, handle = 94152262559840 }
invalid magic (got0x1446698)
read 1/1 bytes at offset 0
1 bytes, 1 ops; 0:00:12.10 (0.082581 bytes/sec and 0.0826 ops/sec) [email protected]:nbd_send_request Sending request to server: { .from = 0, .len = 0, .handle = 94152262559840, .flags =0x0, .type = 3 (flush) }Quoted 8 lines
where the client is now hung. Thankfully, the client is blocked in an idle loop (not eating CPU), so I don't know if this counts as the ability for a malicious server to cause a denial of service against qemu as an NBD client (in general, being unable to read further data from the server because client internal state is now botched is not that much different from being unable to read further data from the server because the client hung up on the invalid server), unless there is some way to cause qemu to die from an assertion failure rather than just get stuck.
With just patch 6/17 applied, the client still hangs, but this time with a different trace:
[email protected]:nbd_opt_go_success Export is good to go [email protected]:nbd_send_request Sending request to server: { .from = 0, .len = 1, .handle = 94716063746144, .flags =
0x0, .type = 0 (read) }
[email protected]:nbd_receive_reply Got reply: { magic =0x1446698, .error = 0, handle = 94716063746144 }
invalid magic (got0x1446698)
read failed: Input/output error [email protected]:nbd_send_request Sending request to server: { .from = 0, .len = 0, .handle = 94716063746144, .flags =0x0, .type = 3 (flush) }
Quoted 9 lines
The client still tried to send a flush request to the server, when it should REALLY quit talking to the server at all and just disconnect, because the moment the client recognizes that the server has sent garbage is the moment that the client can no longer assume that the server will react correctly to any further commands.
So I don't think your patch is quite correct, even if you have identified a shortfall in our client code.
Why do you think so? It just does what it states in commit message.
Patch 17 should fix the case (but I doubt that it can be taken separately).
Signature · 2 lines
Best regards, Vladimir
Re: [Qemu-devel] [Qemu-block] [PATCH 06/17] block/nbd-client: fix nbd_read_reply_entry
Quoted 11 lines · Vladimir Sementsov-Ogievskiy
Quoted 11 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines · Vladimir Sementsov-Ogievskiy
Quoted 2 lines
Set reply.handle to 0 on error path to prevent normal path of
nbd_co_receive_reply.The client still tried to send a flush request to the server, when it should REALLY quit talking to the server at all and just disconnect, because the moment the client recognizes that the server has sent garbage is the moment that the client can no longer assume that the server will react correctly to any further commands.
Quoted 4 lines
Quoted 3 lines
So I don't think your patch is quite correct, even if you have identified a shortfall in our client code.
Why do you think so? It just does what it states in commit message.
The commit message doesn't state much on the way of WHY. It it the subsequent discussion that says that the reason WHY we need to fix something is to make the client robustly hang up when it encounters a broken server - but once you couch it in those terms, this patch is NOT making the client hang up gracefully (it is making the client skip ONE invalid reply, but then immediately lets the client send another request and gets stuck waiting for a reply to that second request). A full fix for the issue would make sure the client hangs up as soon as it detects a bogus server.
Quoted 2 lines
Patch 17 should fix the case (but I doubt that it can be taken separately).
It's okay if it takes more than one patch to fix an issue, if the earlier patches document that more work is still needed. Or maybe we can squash the best parts of 17 and this one. I'm still playing with the best way to minimally address the issue for 2.10.
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
PATCH 7/17 block/nbd-client: refactor request send/receive
In reply to Vladimir Sementsov-OgievskiyMove nbd_co_receive_reply and nbd_coroutine_end calls into nbd_co_send_request and rename the latter to just nbd_co_request.
This removes code duplications in nbd_client_co_{pwrite,pread,...} functions. Also this is needed for further refactoring.
--- block/nbd-client.c | 89 ++++++++++++++++++++---------------------------------- 1 file changed, 33 insertions(+), 56 deletions(-)
block/nbd-client.c +33 -56
@@ -112,12 +112,20 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque) s->read_reply_co =NULL ; } -static int nbd_co_send_request (BlockDriverState *bs, -NBDRequest *request, -QEMUIOVector *qiov) +static void nbd_co_receive_reply (NBDClientSession *s, +NBDRequest *request, +NBDReply *reply, +QEMUIOVector *qiov); +static void nbd_coroutine_end (BlockDriverState *bs, +NBDRequest *request); + +static int nbd_co_request (BlockDriverState *bs, +NBDRequest *request, +QEMUIOVector *qiov) {NBDClientSession *s =nbd_get_client_session (bs);int rc, ret, i; +NBDReply reply;qemu_co_mutex_lock (&s->send_mutex);while (s->in_flight ==MAX_NBD_REQUESTS ) {
@@ -141,7 +149,8 @@ static int nbd_co_send_request(BlockDriverState *bs,return -EPIPE ; } -if (qiov) { +if (request->type ==NBD_CMD_WRITE ) { +assert (qiov !=NULL );qio_channel_set_cork (s->ioc, true); rc =nbd_send_request (s->ioc, request);if (rc >=0 ) {
@@ -156,6 +165,21 @@ static int nbd_co_send_request(BlockDriverState *bs, rc =nbd_send_request (s->ioc, request); }qemu_co_mutex_unlock (&s->send_mutex); + +if (rc <0 ) { + goto out; + } + +if (request->type ==NBD_CMD_READ ) { +assert (qiov !=NULL ); + }else { + qiov =NULL ; + } +nbd_co_receive_reply (s, request, &reply, qiov); + rc = -reply.error; + +out: +nbd_coroutine_end (bs, request);return rc; }
@@ -208,26 +232,16 @@ static void nbd_coroutine_end(BlockDriverState *bs,int nbd_client_co_preadv (BlockDriverState *bs,uint64_t offset,uint64_t bytes,QEMUIOVector *qiov,int flags) { -NBDClientSession *client =nbd_get_client_session (bs);NBDRequest request = { .type =NBD_CMD_READ , .from = offset, .len = bytes, }; -NBDReply reply; -int ret;assert (bytes <=NBD_MAX_BUFFER_SIZE );assert (!flags); - ret =nbd_co_send_request (bs, &request,NULL ); -if (ret <0 ) { - reply.error = -ret; - }else { -nbd_co_receive_reply (client, &request, &reply, qiov); - } -nbd_coroutine_end (bs, &request); -return -reply.error; +return nbd_co_request (bs, &request, qiov); } int nbd_client_co_pwritev(BlockDriverState *bs, uint64_t offset,
@@ -239,8 +253,6 @@ int nbd_client_co_pwritev(BlockDriverState *bs, uint64_t offset, .from = offset, .len = bytes, }; -NBDReply reply; -int ret;if (flags &BDRV_REQ_FUA ) {assert (client->info.flags &NBD_FLAG_SEND_FUA );
@@ -249,27 +261,18 @@ int nbd_client_co_pwritev(BlockDriverState *bs, uint64_t offset,assert (bytes <=NBD_MAX_BUFFER_SIZE ); - ret =nbd_co_send_request (bs, &request, qiov); -if (ret <0 ) { - reply.error = -ret; - }else { -nbd_co_receive_reply (client, &request, &reply,NULL ); - } -nbd_coroutine_end (bs, &request); -return -reply.error; +return nbd_co_request (bs, &request, qiov); }int nbd_client_co_pwrite_zeroes (BlockDriverState *bs,int64_t offset,int bytes,BdrvRequestFlags flags) { -int ret;NBDClientSession *client =nbd_get_client_session (bs);NBDRequest request = { .type =NBD_CMD_WRITE_ZEROES , .from = offset, .len = bytes, }; -NBDReply reply;if (!(client->info.flags &NBD_FLAG_SEND_WRITE_ZEROES )) {return -ENOTSUP ;
@@ -283,22 +286,13 @@ int nbd_client_co_pwrite_zeroes(BlockDriverState *bs, int64_t offset, request.flags |=NBD_CMD_FLAG_NO_HOLE ; } - ret =nbd_co_send_request (bs, &request,NULL ); -if (ret <0 ) { - reply.error = -ret; - }else { -nbd_co_receive_reply (client, &request, &reply,NULL ); - } -nbd_coroutine_end (bs, &request); -return -reply.error; +return nbd_co_request (bs, &request,NULL ); }int nbd_client_co_flush (BlockDriverState *bs) {NBDClientSession *client =nbd_get_client_session (bs);NBDRequest request = { .type =NBD_CMD_FLUSH }; -NBDReply reply; -int ret;if (!(client->info.flags &NBD_FLAG_SEND_FLUSH )) {return 0 ;
@@ -307,14 +301,7 @@ int nbd_client_co_flush(BlockDriverState *bs) request.from =0 ; request.len =0 ; - ret =nbd_co_send_request (bs, &request,NULL ); -if (ret <0 ) { - reply.error = -ret; - }else { -nbd_co_receive_reply (client, &request, &reply,NULL ); - } -nbd_coroutine_end (bs, &request); -return -reply.error; +return nbd_co_request (bs, &request,NULL ); }int nbd_client_co_pdiscard (BlockDriverState *bs,int64_t offset,int bytes)
@@ -325,22 +312,12 @@ int nbd_client_co_pdiscard(BlockDriverState *bs, int64_t offset, int bytes) .from = offset, .len = bytes, }; -NBDReply reply; -int ret;if (!(client->info.flags &NBD_FLAG_SEND_TRIM )) {return 0 ; } - ret =nbd_co_send_request (bs, &request,NULL ); -if (ret <0 ) { - reply.error = -ret; - }else { -nbd_co_receive_reply (client, &request, &reply,NULL ); - } -nbd_coroutine_end (bs, &request); -return -reply.error; - +return nbd_co_request (bs, &request,NULL ); }void nbd_client_detach_aio_context (BlockDriverState *bs)
Signature · 1 line
2.11.1
Re: [Qemu-devel] [PATCH 07/17] block/nbd-client: refactor request send/receive
Quoted 10 lines · Vladimir Sementsov-Ogievskiy
Move
nbd_co_receive_replyandnbd_coroutine_endcalls intonbd_co_send_requestand rename the latter to justnbd_co_request.This removes code duplications in nbd_client_co_{pwrite,pread,...} functions. Also this is needed for further refactoring.
Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>--- block/nbd-client.c | 89 ++++++++++++++++++++---------------------------------- 1 file changed, 33 insertions(+), 56 deletions(-)
The diffstat shows this is a nice improvement.
Quoted 4 lines
+++
b/block/nbd-client.cblock/nbd-client.c +0
-0@@ -112,12 +112,20 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque) s->read_reply_co =NULL ; }
block/nbd-client.c static coroutine_fn void nbd_read_reply_entry(void *opaque) · line 120
Quoted 10 lines
block/nbd-client.c +6
-3-static int nbd_co_send_request (BlockDriverState *bs, -NBDRequest *request, -QEMUIOVector *qiov) +static void nbd_co_receive_reply (NBDClientSession *s, +NBDRequest *request, +NBDReply *reply, +QEMUIOVector *qiov); +static void nbd_coroutine_end (BlockDriverState *bs, +NBDRequest *request);
Is it possible to organize the functions in topological order so that we don't need forward declarations of static functions? (If there is mutual recursion, you need the forward declaration; but other than that, I like reading the building blocks first rather than skipping around)
Otherwise,
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH 07/17] block/nbd-client: refactor request send/receive
Quoted 8 lines · Eric Blake
Quoted 6 lines · Vladimir Sementsov-Ogievskiy
Move
nbd_co_receive_replyandnbd_coroutine_endcalls intonbd_co_send_requestand rename the latter to justnbd_co_request.This removes code duplications in nbd_client_co_{pwrite,pread,...} functions. Also this is needed for further refactoring.
Quoted 14 lines
block/nbd-client.c static coroutine_fn void nbd_read_reply_entry(void *opaque) · line 120
Quoted 9 lines
block/nbd-client.c +6
-3-static int nbd_co_send_request (BlockDriverState *bs, -NBDRequest *request, -QEMUIOVector *qiov) +static void nbd_co_receive_reply (NBDClientSession *s, +NBDRequest *request, +NBDReply *reply, +QEMUIOVector *qiov); +static void nbd_coroutine_end (BlockDriverState *bs, +NBDRequest *request);Is it possible to organize the functions in topological order so that we don't need forward declarations of static functions? (If there is mutual recursion, you need the forward declaration; but other than that, I like reading the building blocks first rather than skipping around)
Answering myself: patch 9 inlines nbd_co_receive_reply into its lone caller, eliminating the need for a forward reference, and it is less code churn to have a temporary forward declaration than it is to move the function body up and then back down. (Maybe the commit message could hint that nbd_co_receive_reply will later be inlined)
Quoted 4 lines
Otherwise,
Reviewed-by Eric Blake <[email protected]>
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
PATCH 8/17 block/nbd-client: rename nbd_recv_coroutines_enter_all
In reply to Vladimir Sementsov-OgievskiyRename nbd_recv_coroutines_enter_all to nbd_recv_coroutines_wake_all, as it most probably just add all recv coroutines into co_queue_wakeup, not directly enter them.
--- block/nbd-client.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
block/nbd-client.c +2 -2
@@ -34,7 +34,7 @@#define HANDLE_TO_INDEX (bs, handle) ((handle) ^ ((uint64_t)(intptr_t)bs))#define INDEX_TO_HANDLE (bs, index) ((index) ^ ((uint64_t)(intptr_t)bs)) -static void nbd_recv_coroutines_enter_all (NBDClientSession *s) +static void nbd_recv_coroutines_wake_all (NBDClientSession *s) {int i;
@@ -108,7 +108,7 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque) } s->reply.handle =0 ; -nbd_recv_coroutines_enter_all (s); +nbd_recv_coroutines_wake_all (s); s->read_reply_co =NULL ; }
Signature · 1 line
2.11.1
Re: [Qemu-devel] [PATCH 08/17] block/nbd-client: rename nbd_recv_coroutines_enter_all
Quoted 8 lines · Vladimir Sementsov-Ogievskiy
Rename
nbd_recv_coroutines_enter_alltonbd_recv_coroutines_wake_all, as it most probably just add all recv coroutines intoco_queue_wakeup, not directly enter them.Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>--- block/nbd-client.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Quoted 24 lines
block/nbd-client.c +2
-2@@ -34,7 +34,7 @@#define HANDLE_TO_INDEX (bs, handle) ((handle) ^ ((uint64_t)(intptr_t)bs))#define INDEX_TO_HANDLE (bs, index) ((index) ^ ((uint64_t)(intptr_t)bs)) -static void nbd_recv_coroutines_enter_all (NBDClientSession *s) +static void nbd_recv_coroutines_wake_all (NBDClientSession *s) {int i;@@ -108,7 +108,7 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque) } s->reply.handle =0 ; -nbd_recv_coroutines_enter_all (s); +nbd_recv_coroutines_wake_all (s); s->read_reply_co =NULL ; }
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH 08/17] block/nbd-client: rename nbd_recv_coroutines_enter_all
Quoted 3 lines · Eric Blake
Quoted 2 lines · Vladimir Sementsov-Ogievskiy
Rename
nbd_recv_coroutines_enter_alltonbd_recv_coroutines_wake_all, as it most probably just add all recv coroutines intoco_queue_wakeup,
s/adds/
Quoted 1 line
Quoted 1 line
not directly enter them.
s/not/rather than/
Quoted 8 lines
Quoted 5 lines
Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>--- block/nbd-client.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)Reviewed-by Eric Blake <[email protected]>
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
PATCH 9/17 block/nbd-client: move nbd_co_receive_reply content into nbd_co_request
In reply to Vladimir Sementsov-OgievskiyMove code from nbd_co_receive_reply into nbd_co_request. This simplify things, makes further refactoring possible. Also, a function starting with qemu_coroutine_yield is weird.
--- block/nbd-client.c | 33 ++++++++++----------------------- 1 file changed, 10 insertions(+), 23 deletions(-)
block/nbd-client.c +10 -23
@@ -112,10 +112,6 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque) s->read_reply_co =NULL ; } -static void nbd_co_receive_reply (NBDClientSession *s, -NBDRequest *request, -NBDReply *reply, -QEMUIOVector *qiov);static void nbd_coroutine_end (BlockDriverState *bs,NBDRequest *request);
@@ -175,39 +171,30 @@ static int nbd_co_request(BlockDriverState *bs, }else { qiov =NULL ; } -nbd_co_receive_reply (s, request, &reply, qiov); - rc = -reply.error; - -out: -nbd_coroutine_end (bs, request); -return rc; -} - -static void nbd_co_receive_reply (NBDClientSession *s, -NBDRequest *request, -NBDReply *reply, -QEMUIOVector *qiov) -{ -int ret;/* Wait until we're woken up by nbd_read_reply_entry. */ qemu_coroutine_yield (); - *reply = s->reply; -if (reply->handle != request->handle || + reply = s->reply; +if (reply.handle != request->handle || !s->ioc) { - reply->error =EIO ; + reply.error =EIO ; }else { -if (qiov && reply->error ==0 ) { +if (qiov && reply.error ==0 ) { ret =nbd_rwv (s->ioc, qiov->iov, qiov->niov, request->len, true,NULL );if (ret != request->len) { - reply->error =EIO ; + reply.error =EIO ; } }/* Tell the read handler to read another header. */ s->reply.handle =0 ; } + rc = -reply.error; + +out: +nbd_coroutine_end (bs, request); +return rc; } static void nbd_coroutine_end(BlockDriverState *bs,
Signature · 1 line
2.11.1
Re: [Qemu-devel] [PATCH 09/17] block/nbd-client: move nbd_co_receive_reply content into nbd_co_request
Quoted 1 line · Vladimir Sementsov-Ogievskiy
Move code from
nbd_co_receive_replyintonbd_co_request. This simplify
s/simplify/simplifies/
Quoted 1 line
things, makes further refactoring possible. Also, a function starting
s/makes/and makes/
Quoted 7 lines
with
qemu_coroutine_yieldis weird.Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>--- block/nbd-client.c | 33 ++++++++++----------------------- 1 file changed, 10 insertions(+), 23 deletions(-)
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
PATCH 10/17 block/nbd-client: move nbd_coroutine_end content into nbd_co_request
In reply to Vladimir Sementsov-OgievskiyMove code from nbd_coroutine_end into nbd_co_request. The function nbd_coroutine_end is not needed separately, also it is better to have in_flight-- in nbd_co_request as in_flight++ lives in nbd_co_request too.
--- block/nbd-client.c | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-)
block/nbd-client.c +1 -13
@@ -112,9 +112,6 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque) s->read_reply_co =NULL ; } -static void nbd_coroutine_end (BlockDriverState *bs, -NBDRequest *request); -static int nbd_co_request (BlockDriverState *bs,NBDRequest *request,QEMUIOVector *qiov)
@@ -193,16 +190,6 @@ static int nbd_co_request(BlockDriverState *bs, rc = -reply.error; out: -nbd_coroutine_end (bs, request); -return rc; -} - -static void nbd_coroutine_end (BlockDriverState *bs, -NBDRequest *request) -{ -NBDClientSession *s =nbd_get_client_session (bs); -int i =HANDLE_TO_INDEX (s, request->handle); - s->recv_coroutine[i] =NULL ;/* Kick the read_reply_co to get the next reply. */
@@ -214,6 +201,7 @@ static void nbd_coroutine_end(BlockDriverState *bs, s->in_flight--;qemu_co_queue_next (&s->free_sema);qemu_co_mutex_unlock (&s->send_mutex); +return rc; } int nbd_client_co_preadv(BlockDriverState *bs, uint64_t offset,
Signature · 1 line
2.11.1
Re: [Qemu-devel] [PATCH 10/17] block/nbd-client: move nbd_coroutine_end content into nbd_co_request
Quoted 10 lines · Vladimir Sementsov-Ogievskiy
Move code from
nbd_coroutine_endintonbd_co_request. The functionnbd_coroutine_endis not needed separately, also it is better to have in_flight-- innbd_co_requestas in_flight++ lives innbd_co_requesttoo.Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>--- block/nbd-client.c | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-)
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
PATCH 11/17 block/nbd-client: fix nbd_co_request: set s->reply.handle to 0 on error
In reply to Vladimir Sementsov-OgievskiyWe set s->reply.handle to 0 on one error path and don't set on another. For consistancy and to avoid assert in nbd_read_reply_entry let's set s->reply.handle to 0 in case of wrong handle too.
--- block/nbd-client.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
block/nbd-client.c +3 -3
@@ -183,13 +183,13 @@ static int nbd_co_request(BlockDriverState *bs, reply.error =EIO ; } } - -/* Tell the read handler to read another header. */ - s->reply.handle =0 ; } rc = -reply.error; out: +/* Tell the read handler to read another header. */ + s->reply.handle =0 ; + s->recv_coroutine[i] =NULL ;/* Kick the read_reply_co to get the next reply. */
Signature · 1 line
2.11.1
Re: [Qemu-devel] [PATCH 11/17] block/nbd-client: fix nbd_co_request: set s->reply.handle to 0 on error
Quoted 8 lines · Vladimir Sementsov-Ogievskiy
We set
s->reply.handleto 0 on one error path and don't set on another. For consistancy and to avoid assert innbd_read_reply_entrylet's sets->reply.handleto 0 in case of wrong handle too.Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>--- block/nbd-client.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
Can this assertion be triggered now (presumably, with a broken server)? I'm trying to figure out if this is 2.10 material.
[Urgh. If a broken server is able to cause an assertion failure that causes a client to abort on an assertion failure, that probably deserves a CVE]
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH 11/17] block/nbd-client: fix nbd_co_request: set s->reply.handle to 0 on error
Quoted 9 lines · Eric Blake
Quoted 8 lines · Vladimir Sementsov-Ogievskiy
We set
s->reply.handleto 0 on one error path and don't set on another. For consistancy and to avoid assert innbd_read_reply_entrylet's sets->reply.handleto 0 in case of wrong handle too.Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>--- block/nbd-client.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
Quoted 7 lines
Can this assertion be triggered now (presumably, with a broken server)? I'm trying to figure out if this is 2.10 material.
[Urgh. If a broken server is able to cause an assertion failure that causes a client to abort on an assertion failure, that probably deserves a CVE]
Hmm looks like I've mistaken, if handle is wrong than read_reply_co should be already finished, so it's impossible
Signature · 2 lines
Best regards, Vladimir
PATCH 12/17 block/nbd-client: refactor nbd_co_request
In reply to Vladimir Sementsov-OgievskiyReduce nesting, get rid of extra variable.
--- block/nbd-client.c | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-)
block/nbd-client.c +12 -13
@@ -118,7 +118,6 @@ static int nbd_co_request(BlockDriverState *bs, {NBDClientSession *s =nbd_get_client_session (bs);int rc, ret, i; -NBDReply reply;qemu_co_mutex_lock (&s->send_mutex);while (s->in_flight ==MAX_NBD_REQUESTS ) {
@@ -171,20 +170,20 @@ static int nbd_co_request(BlockDriverState *bs,/* Wait until we're woken up by nbd_read_reply_entry. */ qemu_coroutine_yield (); - reply = s->reply; -if (reply.handle != request->handle || - !s->ioc) { - reply.error =EIO ; - }else { -if (qiov && reply.error ==0 ) { - ret =nbd_rwv (s->ioc, qiov->iov, qiov->niov, request->len, true, -NULL ); -if (ret != request->len) { - reply.error =EIO ; - } +if (s->reply.handle != request->handle || !s->ioc) { + rc = -EIO ; + goto out; + } + +if (qiov && s->reply.error ==0 ) { + ret =nbd_rwv (s->ioc, qiov->iov, qiov->niov, request->len, true,NULL ); +if (ret != request->len) { + rc = -EIO ; + goto out; } } - rc = -reply.error; + + rc = -s->reply.error; out:/* Tell the read handler to read another header. */
Signature · 1 line
2.11.1
PATCH 13/17 block/nbd-client: refactor NBDClientSession.recv_coroutine
In reply to Vladimir Sementsov-OgievskiyMove from recv_coroutine[i] to requests[i].co. This is needed for further refactoring, new fields will be added to created structure.
--- block/nbd-client.h | 4 +++- block/nbd-client.c | 20 ++++++++++---------- 2 files changed, 13 insertions(+), 11 deletions(-)
block/nbd-client.h +3 -1
@@ -27,7 +27,9 @@ typedef struct NBDClientSession {Coroutine *read_reply_co;int in_flight; -Coroutine *recv_coroutine[MAX_NBD_REQUESTS ]; +struct { +Coroutine *co; + } requests[MAX_NBD_REQUESTS ];NBDReply reply; } NBDClientSession;
block/nbd-client.c +10 -10
@@ -39,8 +39,8 @@ static void nbd_recv_coroutines_wake_all(NBDClientSession *s)int i;for (i =0 ; i <MAX_NBD_REQUESTS ; i++) { -if (s->recv_coroutine[i]) { -aio_co_wake (s->recv_coroutine[i]); +if (s->requests[i].co) { +aio_co_wake (s->requests[i].co); } } }
@@ -88,22 +88,22 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)* one coroutine is called until the reply finishes. */ i =HANDLE_TO_INDEX (s, s->reply.handle); -if (i >=MAX_NBD_REQUESTS || !s->recv_coroutine[i]) { +if (i >=MAX_NBD_REQUESTS || !s->requests[i].co) {break ; } -/* We're woken up by the recv_coroutine itself. Note that there +/* We're woken up by the receiving coroutine itself. Note that there * is no race between yielding and reentering read_reply_co. This * is because: * -* - if recv_coroutine[i] runs on the same AioContext, it is only +* - if requests[i].co runs on the same AioContext, it is only * entered after we yield * -* - if recv_coroutine[i] runs on a different AioContext, reentering +* - if requests[i].co runs on a different AioContext, reentering * read_reply_co happens through a bottom half, which can only * run after we yield. */ -aio_co_wake (s->recv_coroutine[i]); +aio_co_wake (s->requests[i].co);qemu_coroutine_yield (); }
@@ -126,8 +126,8 @@ static int nbd_co_request(BlockDriverState *bs, s->in_flight++;for (i =0 ; i <MAX_NBD_REQUESTS ; i++) { -if (s->recv_coroutine[i] ==NULL ) { - s->recv_coroutine[i] =qemu_coroutine_self (); +if (s->requests[i].co ==NULL ) { + s->requests[i].co =qemu_coroutine_self ();break ; } }
@@ -189,7 +189,7 @@ out:/* Tell the read handler to read another header. */ s->reply.handle =0 ; - s->recv_coroutine[i] =NULL ; + s->requests[i].co =NULL ;/* Kick the read_reply_co to get the next reply. */ if (s->read_reply_co) {
Signature · 1 line
2.11.1
PATCH 14/17 block/nbd-client: exit reply-reading coroutine on incorrect handle
In reply to Vladimir Sementsov-OgievskiyCheck reply-handle == request-handle in the same place, where recv coroutine number is calculated from reply->handle and it's correctness checked - in nbd_read_reply_entry.
Also finish nbd_read_reply_entry in case of reply-handle != request-handle in the same way as in case of incorrect reply-handle.
--- block/nbd-client.h | 1 + block/nbd-client.c | 9 +++++++-- 2 files changed, 8 insertions(+), 2 deletions(-)
block/nbd-client.h +1 -0
@@ -29,6 +29,7 @@ typedef struct NBDClientSession {struct {Coroutine *co; +NBDRequest *request; } requests[MAX_NBD_REQUESTS ];NBDReply reply; } NBDClientSession;
block/nbd-client.c +7 -2
@@ -88,7 +88,9 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)* one coroutine is called until the reply finishes. */ i =HANDLE_TO_INDEX (s, s->reply.handle); -if (i >=MAX_NBD_REQUESTS || !s->requests[i].co) { +if (i >=MAX_NBD_REQUESTS || !s->requests[i].co || + s->reply.handle != s->requests[i].request->handle) + {break ; }
@@ -135,6 +137,7 @@ static int nbd_co_request(BlockDriverState *bs,g_assert (qemu_in_coroutine ());assert (i <MAX_NBD_REQUESTS ); request->handle =INDEX_TO_HANDLE (s, i); + s->requests[i].request = request;if (!s->ioc) {qemu_co_mutex_unlock (&s->send_mutex);
@@ -170,11 +173,13 @@ static int nbd_co_request(BlockDriverState *bs,/* Wait until we're woken up by nbd_read_reply_entry. */ qemu_coroutine_yield (); -if (s->reply.handle != request->handle || !s->ioc) { +if (!s->ioc || s->reply.handle ==0 ) { rc = -EIO ; goto out; } +assert (s->reply.handle == request->handle); +if (qiov && s->reply.error ==0 ) { ret =nbd_rwv (s->ioc, qiov->iov, qiov->niov, request->len, true,NULL );if (ret != request->len) {
Signature · 1 line
2.11.1
PATCH 15/17 block/nbd-client: refactor reading reply
In reply to Vladimir Sementsov-OgievskiyRead the whole reply in one place - in nbd_read_reply_entry.
--- block/nbd-client.h | 1 + block/nbd-client.c | 27 +++++++++++++-------------- 2 files changed, 14 insertions(+), 14 deletions(-)
block/nbd-client.h +1 -0
@@ -30,6 +30,7 @@ typedef struct NBDClientSession {struct {Coroutine *co;NBDRequest *request; +QEMUIOVector *qiov; } requests[MAX_NBD_REQUESTS ];NBDReply reply; } NBDClientSession;
block/nbd-client.c +13 -14
@@ -94,6 +94,18 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)break ; } +if (s->reply.error ==0 && + s->requests[i].request->type ==NBD_CMD_READ ) + { +assert (s->requests[i].qiov !=NULL ); + ret =nbd_rwv (s->ioc, s->requests[i].qiov->iov, + s->requests[i].qiov->niov, + s->requests[i].request->len, true,NULL ); +if (ret != s->requests[i].request->len) { +break ; + } + } +/* We're woken up by the receiving coroutine itself. Note that there * is no race between yielding and reentering read_reply_co. This * is because:
@@ -138,6 +150,7 @@ static int nbd_co_request(BlockDriverState *bs,assert (i <MAX_NBD_REQUESTS ); request->handle =INDEX_TO_HANDLE (s, i); s->requests[i].request = request; + s->requests[i].qiov = qiov;if (!s->ioc) {qemu_co_mutex_unlock (&s->send_mutex);
@@ -165,12 +178,6 @@ static int nbd_co_request(BlockDriverState *bs, goto out; } -if (request->type ==NBD_CMD_READ ) { -assert (qiov !=NULL ); - }else { - qiov =NULL ; - } -/* Wait until we're woken up by nbd_read_reply_entry. */ qemu_coroutine_yield ();if (!s->ioc || s->reply.handle ==0 ) {
@@ -180,14 +187,6 @@ static int nbd_co_request(BlockDriverState *bs,assert (s->reply.handle == request->handle); -if (qiov && s->reply.error ==0 ) { - ret =nbd_rwv (s->ioc, qiov->iov, qiov->niov, request->len, true,NULL ); -if (ret != request->len) { - rc = -EIO ; - goto out; - } - } - rc = -s->reply.error; out:
Signature · 1 line
2.11.1
PATCH 16/17 block/nbd-client: drop reply field from NBDClientSession
In reply to Vladimir Sementsov-OgievskiyDrop 'reply' from NBDClientSession. It's usage is not very transparent:
1. it is used to deliver error to receiving coroutine, and receiving coroutine must save or handle it somehow and then zero out it in NBDClientSession.
2. it is used to inform receiving coroutines that nbd_read_reply_entry is out for some reason (error or disconnect)
To simplify this scheme:
- drop NBDClientSession.reply
- introduce NBDClientSession.requests[...].ret for (1)
- introduce NBDClientSession.eio_to_all for (2)
--- block/nbd-client.h | 3 ++- block/nbd-client.c | 25 ++++++++++--------------- 2 files changed, 12 insertions(+), 16 deletions(-)
block/nbd-client.h +2 -1
@@ -31,8 +31,9 @@ typedef struct NBDClientSession {Coroutine *co;NBDRequest *request;QEMUIOVector *qiov; +int ret; } requests[MAX_NBD_REQUESTS ]; -NBDReply reply; +bool eio_to_all; } NBDClientSession;NBDClientSession *nbd_get_client_session (BlockDriverState *bs);
block/nbd-client.c +10 -15
@@ -72,10 +72,10 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)uint64_t i;int ret;Error *local_err =NULL ; +NBDReply reply;for (;;) { -assert (s->reply.handle ==0 ); - ret =nbd_receive_reply (s->ioc, &s->reply, &local_err); + ret =nbd_receive_reply (s->ioc, &reply, &local_err);if (ret <0 ) {error_report_err (local_err); }
@@ -87,16 +87,14 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)* handler acts as a synchronization point and ensures that only * one coroutine is called until the reply finishes. */ - i =HANDLE_TO_INDEX (s, s->reply.handle); + i =HANDLE_TO_INDEX (s, reply.handle);if (i >=MAX_NBD_REQUESTS || !s->requests[i].co || - s->reply.handle != s->requests[i].request->handle) + reply.handle != s->requests[i].request->handle) {break ; } -if (s->reply.error ==0 && - s->requests[i].request->type ==NBD_CMD_READ ) - { +if (reply.error ==0 && s->requests[i].request->type ==NBD_CMD_READ ) {assert (s->requests[i].qiov !=NULL ); ret = nbd_rwv(s->ioc, s->requests[i].qiov->iov, s->requests[i].qiov->niov,
@@ -106,6 +104,8 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque) } } + s->requests[i].ret = -reply.error; +/* We're woken up by the receiving coroutine itself. Note that there * is no race between yielding and reentering read_reply_co. This * is because:
@@ -121,7 +121,7 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)qemu_coroutine_yield (); } - s->reply.handle =0 ; + s->eio_to_all = true;nbd_recv_coroutines_wake_all (s); s->read_reply_co =NULL ; }
@@ -180,19 +180,14 @@ static int nbd_co_request(BlockDriverState *bs,/* Wait until we're woken up by nbd_read_reply_entry. */ qemu_coroutine_yield (); -if (!s->ioc || s->reply.handle ==0 ) { +if (!s->ioc || s->eio_to_all) { rc = -EIO ; goto out; } -assert (s->reply.handle == request->handle); - - rc = -s->reply.error; + rc = s->requests[i].ret; out: -/* Tell the read handler to read another header. */ - s->reply.handle =0 ; - s->requests[i].co =NULL ;/* Kick the read_reply_co to get the next reply. */
Signature · 1 line
2.11.1
PATCH 17/17 block/nbd-client: always return EIO on and after the first io channel error
In reply to Vladimir Sementsov-OgievskiyDo not communicate after the first error to avoid communicating throught broken channel. The only exclusion is try to send NBD_CMD_DISC anyway on in nbd_client_close.
--- block/nbd-client.c | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-)
block/nbd-client.c +20 -6
@@ -49,6 +49,8 @@ static void nbd_teardown_connection(BlockDriverState *bs) {NBDClientSession *client =nbd_get_client_session (bs); + client->eio_to_all = true; +if (!client->ioc) {/* Already closed */ return ; }
@@ -75,11 +77,15 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)NBDReply reply;for (;;) { +if (s->eio_to_all) { +break ; + } + ret =nbd_receive_reply (s->ioc, &reply, &local_err);if (ret <0 ) {error_report_err (local_err); } -if (ret <=0 ) { +if (ret <=0 || s->eio_to_all) {break ; }
@@ -99,7 +105,7 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque) ret =nbd_rwv (s->ioc, s->requests[i].qiov->iov, s->requests[i].qiov->niov, s->requests[i].request->len, true,NULL ); -if (ret != s->requests[i].request->len) { +if (ret != s->requests[i].request->len || s->eio_to_all) {break ; } }
@@ -133,6 +139,10 @@ static int nbd_co_request(BlockDriverState *bs,NBDClientSession *s =nbd_get_client_session (bs);int rc, ret, i; +if (s->eio_to_all) { +return -EIO ; + } +qemu_co_mutex_lock (&s->send_mutex);while (s->in_flight ==MAX_NBD_REQUESTS ) {qemu_co_queue_wait (&s->free_sema, &s->send_mutex);
@@ -152,16 +162,16 @@ static int nbd_co_request(BlockDriverState *bs, s->requests[i].request = request; s->requests[i].qiov = qiov; -if (!s->ioc) { +if (s->eio_to_all) {qemu_co_mutex_unlock (&s->send_mutex); -return -EPIPE ; +return -EIO ; }if (request->type ==NBD_CMD_WRITE ) {assert (qiov !=NULL );qio_channel_set_cork (s->ioc, true); rc =nbd_send_request (s->ioc, request); -if (rc >=0 ) { +if (rc ==0 && !s->eio_to_all) { ret =nbd_rwv (s->ioc, qiov->iov, qiov->niov, request->len, false,NULL );if (ret != request->len) {
@@ -174,13 +184,16 @@ static int nbd_co_request(BlockDriverState *bs, }qemu_co_mutex_unlock (&s->send_mutex); +if (s->eio_to_all) { + rc = -EIO ; + }if (rc <0 ) { goto out; }/* Wait until we're woken up by nbd_read_reply_entry. */ qemu_coroutine_yield (); -if (!s->ioc || s->eio_to_all) { +if (s->eio_to_all) { rc = -EIO ; goto out; }
@@ -335,6 +348,7 @@ int nbd_client_init(BlockDriverState *bs,logout ("session init %s\n" , export);qio_channel_set_blocking (QIO_CHANNEL (sioc), true,NULL ); + client->eio_to_all = false; client->info.request_sizes = true; ret = nbd_receive_negotiate(QIO_CHANNEL(sioc), export, tlscreds, hostname,
Signature · 1 line
2.11.1
Quoted 3 lines · Vladimir Sementsov-Ogievskiy
A bit more refactoring and fixing before
BLOCK_STATUSseries. I've tried to make individual patches simple enough, so there are a lot of them.
Is your BLOCK_STATUS series something that is in good enough shape to post a preliminary version of it (the version you posted back in February is now horribly out-of-date, with all the good cleanups you have been doing in the meantime). I want to get a running start at reviewing what I can to make sure we get improved NBD functionality into 2.11.
Also, please feel free to offer your Reviewed-by on other patches (whether NBD-related or not). Speaking as the NBD maintainer, I welcome any help I can get. And from personal experience, reviews tend to be one of the largest bottlenecks in open source software - if you are writing patches but not offering reviews, then you are adding to the bottleneck so reviewers tend to set your patches aside for when they have more time; while if you are actively offering reviews, then it is obvious that you care about the project and your patch contributions tend to have an easier time getting in. My personal rule of thumb is to try and review at least 2 other patches for every one that I send, although that is a rather ambitious goal and there's nothing wrong if you can't commit to theh same level of effort.
Quoted 14 lines
Vladimir Sementsov-Ogievskiy (17):
nbd/client: fixnbd_opt_go
nbd/client: refactornbd_read_eof
nbd/client: refactornbd_receive_reply
nbd/client: fixnbd_send_requestto return intblock/nbd-client: get rid ofssize_t
block/nbd-client: fixnbd_read_reply_entryblock/nbd-client: refactor request send/receiveblock/nbd-client: renamenbd_recv_coroutines_enter_allblock/nbd-client: movenbd_co_receive_replycontent into
nbd_co_request
block/nbd-client: movenbd_coroutine_endcontent intonbd_co_requestblock/nbd-client: fixnbd_co_request: sets->reply.handleto 0 on
Quoted 8 lines
error
block/nbd-client: refactornbd_co_requestblock/nbd-client: refactor NBDClientSession.recv_coroutineblock/nbd-client: exit reply-reading coroutine on incorrect handleblock/nbd-client: refactor reading reply
block/nbd-client: drop reply field from NBDClientSessionblock/nbd-client: always return EIO on and after the first io channel
error
Of course, parts of this will need rebasing based on what finally landed in 2.10, but I can start reviewing what I can for this round.
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Quoted 10 lines · Eric Blake
Quoted 3 lines · Vladimir Sementsov-Ogievskiy
A bit more refactoring and fixing before
BLOCK_STATUSseries. I've tried to make individual patches simple enough, so there are a lot of them.Is your
BLOCK_STATUSseries something that is in good enough shape to post a preliminary version of it (the version you posted back in February is now horribly out-of-date, with all the good cleanups you have been doing in the meantime). I want to get a running start at reviewing what I can to make sure we get improved NBD functionality into 2.11.
Every time I want to produce a new version of BLOCK_STATUS, I stumble on something and new 10-20 patches refactoring series appears)
Quoted 13 lines
Also, please feel free to offer your Reviewed-by on other patches (whether NBD-related or not). Speaking as the NBD maintainer, I welcome any help I can get. And from personal experience, reviews tend to be one of the largest bottlenecks in open source software - if you are writing patches but not offering reviews, then you are adding to the bottleneck so reviewers tend to set your patches aside for when they have more time; while if you are actively offering reviews, then it is obvious that you care about the project and your patch contributions tend to have an easier time getting in. My personal rule of thumb is to try and review at least 2 other patches for every one that I send, although that is a rather ambitious goal and there's nothing wrong if you can't commit to theh same level of effort.
Thanks, I get the point, I'll try to do better.
Quoted 25 lines
Quoted 14 lines
Vladimir Sementsov-Ogievskiy (17):
nbd/client: fixnbd_opt_go
nbd/client: refactornbd_read_eof
nbd/client: refactornbd_receive_reply
nbd/client: fixnbd_send_requestto return intblock/nbd-client: get rid ofssize_t
block/nbd-client: fixnbd_read_reply_entryblock/nbd-client: refactor request send/receiveblock/nbd-client: renamenbd_recv_coroutines_enter_allblock/nbd-client: movenbd_co_receive_replycontent into
nbd_co_request
block/nbd-client: movenbd_coroutine_endcontent intonbd_co_requestblock/nbd-client: fixnbd_co_request: sets->reply.handleto 0 onQuoted 8 lines
error
block/nbd-client: refactornbd_co_requestblock/nbd-client: refactor NBDClientSession.recv_coroutineblock/nbd-client: exit reply-reading coroutine on incorrect handleblock/nbd-client: refactor reading replyblock/nbd-client: drop reply field from NBDClientSessionblock/nbd-client: always return EIO on and after the first io channel
errorOf course, parts of this will need rebasing based on what finally landed in 2.10, but I can start reviewing what I can for this round.
Signature · 2 lines
Best regards, Vladimir
Quoted 17 lines · Vladimir Sementsov-Ogievskiy
A bit more refactoring and fixing before
BLOCK_STATUSseries. I've tried to make individual patches simple enough, so there are a lot of them.Vladimir Sementsov-Ogievskiy (17):
nbd/client: fixnbd_opt_go
nbd/client: refactornbd_read_eof
nbd/client: refactornbd_receive_reply
nbd/client: fixnbd_send_requestto return intblock/nbd-client: get rid ofssize_t
block/nbd-client: fixnbd_read_reply_entryblock/nbd-client: refactor request send/receiveblock/nbd-client: renamenbd_recv_coroutines_enter_allblock/nbd-client: movenbd_co_receive_replycontent into
nbd_co_request
block/nbd-client: movenbd_coroutine_endcontent intonbd_co_requestblock/nbd-client: fixnbd_co_request: sets->reply.handleto 0 on
Quoted 8 lines
error
block/nbd-client: refactornbd_co_requestblock/nbd-client: refactor NBDClientSession.recv_coroutineblock/nbd-client: exit reply-reading coroutine on incorrect handleblock/nbd-client: refactor reading reply
block/nbd-client: drop reply field from NBDClientSessionblock/nbd-client: always return EIO on and after the first io channel
error
I've pushed 1-5 and 7-10 onto my NBD staging branch for 2.11:
git://repo.or.cz/qemu/ericb.git nbd
with a couple of changes squashed in as mentioned in individual patches; please double-check that it looks okay. If so, then I will use that branch as the starting point for all NBD commits destined for 2.11, sending a pull request once the tree opens.
Patches 6 and 11 are somewhat subsumed by the work that went into 2.10, and the remaining patches are starting to cause enough conflicts that I'd prefer you complete the rebase of patches 12-17 and post a v2 on top of my staging branch.
I'm also hoping that Stefan will rebase a v2 of his "nbd-client: enter read_reply_co during init to avoid crash" on top of your preliminary work, since Paolo had a good suggestion on improving the semantics in the review of v1.
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Quoted 27 lines · Eric Blake
Quoted 17 lines · Vladimir Sementsov-Ogievskiy
A bit more refactoring and fixing before
BLOCK_STATUSseries. I've tried to make individual patches simple enough, so there are a lot of them.Vladimir Sementsov-Ogievskiy (17):
nbd/client: fixnbd_opt_go
nbd/client: refactornbd_read_eof
nbd/client: refactornbd_receive_reply
nbd/client: fixnbd_send_requestto return intblock/nbd-client: get rid ofssize_t
block/nbd-client: fixnbd_read_reply_entryblock/nbd-client: refactor request send/receiveblock/nbd-client: renamenbd_recv_coroutines_enter_allblock/nbd-client: movenbd_co_receive_replycontent into
nbd_co_request
block/nbd-client: movenbd_coroutine_endcontent intonbd_co_requestblock/nbd-client: fixnbd_co_request: sets->reply.handleto 0 onQuoted 8 lines
error
block/nbd-client: refactornbd_co_requestblock/nbd-client: refactor NBDClientSession.recv_coroutineblock/nbd-client: exit reply-reading coroutine on incorrect handleblock/nbd-client: refactor reading reply
block/nbd-client: drop reply field from NBDClientSessionblock/nbd-client: always return EIO on and after the first io channel
error
Quoted 3 lines
I've pushed 1-5 and 7-10 onto my NBD staging branch for 2.11:
git://repo.or.cz/qemu/ericb.gitnbd
Correction - I've decided to take Stefan's patches first (since his patch 1/3 was a bit more elegant at doing the same thing as your patch 10); that caused rebase conflicts for your patch 7 (which I simplified by creating nbd_co_request as a wrapper rather than trying to inline its parts), and my change to your patch 7 obsoletes the need for 9 or 10. I also placed your patch 8 before your patch 7 (if you don't like what I changed on patch 7, it is now last in my NBD staging area, so I can more easily drop it from my tree if you'd prefer to respin it differently).
Quoted 9 lines
with a couple of changes squashed in as mentioned in individual patches; please double-check that it looks okay. If so, then I will use that branch as the starting point for all NBD commits destined for 2.11, sending a pull request once the tree opens.
Patches 6 and 11 are somewhat subsumed by the work that went into 2.10, and the remaining patches are starting to cause enough conflicts that I'd prefer you complete the rebase of patches 12-17 and post a v2 on top of my staging branch.
These statements still hold; you'll need to rebase the rest of your series on top of my tree.
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org