Thank you William!
Your replies are OK with me.
I will check the updated webrev once you post it.
Joe
William Schumann wrote:
> 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?
>>
