netdev - Re: [RFC net-next 01/10] devlink: bump the instance index directly when iterating

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]

Message-ID: <Y7PatJruZVTEz7Pb@nanopsycho>
Date:   Tue, 3 Jan 2023 08:35:16 +0100
From:   Jiri Pirko <jiri@...nulli.us>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     jacob.e.keller@...el.com, leon@...nel.org, netdev@...r.kernel.org
Subject: Re: [RFC net-next 01/10] devlink: bump the instance index directly
 when iterating

Mon, Jan 02, 2023 at 11:48:13PM CET, kuba@...nel.org wrote:
>On Mon, 2 Jan 2023 14:24:39 +0100 Jiri Pirko wrote:
>> Sat, Dec 17, 2022 at 02:19:44AM CET, kuba@...nel.org wrote:
>> >We use a clever find_first() / find_after() scheme currently,
>> >which works nicely as xarray will write the "current" index
>> >into the variable we pass.
>> >
>> >We can't do the same thing during the "dump walk" because
>> >there we must not increment the index until we're sure
>> >that the instance has been fully dumped.  
>> 
>> To be honest, this "we something" desctiption style makes things quite
>> hard to understand. Could you please rephrase it to actually talk
>> about the entities in code?
>
>Could you be more specific? I'm probably misunderstanding but it sounds
>to me like you're asking me to describe what I'm doing rather than
>the background and motivation.

"We" is us, you me and other people. It is weird to talk about the code
and what there as "we do", "we use" etc. It's quite confusing as the
reader it not able to distinguish if you are talking about code or
people (developers in this case). I'm just askin if it is possible
to make the descriptions easier to understand.


>
>> >diff --git a/net/devlink/devl_internal.h b/net/devlink/devl_internal.h
>> >index 1d7ab11f2f7e..ef0369449592 100644
>> >--- a/net/devlink/devl_internal.h
>> >+++ b/net/devlink/devl_internal.h
>> >@@ -82,18 +82,9 @@ extern struct genl_family devlink_nl_family;
>> >  * in loop body in order to release the reference.
>> >  */
>> > #define devlinks_xa_for_each_registered_get(net, index, devlink)	\
>> >-	for (index = 0,							\
>> >-	     devlink = devlinks_xa_find_get_first(net, &index);	\
>> >-	     devlink; devlink = devlinks_xa_find_get_next(net, &index))
>> >-
>> >-struct devlink *
>> >-devlinks_xa_find_get(struct net *net, unsigned long *indexp,
>> >-		     void * (*xa_find_fn)(struct xarray *, unsigned long *,
>> >-					  unsigned long, xa_mark_t));
>> >-struct devlink *
>> >-devlinks_xa_find_get_first(struct net *net, unsigned long *indexp);
>> >-struct devlink *
>> >-devlinks_xa_find_get_next(struct net *net, unsigned long *indexp);
>> >+	for (index = 0; (devlink = devlinks_xa_find_get(net, &index)); index++)  
>> 
>> You don't need ()' in the 2nd for arg.
>
>Pretty sure I was getting a compiler warning for assignment in 
>a condition....

I see.