Re: [PATCH net-next v1 1/8] devlink: retain error in struct devlink_fmsg

[Przemek Kitszel <przemyslaw.kitszel@intel.com>]

On 10/10/23 13:34, Jiri Pirko wrote:
> Tue, Oct 10, 2023 at 12:43:11PM CEST, przemyslaw.kitszel@intel.com wrote:
>> Retain error value in struct devlink_fmsg, to relieve drivers from
>> checking it after each call.
>> Note that fmsg is an in-memory builder/buffer of formatted message,
>> so it's not the case that half baked message was sent somewhere.
>>
>> We could find following scheme in multiple drivers:
>>   err = devlink_fmsg_obj_nest_start(fmsg);
>>   if (err)
>>   	return err;
>>   err = devlink_fmsg_string_pair_put(fmsg, "src", src);
>>   if (err)
>>   	return err;
>>   err = devlink_fmsg_something(fmsg, foo, bar);
>>   if (err)
>> 	return err;
>>   // and so on...
>>   err = devlink_fmsg_obj_nest_end(fmsg);
>>
>> With retaining error API that translates to:
>>   devlink_fmsg_obj_nest_start(fmsg);
>>   devlink_fmsg_string_pair_put(fmsg, "src", src);
>>   devlink_fmsg_something(fmsg, foo, bar);
>>   // and so on...
>>   err = devlink_fmsg_obj_nest_end(fmsg);
> 
> I like this approach. But it looks a bit odd that you store error and
> return it as well, leaving the caller to decide what to do in his code.
> It is not desirable to leave the caller wondering.
> 
> Also, it is customary to check the return value if the function returns
> it. This approach confuses the customs.
> 
> Also, eventually, the fmsg is getting send. That is the point where the
> error could be checked and handled properly, for example by filling nice
> extack message.
> 
> What I'm saying is, please convert them all to return void, store the
> error and check that before fmsg send. That makes the approach unified
> for all callers, code nicer. Even the custom in-driver put functions
> would return void. The callbacks (e. g. dump) would also return void.

I was also thinking about that,
what about cases that you want to exit early, say inside of some loop?
add also devlink_fmsg_is_err()?

anyway, I like results more with ultimate unification :), only then all 
the drivers require conversion at the very same time

> 
> + a small nit below:
> 
> 
>>
>> What means we check error just at the end
>> (one could return it directly of course).
>>
>> Possible error scenarios are developer error (API misuse) and memory
>> exhaustion, both cases are good candidates to choose readability
>> over fastest possible exit.
>>
>> This commit itself is an illustration of benefits for the dev-user,
>> more of it will be in separate commits of the series.
>>
>> Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
>> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
>> ---
>> add/remove: 2/4 grow/shrink: 11/9 up/down: 325/-646 (-321)
>> ---
>> net/devlink/health.c | 255 ++++++++++++++++---------------------------
>> 1 file changed, 92 insertions(+), 163 deletions(-)
>>
>> diff --git a/net/devlink/health.c b/net/devlink/health.c
>> index 638cad8d5c65..2d26479e9dbe 100644
>> --- a/net/devlink/health.c
>> +++ b/net/devlink/health.c
>> @@ -19,6 +19,7 @@ struct devlink_fmsg_item {
>>
>> struct devlink_fmsg {
>> 	struct list_head item_list;
>> +	int err; /* first error encountered on some devlink_fmsg_XXX() call */
>> 	bool putting_binary; /* This flag forces enclosing of binary data
>> 			      * in an array brackets. It forces using
>> 			      * of designated API:
>> @@ -565,10 +566,8 @@ static int devlink_health_do_dump(struct devlink_health_reporter *reporter,
>> 		return 0;
>>
>> 	reporter->dump_fmsg = devlink_fmsg_alloc();
>> -	if (!reporter->dump_fmsg) {
>> -		err = -ENOMEM;
>> -		return err;
>> -	}
>> +	if (!reporter->dump_fmsg)
>> +		return -ENOMEM;
>>
>> 	err = devlink_fmsg_obj_nest_start(reporter->dump_fmsg);
>> 	if (err)
>> @@ -673,43 +672,59 @@ int devlink_nl_cmd_health_reporter_recover_doit(struct sk_buff *skb,
>> 	return devlink_health_reporter_recover(reporter, NULL, info->extack);
>> }
>>
>> -static int devlink_fmsg_nest_common(struct devlink_fmsg *fmsg,
>> -				    int attrtype)
>> +static bool _devlink_fmsg_err_or_binary(struct devlink_fmsg *fmsg)
> 
> No need for "_" here. Drop it.
> 
> 
>> +{
>> +	if (!fmsg->err && fmsg->putting_binary)
>> +		fmsg->err = -EINVAL;
>> +
>> +	return fmsg->err;
>> +}
>> +
> 
> [...]