0

I am currently stuck on this problem set, as it seems like I am able to recover the 50 jpgs but I am failing the check50. I am not sure what I am doing wrong and would appreciate some help. :( recovers 000.jpg correctly 000.jpg not found :( recovers middle images correctly 001.jpg not found :( recovers 049.jpg correctly 049.jpg not found

It also seems like the jpgs i have recovered are corrupted. This is my code for the problem:

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

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

FILE *f= fopen(argv[1], "r");

if (!f)
{
    printf("File cannot be opened\n");
    return 1;
}

int count=0;
typedef uint8_t BYTE;

//make a buffer of size 512b
BYTE buff[512]={0};
//allocate memory for filenamed pic
char *pic=malloc(sizeof(char)*2048);
//intialise file pointer img 
FILE *img= NULL;

while(fread(buff, 512, 1, f))
//condition fulfilled to be a jpeg
{
    if(buff[0]==0xff && buff[1]==0xd8 && buff[2]==0xff && ((buff[3]&0xf0)==0xe0)  )
    {
            if((count=0))
            {
                sprintf(pic, "%03i.jpg", count);
                img=fopen(pic, "w"); 
                fwrite (buff, 512, 1, img);
                fclose(img);
                count++;
            }
            else if (count>0)
            {
                count++;
                sprintf(pic, "%03i.jpg", count);
                img=fopen(pic, "w"); 
                fwrite (buff, 512, 1, img);
                fclose(img);
            }       
    }


     //if not start of jpeg
    else
    {
        if(count>0)
        {

            fwrite(buff,sizeof(buff),1,img);
            fclose(img);

        }


    }



}

 free(pic);

}

  • can you post the whole recover.c? – stensal Jun 12 at 17:55
  • sorry but the code is the whole recover.c. am i missing something? it seems to compile and run just that there are errors in the images – soong Jun 12 at 18:07
  • @soong there is a '}' missing at the end of the code. add that and it will work fine. – person the human Jun 16 at 3:22
2

There are a few issues going on here.

First, there's the test, as was previously stated.

        if((count=0))

This is an assignment, not a test. It'll set count to 0 every pass.

But the bigger problem is this. In the signature code blocks, the code opens the output file, writes the signature block, and then closes the output file! The output file needs to be left open so that additional non-signature blocks can be written to the output file!

Programming notes: As I said in a comment above, it would be more efficient, in the signature block code, to test whether a file is open and if open, close it. THEN, no matter what the value of count is, open a new output file. Any time you get two or more blocks of code that are identical, it's a red flag to check to see if the code can be written more efficiently, eliminating the duplicate blocks of code. It's not always possible, but should be checked.

Finally, there's this:

    char *pic=malloc(sizeof(char)*2048);

pic is a pointer to memory that will hold the name of the file to be created. The filename will be exactly 7 chars long, plus 1 char for the end of string marker, \0. That's 8 bytes. So, why allocate 2048 bytes for it? why not just allocate 8 bytes?

Take it a step further. Allocating dynamic memory during runtime is great for conditions where you have no idea how much memory will be needed overall, but when you know in advance exactly how much memory is needed, it's more appropriate and efficient to allocate regular stack variables. In this case, it would be less overhead and more efficient to just allocate a char array of 8 chars.

    char pic[8];

And remember to close all of the open files!

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

| improve this answer | |
  • Thank you so much I implemented your recommendations and the code is finally running! btw sorry to ask again but is there anything wrong with overallocation of memory? does it tend to slow the code down? – soong Jun 13 at 8:26
1

You have to work in your logic here.

First, the code in the if conditions if counter is 0 or not, is basically the same. It shouldn't be. If counter is 0, you have to create a new file, write the first 512 bytes and increment counter. If it's not 0, you have to close the previous file, create a new one and leave it open, in order to be able to write on it in the follow iterations.

Also you shouldn't close the file every time you write on it.

Don't forget to close both files when the while loop is finished.

| improve this answer | |
  • 1
    There's a slightly more efficient way to look at that. If count == 0, you have to close an open file. After that, no matter what the value of count is, you have to open a new file. – Cliff B Jun 12 at 21:08
  • If count == 0 there is no open file to close isn't it? – Tritum Jun 12 at 21:46
  • Good catch. I should have said "if count != 0" – Cliff B Jun 12 at 21:55
  • I know what you mean. The code for both conditions is the same except for the need to close the previous file. – Tritum Jun 12 at 22:01
  • Thank you I have changed the conditions and the code works now! – soong Jun 13 at 8:27
0

Your count stays at zero. Change the following

 if((count=0))

to

 if((count==0))
| improve this answer | |

You must log in to answer this question.

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