Defining function macros

Question

Hello,

gcc 4.4.1 C99

I have written a function that I will use for displaying messages to the user. This works ok, and I will need it to be scalable as I will have more error codes to add later.

However, the inputs will be the priority, error_code, and a short description of the error. I have included the function, and the line number.

However, I want to wrap the function call into a macro, but not sure how to go about doing it?

Can anyone point me in the write directions?

Many thanks for any suggestions,

#include <stdio.h>
#include <stdarg.h>

typedef enum
{
    ST_BIND_ERR = 1,
    ST_SOCK_ERR,
    ST_CONNECT_ERR,
    ST_ACCEPT_ERR
}error_codes;

typedef enum
{
    ST_CRITICAL = 1,
    ST_WARNING,
    ST_DEBUG,
    ST_INFO
}priority;

#define REPORT(prio, err, msg) /* Defining macro here */

void report_msg(int prio, int err, const char *fmt, ...);

int main(void)
{
    printf("=== Starting program ===\n");

    report_msg(ST_WARNING, ST_CONNECT_ERR, "Error trying to make connection : FUNCTION [ %s ] : LINE [ %d ]", 
     __func__, __LINE__);

    return 0;
}

void report_msg(int prio, int err, const char *fmt, ...)
{
    va_list ap;
    char priority_msg[512] = {0};
    char error_code[256]   = {0};
    char format[256]       = {0};
    char output_msg[256]   = {0};

    switch(prio)
    {
    case ST_CRITICAL:
     sprintf(priority_msg, "[ ST_CRITICAL ]");
     break;

    case ST_WARNING:
     sprintf(priority_msg, "[ ST_WARNING ]");
     break;

    case ST_DEBUG:
     sprintf(priority_msg, "[ ST_DEBUG ]");
     break;

    case ST_INFO:
     sprintf(priority_msg, "[ ST_INFO ]");
     break;

    default:
     sprintf(priority_msg, "[ UNKNOWN_PRIO ]");
     break;
    }

    switch(err)
    {
    case ST_BIND_ERR:
     sprintf(error_code, "[ ST_BIND_ERR ]");
     break;

    case ST_SOCK_ERR:
     sprintf(error_code, "[ ST_SOCK_ERR ]");
     break;

    case ST_CONNECT_ERR:
     sprintf(error_code, "[ ST_CONNECT_ERR ]");
     break;

    case ST_ACCEPT_ERR:
     sprintf(error_code, "[ ST_ACCEPT_ERR ]");
     break;

    default:
     sprintf(error_code, "[ UNKNOWN_ERR ]");
     break;
    }

    va_start(ap, fmt);
    vsprintf(format, fmt, ap);
    va_end(ap);

    sprintf(output_msg,"%s %s %s", priority_msg, error_code, format);

    fprintf(stderr, output_msg);
}

Answer 1

C99 supports vararg macros, which it seem you could use to make it a bit more convenient:

#define REPORT(prio, err, format, ...) report_msg(prio, err, format, __VA_ARGS__)

This doesn't actually seem to save that much work over just calling the function, though. Perhaps you should re-define the REPORT macro to include the priority, or something?

Answer 2

Does the caller actually need a variable number of arguments? For instance, do you want them to be able to do something like this?

REPORT(ST_WARNING, ST_CONNECT_ERR, "Error trying to make connection : "
    "FUNCTION [ %s ] : LINE [ %d ], target address [ %s]", target);

I'll assume so, because if not then there's no need for varargs at all.

So, on that assumption, I think I'd first do this:

#define REPORT(prio, error, format, ...) report_msg(prio, error, format, __func__, __LINE__, __VA_ARGS__)

But, if you do that, then you're relying on the caller to correctly incorporate the function and line into the error message. This is hassle for the caller. So actually I might go for something more like this:

#define REPORT(prio, error, format, ...) report_msg(prio, error, format, __func__, __LINE__, __VA_ARGS__)

void report_msg(int prio, int err, const char *fmt, const char *func, int line, ...) {
    // print the prio and error codes
    // ...

    // I've put some fprintf in here to avoid introducing even more buffers,
    // but you can still do what you were doing before, building one big message.
    fprintf("in function %s at line %d\n", func, line);
    va_start(ap, fmt);
    vsprintf(format, fmt, ap);
    va_end(ap);
    fprintf("\t%s\n", format);
}

Then the caller does this:

REPORT(ST_WARNING, ST_CONNECT_ERROR, "target address [ %s ]", my_addr);

And the error looks like this:

[ST_WARNING] [ST_CONNECT_ERROR] in function main at line 28
    target address [ 69.59.196.211 ]

Final note: if these priority and error codes are only used with this function, then it might be better to #define them as strings in the first place, and change the corresponding parameters to const char*. That way you don't need the switch statement, and you can add new codes effortlessly (or callers can just quickly specify some other string, if they want an eye-catcher while doing debugging).

Even if they do have to be numbers, your switches contain unnecessary formatting. You could do something like this:

char *priority_msg;
switch(prio) {
    case ST_WARNING: priority_msg = "[ ST_WARNING ]"; break;
    // other cases...
    default: priority_msg = "[ UNKNOWN_PRIO ]"; break;
}

or this:

char *priorities[] = {"[ UNKOWN_PRIO ]", "[ ST_ERROR ]", "[ ST_WARNING ]", ... };
char *priority_msg = priorities[0];
if (prio >= 0) && (prio < sizeof(priorities) / sizeof(*priorities)) {
    priority_msg = priorities[prio];
}

This works as long as you know that the priorities are consecutive numbers starting from 1, and as long as you make sure that the enum and the strings are kept in sync. So it's a little harder to modify than the big switch statement, but IMO a little easier to read.