Hi Fu, Luotao Fu wrote: > Hi, > > On Thu, Nov 26, 2009 at 01:09:55PM +0100, Wolfgang Grandegger wrote: >> Wolfgang Grandegger wrote: >>> Hi Fu, >>> >>> Luotao Fu wrote: >>>> Hi Vlasdiv, >>>> >>>> On Wed, Nov 25, 2009 at 06:30:07PM +0100, [email protected] wrote: > <snip> >>>>> Such a user space component can make things really easier for me, as a >>>>> CANopen SW maintainer under SocketCAN. If I can use someuser space >>>>> library >>>>> "libsocketcan" to: >>>>> >>>>> - start/stop CAN >>>> yeah, start with >>>> scan_do_restart(const char* name) >>>> stop alias driving down the interface is there but used as static, I'll >>>> make a wrapper with proper name for this. >>> Hm, the name "restart" is somehow reserved for bus-off recovery. Even if >>> the name is not well chosen, we should use another name for down/up, >>> e.g. scan_do_down_up() to make that clear, if we need that at all. Or >>> have I missed something? >> Just looked to the code. scan_do_restart() just does the bus-off >> recovery if appropriate. But I realized that the scan_set_* functions do >> stop the device before setting the property. That's dangerous and error >> prune and therefore we did not allow it in the kernel. It's up to the >> user/application to handle up/down properly. > > Certain things like set_bitrate or ctrlmode are anyway not allowed while > the device is up and running. I think that it's a question of policy, > that we if we take care of if_up down in the library or let the user do > it in the application. imho if an application is allowed to change bitrate > while there're communication running, the application is all the way > errnoues, it won't help much to put the if_up/down into the application. > I personally prefer to hide these things from user. Since the usage is > this way much simpler. Another possiblitiy is to expose two kind of > _set functions, one is in the way of do-it-yourself, where the library > relies on that the user has put off the device prioly. The other is the > way "we-do-it-for-you", as we have now.
I disagree. Setting up the device is usually part of the startup phase and it should be done before the device is actually started. It does help the user if he gets an error message because the device is already running, which can happen by accident. Be aware that the device might be accessed by more than one process/thread. If the user wants to change the device properties while the device is already up (on the fly) he can still call scan_stop, scan_set_bitrate, scan_start in his application, but it's then *his* responsibility to make sure that it does not harm. Hiding down/up is error prune, I believe and exactly for that reason it's not done by the kernel code any more. >> Furthermore, these >> functions seem to start the device even if it was not up before. >> > > good point, I didn't thought about this, will add a verification into > that. I also realized, that the scan_get_* functions use duplicated code. A "get_link" function, similar to "set_link", would make sense. > Thanks for reviewing Just started. One more quick remake: The library consists of just one source file, which is not even big: $ wc -l socketcan_netlink.c 679 socketcan_netlink.c That's really nice, but I wonder why you do not simply add it to the canutils. Wolfgang. _______________________________________________ Socketcan-users mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-users
