Re: [Qemu-devel] [PATCH 02/17] nbd/client: refactor nbd_read_eof
On 08/07/2017 07:05 AM, Vladimir Sementsov-Ogievskiy wrote: > 07.08.2017 14:42, Eric Blake wrote: >> On 08/04/2017 10:14 AM, Vladimir Sementsov-Ogievskiy wrote: >>> Refactor 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. >>> >>> Signed-off-by: Vladimir Sementsov-Ogievskiy >>> --- >>> + * 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 Or even better as 'negative errno on failure'. Here's what I'm proposing to squash in, at which point you have: Reviewed-by: Eric Blake diff --git i/nbd/nbd-internal.h w/nbd/nbd-internal.h index 3fb0b6098a..03549e3f39 100644 --- i/nbd/nbd-internal.h +++ w/nbd/nbd-internal.h @@ -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) { >>> + >>> +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). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH 02/17] nbd/client: refactor nbd_read_eof
07.08.2017 14:42, Eric Blake wrote: On 08/04/2017 10:14 AM, Vladimir Sementsov-Ogievskiy wrote: Refactor 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. Signed-off-by: Vladimir Sementsov-Ogievskiy --- nbd/nbd-internal.h | 34 +- nbd/client.c | 5 - tests/qemu-iotests/083.out | 4 ++-- 3 files changed, 27 insertions(+), 16 deletions(-) diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h index 396ddb4d3e..3fb0b6098a 100644 --- a/nbd/nbd-internal.h +++ b/nbd/nbd-internal.h @@ -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 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 */ -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); Effectively the same as assert(size != 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 + +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. +} + +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"); 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. } diff --git a/nbd/client.c b/nbd/client.c index f1c16b588f..4556056daa 100644 --- a/nbd/client.c +++ b/nbd/client.c @@ -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) diff --git a/tests/qemu-iotests/083.out b/tests/qemu-iotests/083.out index a24c6bfece..d3bea1b2f5 100644 --- a/tests/qemu-iotests/083.out +++ b/tests/qemu-iotests/083.out @@ -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? 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. -- Best regards, Vladimir
Re: [Qemu-devel] [PATCH 02/17] nbd/client: refactor nbd_read_eof
On 08/04/2017 10:14 AM, Vladimir Sementsov-Ogievskiy wrote: > Refactor 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. > > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > nbd/nbd-internal.h | 34 +- > nbd/client.c | 5 - > tests/qemu-iotests/083.out | 4 ++-- > 3 files changed, 27 insertions(+), 16 deletions(-) > > diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h > index 396ddb4d3e..3fb0b6098a 100644 > --- a/nbd/nbd-internal.h > +++ b/nbd/nbd-internal.h > @@ -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 In general, mixing negative errno value and generic < 0 in the same function is most likely ambiguous. > */ > -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); Effectively the same as assert(size != 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 (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. > +} > + > +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"); 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? > } > diff --git a/nbd/client.c b/nbd/client.c > index f1c16b588f..4556056daa 100644 > --- a/nbd/client.c > +++ b/nbd/client.c > @@ -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) > diff --git a/tests/qemu-iotests/083.out b/tests/qemu-iotests/083.out > index a24c6bfece..d3bea1b2f5 100644 > --- a/tests/qemu-iotests/083.out > +++ b/tests/qemu-iotests/083.out > @@ -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? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature