Hi Wolfgang On Thu, Nov 26, 2009 at 02:06:42PM +0100, Wolfgang Grandegger wrote: > Hi Fu, > > Luotao Fu wrote: > > Hi, > > > > On Thu, Nov 26, 2009 at 01:09:55PM +0100, Wolfgang Grandegger wrote: > > Wolfgang Grandegger wrote: <snip> > > > > 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. >
all right, agreed, will change this. > >> 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 > set_link was supposed to be the part with if_up/down, while do_set_nl_link does the real work. Since the if_up/down stuffs are to be exported separately, I will surely replace the now obsolete set_link code with do_set_nl_link. > 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. > If I just knew why... ;-). I started this for a customer application and began with canutils later. At that time I should have merged it into canutils directly....missed this though...what ever. Merging the stuffs is apparently any way on the scheduele, we probably can do this for some coming releases. cheers Fu -- Pengutronix e.K. | Dipl.-Ing. Luotao Fu | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
signature.asc
Description: Digital signature
_______________________________________________ Socketcan-users mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-users
