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?
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.
725-875: please remove. You're undoing the work that was done for
CR 4784660.
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.)
52: why is this necessary? Does comparing by name even work
reliably? Doesn't the existing IFF_LOOPBACK or perhaps
INADDR_LOOPBACK tell you what you need to know?
54-58: the comment is true, but this variable isn't really needed
anyway. The only use if it is to copy into a structure that's
initialized by bzero -- and thus is already set to IN6ADDR_ANY.
90: shouldn't this be a static symbol? It looks like it_ops is set
by idm_so_init.
91-120: nit (style): I wouldn't use "&" with the names of functions;
they're already implicitly pointers.
149: nit: use ANSI (void) here.
156, et al: static?
159-161: initializations are not used.
166-203: this doesn't look healthy to me. Why wouldn't we fix
solookup (perhaps by creating a 'solookup_k' variant) rather than
inlining this code here?
173: coding standard: not boolean; compare against zero explicitly.
184: coding standard: missing blank line after declaration.
215: this isn't needed; the argument is used.
243: design: how do you deal with interfaces that get
added/removed/modified? I don't see a routing socket here.
247,259: stray blank lines amid declarations.
267-268: what is this about?
280-281,287-288: consider having a separate clean-up target, so you
can do "goto" rather than copying code.
293: warning: 'numifs' is not guaranteed to be accurate at all. The
call at 301 will fail if this code is run while plumbing operations
are active.
332: use switch (ss->ss_family), perhaps?
388: design: this function signature doesn't make sense to me. How
do you return the length of the received data to the caller?
Shouldn't the caller care how many bytes were written to 'msg'?
467: sending only some of the data is an error?
475-520: it looks to me like it'd be worthwhile to combine the
sosend variants. They exist in user space for portability reasons,
but it's unclear what good they do in the kernel.
560-564: how is receiving less data than requested an "error?"
572,598,625: this isn't needed; these functions use their arguments
just fine.
581-588,595,607-614,621,634-643,650: please remove; this is
appropriate material for a bugster CR, not in the gate.
654: nit: argument is const.
656-660: a bit strange; I would have expected this instead:
return ((ptr[0] << 16) | (ptr[1] << 8) | ptr[2]);
678,783,810,862,et al: cast not needed; "void *" is always
compatible.
713: this comment doesn't say much.
767: don't compare boolean against B_TRUE/B_FALSE constants.
768: comment doesn't appear to be correct; sobind doesn't start any
connection.
884: since you're going to call kmem_free anyway, I suggest you
don't zero out this pointer. Leaving it non-NULL will give a
helpful hint to someone looking at a dump, and (given the context of
the pending kmem_free) can't possibly be harmful here.
908: what does the mutex protect? The code at 903-906 modifies the
contents of 'ic' without holding that lock. (Is it just the
thread-running flags?)
934,936: what does this lock do?
957-966: this doesn't look right to me. I strongly suggest that you
contact the TX team ([EMAIL PROTECTED] or
[EMAIL PROTECTED]) to discuss the meaning of this
flag. Only MAC-aware applications ought to be setting this, and I
don't see where this code handles mandatory access controls at all.
1006,1010: what is this about? Why would you want "either" IPv6 or
IPv4? Shouldn't you either know exactly which one you want, or need
to construct *both*?
1011: c-style: operator goes at end of previous line. Break into
two separate statements (assignment and if-test) if that's not
convenient.
1051-1054,1062-1066: consider gathering these together.
1088-1091: unclear what this lock protects.
1093-1103: this is a bit ugly; would be slightly better as a new
function in sockstr.c.
1151-1155,1161-1165: could be gathered.
1259: not needed; arguments are used.
(My interest waned at about this point.)
iscsit_authglue.c
Why is this file needed? Can't those functions be called directly?
--
James Carlson, Solaris Networking <[EMAIL PROTECTED]>
Sun Microsystems / 35 Network Drive 71.232W Vox +1 781 442 2084
MS UBUR02-212 / Burlington MA 01803-2757 42.496N Fax +1 781 442 1677
_______________________________________________
networking-discuss mailing list
[email protected]
