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
