Re: [dev] [DWM] [dwmstatus] readname + gettemperature

From: Martti Kühne <mysatyre_AT_gmail.com>
Date: Thu, 3 Jan 2013 07:00:10 +0100

On Thu, Jan 3, 2013 at 12:07 AM, <linuxien_AT_legtux.org> wrote:
> Good evening,
>
>
> I created a status bar with official link on suckless.org. I used
> gettemperature pattern with readline (from the page) and it works fine.
>
> Anyway, when I control memory leak with valgrind, 7 bytes are lost due to
> gettemperature (it increases every 3 seconds sleep). I tried to add free(),
> but it doesn't work. I don't understand.
>
> Maybe you could take a look.
>
>


eww, or, probably more to the point, welcome to c programming.

first of all, your smprintf function allocates heap memory and you're
using it in multiple levels, and worse, you're using multiple levels
of calls for a simple task at all. I get lost in it only from looking
at it. No wonter that leaves things lie dead. The part that gets lost
is - as your valgrind log clearly states - allocated by readfile()
line 104 and not handled anywhere in gettemperature() until 124.
Seriously, consider to use stack / decent stack-friendly data types -
that means: fixed-sized without having to malloc() and free() stuff -
where you don't want to mess with these system calls all the time. I'm
not sure why you don't rather have a function return a double by
reference there and have the char buffer[fixedsize] discarded in
function scope. fscanf() would probably not even ask that much.

--- /tmp/dwmstatus_leg.c 2013-01-03 06:32:37.163029095 +0100
+++ /tmp/dwmstatus_leg_new.c 2013-01-03 06:39:12.413038061 +0100
_AT_@ -116,12 +116,14 @@
 char *
 gettemperature(char *base, char *sensor)
 {
- char *co;
+ char *co, *ret;
        co = readfile(base, sensor);
        if (co == NULL)
                return smprintf("");

- return smprintf(TEMPCOL, atof(co) / 1000);
+ ret = smprintf(TEMPCOL, atof(co) / 1000);
+ free(co);
+ return ret;

 }

I still believe it is wrong to help you this way though, because you
should write things in a way you find them yourself, that's what C is
about.
The code is terrible as long as you don't know why things go awry.
After, it still may be bad, but you're in charge and find any
errors...

cheers!
mar77i
Received on Thu Jan 03 2013 - 07:00:10 CET