Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

This is a piece of C89 code testing a function titled logging_function(), which takes two arguments and uses them to log a bit of information.

In this case, the first argument to the function is the string to be logged, and the second argument is a string holding a path in a filesystem to a log file. The first string is printed, with a timestamp, to stdout and the log file.

Note: although yes input on the main() function would be wonderful, I'd love it if you gave thoughts on the function logging_function() itself.

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

#define EXIT_SUCCESS 0
#define EXIT_FAILURE 1

typedef struct tm tm;

int logging_function(char input_string[], char log_file_string[])
{
    /*Initializations.*/
    char output_string[32767];
    FILE* log_file_stream;
    time_t current_time_epoch_format;
    tm* current_time_calandar_format;

    /*Creating timestamp in output string, amending first argument to output string, and logging.*/
    if ((strlen(input_string) + 23) > 32767) return EXIT_FAILURE;
    if ((current_time_epoch_format = time(&current_time_epoch_format)) == -1) return EXIT_FAILURE;
    if ((current_time_calandar_format = localtime(&current_time_epoch_format)) == NULL) return EXIT_FAILURE;
    if (strftime(output_string, 23, "[%d-%m-%Y %H:%M:%S] ", current_time_calandar_format) != 22) return EXIT_FAILURE;
    if ((log_file_stream = fopen(log_file_string, "a")) == NULL) return EXIT_FAILURE;
    if (printf("%s\n", strcat(output_string, input_string)) < 1) return EXIT_FAILURE;
    if (fprintf(log_file_stream, "%s\n", output_string) < 1) return EXIT_FAILURE;
    if (fclose(log_file_stream) == EOF) return EXIT_FAILURE;
    return EXIT_SUCCESS;
}

int main(int argc, char** argv)
{
    /*Initializations.*/
    int EXIT_CODE;

    /*Print the returned integer from logging_function and exit.*/
    printf("%d\n", (EXIT_CODE = logging_function(argv[1], argv[2])));
    exit(EXIT_CODE);
}
share|improve this question

5 Answers 5

Not sure I like all those if statements lined up like that it makes it hard to read. I have tried to make it more readable (at the end). Weather it works is a matter of opinion so I would not say mine is any better just an alternative.

You also need to watch open()/close() on a file. You should always call close() even if the other functions fail. Otherwise there is potential for you to run out of file descriptors (though unlikely). In short it is a resorce leak and that can be a problem.

if ((log_file_stream = fopen(log_file_string, "a")) == NULL) return EXIT_FAILURE;
if (printf("%s\n", strcat(output_string, input_string)) < 1) {
    fclose(log_file_stream);  // Need to close the fstream
    return EXIT_FAILURE;
}
if (fprintf(log_file_stream, "%s\n", output_string) < 1) {
    fclose(log_file_stream);  // Need to close the fstream
    return EXIT_FAILURE;
}
if (fclose(log_file_stream) == EOF) return EXIT_FAILURE;

Your large number of if statements could be reduced (without reducing readability). I would not go to the extreme of putting it all in one if statement but you can definately combine a few.

if (   ((strlen(input_string) + 23) > 32767)
    || ((current_time_epoch_format = time(&current_time_epoch_format)) == -1)
    || ((current_time_calandar_format = localtime(&current_time_epoch_format)) == NULL)
    || (strftime(output_string, 23, "[%d-%m-%Y %H:%M:%S] ", current_time_calandar_format) != 22)
    || (strcat(output_string, input_string) != output_string)
   )
{
    return EXIT_FAILURE;
}

printf("%s\n", output_string); // If this fails I am still going to try and print
                               // to the log file.
                               // I will consider it success if the file printing
                               // works correctly even if this fails.

int result = 0;
if ((log_file_stream = fopen(log_file_string, "a")) != NULL)
{
    int printOk = fprintf(log_file_stream, "%s\n", output_string) == 1;
    int closeOK = (fclose(log_file_stream) == 0);

    result = printOk && closeOk;
}
return result != 0
          ? EXIT_SUCCESS
          : EXIT_FAILURE;
share|improve this answer

EXIT_SUCCESS and EXIT_FAILURE are defined by stdlib.h. I think you need not define them.

You can add the const qualifier to arguments of logging_function(). Those strings are not going to be modified.

You can call fprintf() twice, once with the strftime() output and then with input_string[] and avoid the strcat().

share|improve this answer
if ((log_file_stream = fopen(log_file_string, "a")) == NULL) return EXIT_FAILURE;
if (printf("%s\n", strcat(output_string, input_string)) < 1) return EXIT_FAILURE;
if (fprintf(log_file_stream, "%s\n", output_string) < 1) return EXIT_FAILURE;
if (fclose(log_file_stream) == EOF) return EXIT_FAILURE;

You leak log_file_stream if any of the lines between the fopen and the fclose fail.

There are several ways around this. In C (which does not have exceptions or RAII), the best way around this is to avoid early return statements when there's something to clean up. (Allocation, file handle, lock being held, etc.)

For example you could do (just meant as a quick sketch):

FILE *log_file_stream = NULL;
int status = 0;

if (!status && (log_file_stream = fopen(log_file_string, "a")) == NULL) status = EXIT_FAILURE;
if (!status && printf("%s\n", strcat(output_string, input_string)) < 1) status = EXIT_FAILURE;
if (!status && fprintf(log_file_stream, "%s\n", output_string) < 1) status = EXIT_FAILURE;

// Close the file if it is non-NULL.
//
if (log_file_stream)
   fclose(log_file_stream);

return status;

The key benefit is that this fclose call runs regardless of success or failure, similar to a finally block or a destructor in RAII. You can add more if (!status && /* ... */) blocks at will and you won't have to add redundant cleanup code.

Another way to do this is to use goto (if you are not morally opposed to it for silly reasons - it actually works well in C code for this specific purpose and you'll find it all over large projects, including *nix kernels.)

   FILE *log_file_stream = NULL;
   int status = 0;

   if ((log_file_stream = fopen(log_file_string, "a")) == NULL) goto error;
   if (printf("%s\n", strcat(output_string, input_string)) < 1) goto error;
   if (fprintf(log_file_stream, "%s\n", output_string) < 1) goto error;

   cleanup:
      // Close the file if it is non-NULL.
      //
      if (log_file_stream)
         fclose(log_file_stream);

      return status;

   error:
      status = EXIT_FAILURE;
      goto cleanup;

This gives you the benefit of not having to tediously check status again and again.


Update on 2nd reading

I don't like some of the repeated buffer size constants here, for example strlen(input_string) + 23 > 32767 ... how weird to write out 32767 instead of sizeof(output_string), which would allow you to change the size without having to change it in two places.

Taking strlen() (an O(n) operation) to perform the size check is also a bit odd. One common idiom in C code is to just optimistically start storing characters in the output buffer, and the moment you detect that you don't have enough space, that's when you return the error. (Think of an implementation of strncpy(dst, src, n) which has something like while (*src && n) { *dst++ = *src++; --n; } at its core.) Looking at your code though it doesn't seem like using that idiom directly will be convenient, but you can start using standard string functions in a way that will be more consistent with that. Maybe you should make the strftime call write into a stack-allocated temporary buffer (and again, please pass something derived from sizeof as the size parameter, rather than writing an integer constant size twice), then feed the results into snprintf(output_buffer, sizeof(output_buffer), /* ... */), which will take this approach.

share|improve this answer

Some note on your code as it stands:

  • unnecessary redefinition of EXIT_*
  • unnecessary typedef of struct tm
  • your parameters should be const
  • localtime is not thread safe - better to use localtime_r
  • printing of the message to the output_string is unnecessary. Better to print directly to stdout/file than to an intermediate buffer.
  • the buffer is also too large to safely put on the stack (in particular in an embedded system).
  • variable names are much too long
  • the code is difficult to read

Also, perhaps you should consider the design more carefully.

How will you use the function? Do you really want everything logged to a file and to stdout? These are activities that I would argue should be kept separate. At times you might want to log to a file. If no file is specified you might want to log to stdout instead; or even to stderr depending upon the nature of the messages. So one function to do everything seems a bad design.

Then there is the question of error handling. You have handled all sorts of errors without distinguishing between those which are likely and those which are not, and without considering what should be done if they occur. Let's look at what is likely to fail first:

  1. The most likely is that fopen() fails because the log file path doesn't exist or is protected.

  2. Once you've opened the file successfully, it is unlikely that closing it will fail unless you pass fclose() the wrong pointer or maybe the preceding fprintf() failed.

  3. fprintf(stdout) might fail if stdout is for some reason no longer open.

  4. fprintf(file) could fail if the filesystem has become full

  5. your calls to time(), localtime() or strftime() might conceivably fail but failure is really very unlikely.

How should you respond to each faiure?

For 1, 2, 3 and 4, propagate the error and use perror to print the error to stderr if and when appropriate. But note that failure 1 is most likely at the start of the program, so it makes sense to log a starting message from main() early on, with an exit on failure.

Do you need to handle failure 5 in the same way? It is a remote possibilty, most likely during testing when you have not got the strftime format correct. Unless you consider the fault serious, just omit the time in case of failure (but ensure that the subsequent code does not fail as a result).

For what it is worth, here is how I might rewrite your function. There is a log_msg function that logs to any stream and a log_to_file that logs to a file using log_msg.

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

static int
log_msg(FILE *f, const char *msg)
{
    time_t when = time(NULL);
    struct tm tm;
    char buf[20];
    localtime_r(&when, &tm);
    if (strftime(buf, sizeof buf, "%d-%m-%Y %H:%M:%S", &tm) == 0) {
        buf[0] = '\0';
    }
    if (fprintf(f, "[%s] %s\n", buf, msg) < 0) {
        return -1;
    }
    return 0;
}

static int
log_to_file(const char *msg, const char *filename)
{
    FILE *f = fopen(filename, "a");
    if (!f) {
        return -1;
    }
    int ret = log_msg(f, msg);
    if (fclose(f) != 0) {
        ret = -1;
    }
    return ret;
}

int main(int argc, char** argv)
{
    if (argc > 1) {
        const char *filename = argv[1];
        if (log_to_file("Hello, just testing", filename) < 0) {
            perror(filename);
        }
    } else {
        if (log_msg(stdout, "Hello, just testing") < 0) {
            perror("stdout");
        }
    }
    return 0;
}
share|improve this answer

For one... I don't see any check of argc == 3 to make sure the two arguments were actually passed on the command line.

As to others, well, I'd have to study it some more (and frankly I'm not so sure I want to look that hard right now).

share|improve this answer
    
I actually had a line for that, but I deleted it during testing. ._. Meh, its irrelavent as the main focus should be on the function logging_function. Here, lemme edit my post to reflect such. –  Draeton Dec 16 '12 at 0:40
    
Okay, also sorry for wasting your time. –  Draeton Dec 16 '12 at 0:47
    
Sorry, didn't meant to offend. If anything I couldn't find else obviously wrong and it'd take some work to find it (that's a good thing)! –  JayC Dec 16 '12 at 15:52

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.