Joe,
Thanks for the review ...
Joseph J. VLcek wrote:
> usr/src/cmd/installadm/setup-image.sh
> --------------------------------------------------------------
>
> Issue 1:
> --------
> 310 mkdir -p 755 ${img_ai_dir} > /dev/null 2>&1
>
> This will create directory ./755 and ${img_ai_dir}
Oops. Should have been a -m
>
> Issue 2:
> --------
>
> Prior to returning you don't clean up ${img_ai_dir}.
I'll clean up ${img_ai_dir} before lines 320, 328, and 336.
>
> Issue 3:
> --------
>
> You pipe stderr and strout to /dev/null which could make debugging any
> issues difficult.
Per Jan's comment, I've leaving stderr alone.
> To aid in debugging it might be valuable to allow the user turn on a
> level of debug output
> that would result in the output of these commands to be logged.
Probably a good RFE to have a debug or verbose mode for
installadm in general. I'll file one.
>
> Issue 4:
> ---------
> Instead of using echo for error logging I think you should use print_err.
Will do.
>
> Issue 5:
> --------
>
> This code should trap escape. For example in case a user attempted to
> interrupt it by
> typing ctrl-C. The code should trap that and clean up, not leaving
> lofimounts and directory caid_mnt...
It looks like this script has bigger issues in that it currently
doesn't trap an escape at all. e.g. If ctrl-C is issued during the
cpio at 251, the cpio dies, but the script keeps going.
What I think I can do is trap the script using the existing
cleanup_and_exit() function. Then augment that function to
also cleanup whatever the caid_* setups are.
>
>
> usr/src/cmd/ai-webserver/publish-manifest.py
> -------------------------------------------------------------------------
>
> Issue 1:
> ------------
> Question.
>
> 702 if self._AIschema != None:
>
> Please confirm: Is it really an error to re-set the AIschema?
>
> 732 raise AssertionError('Imagepath is already
> set')
>
> Same question with ImagePath.
Not errors. I've removed those checks.
>
> Issue 2:
> ------------
>
> Suggestion: Wouldn't it be more flexible to defines these strings as
> constants in case these file
> names and or paths were ever to change?
>
> 709
> "/auto_install/ai_manifest.rng")):
> 711
> "/auto_install/ai_manifest.rng"
> 714
> "/usr/share/auto_install/ai_manifest.rng"
done.
>
> usr/src/cmd/installadm/setup-service.sh
> ---------------------------------------------------------------
>
> Define PATH or use full paths for command
> e.g.
>
> cp on lines 302 & 305
done.
thanks,
-ethan
