Hello Andrew,
thank you very much for your suggetions. I'm currently sitting here with Marek and Sven in Berlin and we were discussing your suggestions. Our conclusion is:
* We could write and read "enable/disable" only from the respective files (like aggregation). In this case we don't need the word "enabled" in the name however, as we already use it to set the status, so we would rather skip it. * we can rename aggregate_ogm to aggregate_ogms, i personally don't like CAPS in these names. ;) * We don't like the _msecs suggestion. It does not seem very common with other kernel driver configurations. We could not find any other files in batman-adv which are configured with a unit, so we would suggest to keep it like it is. * As you suggest, we should move the hints/verbose stuff from the module into batctl. There we could explain the possible commands, or also show the units. Then we would keep the sysfs "minimal". Also the "Usage" kernel change is a good idea imho. * The bonding and gateway features should be discussed at another time, i'll have some more changes in bonding soon, not sure about Marek gateway plans. These are 0.3 features and it does not hurt the maint version if we change these a little bit later.
best regards, Simon
On Thu, May 13, 2010 at 01:45:07PM +0200, Andrew Lunn wrote:
The rest can be in sysfs?:
- /sys/class/net/<iface>/batman-adv/mesh_iface
- /sys/class/net/<iface>/batman-adv/iface_status
- /sys/class/net/<mesh_iface>/mesh/aggregate_ogm
- /sys/class/net/<mesh_iface>/mesh/orig_interval
- /sys/class/net/<mesh_iface>/mesh/vis_mode
As far as i can see, these are all single values. I would however suggest a few changes, to make it a bit clearer what the values are and what units are used.
- /sys/class/net/<mesh_iface>/mesh/enable_aggregated_OGMs
- /sys/class/net/<mesh_iface>/mesh/enable_vis_data
- /sys/class/net/<mesh_iface>/mesh/orig_interval_msec
"enable" suggests the control can be enabled/disabled. It gives the user a hint to try Boolean values: 0/1, true/false, enable/disable. Our current code accepts 0/1 and enable/disable, so the user has a good chance of guessing correct.
enable_aggregated_OGMs is more grammatically correct. We are aggregating multiple OGMs into one big OGM packet.
"_msec" gives the user the units to use for the orig_interval.
I would also change the values read from the files. Currently it is not a simple value. It is a value, plus some text which batctl uses, or hints for the user when using cat/echo. eg:
return sprintf(buff, "status: %s\ncommands: enable, disable, 0, 1\n", aggr_status == 0 ? "disabled" : "enabled");
IMHO, we should just output 0/1, true/false/ enabled/disabled. I don't really care which, but is should be only a single word.
For me, batctl parsing the hint the kernel returns does not make much sense. Once this kernel ABI is defined, it will never change, ever. Since it will never change, we can hard code the hint into batctl. That then brings the syfs files in line with the single value requirement.
What we loose is the hint for people using cat/echo, not batctl. How big a problem is this? I don't think it is a big problem. When you cat the file you see "enabled", go guessing "disabled" is not too hard. We already return -EINVAL when in invalid value is tried and log a message to the kernel log. It would be easy to extend this kernel log message with: "Usage: enable | disable. For orig_interval_msec, i would extend the kernel log message with: "Usage: integer between %d and %d", (JITTER * 2)+1, 0x00ffffff.
That i think covers all the mainline configuration options. We have a few more in trunk. I would suggest a rename to be consistent:
enable_bonding
gateway is a bigger problem. I think it needs splitting up into a number of files, but i don't know the code well enough, and could not quickly find a good description of the options.
Andrew