Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
dl1jbe opened this issue Oct 23, 2023 · 9 comments
Closed

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

dl1jbe opened this issue Oct 23, 2023 · 9 comments
Labels

Comments

@dl1jbe
Copy link
Member

dl1jbe commented Oct 23, 2023

There seems to be some problems in the switch to using getline() instead of fgets().

a) In some places the return value is checked against -1 (error) and in other cases against +1 (one char read).

b) Reading from man page if the function does NOT return -1 it means that one line got read from the file without errors. So checking for the length of the buffer being enough and checking for ENOMEM inside the while-loop seems unneeded. It should be checked after leaving the loop to decide between error and EOF condition.

As a further hint - the null pointer guard 'if (buffer) ..' before free() can be dropped as free works fine with null pointers.

@darvark Do you have a chance to work on it? I would suggest starting with one instance and using it as a template for the others later.

@dl1jbe dl1jbe added the BUG label Oct 23, 2023
@darvark
Copy link
Contributor

darvark commented Oct 23, 2023

Hi,
Yes I can work on it.

@dl1jbe
Copy link
Member Author

dl1jbe commented Oct 23, 2023

Good. I would suggest starting with the one in cabrillo_utils.c.

@zcsahok
Copy link
Member

zcsahok commented Oct 23, 2023

Replacing getline() was a longer process. In my comment #398 (comment) I proposed a generic line-by-line file reading function. This was then postponed to a later stage (#398 (comment)). I can also take it.

@dl1jbe
Copy link
Member Author

dl1jbe commented Oct 23, 2023

Yes I saw it from the mails and comments on the PRs.
I just did a check and saw the inconsistent checks of the return value and the wrong error handling.

Your idea of a generic processing function is a good one. I would suggest that we as a first step correct the problems and migrate afterwards. Let us give @darvark a chance to sort it it out with our help.

@darvark
Copy link
Contributor

darvark commented Oct 25, 2023

Now I'm preparing my club station for cqww, so I have not enought free time, please be patient, I'll start working on that just after cqww ssb.

@dl1jbe
Copy link
Member Author

dl1jbe commented Oct 25, 2023

Take your time and good luck in the contest.

@darvark
Copy link
Contributor

darvark commented Dec 22, 2023

Sorry for late reply, finally I found some time and I think I manage to unify all usage of getline(), but now I have problem with one test: run_rules, and as far as I know it uses python script run_tlf.py. As soon as I solve that issue I'll push changes for review

dl1jbe pushed a commit that referenced this issue Dec 27, 2023
* Fix getline usage (see #414)
@dl1jbe
Copy link
Member Author

dl1jbe commented Jan 7, 2024

Commit got merged. If no further comments, I will close the issue in coming week.

@zcsahok
Copy link
Member

zcsahok commented Jan 8, 2024

Fine with me. I'll look into the generic reading once I get to it.

@dl1jbe dl1jbe closed this as completed Jan 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants