BMP file writer

Question

In my current project I'm writing a C function to write a BMP-File. Internally, the function is split up into three parts: write header, write info header, write data.

In some of these functions I do need to allocate heap space with malloc and actually write to disk with fwrite. These functions are doomed to fail from time-to-time (malloc can't find a big enough memory chunk to allocate and fwrite could fail as well).

Now I'm unsure of how to handle failure situations reasonably. My current approach is to have a returned integer value indicate success.

uint8_t BMP32_write(FILE *output, BMP32_FILE *bmp32_file) { 
 uint8_t res = 0;
 res |= BMP32_write_header(output, bmp32_file->bmph);
 res |= BMP32_write_info_header(output, bmp32_file->bmpih);
 res |= BMP32_write_rgbData(output, bmp32_file->bmpih->biWidth, bmp32_file->bmpih->biHeight, bmp32_file->rgbData);
 return res;
}
static uint8_t BMP32_write_header(FILE *output, BMP32_HEADER *h) {
 uint8_t res = 0;
 res |= !fwrite(&h->bfType , sizeof(h->bfType) , 1, output);
 res |= !fwrite(&h->bfSize , sizeof(h->bfSize) , 1, output);
 res |= !fwrite(&h->bfReserved, sizeof(h->bfReserved), 1, output);
 res |= !fwrite(&h->bfOffBits , sizeof(h->bfOffBits) , 1, output);
 return res;
}
static uint8_t BMP32_write_info_header(FILE *output, BMP32_INFO_HEADER *ih) {
 uint8_t res = 0;
 res |= !fwrite(&ih->biSize , sizeof(ih->biSize) , 1, output);
 res |= !fwrite(&ih->biWidth , sizeof(ih->biWidth) , 1, output);
 res |= !fwrite(&ih->biHeight , sizeof(ih->biHeight) , 1, output);
 res |= !fwrite(&ih->biPlanes , sizeof(ih->biPlanes) , 1, output);
 res |= !fwrite(&ih->biBitCount , sizeof(ih->biBitCount) , 1, output);
 res |= !fwrite(&ih->biCompression , sizeof(ih->biCompression) , 1, output);
 res |= !fwrite(&ih->biImageSize , sizeof(ih->biImageSize) , 1, output);
 res |= !fwrite(&ih->biXPelsPerMeter, sizeof(ih->biXPelsPerMeter), 1, output);
 res |= !fwrite(&ih->biYPelsPerMeter, sizeof(ih->biYPelsPerMeter), 1, output);
 res |= !fwrite(&ih->biClrUsed , sizeof(ih->biClrUsed) , 1, output);
 res |= !fwrite(&ih->biClrImportant , sizeof(ih->biClrImportant) , 1, output);
 return res;
}
static uint8_t BMP32_write_rgbData(FILE *output, uint32_t width, uint32_t height, RGB_POINT *rgbData) {
 uint32_t i, currRow, currColumn, bytesPerRow;
 uint8_t *rawDataRow, res;
 bytesPerRow = widthToBytesPerRow(width);
 rawDataRow = calloc(bytesPerRow, sizeof(uint8_t));
 if (rawDataRow != NULL) {
 for (currRow = height; currRow > 0; currRow--) {
 for (currColumn = 0, i = 0; currColumn < width; currColumn++) {
 rawDataRow[i++] = rgbData[width*(currRow-1) + currColumn].blue;
 rawDataRow[i++] = rgbData[width*(currRow-1) + currColumn].green;
 rawDataRow[i++] = rgbData[width*(currRow-1) + currColumn].red;
 }
 res |= !fwrite(rawDataRow, bytesPerRow, 1, output);
 }
 free(rawDataRow);
 } else {
 res = 1;
 }
 return res;
}

What is a common way to handle situations like this? What would you consider doing?

Answer 1

To answer you question on how to handle this, one needs to address why your current approach is good or not. And the main issue I have with your code is that if either fwrite fails, you'll happily continue until all three helper methods have completed. This is not good behaviour, and might trigger bigger errors which might terminate your program, removing your chance of correcting the initial errors.

According to this documentation fwrite will return the number of bytes written, but you keep on or'ing the result as if you're expecting 0 to indicate an OK write. Either way your or-ing will possibly hide the initial error code. I.e if the first fail returns 1 and the second fails with 2 you get an error code of 3. Anyway the final error code is misleading as to why your code originally failed.

Two alternate ways to handle these are:

1. Return at first fail
2. Skip later executions of fwrite after fail

Return at first fail

The easiest way to handle this, is to make your own fwrite, which in addition to the normal parameters has a result flag variable initiated within BMP32_write, where the local variant never executes the fwrite if the flag variable is set. This removes the possibility for changing the error code, and re-executing the fwrite after failing.

Skip later executions

An alternate version I've used in production code for a company is to have code which essentially behaves like the following:

res = res < 0 ? res : fwrite( ... );
res = res < 0 ? res : fwrite( ... );
res = res < 0 ? res : fwrite( ... );
/* Here I've used `res < 0` as the error situation, which might
 need to be replaced with `res == 0` depending on your fwrite
 implementation. */

If either fwrite fails, it sets a value for res and skips the other calls. This can be interleaved whenever you want to break out, i.e. it might be good to bail out BMP32_write if either of the three helper functions fails. This can be prettified using macros, if you want to.

Expected return from fwrite

If your implementation of fwrite returns number of bytes written, you need to compare it each time you write to the file to check if an error situation has arisen or not (compare written bytes with original block size). This can be easily achieved if you write your own fwrite as suggested above.

If your implementation of fwrite does indeed return 0, you might have an issue if compiling your code on a different platform where they have a more standardized version of fwrite...

• LastSecondsToLive: Thanks for the answer. Since fwrite returns the number of elements written and I try to write one element at a time the possible return values are 0 or 1. The or will keep the 1 after each function call, but I do really like your variation with the conditional operator to skip further function calls (and prefer it over the extra function). What do you think of the return type? Is uint8_t reasonably or should I use something like an enum like mentioned below?
• LastSecondsToLive: Notice that fwrite returns the number of objects written not bytes.
• LastSecondsToLive: Also how would the macro of the conditional operator look like?
• holroy: @LastSecondsToLive, Even though it returns the object count with your parameters, it's a dangerous way of handling the error and you still allow for calling the fwrite after the first failure which is dangerous. Regarding return type it's usually better to keep that of fwrite as long as you don't "translate" it into an enum or similar.
• holroy: The macro can have different appearances depending on how much you would like to hide... The entire res = res > 0 ? res : fwrite ( ... ) could be within the macro (having the ... as macro parameter, or you could have only the start of the ternary, res > 0 ? res within the macro.

Answer 2

Providing some kind of error information is IMO the right way to go. You have, however, used an uint8_t as the return type, with possible values of 0 for success and 1 for failure.

I would recommend to use a different, more descriptive and more typesafe way. Possibilities are:

• A typedef for the int return type, plus symbolic constants (macros or constant values) to indicate success or error.
• An enumeration type with properly named enumeration constants.
• Maybe more :-)

The typesafety, btw., is not part of C (C will happily convert all these values from and to), but is achieved since compilers and static code analysis tools will warn you about mixing these types.