William Schumann wrote:
> [Would like to push to integrate with build 129, currently Tuesday, Dec
> 1. Pre-Thanksgiving review would be welcomed.]
> Requesting code review for iSCSI boot support in Caiman
>
> The first delivery of iSCSI support is for the Caiman Automated
> Installer. Only static configuration is supported by this code.
>
> Before Target Discovery, the AI manifest is read. If iSCSI target
> parameters are specified, they will be taken from the manifest.
> If not, dhcpinfo(1) is used to get the DHCP-supplied parameter
> Rootpath. If Rootpath specifies iSCSI, the iSCSI target parameters are
> taken from it.
>
> If the boot target is successfully mounted, it will be chosen as the
> install disk. Only x86 support has been tested. The mounting of the
> boot target is a new function of Target Discovery td_search_target() -
> the target is searched for and mounted, given input through an nvlist -
> in this case, the target is an iSCSI target. It is done in a
> generalized way so that other targets may be mounted in the future.
>
> After mounting the iSCSI target, the device name is automatically
> selected as the install disk target candidate. It must then pass any
> other criteria specified in the manifest to be the final target.
>
> http://defect.opensolaris.org/bz/show_bug.cgi?id=6590
> http://cr.opensolaris.org/~wmsch/bug-6590/
> Project page: http://hub.opensolaris.org/bin/view/Project+caiman/iSCSI+Boot
>
> Enables nwam service in repository database (instead of doing it through
> commands in /var/svc/profile/upgrade).
> Target Discovery enhanced to discover with user-specified input (here,
> iSCSI target parameters). New code module:
> td_iscsi.c:td_target_search(attribute list)
>
> Unit testing:
> - correct configuration
> -- varied use of optional parameters to check defaulting
>
> - incorrect configuration - should fail install
> -- missing required parameters
> -- failure to mount with specified parameters should fail install
>
> - repeat install manually running auto-install
> - regression test - still installs to local disk
>
> Known problems:
> - NWAM phase 0 - DNS config not correct on initial reboot - can be
> corrected manually, but easier just to reboot.
> - one Ultra 20 observed to hang system completely when idle if power
> management is enabled
>
> Kludge:
> - Passing parameters from AI to ICT very clunky and will be refactored
> soon, so employed interim technique: create a marker file,
> /tmp/.iccsi_boot, which will be present only for iSCSI boot cases.
>
> CHAP support is only partly coded here. Will be completed soon.
>
> Please include Bing Zhao, since he is responsible for libima-related
> coding. IMA stands for iSCSI management API.
>
> Also, I noticed that Bing did a direct struct-struct assignment. I
> haven't been using that feature, but it is supported by ANSI C, Sun
> Studio, and gcc.
> td_iscsi.c:
> 79 *oid = lhbaList->oids[0];
>
> William
>
> _______________________________________________
> caiman-discuss mailing list
> caiman-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
Great Job William!
Many of my comments are nits or questions.
Joe
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
usr/src/cmd/auto-install/ai_manifest.rng 35 lines changed
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Issue/Question:
-----------------------
207 <element name="target_device_iscsi_target_ip">
208 <text/>
Would it be better fo use something more specific for IP address instead
of text? Maybe not but I thought I would ask you to think about it.
I Googled and found this for IPv4 addresses:
<datatype name="IP" source="string">
<pattern value="((1?[0-9]?[0-9]|2[0-4][0-9]|25[0-5]).){3}
(1?[0-9]?[0-9]|2[0-4][0-9]|25[0-5])"/>
<annotation>
<info>
Datatype for representing IP addresses. Examples,
129.83.64.255, 64.128.2.71, etc.
This datatype restricts each field of the IP address
to have a value between zero and 255, i.e.,
[0-255].[0-255].[0-255].[0-255]
Note: in the value attribute (above) the regular
expression has been split over two lines. This is
for readability purposes only. In practive the R.E.
would all be on one line.
</info>
</annotation>
</pattern>
</datatype>
Same issue with LUN:
212 <element name="target_device_iscsi_target_lun">
213 <text/>
Would it be better to devise something more specific for LUN representing
the 4 16 bit pieces that reflect a multilevel addressing scheme? Maybe not
but I thought I would ask you to think about it.
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
usr/src/cmd/auto-install/auto_install.c 60 lines changed
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Issue/Question:
-----------------------
48 #define ISCSI_BOOT_INDICATOR_FILE "/tmp/.iscsi_boot"
I think this is a good idea as it is consistent with /.autoinstall
and /.livecd but I was wondering why you put this one in "/tmp" instead
of "/".
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
usr/src/cmd/auto-install/auto_install.h 24 lines changed
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Nit/Issue/Question:
-----------------------
206 char diskiscsiname[MAXNAMELEN];
207 char diskiscsiip[MAXNAMELEN];
208 char diskiscsilun[MAXNAMELEN];
209 unsigned long diskiscsiport;
210 char diskiscsichapname[MAXNAMELEN];
211 char diskiscsichapsecret[MAXNAMELEN];
212 char diskiscsiinitiator[MAXNAMELEN];
I find these names difficult to read. How would you feel about adding
a random "_" here or there? ;)
e.g.:
206 char diskiscsi_name[MAXNAMELEN];
207 char diskiscsi_ip[MAXNAMELEN];
208 char diskiscsi_lun[MAXNAMELEN];
209 unsigned long diskiscsi_port;
210 char diskiscsi_chap_name[MAXNAMELEN];
211 char diskiscsi_chap_secret[MAXNAMELEN];
212 char diskiscsi_initiator[MAXNAMELEN];
Issue/Question:
-----------------------
Is MAXNAMELEN sufficiently accurate for these?
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
usr/src/cmd/auto-install/auto_parse.c 38 lines changed
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Nit: 187-> 224
-----------------------
Please add a CR above the consecutive ai_get_manifest_element_value()
invocations for readibility?
I found this a little hard to read and noticed the <CR> above
calls to ai_get_manifest_element_value elsewhere in the file
improved readibilty.
Nit/Issue/Question:
-----------------------
209 adi->diskiscsiport = strtoll(p, NULL, 0);
Should the expected format be described as a comment in ai_manifest.rng?
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
usr/src/cmd/auto-install/auto_td.c 231 lines changed
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Issue/Question:
-----------------------
471 char *diskiscsiname = "";
472 char *diskiscsiip = "";
473 char *diskiscsiporta = "";
...
475 char *diskiscsilun = "";
476 char *diskiscsiprotocol = "";
...
492 char rootpath[MAXPATHLEN] = "";
Wouldn't it be better to initialize there to (char*)NULL or NULL?
Issue/Question:
-----------------------
454 * Returns -1 if error encountered
455 * Returns 0 if no parameters found, which assumes no iSCSI boot
desired
456 * Attempts to mount using libima with iSCSI initiator
457 * Returns 0 and writes Solaris device name of iSCSI target if mounted
458 * Returns -1 if specified target not mounted successfully
Return of -1 and 0 are differently commented twice.
Issue/Question:
-----------------------
482 if (adi->diskiscsiname[0] == '\0' ^ adi->diskiscsiip[0] == '\0') {
Why are you using bitwise OR, ^ ? Shouldn't this be logical OR, || ?
Issue/Question:
-----------------------
510 if ((ret = pclose(pipe_fp)) != 0)
511 auto_log_print("Error in command to check DHCP "
512 "for iSCSI boot client. Command:%s\n", cmd);
...
If ret is -1 does ERRNO need to be checked?
Issue/Question:
-----------------------
Please confirm the "Parsing iSCSI Rootpath" logic.
546 *p++ = '\0';
547 diskiscsiip = p; /* IP */
548 if ((p = strchr(p, ':')) == NULL)
I think you are truncating p before you are done parsing it.
Issue/Question:
-----------------------
634 char mydevname[512]; /* size used in sscanf format */
Is there a better constant to user for mydevname instead of 512?
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
usr/src/lib/libict_pymod/ict.py 60 lines changed
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Issue:
-----------------------
1356 if os.path.exists(ISCSI_BOOT_INDICATOR_FILE):
self.ISCSI_INSTALL should be set in the constructor to be consistant
with:
338 self.LIVECD_INSTALL = False
339 self.AUTO_INSTALL = False
Issue:
-----------------------
1277 if status == 0:
if status == 0 is backwards from how this is handled throughout.
and add logging message before return ICT_SVCCFG_FAILURE
Suggestion:
1277 if status == 0:
1278 return 0
1279 return ICT_SVCCFG_FAILURE
Change to:
if status != 0:
prerror('Unexpected error issuing svccfg -f command.')
prerror('Failure. Returning: ICT_SVCCFG_FAILURE')
return ICT_SVCCFG_FAILURE
return 0
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
usr/src/lib/libtd/Makefile 4 lines changed
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
usr/src/lib/libtd/td_api.h 30 lines changed
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Nit:
-----------------------
244 td_errno_t td_target_search(nvlist_t *);
Why was td_target_search inserted in the middle of the td_discoverXYZ
functions?
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
usr/src/lib/libtd/td_mg.c 36 lines changed
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Nit/Issue/Question:
-----------------------
684 switch (target_type) {
685 case TD_TARGET_TYPE_ISCSI_STATIC_CONFIG:
Is a switch for a single case better than an "if" statement here?
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
New usr/src/lib/libtd/td_iscsi.c 572 lines changed
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
General Nit/Issue:
-----------------------
Please provide more extensive function block comments.
Good examples can be found in td_mountall.c
Nit/Issue:
-----------------------
57 * return 0: success
58 * 1: failure
-1, not 1, is returned on error
Suggestion:
57 * return 0: success
58 * !0: failure
Question:
-----------------------
136 (void) sprintf(targetIpaddr, "%s:%lu", ipaddress, port);
137
138 (void) mbstowcs(staticConfig.targetAddress.hostnameIpAddress.
139 id.hostname, targetIpaddr, IMA_HOST_NAME_LEN);
Does this work for both IPv4 and IPv6 ?
Nit:
-----------------------
238 * Wait fro a while here for updating.
fro -> for
Issue/Question:
-----------------------
240 (void) sleep(INSTISCSI_SLEEP_INTERVAL * 2);
How is it known this length of sleep will be sufficient.
Issue/Question:
-----------------------
433 (void) nvlist_lookup_uint32(attrs, TD_ISCSI_ATTR_PORT, &port);
448 (void) nvlist_lookup_string(attrs, TD_ISCSI_ATTR_LUN,
&lun_num);
Why isn't the return of nvlist_lookup_uint32/sting checked here?
General Questions:
- iSCSI only supported in AI why not LiveCD?
- Shouldn't test_td.c be updated to support td_iscsi?
