Joe,
I've taken your suggestions if not otherwise mentioned in my replies below.
I will resubmit the webrev when I finish then changes replacing
MAXNAMELEN with proper sizes.
Joseph J VLcek wrote:
> +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
>
>
> 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>
I found references to this online. It has a couple of errors, and it
does not apply to Relax NG.
>
> Same issue with LUN:
> ...
Yes, it would be better for manifest validation to do these things. It
cannot be done with the basic datatypes - a supplemental validation
library or extension of RNG would have to be created. This should be
addressed in semantic validation, since it might be done better in C or
Python.
>
>
> +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
>
>
> 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?
I think that comments in the RNG file should be limited to explaining
specific RNG issues, and the rest should be commented in the code. In
this case, it's just a port number used for a TCP connection.
>
> +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
>
>
> 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?
Subsequent code expects NULL-terminated strings and does not check for
NULL. I'm not sure that it is better one way or the other.
>
> 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.
I was trying to document an algorithm explaining return codes. Changed
to make it more conventional.
>
> Issue/Question:
> -----------------------
>
> 482 if (adi->diskiscsiname[0] == '\0' ^ adi->diskiscsiip[0] == '\0') {
>
> Why are you using bitwise OR, ^ ? Shouldn't this be logical OR, || ?
This is an exclusive OR, which does not have a logical form in C.
Improved comments to clarify this.
>
> 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.
p is a utility pointer that is used to find the separator ":". Each
time it finds it, it writes a NULL and increments so that it points to
the next token, which is an iSCSI boot parameter (or the next separator
if it is empty). Setting *p to \0 is not necessary in 546, but it is
necessary in later lines to NULL-terminate the iSCSI boot parameters.
>
> Issue/Question:
> -----------------------
>
> 634 char mydevname[512]; /* size used in sscanf format */
>
> Is there a better constant to user for mydevname instead of 512?
sscanf (see comment above) does not take constants to limit the input
size (token pasting can be done, but it is a little messy). I thought it
better to use the constant and document that the constant was used in
the sscanf format.
>
> +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
>
>
> 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?
It is placed below the discover functions and above the more general use
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?
I coded it with a switch to emphasize that the new TD interface is
generic and that target_type determines the function that will call a
function for that specific target_type. It should make it easier to add
additional target types later.
Will inform when code review is updated.
Thank you,
William
>
>
>
>
> +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
>
>
> 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?
>
