[PATCH v6] nbd: well form nbd_iter_channel_error errp handler

[PATCH v6] nbd: well form nbd_iter_channel_error errp handler

[Vladimir Sementsov-Ogievskiy]

Make nbd_iter_channel_error errp handler well formed:
rename local_err to errp_in, as it is IN-parameter here (which is
unusual for Error**).

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---

v6: fix commit message
    add Eric's r-b

 block/nbd.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/block/nbd.c b/block/nbd.c
index 5f18f78a94..345bf902e3 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -864,18 +864,18 @@ typedef struct NBDReplyChunkIter {
 } NBDReplyChunkIter;
 
 static void nbd_iter_channel_error(NBDReplyChunkIter *iter,
-                                   int ret, Error **local_err)
+                                   int ret, Error **errp_in)
 {
-    assert(ret < 0);
+    assert(ret < 0 && errp_in && *errp_in);
 
     if (!iter->ret) {
         iter->ret = ret;
-        error_propagate(&iter->err, *local_err);
+        error_propagate(&iter->err, *errp_in);
     } else {
-        error_free(*local_err);
+        error_free(*errp_in);
     }
 
-    *local_err = NULL;
+    *errp_in = NULL;
 }
 
 static void nbd_iter_request_error(NBDReplyChunkIter *iter, int ret)
-- 
2.21.0

Re: [PATCH v6] nbd: well form nbd_iter_channel_error errp handler

[Eric Blake]

[adding Markus]

On 11/27/19 1:08 PM, Vladimir Sementsov-Ogievskiy wrote:
> Make nbd_iter_channel_error errp handler well formed:
> rename local_err to errp_in, as it is IN-parameter here (which is
> unusual for Error**).
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
> 
> v6: fix commit message
>      add Eric's r-b

I'm surprised that you aren't including Markus on a lot of these patches 
- even though you've posted a lot of them as separate threads to make 
them easier for individual maintainers to pick up, it would also be 
possible for Markus to pick up a bunch of them at once through his error 
tree.

At any rate, I'll queue this one through my NBD tree for 5.0 if it does 
not make it through Markus' error tree or the trivial tree sooner.


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

Re: [PATCH v6] nbd: well form nbd_iter_channel_error errp handler

[Vladimir Sementsov-Ogievskiy]

27.11.2019 22:49, Eric Blake wrote:
> [adding Markus]
> 
> On 11/27/19 1:08 PM, Vladimir Sementsov-Ogievskiy wrote:
>> Make nbd_iter_channel_error errp handler well formed:
>> rename local_err to errp_in, as it is IN-parameter here (which is
>> unusual for Error**).
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> ---
>>
>> v6: fix commit message
>>      add Eric's r-b
> 
> I'm surprised that you aren't including Markus on a lot of these patches - even though you've posted a lot of them as separate threads to make them easier for individual maintainers to pick up, it would also be possible for Markus to pick up a bunch of them at once through his error tree.

Oops, you are right, I'm sorry :(

If it helps, all patches Eric saying about are 21 patches from myself,
with v6 tag, sent during last two hours.

Sorry that I didn't answer to
   https://lists.gnu.org/archive/html/qemu-devel/2019-11/msg03105.html
before sending them, and I don't want do it now, to not move v5 above v6.

Week passed since my proposal at that link, so having one against (Eric)
and two for (Kevin and Greg), I decided to follow my plan now.

> 
> At any rate, I'll queue this one through my NBD tree for 5.0 if it does not make it through Markus' error tree or the trivial tree sooner.
> 
> 

Thanks!

-- 
Best regards,
Vladimir

Re: [PATCH v6] nbd: well form nbd_iter_channel_error errp handler

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> Make nbd_iter_channel_error errp handler well formed:
> rename local_err to errp_in, as it is IN-parameter here (which is
> unusual for Error**).
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>
> v6: fix commit message
>     add Eric's r-b
>
>  block/nbd.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/block/nbd.c b/block/nbd.c
> index 5f18f78a94..345bf902e3 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -864,18 +864,18 @@ typedef struct NBDReplyChunkIter {
>  } NBDReplyChunkIter;
>  
>  static void nbd_iter_channel_error(NBDReplyChunkIter *iter,
> -                                   int ret, Error **local_err)
> +                                   int ret, Error **errp_in)
>  {
> -    assert(ret < 0);
> +    assert(ret < 0 && errp_in && *errp_in);
>  
>      if (!iter->ret) {
>          iter->ret = ret;
> -        error_propagate(&iter->err, *local_err);
> +        error_propagate(&iter->err, *errp_in);
>      } else {
> -        error_free(*local_err);
> +        error_free(*errp_in);
>      }
>  
> -    *local_err = NULL;
> +    *errp_in = NULL;

This one is actually in/out.

If we use the convention

    Any Error ** parameter meant for passing an error to the caller must
    be named @errp.  No other Error ** parameter may be named @errp.

then the old name is as good as the new one.  But the new one's "in"
suggestion is misleading.

>  }
>  
>  static void nbd_iter_request_error(NBDReplyChunkIter *iter, int ret)

Re: [PATCH v6] nbd: well form nbd_iter_channel_error errp handler

[Vladimir Sementsov-Ogievskiy]

29.11.2019 16:25, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> 
>> Make nbd_iter_channel_error errp handler well formed:
>> rename local_err to errp_in, as it is IN-parameter here (which is
>> unusual for Error**).
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> ---
>>
>> v6: fix commit message
>>      add Eric's r-b
>>
>>   block/nbd.c | 10 +++++-----
>>   1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/block/nbd.c b/block/nbd.c
>> index 5f18f78a94..345bf902e3 100644
>> --- a/block/nbd.c
>> +++ b/block/nbd.c
>> @@ -864,18 +864,18 @@ typedef struct NBDReplyChunkIter {
>>   } NBDReplyChunkIter;
>>   
>>   static void nbd_iter_channel_error(NBDReplyChunkIter *iter,
>> -                                   int ret, Error **local_err)
>> +                                   int ret, Error **errp_in)
>>   {
>> -    assert(ret < 0);
>> +    assert(ret < 0 && errp_in && *errp_in);
>>   
>>       if (!iter->ret) {
>>           iter->ret = ret;
>> -        error_propagate(&iter->err, *local_err);
>> +        error_propagate(&iter->err, *errp_in);
>>       } else {
>> -        error_free(*local_err);
>> +        error_free(*errp_in);
>>       }
>>   
>> -    *local_err = NULL;
>> +    *errp_in = NULL;
> 
> This one is actually in/out.
> 
> If we use the convention
> 
>      Any Error ** parameter meant for passing an error to the caller must
>      be named @errp.  No other Error ** parameter may be named @errp.
> 
> then the old name is as good as the new one.  But the new one's "in"
> suggestion is misleading.
> 

Agreed. Do you have a suggestion how to rename errp in such cases (using
local_err in general will be misleading too)..

Maybe, "filled_errp" ? Seems too long..
"set_errp" is shorter, but no one will guess that this is the third form of the verb..

>>   }
>>   
>>   static void nbd_iter_request_error(NBDReplyChunkIter *iter, int ret)
> 


-- 
Best regards,
Vladimir