2
\$\begingroup\$

This is my first project in C and I wanted a more experienced person's insight on how I made the whole program. Just looking for feedback it works how I want it to. The Github is here. Thank you so much for the feedback!

#include <stdio.h>
#include <stdlib.h>
#include <dirent.h>
#include <regex.h>
#include <string.h>

#include "dbg.h"

#define MAX_WORD_SIZE 100

int get_file_char_count(FILE *f, char *filename);
int searchForKeyword(char *keyword, char *check_string);
int findLogFilename(char *filename);
int setIsOr(int argc, char *argv[]);
char** logFileNames();

int main(int argc, char *argv[])
{
    check((argc == 0), "Arguements required, please enter the search terms you want to look for in the log\nUse the -o to search for terms in one file.");

    unsigned int is_or = setIsOr(argc, argv); // set the and/or operator from -o flag
    FILE *fp;

    
    // Gets all file names in the log folder that end with .log
    char **logfile = logFileNames();
    check(logfile == NULL, "Failed to open directory and find log files!");
    
    int i = 0;
    while(logfile[i] != NULL)
    {

        char f_path[100];
        strcpy(f_path, "./log/");
        strcat(f_path, logfile[i]);


        fp = fopen(f_path, "r");
        int file_length = get_file_char_count(fp, logfile[i]);

        rewind(fp);

        char *file_contents = malloc(sizeof(char) * file_length); // dynamically allocated because of file size
        fgets(file_contents, file_length, fp); // write the stream to the variable

        if(is_or)
        {
            int j = 1; 
            for(j = 1; j < argc; j++)
            {
                int result = searchForKeyword(argv[j], file_contents);
                if(result == 0) {
                    printf("Match has been found in file: %s\n", logfile[i]);
                    break;
                }
            }
        } else {
            int j = 1;
            int found_all = 0; // bool to hold if all words were found in file
            for(j = 1; j < argc; j++)
            {
                int result = searchForKeyword(argv[j], file_contents);
                if(result == 0) {
                    found_all = 1;
                } else {
                    found_all = 0;
                    break;
                }
            }
            if(found_all)
                printf("Match has been found, for all words, in file: %s\n", logfile[i]);
        }
        

        free(file_contents);
        fclose(fp);

        i++;
    }

    free(logfile);

    return 0;

error:
    return -1;
}


int get_file_char_count(FILE *f, char *filename) 
{
    int returnVal = 0;
    char cursor = ' ';

    while(cursor != EOF)
    {
        cursor = fgetc(f);
        returnVal++;
    }

    return returnVal;
}

int searchForKeyword(char *keyword, char *check_string)
{
    regex_t regex;

    int reg_setup;
    int reg_return_result;

    reg_setup = regcomp(&regex, keyword, 0);
    reg_return_result = regexec(&regex, check_string, 0, NULL, 0);

    return reg_return_result;
}

int findLogFilename(char *filename)
{
    regex_t regex;

    int reg_setup;
    int reg_return_result;

    reg_setup = regcomp(&regex, ".log", 0);
    reg_return_result = regexec(&regex, filename, 0, NULL, 0);

    return reg_return_result;
}

int setIsOr(int argc, char *argv[]) 
{
    int i = 0;
    int ret_int = 0;

    for(i = 0; i < argc; i++)
    {
        if(strcmp(argv[i], "-o") == 0)
        {
            ret_int = 1; // sets the or flag to true
        }
    }

    return ret_int;
}


char** logFileNames() {
    DIR *d;
    struct dirent *dir;
    int isLog = 1;

    char** retArr;
    
    d = opendir("./log");
    check(d, "Could not open");

    int fileCount = 0;

    if(d)
    {
        while ((dir = readdir(d)) != NULL)
        {
            isLog = findLogFilename(dir->d_name);
            if(isLog == 0)
            {
                fileCount++;
            }
        }

        retArr = malloc(sizeof(*retArr) * fileCount);
        check_mem(retArr);

        rewinddir(d);

        int logFilePos = 0;
        while((dir = readdir(d)) != NULL)
        {
            isLog = findLogFilename(dir->d_name);
            if(isLog == 0)
            {
                retArr[logFilePos] = dir->d_name;
                logFilePos++;
            }
        }

        closedir(d);

        retArr[logFilePos + 1] = NULL;

        return retArr;

    }
    return NULL;
    

error:
    closedir(d);
    return NULL;
}
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Some things are missing from this question. First, what is this code supposed to do, and what do the program arguments mean? Secondly, what's in "dbg.h"? It would be very helpful to have the definitions available, for reviewers who wish to test the code. \$\endgroup\$ Nov 13 '21 at 10:36
  • \$\begingroup\$ There's a github link in the question that contains the source. dbg.h is the check macro, mostly. \$\endgroup\$
    – aghast
    Nov 13 '21 at 10:44
2
\$\begingroup\$

Pick a style

You write:

int get_file_char_count(FILE *f, char *filename);
int searchForKeyword(char *keyword, char *check_string);

You are mixing snake_case with camelCase names for your functions. I suggest that you stick with a single naming case for categories of names, so that all functions will use the same style. Pick one and use it everywhere.

Parse your args

Instead of writing a function to look for a single command line arg, write a function to parse and configure all the args. Ideally, you will use an argument-parsing library, even if it's just getopt.

Add a command line arg -d DIR to specify the log directory

This shouldn't be hard once you've picked an arg parsing library.

Don't throw away work you've already done

You write:

char **logfile = logFileNames();
...
while(logfile[i] != NULL)
{
    char f_path[100];
    strcpy(f_path, "./log/");
    strcat(f_path, logfile[i]);

But you have already computed the log file paths in your logFileNames function. Instead of repeating this work, have that function do it for you. Find a log file, construct the path, allocate memory using malloc or strdup, and pass that to the user. (This is in keeping with your functional-programming tag, as well.

Keep all the code in a single function at the same level of abstraction

You write:

    fp = fopen(f_path, "r");
    int file_length = get_file_char_count(fp, logfile[i]);
    rewind(fp);

    char *file_contents = malloc(sizeof(char) * file_length); // dynamically allocated because of file size
    fgets(file_contents, file_length, fp); // write the stream to the variable

I think you'll be disappointed with how that comes out. But ignoring that, you're doing operations at a low level (fopen, rewind) and at a high level (get_file_char_count). Don't do that.

Instead, write nice high-level functions to do all the work. If you want to know the size of a file, pass in the name and expect a size_t result to come back. Let the function do the fopen (if it needs to, which it does not).

Also, have a look at the stat() function. (Which may be some variant of _stat on Windows. Ask the Duck.)

Similar suggestions apply to getting the file's contents - once you have the contents, you don't need the file pointer, so leave that inside the function. In fact, getting the file size is part of getting the contents. So main should not know about the size at all:

#include <stdbool.h>

bool match_all_search_words = ... parse command line ...
const char ** search_words = ... parse command line ...

const char ** log_files = list_log_files(log_dir);

for (int i = 0; log_files[i] != NULL; ++i) {
    const char * log_file = log_files[i];
    const char * text = read_file_contents(log_file);

    if (match_all_search_words) {
        if (all_words_in_text(search_words, text)) {
            report_match(log_file);
        }
        else {
            report_failure(log_file);
        }
    }
    else {
        if (any_words_in_text(search_words, text)) {
            report_match(log_file);
        }
        else {
            report_failure(log_file);
        }
    }
\$\endgroup\$
1
\$\begingroup\$

Bug: failure to read entire file

fgets(file_contents, file_length, fp); only reads up to a line. Research fread().

Bug: insufficient allocation

fileCount does not account for the appended NULL.

// retArr = malloc(sizeof(*retArr) * fileCount);
retArr = malloc(sizeof *retArr * (fileCount + 1));
...
retArr[logFilePos + 1] = NULL;

Bug: 257 vs 256

int fgetc() returns 257 different values. Saving that into a char cursor will lose information. Use int cursor;

Avoid buffer overflow

strcat(f_path, logfile[i]) risks overflowing the buffer.

    char f_path[100];
    //strcpy(f_path, "./log/");
    //strcat(f_path, logfile[i]);

    // Pedantic alternate
    int len = snprintf(f_path, sizeof f_path, "%s%s", "./log/", logfile[i]);
    check(len >= 0 && len < sizeof f_path, "Log file too big");

File length + 1?

int get_file_char_count() returns the file length plus one. Not a problem as the allocation needed that +1 anyways for fgets() to append a null character.

File length may exceed INT_MAX. Perhaps use unsigned long long or uintmax_t?

Looking for option

Why check argv[0], the program name?

Why continue checking if found?

Alternate:

int setIsOr(int argc, char *argv[]) {
  for (int i = 1; i < argc; i++) {
    if (strcmp(argv[i], "-o") == 0) {
      return 1;
    }
  }
  return 0;
}

Alternate get_file_char_count()

For expediency, read a chunk at a time.

#define CHAR_COUNT_BUF_N 1024
intmax_t get_file_char_count(FILE *f) {
  if (f == NULL) {
    return -1;
  }
  rewind(f);  // Let us start at the beginning, clear error flag
  intmax_t char_count = 0;  
  char buf[CHAR_COUNT_BUF_N];
  size_t length;
  while((length = fread(buf, 1, CHAR_COUNT_BUF_N, f) > 0) {
    char_count += length;
  }
  if (ferror(f)) {
    char_count = -1;
  }
  rewind(f);
  return char_count;
}

Usage

intmax_t file_length = get_file_char_count(fp);
check(file_length >= 0 && file_length < SIZE_MAX, "File length trouble");

size_t file_length_size = (size_t) file_length; 
char *file_contents = malloc(file_length_size + 1);
check(file_contents, "Out of memory");

size_t length = fread(file_contents, 1, file_length_size, fp);
file_contents[file_length_size] = '\0';  // Append a 0 to form a string
check(length == file_length_size, "Read problem");
\$\endgroup\$
0
\$\begingroup\$

malloc() can return a null pointer

This code hides Undefined Behaviour:

    char *file_contents = malloc(sizeof(char) * file_length); // dynamically allocated because of file size
    fgets(file_contents, file_length, fp); // write the stream to the variable

We need to ensure that file_contents isn't a null pointer before we pass it into fgets.

Also, multiplying by sizeof (char) is pointless, as that cannot be anything other than 1 (sizeof gives results in units of char).

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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