Re: [Qemu-block] [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-block] [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-block] [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
