Re: [Qemu-devel] [PATCH 02/17] nbd/client: refactor nbd_read_eof

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2674 bytes --]

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 <vsementsov@virtuozzo.com>
>>> ---

>>> + * 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 <eblake@redhat.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]