Hi Alan, I am happy with your answers. I am glad to hear that all memory leaks have been resolved.
Rao. Alan Maguire wrote: > hi Rao > > many thanks for the review! comments inline... > > Rao Shoaib wrote: >> Hi Alan, >> >> Here are my comments. A cscope would have been helpful in making sure >> that the memory is being freed and that arguments match etc ... but I >> did the best I could. >> >> >> usr/src/cmd/cmd-inet/lib/netcfgd/Makefile >> usr/src/cmd/cmd-inet/lib/netcfgd/netcfgd.c >> usr/src/cmd/cmd-inet/lib/nwamd/door_if.c >> >> I don't see why we have to use a constant and truncate the list. >> > I was trying to simplify memory management relating > to door communication. Formerly we'd alloca() a buffer > for the one case where the buffer size was not predetermined, > which was the case where we return scan results. In all other cases > we'd just fill in the buffer passed via the door_call(). I changed > things here to use the passed-in buffer to simplify the code, and > given that NWAMD_MAX_NUM_WLANS is 64, and that it's unlikely > that we'll see 64 WLANs in the scan data, it seemed a reasonable > simplification. As Michael noted, we can skip the memcpy() if > num_wlans is 0. >> usr/src/cmd/cmd-inet/lib/nwamd/events.h >> usr/src/cmd/cmd-inet/lib/nwamd/ncu_phys.c >> usr/src/lib/libnwam/Makefile >> usr/src/lib/libnwam/common/libnwam.h >> usr/src/lib/libnwam/common/libnwam_audit.c >> usr/src/lib/libnwam/common/libnwam_backend.c >> line #229 -- How does strlen helps ?. it wont work if the string not >> null terminated in the first place >> > these strlen()s are used to determine if > the char array argument is zero length - > this indicates the specified argument in > the door request is not set, so when > calling into the files backend, we use NULL. > The door calls ensure that the argument is > either a zero-length string or a null-terminated > string. It might be cleaner to move the > zero-length tests into the files backend functions > themselves so we don't need to do this conversion. >> >> usr/src/lib/libnwam/common/libnwam_enm.c* >> *usr/src/lib/libnwam/common/libnwam_events.c >> usr/src/lib/libnwam/common/libnwam_files.c >> It seems that after the call to nwam_line_to_object (line #478) the >> memory associated with proplist is not always freed. For example if >> the if statement at line 485 is entered but the name does not match >> nwam_free_object_list is not being called. >> > good catch - I've added an else clause at ln498 to > handle the case where the name doesn't match. >> >> line #815: Looking at line 794 Should objlist be freed ? >> > It should indeed, thanks! >> >> usr/src/lib/libnwam/common/libnwam_impl.h >> Nit: Add an empty line b/w line #54 and #55 >> > done. >> usr/src/lib/libnwam/common/libnwam_known_wlan.c >> >> line #159: get_wlans_cb() -- It does not immediately resets wil->list >> pointer after realloc(), so in case of error wil->list is not updated. >> > So I think the right answer here is to update wil->list > around ln174 so that if the realloc() fails, we're still pointing > at the original block of memory, but if it succeeds, and > something else fails, we're pointing at the (possibly moved) > list. >> line #200: nwam_walk_known_wlans() seems too complicated -- why does >> it have to call itself, why cant it simply call nwam_walk or >> get_wlans_cb directly. I am sure there is a good reason. Can some >> comments be added. >> > Sure. The short answer is that it needs to support walking > WLANs in priority order, so to do so we need to first walk > all the WLANs to retrieve their names/priority order, then > walk that list. >> usr/src/lib/libnwam/common/libnwam_loc.c >> usr/src/lib/libnwam/common/libnwam_ncp.c >> >> line# 180: nwam_ncp_create() should the handle be freed in case of an >> error or is the caller going to free it ? > Callers should expect all memory allocated by a function > to be freed if a function fails, so I've modified the function > to free the NCP handle if the function fails. >> To many calls to other functions I need cscope to verify all memory >> is being freed. >> >> usr/src/lib/libnwam/common/libnwam_object.c >> >> line #144:nwam_walk() Why is objlist is not freed in all error cases. >> > If the nwam_read_object_from_backend() fails, objlist > is never allocated. We then free objlist on ln185, so > any subsequent error conditions don't need to free it. >> usr/src/lib/libnwam/common/libnwam_priv.h >> usr/src/lib/libnwam/common/libnwam_util.c >> usr/src/lib/libnwam/common/libnwam_values.c >> usr/src/lib/libnwam/common/libnwam_wlan.c >> usr/src/lib/libnwam/common/llib-lnwam >> usr/src/lib/libnwam/common/mapfile-vers >> >> I just saw your email about cscope. I am almost done now so hopefully >> these comments will help. >> > They certainly do. I've updated the webrev with > these changes (and the ones Michael suggested). > It looks like these suggestions, plus a few other > memory leak issues I found, have fixed things. I > see some oversized leaks, but I'm wondering if > these are false positives from ::findleaks (possibly > relating to mapping in shared libraries). > > Thanks again! > > Alan
