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! -- Glenn
