pset4 recover code review

Question

Below is my code for pset4 recover.c. I tried my best to write these code out but still having bunch of compiling errors. Could anyone give me some guide or just point out my incorrect points? Thanks in advance.

`

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


int main(void)
{
    // open the memory file
    FILE* raw = fopen("card.raw", "r");
    FILE* buffer = fopen("buffer.txt", "w");
    //FILE* a = fopen("a.txt", "w");

    int count = 0;
    char* jTitle;
    FILE* img[count];


    char* check = malloc(2 * 512);
    while (fread(check, 512, 2, raw) == 2)
    {
        fseek(raw, -1024, SEEK_CUR);


        char* block = malloc(512); 
        fread(block, 512 , 1, raw);


        fwrite(block, 512, 1, buffer);

        // check if the block is the start of jpg
        int*  head;


        fread(head, sizeof(int), 1, buffer);

        if (*head == ffd8ffe#)
        {
            count++;

            // check if already opened a jpg file
            if (jTitle != NULL) 
            {
                fclose(img[count-1]);
                sprintf(jTitle, "%03d.jpg", count);
                FILE* img[count] = fopen(jTitle, "a");

                fwrite(buffer, 512, 1 , img);
            }
            else
            {
                sprintf(jTitle, "%03d.jpg", count);
                FILE* img[count] = fopen(jTitle, "a");

                fwrite(buffer, 512, 1 , img);
            }
        }
        else
        {
            if (jTitle != NULL)
            {
                fwrite(buffer, 512, 1, img);
            }
        }
    }
    free(check);

}

`

Answer 1

When you run

make recover

You get a list of errors. Let us tackle them one by one, starting from the first one, and going all the way to the last one.

The first one is:

recover.c:35:22: error: use of undeclared identifier 'ffd8ffe'
    if (*head == ffd8ffe#)
                 ^

Line 35 on the 22nd character of that line. The compiler thinks you are mentioning a variable named ff28ffe that you have not declared. I know what you meant so I know you did not have any intentions of mentioning a variable.

To write hexadecimals in C code you need to preceed them by "0x" for example FF is written 0xFF. ffd8ffe# looks like pseudocode. I don't think that is enough information for C to understand that you want:

The first byte to be 0xFF

The second byte to be 0xD8

The third byte to be 0xFF

and

The fourth byte to be any number between 0xE0 and 0xEF

Instead of creating an int head I would suggest to create an array of 4 bytes called head. For example:

uint8_t head[4];

uint8_t is one byte. To use type uint8_t you need to include the stdint.h library at the top.

You could use your fread statement on line 33 like this:

fread(head, 4, 1, raw);

Which means:

given fread(A, B, C, D);

"Read B bytes from D into A C times"

in our case:

"Read 4 bytes from raw into head 1 time"

After you read those four bytes, you can check if they are what you want with something like this:

buffer[0] == 0xFF &&            // First byte is FF AND
buffer[1] == 0xd8 &&            // Second byte is D8 AND
buffer[2] == 0xFF &&            // Third byte is FF AND
buffer[3] <= 0xEF &&            // Fourth byte is less than EF AND
buffer[3] >= 0xE0               // Fourth byte is greater than E0

Answer 2

You code is much more complicated than necessary.

    int count = 0;
    FILE* img[count];

You create an array of length 0, so there's no space on the stack associated with that variable.

You don't need to store all the different file references, you just have to keep track of the open ones, and you never write to more than one image at once, so just make that a simple pointer to FILE, like

    FILE* img = NULL;

and later

    img = fopen(jTitle, "w");

There's no need for a "buffer.txt", which I don't see is helping with anything, in most cases you would want to replace buffer by block (like fwrite(block, 512, 1, img);).

Also, that might not work:

    int*  head;
    fread(head, sizeof(int), 1, buffer);
    if (*head == ffd8ffe#)

but that probably does:

    if (*(unsigned int*)block >> 4 == 0xffd8ffe)

which is the same as

    if (block[0] == 0xff && block[1] == 0xd8 && block[2] == 0xff && block[3] >> 4 == 0xe)

or a similar line. Not sure about the signedness of char, I think that one is implementation dependent, might have to cast block[3] to unsigned char first, or declare block as unsigned char* in the beginning. Speaking of which, block should be declared and allocated outside of the loop, otherwise you'd allocate a new block per iteration.

BTW... why do you read to a check first? You could just read to block with

    while (fread(block, 512, 1, raw) == 1)

and forget about check and buffer completely.

And one last segfault source - using char jTitle[8]; instead of char* jTitle; would help a bit, as you pass a random pointer to sprintf.

Hope that helps, feel free to ask if I you don't understand something.