James Carlson wrote: > Peter Dunlap writes: > >> http://cr.opensolaris.org/~pdunlap/iscsit-webrev/webrev/ >> > [...] > >> The code that interacts with sockfs is contained within this file: >> > > Why RADIUS in the kernel? This should only be a feature on the server > ('target') side of the protocol, so it shouldn't be a requirement in > order to boot. Couldn't connection authentication (a control-path > item) be done in user space rather than in kernel space? >
We originally started with a split user/kernel model, then decided to keep the code together for the sake of simplicity. It has since become clear that a substantial amount of code would be better off in user-space including (probably): Login Authentication iSNS Needless to say we are too close to integration to make such changes without a significant schedule hit but we plan to move some functionality into user-space in the future. Would it be OK to address this by filing an RFE to move the login, authentication code (including RADIUS) and iSNS into user space? Is there an existing user-space RADIUS facility we could leverage if we handled authentication in user-space? > idm_impl.c: > > (Not completely reviewed; I ended up here after seeing generic CRC > routines with unexpected "idm_" prefix on them in idm_so.c.) > > 524-722: consider using vmem_alloc to allocate IDs instead. > Is an RFE OK? (I know I'm saying that a lot. I will followup with a complete list of RFE's) > 725-875: please remove. You're undoing the work that was done for > CR 4784660. > I didn't know there was a generic implementation of this -- if we had known we definitely would have used it. We definitely don't want to deliver duplicate code (the first thing we did in our iSER initiator changeset was yank out the duplicate MD5 code). Because of our schedule constraints I'd like to file an RFE for this too (P2, P1 whatever is acceptable). > idm_so.c: > > nit: as minor issue, I'd avoid putting comments on the same line as > the #includes. It gets messy quickly and doesn't really help that > much. Either you need the include files or not. (And if not -- > 'dmake LINTFLAGS=-axsNlevel=2 lint' will tell you -- then yank 'em.) > Comments removed. I will try the LINTFLAGS thing this week to confirm there are no unneeded header files. We'll need some more time to respond to the remaining comments. -Peter _______________________________________________ networking-discuss mailing list [email protected]
