#348 Review usage of fgets in file reading #398

darvark · 2023-07-09T14:08:58Z

Removed all occurrences of fgets and replaced with getline.
Additionally code re-formated with astyle

merge to master

… to getline

* make band limits configurable * allow showing out-of-band spots on bandmap

… to getline

test/rules/cqww/rules/cqww

zcsahok · 2023-08-03T19:21:33Z

Would it make sense to have a function that reads a file and return the lines as an array of pointers?
Just in order to split file reading and the actual processing.

darvark · 2023-08-03T19:44:34Z

Would it make sense to have a function that reads a file and return the lines as an array of pointers? Just in order to split file reading and the actual processing.

So you want to have new file dedicated only to read file and returning content of this file as array of char * ?

airween · 2023-08-04T06:07:47Z

Wondering if it wouldn't be too much to add checking if errno != ENOMEM to codition if (inputbuffer_len > 0). Theoretically getline still can read some amount of data before exceed memory so input buffer will be non zero.

IMHO checking the result of memory allocation according to different aspects can never be too much. :)

zcsahok · 2023-08-04T07:07:30Z

So you want to have new file dedicated only to read file and returning content of this file as array of char * ?

The typical usage pattern is

look for the file and open it
read it line-by-line in a loop
3. process each line

The loop in Step 2 is surely generic. So one could even make a function for it

int process_file(FILE *f, char *filename, char *(*func)(char *), char *comment_chars);

where func does the use case specific processing (Step 3). It returns a non-null message in case of an error.
Lines that start with one of the chars in comment_chars are treated as comments and skipped.

This would be even better than reading-in the whole file.

darvark · 2023-08-05T07:27:21Z

@zcsahok I think it worth to consider such functionality, but probably it's outside of this issue scope. We should/can discuss this in separate thread and create new issue for it. Such change would be bigger than just switch from fgets to getline.

This reverts commit 487f21e.

zcsahok · 2023-08-08T17:55:22Z

src/addmult.c

    }

    fclose(cfp);
-
+    if (s_inputbuffer > 0)


Comparing a pointer and 0. It works, but using the typical check

if (s_inputbuffer != NULL)

would be clearer.

zcsahok · 2023-08-08T17:59:15Z

src/checklogfile.c

    gchar *backupfile;
    gchar *cmd;
-    char buffer[200];
+    char *buffer = NULL;
+    size_t buffer_len = 200;


As buffer==NULL the assigned value is ignored by getline, so do not set it.
See https://elixir.bootlin.com/glibc/latest/source/libio/iogetdelim.c#L62

zcsahok · 2023-08-08T18:01:09Z

src/checklogfile.c

-    char buffer[160];
+    int read;
+    char *buffer = NULL;
+    size_t buffer_len = 160;


Do not set buffer_len.

zcsahok · 2023-08-08T18:01:43Z

src/checklogfile.c

    struct stat statbuf;
-    char inputbuffer[800];
+    char *inputbuffer = NULL;
+    size_t read_len = 160;


Do not set read_len.

zcsahok · 2023-08-08T18:02:13Z

src/dxcc.c

@@ -332,8 +333,10 @@ void dxcc_add(char *dxcc_line) {
 /** load cty database from filename */
 int load_ctydata(char *filename) {
    FILE *fd;
-    char buf[181] = "";
+    char *buf = NULL;
+    size_t buf_len = 181;


Do not set buf_len.

zcsahok · 2023-08-08T18:03:38Z

src/initial_exchange.c

 struct ie_list *make_ie_list(char *file) {

    FILE *fp;
-    char inputbuffer[91];
+    char *inputbuffer = NULL;
+    size_t inputbuffer_len = 91;


Do not set inputbuffer_len.

zcsahok · 2023-08-08T18:06:38Z

src/parse_logcfg.c

    g_strchomp(buffer);	/* drop trailing whitespace */

+    printf("%s\n", buffer);


Private logging?

zcsahok · 2023-08-08T18:08:38Z

src/readcabrillo.c

    struct cabrillo_desc *cabdesc;
    char input_logfile[24];
    char output_logfile[80], temp_logfile[80];
-    char logline[MAX_CABRILLO_LEN];


Remove unused MAX_CABRILLO_LEN from MAX_CABRILLO_LEN.

zcsahok · 2023-08-08T18:10:40Z

src/readqtccalls.c

 int qtcdirection = 0;
 int next_qtc_qso;
 int qsoflags_for_qtc[MAX_QSOS];
 int nr_qtcsent = 0;

 int readqtccalls() {
    int s = 0;
-    char inputbuffer[160];
+    char *inputbuffer = NULL;
+    size_t inputbuffer_len = 160;


Do not set length.

zcsahok · 2023-08-08T18:16:28Z

src/parse_logcfg.c

-	    country_list_raw = colon + 1;   // skip optional contest name
-	} else {
-	    country_list_raw = buffer;
+	if (buffer != NULL) {


buffer is never NULL at this point. In case the previous calloc fails, then the following g_strlcpy (line 687) will already cause a segmentation violation.

zcsahok · 2023-08-08T18:18:42Z

src/searchlog.c

@@ -747,7 +748,9 @@ int load_callmaster(void) {

    FILE *cfp;
    char *callmaster_location;
-    char s_inputbuffer[186] = "";
+    char *s_inputbuffer = NULL;
+    size_t s_inputbuffer_len = 186;


Do not set length.

zcsahok · 2023-08-08T18:21:36Z

src/writecabrillo.c

-
-    char buffer[160];
+    char *buffer = NULL;
+    size_t buffer_len = 160;


Do no set length.

zcsahok · 2023-08-08T18:27:47Z

My point was to avoid copy-pasting the same code to all affected source files. This can be done also later, no problem.

This PR contains 43 changed files including 3 file mode changes. On the other hand there are only 12 files in src/ that use fgets. The extra changes are due to astyle reformatting as this was not applied previously in a consistent manner. Additionally, the comments in some files like globalvars.h or qtcvars.h get mangled when reformatted.

So, I suggest to consolidate this PR as follows

take this as a successful prototyping and create a brand new branch from master head
copy the 12 .c files from this branch considering the review comments
apply astyle to the 12 files only, keep others unchanged
open a new PR

In the meantime I am preparing a PR to allow unconditional application of astyle
and also actually reindent the code in a controlled way.

Then we check the feasibility of introducing a generic file reading function.

Does this sound like a plan?

darvark · 2023-08-08T19:03:31Z

My point was to avoid copy-pasting the same code to all affected source files. This can be done also later, no problem.

This PR contains 43 changed files including 3 file mode changes. On the other hand there are only 12 files in src/ that use fgets. The extra changes are due to astyle reformatting as this was not applied previously in a consistent manner. Additionally, the comments in some files like globalvars.h or qtcvars.h get mangled when reformatted.

So, I suggest to consolidate this PR as follows
* take this as a successful prototyping and create a brand new branch from master head

* copy the 12 .c files from this branch considering the review comments

* apply astyle to the 12 files only, keep others unchanged

* open a new PR
In the meantime I am preparing a PR to allow unconditional application of astyle and also actually reindent the code in a controlled way.

Then we check the feasibility of introducing a generic file reading function.

Does this sound like a plan?

Yes, thank you for all your comments.

darvark and others added 30 commits June 23, 2023 20:43

Merge pull request #1 from Tlf/master

38e6af1

merge to master

Add keyword to start tlf in S&P mode

19621b8

Fix formating

b294490

Move setup operation mode to config file

f8a986a

Update manual and added test for new function

2d0318e

Fix tests

b3500cf

Post review fixes.

2e6e2f6

Update man pages

eddfbfd

Move operating_mode initalization to init_variables

5fe36d5

Update ToDo. Marked item finished

d10b061

Initial commit for Issue #348 - removing usage of fgets and switching…

a948f90

… to getline

restore rules content

974d984

restore execute bit

579e53d

Merge branch 'Tlf:master' into master

7a11229

Add keyword to start tlf in S&P mode

c5a5841

Fix formating

0ce1332

Move setup operation mode to config file

0a7ef03

Update manual and added test for new function

8cf8548

Fix tests

4e58d00

Post review fixes.

9bf132c

Update man pages

c0d8c67

Move operating_mode initalization to init_variables

42da065

Update ToDo. Marked item finished

1ec81e9

Make band limits configurable (#392)

cb7dce9

* make band limits configurable * allow showing out-of-band spots on bandmap

Initial commit for Issue #348 - removing usage of fgets and switching…

24b4d1a

… to getline

Initial commit for Issue #348 - removing usage of fgets and switching…

06d6617

… to getline

restore rules content

ae33beb

restore execute bit

7fb00c5

Initial commit for Issue #348 - removing usage of fgets and switching…

bc6ca57

… to getline

rebase

35947c7

zcsahok reviewed Aug 3, 2023

View reviewed changes

test/rules/cqww/rules/cqww Outdated Show resolved Hide resolved

restore test rules

0c90128

restore run_tlf.py permissions

67d076f

darvark added 2 commits August 4, 2023 21:54

fix astyle, add check to errno == ENOMEM

eb7e2ef

add missing include errno.h

ca794d3

darvark added 3 commits August 6, 2023 13:30

reformat and add check for memory

487f21e

Revert "reformat and add check for memory"

683adb4

This reverts commit 487f21e.

restore default formating with astyle

8b6e88c

zcsahok reviewed Aug 8, 2023

View reviewed changes

src/checklogfile.c

char buffer[160];

int read;

char *buffer = NULL;

size_t buffer_len = 160;

Copy link

Member

zcsahok Aug 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not set buffer_len.

zcsahok reviewed Aug 8, 2023

View reviewed changes

src/parse_logcfg.c

g_strchomp(buffer); /* drop trailing whitespace */

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

Copy link

Member

zcsahok Aug 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Private logging?

zcsahok reviewed Aug 8, 2023

View reviewed changes

src/writecabrillo.c

char buffer[160];

char *buffer = NULL;

size_t buffer_len = 160;

Copy link

Member

zcsahok Aug 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do no set length.

darvark closed this Aug 12, 2023

zcsahok mentioned this pull request Sep 10, 2023

Apply astyle #408

Merged

zcsahok mentioned this pull request Oct 23, 2023

Use of 'getline' (PR 403) needs more work #414

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#348 Review usage of fgets in file reading #398

#348 Review usage of fgets in file reading #398

darvark commented Jul 9, 2023

zcsahok commented Aug 3, 2023

darvark commented Aug 3, 2023

airween commented Aug 4, 2023

zcsahok commented Aug 4, 2023

darvark commented Aug 5, 2023

zcsahok Aug 8, 2023

zcsahok Aug 8, 2023

zcsahok Aug 8, 2023

zcsahok Aug 8, 2023

zcsahok Aug 8, 2023

zcsahok Aug 8, 2023

zcsahok Aug 8, 2023

zcsahok Aug 8, 2023

zcsahok Aug 8, 2023

zcsahok Aug 8, 2023

zcsahok Aug 8, 2023

zcsahok Aug 8, 2023

zcsahok commented Aug 8, 2023

darvark commented Aug 8, 2023

		g_strchomp(buffer); /* drop trailing whitespace */

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

#348 Review usage of fgets in file reading #398

#348 Review usage of fgets in file reading #398

Conversation

darvark commented Jul 9, 2023

zcsahok commented Aug 3, 2023

darvark commented Aug 3, 2023

airween commented Aug 4, 2023

zcsahok commented Aug 4, 2023

darvark commented Aug 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zcsahok commented Aug 8, 2023

darvark commented Aug 8, 2023