Hi Sanjay:
Thank you very much for your time and comments. :-) Please see my email
inline. Thanks.
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 ?
Maybe they are different things. Here the device name stands for os
device name. It is not the disk name said in dkio(7I) I think.
For iscsi and fibre channel os device name, they usually contain a GUID.
So the length can be longer than 32 .You can see them from format(1M).
-bash-3.2# format
Searching for disks...done
AVAILABLE DISK SELECTIONS:
0. c1t0d0 <DEFAULT cyl 17831 alt 2 hd 255 sec 63>
/pci at 0,0/pci10de,376 at a/pci1000,3150 at 0/sd at 0,0
1. c1t2d0 <DEFAULT cyl 60797 alt 2 hd 255 sec 126>
/pci at 0,0/pci10de,376 at a/pci1000,3150 at 0/sd at 2,0
2. c1t3d0 <DEFAULT cyl 13053 alt 2 hd 255 sec 63>
/pci at 0,0/pci10de,376 at a/pci1000,3150 at 0/sd at 3,0
3. c3t600144F00008272C70AF4AB8A05E0001d0 <DEFAULT cyl 2607 alt 2
hd 255 sec 63> <=====The name is longer than 32 bytes.
/scsi_vhci/disk at g600144f00008272c70af4ab8a05e0001
4. c3t600144F000082747EEEB4AF2715D0001d0 <DEFAULT cyl 1956 alt 2
hd 255 sec 63>
/scsi_vhci/disk at g600144f000082747eeeb4af2715d0001
5. c3t600144F00008276216FD4AAA179F0001d0 <DEFAULT cyl 1302 alt 2
hd 255 sec 63>
/scsi_vhci/disk at g600144f00008276216fd4aaa179f0001
Specify disk (enter its number):
On iscsi related command iscsiadm(1M), you can also find above iscsi disks:
-bash-3.2# iscsiadm list target -S
Target: iqn.1986-03.com.sun:02:7ff23d41-0ff1-c92e-9549-fb6532f2a55f
Alias: -
TPGT: 1
ISID: 4000002a0000
Connections: 1
LUN: 3
Vendor: SUN
Product: COMSTAR
OS Device Name:
/dev/rdsk/c3t600144F000082747EEEB4AF2715D0001d0s2 <=====The os
device name is longer than 32 bytes.
LUN: 1
Vendor: SUN
Product: COMSTAR
OS Device Name: /dev/rdsk/c3t600144F00008272C70AF4AB8A05E0001d0s2
LUN: 0
Vendor: SUN
Product: COMSTAR
OS Device Name: /dev/rdsk/c3t600144F00008276216FD4AAA179F0001d0s2
Here I think the disk name said in dkio(7I) is a different thing with
the os device name.
So I use MAXNAMELEN here.
>
> 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 ?
Here we use IMA. And IMA is a SNIA specification. Currently we are using
v2.0. It can be found here:
http://www.snia.org/tech_activities/standards/curr_standards/ima/
In the iSCSI Management API (IMA) v2.0
<http://www.snia.org/tech_activities/standards/curr_standards/ima/iSCSIManagementAPI_v2.0.pdf>
section 5.56, the max length of chap name is set to 512. So I set it to 512.
In the iSCSI_ibft_target is defined according to iBFT specification
here: http://www.microsoft.com/whdc/system/platform/firmware/ibft.mspx
In section 3.7, it uses two bytes for the length of the chap name. So we
that's why we used ushort_t there.
>
> 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 ?
I will correct our code according to your other comments and send the
webrev later.
Thank you again for your comments. It is really helpful. :-)
Regards,
Bing
>
> 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
>
>
> _______________________________________________
> caiman-discuss mailing list
> caiman-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://mail.opensolaris.org/pipermail/caiman-discuss/attachments/20091124/fdec3ee2/attachment.html>
