0

First, I have read the forum's discussion rules, and I have reviewed every post relating to recover.c I can find on reddit, StackExchange, and the other discussion boards. Since none seems to address whatever problem affects my code when run on check50, I'm posting now for help.

This is my code:

https://gist.github.com/bruce-mcgoveran/d487f57a2f6033e735a5f9fef57669bc

This is the data generated by CS50's sandbox:

https://sandbox.cs50.net/checks/1a2ca6ca27404d979d12bd9d3a468fa8

As you will see, my code does not recover the middle and last files correctly. At this point, I have no really good explanations for why not. Perhaps I need to refine how I handle extracting data after the last call to fread(), which I realize may well not return a full buffer's (512 bytes) worth of data. However, I can't see how that relates to my code's failure to identify the middle images in the check50 tests. Lastly, to confirm, my code finds and appears to display correctly the 50 images contained within card.raw.

Thank you in advance for any help you can offer to point me in the right direction. Also, if I need to improve how I present my code for help, please let me know - I'm new to posting.

2

Wow, so many people missing the problem that is hiding in plain sight.

As some people know, check50 uses a different data set than the one provided. (sneaky way to provide a challenge! ;-) ) When check50 runs that data set, it produces 15 files normally, but this code is producing 16 files. So where is the extra file coming from?

The code is detecting a false signature that is buried inside the middle of a block in an image. Why is it doing this? Look at the following line of code:

while (fread(&buffer, sizeof(BLOCK), 1, in))  

BLOCK is a constant for the number 512, the number of bytes that should be read. The problem is that it is reading sizeof(BLOCK). The size of BLOCK, or any constant is 8 bytes. sizeof(BLOCK) is not the value of block. By reading 8 bytes at a time, it's getting snagged on that hidden false signature.

As for the other observations:

filename[8] = '\0'; This has no effect, and in fact, is entirely unnecessary. The sprintf() call will write the '\0' in the appropriate place as needed.

MARS is almost right about the "w" vs. "a" modes, except if I read it right, he has it backwards. If the output file is opened in append mode, it will simply append to an existing file. If the file doesn't exist, it'll work correctly, by accident. The code will only work if the output file doesn't exist. The output file needs to be opened in write mode.

To be honest, I looked at this for an hour and had to enlist a little help. Thanks, @curiouskiwi. ;-) While I picked up on the extra file, she caught the sizeof(BLOCK) issue.

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

3
  • Absolutely brilliant. Thank you so much for taking time to help me. I could have spent the rest of the year staring at this code, and I'm not sure it would have occurred to me look at sizeof() more closely. Thank you also to @curiouskiwi. My code now passes all check50's tests.
    – BruceM
    Jul 2 '17 at 2:51
  • Nice catch. But how do you know about 16th file?
    – obesman
    Jul 2 '17 at 5:09
  • Its easy when you have all the input data files. ;-)
    – Cliff B
    Jul 2 '17 at 9:44
0

Haven't look deep into your code, but one thing looks quite suspicious. You declare char filename[9]. Let's count: 001.jpg looks like 7 symbols + \0 = 8. Why 9? Then, let's look at your definition filename[8] = '\0'. 001.jpg is *char 001.jpg\0 so filename[7] = \0, not filename[8].

I admit I might be wrong, as there could be some logic behind in your code, which I haven't checked in details. However....

1
  • It's not so much a problem as it is unnecessary code.
    – Cliff B
    Jul 1 '17 at 22:32

You must log in to answer this question.

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