0

Please could I get some help with my code for Pset4's recover? I have been stuck on it for about 8 hours today and cannot work out why it isn't working. I think the current issue is that my current[] array isn't populating using the fread() function (I have tried to highlight where this is in the code with //error here either side of the line). Is there a fundamental issue with my logic in this task or is there something small that I've missed? Thanks in advance

#include <stdio.h>
#include <stdlib.h>
#include <stdint.h>

typedef uint8_t BYTE;

int jpg_start(BYTE buffer[]);

int main(int argc, char *argv[])
{
    //check if arguments corrently inputted by user
    if (argc != 2)
    {
        printf("Usage: ./recover image\n");
        return 1;
    }

    //open memory card and check return value
    FILE *card = fopen(argv[1], "r");
    if (card == NULL)
    {
        printf("File not found\n");
        return 2;
    }

    //repeat until end of card
    int length_of_card_bytes = 0;
    while (fgetc(card) != EOF)
    {
        length_of_card_bytes++;
    }
    int length_of_card_blocks = length_of_card_bytes / 512;
    FILE *new_file = NULL;
    int file_number = 0;
    for (int i = 0; i < length_of_card_blocks; i++)
    {
        //read 512 bytes into a buffer called current
        BYTE current[512];
        if (i > 0)
        {
            fseek(card, 512, SEEK_CUR);
        }
//error here        
fread(current, 1, 512, card);
//error here
        if (jpg_start(current) == 1)
        {
            //if not first jpg
            if (file_number != 0)
            {
                //create jpg file
                char *filename = NULL;
                sprintf(filename, "%03i.jpg", file_number);
                new_file = fopen(filename, "w");

                //write buffer to jpg file
                fwrite(current, 1, 512, new_file);
                file_number++;
            }
            //if first jpg
            if (file_number == 0)
            {
                //create jpg file
                char *filename[7] = {"0", "0", "0", ".", "j", "p", "g"};
                new_file = fopen("000.jpg", "w");

                //write buffer to jpg file
                fwrite(current, 1, 512, new_file);
                file_number++;
            }
        }
        if (jpg_start(current) == 0)
        {
            if (new_file != NULL)
            {
                fwrite(current, 1, 512, new_file);
            }
        }
    }
    //close the file
    fclose(card);
}

int jpg_start(BYTE buffer[])
{
    if  (buffer[0] == 0xff && buffer[1] == 0xd8 && buffer[2]== 0xff && (buffer[3] & 0xf0) == 0xe0)
    {
        return 1;
    }
    return 0;
}
0

This is not going to be the answer you want to hear. This code has a significant number of issues, related more to how it is trying to do things than anything else. To put it bluntly, it's far too complicated for the task, and it does many things either poorly or incorrectly. The code is just too complex for what it needs to do.

I spent a considerable amount of time trying to figure out exactly how to make it work, but in the end, couldn't. (Maybe I'm just too tired? I usually don't give up.)

Think about what the code needs to do. It needs to make a single pass through the code, doing the following:

  1. Read a 512 byte block of data.
  2. check for a signature. If it doesn't have one, either throw it out before the first file is opened, or write it out to the already opened file. If it does have a signature, close the current output file (if open), and open a new output file.
  3. on EOF of the input file, close any open files and terminate the program.

This code makes two passes through the input file - first to see how big it is (totally not needed) and then to process the data. It uses two different char array vars for output file names, one for the first file (which it doesn't even use, but initializes char by char, why???) and a second for all other output files. Only the latter is needed for all files.

There's a fseek that moves forward 512 bytes just before the read. Why??? Any time you call fread or fwrite, it will automatically reposition the pointer to the byte position following the read or write. The fseek is effectively skipping over 512 bytes with each call. There may still be other issues, but it's just too complex for what it needs to do.

My sincere advice is to throw it all out and start over. Think carefully about what it needs to do and how to keep it simple.

Hint: the main part of this program can be done with just one of each of these types of statements: a while loop, an fread and an fwrite. It does need two fopen statements, one for input and one for output. If you can do this, you've probably got an efficient program!

I know it isn't what you wanted to hear, but it's an important lesson. Sometimes, it's better to scrap the work and try again fresh. Keep in mind that you've probably been flogging at this for so long that you may have more skill at how things work than you even know. You might also find it helpful to set it aside for a couple days and don't even think about it. Come back fresh after that. You may find that your subconscious has had enough time to digest it all and you'll have some fresh ideas!

If you're still not getting anywhere, I suggest that you try and contact myself or @curiouskiwi over on Slack. (it's one of the other forums) for some one-on-one help.

Good luck!

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .