Clay Baenziger wrote: > Hi Joe, > Here's my findings: > On line 200 directy -> directory
fixed > On line 232 you set tmpdir_existed='NO', since you set the error > handler on line 249 but don't check to set tmpdir_existed='YES' > until 270 as this script evolves a race could develop where > we'd blow away the tmpdir unless I'm missing something. (Why > not just set tmpdir_existed to the correct state on > initialization or to yes for safety's sake and change line 270) good catch. fixed > > These two comments are probably best rolled into the one below for > lines 254-255 > On line 255 why not use (( )) ksh arithmetic as used elsewhere, > so if (( $? != 0 )); then...? > On line 254 this checks for a uid=0, uid=1, uid=2... uid=9 it > seems I think you want id |grep -q 'uid=0(root)'? done > > To replace lines 254-255 why not combine the two lines as: > "if $(id | grep -q "uid=0(root)"); then..." > (otherwise not being root causes an error from grep exiting > uncaught with a non-zero exit and invokes the error handler > instead of printing the you must be root error) > > On line 271, 276; if mkdir fails we won't get to the if statements > below checking to see if the directories exist (as the error > handler will be invoked and kick us out first (with no mention > of where we died (why not just check the return of mkdir or > are there cases where mkdir won't return an exit status on > failure? As is if mkdir fails, we'd just get: > ------------------------------------------------------ > Error: > > /export/backup/usbgen > Error or interrupt encountered. Exiting > ------------------------------------------------------ As we discussed... I've tested this and it works well as is... > > On line 297 shouldn't this use the (( )) ksh arithmetic as used > elsewhere, so "if (( usb_size == 1 )); then...fi"? fixed > On line 346 why use printf when print is used everywhere else? > For example, print "=== $0 finished at $(date)". fixed. consistently using print > > Very cool job rescripting this in ksh93 and I like the trapping > and other cool bits. Let me know where I probably got confused. > > Thank you, > Clay Huge thanks you! for the help! Joe > > On Thu, 15 May 2008, Joseph J VLcek wrote: > >> Joseph J VLcek wrote: >>> Roland, and anyone else interested... >>> >>> Please review: >>> >>> http://cr.opensolaris.org/~joev/bug1462 >>> >>> >>> which addresses >>> >>> >>> http://defect.opensolaris.org/bz/show_bug.cgi?id=1462 >>> >>> This change represents a fairly extensive re-write of usbgen. >>> >>> >>> Testing done: >>> >>> [1] Ran full Distro Constructor build >>> [2] Built usb images using OpenSolaris-2008-05_rc3.iso >>> [3] Because I added error handling I manually induced error into the >>> script to confirm the error handling. I then removed the errors. >>> [4] usbcopy-ed usb image to usb-stick, booted on test machine and ran >>> full Install. >>> >>> >>> Any and all input/suggestions most welcome. >>> >>> Thanks to Roland for the input he has already provided. >>> >>> Joe >>> >> I've addressed all review input received so far. Thanks Jean. >> >> The new webrev is at: >> >> http://cr.opensolaris.org/~joev/bug1462_2nd >> >> These changes addresses two bugs: >> >> http://defect.opensolaris.org/bz/show_bug.cgi?id=1402 >> http://defect.opensolaris.org/bz/show_bug.cgi?id=1462 >> >> >> >> _______________________________________________ >> caiman-discuss mailing list >> caiman-discuss at opensolaris.org >> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >>
