Hi Sanjay:
sanjay nadkarni (Laptop) wrote:
> 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.
Thank you for your reminder. I realize that we should obey the IMA
specification as well. And in IMA specification 5.52,
the max length of os device name is 64. So maybe we should use 64 here.
>>
>> -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.
Yes, I think you are right. But in IMA specification, it says the max
length is 512, so we have to use 512. Please note that there is no
relationship between IMA and iBFT.
And the code in AI here is written according to IMA.
Regards,
Bing
>>>
>>> 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
>>
>
