Sarah,
Thanks for the review.
Sarah Jelinek wrote:
> Hi Sundar,
>
> I reviewed this... comments below:
>
>
> auto_install.c:
> configure_ips_init_nv_list
> configure_ips_addl_publisher
>
> you should check to be sure that the data, specifically
> auto_repo_info_t * is not null.
I can check in those functions but they won't be NULL in these specific
cases since default_ips_repo is passed to configure_ips_init_nv_list()
and it is checked at line 1031. The "for loop at line 1113" checks for
the value of the ptr that is being passed to configure_ips_addl_publisher()
>
> auto_parse.c:
> ai_get_manifest_repo_publisher()
>> 535 + * If publisher is not supplied, check for authority
>> 536 + */
>> 537 + if (len <= 0) {
>> 538 + if (is_default_publisher) {
>> 539 + snprintf(tag, sizeof (tag),
>> 540 + AIM_ADD_DEFAULT_URL_AUTH_NAME,
>> url);
>> 541 + } else {
>> 542 + snprintf(tag, sizeof (tag),
>> 543 + AIM_ADD_ADDL_URL_AUTH_NAME, url);
>> 544 + }
>> 545 + }
>
>
> Don't we need to call ai_get_manifest_values(tag, &len) again here
> since we are now looking for the authority as opposed to the publisher
> for backward compatibility?
Yes. You are right. I will fix that.
>
> The addition of:
> AIM_ADD_XXX macros in auto_install.h are actually more confusing to
> me. I see what you are doing, but it seems confusing to see these, and
> wonder why they are different than the previously defined AIM_IPS_XXX.
Before we parsed only one additional publisher. We take the first entry
and ignore the rest. I added these new definition to find publisher
name and mirrors for each publisher based on publisher url. So we use
the url and find the entry and go from there.
>
> On this note.. the way we are specifying the tags to find seems wrong.
> But, I may be wrong :-). Why do we need to need to specify this tag
> this way:
>
>> 188 +#define AIM_ADD_DEFAULT_URL_PUBLISHER_MIRROR \
>> 189 + "ai_manifest/ai_pkg_repo_default_publisher/" \
>> 190 + "main[url=\"%s\"]/../mirror/url"
>
> In particular the traversing up one level in our specification with '..'.
After the substitution of url, we will at
ai_manifest/ai_pkg_repo_default_publisher/main level. The mirror at the
same level as that of main. We have to go to
ai_manifest/ai_pkg_repo_default_publisher/mirror level. That is why
there is .. there. I talked to Jack and he told me that we can do
differently to avoid '..'. I will check with him and update you.
Thanks,
Sundar
>
> thanks,
> sarah
> ****
>
>
>
> sundar Yamunachari wrote:
>> Ethan, Alok and William,
>>
>> Please do a code review for the bugs 12659 and 13112. This is to
>> allow more than one additional publishers to be specified with AI
>> manifest.
>>
>> The webrev is at: http://cr.opensolaris.org/~ysundar/12659/
>>
>> The bugs are at
>> http://defect.opensolaris.org/bz/show_bug.cgi?id=12659 and
>> http://defect.opensolaris.org/bz/show_bug.cgi?id=13112
>>
>> Thanks,
>> Sundar
>> _______________________________________________
>> caiman-discuss mailing list
>> caiman-discuss at opensolaris.org
>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
