Opening a file with path in malloc

Question

I'm trying to open a file with fopen, but I don't want a static location so I am getting the string in from the user when he/she runs the program. However if a user does not enter one a default file is specified.

Can I just put the malloc var in to the fopen path parameter?

char *file_path_mem = malloc(sizeof(char));
if (file_path_mem != NULL) //Null if out of memory
{
  printf("Enter path to file, if in current directory then specify name\n");
  printf("File(default: marks.txt): ");

  while ((c = (char)getchar()) != '\n')
  {
    file_path_mem[i++] = c;
    file_path_mem = realloc(file_path_mem, i+1 * sizeof(char));
  }
    file_path_mem[i] = '\0';
    if (i == 0 && c == '\n')
    {
      file_path_mem = realloc(file_path_mem, 10 * sizeof(char);
      file_path_mem = "marks.txt";
    }
  }
  else
  {
    printf("Error: Your system is out of memory, please correct this");
    return 0;
  }
  if (i==0)
  {
    FILE *marks_file = fopen("marks.txt", "r");
  }
  else
  {
    FILE *marks_file = fopen(file_path_mem, "r");
  }
  free(file_path_mem);

As you might have guess I am a c novice so if I have done something horrible wrong, then sorry.

Answer 1

This is not doing what you think it is:

file_path_mem = realloc(file_path_mem, 10 * sizeof(char);
file_path_mem = "marks.txt";

What you want to do is change that second line to copy the default name into the allocated buffer:

strcpy(file_path_mem, "marks.txt");

Answer 2

You CAN pas the char* returned from malloc straight into fopen. Just make sure it contains valid data. Though make sure you strcpy the new string in as R Samuel points out.

Btw, your use of realloc will give pretty awful performance. Realloc works by seeing if there is enogh space to grow the memory (This is not guranteed, realloc must just return a block that is the newSize containing the old data). If there isn't enough space, it allocs a new block of the new size and copies the old data ot the new block. Obviously this is suboptimal.

You are better off allocating a block of, say, 16 chars and then , if you need more than 16, realloc'ing a further 16 chars. There will be a bit of memory wastage but you are not, potentially, copying anything like as much memory around.

Edit: Further to that. For a string that contains 7 character. Using your realloc every byte scheme you will generate the following processes.

alloc 1 byte
alloc 2 bytes
copy 1 byte
free 1 byte
alloc 3 bytes
copy 2 bytes
free 2 bytes
alloc 4 bytes
copy 3 bytes
free 3 bytes
alloc 5 bytes
copy 4 bytes
free 4 bytes
alloc 6 bytes
copy 5 bytes
free 5 bytes
alloc 7 bytes
copy 6 bytes
free 6 bytes
alloc 8 bytes
copy 7 bytes

This is 8 allocations, 7 frees and a copy of 28 bytes. Not to mention the 8 bytes you assign to the array (7 characters + 1 null terminator).

Allocations are SLOW. Frees are SLOW. Copying is much slower than not copying.

For this example using my allocate 16 bytes at a time system you alloc once, assign 8 bytes. No copies and the only free is when you've finished with it. You DO waste 8 bytes though. But ... well ... 8 bytes is nothing in the grand scheme of things...

Answer 3

realloc is relatively more expensive than the rest of your loop, so you might as well just start with a 128 byte buffer, and realloc another 128 bytes if you fill that up.

I suggest some of the following changes:

define your default location at the top of the file

char* defaultLocation = 'myfile.txt';
char* locationToUse;

use constants instead of hard coding numbers in your code (like the 10 you have in there)

int DEFAULT_INPUT_BUFFER_SIZE = 128;
char* userInputBuffer = malloc(sizeof(char) * DEFAULT_INPUT_BUFFER_SIZE) );
int bufferFillIndex = 0;

only reallocate infrequently, and not at all if possible (size 128 buffer)

while ((c = (char)getchar()) != '\n')
  {
    file_path_mem[bufferFillIndex++] = c;
    if (bufferFillIndex % DEFAULT_INPUT_BUFFER_SIZE == 0)
    {
        realloc(file_path_mem, (bufferFillIndex + DEFAULT_INPUT_BUFFER_SIZE) * sizeof(char);
    }
  }

Answer 4

Should not be an issue here but for realocation you should state the new size like that

file_path_mem = realloc(file_path_mem, (i+1) * sizeof(char));

Answer 5

You could simplify this greatly with getline(), assuming it is available on your system:

printf("Enter path to file, if in current directory then specify name\n");
printf("File(default: marks.txt): ");

int bufSize = 100;
char* fileNameBuf = (char*) malloc(bufSize + 1);
int fileNameLength = getline(&fileNameBuf, &bufSize, stdin);

if(fileNameLength < 0)
{
    fprintf(stderr, "Error: Not enough memory.\n")
    exit(1);
}

/* strip line end and trailing whitespace. */
while(fileNameLength && fileNameBuf[fileNameLength - 1] <= ' ')
    --fileNameLength;
fileNameBuf[fileNameLength] = 0;

if(!fileNameLength)
{
    free(fileNameBuf); /* Nothing entered; use default. */
    fileNameBuf = "marks.txt";
}

FILE *marks_file = fopen(fileNameBuf, "r");

if(fileNameLength)
    free(fileNameBuf);

I don't know if your C supports in-block declarations, so I'll let you fix that if needed.

EDIT: Here's a getline tutorial.

Answer 6

As a learning experience I would second the suggestions of others to expand your buffer by more than one byte. It won't make any difference if it's not in a loop but calling realloc() on each byte would be unacceptable in other contexts.

One trick that works fairly well is to double the size of the buffer every time it fills up.

On the other hand, if you really just want to open a file and don't care about writing the world's greatest name loop, then standard procedure is to simplify the code even at the price of a limit on file name length. There is no rule that says you have to accept silly pathnames.

FILE *read_and_open(void)
{
  char *s, space[1000];

  printf("Enter path to file, if in current directory then specify name\n");
  printf("File(default: marks.txt): ");

  fgets(space, sizeof space, stdin);
  if((s = strchr(space, '\n')) != NULL)
    *s = 0;
  if (strlen(space) == 0)
    return fopen("marks.txt", "r");
  return fopen(space, "r");
}