Great Ethan!
Thanks for addressing these issues.
Suggestion:
-----------
It might be good to broaden the RFE you plan to file to include better
trap handling in our AI scripts too.
Joe
Ethan Quach wrote:
> 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
>
