| Message ID | 20220508214500.60446-1-colin.i.king@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | ecd17a87eb78b5bd5ca6d1aa20c39f2bc3591337 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | x25: remove redundant pointer dev | expand |
On Mon, May 9, 2022 at 1:45 AM Colin Ian King <colin.i.king@gmail.com> wrote: > > Pointer dev is being assigned a value that is never used, the assignment > and the variable are redundant and can be removed. Also replace null check > with the preferred !ptr idiom. > Hello, *dev pointer is device assign global linked list and shouldnt be touched by the driver so *dev wont get any value right? Also seems to use this while network interface is initializing because some activation information and stats information is also kept here, for example, open *dev will call when ifconfig is called from. route, link, forward these inital activate and move all values with net_device *dev? Regards > Cleans up clang scan warning: > net/x25/x25_proc.c:94:26: warning: Although the value stored to 'dev' is > used in the enclosing expression, the value is never actually read > from 'dev' [deadcode.DeadStores] > > Signed-off-by: Colin Ian King <colin.i.king@gmail.com> > --- > net/x25/x25_proc.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/net/x25/x25_proc.c b/net/x25/x25_proc.c > index 3bddcbdf2e40..91a2aade3960 100644 > --- a/net/x25/x25_proc.c > +++ b/net/x25/x25_proc.c > @@ -79,7 +79,6 @@ static int x25_seq_socket_show(struct seq_file *seq, void *v) > { > struct sock *s; > struct x25_sock *x25; > - struct net_device *dev; > const char *devname; > > if (v == SEQ_START_TOKEN) { > @@ -91,7 +90,7 @@ static int x25_seq_socket_show(struct seq_file *seq, void *v) > s = sk_entry(v); > x25 = x25_sk(s); > > - if (!x25->neighbour || (dev = x25->neighbour->dev) == NULL) > + if (!x25->neighbour || !x25->neighbour->dev) > devname = "???"; > else > devname = x25->neighbour->dev->name; > -- > 2.35.1 >
On Mon, May 09, 2022 at 04:57:40AM +0400, Ozgur wrote: > On Mon, May 9, 2022 at 1:45 AM Colin Ian King <colin.i.king@gmail.com> wrote: > > > > Pointer dev is being assigned a value that is never used, the assignment > > and the variable are redundant and can be removed. Also replace null check > > with the preferred !ptr idiom. > > > > Hello, > > *dev pointer is device assign global linked list and shouldnt be > touched by the driver so *dev wont get any value right? Why are you talking about "*dev" instead of "dev"? > Also seems to use this while network interface is initializing because > some activation information and stats information is also kept here, > for example, open *dev will call when ifconfig is called from. > > route, link, forward these inital activate and move all values with > net_device *dev? It's not clear what you are saying... When I review these kinds of patches I ask: 1) Does Colin's patch change run time behavior? Obviosly not. 2) Is the current code buggy? Sometimes when there is a static checker warning it indicates a typo in the code. I do not see a bug in the original code before Colin's patch. 3) What was the author's original intent? This code predates git but I think the "dev" was just a going to be a shorter name to type than "x25->neighbour->dev". I honestly have no idea what you are saying. At first I thought you might be saying that this is stub code. But that seems wrong. Also we do not allow stub code in the kernel. regards, dan carpenter
On Mon, May 9, 2022 at 4:44 PM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > On Mon, May 09, 2022 at 04:57:40AM +0400, Ozgur wrote: > > On Mon, May 9, 2022 at 1:45 AM Colin Ian King <colin.i.king@gmail.com> wrote: > > > > > > Pointer dev is being assigned a value that is never used, the assignment > > > and the variable are redundant and can be removed. Also replace null check > > > with the preferred !ptr idiom. > > > > > > > Hello, > > > > *dev pointer is device assign global linked list and shouldnt be > > touched by the driver so *dev wont get any value right? > > Why are you talking about "*dev" instead of "dev"? > Hi Dan, I just realized this, i thought dev was necessary for launch functions device and to activate. if carries this with x25, dev is useless I understand. > > Also seems to use this while network interface is initializing because > > some activation information and stats information is also kept here, > > for example, open *dev will call when ifconfig is called from. > > > > route, link, forward these inital activate and move all values with > > net_device *dev? > > It's not clear what you are saying... > > When I review these kinds of patches I ask: > 1) Does Colin's patch change run time behavior? Obviosly not. > 2) Is the current code buggy? Sometimes when there is a static checker > warning it indicates a typo in the code. I do not see a bug in the > original code before Colin's patch. > 3) What was the author's original intent? This code predates git but > I think the "dev" was just a going to be a shorter name to type than > "x25->neighbour->dev". Actually i thought similarly because when this patch remove dev from i thought would depend on other source code whose x25 interface will be implemented are not ready. I thought any x25 interface or x25route was using it. > I honestly have no idea what you are saying. At first I thought you > might be saying that this is stub code. But that seems wrong. Also we > do not allow stub code in the kernel. thanks for enlightening, Regards > regards, > dan carpenter >
Hello: This patch was applied to netdev/net-next.git (master) by Paolo Abeni <pabeni@redhat.com>: On Sun, 8 May 2022 22:45:00 +0100 you wrote: > Pointer dev is being assigned a value that is never used, the assignment > and the variable are redundant and can be removed. Also replace null check > with the preferred !ptr idiom. > > Cleans up clang scan warning: > net/x25/x25_proc.c:94:26: warning: Although the value stored to 'dev' is > used in the enclosing expression, the value is never actually read > from 'dev' [deadcode.DeadStores] > > [...] Here is the summary with links: - x25: remove redundant pointer dev https://git.kernel.org/netdev/net-next/c/ecd17a87eb78 You are awesome, thank you!
diff --git a/net/x25/x25_proc.c b/net/x25/x25_proc.c index 3bddcbdf2e40..91a2aade3960 100644 --- a/net/x25/x25_proc.c +++ b/net/x25/x25_proc.c @@ -79,7 +79,6 @@ static int x25_seq_socket_show(struct seq_file *seq, void *v) { struct sock *s; struct x25_sock *x25; - struct net_device *dev; const char *devname; if (v == SEQ_START_TOKEN) { @@ -91,7 +90,7 @@ static int x25_seq_socket_show(struct seq_file *seq, void *v) s = sk_entry(v); x25 = x25_sk(s); - if (!x25->neighbour || (dev = x25->neighbour->dev) == NULL) + if (!x25->neighbour || !x25->neighbour->dev) devname = "???"; else devname = x25->neighbour->dev->name;
Pointer dev is being assigned a value that is never used, the assignment and the variable are redundant and can be removed. Also replace null check with the preferred !ptr idiom. Cleans up clang scan warning: net/x25/x25_proc.c:94:26: warning: Although the value stored to 'dev' is used in the enclosing expression, the value is never actually read from 'dev' [deadcode.DeadStores] Signed-off-by: Colin Ian King <colin.i.king@gmail.com> --- net/x25/x25_proc.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)