Hi Sundar.
Here are my comments.
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.)
1065: May be clearer to say "pkg install and remove"
Nit: 1285: extra space before \n not needed
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
auto_parse.c:
545: Looks like you need to call ai_get_manifest_values() here.
673: current_url needs to be freed under get_out
Actually, why is current_url strduped to default_url? Why is a dup required?
Both actually need to be freed... (I checked ai_lookup_manifest_values();
it mallocs the memory for current_url.
709: Similarly, urls needs to be freed.
Freeing memory allocated ultimately by ai_lookup_manifest_values seems to be
a more global problem. Is there a bug filed for it?
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
