A neutral way to handle errors from multiple fread calls

Question

I have the following method in C for downloading a binary, it seems quite long and tedious to check the error value from each call fread, is there an easier way to handle this?

I know that some calls can be reduced by reading in the structure at a time, but due to the way C can add padding bytes between the elements of the structure, I would rather avoid this.

some_type_t *load_something(FILE *file) {
    some_type_t *something = (some_type_t *)malloc(sizeof(some_type_t));
    if (something == NULL) {
        return NULL;
    }

    if (fread(&something->field1, sizeof(something->field1), 1, file) == 0) {
        free(something);
        return NULL;
    }
    if (fread(&something->field2, sizeof(something->field2), 1, file) == 0) {
        free(something);
        return NULL;
    }
    if (fread(&something->field3, sizeof(something->field3), 1, file) == 0) {
        free(something);
        return NULL;
    }

    uint16_t some_var1, some_var2, some_var3;

    some_other_type_t *something_else1 = (some_other_type_t *)malloc(sizeof(some_other_type_t));
    if (fread(&some_var1, sizeof(some_var1), 1, file) == 0) {
        free(something);
        free(something_else1);
        return NULL;
    }

    some_other_type_t *something_else2 = (some_other_type_t *)malloc(sizeof(some_other_type_t));
    if (fread(&some_var2, sizeof(some_var2), 1, file) == 0) {
        free(something);
        free(something_else1);
        free(something_else2);
        return NULL;
    }

    some_other_type_t *something_else3 = (some_other_type_t *)malloc(sizeof(some_other_type_t));
    if (fread(&some_var3, sizeof(some_var3), 1, file) == 0) {
        free(something);
        free(something_else1);
        free(something_else2);
        free(something_else3);
        return NULL;
    }
    // Do something with the vars and allocated something elses.
    // ...

    return something;
}

Answer 1

Why not create a macro:

#define READ_FIELD(data) \
    do { if (fread(&data, sizeof(data), 1, file) == 0) { \
        free(something); \
        free(something_else1);
        free(something_else2);
        return NULL; \
    } } while(0)

then name it as a function call:

READ_FIELD(something->field1);
READ_FIELD(something->field2);

READ_FIELD(some_var1);
READ_FIELD(some_var2);

, , , / ( ).

free , , . - NULL, free . :

free(something); something = NULL;

(, something , NULL , )

, M Oehm , , / :

#define DO_ALL \
    DO_FIELD(something->field1); \
    DO_FIELD(something->field2); \
    DO_FIELD(some_var1); \
    DO_FIELD(some_var2)

DO_FIELD READ_FIELD WRITE_FIELD:

#define DO_FIELD READ_FIELD
DO_ALL;
#undef DO_FIELD

Answer 2

, , , , goto ( goto C, ):

    if (first_call() < 0) goto error;
    if (second_call() < 0) goto error;

    // [...]
    // when everything succeeded:
    return result;

error:
    // free resources
    // return error-indicator, e.g.
    return 0;

, NULL/0 ( ). free() error , . " ", , , free() - NULL, no-op.

Answer 3

something NULL .

    ...
    something = something_else1 = something_else2 = something_else3 = NULL;
    ...
    some_other_type_t *something_else3 = (some_other_type_t *)malloc(sizeof(some_other_type_t));
    if (fread(&some_var3, sizeof(some_var3), 1, file) == 0) {
      goto error;
    }
    // Do something with the vars and allocated something elses.
    // ...

    return something;

  error:
    free(something);
    free(something_else1);
    free(something_else2);
    free(something_else3);
    return NULL;
  }

NULL , , , someting NULL free.

Sidenote: C malloc.

Answer 4

Here is a simple way to group operations mallocand freadand check only once to complete correctly:

some_type_t *load_something(FILE *file) {
    uint16_t some_var1, some_var2, some_var3;
    some_type_t *something = malloc(sizeof(*something));
    some_other_type_t *something_else1 = malloc(sizeof(*something_else1));
    some_other_type_t *something_else2 = malloc(sizeof(*something_else2));
    some_other_type_t *something_else3 = malloc(sizeof(*something_else3));

    if (!something || !something_else1 || !something_else2 || !something_else3 ||
        !fread(&something->field1, sizeof(something->field1), 1, file) ||
        !fread(&something->field2, sizeof(something->field2), 1, file) ||
        !fread(&something->field3, sizeof(something->field3), 1, file) ||
        !fread(&some_var1, sizeof(some_var1), 1, file) ||
        !fread(&some_var2, sizeof(some_var2), 1, file) ||
        !fread(&some_var3, sizeof(some_var3), 1, file))
    {
        free(something);
        free(something_else1);
        free(something_else2);
        free(something_else3);
        return NULL;
    }

    // Do something with the vars and allocated something elses.
    // ...

    return something;
}

Note that passing a null pointer to free()is OK.