Tips, comments to improve my code

**collinm** · 04-08-2005

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

**Salem** · 04-08-2005

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.

**collinm** · 04-08-2005

Originally Posted by Salem

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

ok done

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

Originally Posted by Salem

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

ok done

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

Thread: Tips, comments to improve my code

Thread Tools

Search Thread

Display

Tips, comments to improve my code

Similar Threads

Obfuscated Code Contest

Problem : Threads WILL NOT DIE!!

Request for comments

Interface Question

Can you have nested code block? What does the compiler do? For example ...