On 07/09/2013 02:53 AM, Pádraig Brady wrote: > On 07/08/2013 03:53 PM, Anton Ovchinnikov wrote: >> Valgrind shows some memory leaks while launching 'df' without arguments: >> >> ==8809== LEAK SUMMARY: >> ==8809== definitely lost: 48 bytes in 1 blocks >> ==8809== indirectly lost: 16 bytes in 3 blocks >> ... >> >> If I launch 'df -a' valgrind doesn't detect any 'lost' memory. >> As far as I understand, this is not the case for using IF_LINT macro, >> as it's impossible to predict the size of lost memory. >> I believe the following patch eliminates the leak. > > It looks good thanks. > >> One more thing: the inserted code looks similar to the fragment at >> gnulib/lib/mountlist.c:964. What about moving this fragment to >> stand-alone function (free_mount_entry(struct mount_entry *me))? It >> implies exposing this function in mountlist.h. > > Yes it makes sense to expose such a function in gnulib, > as theoretically one could be calling read_file_system_list() > from a long lived process. > > I'll push such a patch to gnulib and will adjust
http://git.sv.gnu.org/gitweb/?p=gnulib.git;a=commitdiff;h=41d1b6c4 > your patch to use it and push in your name. Attached. Note it's updated to free _all_ discarded mount items, not just those being replaced. thanks, Pádraig.
>From 3ebc58cc4370567be7301e02b661a2fe5478d86c Mon Sep 17 00:00:00 2001 From: Anton Ovchinnikov <[email protected]> Date: Thu, 11 Jul 2013 13:44:24 +0100 Subject: [PATCH] df: reduce memory usage when filtering mount entries Avoid Valgrind reports of "definitely lost" items and while at it, free all discarded mount entries to minimize the amount of memory used. * src/df.c (filter_mount_list): Use the newly exported free_mount_entry() from gnulib to free all mount entries as they're discarded. --- src/df.c | 30 +++++++++++++++++++++--------- 1 files changed, 21 insertions(+), 9 deletions(-) diff --git a/src/df.c b/src/df.c index bb11bb4..e018064 100644 --- a/src/df.c +++ b/src/df.c @@ -613,10 +613,11 @@ filter_mount_list (void) struct devlist *devlist_head = NULL; /* Sort all 'wanted' entries into the list devlist_head. */ - for (me = mount_list; me; me = me->me_next) + for (me = mount_list; me;) { struct stat buf; struct devlist *devlist; + struct mount_entry *discard_me = NULL; if (-1 == stat (me->me_mountdir, &buf)) { @@ -635,25 +636,36 @@ filter_mount_list (void) if (devlist) { + discard_me = me; + /* Let the shorter mountdir win. */ if (! strchr (devlist->me->me_devname, '/') || (strlen (devlist->me->me_mountdir) > strlen (me->me_mountdir))) { - /* FIXME: free ME - the others are also not free()d. */ + discard_me = devlist->me; devlist->me = me; } - continue; /* ... with the loop over the mount_list. */ } } } - /* Add the device number to the global list devlist. */ - devlist = xmalloc (sizeof *devlist); - devlist->me = me; - devlist->dev_num = buf.st_dev; - devlist->next = devlist_head; - devlist_head = devlist; + if (discard_me) + { + me = me->me_next; + free_mount_entry (discard_me); + } + else + { + /* Add the device number to the global list devlist. */ + devlist = xmalloc (sizeof *devlist); + devlist->me = me; + devlist->dev_num = buf.st_dev; + devlist->next = devlist_head; + devlist_head = devlist; + + me = me->me_next; + } } /* Finally rebuild the mount_list from the devlist. */ -- 1.7.7.6
