Pset 4- Recover. Issues with properly formatting code

Question

I have a problem where I need to check if I have an open file on my loop, so I properly close the previous file before writing a new one. But my check only works if I fclose(img) if my counter > 0. The problem here is that (img) is not actually declared until after the check, but if I put it above the check it renders the check pointless. How do I reformat this code so that I can fclose(img) if my counter > 0, while also declaring img before the counter somehow?

    do
{
        unsigned char read[512];
        fread(&read, 512, 1, memcard);
        if (read[0] == 0xff && read[1] == 0xd8 && read[2] == 0xff && (read[3] & 0xf0) == 0xe0)
        {
            if(counter > 0)
            {
                fclose(img);
            }

            counter++;
            sprintf(filename, "%03i.jpg", counter);
            FILE *img = fopen(filename, "w");

        }
        if (counter > 0)
        {
            fwrite(&read, 512, 1, img);
        }
        readNum = fread(&read, 512, 1, memcard);
}
while (readNum == 512);

Edited Code

int counter = 0; // Initialize counter for .jpgs found
char *filename = NULL;
FILE *img = NULL;
unsigned char read[512];
while (fread(&read, 1, 512, memcard) == 512)
{
        //unsigned char read[512];
        //fread(&read, 1, 512, memcard);
        if (read[0] == 0xff && read[1] == 0xd8 && read[2] == 0xff && (read[3] & 0xf0) == 0xe0)
        {
            if(counter > 0)
            {
                fclose(img);
            }

            counter++;
            sprintf(filename, "%i.jpg", counter);
            img = fopen(filename, "w");

        }
        if (counter > 0)
        {
            fwrite(&read, 1, 512, img);
        }

}

Answer 1

That's a scope issue.

If you declare a variable, it exists on stack until you leave this block. You declare img on the last line of the block, hit the }, and - poof - the stack pointer gets reset to the state it had before entering the block, and now img no longer exists (its content still exists, and the next pointer you declare will likely be at the same position on stack and contain that value, but the variable has disappeared). Similar problem with read, but that has more problems, like that you read two sectors per iteration, but write only one.

I'd suggest that you declare both img and your buffer outside of the loop.

And the first number in fread is the block size, the second is the number of blocks to read, the function returns the number of blocks it successfully read. fread(read, 512, 1, memcard) would return 0 or 1, fread(read, 1, 512, memcard) would return something from 0 to 512.

Also, what if your input file is empty? You would enter the loop (as it's a do..while loop), fread would return 0 for no blocks read (but you ignore the value), and you'd test arbitrary data for representing a JPEG header. Behaviour is undefined.

One solution to processing existing sectors only is to fread, then check feof and leave the loop if it returns truthy (feof checks a bit set on an unsuccessful read). Or use fread in the condition of a while loop (not do..while, for reasons above), and immediately check its return value. But whatever you do, try to avoid multiple fread, as you already did for fwrite.