Hi Sundar.
On 12/10/09 11:17, Sundar Yamunachari wrote:
> Jack,
>
> Thanks for the review. Please see my comments below.
> Jack Schwartz wrote:
>> Hi Sundar.
>>
>> Here are my comments.
> I have updated the webrev based on your and Sarah's comments. It is in
> the same place http://cr.opensolaris.org/~ysundar/12659/. Please check
> it and let me know.
>>
>> auto_install.c:
>>
>> Nit: 761-764: The "_t" of auto_repo_info_t and auto_mirror_repo_t is
>> of different form than auto_disk_info of 744 and auto_sc_params of
>> 745. (Personally I like your way better, but it is inconsistent with
>> what is there.)
> I agree with you. This module has few definitions with _t and few with
> out _t. Please take a look at auto_install.h. I also looked at the
> /usr/include/sys/*h files to see some guidance. I don't find a
> clear-cut answer. I would like to leave it as defined.
OK.
>>
>> 1065: May be clearer to say "pkg install and remove"
> Okay.
>>
>> Nit: 1285: extra space before \n not needed
> okay.
>>
>> auto_install.h:
>>
>> 188-190: This can be rewritten without ".."
>> #define AIM_ADD_DEFAULT_URL_PUBLISHER_MIRROR
>> "ai_manifest/ai_pkg_repo_default_publisher/" \
>> "[main/url=\"%s\"]/mirror/url"
>>
>> Likewise for 196-198.
>> Likewise for 204-205.
>> Likewise for 211-212
> Thanks for the suggestions. I have updated and tested it.
Looks good now.
>>
>> auto_parse.c:
>>
>> 545: Looks like you need to call ai_get_manifest_values() here.
> Right. I will fix it.
>>
>> 673: current_url needs to be freed under get_out
>> Actually, why is current_url strduped to default_url? Why is a dup
>> required?
> The ai_get_manifest_*() overwrite the values from the previous
> ai_get_manifest_*(). It looks like it reuses the same memory. If I
> don't save the results, it will be garbage the next time I call
> another ai_get_manifest_*(), which calls ai_lookup_manifest_values().
>>
>> Both actually need to be freed... (I checked
>> ai_lookup_manifest_values();
>> it mallocs the memory for current_url.
> It allocates only the pointers and not the actual values.
Oops. OK. You are correct.
>>
>> 709: Similarly, urls needs to be freed.
> Only the pointers to the values should be freed. The actual values are
> coming from python function
OK
>>
>> Freeing memory allocated ultimately by ai_lookup_manifest_values
>> seems to be
>> a more global problem. Is there a bug filed for it?
> There are two kinds of usage in auto_parse.c. One return single value
> (char *) and another array of values (char **). I have changed so that
> the functions returning (char *) will free the ptr in the function and
> the functions returning char ** expect the caller to free the pointer.
OK.
BTW, I just noticed that "mirror" around lines 721-722 can be removed.
You can init tmp_ptr to NULL, and use it in the IF condition at 721, and
return ptr instead of mirror on 730.
Not a big deal; don't worry about it if you don't have time.
Rest looks fine.
Thanks,
Jack
>
> Thanks,
> Sundar
>>
>> Other files look OK to me.
>>
>> Thanks,
>> Jack
>>
>>
>>
>>
>>
>> On 12/06/09 22:37, 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
>>
>
