|
From: | Vladimir Sementsov-Ogievskiy |
Subject: | Re: [Qemu-devel] [PATCH 01/12] nbd: rename read_sync and friends |
Date: | Fri, 2 Jun 2017 15:00:34 +0300 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.1 |
01.06.2017 11:03, Sementsov-Ogievskiy Vladimir wrote:
On 31.05.2017 21:46, Eric Blake wrote: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_ prefixread_sync and write_sync are already shared, so it is good to have anamespace 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 ifs/fact,/fact/write to socket returns EAGAIN. In first implementation nbd_wr_syncvjust 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.hmm, I like my wording (with adding note "... implementaion nbd_wr_syncv (was wr_sync in 7a5ca8648b) just ...") more, because: 1. not only nbd_wr_syncv has that suffix, so nbd_wr_syncv should be mentioned (as we mention wr_sync) 2. I don't say about contrast between old and current, I say that they are similar.
Finally, are you OK with my wording? If I reroll, can I add your r-b?
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.I hope for it)+++ 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_eofAs long as you are touching this: s/are no needs/is no need/ Reviewed-by: Eric Blake <address@hidden>
-- Best regards, Vladimir
[Prev in Thread] | Current Thread | [Next in Thread] |