HiveBrain v1.2.0
Get Started
← Back to all entries
debugcMinor

BMP file writer

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
writerfilebmp

Problem

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

Solution

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:

  • Return at first fail



  • 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...

Code Snippets

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. */

Context

StackExchange Code Review Q#119703, answer score: 4

Revisions (0)

No revisions yet.