Re: [Qemu-devel] [PATCH 06/18] nbd/client: refactor drop_sync

[Paolo Bonzini]

On 08/02/2017 08:55, Vladimir Sementsov-Ogievskiy wrote:
> 07.02.2017 02:19, Eric Blake wrote:
>> On 02/03/2017 09:47 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> Return 0 on success to simplify success checking.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>>> ---
>>>  nbd/client.c | 35 +++
>>>  1 file changed, 19 insertions(+), 16 deletions(-)
>> I'm not sure that this simplifies anything.  You have a net addition in
>> lines of code, so unless some later patch is improved because of this,
>> I'm inclined to say this is needless churn.
>>
> 
> I just dislike duplicating information like "drop_sync(ioc, 124) !=
> 124". In the code there is no place where positive and not equal to size
> return value actually handled. But it is not so important, if you are
> against i'll drop this, no problem.

I think I agree with Vladimir.

> One note: I don't have good understanding of the following: actually
> read can return positive value < queried size, which means that we
> should read again. But it is not handled in the code (handled, but just
> as an error), except drop_sync.. (With drop_sync it is side effect of
> using limited buffer size, yes?). Is it all ok?

It is handled in nbd_wr_syncv.

Paolo

Re: [Qemu-devel] [PATCH 06/18] nbd/client: refactor drop_sync

[Vladimir Sementsov-Ogievskiy]

07.02.2017 02:19, Eric Blake wrote:

On 02/03/2017 09:47 AM, Vladimir Sementsov-Ogievskiy wrote:

Return 0 on success to simplify success checking.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  nbd/client.c | 35 +++
  1 file changed, 19 insertions(+), 16 deletions(-)

I'm not sure that this simplifies anything.  You have a net addition in
lines of code, so unless some later patch is improved because of this,
I'm inclined to say this is needless churn.



I just dislike duplicating information like "drop_sync(ioc, 124) != 
124". In the code there is no place where positive and not equal to size 
return value actually handled. But it is not so important, if you are 
against i'll drop this, no problem.


One note: I don't have good understanding of the following: actually 
read can return positive value < queried size, which means that we 
should read again. But it is not handled in the code (handled, but just 
as an error), except drop_sync.. (With drop_sync it is side effect of 
using limited buffer size, yes?). Is it all ok?



--
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 06/18] nbd/client: refactor drop_sync

[Eric Blake]

On 02/03/2017 09:47 AM, Vladimir Sementsov-Ogievskiy wrote:
> Return 0 on success to simplify success checking.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  nbd/client.c | 35 +++
>  1 file changed, 19 insertions(+), 16 deletions(-)

I'm not sure that this simplifies anything.  You have a net addition in
lines of code, so unless some later patch is improved because of this,
I'm inclined to say this is needless churn.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PATCH 06/18] nbd/client: refactor drop_sync

[Vladimir Sementsov-Ogievskiy]

Return 0 on success to simplify success checking.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 nbd/client.c | 35 +++
 1 file changed, 19 insertions(+), 16 deletions(-)

diff --git a/nbd/client.c b/nbd/client.c
index 351731bc63..1c274f3012 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -86,31 +86,34 @@ static QTAILQ_HEAD(, NBDExport) exports = 
QTAILQ_HEAD_INITIALIZER(exports);
 
 */
 
-/* Discard length bytes from channel.  Return -errno on failure, or
- * the amount of bytes consumed. */
-static ssize_t drop_sync(QIOChannel *ioc, size_t size)
+/* Discard length bytes from channel.
+ * Return 0 on success and -errno on fail.
+ */
+static int drop_sync(QIOChannel *ioc, size_t size)
 {
-ssize_t ret = 0;
+ssize_t ret;
 char small[1024];
 char *buffer;
 
 buffer = sizeof(small) > size ? small : g_malloc(MIN(65536, size));
 while (size > 0) {
-ssize_t count = read_sync(ioc, buffer, MIN(65536, size));
-
-if (count <= 0) {
-goto cleanup;
+ret = read_sync(ioc, buffer, MIN(65536, size));
+if (ret == 0) {
+ret = -EIO;
+}
+if (ret < 0) {
+break;
 }
-assert(count <= size);
-size -= count;
-ret += count;
+
+assert(ret <= size);
+size -= ret;
 }
 
- cleanup:
 if (buffer != small) {
 g_free(buffer);
 }
-return ret;
+
+return ret < 0 ? ret : 0;
 }
 
 /* Send an option request.
@@ -334,7 +337,7 @@ static int nbd_receive_list(QIOChannel *ioc, const char 
*want, bool *match,
 return -1;
 }
 if (namelen != strlen(want)) {
-if (drop_sync(ioc, len) != len) {
+if (drop_sync(ioc, len) < 0) {
 error_setg(errp, "failed to skip export name with wrong length");
 nbd_send_opt_abort(ioc);
 return -1;
@@ -350,7 +353,7 @@ static int nbd_receive_list(QIOChannel *ioc, const char 
*want, bool *match,
 }
 name[namelen] = '\0';
 len -= namelen;
-if (drop_sync(ioc, len) != len) {
+if (drop_sync(ioc, len) < 0) {
 error_setg(errp, "failed to read export description");
 nbd_send_opt_abort(ioc);
 return -1;
@@ -635,7 +638,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char 
*name, uint16_t *flags,
 }
 
 TRACE("Size is %" PRIu64 ", export flags %" PRIx16, *size, *flags);
-if (zeroes && drop_sync(ioc, 124) != 124) {
+if (zeroes && drop_sync(ioc, 124) < 0) {
 error_setg(errp, "Failed to read reserved block");
 goto fail;
 }
-- 
2.11.0