Having trouble getting recover off the ground

Question

I'm sorry to do this but I'm fairly lost with this pset3 recover for cs50. I've been researching for hours and am no closer to figuring out how to utilize FILE *outptr = fopen and sprintf(filename, "%03i.jpg in order to make new files and name them. Also, my logic for making this work is totally not figured out yet as well. I'm feeling pretty defeated. If any kind soul could take a quick gander and maybe push me in a direction of some kind, I'd really appreciate it.

My code compiles but basically doesn't do anything. Thanks guys. Appreciate it.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <cs50.h>


int main(int argc, char *argv[])
{
    //check for proper input
    if (argc != 2 || strcmp(argv[1], "card.raw") != 0)
        {
            fprintf(stderr, "Usage: ./recover image\n");
            return 1;
        }

     // open input file
    FILE *inptr = fopen("card.raw", "r");
    if (inptr == NULL)
        {
            fprintf(stderr, "Could not open %s.\n", "card.raw");
            return 2;
        }


    // keeps track of jpegs found
    int jpegCount = 0;

    //temp storage
    int buffer[512];

    //read 512 chunks
    fread(&buffer, 512, 1, inptr);

    //repeat until EOF.
    while (fread(buffer, 512, 1, inptr) == 1)
        {
        //read 512 chunks
        fread(&buffer, 512, 1, inptr);
        //use this to recognize the start of jpeg
        if (buffer[0] == 0xff && buffer[1] == 0xd8 && buffer[2] == 0xff && (buffer[3] & 0xf0) == 0xe0)
            {

                char filename[8];

                //open new jpeg and write - x means return null if it already exists
                FILE *outptr = fopen(filename, "wx");

                //make new filename for each jpeg
                sprintf(filename, "%03i.jpg", jpegCount);

                //write a 512 block to new file
                fwrite(inptr, 512, 1, outptr);

                //read next chunk
                fread(buffer, 512, 1, inptr);

                //is the next part a beginning of a jpeg?
                if (!(buffer[0] == 0xff && buffer[1] == 0xd8 && buffer[2] == 0xff && (buffer[3] & 0xf0) == 0xe0))
                {
                    //write another 512 block to  file
                    fwrite(inptr, 512, 1, outptr);
                }
                //close the file
                fclose(inptr);

            }

        }

    fclose(inptr);
}

Answer 1

|| strcmp(argv[1], "card.raw") != 0 means you expect a command line argument, but complain if it's not the hard-coded value? You should accept any string, and if it's not a file name, complain after failing to fopen(argv[1], "r") it.

Each time you fread(&buffer, 512, 1, inptr), you read 512 bytes. Yes, even if it appears in the condition, it will read 512 bytes each time the condition is evaluated. Make sure to not read a new 512 block before handling the last one in some way. In my solution, I have only one fread, the one in the condition, and handle one block per iteration (declaring the output pointer outside the loop, so I avoid duplicating header/EOF checks which I would have when using an inner loop).

You fopen using filename, but at that point you haven't specified the content of filename. sprintf must come before fopen. It's good practice to check the file pointer in case something went wrong (which would mean fopen returns NULL, and passing NULL to other file functions often causes segmentation faults).

Not sure what mode "wx" is meant to do (there's permissions "wx" for files, "write" and "execute", maybe you confused with those). "w" for "write" should be enough. Edit: Just learnt that "x" got introduced in C11. Still, no NULL test.

Inside the loop, you close input, not output.