On 06/16/10 00:19, Jan Damborsky wrote:
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).
OK, just a couple more that I missed ...
452 - I think its a bit dangerous to hardcode "rpool" as the
pool name here (even though currently in AI we always expect
to install into 'rpool') It would be nice if you just wrote a small
bit of code to figure out the name of the pool on which the
dirname of $home_mntpoint file system lives, and fill that in
as the poolname. That way it'll just work when we use this with
text-installer, and when AI supports pool naming and multiple
pools. I would prefer that you do this now but if you choose
not to, a bug to track this would be ok I suppose.
545,561,577,584,598,657,664 - These seem to be more cases
of needing to check for empty string-ness of value before use.
430,435 - I think the same applies here? These are of type
'count' and you have default values for them defined in the
manifest definition, so I'm not sure it its possible if these
will ever return ""
thanks,
-ethan
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
