Code Review

This is a discussion on Code Review within the C Programming forums, part of the General Programming Boards category; Anyone see anything really wrong with this? Its purpose is to get the name of the executed file strip off ...

  1. #1
    & the hat of GPL slaying Thantos's Avatar
    Join Date
    Sep 2001
    Posts
    5,681

    Code Review

    Anyone see anything really wrong with this? Its purpose is to get the name of the executed file strip off all the junk before and any extentions after it and then append .cfg to the end.

    IE: The exe name is "bob.exe" and it is in "/home/me/blah/" the argv[0] value might be "/home/me/blah/bob.exe" which the program converts into "bob.cfg".

    I ran it on Linux and Windows using gcc and Dev-C++ respectfully and it works on both.

    Code:
    #include <stdio.h>
    #include <string.h>
    
    void getcfgname (char *, char *, char *);
    
    int main (int argc, char *argv[])
    {
      int count;
      char *p_first, *p_end, filename[30];
    
      if ( (p_first = strrchr (argv[0], '/')) == NULL) /* If Null not unix */
      {
        if ( (p_first = strrchr (argv[0], '\\')) == NULL) /* If Null not windows */
          p_first = argv[0]; /* so set it to the first */
        else
          p_first++;
      }
      else
        p_first++;
    
      if ( (p_end = strchr ( p_first, '.')) == NULL) /* No . after the first */
        p_end = p_first + strlen(p_first);
    
      getcfgname( p_first, p_end, filename);
    
      printf("%s\n", filename);
    
      return 0;
    }
    
    void getcfgname ( char *fi, char *en, char *name)
    {
      int count=0;
      char *p;
    
      for (p=fi; p!=en; p++, count++)
      {
        name[count] = *p;
      }
      name[count]='\0';
      strcat (name, ".cfg");
    }

  2. #2
    End Of Line Hammer's Avatar
    Join Date
    Apr 2002
    Posts
    6,231
    Before anyone complains, this:
    >>if ( (p_first = strrchr (argv[0], '\\')) == NULL)
    isn't a problem with Thantos's code. The single backslash is actually a double backslash, the forum just didn't display it correctly


    >>count
    What's the count var for in main() ?

    >>strcat (name, ".cfg");
    This begs for a buffer overflow. The getcfgname() should know how big the target buffer is and act accordingly. If the buffer isn't big enough, a suitable return code would be useful (to denote failure to build filename)
    When all else fails, read the instructions.
    If you're posting code, use code tags: [code] /* insert code here */ [/code]

  3. #3
    and the hat of wrongness Salem's Avatar
    Join Date
    Aug 2001
    Location
    The edge of the known universe
    Posts
    32,742
    Why not put all the pointer stuff into getcfgname() as well?

    Pass the length of your filename as an additional parameter, and check you don't overflow the name.

    argv[0] might not be available, or may simply be wrong.
    Code:
    char *args[] = {
      "/bin/vi",
      "prog.c",
      NULL
    };
    ...
    execv( "/usr/games/rogue", args );
    To pretty much everyone else, it looks like you're editing a file.
    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.

  4. #4
    Guest Sebastiani's Avatar
    Join Date
    Aug 2001
    Location
    Waterloo, Texas
    Posts
    5,708
    ...just an idea...


    Code:
     char * findlast(const char str[], const char pat[])
    {
     const char * nxt = str, * end = 0;
     
         while(0 != (nxt = strstr(nxt, pat)))
        {
         end = nxt;
         ++nxt;
        } 
        
     return (char*)end;   
    }
    
    
     char * addext(char str[], const char * ext)
    {
     char * p = findlast(str, "\\"), * x = findlast(str, ".");
     
         if(!p)
        {
         p = findlast(str, "/");
        } 
    
     p = (p ? p+1 : str); 
     
         if(x)
        {
         *x = 0;
        }        
       
     return strcat(strcpy(str, p), ext);  
    }
    
    
     char * config(char str[])
    {
     return addext(str, ".cfg");
    } 
    
    
    
    
     int main(int argc, char ** argv)
    {
     char s[MAX_PATH];
     char s1[MAX_PATH] = "c:/etc/dev/a";
     char s2[MAX_PATH] = "c:/etc/dev/b.exe";
     char s3[MAX_PATH] = "c.exe"; 
     char s4[MAX_PATH] = "d";  
    
     strcpy(s, *argv);
    
     printf("%s\n", config(s)); 
     printf("%s\n", config(s1));
     printf("%s\n", config(s2));
     printf("%s\n", config(s3));
     printf("%s\n", config(s4));
    
     return 0; 
    }
    Code:
    #include <cmath>
    #include <complex>
    bool euler_flip(bool value)
    {
        return std::pow
        (
            std::complex<float>(std::exp(1.0)), 
            std::complex<float>(0, 1) 
            * std::complex<float>(std::atan(1.0)
            *(1 << (value + 2)))
        ).real() < 0;
    }

  5. #5
    & the hat of GPL slaying Thantos's Avatar
    Join Date
    Sep 2001
    Posts
    5,681
    Thanks for the ideas. I knew I had to be missing some things since I wrote in about 10 mins and didn't have errors

    argv[0] might not be available, or may simply be wrong.
    I'm not sure what you mean Salem. I thought the first value in argv was the program's name.

    What's the count var for in main() ?
    Nothing. Surprised gcc didn't tell me I wasn't using it
    Last edited by Thantos; 03-06-2004 at 03:13 PM.

  6. #6
    and the hat of wrongness Salem's Avatar
    Join Date
    Aug 2001
    Location
    The edge of the known universe
    Posts
    32,742
    Standard snippet
    -- If the value of argc is greater than zero, the string
    pointed to by argv[0] represents the program name;
    argv[0][0] shall be the null character if the program
    name is not available from the host environment. If
    the value of argc is greater than one, the strings
    pointed to by argv[1] through argv[argc-1] represent
    the program parameters.
    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.

  7. #7
    & the hat of GPL slaying Thantos's Avatar
    Join Date
    Sep 2001
    Posts
    5,681
    Ah so I should check to make sure that argc > 0. Thanks Salem

    Ok version 2. Thanks for the suggestion Sebastiani but I prefer the KISS method.
    Code:
    #include <stdio.h>
    #include <string.h>
    
    void getcfgname (char *, char *, int);
    
    int main (int argc, char *argv[])
    {
      char filename[30];
      int size = sizeof(filename)/sizeof(filename[0]);
    
      if ( argc > 0 )
        getcfgname(argv[0], filename, size);
      else
        strcpy(filename, "default.cfg");
    
      return 0;
    }
    
    void getcfgname ( char *str, char *name, int len)
    {
      int count=0;
      char *p_temp, *p_first, *p_end;
    
      if ( (p_first = strrchr(str, '/')) == NULL)
      {
        if ( (p_first = strrchr (str, '\\')) == NULL)
          p_first=str;
        else
          p_first++;
      }
      else
        p_first++;
    
      if ( (p_end = strchr (p_first, '.')) == NULL)
        p_end = p_first + strlen(p_first);
    
      for (p_temp=p_first; p_temp!=p_end && count < len-5; p_temp++, count++)
      {
        name[count] = *p_temp;
      }
      name[count]='\0';
      strcat (name, ".cfg");
    }
    The reason for the len-5 is because I decided truncation would be the better option for what I want.

    Also I realize there might be a problem when a executable has a space in its name but I don't think it would be that big of a problem.

    Putting it all in one function is much better since I planned on exporting it anyways
    Last edited by Thantos; 03-06-2004 at 03:37 PM.

  8. #8
    End Of Line Hammer's Avatar
    Join Date
    Apr 2002
    Posts
    6,231
    >>Ah so I should check to make sure that argc > 0
    No, no, you miss the point. argv[0] will always be a pointer to a string, but the contents of that string cannot be relied upon to be the program name. EG, the standard says that the string will be be zero bytes long (ie just \0) "if the program name is not available from the host environment. "

    Also, as per Salem's code snippet, argv[0] can be faked to be anything you like (that's what Salem's exec() function is doing, faking it )

    You can loose the strcpy() in main() by declaring the variable as so:
    >>char filename[30] = "default.cfg";
    When all else fails, read the instructions.
    If you're posting code, use code tags: [code] /* insert code here */ [/code]

  9. #9
    & the hat of GPL slaying Thantos's Avatar
    Join Date
    Sep 2001
    Posts
    5,681
    Ah and the light goes on

    As for the faking of it, thats ok for my purposes

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Code Review for upcoming contest
    By Darryl in forum Contests Board
    Replies: 2
    Last Post: 02-28-2006, 02:39 PM
  2. Problem : Threads WILL NOT DIE!!
    By hanhao in forum C++ Programming
    Replies: 2
    Last Post: 04-16-2004, 02:37 PM
  3. True ASM vs. Fake ASM ????
    By DavidP in forum A Brief History of Cprogramming.com
    Replies: 7
    Last Post: 04-02-2003, 04:28 AM
  4. Interface Question
    By smog890 in forum C Programming
    Replies: 11
    Last Post: 06-03-2002, 06:06 PM
  5. Replies: 0
    Last Post: 02-21-2002, 06:05 PM

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