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...
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?
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?
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?
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 ?
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?
auto_td.c
---------
454-455 - What's a "serious error" as opposed to a normal error? :-)
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?
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 ?
653 - What's the significance of the length 4? Why is something
less than that an invalid device name?
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 = "" ??
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.
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
