Create, read and print to stdout using C-CodePudding

This is my first time asking on Stack Overflow, i'll try my best to make a good question. Fell free to correct me if i miss relevant information or stuff like that.

I'm writting a little program that creates a simple options menu. My plan consist in very few steps:

Read file names from that macro #define file_dir "/home/me/dir"
Store that file names into names.txt.
I have to display the content of names.txt as options in my simple menu.

At the moment i was able to accomplish two of three steps but not so well i guess. I create 2 function to do these 2 jobs. create_file(), read_file(), respectively.

Now is where my question really begins:

Each function works ok when i execute isolated. If i call as it intended to be The second function read_file() instead to print the content of the file to stdout it rewrite the names.txt and put a "square" character at the end of the file.

My plan is to redirect the return of the read_file() to an array. So i can display as options in this bare bone menu.

Please help me understand. Why i can't use this two functions like that ? I know i am new to C and this program is far from be complete.

Here's my code:

#include <stdio.h>
#include <dirent.h>
#include <unistd.h>

#define my_dir "/home/me/dir"

  int roms_list;

  int create_list()
    {
    /* redirect stdout to a file */
    freopen("names.txt", "a ", stdout);

      /* open dir and print their content */
      DIR *dir;
      struct dirent *ent;
        if ((dir = opendir (nes_dir)) != NULL)
      {
            while ((ent = readdir (dir)) != NULL)
        {
                printf ("%s\n", ent->d_name);
          }
        }
      closedir(dir);
      close(names.txt);
    }

  int read_list()
  {
    FILE * list;
    char  ch;

    list = fopen("names.txt", "r ");

    if(NULL == list)
      {
        printf("file cant' be opened \n");
        return 1;
      }

    do
      {
        ch = fgetc(list);
        printf("%c", ch);
      }
    while (ch != EOF);

    fclose(list);
  }

int main()
{
  create_list();
  read_list();

  return 0;
}

CodePudding user response：

It looks like you are printing EOF. You should check if ch is EOF before printing that.

Also fgetc() returns int and convering the return value to char will prevent it from distinguishing EOF from one of valid byte, so you should use int instead of char for ch.

Instead of this:

    char  ch;

    /* ... */

    do
      {
        ch = fgetc(list);
        printf("%c", ch);
      }
    while (ch != EOF);

You should use:

    int  ch;

    /* ... */

    while ((ch = fgetc(list)) != EOF)
      {
        printf("%c", ch);
      }

Or:

    int  ch;

    /* ... */

    ch = fgetc(list);
    while (ch != EOF)
      {
        printf("%c", ch);
        ch = fgetc(list);
      }

CodePudding user response：

As MikeCAT points out, you attempt to printf("%c", ch); before checking ch != EOF resulting in attempting to print the int EOF values with the %c conversion specifier resulting in Undefined Behavior due to the mismatch in argument type and conversion specifier. ch must be type int to match the return type of fgetc() and to make a valid comparison with EOF.

If a conversion specification is invalid, the behavior is undefined. If any argument is not the correct type for the corresponding conversion specification, the behavior is undefined.

C11 Standard - 7.21.6.1(p9)

Additional Areas Where Your Code Needs Improvement

Your create_list() function is type int, but fails to return any value. Since create_list() can succeed or fail, it is imperative that the return type be able to communicate whether it succeeded or failed. Type int is fine, you can for example return 0; on a failure to read or on success, return the number of entries written to the file;
Your read_list() function is simply an output function that outputs the contents of the file written. While it can succeed or fail, it isn't critical to the continued operation of your program. Choosing type void for an output function is fine.
Do not hardcode file or directory names in functions. You shouldn't have to recompile your program just to read from a different directory or write to a different filename. Pass the directory to read and the filename to write as arguments to your program. That is what the arguments to main() are for, e.g. int main (int argc, char **argv). (or prompt the user to input both string values)
open your file in main() once and on successful open, pass a FILE* pointer for the open file stream to each of your functions as a parameter. You validate the open in main() because there is no need to call either function if fopen() fails.
pass the directory name to read to create_list() as a const char * parameter.
condition your call to read_list() on a successful return from create_list(). If create_list() fails, there is no need to call read_list().

Putting the improvements together, you could do something similar to the following:

#include <stdio.h>
#include <dirent.h>

/* returns 0 on failure, no. of files written on success */
int create_list (FILE *fp, const char *dname)
{
  /* open dir and print their content */
  DIR *dir;
  struct dirent *ent;
  int n = 0;              /* simple counter for no. of entries read */
  
  if ((dir = opendir (dname)) == NULL) {  /* return 0 on failure to open */
    return 0;
  }
  
  while ((ent = readdir (dir)) != NULL) {
    /* skip dot files */
    if ((ent->d_name[0] == '.' && !ent->d_name[1]) ||
        (ent->d_name[0] == '.' && ent->d_name[1] == '.')) {
      continue;
    }
    fprintf (fp, "%s\n", ent->d_name);
    n  ;                                  /* increment counter */
  }
  
  closedir(dir);
  
  return n;     /* return the number of enteries written */
}

/* read list can be type void - it simply outputs contents of file */
void read_list (FILE *fp)
{
  int  ch;    /* must be int  */

  while ((ch = fgetc (fp)) != EOF) {  /* read char, validate not EOF */
      putchar (ch);                   /* write to stdout */
  }
}


int main (int argc, char **argv) {
  
  char *dname, *fname;    /* dirname and filename pointers */
  int nfiles = 0;         /* no. of files written */
  FILE *fp = NULL;        /* file pointer */
  
  if (argc != 3) {  /* validate 2 arguments given (dirname filename) */
    fputs ("error: dirname and filename required\n"
           "usage: ./program \"/path/to/files\" \"filename\"\n", stderr);
    return 1;
  }
  dname = argv[1];        /* assign arguments to give descriptive names */
  fname = argv[2];        /* (you could just use argv[x], a name helps) */
  
  fp = fopen (fname, "w ");   /* open file for reading/writing */
  
  if (!fp) {  /* validate file open for reading/writing */
    perror ("file open failed");
    return 1;
  }
  
  /* validate create_list succeeds */
  if ((nfiles = create_list (fp, dname))) {
    printf ("%d files:\n\n", nfiles);     /* number of entries in file */
    rewind (fp);                          /* rewind file pointer */
    read_list (fp);                       /* read list */
  }
  
  if (fclose (fp) != 0) {             /* always validate close-after-write */
    perror ("fclose fp");
  }
}

Example Use/Output

You provide the directory to read as the first argument and the filename to write as the second. ./progname /path/to/read /file/to/write

A short example:

$ ./bin/dirlist_names ./km dat/dnames.txt
47 files:

startstop.o
kernelmod_hello1.c
.chardev.o.cmd
hello-4.o
.hello-2.mod.cmd
hello-2.mod

<snip>

hello-5.mod
.startstop.o.cmd
.hello-4.mod.cmd
chardev.mod
Makefile
hello-2.c