if I run shcomp -n on the ksh93 files I see the following: > nadkarni at skinnymoose:/tmp$ shcomp -n create_vm /dev/null > create_vm: warning: line 89: `...` obsolete, use $(...) > create_vm: warning: line 89: " quote may be missing > create_vm: warning: line 90: `...` obsolete, use $(...) > create_vm: warning: line 176: -ir invalid typeset option order > create_vm: warning: line 223: `...` obsolete, use $(...) > create_vm: warning: line 284: `...` obsolete, use $(...) > nadkarni at skinnymoose:/tmp$ shcomp -n export_vm /dev/null > export_vm: warning: line 158: `...` obsolete, use $(...) > export_vm: warning: line 158: `...` obsolete, use $(...) > export_vm: warning: line 213: `...` obsolete, use $(...) > export_vm: warning: line 229: `...` obsolete, use $(...) > export_vm: warning: line 237: `...` obsolete, use $(...) Can you please address these.
-Sanjay Glenn Lagasse wrote: > Hi Jack! > > * Jack Schwartz (Jack.A.Schwartz at Sun.COM) wrote: > >> Hi Glenn and Joe. >> >> Regarding: >> >>> <http://cr.opensolaris.org/~glagasse/webrev.diffs> >>> >> For round 2, all files look good to me, except I still have some comments on >> im_pop.py: >> >> 573: If (QUIT_ON_PKG_FAILURE == True) and ips_set_auth fails, do calls to >> ips_cleanup_authorities() get made to clean up the UNSET_AUTH_LIST? >> Likewise >> for ips_cleanup_mirrors() and UNSET_MIRROR_LIST? Are such calls necessary? >> The main just raises an exception and bails. >> >> Since this is lifted from existing code (DC_tm.py) and you don't feel >> comfortable changing it, I'm happy with your offer to file a bug. >> > > Ok. I'll file a bug. > > >> - - - >> >> The use of PKGFILE between its flush on 523 and its close on 529 still don't >> make sense to me. Close it in one process before using it by another. The >> close will do the flush. What you have will work, but it would be >> cleaner to >> do this: >> >> for pkg in PKGS: >> PKGFILE.write(...) >> PKGFILE.close() >> STATUS = ips_pkg_op(PKG_FILE_NAME, ...) >> >> Likewise for 479-505: >> >> for pkg in PKGS: >> PKGFILE.write(pkg + '\n') >> PKGFILE.close() >> STATUS = ips_contents_verify(PKG_FILE_NAME, ...) >> if STATUS and QUIT_ON_PKG_FAILURE == 'true': >> os.unlink(PKG_FILE_NAME) >> raise (...) >> ... >> STATUS = ips_pkg_op(PKG_FILE_NAME, ...) >> if STATUS and QUIT_ON_PKG_FAILURE == 'true': >> os.unlink(PKG_FILE_NAME) >> raise (...) >> >> os.unlink(PKG_FILE_NAME) >> > > I looked into this, since the transfer module does indeed specifically > open the file itself I'm pretty comfortable with this suggestion and so > I'll make this change since I agree with what you're saying. > > >> - - - >> >> Nit: Change QUIT_ON_PKG_FAILURE to be a boolean instead of a string; >> then you >> can just say >> if STATUS and QUIT_ON_PKG_FAILURE: >> > > I'd like to change this, but it will change quite a few places in the > code and while seemingly innocuous I already have a couple of > regressions in this code that I'm tracking down and I'm leary to make > that situation worse. Since this is existing code and I'm not > regressing anything I'm going to pass on addressing this other than I > can offer to file a low priority bug to clean it up. > > Thanks Jack! > >
