William Schumann wrote: > Ethan, > Replies below. > >>>> http://defect.opensolaris.org/bz/show_bug.cgi?id=6590 > >>>> http://cr.opensolaris.org/~wmsch/bug-6590/ > >>>> previous webrev: http://cr.opensolaris.org/~wmsch/bug-6590/oldwebrev3 > > Ethan Quach wrote: >> William Schumann wrote: >>> Ethan, >>> I coded your suggestions, if not otherwise mentioned below. >>> >>> Made 2nd round of changes and issued webrev at: >>> http://cr.opensolaris.org/~wmsch/bug-6590 >>> Previous webrev at: >>> http://cr.opensolaris.org/~wmsch/bug-6590/oldwebrev2/ >>> >>> Ethan Quach wrote: >>>> William, >>>> >>>> General comments/questions >>>> -------------------------- >>>> 1. On every AI boot now, where iSCSI is *not* specified, it seems >>>> td will go asking DHCP to check if this client has an iscsi >>>> Rootpath specified for it there, is that correct? If so, I'll >>>> probably have follow-on comments/questions... >>> Yes. The query will be replied to locally by dhcpagent(1m). At this >>> time, since only one interface is configured at install time, it will >>> take the value from that interface. When multiple interfaces is >>> supported, the code may have to query individual interfaces. >> >> a) Did you consider making this a declarative specification in >> the manifest? i.e. make the user specify in the manifest >> whether or not they want to probe DHCP for the client's iscsi >> root target info. If so, what were the tradeoffs? > After discussing this with you and Sanjay, we decided to add a tag > directing AI to the source of the iSCSI parameters. > New tag: target_device_iscsi_parameter_source > Default: "manifest" > Optional: "DHCP" > This was chosen over the existing logic, which automatically took the > DHCP Rootpath for iSCSI parameters if they were present (and > not in the AI manifest), in order to avoid a user accidentally using > Rootpath. > > So, if "manifest" is selected or the tag isn't present, DHCP Rootpath is > ignored. If "DHCP" is selected, the manifest is ignored for iSCSI > parameters. > > This opens the question of what AI does if "DHCP" is selected and the > DHCP Rootpath doesn't contain iSCSI parameters. Currently, it is coded > to fail the install in this case. This might not be flexible enough for > more heterogeneous sites. An alternative would be not to abort the AI > install and to allow it to continue to search for a install disk > target. Another option might be to allow the user to choose the > behavior in this regard. Perhaps it could come in the form of a > prioritized list, as one you might see in a BIOS definition of boot > device order, e.g.: > - DHCP, otherwise fail > - DHCP, try local disk > - DHCP, manifest, otherwise fail > - manifest, DHCP, otherwise fail > - manifest, DHCP, try local disk > - etc. > Suggest to discuss this with real users before getting into complicated > logic.
Per our discussion, I think what you have coded now is fine. The flexible cases to support heterogeneous sites are more of a problem solved with dynamically derived manifests. >>>> >>>> 2. Is it possible for a user to specify iscsi parameters but >>>> then the ai_target_device they've actually specified to install >>>> onto isn't that device? If so, what would result? >>> Yes. As coded, the iSCSI-mounted device would become the specified >>> install device. >> >> How/when does this happen? I'm not seeing this in the code. > auto_install.c: > 647 (void) strncpy(adi.diskname, iscsi_devnam, > 648 sizeof (adi.diskname)); > Thanks. >>>> >>>> >>>> 664 - If the answer to general question #2 above is yes, it >>>> seems 'p' should be compared against 'iscsidevnam' before >>>> creating the iSCSI marker file? >>> iscsidevnam is written as the install device, so no comparison is >>> necessary. >> >> Are you saying that if the user specifies iscsi disk information, >> that the code that validates (and instantiates) that iscsi disk >> also writes adi->diskname? Where is that happening? I'm not >> finding that in the code. This logic might be happening somewhere >> in code you're not touching, so can you point me to it if that is >> the case? > In auto_install.c: auto_select_install_target() takes the diskname from > the auto_disk_info, which has by this time been loaded with iSCSI device > name. Then in auto_td.c: auto_validate_target(), TD is run, then the > diskname is validated against those found by TD. See else clause for > *diskname == NULL (here, line 190). Yep, I see this now per your explanation above. Thanks. >> >>>> >>>> >>>> auto_td.c >>>> --------- >>>> 454-455 - What's a "serious error" as opposed to a normal error? :-) >>> Not every inconsistency or failure is sufficient to justify >>> terminating the install. If dhcpinfo fails, we want to know about >>> it, but not give up completely, eh? >>>> Can you clarify in the comment when devname could be set? It looks >>>> like with a return of -1, devname is untouched. With a return of 0, >>>> devname will have been populated if found? >>> Yes, you read it correctly. Will try again to improve the explanation. >> >> Perhaps I'm being a little picky here, but the comment still doesn't >> clarify which return codes correlate with when devname could be >> modified, and when its left untouched. > Trying again. Thanks, its clearer now. >> >>>> >>>> 645 - any reason why this is hardcoded instead of using MAXNAMELEN ? >>> As indicated in the comment, the size of the buffer is used as a >>> maximum length in sscanf. Since the scanf format string cannot have >>> a variable maximum length, given options of token-pasting or >>> dynamically building the sscanf format string, I thought this was the >>> simplest choice. >> >> I really would prefer against the hardcoding. Would a strncmp() and >> then a strlcpy() be too much trouble? >> > Using sscanf() is clear and concise and uses a single line of code. > However, there was another complaint, so taking the suggestion, finally. auto_td.c --------- 683 - Should the 'if' block beginning here only be executed inside the 'if' block at 681? Otherwise, it seems we could end up copying something we're not expecting. >> >> auto_td.c >> --------- >> 473 - initialize to NULL. > Pointers should not be initialized to NULL unless there is a specific > meaning for NULL in the context. In this case, the pointers > all point to NULL-terminated strings, so assigning them to NULL goes > against the intended usage and can confuse programmers who pick up the > code in the future. This is a long-standing Sun coding standard. > > Use of lint checking for uninitialized variables should be restored to > the build process. I will file a bug for this. Thanks for doing this. >> >> 724 - Aren't IPv6 addresses separated by colons, and aren't >> there up to eight sections, not six? >> > IPv6 is not supported for iSCSI boot, so removing the code. 707 - nit - remove IPv6 from comment. thanks, -ethan
