Re: [Qemu-devel] [PATCH 01/12] nbd: rename read_sync and friends

[Eric Blake]

On 05/31/2017 11:55 AM, Vladimir Sementsov-Ogievskiy wrote:
> Rename
>   nbd_wr_syncv -> nbd_rwv
>   read_sync -> nbd_read
>   read_sync_eof -> nbd_read_eof
>   write_sync -> nbd_write
>   drop_sync -> nbd_drop
> 
> 1. nbd_ prefix
>    read_sync and write_sync are already shared, so it is good to have a
>    namespace prefix. drop_sync will be shared, and read_sync_eof is
>    related to read_sync, so let's rename them all.
> 
> 2. _sync suffix
>    _sync is related to the fact, that nbd_wr_syncv doesn't return if

s/fact,/fact/

>    write to socket returns EAGAIN. In first implementation nbd_wr_syncv
>    just loops while getting EAGAIN, current implementation yields in
>    this case.

As mentioned in your followup, you may want to rewrite this to:

_sync was originally used (back in commit 7a5ca864 when it was named
wr_sync) to indicate that we looped rather than returned on EAGAIN.  But
now we use qio_channel which yields on our behalf rather than giving us
EAGAIN.

>    Why to get rid of it:
>    - it is normal for r/w functions to be synchronous, so having
>      additional suffix for it looks redundant (contrariwise, we have
>      _aio suffix for async functions)
>    - _sync suffix in block layer is used when function does flush (so
>      using it for other thing is confusing a bit)
>    - keep function names short after adding nbd_ prefix
> 
> 3. for nbd_wr_syncv let's use more common notation 'rw'
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---

The maintainer may be willing to tweak the commit message without
needing a v2.


> +++ b/nbd/nbd-internal.h
> @@ -94,14 +94,14 @@
>  #define NBD_ENOSPC     28
>  #define NBD_ESHUTDOWN  108
>  
> -/* read_sync_eof
> +/* 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 are no needs to call 
> read_sync_eof
> + * qio_channel_readv() returns 0. So, there are no needs to call nbd_read_eof

As long as you are touching this:

s/are no needs/is no need/

Reviewed-by: Eric Blake <address@hidden>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature