Patchwork [04/10] batman-adv: export compatibility version via debugfs

login
register
mail settings
Submitter Antonio Quartulli
Date Nov. 19, 2012, 8:24 a.m.
Message ID <1353313451-2930-5-git-send-email-ordex@autistici.org>
Download mbox | patch
Permalink /patch/199943/
State Rejected
Delegated to: David Miller
Headers show

Comments

Antonio Quartulli - Nov. 19, 2012, 8:24 a.m.
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.

Reported-by: Moritz Warning <moritzwarning@web.de>
Signed-off-by: Antonio Quartulli <ordex@autistici.org>
Signed-off-by: Marek Lindner <lindner_marek@yahoo.de>
---
 net/batman-adv/debugfs.c | 12 ++++++++++++
 net/batman-adv/main.c    | 12 ++++++++++++
 net/batman-adv/main.h    |  1 +
 3 files changed, 25 insertions(+)
Ben Hutchings - Nov. 19, 2012, 6:52 p.m.
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.
David Miller - Nov. 19, 2012, 7:10 p.m.
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
Sven Eckelmann - Nov. 19, 2012, 7:29 p.m.
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
Ben Hutchings - Nov. 19, 2012, 7:40 p.m.
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.
David Miller - Nov. 19, 2012, 7:47 p.m.
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
Sven Eckelmann - Nov. 19, 2012, 8:25 p.m.
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
Antonio Quartulli - Nov. 19, 2012, 9:09 p.m.
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,

Patch

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