Tips, comments to improve my code

This is a discussion on Tips, comments to improve my code within the C Programming forums, part of the General Programming Boards category; hi in my main function i do: searchFile(fd, mnt_dir_led, "txt"); my program call are: searchFile |-----------------readFile |---------------------- analyzeFilename(int i don't ...

  1. #1
    Registered User
    Join Date
    Mar 2005
    Posts
    5

    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

  2. #2
    and the hat of wrongness Salem's Avatar
    Join Date
    Aug 2001
    Location
    The edge of the known universe
    Posts
    32,434
    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.
    If you dance barefoot on the broken glass of undefined behaviour, you've got to expect the occasional cut.
    If at first you don't succeed, try writing your phone number on the exam paper.
    I support http://www.ukip.org/ as the first necessary step to a free Europe.

  3. #3
    Registered User
    Join Date
    Mar 2005
    Posts
    5
    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

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Obfuscated Code Contest
    By Stack Overflow in forum Contests Board
    Replies: 51
    Last Post: 01-21-2005, 03:17 PM
  2. Problem : Threads WILL NOT DIE!!
    By hanhao in forum C++ Programming
    Replies: 2
    Last Post: 04-16-2004, 01:37 PM
  3. Request for comments
    By Prelude in forum A Brief History of Cprogramming.com
    Replies: 15
    Last Post: 01-02-2004, 09:33 AM
  4. Interface Question
    By smog890 in forum C Programming
    Replies: 11
    Last Post: 06-03-2002, 05:06 PM
  5. Replies: 4
    Last Post: 01-15-2002, 11:04 PM

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21