PDA

View Full Version : The wonders of rewrites...



Slougi
June 5th, 2003, 18:25
Before:


char *parsecpu()
{
/*parse /proc/cpuinfo to get cpu model and Mhz*/
FILE *cpu=getfile("/proc/cpuinfo", "r");
char *buffer2=getmem(50);
char *buffer3=getmem(50);
char *buffer1=lineget(5, cpu);
strcpy(buffer2, buffer1+13);
buffer1=lineget(7, cpu);
strcpy(buffer3, buffer1+11);
strcat(buffer2, "@ ");
strcat(buffer2, buffer3);
/*get rid of newlines*/
char *newline=strchr(buffer2, '\n');
while(newline)
{
*(char *)newline=' ';
newline=strchr(buffer2, '\n');
}
free(newline);
free(cpu);
free(buffer1);
free(buffer3);
strcat(buffer2, "MHz");
return buffer2;
}


After:


char *lineget(int line, FILE *fp)
{
char *buffer=getmem(200);
int i=0;
while (i<line)
{
fgets(buffer, 200, fp);
i++;
}
rewind(fp);
return buffer;
}

char *getcpumhz()
{
FILE *fp=getfile("/proc/cpuinfo", "r");
char *buffer=(lineget(7, fp)+11);
return buffer;
}

char *getcpumodel()
{
FILE *fp=getfile("/proc/cpuinfo", "r");
char *buffer=(lineget(5, fp)+13);
return buffer;
}

char *getcpu()
{
char *buffer=getmem(50);
strcpy(buffer, getcpumodel());
strcat(buffer, "@ ");
strcat(buffer, getcpumhz());
strcat(buffer, "MHz");
return buffer;
}

char *removenewlines(char *string)
{
char *buffer=strchr(string, '\n');
while (buffer)
{
*(char *)buffer=' ';
buffer=strchr(string, '\n');
}
return string;
}


Don't slap me for the before code, I wrote that at 3 am, and blight already did slap me anyway:saint:

Cyberman
June 6th, 2003, 16:46
Originally posted by Slougi
Before:


char *parsecpu()
{
/*parse /proc/cpuinfo to get cpu model and Mhz*/
FILE *cpu=getfile("/proc/cpuinfo", "r");
char *buffer2=getmem(50);
char *buffer3=getmem(50);
char *buffer1=lineget(5, cpu);
strcpy(buffer2, buffer1+13);
buffer1=lineget(7, cpu);
strcpy(buffer3, buffer1+11);
strcat(buffer2, "@ ");
strcat(buffer2, buffer3);
/*get rid of newlines*/
char *newline=strchr(buffer2, '\n');
while(newline)
{
*(char *)newline=' ';
newline=strchr(buffer2, '\n');
}
free(newline);
free(cpu);
free(buffer1);
free(buffer3);
strcat(buffer2, "MHz");
return buffer2;
}


After:


char *lineget(int line, FILE *fp)
{
char *buffer=getmem(200);
int i=0;
while (i<line)
{
fgets(buffer, 200, fp);
i++;
}
rewind(fp);
return buffer;
}

char *getcpumhz()
{
FILE *fp=getfile("/proc/cpuinfo", "r");
char *buffer=(lineget(7, fp)+11);
return buffer;
}

char *getcpumodel()
{
FILE *fp=getfile("/proc/cpuinfo", "r");
char *buffer=(lineget(5, fp)+13);
return buffer;
}

char *getcpu()
{
char *buffer=getmem(50);
strcpy(buffer, getcpumodel());
strcat(buffer, "@ ");
strcat(buffer, getcpumhz());
strcat(buffer, "MHz");
return buffer;
}

char *removenewlines(char *string)
{
char *buffer=strchr(string, '\n');
while (buffer)
{
*(char *)buffer=' ';
buffer=strchr(string, '\n');
}
return string;
}


Don't slap me for the before code, I wrote that at 3 am, and blight already did slap me anyway:saint:

Can I slap you for BOTH?
You have a memory leak that could be VERY bad :)

See those getmems? bad Slougi don't do that :)



char *lineget(int line, FILE *fp)
{
char static buffer[200];
//char *buffer=getmem(200);
int i=0;
while (i<line)
{
fgets(buffer, 200, fp);
i++;
}
rewind(fp);
return buffer;
}

char *getcpumhz()
{
FILE *fp=getfile("/proc/cpuinfo", "r");
char *buffer=(lineget(7, fp)+11);
return buffer;
}

char *getcpumodel()
{
FILE *fp=getfile("/proc/cpuinfo", "r");
char *buffer=(lineget(5, fp)+13);
return buffer;
}

char *getcpu()
{
char static buffer[50];
//char *buffer=getmem(50);
strcpy(buffer, getcpumodel());
strcat(buffer, "@ ");
strcat(buffer, getcpumhz());
strcat(buffer, "MHz");
return buffer;
}

char *removenewlines(char *string)
{
char *buffer=strchr(string, '\n');
while (buffer)
{
*(char *)buffer=' ';
buffer=strchr(string, '\n');
}
return string;
}


try that.. each time you call your routines you allocate 200 bytes per line and 50 bytes for each time you call getcpu(). You do not free it.. this is a bad thing.

static inside a function means that this is allocated in the global memory pool but is only accessable within the scope it was defined. Thus it's a 'hidden' buffer that you can abuse without the allocation problems you'll suffer with what you have.

Since I didn't see the rest of the code this is an assumption that you aren't freeing the allocated data, however .. ;)

Cyb

nephalim
June 6th, 2003, 17:48
Originally posted by Cyberman
Can I slap you for BOTH?
You have a memory leak that could be VERY bad :)

See those getmems? bad Slougi don't do that :)

<snip>

try that.. each time you call your routines you allocate 200 bytes per line and 50 bytes for each time you call getcpu(). You do not free it.. this is a bad thing.

static inside a function means that this is allocated in the global memory pool but is only accessable within the scope it was defined. Thus it's a 'hidden' buffer that you can abuse without the allocation problems you'll suffer with what you have.

Since I didn't see the rest of the code this is an assumption that you aren't freeing the allocated data, however .. ;)

Cyb

Even I know this much, and I don't know much! You should declare your variables at the beginning of your code, and be sure to free the allocated data. Variables declared in small function calls should only be used if you are only using them within function only. Even then, that's a very slow choice, you don't want to have to take the extra step of allocating a variable every time you run the function...

Please feel free to correct me if i'm incorrect...

Doomulation
June 6th, 2003, 22:37
On the contrary, then, why don't you always use global variables? The answer is that it's dangerous! If it's a small function, you might concider doing it inline instead.

And remember that of course you need to free allcolated data, but if the data is "local," and not placed on the heap, placed on the stack, it's automatically cleaned up when the function exits. But you already knew that, didn't you?

tooie
June 6th, 2003, 22:44
the problem is passing strings as the results .. I dislike having static members as you have done .. it is the same as global variables .. I find personally for strings if I am returning a string I will return it in std::string .. granted that is c++ only ..

otherwise I pass a pointer to the string and the length and make sure the function only copies to the max amount passed .. It stops the chance of buffer overflows

Cyberman
June 7th, 2003, 16:44
[QUOTE]Originally posted by tooie
the problem is passing strings as the results .. I dislike having static members as you have done .. it is the same as global variables .. I find personally for strings if I am returning a string I will return it in std::string .. granted that is c++ only ..

otherwise I pass a pointer to the string and the length and make sure the function only copies to the max amount passed .. It stops the chance of buffer overflows [/QUOTE

Tooie:
Unlike goto's static function variables aren't considered dangerous. They should be used sparingly. As I said I don't know what the rest of the program looks like so this is a quick fix.

Your programing model is assumptive, that is very dangerous, always program to the needs of what you are dealing with. Sometimes goto's are necessary (obviously you put lots of comments on it when you use it). Sometimes static variables are necessary. As for if it is the same as global, it is not actually, global variables are globally accessable the reason why 'thou shalt not use global variables' is that you can have problems with bits of code twiddling a global variable almost anywhere in your program, and that is a VERY bad thing. You CANNOT do this with a static variable, period, end of story. The use of static is quite common especially when you get into memory leak issues. An example of common C functions that use static variables? I'm glad you asked: from the ANSI C SPEC:
strtok()

#include <string.h>
char *strtok(char *s1, const char *s2);

The first call to strtok returns a pointer to the first character of the first token in s1 and writes a null character into s1 immediately following the returned token. Subsequent calls with null for the first argument will work through the string s1 in this way, until no tokens remain.

The separator string, s2, can be different from call to call.

this uses a STATIC variable.. and it's standard C.. how do I know? because it would be impossible for it to know where it was in your string without retaining a pointer to the string you are parsing.

With C++ objects the allocation issue becomes a bit more stressful, you will have the same problem as before if you aren't careful. This mainly pertains to how your objects were designed though. C++ can have huge memory leak problems so design of something like a string class requires VERY careful programing.

Cyb

tooie
June 7th, 2003, 23:27
I use to use static variables a lot for functions returning strings .. yes they are safe .. there is nothing wrong with them .. I just had issues with them at times when I was in a multithreaded environment .. or I disliked if I had a common function that worked out a string based on the return value .. then you could not use it multiple times in a sprintf statement as all the calls would be returning the same pointer .. If the function is always going to be returning the same string .. great .. if your single thread great .. if not then I am cautious .. granted VC implementation of std::string is not totaly thread safe .. so multiple threads accessing the same string cause massive problems as well .. but if it is a new one and they are not shared then it is ok ...

Cyberman
June 9th, 2003, 05:03
Nope Static variables are not thread safe.

Threads are a problem and I'm not sure how to handle some things with threads. Even creating a new variable each time you execute something could be a problem. I probably should deal with thread issues in a number of my programs (because they have them).

:)

Cyb