On 06/15/10 10:46 PM, Ethan Quach wrote:
On 06/15/10 09:55, Jan Damborsky wrote:
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 ?
Just to consolidate flag days / heads up where possible.
I see. Thank you for clarifying this.
Since
these changes do include support for older SC manifests
automatically on the client, it wouldn't be crucial.
ok.
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 - 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.
Looks much simpler now. However, I have another question.
At line 164, this would seem to mean that it is valid that a
property of type astring can be an empty string, and that
the caller needs to determine that case by checking the return
value itself in conjunction with the return code.
Yep. Since special meaning of empty string was removed,
it can now be passed in SC manifest as a valid value.
If this is the case, there are places that call get_smf_prop() that
may seem to result in error. For example, at line 438-439, if return
code is 0, but shell="", the command option snippet that gets built
is broken isn't it? Same goes for subsequent calls go to get_smf_prop()
in this function.
You are right, thank you for catching this. I have added explicit check
for those cases (login, shell, roles, home_zfs_fs, home_mntpoint).
I have updated the webrev accordingly:
http://cr.opensolaris.org/~dambi/bug-15723-cr/
http://cr.opensolaris.org/~dambi/bug-15723-cr-delta/
Thank you,
Jan
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
