HI Karen.
A few more comments inline.
Karen Tung wrote:
> 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.
Maybe I didn't ask the question right... What I was referring to was
the DC_ips_refresh() call on line 98, done after the UNSET_AUTH. Were
lines 97-100 removed because they don't make sense after unsetting an
authority, or for a different reason?
>>
>> 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.
OK. Yes, one calls TM_IPS_INIT and the other calls TM_IPS_SET_AUTH.
>>
>> 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.
OK, by extension of your answer to line 144 above.
Looks good to me.
Thanks,
Jack
>
> 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
>>>
>>
>
