0

I'm trying to get a program to parse a text file line by line for some data storage (I've tried binary file storage, but having mixed data types in the file seems to mess things up).

I'm trying to save a number of entries contained in hist[], which is an array of the history structure which basically contains a float (.value) and a time_t (.event_Time). Before saving these entries, the number of entries should be saved (currently an int).

So far I can write the file just fine using the following function:

void data_save(int node_Id, int sensor_Id, history *hist, int entries){
    FILE *file;
    char data_Dir[FILENAME_MAX] = "";
    char directory[FILENAME_MAX] = "";
    char fileName[FILENAME_MAX] = "";
    int length = 0;

    //define path of the file
    _getcwd(data_Dir, FILENAME_MAX);
    strcat(data_Dir, "\\Data");
    strcat(directory,"\\Node_");
    length = snprintf(NULL, 0,"%d",node_Id);
    char str1[length];
    sprintf(str1, "%d", node_Id);
    strcat(directory,str1);
    strcat(fileName,"\\Sensor_");
    length = snprintf(NULL, 0,"%d",sensor_Id);
    char str2[length];
    sprintf(str2, "%d", sensor_Id);
    strcat(fileName,str2);
    strcat(fileName,".txt");

    printf("%s\n", directory);
    printf("%s\n", fileName);

    //check if the Data directory exists, create it if not
    if (directory_exists(data_Dir) == false) {
        printf("Making directory\n");
        _mkdir(data_Dir);
    }
    strcat(data_Dir, directory);

    //check if the Node directory exists, create it if not
    if (directory_exists(data_Dir) == false) {
        printf("Making directory\n");
        _mkdir(data_Dir);
    }
    strcat(data_Dir, fileName);
    printf("%s\n", data_Dir);

    //open the file
    file = fopen(data_Dir, "w");
    if(file == NULL){
        printf("Error while opening file.\n");
        exit (1);
    }

    //Save the number of entries
    printf("Saving %d entries\n", entries);
    fprintf(file, "%d\n", entries);

    //Save each entry in the inverse chronological order
    //(ie. latest event first)
    for(int i=entries-1; i > -1; i--){
        fprintf(file, "%f %ld\n", hist[i].value, hist[i].event_Time);
    }

    fclose(file);

    free(data_Dir);
    free(directory);
    free(fileName);

    printf("Node %d, sensor %d: Data saved Successfully (%d Entries)\n", node_Id, sensor_Id, entries);
    return;
}

However, I am getting issues when trying to load the file I've just created using the following function:

history * data_load(int node_Id, int sensor_Id, int *entries){
    FILE *file;
    char data_Dir[FILENAME_MAX] = "";
    char directory[FILENAME_MAX] = "";
    char fileName[FILENAME_MAX] = "";
    int length = 0;
    int entries_Temp;
    int maxChar = 1000;
    char stream[maxChar];

    //define path of the file
    _getcwd(data_Dir, FILENAME_MAX);
    strcat(data_Dir, "\\Data");
    strcat(directory,"\\Node_");
    length = snprintf(NULL, 0,"%d",node_Id);
    char str1[length];
    sprintf(str1, "%d", node_Id);
    strcat(directory,str1);
    strcat(fileName,"\\Sensor_");
    length = snprintf(NULL, 0,"%d",sensor_Id);
    char str2[length];
    sprintf(str2, "%d", sensor_Id);
    strcat(fileName,str2);

    //check if the Data directory exists, exit if not
    if (directory_exists(data_Dir) == false) {
        printf("//Data does not exist\n");
        *entries = 0;
        return NULL;
    }
    strcat(data_Dir, directory);

    //check if the Node directory exists, exit if not
    if (directory_exists(data_Dir) == false) {
        printf("//Data//Node%d does not exist\n", node_Id);
        *entries = 0;
        return NULL;
    }
    strcat(data_Dir, fileName);

    printf("%s\n", data_Dir);

    //check if file exists (ie. there has been no previous
    //data for the given sensor) exit and return 0
    //existing entries
    file = fopen(data_Dir, "r");
    if(file!=NULL){
        printf("No file found for given sensor\n");
        *entries = 0;
        return NULL;
    }

    //Read the number of entries in the file
    printf("Reading number of entries\n");
    printf("%s", fgets(stream, sizeof(stream), file));
    printf("%s\n", stream);
    *entries = strtol(stream, NULL, 10);
    printf("Entries : %d\n", *entries);

    if(*entries > 100){
        printf("Entries is NOK\n");
        exit(1);
    }

    //create the array of structures containing the data
    printf("Creating the data array\n");
    history *hist = malloc(*entries * sizeof(history));

    //Read the data and copy it to the array
    //this has not been tackled yet

    printf("Closing file\n");
    fclose(file);

    printf("Freeing memory (filenames...)\n");
    free(data_Dir);
    free(directory);
    free(fileName);

    printf("Node %d, sensor %d: Data loaded Successfully (%d Entries)", node_Id, sensor_Id, *entries);
    return hist;
}

From what I can gather, it seems fgets returns NULL every time. I am unsure if the file is being read correctly, but it seems that the program manages to open the file as fopen returns non NULL. I also suspect that my first attempt at this using binary files might have failed for similar reasons, but due to the format, I couldn't check if the error was occuring during writing or reading of the file.

I'd like to get some insight on why fgets is failing. I'd also appreciate any guidance on better ways to handle saving and loading data from files, as I'm only a beginner in C and I'm pretty sure there's some more optimal ways of doing what I'm trying to achieve.

  • Have you checked the contents of the file? Do they look ok? – John Bollinger Jun 26 '20 at 21:38
  • 1
    [mcve], emphasis on _minmal_. Can you reproduce the error with a _smaller_ code segment, then repost, highlighting where exactly you are seeing the error? – ryyker Jun 26 '20 at 21:41
  • You should call fgets() with error/EOF checking and reporting: If ( NULL == fgets(...)){ if( ferror(){ perror( "fegts()" ); return NULL } fprintf( stderr, "fgets() EOF\n" ): return NULL } Also fgets() can return an empty string if no line feed before EOF, or an empty line "\n" if in the input. – David G. Pickett Jun 26 '20 at 22:09
  • What is your value for `FILENAME_MAX`? – chux - Reinstate Monica Jun 26 '20 at 22:31
  • @JohnBollinger, Yes, the contents of the file are exactly what they need to be, which was why I switched to a text file. – remi delamare Jun 26 '20 at 23:00
  • 1
    @chux-ReinstateMonica, It's 260, it's included in io.h if I remember correctly – remi delamare Jun 26 '20 at 23:00
  • @DavidG.Pickett, thanks for the tip, I'll look into it – remi delamare Jun 26 '20 at 23:01
  • It seems that there is no file error since ferror returns 0. However, fprintf(sterr, "fgets() EOF\n"); does not output anything to the console. It seems strange that fgets is getting EOF without reading any of the text in the file which is this: 2 1.500000 1593203411 2.500000 1593203411 – remi delamare Jun 26 '20 at 23:44

1 Answers1

2

At least these problems:

Off by 1.

With a short buffer, sprintf(str1, "%d", node_Id); is undefined behavior (UB) and rest of code is all suspect.

length = snprintf(NULL, 0,"%d",node_Id);
// char str1[length];
char str1[length + 1];
sprintf(str1, "%d", node_Id);

...
//char str2[length];
char str2[length+1];

Bad free

Do not call free() on something that lacks a matching *alloc().

//free(data_Dir);
//free(directory);
//free(fileName);

Suggest simplify string code.

E.g. filename

char fileName[FILENAME_MAX];
int length = snprintf(fileName, sizeof fileName, "%s%d%s",
    "\\Sensor_", sensor_Id, ".txt");
if (length < 0 || length >= sizeof fileName) {
  Handle_BufferTooSmall_Error();
}
else {
  printf("%s\n", fileName);
}
chux - Reinstate Monica
  • 143,097
  • 13
  • 135
  • 256
  • For robustness the first code should check `length > 0` , I have heard of some snprintf implementations failing and returning `-1` in certain circumstances (which the standard does permit) – M.M Jun 27 '20 at 01:36
  • @M.M Err, the first check is `length < 0`. This meets your concern and the spec's "Thus, the null-terminated output has been completely written if and only if the returned value is nonnegative and less than n." – chux - Reinstate Monica Jun 27 '20 at 01:58
  • @chux-ReinstateMonica by "the first code" I mean the first code snippet in your answer, which contains no length checks – M.M Jun 27 '20 at 04:30