| Message ID | 1353313451-2930-5-git-send-email-ordex@autistici.org |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
On Mon, 2012-11-19 at 09:24 +0100, Antonio Quartulli wrote: > Different versions of the batman-adv module may use the same compatibility > version, but this is not understandable at runtime (the only way is to parse the > kernel log and fetch the batman-adv advertisement message on loading). The user > may want to know whether two nodes using different versions can communicate or > not. For this purpose the module has to export this value through debugfs. [...] I do hope that you're not planning to use unstable debugfs interfaces indefinitely. Ben.
From: Ben Hutchings <bhutchings@solarflare.com> Date: Mon, 19 Nov 2012 18:52:30 +0000 > On Mon, 2012-11-19 at 09:24 +0100, Antonio Quartulli wrote: >> Different versions of the batman-adv module may use the same compatibility >> version, but this is not understandable at runtime (the only way is to parse the >> kernel log and fetch the batman-adv advertisement message on loading). The user >> may want to know whether two nodes using different versions can communicate or >> not. For this purpose the module has to export this value through debugfs. > [...] > > I do hope that you're not planning to use unstable debugfs interfaces > indefinitely. I think this is a huge mess and I don't think I'm going to pull this series for that reason. Once we allow you to merge your protocol/facility/whatever into the main kernel tree, you have to be mindful of compatability in a very real way. These kinds of changes show that this isn't actually happening. So the only weapon I have against you doing more foolishly incompatable changes is to simply stop taking your submissions until things are corrected. -- 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 19 November 2012 18:52:30 Ben Hutchings wrote: > On Mon, 2012-11-19 at 09:24 +0100, Antonio Quartulli wrote: > > Different versions of the batman-adv module may use the same compatibility > > version, but this is not understandable at runtime (the only way is to > > parse the kernel log and fetch the batman-adv advertisement message on > > loading). The user may want to know whether two nodes using different > > versions can communicate or not. For this purpose the module has to > > export this value through debugfs. > [...] > > I do hope that you're not planning to use unstable debugfs interfaces > indefinitely. Why are you refering to the adding of an file as "incompatible" debugfs interface? I would call it additional information. And the rest of the commit message is talking about *possible* protocol incompatibilies of the network protocol (which caused this uncontrolled erruption of anger in a network maintainer). Kind regards, Sven
On Mon, 2012-11-19 at 20:29 +0100, Sven Eckelmann wrote: > On Monday 19 November 2012 18:52:30 Ben Hutchings wrote: > > On Mon, 2012-11-19 at 09:24 +0100, Antonio Quartulli wrote: > > > Different versions of the batman-adv module may use the same compatibility > > > version, but this is not understandable at runtime (the only way is to > > > parse the kernel log and fetch the batman-adv advertisement message on > > > loading). The user may want to know whether two nodes using different > > > versions can communicate or not. For this purpose the module has to > > > export this value through debugfs. > > [...] > > > > I do hope that you're not planning to use unstable debugfs interfaces > > indefinitely. > > Why are you refering to the adding of an file as "incompatible" debugfs > interface? I would call it additional information. And the rest of the commit > message is talking about *possible* protocol incompatibilies of the network > protocol (which caused this uncontrolled erruption of anger in a network > maintainer). I didn't say that. But debugfs tends to be exempted from expectations of interface stability or cleanliness (as it's for development, not production... right?). It's also an optional feature that might not be enabled at compile time or run-time. If BATMAN is ever going to be ready for general use it will need stable interfaces that are updated in a backward-compatible way and don't depend on debugfs. Ben.
From: Sven Eckelmann <sven@narfation.org> Date: Mon, 19 Nov 2012 20:29:07 +0100 > On Monday 19 November 2012 18:52:30 Ben Hutchings wrote: >> On Mon, 2012-11-19 at 09:24 +0100, Antonio Quartulli wrote: >> > Different versions of the batman-adv module may use the same compatibility >> > version, but this is not understandable at runtime (the only way is to >> > parse the kernel log and fetch the batman-adv advertisement message on >> > loading). The user may want to know whether two nodes using different >> > versions can communicate or not. For this purpose the module has to >> > export this value through debugfs. >> [...] >> >> I do hope that you're not planning to use unstable debugfs interfaces >> indefinitely. > > Why are you refering to the adding of an file as "incompatible" debugfs > interface? I would call it additional information. And the rest of the commit > message is talking about *possible* protocol incompatibilies of the network > protocol (which caused this uncontrolled erruption of anger in a network > maintainer). Debugfs files aren't where you store information that userspace actually uses in normal situations. It's for developer debug, to expose internals that otherwise have no business being part of the user visible interfaces. -- 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 19 November 2012 19:40:18 Ben Hutchings wrote: [...] > > > I do hope that you're not planning to use unstable debugfs interfaces > > > indefinitely. > > > > Why are you refering to the adding of an file as "incompatible" debugfs > > interface? I would call it additional information. And the rest of the > > commit message is talking about *possible* protocol incompatibilies of > > the network protocol (which caused this uncontrolled erruption of anger > > in a network maintainer). > > I didn't say that. But debugfs tends to be exempted from expectations > of interface stability or cleanliness (as it's for development, not > production... right?). It's also an optional feature that might not be > enabled at compile time or run-time. If BATMAN is ever going to be > ready for general use it will need stable interfaces that are updated in > a backward-compatible way and don't depend on debugfs. Ok, I thought this file was meant for an optional developer information (the interface to work with batman-adv is in /sys) and concluded too fast that you are questioning this change because it makes something "unstable" (sry, for using the wrong term before). Thanks, Sven
On Mon, Nov 19, 2012 at 02:47:45PM -0500, David Miller wrote: > From: Sven Eckelmann <sven@narfation.org> > Date: Mon, 19 Nov 2012 20:29:07 +0100 > > > On Monday 19 November 2012 18:52:30 Ben Hutchings wrote: > >> On Mon, 2012-11-19 at 09:24 +0100, Antonio Quartulli wrote: > >> > Different versions of the batman-adv module may use the same compatibility > >> > version, but this is not understandable at runtime (the only way is to > >> > parse the kernel log and fetch the batman-adv advertisement message on > >> > loading). The user may want to know whether two nodes using different > >> > versions can communicate or not. For this purpose the module has to > >> > export this value through debugfs. > >> [...] > >> > >> I do hope that you're not planning to use unstable debugfs interfaces > >> indefinitely. > > > > Why are you refering to the adding of an file as "incompatible" debugfs > > interface? I would call it additional information. And the rest of the commit > > message is talking about *possible* protocol incompatibilies of the network > > protocol (which caused this uncontrolled erruption of anger in a network > > maintainer). > > Debugfs files aren't where you store information that userspace actually > uses in normal situations. > > It's for developer debug, to expose internals that otherwise have no > business being part of the user visible interfaces. Ok, you are right. As you already stated, I do see two problems here: 1) debugfs not used properly (this information is not really for "devs") 2) this compatibility version export is like the claim: "compatibility is broken everyday". But actually this is not what we really wanted. This patch simply came from a feature request raised by a user of ours. As Sven pointed out, we have seen it just as "more information available for network debugging" and we (in particular me) didn't think about the fact that having such a patch was against the "stabilisation" process that we are following (we discussed about this process during the last pull request, so you already know that we are already going towards this direction). David, Ben, if you agree I would remove this patch and repost the pull request. Regards,
diff --git a/net/batman-adv/debugfs.c b/net/batman-adv/debugfs.c index 6f58ddd..ae79b19 100644 --- a/net/batman-adv/debugfs.c +++ b/net/batman-adv/debugfs.c @@ -245,6 +245,16 @@ static int batadv_algorithms_open(struct inode *inode, struct file *file) return single_open(file, batadv_algo_seq_print_text, NULL); } +/** + * batadv_compat_open - Prepare file handler for printing of the compat version + * @inode: inode which was opened + * @file: file handle to be initialized + */ +static int batadv_compat_open(struct inode *inode, struct file *file) +{ + return single_open(file, batadv_compat_seq_print_text, NULL); +} + static int batadv_originators_open(struct inode *inode, struct file *file) { struct net_device *net_dev = (struct net_device *)inode->i_private; @@ -327,9 +337,11 @@ struct batadv_debuginfo batadv_debuginfo_##_name = { \ * placed in the BATADV_DEBUGFS_SUBDIR subdirectory of debugfs */ static BATADV_DEBUGINFO(routing_algos, S_IRUGO, batadv_algorithms_open); +static BATADV_DEBUGINFO(compat_version, S_IRUGO, batadv_compat_open); static struct batadv_debuginfo *batadv_general_debuginfos[] = { &batadv_debuginfo_routing_algos, + &batadv_debuginfo_compat_version, NULL, }; diff --git a/net/batman-adv/main.c b/net/batman-adv/main.c index dc33a0c..70797de 100644 --- a/net/batman-adv/main.c +++ b/net/batman-adv/main.c @@ -420,6 +420,18 @@ int batadv_algo_seq_print_text(struct seq_file *seq, void *offset) return 0; } +/** + * batadv_compat_seq_print_text - print the compatibility version + * @seq: debugfs table seq_file struct + * @offset: not used + */ +int batadv_compat_seq_print_text(struct seq_file *seq, void *offset) +{ + seq_printf(seq, "%d\n", BATADV_COMPAT_VERSION); + + return 0; +} + static int batadv_param_set_ra(const char *val, const struct kernel_param *kp) { struct batadv_algo_ops *bat_algo_ops; diff --git a/net/batman-adv/main.h b/net/batman-adv/main.h index 8f149bb..3243189 100644 --- a/net/batman-adv/main.h +++ b/net/batman-adv/main.h @@ -174,6 +174,7 @@ void batadv_recv_handler_unregister(uint8_t packet_type); int batadv_algo_register(struct batadv_algo_ops *bat_algo_ops); int batadv_algo_select(struct batadv_priv *bat_priv, char *name); int batadv_algo_seq_print_text(struct seq_file *seq, void *offset); +int batadv_compat_seq_print_text(struct seq_file *seq, void *offset); /** * enum batadv_dbg_level - available log levels