Tips, comments to improve my code

hi

in my main function i do:
searchFile(fd, mnt_dir_led, "txt");

my program call are:

searchFile
|-----------------readFile
|---------------------- analyzeFilename(int

i don't seem to have memory leak

i do a infinit loop on searchFile and i checked memory with top and the memory don't grow...

i post my code to have comments, tips...

Code:

---

             
int searchFile(int fd, char *directory, char *ext)
{
    DIR *dirh;
    struct dirent *dirinfo;
    char *file_ext;
    int num = 0;
    int trouve=0;
    dirh = opendir(directory);
    char *tmp[2];
    int size[2];
    while(dirh)
    {
        if((dirinfo = readdir(dirh)) != NULL)
        {
            if((file_ext = strrchr(dirinfo->d_name, '.')) != NULL)
                file_ext++;
            else
                file_ext = dirinfo->d_name;
     
            if(!strcmp(file_ext, ext))
            {
                trouve++;
                if( trouve == 1 ){
                    sleep(3);
                 
                    size[0]=strlen(dirinfo->d_name) + 1;
                    if ( ( tmp[0] = malloc(size[0] ) ) == NULL )
                        exit( EXIT_FAILURE );
                    tmp[0] =  dirinfo->d_name;                     
                }
                else
                { 
                    size[1]=strlen(dirinfo->d_name) + 1;
                    if ( ( tmp[1] = malloc( size[1] ) ) == NULL )
                        exit( EXIT_FAILURE );
                    tmp[1] =  dirinfo->d_name; 
                                         
                    readFile(fd, directory, tmp, strlen(directory), size);
                    sleep(3);
                    free(tmp);
                    trouve==0;                 
                }
            }
            num++;
        }
        else break;
    }
    closedir(dirh); 
    return num;
}

---

my readFile function

Code:

---

/* read the text file */
void readFile(int fd,char *directory, char *filename[], int sizedir, int sizefile[])
{
    FILE *fp;
 
    char *line[2];
   
    char *tmp;
    int i;
    int size[2];
       
    if ( ( tmp = malloc( sizedir +  sizefile[0] + 1) ) == NULL )
      exit( EXIT_FAILURE ); 
       
    strcpy(tmp, directory);
    strcat(tmp, "/");
    strcat(tmp, filename[0]);
       
    fp = fopen(tmp, "r");
    if(fp != NULL)
    {     
        if ( ( line[0] = malloc( LINE_MAX) + 1) == NULL )
            exit( EXIT_FAILURE );
       
        fgets(line[0], LINE_MAX, fp);
        fclose(fp);           
        free(tmp);
       
        if ( ( line[1] = malloc( LINE_MAX) + 1 ) == NULL )
            exit( EXIT_FAILURE );
           
        if ( ( tmp = malloc( sizedir + sizefile[1] + 1) ) == NULL )
            exit( EXIT_FAILURE );
           
            strcpy(tmp, directory);
            strcat(tmp, "/");
            strcat(tmp, filename[1]);
           
            fp = fopen(tmp, "r");
            if(fp != NULL)
            {
                fgets(line[1], LINE_MAX, fp);
                size[0]= strlen(line[0]);
                size[1]= strlen(line[1]);

               
                analyzeFilename(fd, filename, line, size);
                fclose(fp);
            }
            else
                fprintf(stderr, "%s - Not able to open the file: %s\n", strerror(errno),filename[1] );         
        }
    else
        fprintf(stderr, "%s - Not able to open the file: %s\n", strerror(errno),filename[0]);   
   
    free(tmp);
}

---

my analyzeFilename function

Code:

---

void analyzeFilename(int fd, char *filename[], char *led_line[], int size[])
{
    #ifdef DEBUG
            printf("analyse\n");
    #endif

    char CMD_INIT[]          = { '\0', '\0', '\0', '\0', '\0', '\1', 'Z',  '0',  '0',  '\2'  };
    char CMD_TEXT_FILE_TYPE[ ] = { 'A' };
       
    char CMD_FILE_LABEL        = *filename[ 0 ];

    char CMD_TOP[]            = {'\33', '\42' };
    char CMD_BOT[]            = {'\33', '\46' };
       
    char CMD_HOLD[]            = { '\142' };
   
    char CMD_TXT_RED[]        = {'\34', '\61' };//61
    char CMD_TXT_GREEN[]      = {'\34', '\61' };//62
    char CMD_TXT_YELLOW[]      = {'\34', '\61' };//63
       
    char CMD_END[]            = {'\04' };
   
    int color[2]={0,0};
    int position[2]={0,0};
    int mode[2]={0,0}; 
   
    int i;
   
    char *ALL, *p;
   
    size_t len = sizeof CMD_INIT + sizeof CMD_TEXT_FILE_TYPE +
                  sizeof CMD_FILE_LABEL + size[0] + size[1] + sizeof CMD_END;
               
    char *tokenptr;
    char *seperators="_";
    for(i=0;i<2;i++)
    {
        tokenptr = strtok(filename[i],seperators);     
        while (tokenptr != NULL)                   
        {               
            tokenptr = strtok(NULL,seperators);     
       
            if(tokenptr!=NULL)
            {     
                if(strcmp(tokenptr,"L1")==0)
                { 
                    len += sizeof CMD_TOP;
                    position[i]=1;
                }
                else if(strcmp(tokenptr,"L2")==0)
                { 
                    len += sizeof CMD_BOT;
                    position[i]=2; 
                }
                else if(strcmp(tokenptr,"HLD")==0)
                {
                    len += sizeof CMD_HOLD;
                    mode[i] = 1;   
                }
                else if (strcmp(tokenptr,"GRN")==0)
                {
                    len += sizeof CMD_TXT_GREEN;
                    color[i] = 1;
                }
                else if (strcmp(tokenptr,"YEL")==0)
                {
                    len += sizeof CMD_TXT_YELLOW;
                    color[i] = 2;
                }
                else if (strcmp(tokenptr,"RED")==0)
                {
                    len += sizeof CMD_TXT_RED;
                    color[i] = 3;
                }
            }
        }
    }   
   
    if ( ( ALL = p = malloc( len ) ) == NULL )
            exit( EXIT_FAILURE );
   
    memcpy( p, CMD_INIT, sizeof CMD_INIT );
    p += sizeof CMD_INIT; 
       
    memcpy( p, CMD_TEXT_FILE_TYPE, sizeof CMD_TEXT_FILE_TYPE );
    p += sizeof CMD_TEXT_FILE_TYPE;
       
    *p++ = CMD_FILE_LABEL;
           
    for(i=0;i<2;i++)
    {                     
        /* text position*/ 
        if(position[i]==1)
        {
            memcpy ( p, CMD_TOP, sizeof CMD_TOP );
        p += sizeof CMD_TOP;
        }
        else if(position[i]==2)
        {
            memcpy ( p, CMD_BOT, sizeof CMD_BOT );
            p += sizeof CMD_BOT;
        }       
        /*text mode display*/
        if(mode[i]==1)
        {
            memcpy ( p, CMD_HOLD, sizeof CMD_HOLD );
            p += sizeof CMD_HOLD;
        }           
        /* text color*/
        if(color[i]==1)
        {
            memcpy( p, CMD_TXT_GREEN, sizeof CMD_TXT_GREEN );
            p += sizeof CMD_TXT_GREEN;
        }
        else if(color[i]==2)
        {
            memcpy( p, CMD_TXT_YELLOW, sizeof CMD_TXT_YELLOW );
            p += sizeof CMD_TXT_YELLOW;
        }
        else if(color[i]==3)
        {
            memcpy( p, CMD_TXT_RED, sizeof CMD_TXT_RED );
            p += sizeof CMD_TXT_RED;
        }             
        memcpy( p, led_line[i], size[i] );
        p += size[i];         
    }     
    memcpy( p, CMD_END, sizeof CMD_END );
       
    free( ALL ); 
}

---

thanks

Well lets deal with the first function first.

> while(dirh)
> {
> if((dirinfo = readdir(dirh)) != NULL)
dirh never changes, so you could do this, and save yourself a 'break' at the end

Code:

---

if ( dirh ) {
  while ( (dirinfo = readdir(dirh)) != NULL ) {
    /* your code */
  }
}

---

> tmp[0] = dirinfo->d_name;
Two things wrong here
1. You don't actually copy the string, all you do is lose the pointer which you allocated (this is bad)
2. dirinfo->d_name may be a static buffer, so every filename you get is stored in the same block of memory (as allocated by readdir()). This too is bad.
You should have
strcpy( tmp[0], dirinfo->d_name );
Ditto for tmp[1] as well.

> free(tmp);
Very bad!
You didn't allocate this, so you definitely shouldn't free it.
What you should be freeing is tmp[0] and tmp[1]

As in
free( tmp[0] ); free ( tmp[1] );
tmp[0] = NULL; tmp[1] = NULL;

> else break;
Also, consider what to do with any memory you've allocated if you go round the loop an odd number of times.

> trouve==0;
Compares trouve to 0, and throws the answer away
You probably want
trouve=0;

> if ( ( tmp = malloc( sizedir + sizefile[0] + 1) ) == NULL )
You're 1 byte short.
That +1 accounts for the "/" in the following strcat, but you also need to count the \0 as well.

Probably some other stuff, more later no doubt.

Quote:

---

Originally Posted by Salem

You should have
strcpy( tmp[0], dirinfo->d_name );
Ditto for tmp[1] as well.

---

ok done

Quote:

---

Originally Posted by Salem

Very bad!
You didn't allocate this, so you definitely shouldn't free it.
What you should be freeing is tmp[0] and tmp[1]

---

ok done

Quote:

---

Originally Posted by Salem

> trouve==0;
Compares trouve to 0, and throws the answer away
You probably want
trouve=0;

---

ok done

Quote:

---

Originally Posted by Salem

> if ( ( tmp = malloc( sizedir + sizefile[0] + 1) ) == NULL )
You're 1 byte short.
That +1 accounts for the "/" in the following strcat, but you also need to count the \0 as well.

---

what i would like to do in my analyzeFilename function is: remove the \0 to the led_line string (this string come from readFile and it's tmp...)

why i want to do that:

i analyse string, i need to send p to a serial port...

right now if i print p i get somethink like:

......Z00.AB."b.1 1111..&b.12222..

but i want

......Z00.AA."b.11111.&b.12222.

i need to remove \0 from tmp... don't know if it's better to remove it in the analyse function or directly in readFile function