Hi Ethan,
On 06/17/10 02:31 AM, Ethan Quach wrote:
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.
At this point I would prefer to address that issue with separate bug,
since I think it requires more thought about possible scenarios - e.g.
how to derive pool name in case mountpoint is not provided and
what approach to choose, so that it works for zones as well.
As far as text installer is concerned, I can see that we might want to
prefer SC profile completely filled in, since I assume all those
information could be stored in DOC and SC profile would be generated
from that. We will better know when we get there.
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.
Thank you for pointing this out. I have changed parameters
where empty string would lead to no-op, but left possibility to
set empty string for 'expire' and 'password', since for those
empty string has meaningful value.
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 ""
Count property can't end up with "", since such profile would
not pass syntactic validation:
# cat /tmp/sc.xml
<!DOCTYPE service_bundle SYSTEM
"/usr/share/lib/xml/dtd/service_bundle.dtd.1">
<service_bundle type='profile' name='sc_install_interactive'>
<service name='system/keymap' version='1' type='service'>
<instance name='default'>
<property_group name='keymap' type='system'>
<propval name='count_prop' type='count' value='' />
</property_group>
</instance>
</service>
</service_bundle>
# svccfg apply -n /tmp/sc.xml ; echo $?
svccfg: illegal value "" for count (Illegal character)
1
#
I have updated webrevs with changes:
http://cr.opensolaris.org/~dambi/bug-15723-cr/
http://cr.opensolaris.org/~dambi/bug-15723-cr-delta/
Thank you very much,
Jan
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
