Hi David,
On 02/26/13 11:11 AM, [email protected] wrote:
I've got an updated code review of this wad at
https://cr.opensolaris.org/action/browse/pkg/timf/periodic-recv-2/
Thanks for the very thorough review. I've got an updated webrev at
https://cr.opensolaris.org/action/browse/pkg/timf/periodic-recv-3/
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'"?
Sorry, thanks. This was a throwback to the very first rev of this
changeset, when we had a 'config/origins' property.
When we decided to switch to auto-configuring the service using the
origins in a reference image, I was going to keep 'config/origins' (used
in the original implementation) and only consult it for URIs only if
'config/publishers' wasn't set, but ultimately decided that it was
really too confusing. I've removed the incomplete bit of the comment.
Lines 198-212 - Nit but could you list these in the same order
as above for easier cross-checking?
Absolutely (I should have spotted that)
Will this service be documented under the pkgrecv(1) manual
page along with the usual documentation?
That was my intention, yes.
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?
Sorry, copy/paste error for autofs :-/ I think it needs
filesystem/local because I'd like any other mounts near /var/share to
appear before I try to create this new dataset.
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 $?
Sounds good, though 'return $EXIT' as the last line, I think.
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.
Sure.
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...)
No problem at all, I've fixed these.
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?
That seems much better, yes. I've moved the removal of the temporary
files to just before we release the crontab lock.
src/svc/svc-pkg-mirror
See above meta-nits on ';' and block comments.
Fixed.
Line 27 - Extraneous whitespace before 'start' and after 'stop.
Line 81 - s/others/other's/
Line 83 - Missing a blank comment line?
Fixed all of these, thanks.
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
}'
Oh, that's nicer (and the same length!) thanks.
Lines 132-133 - You can do this straight out of the shell:
RAND==$(( ($RANDOM % 28) + 1 ))
Sounds good.
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.
Yes, I've no idea what I was thinking, sorry.
Line 138 - Incomplete comment?
Right, the comment should be that because this value gets set using
svcprop, it won't appear on the running SMF instance until after the
cron job has fired (which refreshes the instance)
Line 164 - Maybe a comment referencing that $special comes from
/lib/svc/share/fs_include.sh?
Sure
Line 206 - Is the "exit 1" actually necessary here as we should
have never returned from check_failure?
It's required because I'm using the argument 'degrade' on this call to
check_failure(), which causes it to mark the service as being in
maintenance, but returns to allow us to continue cleaning up.
Line 218 - "instance" is already defined on line 197.
Thanks!
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.
That's much nicer.
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
Thanks, fixed those.
Lines 263-264 - Rather than directing to /dev/null, you can use
the -s option to $GREP.
Oh, I didn't know about that (though I think you mean -q? but yep)
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=$?
That doesn't quite work, but I see your point: we need to run the
command without using the "--key" and "--cert" options, and just use
$key and $cert instead (without quotes) I've cleaned this up.
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"?
Those all sound good, fixed.
cheers,
tim
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
