trjhtr

I'm teaching myself a little C, and I've found this website. It has an exercise about manipulating a BMP file in C. I would like to know if I can make my code look more "idiomatic" or professional, from the format of the comments to the name of the variables.

I also have some other questions:

1. This is the standard way to handle errors in C? There is such a thing?


2. When programming in C, I hear a lot the term "robust". Is my program robust enough? For example, should I always check for null in a function that takes a pointer as a parameter? Should I check for errors every time I close a file?


3. In bmp.c, the _string_duplicate function returns a string (an error message) on the heap. But in case of an error (can't allocate enough memory) it returns a string literal. I'm not sure about which is the best way to handle the error here. This returned string is going to be freed by the caller, but in case of an error in _string_duplicate the caller will be freeing a pointer to a string literal, which is bad.

bmp.c

/*
 * A program to read, write, and crop BMP image files.
 * 
 */
#include <stdio.h>
#include <string.h> // for strlen, strcopy
#include <stdlib.h> // for malloc
#include "bmp.h"

// Correct values for the header
#define MAGIC_VALUE 0x4D42 
#define NUM_PLANE 1
#define COMPRESSION 0
#define NUM_COLORS 0
#define IMPORTANT_COLORS 0
#define BITS_PER_PIXEL 24
#define BITS_PER_BYTE 8

BMPImage *read_bmp(FILE *fp, char **error);
bool write_bmp(FILE *fp, BMPImage *image, char **error);
bool check_bmp_header(BMPHeader *bmp_header, FILE *fp);
void free_bmp(BMPImage *image);
long _get_file_size(FILE *fp);
int _get_image_size_bytes(BMPHeader *bmp_header);
int _get_image_row_size_bytes(BMPHeader *bmp_header);
int _get_bytes_per_pixel(BMPHeader *bmp_header);
int _get_padding(BMPHeader *bmp_header);
int _get_position_x_row(int x, BMPHeader *bmp_header);
bool _check(bool condition, char **error, const char *error_message);
char *_string_duplicate(const char *string);

/*
 * Read a BMP image from an already open file.
 * 
 * - Postcondition: it is the caller's responsibility to free the memory
 * for the error message and the returned image.
 * 
 * - Return: the image as a BMPImage on the heap.
 */
BMPImage *read_bmp(FILE *fp, char **error)
 
 BMPImage *image = malloc(sizeof(*image));
 if (!_check(image != NULL, error, "Not enough memory"))
 
 return NULL;
 
 // Read header
 rewind(fp);
 int num_read = fread(&image->header, sizeof(image->header), 1, fp);
 if(!_check(num_read == 1, error, "Cannot read header"))
 
 return NULL;
 
 // Check header
 bool is_valid_header = check_bmp_header(&image->header, fp);
 if(!_check(is_valid_header, error, "Invalid BMP file"))
 
 return NULL;
 
 // Allocate memory for image data
 image->data = malloc(sizeof(*image->data) * image->header.image_size_bytes);
 if (!_check(image->data != NULL, error, "Not enough memory"))
 
 return NULL;
 
 // Read image data
 num_read = fread(image->data, image->header.image_size_bytes, 1, fp);
 if (!_check(num_read == 1, error, "Cannot read image"))
 
 return NULL;
 

 return image;


/*
 * Write an image to an already open file.
 * 
 * - Postcondition: it is the caller's responsibility to free the memory
 * for the error message.
 * - Return: true if and only if the operation succeeded.
 */
bool write_bmp(FILE *fp, BMPImage *image, char **error)

 // Write header
 rewind(fp);
 int num_read = fwrite(&image->header, sizeof(image->header), 1, fp);
 if (!_check(num_read == 1, error, "Cannot write image"))
 
 return false;
 
 // Write image data
 num_read = fwrite(image->data, image->header.image_size_bytes, 1, fp);
 if (!_check(num_read == 1, error, "Cannot write image"))
 
 return false;
 

 return true;


/*
 * Test if the BMPHeader is consistent with itself and the already open image file.
 * 
 * Return: true if and only if the given BMPHeader is valid.
 */
bool check_bmp_header(BMPHeader* bmp_header, FILE* fp)

 /*
 A header is valid if:
 1. its magic number is 0x4d42,
 2. image data begins immediately after the header data (header->offset == BMP HEADER SIZE),
 3. the DIB header is the correct size (DIB_HEADER_SIZE),
 4. there is only one image plane,
 5. there is no compression (header->compression == 0),
 6. num_colors and important_colors are both 0,
 7. the image has 24 bits per pixel,
 8. the size and image_size_bytes fields are correct in relation to the bits,
 width, and height fields and in relation to the file size.
 */
 return
 bmp_header->type == MAGIC_VALUE
 && bmp_header->offset == BMP_HEADER_SIZE
 && bmp_header->dib_header_size == DIB_HEADER_SIZE
 && bmp_header->num_planes == NUM_PLANE
 && bmp_header->compression == COMPRESSION
 && bmp_header->num_colors == NUM_COLORS && bmp_header->important_colors == IMPORTANT_COLORS
 && bmp_header->bits_per_pixel == BITS_PER_PIXEL
 && bmp_header->size == _get_file_size(fp) && bmp_header->image_size_bytes == _get_image_size_bytes(bmp_header);


/*
 * Free all memory referred to by the given BMPImage.
 */
void free_bmp(BMPImage *image)

 free(image->data);
 free(image);


/*
 * Create a new image containing the cropped portion of the given image.
 * 
 * - Params:
 * x - the start index, from the left edge of the input image.
 * y - the start index, from the top edge of the input image.
 * w - the width of the new image.
 * h - the height of the new image.
 * 
 * - Postcondition: it is the caller's responsibility to free the memory
 * for the error message and the returned image.
 * 
 * - Return: the cropped image as a BMPImage on the heap.
 */
BMPImage *crop_bmp(BMPImage *image, int x, int y, int w, int h, char **error)

 BMPImage *new_image = malloc(sizeof(*new_image));
 // Check size of cropedd image is less or equal than the size of original image
 if (!_check
 (
 x + w <= image->header.width_px && y + h <= image->header.height_px,
 error,
 "The size of the new image should be equal or less than the size of the original")
 )
 
 return NULL;
 
 // Update new_image header
 new_image->header = image->header;
 new_image->header.width_px = w;
 new_image->header.height_px = h;
 new_image->header.image_size_bytes = _get_image_size_bytes(&new_image->header);
 new_image->header.size = BMP_HEADER_SIZE + new_image->header.image_size_bytes;
 // Allocate memory for image data
 new_image->data = malloc(sizeof(*new_image->data) * new_image->header.image_size_bytes);
 if(!_check(new_image->data != NULL, error, "Not enough memory"))
 
 return NULL;
 
 int position_y = y * _get_image_row_size_bytes(&image->header);
 int position_x_row = _get_position_x_row(x, &image->header);
 int new_index = 0;
 // Iterate image's columns
 for (int i = 0; i < h; i++)
 
 // Iterate image's rows
 for (int j = 0; j < w; j++)
 
 // Iterate image's pixels
 for(int k = 0; k < 3; k++)
 
 new_image->data[new_index] = image->data[position_y + position_x_row];
 new_index++;
 position_x_row++;
 
 
 // Add padding to new_image
 int padding = _get_padding(&new_image->header);
 for (int l = 0; l < padding; l++)
 
 new_image->data[new_index] = 0x00;
 new_index++;
 
 position_y += _get_image_row_size_bytes(&image->header);
 position_x_row = _get_position_x_row(x, &image->header);
 

 return new_image;


/* 
 * Return the size of the file.
 */
long _get_file_size(FILE *fp)
 
 // Get current file position
 long current_position = ftell(fp);
 if (current_position == -1)
 
 return -1;
 
 // Set file position to the end
 if (fseek(fp, 0, SEEK_END) != 0)
 
 return -2;
 
 // Get current file position (now at the end)
 long file_size = ftell(fp);
 if (file_size == -1)
 
 return -3;
 
 // Restore previous file position
 if (fseek(fp, current_position, SEEK_SET) != 0)
 
 return -4;
 

 return file_size;


/* 
 * Return the size of the image in bytes.
 */
int _get_image_size_bytes(BMPHeader *bmp_header)

 return _get_image_row_size_bytes(bmp_header) * bmp_header->height_px;


/* 
 * Return the size of an image row in bytes.
 * 
 * - Precondition: the header must have the width of the image in pixels.
 */
int _get_image_row_size_bytes(BMPHeader *bmp_header)

 int bytes_per_row_without_padding = bmp_header->width_px * _get_bytes_per_pixel(bmp_header);
 return bytes_per_row_without_padding + _get_padding(bmp_header);


/*
 * Return size of padding in bytes.
 */ 
int _get_padding(BMPHeader *bmp_header)

 return (4 - (bmp_header->width_px * _get_bytes_per_pixel(bmp_header)) % 4) % 4;


/* 
 * Return the number of bytes per pixel.
 * 
 * Precondition:
 * - the header must have the number of bits per pixel.
 */
int _get_bytes_per_pixel(BMPHeader *bmp_header)

 return bmp_header->bits_per_pixel / BITS_PER_BYTE;


/*
 * Return the position of the pixel x from the beginning of a row.
 */ 
int _get_position_x_row(int x, BMPHeader *bmp_header)

 return x * _get_bytes_per_pixel(bmp_header);


/*
 * Check condition and set error message.
 */
bool _check(bool condition, char **error, const char *error_message)

 bool is_valid = true;
 if(!condition)
 
 is_valid = false;
 if (*error == NULL) // to avoid memory leaks
 
 *error = _string_duplicate(error_message);
 
 
 return is_valid;


/*
 * Make a copy of a string on the heap.
 * 
 * - Postcondition: the caller is responsible to free
 * the memory for the string.
 */ 
char *_string_duplicate(const char *string)

 char *copy = malloc(sizeof(*copy) * (strlen(string) + 1));
 if (copy == NULL)
 
 return "Not enough memory for error message";
 
 strcpy(copy, string);

 return copy;

The header file:

bmp.h

#ifndef _BMP_H_ // prevent recursive inclusion
#define _BMP_H_

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

#define BMP_HEADER_SIZE 54
#define DIB_HEADER_SIZE 40

#pragma pack(push) // save the original data alignment
#pragma pack(1) // Set data alignment to 1 byte boundary

// uint16_t is a 16-bit unsigned integer
// uint32_t is a 32-bit unsigned integer

typedef struct 
 uint16_t type; // Magic identifier: 0x4d42
 uint32_t size; // File size in bytes
 uint16_t reserved1; // Not used
 uint16_t reserved2; // Not used
 uint32_t offset; // Offset to image data in bytes from beginning of file
 uint32_t dib_header_size; // DIB Header size in bytes
 int32_t width_px; // Width of the image
 int32_t height_px; // Height of image
 uint16_t num_planes; // Number of color planes
 uint16_t bits_per_pixel; // Bits per pixel
 uint32_t compression; // Compression type
 uint32_t image_size_bytes; // Image size in bytes
 int32_t x_resolution_ppm; // Pixels per meter
 int32_t y_resolution_ppm; // Pixels per meter
 uint32_t num_colors; // Number of colors
 uint32_t important_colors; // Important colors
 BMPHeader;

#pragma pack(pop) // restore the previous pack setting

typedef struct 
 BMPHeader header;
 unsigned char* data;
 BMPImage;

BMPImage* read_bmp(FILE* fp, char** error);
bool check_bmp_header(BMPHeader* bmp_header, FILE* fp);
bool write_bmp(FILE* fp, BMPImage* image, char** error);
void free_bmp(BMPImage* image);
BMPImage* crop_bmp(BMPImage* image, int x, int y, int w, int h, char** error);

#endif /* bmp.h */

And a simple test.
I feel this one is a little messy. I'm not so sure about the helper functions I've made.

#include <stdio.h>
#include <stdlib.h> // for EXIT_SUCCESS and EXIT_FAILURE
#include "bmp.h"

BMPImage *read_image(const char *filename, char **error);
void write_image(const char *filename, BMPImage *image, char **error);
FILE *_open_file(const char *filename, const char *mode);
void _handle_error(char **error, FILE *fp, BMPImage *image);
void _clean_up(FILE *fp, BMPImage *image, char **error);

int main(void)

 char *error = NULL;
 BMPImage *image = read_image("6x6_24bit.bmp", &error);
 write_image("copy.bmp", image, &error);
 BMPImage *crop_image = crop_bmp(image, 2, 3, 4, 2, &error);
 write_image("crop.bmp", crop_image, &error);

 bool is_valid = check_bmp_header(&crop_image->header, fopen("crop.bmp", "rb"));

 _clean_up(NULL, image, &error);
 _clean_up(NULL, crop_image, &error);

 return EXIT_SUCCESS;


BMPImage *read_image(const char *filename, char **error)

 FILE *input_ptr = _open_file(filename, "rb");

 BMPImage *image = read_bmp(input_ptr, error);
 if (*error != NULL)
 
 _handle_error(error, input_ptr, image);
 
 fclose(input_ptr);

 return image;


void write_image(const char *filename, BMPImage *image, char **error)

 FILE *output_ptr = _open_file(filename, "wb");

 if (!write_bmp(output_ptr, image, error))
 
 _handle_error(error, output_ptr, image);
 
 fclose(output_ptr);


/*
 * Open file. In case of error, print message and exit.
 */
FILE *_open_file(const char *filename, const char *mode)

 FILE *fp = fopen(filename, mode);
 if (fp == NULL)
 
 fprintf(stderr, "Could not open file %s", filename);

 exit(EXIT_FAILURE);
 

 return fp;


/*
 * Print error message and clean up resources.
 */
void _handle_error(char **error, FILE *fp, BMPImage *image)

 fprintf(stderr, "ERROR: %sn", *error);
 _clean_up(fp, image, error);

 exit(EXIT_FAILURE);


/*
 * Close file and release memory.
 */
void _clean_up(FILE *fp, BMPImage *image, char **error)

 if (fp != NULL)
 
 fclose(fp);
 
 free_bmp(image);
 free(*error);

I would like to know if I can make my code look more "idiomatic" or professional,

Packing

#pragma pack(push) #pragma pack(1) are not standard C. Yet BMPHeader is a pain due to the first member is 16-bit. Unless high portability is needed, stay with the non-standard packing for now.

unsigned char

Other struct members used fixed width types (good). Recommend to apply that to data. It is more informative and will force a necessary compiler error on a rare machine with 16-bit char.

typedef struct 
 BMPHeader header;
 // unsigned char* data;
 uint8_t* data;
 BMPImage;

bmp.h Comments

A user of the code may never see bmp.c, so comment important attributes in bmp.h. Consider more documentation in this file.

Take the below from bmp.c and put only in bmp.h - and so for all global functions. In the bmp.c leave a comment with the function if desired to "see bmp.h". You do not want to maintain dual documentation.

/*
 * Read a BMP image from an already open file.
 * 
 * - Postcondition: it is the caller's responsibility to free the memory
 * for the error message and the returned image.
 * 
 * - Return: the image as a BMPImage on the heap.
 */

What does true mean in bool check_bmp_header()?

uint16_t is a 16-bit unsigned integer is an unnecessary comment.

Magic identifier: 0x4d42 is more informative as 'B' 'M'

Its is not stated here that *error in read_bmp(FILE* fp, char** error) requires prior assignment before calling this function.

Unclear why int x4 in crop_bmp(BMPImage* image, int x, int y, int w, int h, char** error) when the format type uses int32_t. I'd recommend the same type. (or at least consider what code needs to do when int is narrower then int32_t.)

Good use of minimal #include <> files.

Error message

The error handling, often a tricky point, lacks symmetry here amongst the various ..._bmp().

As error messages are copy of string literals, instead of copying the text, copy the pointer and make know the user need not free it.

Consider instead of a sometimes char** error function parameter, add a .error member to BMPImage.

Naming

The bmp.h file uses BMP_..., DIB_..., BMP..., ..._bmp naming. I would strive for consistency and use only bmp_... and BMP_...

bmp.c: Use static functions

_get_file_size(), ... _string_duplicate(), should bestatic. these function names are not needed outside `bmp.c

bmp.c: global functions

No need for another declaration of functions found in bmp.h

include test

In bmp.c only, code #include "bmp.h" first to test that it compiles without prior #includes.

#include "bmp.h"
#include <stdio.h>
#include <string.h>
#include <stdlib.h>
// #include "bmp.h"
#include "other_user_header_files1.h"
#include "other_user_header_files2.h"

Comments like // for strlen, strcopy are a pain to maintain. I recommend dropping that for standard headers.

No computational check

sizeof(*image->data) * image->header.image_size_bytes lacks overflow protection. Be very careful with external data from files that can cause undefined behavior with mis-szied allocation. Watch out for .image_size_bytes == 0 as the allocation may be NULL (which is not a "Not enough memory", but other concern).

uint64_t sz = image->header.image_size_bytes
sz *= sizeof(*image->data);
image->data = NULL;
if (sz <= SIZE_MAX) 
 image->data = malloc((size_t) sz);

if (image->data == NULL && sz > 0) 
 // assign error

free() style

free() tolerates free(NULL) and so I recommend to do so here with free_bmp(BMPImage *image)

void free_bmp(BMPImage *image) 
 if (image) 
 free(image->data);
 free(image);
 

Advanced:

Endian

.bmp file format assume an endian (little) and so each member of BMPHeader requires an endian adjustment should code compile on a non-little endian machine for both reading and writing.

Minor:

Allocating

Good use of allocating by size of de-referenced pointer rather than type. Style thought: () not needed

image = malloc(sizeof(*image));
// or
image = malloc(sizeof *image);

GTG