I have reviewed all the files except td_iscsi.c. I have reviewed half
of the file and finish the rest tomorrow.
General: Thre seems like an overzealous use of MAXNAMELEN. A quick
scan of dkio(7I) indicates that disknames will not be bigger than 32
chars 2*DK_DEVLEN. MAXNAMELEN is 256.
Does scsiip really need this size or can it be INET_ADDRSTRLEN ?
auto_install.c: (optional)
648-660: Since all you are doing here is putting a specific string in
file, use of popen to do this will make it cleaner.
auto_install.h:
204-213 (and the lines above). Also above the disk vendor (not part
your changes) I am assuming is filled out from the inquiry string. If
so scsi_inquiry structure allocates only 24 bytes for vendor[8] and
product[16]
auto_parse.c:
193 and others:
if p is NULL, wouldn't this be an error. If so it will not be
caught. Similar issues with other parsed values.
auto_td.c:
482: Just to be sure..when "if" statement fails,
diskiscsiname and diskiscsiip are both either
defined or undefined. A comment to that effect would
be useful.
492: I would have thought that rootpath[MAXNAMELEN] would be
sufficient (i.e. 256)
instead of MAXPATHLEN which is 1024.
539: strncmp(rootpath, "iscsi:", strlen("iscsi:")) would be
preferable.
544- 568: parsing with strtok might be a better option.
td_api.h:
54 -57: Please delete these lines. To designate iscsi specific
enums state that as part of the comment
eg:
TD_E_NOT_FOUND, /* iscsi specific */
TD_E_LUN_NOT_FOUND, /* iscsi specific. LUN not found or a
non-specified LUN found */
td_iscsi.c:
I am a bit confused by the values chosen for CHAP. In struct
iSCSI_ibtf_target chap_name_len
and chap_secret_len are both ushort_t. Given this, would 512 be
big enough for name ?
58: comment for failure does not match the actual value being
returned.
79: Can lhbaList->oidCount be zero since one can get here with
lhbaList != NULL and should
this be marked as an error ?
99: Why is targetIpaddr[128] and not a predefined value
[INET_ADDRSTRLEN] or something
appropriate ? I noticed below that this string is used
to contain IP address and port, so
changing the the name to be something appropriate would
be helpful.
122-124: /* Currently iscsi driver doesn't allow one to add a
duplicate static configuration. So
we try to get the existing static configuration first
and then do a comparision....
*/
A comment as to what mbstowcs does would be useful.
421: char devnam[MAXNAMELEN] --- overkill ?
William Schumann wrote:
> quick iSCSI boot status update:
>
> Am doing final unit testing on OpenSolaris code, and would like to put
> it up for review on Wednesday.
>
> Current webrev: http://cr.opensolaris.org/~wmsch/bug-6590/
>
> Working on text describing manual network configuration to eliminate
> NWAM.
>
> RFC 4173 Bootstrapping Clients using the Internet Small Computer
> System Interface (iSCSI) Protocol
> http://tools.ietf.org/html/rfc4173
> shows technique for DHCP server to supply IP address to iSCSI boot
> target using DHCP Rootpath (it works on Intel e1000g0). dhcpinfo can
> then be used to get the iSCSI boot parameters from the DHCP server:
> "dhcpinfo Rootpath". Still trying to figure out best way to do this
> to handle x86 2-NIC case. Interface must first be set as DHCP -
> "ifconfig <if> dhcp". Maybe DHCP Rootpath can be configured for PXE
> interface...
>
> Unusual behavior: iSCSI boot network interface not listed as 'DHCP' in
> ifconfig. I think that this might be causing some confusion in the
> DHCP server, which periodically marks the address as "unusable". I
> see "ICMP ECHO reply to OFFER candidate: <IP> disabling" in system
> log. From http://docs.sun.com/app/docs/doc/806-0916/6ja8539b0?a=view
>>
>> ICMP ECHO reply to OFFER candidate: n.n.n.n, disabling
>>
>> The IP address being considered for offering to a DHCP client is
>> already in use. This might occur if more than one DHCP server owns
>> the address, or if an address was manually configured for a non-DHCP
>> network client.
>> Determine the proper ownership of the address and correct either the
>> DHCP server database or the host's network configuration.
>>
> Initial reboot with nwam (phase 0.n) service up by default works, but
> DNS is not correctly configured until second reboot. Can be fixed
> manually. Also might be due to the interface not looking like a DHCP
> interface.
>
> Did some NWAM phase 1 testing. Looks OK, with some minor service
> startup problems possibly due to incorrect dependencies or timing issues.
>
> Back on Wednesday,
> William
