PSET4 recover.c worked well but didn't pass check50 test

Question

So my recover.c worked and recovered all 50 photos, all of them are viewable. The program also passed Valgrind test with no leak in memory. However, it didn't pass check50 test. The results are:

:) recover.c exists

:) recover.c compiles

:) recovers 000.jpg correctly

:( recovers middle files correctly

:( recovers last file correctly

\ expected 015.jpg to exist

Actually, 015.jpg is right there and viewable. I was wondering if you could help. I am posting the code here (includes: stdio.h, stdlib.h). Thank you very much!

typedef uint8_t  BYTE;
typedef int32_t  LONG;

int main(void)
{
char* infile = "card.raw";
int count = 0; // record the number of photos recovered

// open input file for JPEG checking
FILE* inptr = fopen(infile, "r");
if (inptr == NULL)
{
    printf("Could not open %s.\n", infile);
    return 2;
}

char outfile[10]; // name of recovered JPEG photo
LONG read_tag; 
FILE* outptr = NULL;

// read through input file    
while(!feof(inptr))
{
    fread(&read_tag, sizeof(LONG), 1, inptr);         
    if(read_tag == 0xe0ffd8ff) // check whether the first 4 bytes correspond to the signature of a JPEG file
    {
        fseek(inptr, -sizeof(read_tag), SEEK_CUR); //go back to the beginning of this 512B block for copying

        if(outptr != NULL) // if the file of previously found JPEG file is still open, close it
            fclose(outptr);

        sprintf(outfile, "%03d.jpg", count); // create new JPEG file
        count++; //found one JPEG
        outptr = fopen(outfile, "a");
        if (outptr == NULL)
        {
            fclose(inptr);
            fprintf(stderr, "Could not create %s.\n", outfile);
            return 3;
        }

        BYTE* buf; // copy the content to this JPEG file
        for(int j = 0; j < 512; j++)
        {
            fread(&buf, sizeof(BYTE), 1, inptr);
            fwrite(&buf, sizeof(BYTE), 1, outptr);
        }
    }
    else if(count > 0) // if it is a 512B block after JPEG signature, copy the content to newly created file
    {
        fseek(inptr, -sizeof(read_tag), SEEK_CUR); 
        BYTE* buf;
        for(int j = 0; j < 512; j++)
        {
            fread(&buf, sizeof(BYTE), 1, inptr);
            fwrite(&buf, sizeof(BYTE), 1, outptr);
        }
    }
    else // if this 512B block is before the first JPEG file, skip it and go to the next 512B block
    {
        fseek(inptr, sizeof(BYTE) * 508, SEEK_CUR); 
    }
}
fclose(outptr);
fclose(inptr);

}

Answer 1

You should know that the check50 test uses a different set of test data than those provided with the downloaded code, so while it appears to work, the check50 code is likely to find other problems.

I see a few issues. First, it is only checking for one possible signature. There are 16. This is probably causing the middle file failures. Second, because the while loop setup is only looking for the end of file and not actually doing a read, the code is essentially reading in a 512 block, processing, writing 512 bytes out, and then checking for EOF. Since the last valid read goes right up to the EOF marker without tripping on it, this has the effect of writing 512 extra bytes of garbage data to the last file. This is almost certainly why the last file fails.

As a side note, while the code may be technically correct, it's not terribly efficient. It reads in one byte at a time x 512 times on each pass. Remember that each read has disk and system overhead. By reading one byte at a time, the total effect is 512 x overhead + 512 bytes read. Instead, if 512 bytes were read at once, it would be 1 x overhead + 512 bytes. The difference is particularly important in reading/writing many megabytes of data.

There's also the issue of moving back and forth in the file. It is far more efficient to read in an entire 512 byte block and examine the first 4 bytes, rather than reading in 4 bytes, moving the file pointer back, and then reading 512 bytes.

If this isn't enough to make you scratch your head, it is possible to write this program using exactly one fread and one fwrite call, instead of several of each throughout your program. That would be far more efficient! My experience has been that the more of each, the higher the probability of errors in the code.

If this answers your question, please click on the check mark to accept. Let's keep up on forum maintenance. ;-)