| Message ID | 1248078849-7343-1-git-send-email-remi.denis-courmont@nokia.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
Hi Remi, > From: Rémi Denis-Courmont <remi.denis-courmont@nokia.com> isn't there are proper explaining commit message missing here? > Signed-off-by: Rémi Denis-Courmont <remi.denis-courmont@nokia.com> > --- > include/net/phonet/pn_dev.h | 2 + > net/phonet/pn_dev.c | 7 +++ > net/phonet/socket.c | 97 +++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 106 insertions(+), 0 deletions(-) > > diff --git a/include/net/phonet/pn_dev.h b/include/net/phonet/pn_dev.h > index 29d1267..44c923c 100644 > --- a/include/net/phonet/pn_dev.h > +++ b/include/net/phonet/pn_dev.h > @@ -49,4 +49,6 @@ void phonet_address_notify(int event, struct net_device *dev, u8 addr); > > #define PN_NO_ADDR 0xff > > +extern const struct file_operations pn_sock_seq_fops; > + > #endif > diff --git a/net/phonet/pn_dev.c b/net/phonet/pn_dev.c > index b0d6ddd..5107b79 100644 > --- a/net/phonet/pn_dev.c > +++ b/net/phonet/pn_dev.c > @@ -218,6 +218,11 @@ static int phonet_init_net(struct net *net) > if (!pnn) > return -ENOMEM; > > + if (!proc_net_fops_create(net, "phonet", 0, &pn_sock_seq_fops)) { > + kfree(pnn); > + return -ENOMEM; > + } > + > INIT_LIST_HEAD(&pnn->pndevs.list); > spin_lock_init(&pnn->pndevs.lock); > net_assign_generic(net, phonet_net_id, pnn); > @@ -233,6 +238,8 @@ static void phonet_exit_net(struct net *net) > for_each_netdev(net, dev) > phonet_device_destroy(dev); > rtnl_unlock(); > + > + proc_net_remove(net, "phonet"); > kfree(pnn); > } > > diff --git a/net/phonet/socket.c b/net/phonet/socket.c > index ada2a35..2a99851 100644 > --- a/net/phonet/socket.c > +++ b/net/phonet/socket.c > @@ -412,3 +412,100 @@ found: > return 0; > } > EXPORT_SYMBOL(pn_sock_get_port); > + > +static struct sock *pn_sock_get_idx(struct seq_file *seq, loff_t pos) > +{ > + struct net *net = seq_file_net(seq); > + struct hlist_node *node; > + struct sock *sknode; > + > + sk_for_each(sknode, node, &pnsocks.hlist) { > + if (!net_eq(net, sock_net(sknode))) > + continue; > + if (!pos) > + return sknode; > + pos--; > + } > + return NULL; > +} > + > +static struct sock *pn_sock_get_next(struct seq_file *seq, struct sock *sk) > +{ > + struct net *net = seq_file_net(seq); > + > + do > + sk = sk_next(sk); > + while (sk && !net_eq(net, sock_net(sk))); > + > + return sk; > +} > + > +static void *pn_sock_seq_start(struct seq_file *seq, loff_t *pos) > + __acquires(pnsocks.lock) > +{ > + spin_lock_bh(&pnsocks.lock); > + return *pos ? pn_sock_get_idx(seq, *pos - 1) : SEQ_START_TOKEN; > +} > + > +static void *pn_sock_seq_next(struct seq_file *seq, void *v, loff_t *pos) > +{ > + struct sock *sk; > + > + if (v == SEQ_START_TOKEN) > + sk = pn_sock_get_idx(seq, 0); > + else > + sk = pn_sock_get_next(seq, v); > + (*pos)++; > + return sk; > +} > + > +static void pn_sock_seq_stop(struct seq_file *seq, void *v) > + __releases(pnsocks.lock) > +{ > + spin_unlock_bh(&pnsocks.lock); > +} > + > +static int pn_sock_seq_show(struct seq_file *seq, void *v) > +{ > + int len; > + > + if (v == SEQ_START_TOKEN) > + seq_printf(seq, "%s%n", "pt loc rem rs st tx_queue rx_queue " > + " uid inode ref pointer drops", &len); > + else { > + struct sock *sk = v; > + struct pn_sock *pn = pn_sk(sk); > + > + seq_printf(seq, "%2d %04X:%04X:%02X %02X %08X:%08X %5d %lu " > + "%d %p %d%n", > + sk->sk_protocol, pn->sobject, 0, pn->resource, > + sk->sk_state, > + atomic_read(&sk->sk_wmem_alloc), > + atomic_read(&sk->sk_rmem_alloc), > + sock_i_uid(sk), sock_i_ino(sk), > + atomic_read(&sk->sk_refcnt), sk, > + atomic_read(&sk->sk_drops), &len); > + } > + seq_printf(seq, "%*s\n", 127 - len, ""); > + return 0; > +} The more appropriate place for this would be debugfs. Regards Marcel -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Rémi Denis-Courmont a écrit : > From: Rémi Denis-Courmont <remi.denis-courmont@nokia.com> > > +static int pn_sock_seq_show(struct seq_file *seq, void *v) > +{ > + int len; > + > + if (v == SEQ_START_TOKEN) > + seq_printf(seq, "%s%n", "pt loc rem rs st tx_queue rx_queue " > + " uid inode ref pointer drops", &len); > + else { > + struct sock *sk = v; > + struct pn_sock *pn = pn_sk(sk); > + > + seq_printf(seq, "%2d %04X:%04X:%02X %02X %08X:%08X %5d %lu " > + "%d %p %d%n", > + sk->sk_protocol, pn->sobject, 0, pn->resource, > + sk->sk_state, > + atomic_read(&sk->sk_wmem_alloc), > + atomic_read(&sk->sk_rmem_alloc), Please use sk_wmem_alloc_get(sk) and sk_rmem_alloc_get(sk) > + sock_i_uid(sk), sock_i_ino(sk), > + atomic_read(&sk->sk_refcnt), sk, > + atomic_read(&sk->sk_drops), &len); > + } > + seq_printf(seq, "%*s\n", 127 - len, ""); > + return 0; > +} -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday 20 July 2009 11:36:55 ext Marcel Holtmann wrote: > Hi Remi, > > > From: Rémi Denis-Courmont <remi.denis-courmont@nokia.com> > > isn't there are proper explaining commit message missing here? AFAIK, a one-liners stick to the Subject line. (I use explicit From: due to my broken Microsoft-provided MTA).
Hi Remi, > > > From: Rémi Denis-Courmont <remi.denis-courmont@nokia.com> > > > > isn't there are proper explaining commit message missing here? > > AFAIK, a one-liners stick to the Subject line. > > (I use explicit From: due to my broken Microsoft-provided MTA). the From: is not the problem here. However it would be nice to have a description of the change. Especially details like this is for debugging or this is a public API or etc. Regards Marcel -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday 20 July 2009 12:34:47 ext Marcel Holtmann wrote: > Hi Remi, > > > > > From: Rémi Denis-Courmont <remi.denis-courmont@nokia.com> > > > > > > isn't there are proper explaining commit message missing here? > > > > AFAIK, a one-liners stick to the Subject line. > > > > (I use explicit From: due to my broken Microsoft-provided MTA). > > the From: is not the problem here. However it would be nice to have a > description of the change. Especially details like this is for debugging > or this is a public API or etc. It's just like most network protocols exposing their sockets list in /proc/net. Debugging/monitoring indeed.
Hi Remi, > > > > > From: Rémi Denis-Courmont <remi.denis-courmont@nokia.com> > > > > > > > > isn't there are proper explaining commit message missing here? > > > > > > AFAIK, a one-liners stick to the Subject line. > > > > > > (I use explicit From: due to my broken Microsoft-provided MTA). > > > > the From: is not the problem here. However it would be nice to have a > > description of the change. Especially details like this is for debugging > > or this is a public API or etc. > > It's just like most network protocols exposing their sockets list in > /proc/net. Debugging/monitoring indeed. I think that for new protocols, we should not do this anymore and just use debugfs. Since that is exactly its job. Regards Marcel -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Marcel Holtmann a écrit : > Hi Remi, > >>>>>> From: Rémi Denis-Courmont <remi.denis-courmont@nokia.com> >>>>> isn't there are proper explaining commit message missing here? >>>> AFAIK, a one-liners stick to the Subject line. >>>> >>>> (I use explicit From: due to my broken Microsoft-provided MTA). >>> the From: is not the problem here. However it would be nice to have a >>> description of the change. Especially details like this is for debugging >>> or this is a public API or etc. >> It's just like most network protocols exposing their sockets list in >> /proc/net. Debugging/monitoring indeed. > > I think that for new protocols, we should not do this anymore and just > use debugfs. Since that is exactly its job. netstat uses /proc/net iproute2 uses netlink Now you suggest adding debugfs support ? What a mess... -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday 20 July 2009 13:21:01 ext Marcel Holtmann wrote: > > It's just like most network protocols exposing their sockets list in > > /proc/net. Debugging/monitoring indeed. > > I think that for new protocols, we should not do this anymore and just > use debugfs. Since that is exactly its job. Requiring debugfs for something like 'netstat' sounds restrictive to me...
From: "Rémi Denis-Courmont" <remi.denis-courmont@nokia.com> Date: Mon, 20 Jul 2009 13:32:04 +0300 > On Monday 20 July 2009 13:21:01 ext Marcel Holtmann wrote: >> > It's just like most network protocols exposing their sockets list in >> > /proc/net. Debugging/monitoring indeed. >> >> I think that for new protocols, we should not do this anymore and just >> use debugfs. Since that is exactly its job. > > Requiring debugfs for something like 'netstat' sounds restrictive to me... Me too. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Dave, > > On Monday 20 July 2009 13:21:01 ext Marcel Holtmann wrote: > >> > It's just like most network protocols exposing their sockets list in > >> > /proc/net. Debugging/monitoring indeed. > >> > >> I think that for new protocols, we should not do this anymore and just > >> use debugfs. Since that is exactly its job. > > > > Requiring debugfs for something like 'netstat' sounds restrictive to me... > > Me too. hold on, what has netstat to do with Phonet support? Does netstat now support Phonet or what are we trying to achieve here? And if we are talking about debugging information, then they should go into debugfs and not /proc. At least for new protocols. Regards Marcel -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday 20 July 2009 15:07:30 ext Marcel Holtmann wrote: > Hi Dave, > > > > On Monday 20 July 2009 13:21:01 ext Marcel Holtmann wrote: > > >> > It's just like most network protocols exposing their sockets list in > > >> > /proc/net. Debugging/monitoring indeed. > > >> > > >> I think that for new protocols, we should not do this anymore and just > > >> use debugfs. Since that is exactly its job. > > > > > > Requiring debugfs for something like 'netstat' sounds restrictive to > > > me... > > > > Me too. > > hold on, what has netstat to do with Phonet support? First, I said "*like* netstat". Second, *net*stat has at least as much to do with Pho*net* than it has with Unix sockets. I'm interested in monitoring userland usage of Phonet sockets, not so much in debugging the kernel-mode Phonet stack. I am not convinced that debugfs is the right tool at all here - regardless whether we extend netstat or write a Phonet-specific tool. > Does netstat now support Phonet or what are we trying to achieve here? There is currently no Phonet socket monitoring hook. I guess you refer to backward compatibility, which -I would agree- is a non-issue.
Hi Remi, > > > > On Monday 20 July 2009 13:21:01 ext Marcel Holtmann wrote: > > > >> > It's just like most network protocols exposing their sockets list in > > > >> > /proc/net. Debugging/monitoring indeed. > > > >> > > > >> I think that for new protocols, we should not do this anymore and just > > > >> use debugfs. Since that is exactly its job. > > > > > > > > Requiring debugfs for something like 'netstat' sounds restrictive to > > > > me... > > > > > > Me too. > > > > hold on, what has netstat to do with Phonet support? > > First, I said "*like* netstat". Second, *net*stat has at least as much to do > with Pho*net* than it has with Unix sockets. I'm interested in monitoring > userland usage of Phonet sockets, not so much in debugging the kernel-mode > Phonet stack. I am not convinced that debugfs is the right tool at all here - > regardless whether we extend netstat or write a Phonet-specific tool. in the end it is up to Dave, however I would put these things into debugfs. Note that even something like mac80211 still provides a wireless file here while most debugging is nowaydays in debugfs and it might be better removed. Regards Marcel -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Marcel Holtmann <marcel@holtmann.org> Date: Mon, 20 Jul 2009 14:07:30 +0200 > And if we are talking about debugging information, then they should go > into debugfs and not /proc. At least for new protocols. Getting a list of active sockets is not, and never will be, "debugging" information. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Marcel Holtmann <marcel@holtmann.org> Date: Mon, 20 Jul 2009 15:00:40 +0200 > in the end it is up to Dave, however I would put these things into > debugfs. You're being rediculious. Listing sockets is a standard facility, not a debugging thing. Just like listing processes. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Dave, > > in the end it is up to Dave, however I would put these things into > > debugfs. > > You're being rediculious. > > Listing sockets is a standard facility, not a debugging thing. > Just like listing processes. fair enough. I would still have used debugfs ;) Regards Marcel -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Em Mon, Jul 20, 2009 at 12:31:41PM +0200, Eric Dumazet escreveu: > Marcel Holtmann a écrit : > > Hi Remi, > > > >>>>>> From: Rémi Denis-Courmont <remi.denis-courmont@nokia.com> > >>>>> isn't there are proper explaining commit message missing here? > >>>> AFAIK, a one-liners stick to the Subject line. > >>>> > >>>> (I use explicit From: due to my broken Microsoft-provided MTA). > >>> the From: is not the problem here. However it would be nice to have a > >>> description of the change. Especially details like this is for debugging > >>> or this is a public API or etc. > >> It's just like most network protocols exposing their sockets list in > >> /proc/net. Debugging/monitoring indeed. > > > > I think that for new protocols, we should not do this anymore and just > > use debugfs. Since that is exactly its job. > > netstat uses /proc/net > > iproute2 uses netlink > > Now you suggest adding debugfs support ? > > What a mess... Exactly, the proper way to do that is to have a base class for inet_diag, net_diag, from where phone would be derived. But yeah, that requires more work than cut'n'pasting from the old, /proc based way of doing things 8-) - Arnaldo -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/net/phonet/pn_dev.h b/include/net/phonet/pn_dev.h index 29d1267..44c923c 100644 --- a/include/net/phonet/pn_dev.h +++ b/include/net/phonet/pn_dev.h @@ -49,4 +49,6 @@ void phonet_address_notify(int event, struct net_device *dev, u8 addr); #define PN_NO_ADDR 0xff +extern const struct file_operations pn_sock_seq_fops; + #endif diff --git a/net/phonet/pn_dev.c b/net/phonet/pn_dev.c index b0d6ddd..5107b79 100644 --- a/net/phonet/pn_dev.c +++ b/net/phonet/pn_dev.c @@ -218,6 +218,11 @@ static int phonet_init_net(struct net *net) if (!pnn) return -ENOMEM; + if (!proc_net_fops_create(net, "phonet", 0, &pn_sock_seq_fops)) { + kfree(pnn); + return -ENOMEM; + } + INIT_LIST_HEAD(&pnn->pndevs.list); spin_lock_init(&pnn->pndevs.lock); net_assign_generic(net, phonet_net_id, pnn); @@ -233,6 +238,8 @@ static void phonet_exit_net(struct net *net) for_each_netdev(net, dev) phonet_device_destroy(dev); rtnl_unlock(); + + proc_net_remove(net, "phonet"); kfree(pnn); } diff --git a/net/phonet/socket.c b/net/phonet/socket.c index ada2a35..2a99851 100644 --- a/net/phonet/socket.c +++ b/net/phonet/socket.c @@ -412,3 +412,100 @@ found: return 0; } EXPORT_SYMBOL(pn_sock_get_port); + +static struct sock *pn_sock_get_idx(struct seq_file *seq, loff_t pos) +{ + struct net *net = seq_file_net(seq); + struct hlist_node *node; + struct sock *sknode; + + sk_for_each(sknode, node, &pnsocks.hlist) { + if (!net_eq(net, sock_net(sknode))) + continue; + if (!pos) + return sknode; + pos--; + } + return NULL; +} + +static struct sock *pn_sock_get_next(struct seq_file *seq, struct sock *sk) +{ + struct net *net = seq_file_net(seq); + + do + sk = sk_next(sk); + while (sk && !net_eq(net, sock_net(sk))); + + return sk; +} + +static void *pn_sock_seq_start(struct seq_file *seq, loff_t *pos) + __acquires(pnsocks.lock) +{ + spin_lock_bh(&pnsocks.lock); + return *pos ? pn_sock_get_idx(seq, *pos - 1) : SEQ_START_TOKEN; +} + +static void *pn_sock_seq_next(struct seq_file *seq, void *v, loff_t *pos) +{ + struct sock *sk; + + if (v == SEQ_START_TOKEN) + sk = pn_sock_get_idx(seq, 0); + else + sk = pn_sock_get_next(seq, v); + (*pos)++; + return sk; +} + +static void pn_sock_seq_stop(struct seq_file *seq, void *v) + __releases(pnsocks.lock) +{ + spin_unlock_bh(&pnsocks.lock); +} + +static int pn_sock_seq_show(struct seq_file *seq, void *v) +{ + int len; + + if (v == SEQ_START_TOKEN) + seq_printf(seq, "%s%n", "pt loc rem rs st tx_queue rx_queue " + " uid inode ref pointer drops", &len); + else { + struct sock *sk = v; + struct pn_sock *pn = pn_sk(sk); + + seq_printf(seq, "%2d %04X:%04X:%02X %02X %08X:%08X %5d %lu " + "%d %p %d%n", + sk->sk_protocol, pn->sobject, 0, pn->resource, + sk->sk_state, + atomic_read(&sk->sk_wmem_alloc), + atomic_read(&sk->sk_rmem_alloc), + sock_i_uid(sk), sock_i_ino(sk), + atomic_read(&sk->sk_refcnt), sk, + atomic_read(&sk->sk_drops), &len); + } + seq_printf(seq, "%*s\n", 127 - len, ""); + return 0; +} + +static const struct seq_operations pn_sock_seq_ops = { + .start = pn_sock_seq_start, + .next = pn_sock_seq_next, + .stop = pn_sock_seq_stop, + .show = pn_sock_seq_show, +}; + +static int pn_sock_open(struct inode *inode, struct file *file) +{ + return seq_open(file, &pn_sock_seq_ops); +} + +const struct file_operations pn_sock_seq_fops = { + .owner = THIS_MODULE, + .open = pn_sock_open, + .read = seq_read, + .llseek = seq_lseek, + .release = seq_release, +};