Hi Sundar, Looks ok to me.
sarah **** 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. >> >> 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 >> >
