Hey Ethan,
thank you very much for review !
Please see my response below.
Jan
On 06/15/10 10:51 AM, Ethan Quach wrote:
Jan,
Looking at the copy of the webrev at:
http://cr.opensolaris.org/~dambi/bug-15723-cr
General comment/question - In relation to static IP support, what is the
timing in accordance with the changes from the networking team and
this putback? Preferably, it would be the same build, and a flag day
would note how to use the updated SC profile to specify static IP.
Right now, support for static IP is handled separately, since the target
build is TBD and I didn't want to potentially block support for other
SC parameters (e.g. John is currently working on nodename/defaultdomain,
Felix Feng on keyboard layout).
That said, since all necessary infrastructure is being reviewed now
(network/physical controlled via SC manifest), adding support for static
IP will be mostly about modifying default AI manifest.
I can see that networking configuration might be ready by the time this
stuff is ready for the integration, but to be honest I didn't explicitly
plan on synchronizing those pushes.
Could you please elaborate more on why you would like to synchronize
them ?
perform_slim_install.c - 423-424 - Any reason why the user password
is processed differently than the others?
No. I have overlooked this part when modifying the rest, so
I have changed the code accordingly.
While I was there, I also noticed the same applies to 'hostname',
so I changed that as well.
Thank you for catching this.
Isn't it possible that if the call
to nvlist_lookup_string() fails, that garbage can be left in upasswd?
Looking at implementation of nvlist_lookup_string(), if it fails, pointer
is left unmodified, but since that behavior is not documented in man
pages (it is only mentioned when NV_FLAG_NOENTOK is utilized), I have
changed the code, so that we are covered.
ict.py - 2249 - nit - "take effect" -> "be applied"
I have changed it.
system-config.xml - 92-103, 112-114 - Are these properties all listed
here in the service's definition manifest to define type?
yes. Once following bug is addressed, SC manifest could
be simplified by omitting property types (since they will
be derived from those delivered by manifest):
6931819 SMF should default property types when existing properties are
being modified.
system-config.xml - 96-97 - Are these defining default values for these
properties?
Yep. However, see below.
svc-system-config - 102 - Perhaps name this variable to be more attune
to what its used for so that its more clear? Like,
SMF_UNCONFIGURED_VALUE?
I agree. Changed.
svc-system-config - 128 - typo - "vlaue"
Thank you for catching this. I changed this.
svc-system-config - 148-154 - Its not clear to me what value this check
adds. First, it seems rather constricting; this service only supports
properties that are of type 'astring' and 'count' today, but what if that
changes?
Second, if we're worried that the property being read in from the live
repo must be the same type as what we're expecting that property type
to be, it seems we would need to define the types of each property
locally
and compare the type that we read in from the live repo with that value.
svc_system-config - 172 - Does this mean that we're not expecting this
service to ever support a property of type 'count' that a user will
provide
a value of '0' for in the profile?
Based on your comments, I was thinking about this again and I think we could
go with better approach - e.g. not to reserve some of values for
distinguishing
if particular property was set in SC profile and take more relaxed path.
I have changed get_smf_prop() function - it now checks if particular SMF
property
is set for instance of the SMF service (which is the case if property
was configured
in SC profile). Result is indicated via return code to the caller.
I have updated webrevs accordingly:
http://cr.opensolaris.org/~dambi/bug-15723-cr/
http://cr.opensolaris.org/~dambi/bug-15723-cr-delta/
Please let me know if it might address your concerns.
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
