1

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);

}

`

0

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
2
  • Thanks, Ricardo! Very helpful! I should check the head one byte by one byte. In fact, I don't know how to check 4 bytes at one time. Blauelf provides something above and I'm not quite there yet. And, instead of pulling the 4 bytes out of raw, I just check the first 4 bytes of the block. so I declared uint8_t buffer[512]. And your idea of compare the last byte also helps. I was confusing about how to check the value of the last byte. With the help of you and Blauelf. My code works now!! – michael zhang Oct 21 '16 at 7:19
  • Awesome! I see you are doing hard and good work. With CS50 there's not many if any way around it. But it will sure be rewarding all the knowledge you get. – Ricardo David Antonetti Oct 21 '16 at 11:23
0

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.

3
  • Thanks, Blauelf! It helped a lot. I was confusing about the open and close image files. and not very clear how to check the end of the file. You comment helps on those. and as for (unsigned int)block >> 4 == 0xffd8ffe, it seems something new for me...could you help to explain a little bit more? BTW, to have jTitle as 8 because we have 7 chars and 1 more for \0, right? Now my code is working. WOW!! – michael zhang Oct 21 '16 at 7:26
  • *(unsigned int*)block >> 4 == 0xffd8ffe casts the pointer block to a pointer to unsigned int (on most systems that's a 32 bit integer, but in theory only required to be at least 16 bit, not shorter than a short int, and not longer than a long int). This code breaks if your int is not 32 bit, should use uint32_t instead of unsigned int. Now I dereference it (interpreting the first 32 bit of the block array as unsigned int), and do a bitshift by 4 bits right (like a division by 16). Then I compare the remaining 28 bit. – Blauelf Oct 21 '16 at 15:53
  • That's a little bit more for me but make sense. Thanks @Blauelf. – michael zhang Oct 24 '16 at 3:13

You must log in to answer this question.

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