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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Tue, 10 Oct 2023 15:36:48 +0200
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Jiri Pirko <jiri@...nulli.us>
CC: <netdev@...r.kernel.org>, "David S . Miller" <davem@...emloft.net>, "Eric
 Dumazet" <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni
	<pabeni@...hat.com>, Shannon Nelson <shannon.nelson@....com>, Michael Chan
	<michael.chan@...adcom.com>, Edwin Peer <edwin.peer@...adcom.com>, "Cai
 Huoqing" <cai.huoqing@...ux.dev>, George Cherian
	<george.cherian@...vell.com>, Danielle Ratson <danieller@...dia.com>, "Moshe
 Shemesh" <moshe@...dia.com>, Saeed Mahameed <saeedm@...dia.com>, Brett
 Creeley <brett.creeley@....com>, Sunil Goutham <sgoutham@...vell.com>, Linu
 Cherian <lcherian@...vell.com>, Geetha sowjanya <gakula@...vell.com>, Jerin
 Jacob <jerinj@...vell.com>, hariprasad <hkelam@...vell.com>, Subbaraya
 Sundeep <sbhatta@...vell.com>, Ido Schimmel <idosch@...dia.com>, Petr Machata
	<petrm@...dia.com>, Eran Ben Elisha <eranbe@...dia.com>, Aya Levin
	<ayal@...lanox.com>, Leon Romanovsky <leon@...nel.org>, Jesse Brandeburg
	<jesse.brandeburg@...el.com>
Subject: Re: [PATCH net-next v1 1/8] devlink: retain error in struct
 devlink_fmsg

On 10/10/23 13:34, Jiri Pirko wrote:
> Tue, Oct 10, 2023 at 12:43:11PM CEST, przemyslaw.kitszel@...el.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@...el.com>
>> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@...el.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;
>> +}
>> +
> 
> [...]