Make,
Thanks for reviewing this. My responses are inline.
Mike Gerdts wrote:
On Wed, Jun 10, 2009 at 2:18 PM, Jerry Jelinek <[email protected]> wrote:
I need another code review for the p2v support for
zones using IPS. This is bug:
6793 p2v support for ipkg-branded zones
I think I reviewed this before, but with another look I've found a few
items. The only one that is likely to cause problems in a sane
environment is the last one. The rest are probably nits for things
that would (at worst) be problematic only if files or directories have
white space or PATH has buggy utilities ahead of /usr/bin.
image_install
general:
- there seems to be inconsistent use of variable quoting within [[ ]]
- variables should always be quoted when put on the command line
Fixed.
26 - 31: should probably move PATH setting ahead of ". <file>" so that
commands in the included files (e.g. gettext) don't come from the
wrong place.
I moved these lines.
40, 42, 50, 88, ..: sometimes "error" sometimes "ERROR"
I changed these all to consistently say "Error".
166-193: most variables are unset before getopt, but install_media,
preserve_zone, unconfig_zone aren't
preserve_zone and unconfig zone were, at line 180, but I moved these
to be all together. I also added install_media.
233: avoid fork with:
if [[ "$install_media" != /* ]]
Changed.
403-418: I would use "case $filetype in ... esac" instead of "if elif
elif ... fi"
Changed.
498: s/0/$ZONE_SUBPROC_OK/ ?
Changed.
p2v
General:
- either set PATH or specify complete path to commands (e.g. calls to
gettext, lines 218, 629, 632, etc.). Setting PATH and not specifying
full paths to commands allows for use of built-ins as more are added
to ksh.
Fixed.
533: What about TERM, HUP, etc? Perhaps EXIT would be best...
Changed.
src/pkgdefs/SUNWipkg-brand/prototype
6-7: *.conf files have comments that look like they should be type e not f.
Thanks for catching that, fixed. I also had to update the src/pkgdefs/Makefile
to make it handle editable files in the check target.
I have posted an updated webrev at the same url. Thanks again for the review,
Jerry
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
