0

I'm writing a simple utility in C and I'm writing codes to print error messages in STDERR.

I have a struct, defined as:

struct arguments
{
    FILE *source;
    int modifier_value;
    int filesize;
};

I have declared a pointer to the above struct, and allocated memory to it:

struct arguments *arg = NULL;
arg = malloc(sizeof(struct arguments));
if(arg == NULL)
{
    fprintf(stderr, "error: malloc - %s\n", strerror(errno));  //I know i could use perror as well, but I like the convention of using fprintf() for both stdout, stderr, and other file streams, just for symmetry
    return EXIT_FAILURE;
}

As you can see, I have only allocated memory sufficient to store one object of type struct arguments and I still error checking for it.

The problem is I have many pointers like that which points to space of one object, and error checking all of them just increases number of codes and affects readability of code.

Is it a "good practice" / "is it ok" if I ignore error checking just for the reason that I'm not allocating memory too much memory (I heard something about paging and I think system combines many pages if I request for too much memory and chances of error in that case would be high, but not for memory request of something like 64 bytes).

Improve this question
10
  • 6
    Create a macro/wrapper function for the repeated code. Commented Nov 14, 2022 at 20:00
  • 1
    Yes it's a good idea. You don't know what's happening elsewhere on your computer, what other processes might have used the (virtual) memory. Commented Nov 14, 2022 at 20:01
  • 1
    ..and pass enough information to identify the source. Commented Nov 14, 2022 at 20:02
  • 3
    Also, never lay off the good habits. If you start making small easy shortcuts, then it's easier to to it elsewhere where it might really matter. Commented Nov 14, 2022 at 20:02
  • 2
    You can pass an argument to identify the caller. Perhaps as unique integer (although that is hard to maintain), or the __func__ name which resolves to a string. For example declare/define void *my_malloc(size_t size, const char *func); and then call as arg = my_malloc(sizeof(struct arguments), __func__); Commented Nov 14, 2022 at 20:16

3 Answers 3

Reset to default
1

Is it a "good practice" / "is it ok" if I ignore error checking just for the reason that I'm not allocating memory too much memory

It is poor practice to fail to error-check function calls that report on errors, except where you don't care whether the call succeeded. And you always care whether malloc() succeeds, or else you shouldn't be calling it in the first place. You don't know whether you are allocating too much memory unless you check whether your malloc() calls succeed.

The problem is I have many pointers like that which points to space of one object, and error checking all of them just increases number of codes and affects readability of code.

In the first place, use dynamic allocation only where you actually need it. Some people seem to have the idea that they need dynamic allocation wherever they want a pointer to an object. This absolutely is not the case. You need pointers if you are performing dynamic allocation, but you don't necessarily need dynamic allocation where you use pointers. Very often, static or automatic allocation can be combined with the address-of operator (unary &) instead. For example:

{
    struct arguments arg = {0};
    init_arguments(&arg);
    do_something(&arg);
    // all done with arg
}

You need dynamic allocation only when (i) you do not know at compile time how much memory you will need, or (ii) you need an object whose lifetime extends past the termination of the innermost block enclosing its creation.

When you really do need dynamic allocation, you can reduce the amount of boilerplate code by using a macro or a wrapper function. Similar applies to performing other success checks. For example, use a function such as this instead of using malloc() directly:

void *checked_malloc(size_t size) {
    void *result = malloc(size);

    if (result == NULL && size != 0) {
        fputs("error: malloc failed -- aborting...\n", stderr);
        abort();
    }
    return result;
}
Improve this answer
Sign up to request clarification or add additional context in comments.

5 Comments

Even with C17, checked_malloc(0) may still result in an abort(). IMO, better to simply return NULL or change to if (!result && size > 0) { to handle this pesky corner case.
Good point, @chux-ReinstateMonica. I have updated the example code accordingly.
Cinverse, Notice what has been just demonstrated here with John and me. A corner case improvement that only needed changes to one small function. Consider a large program that had many malloc() all over the place that had to deal with this and the large change needed. So nice to have the malloc() in one place.
@chux-ReinstateMonica Yeah, understood, thanks. Also, I think it would be good if value of size is mentioned in error message, as it would be another step ahead to identify exactly which call caused error. if size value is some garbage value, then we can Identify source by checking which code had the potential to make size have such value.
If that seems good to you, @Cinverse, then by all means go ahead. The code presented in this answer is meant to be demonstrative, not definitive. I applied the correction chux suggested as a matter of correctness for a corner case, but I consider the details of the error message to be a matter of fitting such a function to your particular purposes.
0

Create allocation wrapper functions to make always checking allocation success easy.

To expand on @Weather Vane idea of passing an argument to identify the caller, yet do it in a macro wrapper.

my_malloc.h

#ifndef MY_MALLOC
#define MY_MALLOC(size) my_malloc((size), __FUNC__, __LINE__)
#include <stdlib.h>

// Return a valid allocation or don't return.
void *my_malloc(size_t size, const char *func, unsigned line);

#endif

my_malloc.c

#include "my_maloc.h"

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

void *my_malloc(size_t size, const char *func, unsigned line) {
  if (size == 0) {
    return NULL;
  }
  void *ptr = malloc(size);
  if (ptr == NULL) {
    fprintf(stderr, "Failed to allocate %zu bytes at \"%s()\", line %u\n",
        size, func, line);
    exit(EXIT_FAILURE);
  }
  return ptr;
}

user_code.c

#include "my_malloc.h"

...
  // No need to explicitly pass the function name, line number
  arg = MY_MALLOC(sizeof *arg * n);
  // Error checking not needed.

  ...
  free(arg);

I'd even consider a matching MY_FREE where freeing passes in the pointer and the expected size. Then those 2 routines could keep allocation statistics, free size validation, ...

Improve this answer

Comments

-1

If you have many objects, you cam allocate memory for all immediately:

void *pvc;
pvc = malloc(how_many_for_all);

and then consecutively declare pointers to objects like this:

Obj1 *p1;
Obj2 *p2;
Obj3 *p3;
p1 = (Obj1)pvc;
p2 = (Obj2)(pvc + sizeof(Obj1));
p3 = (Obj3)(pvc + sizeof(Obj1) + sizeof(Obj2));

This is pseudocode sooner. Compiler will be make warnings, but this works.

Improve this answer

2 Comments

1) (Obj2)(pvc + sizeof(Obj1)) should be (Obj2*)((char *)pvc + sizeof(Obj1)) else the addition is wrong. 2) This works OK if the objects are the same size. If different, then alignment issues come into play.
My mistake, p1 = (Obj1 *)pvc;

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.