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.
>
> 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.
>
> 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.
>
> 709: Similarly, urls needs to be freed.
Only the pointers to the values should be freed. The actual values are
coming from python function
>
> 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.
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
>
