Thanks Jan.
My comments below.
Joe
On 06/ 9/10 12:45 PM, Jan Damborsky wrote:
Hey Joe,
thank you very much for review !
I have incorporated most of the suggestions, please see in line
for questions/comments for those I was not sure about.
The updated webrev is available at (including John's part):
http://cr.opensolaris.org/~dambi/bug-15723-cr/
delta webrev can be seen at
http://cr.opensolaris.org/~dambi/bug-15723-cr-delta/
Get well soon !
Jan
General:
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Issue:
------
"Not all the copyrights are the same?"
To be honest, I am not sure what the issue is. 'hg pbchk'
does not complain, but I can see there might be some discrepancies
I am not aware of. Could you please elaborate more on this ?
I just noticed differences in the year ranges. Maybe this is OK. Here
are a few examples:
usr/src/cmd/Makefile.cmd
23 # Copyright (c) 2007, 2010, Oracle and/or its affiliates. All rights
reserved.
usr/src/cmd/auto-install/auto_install.h
22 * Copyright (c) 2008, 2010, Oracle and/or its affiliates. All rights
reserved.
usr/src/cmd/system-config/svc/svc-system-config
23 # Copyright (c) 2010, Oracle and/or its affiliates. All rights reserved.
[...]
usr/src/cmd/Makefile.cmd 1 line changed
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Issue:
------
"Perhaps ROOTMANIFEST should include "LIB" as all the other variables
that point to something under /lib do. That is it could be changed
to ROOTLIBMANIFEST. What do you think?"
I agree. In order to be consistent with lines above I changed
ROOTMANIFEST -> ROOTLIBSVCMANIFEST
[...]
OK. Thank you.
usr/src/cmd/auto-install/auto_parse.c 161 lines changed
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Issues:
-------
Line: 61 char buf[2048];
line: 1273 char cmd[2048];
"Is 2048 a good choice for buf size? Elsewhere in the code
MAXPATHLEN is uses, which also might not be a great choice. ;)"
I think MAXPATHLEN is misleading, since we are not dealing with
paths here.
"Perhaps a new constant for SHELLCMDLEN should be defined. ??"
In order to avoid hardwired numbers, I defined new constant
in auto_install.h, since I have not found anything existing
which might fit the needs here:
#define MAX_SHELLCMD_LEN 2048
[...]
OK. Thank you.
Question:
---------
Line: 1205 -> 1207 if (strstr(token, AUTO_PROPERTY_ROOTPASS) != NULL) {
"Is it correct for ROOTPASS to still be handled by this code?
If ROOTPASS is still handled here then shouldn't the code to validate
it's length be re-added @ lines 1248 -> 1249.
I just realized what you are doing with ROOTPASS. If found then
attempt to convert the old style manifest.
However I will still leave this code review response until we
can talk about this. It is not clear to me if lines 1205 -> 1207
are needed."
Yep, they are needed for the reason you mention above. We check
for presence of 'rootpass' XML tag in order to determine, if legacy
manifest was provided.
But we do not use the value itself, so this is the reason why
rest of the related code is not needed.
[...]
OK. Thank you.
usr/src/cmd/auto-install/default.xml 43 lines changed
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Question/Issue:
---------------
"Why do the left shift the indentation starting at line 77?"
The shift was introduced when changing content of SC manifest -
I reverted these changes.
OK. Thank you.
Question:
---------
Line: 90 <propval name="profiles" type="astring" value=""/>
"If the value for this is empty by default is it valuable to have
it in this manifest?"
I can see it might be misleading given the fact that this is
the only empty tag listed there - I removed that line.
[...]
OK. Thank you.
New usr/src/cmd/system-config/svc/svc-system-config 778 lines changed
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Issue:
------
"Looks as if you already ran:
shcomp -n svc-system-config /dev/null
If not remember to do so."
Yep - I did this:
da...@tio:~/ws/os-install-bug-15723-cr/usr/src$ shcomp -n
cmd/system-config/svc/svc-system-config /dev/null ; echo $?
0
[...]
OK. Thank you.
Issue:
------
"I don't believe I am suggesting this as I usually like to break
code up into functions but Lines 105 -> 145 are used to encase
a single line of code into 2 different functions.
Why are the functions necessary?"
Why not change from:
342 ! $(astring_prop_configured "$expire") && return
To:
if [[ "${expire}" != "${SMF_DEF_ASTRING}" ]]; then return; fi
I believe having them separated in function improves readability
and maintainability of the code. The similar reason why
macros are used in C code.
"This would eliminate 40 lines of code from this file."
Well, the most of those 40 lines is only comment section
explaining what those small functions do.
I would prefer to leave the code as is, but if you would rather
change it, I will do that. Please let me know.
[...]
I see the value in your perspective too. It's fine with me to leave it
as you have it. Thank you.
Suggestion:
-----------
"
I've not seen the use of command substitution, $(), for invoking
a local function. I've only seen it used to invoke a shell command.
Which makes me wonder if using $() might spawn a subshell. I can't
seem to confirm that so maybe it does not."
To be honest, I am not sure if separate shell is spawned for this,
but it seems that it is being used - e.g. see definition of
smf_zonename() function in /lib/svc/share/smf_include.sh and
how it is consumed by /lib/svc/method/net-nwam, /lib/svc/method/net-init
But instead of doing:
305 type=$(get_astring_prop $prop_name)
You might want to consider doing:
get_astring_prop $prop_name; type = $?
It does not work, since we want to capture 'stdout', not
return value.
[...]
Ah yes. Thanks for pointing that out.
Issue/Suggestion:
-----------------
"ksh93 has built in variable/parameter expansion formats."
For example:
182 prop_value=$(print $prop_value | $SED -e 's/\\\(.\)/\1/g')
Could be done with:
prop_value=${prop_value//\//}
Unfortunately, it does not seem to work for quoted backslashes, i.e.
we want to replace '\\' with '\',but the example above removes
them all.
[...]
Ah yes. I believe an extra "\/" in the match string should do the trick.
Does this work for what you need?
e.g.:
$ prop_value="aaa//bbb/ccc//ddd"
$ print ${prop_value//\/\//}
aaabbb/cccddd
Issue/Suggestion:
-----------------
"Using Parameter Expansion would reduce the difference between
functions"
161 get_astring_prop()
and
207 get_count_prop()
"Which could allow them to be more easily combined into a single
fuction as the logic in both is very similar."
To be honest, I do not see how Parameter expansion might be
used in this case - could you please elaborate more on that ?
[...]
What I am thinking is if the Parameter expansion could be made to work
in this case Lines 181 -> 186 would be collapsed to a single line. This
would result in get_astring_prop() and get_count_prop() to be identical
except for a couple of different lines. Which would make them a good
candidate to collapse into a single function.
Maybe maybe not. It's just a suggestion.
Issue:
------
554 $ZFS set mountpoint="$home_mntpoint" "$home_zfs_fs"
"Is it possible the ZFS data set is already mounted?"
No, since this SMF service is run before
svc:/system/filesystem/local:default
which mounts ZFS datasets.
[...]
OK. Thanks.
Issue:
------
666 rmdir -ps $home_mntpoint
"Suggest:
Since this command could fail pipe output to /dev/null"
rmdir -p $home_mntpoint > /dev/null 1>&1
This is what '-s' accomplishes - from rmdir(1) man page:
$ man -M /usr/share/man rmdir
-s Suppresses the message printed on the standard error
when -p is in effect.
Ah. That's good to know. Thanks.
ksh93
The following options are supported for the rmdir built-in
for ksh93:
...
-s Suppress the message printed
--suppress on the standard error when -p
is in effect.
...
What's the -s flag do? I do see it in RMDIR(1)
I guess you do not see '-s' in man page, since you perhaps have
/usr/gnu/share/man listed before /usr/share/man in MANPATH and
'-s' is not available in GNU version of rmdir.
[...]
Ah yes. Of course. Thanks.
New usr/src/cmd/system-config/tools/sc_conv.ksh 276 lines changed
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
[...]
Issue:
------
"Count the NAWK code on lines 207 -> 224 be place in a function?
Then it coule be invoked with, for example:"
get_value(username); username=$?
get_value(userpass); userpass=$?
get_value(description); description=$?
get_value(rootpass); rootpass=$?
get_value(timezone); timezone=$?
get_value(nodename); nodename=$?
"If it is difficult to pass an argument to the NAWK command for the
string to match perhaps this wouldn't be a good idea, but if it
could be done the code would be more readable."
To be honest, I have not found how to expand variable as part
of regular expression in nawk, but I am open to suggestions, since I
agree
this is candidate to be moved to function.
[...]
I don't know how either. I was hoping you might know! ;)
usr/src/lib/liborchestrator/perform_slim_install.c 350 lines changed
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
[...]
Question:
---------
"If uname is NULL at line 387 doesn't need to be set to EMPTY_STR
as it was done in the original version on line 403?"
After refactoring that function, 'uname' is now initialized to
'EMPTY_STR'
when declared at the beginning of the function.
OK. Thanks.
_______________________________________________
caiman-discuss mailing list
caiman-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
