Jack Schwartz wrote: > Hi Evan. > > Here are my comments: > > be_utils.c: > > 3453-3461: have you considered using dirname(3C) for this?
No I hadn't. Good suggestion since I'm already including libgen for mkdirp(). Thanks for thinking of this! > > 3459: Is this an appropriate error, or would it be better to have an > error that the menu_path is invalid as it is missing "menu.lst"? Hmmm. That fact that the path is invalid does mean that there is no available menu.lst. That being said it probably makes sense to indicate here a bit more clearly why the file is missing. I'll add a new error for this stating that the path to the menu file is invalid. > > tbeadm/Makefile: > > How come both Makefiles need the change to include libgen? Does > be_utils.c get compiled by both Makefiles? The test binary tbeadm needs this due to the fact that it's linking in the static library libbe.a > > ict.py: > > 305: typo: durring -> during Fixed > > 328: logic appears inverted Inverted in what way? If this is being run as part of a liveCD or Automated install then we don't want to allow the use of "/" as the base dir. > > 334,336: I would expect different comments for these two lines as 334 > has BASEDIR added Yes I need to add more comments around this. joe mentioned that this should have the comment that this is done here to accomadate the possibility that this will be run for creating the menu.lst file on a separate pool and as such should not be using the BASEDIR when setting self.BOOTENVRC. I'll add this comment. Thanks! -evan > > Thanks, > Jack > > > On 10/09/09 00:47, Evan Layton wrote: >> I need to get reviews for the following bugs that are causing failures >> if the directories where the boot/grub menu files live are missing >> (/rpool/boot and /rpool/boot/grub). Also this fix resolves the issue >> with the add_splash_image_to_grub_menu that was causing a failure to >> create a menu.lst file when it's missing. >> >> >> The bugs: >> http://defect.opensolaris.org/bz/show_bug.cgi?id=7880 >> http://defect.opensolaris.org/bz/show_bug.cgi?id=11436 >> >> >> The webrev: >> http://cr.opensolaris.org/~evanl/7880/ >> >> >> Thanks, >> -evan >> >> _______________________________________________ >> caiman-discuss mailing list >> caiman-discuss at opensolaris.org >> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >
