Passing filename - as inputted string - to fopen() function

I figured this might as well be in its own thread.

I'm surprised how I can't find a solution to this problem on the internet. It must be a very common error:

Here is the code in question:

Code:

    
   #define IBS 150
 
   ...

   int main() {

   /* Get Filename from user */
   char szbuf[IBS];
   puts ("Enter file to be opened: ");
   
   fgets(szbuf, IBS, stdin );
   char filename[ strlen(szbuf)];
   strcpy(filename, szbuf);
    
   /* Open File */  
   FILE * fp = fopen(filename, "r");
   
   ...

I always get an error involving "constant char".

I'm doing something wrong in what I am passing to the "fopen()" function. What does it want? I thought it was a string?

Thanks in advance,

Clint

What is the error message?

By the way, filename is a variable length array. Is that intended?

One problem I see is in your input; fgets takes in the newline character you enter at the end. I deal with this by doing :

Code:

szbuf[strlen(szbuf) - 1] = 0; //Nullfies newline

Originally Posted by epboyd

One problem I see is in your input; fgets takes in the newline character you enter at the end. I deal with this by doing :

Code:

szbuf[strlen(szbuf) - 1] = 0; //Nullfies newline

Except that if you do that, and the line was longer than the capacity given, you will overwrite the last character. If instead you do

Code:

char *newlineptr;
...
newlineptr = strchr(szbuf, '\n');
if (newlineptr != NULL) // We found a newline
   *newlineptr = 0;
// Potentially have an "else" to deal with lines that are too long to fit in the buffer.

then we do not overwrite something that isn't a newline (and the difference in performance between strchr() and strlen() is marginal, in case anyone wonders).

--
Mats

> I always get an error involving "constant char".
As laserlight hinted, C doesn't have variable length arrays.
Nor does it have mixed declarations and statements.

I'd replace

Code:

char filename[ strlen(szbuf)];

with something like

Code:

char *filename;
size_t len=strlen(szbuff);
filename=(char *)malloc(sizeof(char)*(len+1)); //sizeof(char)=1, i put just to warn if you have to deal with other types
if (filename==NULL)
{
printf("Out of memory\n");
return 1;
}

Does the rest of the community agree?

Originally Posted by smoking81

I'd replace

Code:

char filename[ strlen(szbuf)];

with something like

Code:

char *filename;
size_t len=strlen(szbuff);
filename=(char *)malloc(sizeof(char)*(len+1)); //sizeof(char)=1, i put just to warn if you have to deal with other types
if (filename==NULL)
{
printf("Out of memory\n");
return 1;
}

Does the rest of the community agree?

Two aspects I'd like to point out:
First: Why have two variables?
There is szbuf already, what's the point of a second one called filename. Ok, it may be better to change szbuf to filename for naming practices, but I don't see any reason to copy it around.

Second, if you must dynamically allocate the space, don't cast malloc() - this is the C section, not C++. Casting malloc is bad, and it's explained in the FAQ why it is so.

--
Mats

Personally, I think this would suffice:

Code:

/* Get Filename from user */
char filename[IBS];
FILE *fp = NULL;

puts("Enter file to be opened: ");
fgets(filename, IBS, stdin);

/* Open File */
fp = fopen(filename, "r");

IBS is not a very descriptive name for the size of a filename though. Perhaps it stands for "input buffer size", but it leaves one to guess.

Originally Posted by laserlight

Personally, I think this would suffice:

Code:

/* Get Filename from user */
char filename[IBS];
FILE *fp = NULL;

puts("Enter file to be opened: ");
fgets(filename, IBS, stdin);

/* Open File */
fp = fopen(filename, "r");

IBS is not a very descriptive name for the size of a filename though. Perhaps it stands for "input buffer size", but it leaves one to guess.

Yes, that's what I was trying to say. The string may need cleaning of the newline at the end, but other than tat, it's fine.

And IBS to me stands for Irritable Bowel Syndrom, but I can accept that is not what the original poster meant. Longer names are better [and in the case of a filename, perhaps MAX_PATH or PATH_MAX is a better choice - at least that stands a chance of holding a full path to some file deep down in the system. [Although for non-Windows, that solution CAN go wrong - but it's a better choice than some arbitrarily selected manual size].

--
Mats

I had this same problem a few weeks ago trying to open a text file.

Just thought I'd add a reply to this aswell. My code assumes the person doesn't know the file extension and adds on ".txt" but you can change that to anything you like.

Code:

#define LNAME  	"C:\\Documents and Settings\\"              /* loadfile location */

int load()
{	
     int i;
     int len;	

     char file_name[50];
     char load_location[50] = LNAME;

     printf(" Enter file name: ");
     fflush(stdin);
     
     fgets(file_name, sizeof(file_name), stdin);

          /* remove newline left by fgets */

     len = strlen(file_name);
     if (file_name[len-1] == '\n') 
     file_name[len-1] = 0;
     
			
     strcat(load_location, file_name);
     strcat(load_location, ".txt");
	
     FILE *file = fopen(load_location, "r");
}

You do not need a loop to remove the newline; strlen() already performs the loop, so you can use its return value to check the last character in the string.

Code:

     for(i = 0; i < strlen(file_name); i++)
          if(file_name[i] == '\n')
               file_name[i] = '\0';

This is VERY bad coding style, calling strlen() over and over in the loop. The length only changes when you get to the very end of the string, when you reach the '\n'. You can simplify it by just doing :

Code:

len = strlen(file_name);
if (file_name[len-1] == '\n') 
   file_name[len-1] = 0;

I make a separate variable for storing the length, so that we don't have to call strlen twice.

--
Mats

Woops...

Yeah I usually do something like:

Code:

len = strlen(file_name);
if (file_name[len-1] == '\n') 
   file_name[len-1] = 0;

When I'm writing code I have a habit of just doing "what works" for the first time. I usually go over my code when I'm ready to optimise it. Bad habit I can't get out of.

Originally Posted by george_1988

Woops...

Yeah I usually do something like:

Code:

len = strlen(file_name);
if (file_name[len-1] == '\n') 
   file_name[len-1] = 0;

When I'm writing code I have a habit of just doing "what works" for the first time. I usually go over my code when I'm ready to optimise it. Bad habit I can't get out of.

That's not a bad habit, however, "avoiding pessimisation" is a good thing.... A big warning light goes up when I see strlen in a loop-control, and in this case, it's definitely not meaningfull.

--
Mats

Code:

fflush(stdin);

<Darth Vader voice>NOOOOOOOOOOOOOOOOOOO!!!!!!!!!!!!!</Darth Vader voice>