I've got an updated code review of this wad at
https://cr.opensolaris.org/action/browse/pkg/timf/periodic-recv-2/
src/svc/pkg-mirror.xml
Line 171 - Comment seems incomplete as it ends with "we
ignore". Did you mean "we ignore the ones configured in the
image at 'config/ref_image'"?
Lines 198-212 - Nit but could you list these in the same order
as above for easier cross-checking?
Will this service be documented under the pkgrecv(1) manual
page along with the usual documentation?
src/svc/pkg-repositories-setup.xml
Does this service really need dependencies on
system/filesystem/local and system/filesystem/autofs given that
the dataset must be local?
src/svc/pkg5_include.sh
Lines 101-109 - The window is very small already but I suppose
if you wanted to reduce it further, you could say:
$DIFF /tmp/actual-crontab.$$ - < $CURRENT_CRONTAB 2>&1 \
> /dev/null
if [ $? == 0 ]; then
$CRONTAB $NEW_CRONTAB
EXIT=$?
else
echo "Crontab file was modified unexpectedly!"
EXIT=1
fi
$RM /tmp/actual-crontab.$$
return $?
Line 126 - Can you move the acquire_crontab_lock closer to
where you update the crontab. I realize it doesn't semantically
matter but visually it looks strange prior to the typesets.
Meta-nit - The Shaprio normal form for sh-style would say that
there shouldn't be a leading space before the ';' in a
conditional such as "if" and "while" as in:
while [ "$LOCK_OWNED" == "false" ] ; do
or
if [ $? -eq 0 ] ; then
Also, the same says that shell multi-line block comments should
include a leading and trailing "#" comment line as in
#
# First line
# Second line
#
(I know you're following the existing convention in the file
but since you're rewriting much of this...)
Finally a meta-comment - the prior version of these functions
and the new ones aren't secure with respect to /tmp. A
malicious user could guess one of the temporary crontab files
and create a symlink to somewhere that 'pkg5srv' can write to.
It looks like though you could do something like:
UID=$($ID -u)
CRONTAB_LOCK=$MKDIR /tmp/pkg5-crontab-lock.$UID
.
.
.
And then in acquire_crontab_lock, reference $CRONTAB_LOCK and
place all of the temporary files under $CRONTAB_LOCK?
src/svc/svc-pkg-mirror
See above meta-nits on ';' and block comments.
Line 27 - Extraneous whitespace before 'start' and after 'stop.
Line 81 - s/others/other's/
Line 83 - Missing a blank comment line?
Lines 117-127 - Aiigh, embedded Python! :-) I think that's fine
for one/two lines but otherwise, how about just using awk(1) as
in:
echo "$schedule" | awk '
NF != 5 {
print "config/crontab_period property must contain 5 " \
"values.";
exit 1
}
$1 == "random" || $2 == "random" || $4 == "random" || \
$5 == "random" {
print "only field 3 can have the value random";
exit 1
}'
Lines 132-133 - You can do this straight out of the shell:
RAND==$(( ($RANDOM % 28) + 1 ))
Lines 135-136 - I think I'm missing something; haven't you
stripped the '\'s from the $schedule property but you're adding
it to $new_schedule prior to the comparison? It would be nice
to leave this without the quoting and only quote it when you're
about to run the setprop.
Line 138 - Incomplete comment?
Line 164 - Maybe a comment referencing that $special comes from
/lib/svc/share/fs_include.sh?
Line 206 - Is the "exit 1" actually necessary here as we should
have never returned from check_failure?
Line 218 - "instance" is already defined on line 197.
Lines 242, 246 - I don't think you need the "sed -e 's/^ *//g'"
here as the subsequent $AWK should do the right thing,
correct?
Alternatively, you could just drop the $AWK and change the $SED
to "sed -e 's/.* //g'". This means you don't have a leading
space in the path to the key and cert.
Line 254 - Nit, missing space after "status".
Line 256 - You can remove a level of indentation with
if [ "$pub" != "$publisher" ] ; then
continue
fi
or
[[ "$pub" != "$publisher" ]] && continue
Lines 263-264 - Rather than directing to /dev/null, you can use
the -s option to $GREP.
Line 350 - Extraneous spaces? "from $origin : " should be
"from $origin: "?
Lines 353-377 - This can be simplied a bit as follows:
if [ -n "$key" ] && [ -n "$cert" ]; then
key="--key $key"
cert="--cert $cert"
fi
# show the command we're running
if [ "$debug_flag" = "true" ] ; then
echo $PKGRECV -s $origin -c "$cachedir"/$instance \
-d "$repo" -m all-timestamps \ --key "$key" \
--cert "$cert" '*'
fi
$PKGRECV -s $origin -c "$cachedir"/$instance -d "$repo" \
-m all-timestamps --key "$key" --cert "$cert" '*' \
> $LOG.tmp 2>&1
EXIT=$?
src/svc/svc-pkg-repositories-setup
Line 39 - Or just "exit $SMF_EXIT_OK"?
Line 44 - Maybe a comment referencing that $special comes from
/lib/svc/share/fs_include.sh?
Line 54 - Or just "exit $result"?
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
