[rrd-developers] [PATCH] rrd_client.c issues

[Tobias Oetiker]

Hi Svante,

Today Svante Signell wrote:

> On Fri, 2013-08-16 at 14:22 +0200, Tobias Oetiker wrote:
> > Hi Svante,
>
> > >
> > > OK, I will continue next with rrd_client.c. Is that one built into the
> > > library librrd*.so* ?
> >
> > yes, but it can also be used standalone
>
> How?

in the sense that it has little dependence on rrdtool and the
license is chosen to allow linking non gnu projects

> > > One issue with rrd_client:get_path is the const definitions. In order to
> > > free the dynamically allocated strings, to avoid memory leaks const has
> > > to be abandoned in some places. OK?
> >
> > I can only tell you when I see the patch ...

if I am reading your code corectly, you are causing a memory leak
by makeing get_path return an allocated string instead of a
constant ... the root cause of the problem is that you are
changeing the API of get_path ...

for backward compatibility this is rather sub optimal ...

better choose a new function name and whoever is using it knows
that they have to free the data they get from the call.

cheers
tobi

> Attached below is a stripped down example from rrd_client.c. Defining
> NOT_CONST in the code and
> gcc -g -Wall test_get_path.c
> valgrind --leak-check=full ./a.out .
> gives:
>
>  valgrind --leak-check=full ./a.out .
> ==5155== Memcheck, a memory error detector
> ==5155== Copyright (C) 2002-2011, and GNU GPL'd, by Julian Seward et al.
> ==5155== Using Valgrind-3.7.0 and LibVEX; rerun with -h for copyright
> info
> ==5155== Command: ./a.out .
> ==5155==
> calling rrd_create
> rrdc_create: filename=.
> filename=.
> get_path: path=.
> absfilename=/my/path/to/rrdtool/rrdtool-git
> rc = 0 (0=OK)
> ==5155==
> ==5155== HEAP SUMMARY:
> ==5155==     in use at exit: 0 bytes in 0 blocks
> ==5155==   total heap usage: 1 allocs, 1 frees, 4,096 bytes allocated
> ==5155==
> ==5155== All heap blocks were freed -- no leaks are possible
> ==5155==
> ==5155== For counts of detected and suppressed errors, rerun with: -v
> ==5155== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 4 from 4)
>
> However uncommenting #undef NOT_CONST results in a small memory leak:
>
> ==5190==
> ==5190== HEAP SUMMARY:
> ==5190==     in use at exit: 4,096 bytes in 1 blocks
> ==5190==   total heap usage: 1 allocs, 0 frees, 4,096 bytes allocated
> ==5190==
> ==5190== 4,096 bytes in 1 blocks are definitely lost in loss record 1 of
> 1
> ==5190==    at 0x4C28BED: malloc
> (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==5190==    by 0x4E6F5F7: realpath@@GLIBC_2.3 (canonicalize.c:79)
> ==5190==    by 0x400842: get_path (test_get_path.c:46)
> ==5190==    by 0x400905: rrdc_create (test_get_path.c:83)
> ==5190==    by 0x400983: rrd_create (test_get_path.c:107)
> ==5190==    by 0x4009F7: main (test_get_path.c:124)
> ==5190==
> ==5190== LEAK SUMMARY:
> ==5190==    definitely lost: 4,096 bytes in 1 blocks
> ==5190==    indirectly lost: 0 bytes in 0 blocks
> ==5190==      possibly lost: 0 bytes in 0 blocks
> ==5190==    still reachable: 0 bytes in 0 blocks
> ==5190==         suppressed: 0 bytes in 0 blocks
> ==5190==
> ==5190== For counts of detected and suppressed errors, rerun with: -v
> ==5190== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 4 from 4)
>
> Heap usage it OK in both cases. What do you think, should the
> memory-leak-free version be used for patching rrd_client.c (I have
> problems to test this directly, see above)
>
>

-- 
Tobi Oetiker, OETIKER+PARTNER AG, Aarweg 15 CH-4600 Olten, Switzerland
http://it.oetiker.ch tobi at oetiker.ch ++41 62 775 9902 / sb: -9900