Sarah Jelinek wrote: > Hi Sundar, > > Looks ok to me. Thanks Sarah.
- Sundar > > 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 >>> >>
