Re: [Tipc-discussion] New file release tipc-1.3.10

[Jon M. <jon...@er...>]

Thank you for the comments. Most of the problems you point out are
omissions that are relatively easy to fix, as far as I can see, but some
require more explanation, see below. Some constructive suggestions
would also be appreciated, especially in the cases where you seem
to have particularly dislike for the code.

/jon


Stephen Hemminger wrote:

>Overall TIPC 1.3 comments:
>
>* README seems out of date
>
>* Configuration by module parameters is bad because in most
>  standard environments, module loading is done automatically.
>
What do you suggest ? With manual insmod loading this very practical.

>
>* Need 2.6 only, no standalone build environment
>  No extra CFLAGS in Makefile please
>
It was never the intention that the current directory structure and make
support at SF would be the one eventually integrated into the kernel. But
we do need a standalone build support, so I guess we need to keep the
code in two different structures in the future.

>
>* Hard coding clusters, zones, nodes maximum makes it impossible
>  for distro vendors to ship a standard kernel.
>
>* having own caching allocator, if you insist on caching
>  then use kmem_cache_alloc.
>
The only caching we do is for ready-built sk_buffs. I don't see how this 
can
be achieved with kmem_cache_alloc, but maybe I am missing something ?

>
>* it links to network devices, but does not handle up/down/unregister
>  notifications.  THIS IS A SHOWSTOPPER
>
>* holds references to network devices without holding reference counts.
>  THIS IS A SHOWSTOPPER
>
No problem. Something I have missed.

>
>* why own version of wait_event_interruptible?
>
I guess you mean wait_event_interruptible_timeout. This did not exist in
the versions of 2.4 that we support, so it is just legacy. This has been
fixed in all files except proc.c, which is anyway to be removed, -see
previous mail.

>
>* excessive use of /proc to provide interface.  /proc is okay
>  as a statistical interface, not a control channel!
>
See above.

>
>* your socket locking is a mess. What is wrong with lock_sock()?
>  
>
>* you invent your own socket queues, there are standard macros
>  to do that.
>
>* get rid of all the macros in tipc_adapt.h
>	err, warn, info, dbg, ...
>  use the standard macros in kernel.h
>
The Linux standard macros are not sufficent. Neither is Ethereal.
This is the most important tool I have for trouble shooting.
Try to trace a lost packet when the loss is detected one minute
and 2 million packets later, and you will understand what I mean.

>
>* hardcoding device names 'eth0,eth1,eth2,...' in code and
>  configuration is unacceptable.  What about other media types
>  and netdevice names can be anything the administrator wants.
>
Yes, I have been aware of that, but strings are no good either. Any
suggestions?

>
>tipc.h
>- cplusplus support not usually done in linux header files
>
>- get rid of typedef's for tipc_net_addr_t, tipc_ref_t
>  kernel standard is to not use typedef's if possible
>
>- tipc_addr,zone,cluster,node
>	using inline instead would gain type checking
>
>- no IPV6 support, would probably be a show stopper for many people
>
It is in the pipe.

>
>addr.c
>* global variable, buffers's waiting to be overwritten, ugh.
>
>
>* k_time_ms == jiffies_to_clock_t
>
>* The whole TIPC_MAGIC in header crap looks a trap waiting
>  to happen
>
I guess you refer to the mangement interface header. We need some
minimum level of security, apart from disabling the whole functionality,
which was meant to be configurable, and this was the simple solution
I came up with.  If you literally mean this may cause a trap, I don't
understand what you mean. A wrong "magic" will cause a command
rejection, and that is it.

Please elaborate "crap".  I realize that the management interface
is not perfect, but the fact that you don't understand it in five
seconds does not either imply that it is all wrong.

>
>That is enough for now, that is just what I found in a 15min
>quick look.  If Viro saw it, he wouldn't be as nice ;-)
>  
>