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.
OK.
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.
Sounds fair.
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,
OK.
but left possibility to
set empty string for 'expire' and 'password', since for those
empty string has meaningful value.
Hmm, the code suspicious then. If 'expire' is an empty string,
then will the call to set_expiration_date() see a "0" for the
second arg? i.e. the check at line 279
A similar comment for set_password() I guess. It seems
set_password() just assumes the second arg is usable.
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
#
OK.
I have updated webrevs with changes:
http://cr.opensolaris.org/~dambi/bug-15723-cr/
http://cr.opensolaris.org/~dambi/bug-15723-cr-delta/
The updated webrev looks fine to me modulo the
comments above.
thanks,
-ethan
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
