Skip to contentSkip to conversation

: udp_proc_register simplification.

PATCH In review Denis V. Lunev 2 participants 8 messages Mar 27, 2008 – Mar 29, 2008

Subsystems (3)
Networking Network drivers UDP
lore
Share & tools
lore

Denis V. Lunev

author
#0 lore raw

cover : udp_proc_register simplification.

Hello, Dave!

Here is a small set of changes to simplify udp_proc_register. It seems to difficult for me. Let's simplify it a bit.

Regards,
Den

author
#1 lore raw

PATCH 1/6 : Replace struct net on udp_iter_state with seq_net_private.

Signed-off-by Denis V. Lunev <[email protected]>
---
 include/net/udp.h |    2 +-
 net/ipv4/udp.c    |    8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)
include/net/udp.h +1 -1
index 635940d..77af7d4 100644
@@ -194,7 +194,7 @@ struct udp_seq_afinfo {
 };
 
 struct udp_iter_state {
-	struct net              *net;
+	struct seq_net_private  p;
 	sa_family_t		family;
 	struct hlist_head	*hashtable;
 	int			bucket;
net/ipv4/udp.c +4 -4
index 80007c7..6d740ad 100644
@@ -1506,7 +1506,7 @@ static struct sock *udp_get_first(struct seq_file *seq)
 {
 	struct sock *sk;
 	struct udp_iter_state *state = seq->private;
-	struct net *net = state->net;
+	struct net *net = seq_file_net(seq);
 
 	for (state->bucket = 0; state->bucket < UDP_HTABLE_SIZE; ++state->bucket) {
 		struct hlist_node *node;

@@ -1525,7 +1525,7 @@ found:
 static struct sock *udp_get_next(struct seq_file *seq, struct sock *sk)
 {
 	struct udp_iter_state *state = seq->private;
-	struct net *net = state->net;
+	struct net *net = seq_file_net(seq);
 
 	do {
 		sk = sk_next(sk);

@@ -1598,7 +1598,7 @@ static int udp_seq_open(struct inode *inode, struct file *file)
 	s->seq_ops.next		= udp_seq_next;
 	s->seq_ops.show		= afinfo->seq_show;
 	s->seq_ops.stop		= udp_seq_stop;
-	s->net                  = net;
+	s->p.net                = net;
 
 	rc = seq_open(file, &s->seq_ops);
 	if (rc)

@@ -1620,7 +1620,7 @@ static int udp_seq_release(struct inode *inode, struct file *file)
 	struct seq_file *seq = file->private_data;
 	struct udp_iter_state *s = seq->private;
 
-	put_net(s->net);
+	put_net(s->p.net);
 	seq_release_private(inode, file);
 	return 0;
 }
Signature · 1 line
1.5.3.rc5
author
#2 lore raw

PATCH 2/6 : No need to check afinfo != NULL in udp_proc_(un)register.

In reply to Denis V. Lunev

udp_proc_register/udp_proc_unregister are called with a static pointer only.

Signed-off-by Denis V. Lunev <[email protected]>
---
 net/ipv4/udp.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)
net/ipv4/udp.c +0 -4
index 6d740ad..c05f45b 100644
@@ -1631,8 +1631,6 @@ int udp_proc_register(struct net *net, struct udp_seq_afinfo *afinfo)
 	struct proc_dir_entry *p;
 	int rc = 0;
 
-	if (!afinfo)
-		return -EINVAL;
 	afinfo->seq_fops->owner		= afinfo->owner;
 	afinfo->seq_fops->open		= udp_seq_open;
 	afinfo->seq_fops->read		= seq_read;

@@ -1649,8 +1647,6 @@ int udp_proc_register(struct net *net, struct udp_seq_afinfo *afinfo)
 
 void udp_proc_unregister(struct net *net, struct udp_seq_afinfo *afinfo)
 {
-	if (!afinfo)
-		return;
 	proc_net_remove(net, afinfo->name);
 	memset(afinfo->seq_fops, 0, sizeof(*afinfo->seq_fops));
 }
Signature · 1 line
1.5.3.rc5
author
#3 lore raw

PATCH 3/6 : Move seq_ops from udp_iter_state to udp_seq_afinfo.

In reply to Denis V. Lunev

No need to create seq_operations for each instance of 'netstat'.

Signed-off-by Denis V. Lunev <[email protected]>
---
 include/net/udp.h  |    3 +--
 net/ipv4/udp.c     |   14 ++++++++------
 net/ipv4/udplite.c |    4 +++-
 net/ipv6/udp.c     |    4 +++-
 net/ipv6/udplite.c |    4 +++-
 5 files changed, 18 insertions(+), 11 deletions(-)
include/net/udp.h +1 -2
index 77af7d4..0079d17 100644
@@ -189,8 +189,8 @@ struct udp_seq_afinfo {
 	char			*name;
 	sa_family_t		family;
 	struct hlist_head	*hashtable;
-	int 			(*seq_show) (struct seq_file *m, void *v);
 	struct file_operations	*seq_fops;
+	struct seq_operations	seq_ops;
 };
 
 struct udp_iter_state {

@@ -198,7 +198,6 @@ struct udp_iter_state {
 	sa_family_t		family;
 	struct hlist_head	*hashtable;
 	int			bucket;
-	struct seq_operations	seq_ops;
 };
 
 #ifdef CONFIG_PROC_FS
net/ipv4/udp.c +8 -6
index c05f45b..43920f1 100644
@@ -1594,13 +1594,9 @@ static int udp_seq_open(struct inode *inode, struct file *file)
 
 	s->family		= afinfo->family;
 	s->hashtable		= afinfo->hashtable;
-	s->seq_ops.start	= udp_seq_start;
-	s->seq_ops.next		= udp_seq_next;
-	s->seq_ops.show		= afinfo->seq_show;
-	s->seq_ops.stop		= udp_seq_stop;
 	s->p.net                = net;
 
-	rc = seq_open(file, &s->seq_ops);
+	rc = seq_open(file, &afinfo->seq_ops);
 	if (rc)
 		goto out_put_net;
 

@@ -1637,6 +1633,10 @@ int udp_proc_register(struct net *net, struct udp_seq_afinfo *afinfo)
 	afinfo->seq_fops->llseek	= seq_lseek;
 	afinfo->seq_fops->release	= udp_seq_release;
 
+	afinfo->seq_ops.start		= udp_seq_start;
+	afinfo->seq_ops.next		= udp_seq_next;
+	afinfo->seq_ops.stop		= udp_seq_stop;
+
 	p = proc_net_fops_create(net, afinfo->name, S_IRUGO, afinfo->seq_fops);
 	if (p)
 		p->data = afinfo;

@@ -1693,8 +1693,10 @@ static struct udp_seq_afinfo udp4_seq_afinfo = {
 	.name		= "udp",
 	.family		= AF_INET,
 	.hashtable	= udp_hash,
-	.seq_show	= udp4_seq_show,
 	.seq_fops	= &udp4_seq_fops,
+	.seq_ops	= {
+		.show		= udp4_seq_show,
+	},
 };
 
 static int udp4_proc_init_net(struct net *net)
net/ipv4/udplite.c +3 -1
index 53f3ed4..3cb8b7f 100644
@@ -80,8 +80,10 @@ static struct udp_seq_afinfo udplite4_seq_afinfo = {
 	.name		= "udplite",
 	.family		= AF_INET,
 	.hashtable	= udplite_hash,
-	.seq_show	= udp4_seq_show,
 	.seq_fops	= &udplite4_seq_fops,
+	.seq_ops	= {
+		.show		= udp4_seq_show,
+	},
 };
 
 static int udplite4_proc_init_net(struct net *net)
net/ipv6/udp.c +3 -1
index aacbc82..cb3a3c6 100644
@@ -983,8 +983,10 @@ static struct udp_seq_afinfo udp6_seq_afinfo = {
 	.name		= "udp6",
 	.family		= AF_INET6,
 	.hashtable	= udp_hash,
-	.seq_show	= udp6_seq_show,
 	.seq_fops	= &udp6_seq_fops,
+	.seq_ops	= {
+		.show		= udp6_seq_show,
+	},
 };
 
 int udp6_proc_init(struct net *net)
net/ipv6/udplite.c +3 -1
index 706c5c3..56223eb 100644
@@ -105,8 +105,10 @@ static struct udp_seq_afinfo udplite6_seq_afinfo = {
 	.name		= "udplite6",
 	.family		= AF_INET6,
 	.hashtable	= udplite_hash,
-	.seq_show	= udp6_seq_show,
 	.seq_fops	= &udplite6_seq_fops,
+	.seq_ops	= {
+		.show		= udp6_seq_show,
+	},
 };
 
 static int udplite6_proc_init_net(struct net *net)
Signature · 1 line
1.5.3.rc5
author
#4 lore raw

PATCH 4/6 : Cleanup /proc/udp[6] creation/removal.

In reply to Denis V. Lunev

Replace seq_open with seq_open_net and remove udp_seq_release completely. seq_release_net will do this job just fine.

Signed-off-by Denis V. Lunev <[email protected]>
---
 net/ipv4/udp.c |   46 +++++++++-------------------------------------
 1 files changed, 9 insertions(+), 37 deletions(-)
net/ipv4/udp.c +9 -37
index 43920f1..1b1f299 100644
@@ -1579,46 +1579,18 @@ static void udp_seq_stop(struct seq_file *seq, void *v)
 static int udp_seq_open(struct inode *inode, struct file *file)
 {
 	struct udp_seq_afinfo *afinfo = PDE(inode)->data;
-	struct seq_file *seq;
-	struct net *net;
-	int rc = -ENOMEM;
-	struct udp_iter_state *s = kzalloc(sizeof(*s), GFP_KERNEL);
-
-	if (!s)
-		goto out;
+	struct udp_iter_state *s;
+	int err;
 
-	rc = -ENXIO;
-	net = get_proc_net(inode);
-	if (!net)
-		goto out_kfree;
+	err = seq_open_net(inode, file, &afinfo->seq_ops,
+			   sizeof(struct udp_iter_state));
+	if (err < 0)
+		return err;
 
+	s = ((struct seq_file *)file->private_data)->private;
 	s->family		= afinfo->family;
 	s->hashtable		= afinfo->hashtable;
-	s->p.net                = net;
-
-	rc = seq_open(file, &afinfo->seq_ops);
-	if (rc)
-		goto out_put_net;
-
-	seq = file->private_data;
-	seq->private = s;
-out:
-	return rc;
-out_put_net:
-	put_net(net);
-out_kfree:
-	kfree(s);
-	goto out;
-}
-
-static int udp_seq_release(struct inode *inode, struct file *file)
-{
-	struct seq_file *seq = file->private_data;
-	struct udp_iter_state *s = seq->private;
-
-	put_net(s->p.net);
-	seq_release_private(inode, file);
-	return 0;
+	return err;
 }
 
 /* ------------------------------------------------------------------------ */

@@ -1631,7 +1603,7 @@ int udp_proc_register(struct net *net, struct udp_seq_afinfo *afinfo)
 	afinfo->seq_fops->open		= udp_seq_open;
 	afinfo->seq_fops->read		= seq_read;
 	afinfo->seq_fops->llseek	= seq_lseek;
-	afinfo->seq_fops->release	= udp_seq_release;
+	afinfo->seq_fops->release	= seq_release_net;
 
 	afinfo->seq_ops.start		= udp_seq_start;
 	afinfo->seq_ops.next		= udp_seq_next;
Signature · 1 line
1.5.3.rc5
author
#5 lore raw

PATCH 5/6 : Place file operations directly into udp_seq_afinfo.

In reply to Denis V. Lunev

No need to have separate never-used variable.

Signed-off-by Denis V. Lunev <[email protected]>
---
 include/net/udp.h  |    2 +-
 net/ipv4/udp.c     |   15 ++++++---------
 net/ipv4/udplite.c |    2 --
 net/ipv6/udp.c     |    2 --
 net/ipv6/udplite.c |    2 --
 5 files changed, 7 insertions(+), 16 deletions(-)
include/net/udp.h +1 -1
index 0079d17..5cf0e59 100644
@@ -189,7 +189,7 @@ struct udp_seq_afinfo {
 	char			*name;
 	sa_family_t		family;
 	struct hlist_head	*hashtable;
-	struct file_operations	*seq_fops;
+	struct file_operations	seq_fops;
 	struct seq_operations	seq_ops;
 };
net/ipv4/udp.c +6 -9
index 1b1f299..3f5ad77 100644
@@ -1599,17 +1599,17 @@ int udp_proc_register(struct net *net, struct udp_seq_afinfo *afinfo)
 	struct proc_dir_entry *p;
 	int rc = 0;
 
-	afinfo->seq_fops->owner		= afinfo->owner;
-	afinfo->seq_fops->open		= udp_seq_open;
-	afinfo->seq_fops->read		= seq_read;
-	afinfo->seq_fops->llseek	= seq_lseek;
-	afinfo->seq_fops->release	= seq_release_net;
+	afinfo->seq_fops.owner		= afinfo->owner;
+	afinfo->seq_fops.open		= udp_seq_open;
+	afinfo->seq_fops.read		= seq_read;
+	afinfo->seq_fops.llseek		= seq_lseek;
+	afinfo->seq_fops.release	= seq_release_net;
 
 	afinfo->seq_ops.start		= udp_seq_start;
 	afinfo->seq_ops.next		= udp_seq_next;
 	afinfo->seq_ops.stop		= udp_seq_stop;
 
-	p = proc_net_fops_create(net, afinfo->name, S_IRUGO, afinfo->seq_fops);
+	p = proc_net_fops_create(net, afinfo->name, S_IRUGO, &afinfo->seq_fops);
 	if (p)
 		p->data = afinfo;
 	else

@@ -1620,7 +1620,6 @@ int udp_proc_register(struct net *net, struct udp_seq_afinfo *afinfo)
 void udp_proc_unregister(struct net *net, struct udp_seq_afinfo *afinfo)
 {
 	proc_net_remove(net, afinfo->name);
-	memset(afinfo->seq_fops, 0, sizeof(*afinfo->seq_fops));
 }
 
 /* ------------------------------------------------------------------------ */

@@ -1659,13 +1658,11 @@ int udp4_seq_show(struct seq_file *seq, void *v)
 }
 
 /* ------------------------------------------------------------------------ */
-static struct file_operations udp4_seq_fops;
 static struct udp_seq_afinfo udp4_seq_afinfo = {
 	.owner		= THIS_MODULE,
 	.name		= "udp",
 	.family		= AF_INET,
 	.hashtable	= udp_hash,
-	.seq_fops	= &udp4_seq_fops,
 	.seq_ops	= {
 		.show		= udp4_seq_show,
 	},
net/ipv4/udplite.c +0 -2
index 3cb8b7f..f73bb17 100644
@@ -74,13 +74,11 @@ static struct inet_protosw udplite4_protosw = {
 };
 
 #ifdef CONFIG_PROC_FS
-static struct file_operations udplite4_seq_fops;
 static struct udp_seq_afinfo udplite4_seq_afinfo = {
 	.owner		= THIS_MODULE,
 	.name		= "udplite",
 	.family		= AF_INET,
 	.hashtable	= udplite_hash,
-	.seq_fops	= &udplite4_seq_fops,
 	.seq_ops	= {
 		.show		= udp4_seq_show,
 	},
net/ipv6/udp.c +0 -2
index cb3a3c6..475f2aa 100644
@@ -977,13 +977,11 @@ int udp6_seq_show(struct seq_file *seq, void *v)
 	return 0;
 }
 
-static struct file_operations udp6_seq_fops;
 static struct udp_seq_afinfo udp6_seq_afinfo = {
 	.owner		= THIS_MODULE,
 	.name		= "udp6",
 	.family		= AF_INET6,
 	.hashtable	= udp_hash,
-	.seq_fops	= &udp6_seq_fops,
 	.seq_ops	= {
 		.show		= udp6_seq_show,
 	},
net/ipv6/udplite.c +0 -2
index 56223eb..5a9e646 100644
@@ -99,13 +99,11 @@ void udplitev6_exit(void)
 }
 
 #ifdef CONFIG_PROC_FS
-static struct file_operations udplite6_seq_fops;
 static struct udp_seq_afinfo udplite6_seq_afinfo = {
 	.owner		= THIS_MODULE,
 	.name		= "udplite6",
 	.family		= AF_INET6,
 	.hashtable	= udplite_hash,
-	.seq_fops	= &udplite6_seq_fops,
 	.seq_ops	= {
 		.show		= udp6_seq_show,
 	},
Signature · 1 line
1.5.3.rc5
author
#6 lore raw

PATCH 6/6 : Remove owner from udp_seq_afinfo.

In reply to Denis V. Lunev

Move it to udp_seq_afinfo->seq_fops as should be.

Signed-off-by Denis V. Lunev <[email protected]>
---
 include/net/udp.h  |    1 -
 net/ipv4/udp.c     |    5 +++--
 net/ipv4/udplite.c |    4 +++-
 net/ipv6/udp.c     |    4 +++-
 net/ipv6/udplite.c |    4 +++-
 5 files changed, 12 insertions(+), 6 deletions(-)
include/net/udp.h +0 -1
index 5cf0e59..24a41fa 100644
@@ -185,7 +185,6 @@ DECLARE_SNMP_STAT(struct udp_mib, udplite_stats_in6);
 
 /* /proc */
 struct udp_seq_afinfo {
-	struct module		*owner;
 	char			*name;
 	sa_family_t		family;
 	struct hlist_head	*hashtable;
net/ipv4/udp.c +3 -2
index 3f5ad77..954bd6d 100644
@@ -1599,7 +1599,6 @@ int udp_proc_register(struct net *net, struct udp_seq_afinfo *afinfo)
 	struct proc_dir_entry *p;
 	int rc = 0;
 
-	afinfo->seq_fops.owner		= afinfo->owner;
 	afinfo->seq_fops.open		= udp_seq_open;
 	afinfo->seq_fops.read		= seq_read;
 	afinfo->seq_fops.llseek		= seq_lseek;

@@ -1659,10 +1658,12 @@ int udp4_seq_show(struct seq_file *seq, void *v)
 
 /* ------------------------------------------------------------------------ */
 static struct udp_seq_afinfo udp4_seq_afinfo = {
-	.owner		= THIS_MODULE,
 	.name		= "udp",
 	.family		= AF_INET,
 	.hashtable	= udp_hash,
+	.seq_fops	= {
+		.owner	=	THIS_MODULE,
+	},
 	.seq_ops	= {
 		.show		= udp4_seq_show,
 	},
net/ipv4/udplite.c +3 -1
index f73bb17..1a4f2d4 100644
@@ -75,10 +75,12 @@ static struct inet_protosw udplite4_protosw = {
 
 #ifdef CONFIG_PROC_FS
 static struct udp_seq_afinfo udplite4_seq_afinfo = {
-	.owner		= THIS_MODULE,
 	.name		= "udplite",
 	.family		= AF_INET,
 	.hashtable	= udplite_hash,
+	.seq_fops	= {
+		.owner	=	THIS_MODULE,
+	},
 	.seq_ops	= {
 		.show		= udp4_seq_show,
 	},
net/ipv6/udp.c +3 -1
index 475f2aa..5279bb2 100644
@@ -978,10 +978,12 @@ int udp6_seq_show(struct seq_file *seq, void *v)
 }
 
 static struct udp_seq_afinfo udp6_seq_afinfo = {
-	.owner		= THIS_MODULE,
 	.name		= "udp6",
 	.family		= AF_INET6,
 	.hashtable	= udp_hash,
+	.seq_fops	= {
+		.owner	=	THIS_MODULE,
+	},
 	.seq_ops	= {
 		.show		= udp6_seq_show,
 	},
net/ipv6/udplite.c +3 -1
index 5a9e646..2fd1334 100644
@@ -100,10 +100,12 @@ void udplitev6_exit(void)
 
 #ifdef CONFIG_PROC_FS
 static struct udp_seq_afinfo udplite6_seq_afinfo = {
-	.owner		= THIS_MODULE,
 	.name		= "udplite6",
 	.family		= AF_INET6,
 	.hashtable	= udplite_hash,
+	.seq_fops	= {
+		.owner	=	THIS_MODULE,
+	},
 	.seq_ops	= {
 		.show		= udp6_seq_show,
 	},
Signature · 1 line
1.5.3.rc5
maintainer
#7 lore raw
In reply to Denis V. Lunev

From: "Denis V. Lunev" <[email protected]>
Date: Thu, 27 Mar 2008 17:10:41 +0300

Quoted 4 lines

Hello, Dave!

Here is a small set of changes to simplify udp_proc_register. It seems to difficult for me. Let's simplify it a bit.

All applied, thanks!