Sanjay,
Unless otherwise indicated below as well as those indicated by Bing in
his response, I will implement your suggestions, coordinate changes with
Bing, and issue another webrev.
sanjay nadkarni (Laptop) wrote:
>
> 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.
Errors could occur in auto_parse_manifest.c during tag parsing that
would be ignored. However, these errors would be very unlikely, since
they would involve a basic breakdown in the embedded Python interpreter,
which has already been invoked to validate the manifest. Since fixing
this would involve many changes in three different modules, and since
this was not introduced by iSCSI, I would prefer to file a bug on this
and handle it later at a more appropriate priority.
>
>
> 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.
According to RFC 3720, the iSCSI name itself can be up to 223 bytes.
Add to that the maximum length of the IP address, port number, protocol,
and LUN, plus the "iscsi:" and separator characters and this is over 256
bytes.
>
> 539: strncmp(rootpath, "iscsi:", strlen("iscsi:")) would be
> preferable.
>
> 544- 568: parsing with strtok might be a better option.
These parameters are position-dependent, and strtok just jumps over
separators without tokens between them.
William
>
> 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
>
>
