search

clicon / clixon

YANG-based toolchain including NETCONF and RESTCONF interfaces and an interactive CLI
http://www.clicon.org/
Other
216 stars 72 forks source link

Cleanup error handling #449

Closed pprindeville closed 12 months ago

pprindeville commented 1 year ago

There are calls to fprintf(stderr, ...); and perror(...); that only are seen if we're running in the foreground, with logging sent to stdout.

pprindeville commented 1 year ago

What are the steps to reproduce the CI checks from my command-line locally?

olofhagsand commented 1 year ago

What are the steps to reproduce the CI checks from my command-line locally?

"make test" more detailed here: https://github.com/clicon/clixon/blob/master/.github/workflows/ci.yml

pprindeville commented 1 year ago

Addressed all review issues.

pprindeville commented 1 year ago

Clixon does not use strlcpy intentionally and has no intention to do so at the moment. TI know there are pros and cons, but this was discused ~10 years ago, and I do not want to go there again. There are more important things to focus on.

Why do we have this?

AC_CHECK_FUNCS(inet_aton sigaction sigvec strlcpy strsep strndup alphasort versionsort strverscmp)

That went in as part of the Initial commit.

pprindeville commented 1 year ago

In general, these are utility functions exclusively for testing. Some dont even link to clixon, they are never run in a clixon binary. Therefore, error handling need not be integrated with clixon logging. Some are for convenience, some are not by design.

Some of the utils link against libclixon. What's the razor?

pprindeville commented 1 year ago

Review issues have been addressed. Please re-review.

olofhagsand commented 1 year ago

Clixon does not use strlcpy intentionally and has no intention to do so at the moment. TI know there are pros and cons, but this was discused ~10 years ago, and I do not want to go there again. There are more important things to focus on.

Why do we have this?

AC_CHECK_FUNCS(inet_aton sigaction sigvec strlcpy strsep strndup alphasort versionsort strverscmp)

That went in as part of the Initial commit.

That strlcpy should be removed. As for that commited line it is there for easing some embedded builds using buildroot or yocto, or even QNX, I forget.

olofhagsand commented 1 year ago

In general, these are utility functions exclusively for testing. Some dont even link to clixon, they are never run in a clixon binary. Therefore, error handling need not be integrated with clixon logging. Some are for convenience, some are not by design.

Some of the utils link against libclixon. What's the razor?

Sometimes it is convenient sometimes not. They should probably not be a part of the repo, but then the complete test part needs to move. We have had this discussion and decided to keep it to get immediate response on the CI instead of a synching with a separate test repo. However, it leads to somewhat double standards regarding coding rules, etc, for the rest of the src that is a part of the running code.

olofhagsand commented 1 year ago

Need to run this against second-line of tests, but that is now occupied by #452

pprindeville commented 1 year ago

Need to run this against second-line of tests, but that is now occupied by #452

And how did that go?

olofhagsand commented 1 year ago

Need to run this against second-line of tests, but that is now occupied by #452

And how did that go?

I will look at it next week