td_iscsi.c:
Inconsistent coding. In some functions the variables are upper and
lower case, in others they are lower case. To be consistent with other
C files, all variables should be lower names.
On 11/24/09 08:45 AM, William Schumann wrote:
> 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.
Sorry for not being clear. There are 2 issues here:
1. If you truly believe that p != NULL then why not change them to an
assert.
2. Sorry for not being clear. I was really asking where is the sematic
parsing occurring ?
>>
>>
>> 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.
Okay
>>
>> 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.
Okay
> 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
>>
>>
