Re: [B.A.T.M.A.N.] [PATCH 13/26] staging:batman-adv: convert multiple /proc files to use sysfs

[Simon Wunderlich <simon.wunderlich@s2003.tu-chemnitz.de>]

[-- Attachment #1: Type: text/plain, Size: 4219 bytes --]

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
> 
> 
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]