Thread: Question regarding global variable in C

I have a simple logger function. It takes two parameters:

Code:

char * string 
int severity

The logger function is called by multiple (about 20) functions, distributed over 3 source files. For instance in this way:

Code:

snprintf(logMsg,sizeof(logMsg),"Client connection accepted by %zu..",myId);
logr(logMsg,1);

I have an option to make the "logMsg" var global, meaning I wouldn't have to declare this variable in every function where I use the logger function. Or would it be better to just declare the logMsg var whenever it is needed?

I would like to hear any thoughts on that, and here is the logger function so far:

Code:

void logr(char* msg, int sev){
        int loglvl=3; //0-3 - debug, info, warn, crit
        const char * sevs[4];
        sevs[0]="debug";
        sevs[1]="info";
        sevs[2]="warn";
        sevs[3]="crit";

        char logfile[12]="logfile.log\0";

        if(sev >= loglvl){
                FILE *fp=fopen(logfile, "a");
                fprintf(fp, "%zu :: %s\t%s\n",time(NULL),sevs[sev],msg);
                fclose(fp);
        }
        memset(msg,0,sizeof(*msg));
}

It is usually a good idea to avoid global variables whenever possible.

Will your log message always be built with a single variable? If so, you can just pass the string and that variable to your log function and build the string in there.

Code:

#include <stdio.h>

#define LOG_STR_MAX  64

// Additional functionality excluded for this example
void logr(const char *message, int id)
{
    char log_str[LOG_STR_MAX];

    snprintf(log_str, sizeof(log_str), "%s %d", message, id);

    printf("%s\n", log_str);  // replace w/ write to file
}

int main(void)
{
    logr("Error message, ID:", 4);

    return 0;
}

If not, other approaches could be taken to avoid using a global variable.

---

Some comments on your logr() function:

If "loglvl" is to be a constant, define it as a constant. You can use this constant throughout the function. I also personally find it neater to initialize the pointer array during declaration.

Code:

#define SEVERITY_MAX  4

void logr(char *msg, int severity)
{
    const char *severity_str[SEVERITY_MAX] = {
        "debug",
        "info",
        "warn",
        "crit"
    };
    
    // ...
    
    if(severity < SEVERITY_MAX)
    {
        // ...
    }
}

~

Code:

char logfile[12]="logfile.log\0";

You don't need to specify the length of this array if it is initialized with a string constant, the compiler will figure it out. You also don't need to manually include the null-character; this is appended automatically (given that there's room, which is another reason not to explicitly provide the size of the array):

Code:

char logfile[] = "logfile.log";

If this string will never be changed though, just use:

Code:

const char *logfile = "logfile.log";

~

Code:

if(sev >= loglvl)

You probably mean <= here. Otherwise, the following sevs[sev] will be out of bounds.

By using the named constant as I mentioned above, this can be a < instead of <= (as illustrated in the second code snippet).

~

Code:

FILE *fp=fopen(logfile, "a");

You should be verifying that the file has opened successfully before attempting to access it.

~

Code:

memset(msg,0,sizeof(*msg));

sizeof(*msg) is 1, as *msg is a single character. The function does not know the size of the array, as it actually only receives a pointer. One option would be to pass the size of the array to the function as well. However, do you really need to be clearing the received message? If not, this is a moot point, and the "msg" parameter can be const char*.

Without seeing your code, I can only give the following.

It is usually better to define variable(s) in main(), or in a lower functions if only needed from there down, then pass the data to the functions as arguments.

Global data can be altered in any function, making it difficult to track down an error in the code that incorrectly alters the data!

Why do you open the file each time a message is logged. Open once at the start of the program, and close at the end. There is a lot of overhead to open and close a file.

"sizeof(*msg)" You only have a pointer to the original array, but cannot determine the size with sizeof(). Whenever you pass an array to a function, you should pass the array size as a 'size_t' type, (The same type returned from sizeof()).

Better:

Code:

msg[0] = '\0';

That is all that is needed to clear the string.

If you find your code passing a lot of local variables to other functions, you could define a struct and pass a pointer to the struct, as a single argument.

I have an option to make the "logMsg" var global, meaning I wouldn't have to declare this variable in every function where I use the logger function.

I would probably stick to the "local" variable. Most of the time the "message" will probably be some constant in that local scope so keeping the variable local will probably not affect most things.

By way in your logger() function why are you trying to "clear" the message. This variable should probably be a constant in this scope. Plus that sizeof() call is not correct in this scope for a couple of reasons. First (*msg) is a single character. Second when you passed the string into this function you lost any "size" information because all you have is the pointer. You should be using strlen() in this instance, if you really must alter the string.

IMO the opening and closing of the file will take more resources than creating a string in the various calling function. If you're going to continually open and close the log file in the log function then you really should check that the file opened correctly and that the write operation actually succeeded and take appropriate action on a failure.



Jim

If the log output is meant for human consumption than it's pretty much useless to print out the raw time() value. you should convert it to a human-readable format. And it's a hassle to build the message string in the calling functions. You should use a variable argument list:

Code:

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

#define LOGFILE "logfile.log"

void logr(const char *fmt, ...) {
    va_list va;
    va_start(va, fmt);

    time_t t = time(NULL);
    char buf[50];
    strftime(buf, sizeof buf, "%c", localtime(&t));

    FILE *f = fopen(LOGFILE, "a");
    fprintf(f, "%s :: ", buf);
    vfprintf(f, fmt, va);
    fputc('\n', f);
    fclose(f);

    va_end(va);
}

int main() {
    size_t id = 1234;
    logr("Client connection accepted by %zu", id);
    return 0;
}

I am impressed by the excellent suggestions, all very helpful indeed!

Thanks a lot guys/girls, can you hear the new synaptic connections being formed? I can and will post the new and improved version of this function, hoping for another review.

Had some fun implementing these suggestions that I got from you guys:

//!implement varargs and build log string in function
//!define loglvl as a constant..
//!initialize pointer array during declaration
//!remove explicit size definition of array char logfile and make const
//!verify file open success before writing
//!open file on startup, and keep open until program closes.
//! GV struct: If you find your code passing a lot of local variables to other functions
// you could define a struct and pass a pointer to the struct, as a single argument
//!implement strftime for human readability
//!fix out of bounds possibility sevs[sev]

My first effort resulted in this as a quick modification:

Code:

#include <stdio.h>
#include <stdlib.h>
#include <time.h>
#include <stdarg.h> //varargs
#define loglvl 1
#define maxloglvl 4

 //GV, generic vars - to be passed to multiple functions
struct GV{
        FILE * lfp;
        int wSock;
};

FILE * openLogFile(){
                const char logfile[]="tcp_pthread_coban.log";
                FILE *lfp=fopen(logfile, "a");
                if(lfp == NULL){
                        printf("Error opening log file '%s', exit..\n",logfile);
                        exit(1);
                }else{
                        return lfp;
                }
}

void logr(struct GV *genVars,const int sev, const char* msg, ...){
        const char * sevs[4] = {
                "debug","info","warn","crit"
        };

        char buf[256];
        va_list args;
        va_start (args, msg);
        vsnprintf (buf,256,msg, args);

        time_t t = time(NULL);
        char strftBuf[50];
        strftime(strftBuf, sizeof strftBuf, "%c", localtime(&t));

        if(sev >= loglvl && sev < maxloglvl){
                fprintf(genVars->lfp, "%s :: %s\t%s\n",strftBuf,sevs[sev],buf);
                printf("%s :: %s\t%s\n",strftBuf,sevs[sev],buf);
        }
}

int main(){
        //inititalize struct for passing frequently used vars to functions
        struct GV genVars;
        genVars.lfp=openLogFile();
        genVars.wSock=2; //just an example

        //usage: logr(GV struct *, severity 0-3, const string, ...multivar)
        logr(&genVars,2,"logmsg with one param, string: %s","strinLitValue");
        logr(&genVars,3,"logmsg with two parameters, string/int: %s, %d","AnotherstringLitValue",4312);

        fclose(genVars.lfp);
        return 0;
}

I think I covered most comments, but feel free to take it apart

Thanks algorism, that answers a lot of unknowns for me!

Be consistent.

I will try

Macro constants are always in UPPERCASE in common C styles.

Noted!

The best solution is to make it a static global in the logger code file. Then it's only visible in that file. Any modifications to its data needed by other files could be done through setter/getter functions. You could pass in your min/max log levels and even the log file name when initializing the logger.

I like that

You forgot to use va_end. That's important!

Oeps, will do

There's not much point in making sev a const int in logr. People don't tend to use const like that. It's more for promising the caller that you won't change his data, which is impossible in that case since you get your own copy.

Still parsing this comment

I find it odd that your severity levels are 0 to 3 but your log level and max log level are (apparently) 1 to 4. You should make them the same and alter your comparisons as necessary.

My logic is as follows: sev is 0,1,2 or 3. if sev < 4 its ok, bounds check done. I understand your assumption and make it less confusing

Thanks again m8!

Originally Posted by Salem

> if (sev >= logVars.min_lvl && sev <= logVars.max_lvl)
All the code should be inside this condition.
There's no point going to all the trouble of formatting a string, just to throw it all away.

You are absolutely right