Hi Jack, Thank you for doing the code review so quickly! Please see my responses inline below.
Jack Schwartz wrote: > Hi Karen. > > I'm impressed by your fast work! Here are my comments: > > transfermod.h: OK > > transfer_mod.py: > > 1166: Please check that this option is used only for set-authority and > throw an exception if not. > Done. > transfer_defs.ph: OK > > ict.py: OK. > > DC_tm.py: > > ~97: Just so that I am clear, the call to DC_ips_refresh() is removed > here because it doesn't make any sense to do a refresh after unsetting > an authority. Is that correct? Or is it because another authority > (mirror, for example) is reinstated and would normally get a catalog > refresh but we don't want to do this? It is removed here, because there's no caller to this function anymore. Don't want to have a function there that's not going to be called by anything. Previously, the function is for calling "pkg refresh" explicitly after every "pkg set-authority" to make sure that the authority/mirror specified in the manifest is valid. > > 144: I don't see the reason for differentiating preferred vs mirror > authority here. All that's being done is to validate that the > authority at the URL given has a viable repo. Whether that repo is > being used as a preferred repo or mirror shouldn't matter, right? If > it indeed doesn't make a difference, you can just call DC_ips_set_auth > and get rid of DC_ips_validate_auth. Yes it does matter. If it is a pref authority, I am actually calling "pkg image-create...." so that the "validate mountpoint" is setup. If it is not a preferred repo, then, "pkg set-authority" is called on the "validate mount point" so that we can validate whatever is specified is valid. Notice that for this validate call, the --no-refresh is not specified for pkg set-authority so the alternate repo/mirror will get contacted to get the catalog refreshed. > > 386-389: Nit: Please add a space after the #. Done. > > 396: It seems cleaner to me to unconditionally cleanup the temporary > directory used for validation, inside DC_ips_validate_auth(), since > you'll always be deleting this directory after ...validate_auth() > completes. This will allow you to eliminate the many calls to > cleanup_dir() in DC_populate_pkg_image(). > Actually, I can not do that, because the validate_mntpt is used over and over again, and its state is dependent on the previous call. The validate_mntpt gets setup when we validate the post-install preferred authority. Then, all subsequent mirror or alternate authority are set up on the validate mount point. So, it can't be deleted until we are finally done with validating everything. All the cleanup_dir() stuff is to clean it up in case we need to exit because of error. The updated webrev is here in case you want to check out the change to transfer_mod.py http://cr.opensolaris.org/~ktung/4550_fix_update/ Thanks again for your review. --Karen > Thanks, > Jack > > > > Karen Tung wrote: >> Hi, >> >> Can I have 2 reviews for the changes to fix bug 4550 asap? >> >> 4550 'pkg uninstall SUNWslim-utils' fails with osol-0811-101a-rc1a.iso >> http://defect.opensolaris.org/bz/show_bug.cgi?id=4550 >> >> webrev: >> >> http://cr.opensolaris.org/~ktung/4550_fix/ >> >> Thanks, >> >> --Karen >> _______________________________________________ >> caiman-discuss mailing list >> caiman-discuss at opensolaris.org >> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >> >
