Bing Zhao - Sun Microsystems wrote:
> 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).
Is this length constrained ? For example, there is a definition of
GUID in heci.h which defines it to 32 bytes.
>
> -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.
So is version 2 of CHAP limited to 512 whereas the maximum can be much
higher ? I am still unclear about the relationship between the two.
>>
>> 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
>
