Reading a file 16 bytes at a time in C

Question

I am trying to read a file parsed from a command line argument 16 bytes at a time. I am storing the bytes in an unsigned char array. I am then trying to print the elements in their HEX format and then, if they are printable characters, I am printing them and if not I am printing a dot "." I also would like to print the offset of the byte from the beggining of the file on each new line but I want to get the rest working before I start working on that. The problem I am having is the file I am reading from is not printing so I don't think I am doing it right. I started out using fread() but I think I may be needing to use fseek() but I am unsure. If anyone could point me in the right direction or tell me if I'm doing anything wrong I'd appreciate it.

Code:

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

int main(int argc, char *argv[])
{
    FILE *fp;

    int i, j;

    unsigned char buffer[17];

    fp = fopen(argv[1], "r");

    while(!feof(fp))
    {
        while(fread(buffer, 16, 1, fp) !=0)
        {

            for(i=0; i < 16; i++)
            {
                printf("%X ", buffer[i]);
            }

            for(j = 0; j < 16; j++)
            {
                if(isprint(buffer[j]))
                    printf("%s", buffer[j]);
                else
                    printf("." );
            }

            printf("\n");
        }
    }

    fclose(fp);

    return 0;
}

Expected output:

0001:  40 4F 28 37 0B B8 FF 9D 81 5E 2E 73 B5 CC 6A 70 @O(7.....^.s..jp
0010: 0F 30 9C 6E 7E F7 53 E0 C1 5E 9A 38 C5 02 F2 33 .0.n .S..^.8...3

EDIT: Fixed problem with suggestions. Was opening file in text mode instead of binary. Changed read mode to "rb" and code is working correctly now.

Answer 1

You need to open the file in binary mode, using fseek/ftell on a file in text mode gives erratic values.

So open the file with

fp = fopen(argv[1], "rb");

also good practice is to always check if a function succeeded by checking the return value, in this case if (fp != NULL) ...

Instead of reading 16 byte chunks read 1 byte 16 times, otherwise if the file length is not evenly dividable by 16 you miss the last bytes

if ( fp != NULL )
{
  int read = 0;
  while((read = fread(buffer, 1, 16, fp)) > 0)
  {
    for(i=0; i < read; i++)
    {
    ...
  }
  fclose(fp);
}

IOW no need to for the outer while (feof(fp)) ...

in

printf("%s", buffer[j]);

you are not using the correct format specifier, buffer[j] is a character, not a string so write

printf("%c", buffer[j]);

EDIT: if you are not in some embedded environment with minimal stack reading 16 bytes at a time is not so effective, you can just as might read 2k or some bigger size to read faster.