ok, enough 'storming' about the code. I hope someone can close the topic.
I managed to find the leak in the meanwhile (and yes, I posted the code here because I needed a SECOND opinion).
first of all, some answers:
legalize:
thanks for the tip, I used valgrind (and reported leaks in libpthread which actually aren't leaks, but that's all it reported), next time I'll have a look at purify
Zahlman:
Quote:Just because you "have to" (???) use C is no excuse for not organizing your code better to avoid all the trickiness in the allocation code. And even in C, you can initialize variables at the point of declaration, and don't have to put them all at the top of the function... braces still introduce new scopes.
(Oh, and what exactly is recursive about this?)
1. Maybe the comment about what the function isn't very well formulated. The function is not recursive, but what the function does for path='/a/b/c' is mkdir('/a'); mkdir('/a/b'); mkdir('/a/b/c') and that "looked" like recursive. sorry, my mistake.
2. your code doesn't compile (unless you change FILE into XFILE) :D
3. yes, I provided an extra space for the null terminator :D
4. your second example (without any allocations) segfaults nicely when called like this: utils_mkdir("/a/b/c") (and not utils_mkdir(some_buf)) because you cannot modify the supplied string.
5. I didn't say I "have to" use C, I LIKE using C and sorry, but your 'organised' code looks like C code from a C++ programmer. I try code like that in C++ and C#, but NOT in C/linux. Do an apt-get source any_daemon_you_like and look at how the code is written. Maybe my personal preference isn't the best, but I like to continue writing it like that.
6. casting return from malloc is in general "good C practice" (similar to "using static_cast is in general good C++ practice", each world with his 'habbits'). I'm not a beginner so you don't have to tell me "did you know you don't have to cast the result from malloc()?"
Quote:
You can store the result of malloc into any pointer variable without a cast, because ISO C automatically converts the type void * to another type of pointer when necessary. But the cast is necessary in contexts other than assignment operators or if you might want your code to run in traditional C.
this is taken from
http://www.gnu.org/software/libc/manual/html_node/Basic-Allocation.html.. what can I say, call me a 'traditionalist' ...
Now, for the FINAL answer to the original post:
I managed to solve the problem by using dmalloc (a very nice library, available at
http://dmalloc.com/). It reported 2 free (NULL), one was in libpthread (I guess they know what they're doing) and one in this function.
Took me one day to find the problem:
strsep() was causing it.
I misread the man page for it (no, I don't like remembering by heart all the functions in a library, that's why they invented documentation) and only saw the part which says "it modifies the supplied buffer and replaces token with \0" (that's why I use strdup on the original path).
What I didn't read was
"*stringp is updated to point past the token", which in my case changed the buf pointer so the final free (buf) ended up in free(NULL).
corrected function:
int utils_mkdir (char *path){ char *buf; char *bp; // instead of char **bp... ... bp = buf; while ((tok = strsep(&bp, "/"))) { ...
Sorry if I offended anyone with the original post, but as someone said: you don't have to read the code, NOR answer the post.
I was hoping that someone with a strong C and linux background might point what I'm doing wrong.
And some more infos about the daemon: it is used in production as image server (serves images to a bunch of web servers, from a storage server). We have around 13 million images stored on our servers and 6 million pageviews each day and I couldn't afford to have the storage server get all the 'hits' for images (and client INSISTS on having all the files on one server, they feel "safer" like that - whatever).
So, the system works like this:
- storage server runs this daemon (uses pthread to handle incoming connections)
- web servers run apache and php, whenever a request for 'image x' is received then apache tries to serve the file, if file not found, it runs php script which gets the file from the image server, stores it locally (future requests for the file will be satisfied directly by apache, until file is deleted because the frontend's cache was cleared).
Of course, users can also upload pics and in that case the daemon is used to safely store them on the storage server (after being processed on a web server, resize, etc.)
Maybe not the 'most elegant' way to do it (also, not the 'ugliest' way to do it, the person who handled this before me used NFS and had problems with 10 webservers crashing because one of the two storage server wasn't responding) but it runs very well (since january until now it served millions of request, I said the daemon runs since june because I restarted it then and started wondering about the vmSize of the process).
Thanks for all your replies.
Q: How many programmers does it take to write a nice piece of software?A: MORE.
