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.
>
> 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. This assumes that if the user specifies iSCSI boot
parameters, there is a specific target disk. The code takes the same
path used when the device name is explicit. Any other ai_target_device
device selection parameters are silently ignored.
>
>
> ai_manifest.rng
> ---------------
> Is a flat set of all optional tags here what we really want?
> Namely, in the auto_td code it seems 'iscsi_name' and 'iscsi_ip'
> are expected to either both be there, or not at all. If that is
> the case why didn't you define that in the schema instead?
It is possible to precisely describe the legal combination of tags in
the RNG schema for iSCSI.
Recall that the manifest parser/validator is run early - before AI looks
at the parsed output - and AI terminates if errors are found before AI
can look at the result.
If the user is in error, for whatever reason, the log has 2 lines:
error validating the manifest
Auto install failed - Invalid manifest <manifest name> specified.
and says nothing explaining the error further.
To test, I left out a mandatory iSCSI parameter in the manifest, ran AI,
and the console showed the following messages:
Relax-NG validity error: Extra element ai_target_device in interleave
[this is overly technical; does not name the "extra element"
correctly, but the parent element]
/tmp/ai_manifest_temp_1116.xml:15: element ai_target_device: Relax
-NG validity error : Element ai_manifest fails to validate content
[temp file is deleted so user can't examine line 15, provided the
user can decipher the message; the error message makes no sense literally]
... and so on.
Now, even if the validation messages were improved, they would still be
generic and still wouldn't help as much as logging statements provided
from AI. However, if the schema were improved, these errors would be
found on the AI server side during 'installadm add' without any
additional code to validate. Because of the latter, perhaps the better
option is to do the improved validation you suggest in the schema, and
work on improving the validator output.
Upgrading ai_manifest.rng per your suggestion.
>
>
> auto_install.h
> --------------
> 206-215 - since these are iscsi specific, why not define a new,
> separate structure for this set of info and include an instance
> in auto_disk_info?
Made a separate typedef for the iSCSI target parameters.
>
> 210 - Should we be using uint32_t here instead?
>
> 222 - Can we get a #define for this value?
> auto_install.c
> --------------
> 63 - why is this declaration here instead of in td_lib.h
> or something like that ?
It is AI code in auto_td.c.
>
> 635,654 - you're not passing anything to fill in the %s
>
> 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.
>
>
> 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.
>
> 485-486 - nit - It would be better if the error message actual
> said which of the two wasn't specified.
>
> 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.
>
> 653 - What's the significance of the length 4? Why is something
> less than that an invalid device name?
The shortest device name I can think of for disks is cNdN, length 4.
Changed length 4 to strlen("cNdN");
>
>
> td_iscsi.c
> ----------
> 452 - These three pointers should all be initialized to NULL.
> Not quite sure what you're trying to achieve by initializing
> *lun_num = "" ??
Subsequent code handles NULL-terminated strings.
>
>
> ict.py
> ------
> 1365-1368 - I read this multiple times, and still couldn't
> see the difference between sentence #1 and #2. Then I
> remembered the issue, and finally figured out what is being
> said here. May I suggest clarifying a little bit with ...
>
> "For iSCSI boot, if the NWAM service transitions from disabled
> to enabled during boot, the connection to the iSCSI boot target
> will be lost. If the system is booted with NWAM already enabled,
> the iSCSI boot target connection is maintained."
>
> And also add to the comment that we're doing this to work around
> behavior in NWAM.
>
>
Done
William
> thanks,
> -ethan
>
>
> William Schumann wrote:
>> Updated webrev including all requested changes with Bing's changes
>> included.
>>
>> 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/oldwebrev
>>
>> Thank you,
>> William
>> _______________________________________________
>> caiman-discuss mailing list
>> caiman-discuss at opensolaris.org
>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
>
