I know that my recover is wrong, but I'm not sure what steps to take-

Question

I 'm getting a segmentation fault for my recover code. I know that it's wrong, and I think that this boils down to me not really understanding how to apply pointers/file functions. Maybe you could give me some pointers on what I don't seem to understand? I haven't put in any code for checking the argc input yet since I can't get this recover part down.

{

    //makes sure that the file is not null

    FILE *f = fopen("argv[1]", "r"); //opens a file


    int *buffer = malloc(512);
    if (buffer==NULL)
    {
        printf ("failed");
        return 1;//failed
    }
    else
        {
        char check;
        char filename[8];
        char prevfile[8];
        int i = 0;
        sprintf(filename, "%03i.jpg", i); //file is named here
        sprintf(prevfile, "%03i.jpg", i-1); //the file that will need to be closed
        while ((check = fgetc(f)) != EOF)//while the EOF has not been reached
            {
            fread (buffer, 512, 512*50, f);//read file f into a buffer in 512 byte chunks
            if (buffer[0] == 0xff && buffer[1] == 0xd8 && buffer[2] == 0xff && (buffer[3] & 0xf0) == 0xe0) //if the header is present
            {
                if (i == 0)//if this is the first jpg, open 000.jpg and start writing into this file
                {
                    FILE *img = fopen(filename, "w"); //the img, heretofore unnamed, is opened for writing
                    fwrite (buffer, 512, 1, img); //fwrite for the 000.jpg
                }
                else //else this is the nth jpg, close 0xx.jpg, open jpg, and start writing into a new file
                {
                    FILE *img = fopen(filename, "w"); //open 0xx.jpg
                    fwrite (buffer, 512, 1, img);
                }
                i++;
            }
            else
            {
                //if file is already open
                {
                    FILE *img = fopen(prevfile, "a");
                    fwrite (buffer, 512, 1, img);
                }

            }
        }
    }
    free(buffer);

}

Answer 1

There are several issues and areas for improvement here.

First, there's this:

FILE *f = fopen("argv[1]", "r"); 

By putting argv[1] in quotes, you're saying to treat whatever is in quotes as a literal string. In other words, open a file named "argv[1]". Remove the quotes to use the string/filename contained inside argv[1]. That should clean up this seg fault (but exposes another one later.)

Next, some programming tips. An if statement does not require an else clause. In fact, an else clause should NOT be used unless absolutely necessary. It just adds to code, decreases runtime efficiency and introduces the possibility of adding code bugs if the code is later modified. Often, this means that the bulk of the program that follows is encapsulated in this else clause - this is a bad programming practice and should be a red flag. It's an invitation for introducing a bug.

In deciding whether to use an else clause, ask this: Does the code need to do EITHER A or B at runtime? If it does, use an else clause. If the situation is just "If TRUE, do something (including terminate), and then continue", then the else is not needed and should not be used.

In this case, look at this code:

if (buffer==NULL)
{
    printf ("failed");
    return 1;//failed
}
else
    {...

Think about what happens if buffer is NULL and if it isn't. If buffer is null, the program will terminate, end of story. If it isn't null, then the code must continue to execute. It's not a "do code block A or do code block B, based on the test". It's a case of "IF true, then termiate the program. Otherwise continue."

Now, let's talk about the file pointers. This code opens the same output files multiple times and yet never closes them. It also starts by opening the output file for write, and then for append. Like your comment said, //if the file is already open, it really is already open. It doesn't need to be reopened.

When a file is opened in a program, it remains open until explicitly closed with an fclose() call. Opening a file multiple times is also dangerous because of the interactions that can occur when two pointers are being used to alter the file content.

Looking at the big picture, the concept is simple. When necessary, open an output file for write. Write as needed to the output file. When done, close the file. That pointer can now be used to open another file with a different name. There's also no need for preserving multiple output file names. You should only be working with one at a time.

About reading from the input file: The code is using both fgetc() and fread() calls to read data. The fgetc call is going to move the pointer one char in the file, corrupting positioning for fread. I suggest you think about how to incorporate the fread into the while statement. Read the man page for fread, particularly the return values and the input parameters.

Speaking of fread, the statement fread (buffer, 512, 512*50, f); will read 512 * 512 * 50 = 13107200 bytes! That's 13 MB, a bit more than 512 bytes.

And of course, never use single letter variable names, except for standard usage as for loop counters. Anything that has any significance, like the pointer for the input file, deserves a descriptive name, like infile. If you've ever tried to debug a program with widespread use of single letter vars, you'd understand. Try finding every variable named "f".

Did I mention that there's not a single call to fclose()?

If this answers your question, please click on the check mark to accept. Let's keep up on forum maintenance. ;-)