Thread: Code Review

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)

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.

...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; 
}

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

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.

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

>>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";