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

Error Concept Improvements Metaissue #2572

Open
Piankero opened this Issue Apr 2, 2019 · 13 comments

Comments

Projects
None yet
4 participants
@Piankero
Copy link
Contributor

Piankero commented Apr 2, 2019

Summary of improvement TODOs from PR #2435

  • Write guidelines how to categorize errors.
  • Please put memory allocation errors in its own category so that they are easy to find in future.
  • Reduce the size of kdberror.h by refactoring (#2435 (comment))
  • Possibly add a macro ELEKTRA_ADD_SOLUTION which can serve as new method of supplying a solution (#2435 (comment))
  • Error code format changes for Warning/Errors (#2435 (comment))
  • Make a concept for the API (#2435 (review))
  • Errno should be handles by error framework
  • Checker tool to see for unused errors/warnings (#2435 (comment))
  • Optimize the dynamic dispatching in the kdberrors.h to not strcmp for the full code but for the first digits and then for the full string. This reduces the strcmp calls significantly if there are lots of error codes. This performance optimization though should be more relevant if we have much more error codes (currently only 8) (Idea from @kodebach )
  • Differentiate the error by the name rather than by the parameter, eg. ELEKTRA_ERROR_RESOURCE(...) instead of ELEKTRA_ERROR(RESOURCE,...). (#2589 (comment))
  • Change the warning key structure to not be limited to 100 entries but instead use arrays (#2589 (comment))
  • Incorporate an INFO level (#2610)
  • Add ElektraError accessor functions to highlevel API (@kodebach idea)

@Piankero Piankero added the enhancement label Apr 2, 2019

@markus2330

This comment has been minimized.

Copy link
Contributor

markus2330 commented Apr 3, 2019

Thank you for starting collecting the issues!

Please collect all problems/open discussions, even if you maybe will not solve them. E.g. "errno should be handled by error framework".

@Piankero

This comment has been minimized.

Copy link
Contributor Author

Piankero commented Apr 4, 2019

In #2435 we never talked about errno handling, I think it was another discussion which I do not remember unfortunately.
After going through the discussion I found another task but I think thats it. Do you remember anything else?

@markus2330

This comment has been minimized.

Copy link
Contributor

markus2330 commented Apr 4, 2019

I also did not remember "errno", I found it by skimming the discussion. I expected that you will look through everything properly, so that nothing gets lost.

@markus2330 markus2330 referenced this issue Apr 8, 2019

Open

New error codes #2589

3 of 9 tasks complete
@markus2330

This comment has been minimized.

Copy link
Contributor

markus2330 commented Apr 8, 2019

Very good that you finally improve our error system!

I just saw #373. It is a very old problem and still unfixed.

@Piankero

This comment has been minimized.

Copy link
Contributor Author

Piankero commented Apr 8, 2019

So #373 is basically a macro generation where the error type is integrated into the name if I have understand that correctly?

@markus2330

This comment has been minimized.

Copy link
Contributor

markus2330 commented Apr 8, 2019

#373 suggested that we introduce something like macro in the specification. Unfortunately, we never completely finished it, so we still have error codes/macros intermixed.

@markus2330

This comment has been minimized.

Copy link
Contributor

markus2330 commented Apr 9, 2019

As discussed in #2610 it would also make sense to have info messages which are not shown by default at all.

@Piankero

This comment has been minimized.

Copy link
Contributor Author

Piankero commented Apr 9, 2019

This is basically abusing the error message concept to also use for logging. I have never seen anything like that in a project before. There should be another concept or way of logging and saving cache hit messages. I can add it here as idea but it would really be against my intuition to incorporate it into the error concept

@Piankero

This comment has been minimized.

Copy link
Contributor Author

Piankero commented Apr 9, 2019

#373 suggested that we introduce something like macro in the specification. Unfortunately, we never completely finished it, so we still have error codes/macros intermixed.

At the moment we do not have them mixed anymore with the new concept. But I am still unsure what you actually want with #373. A macro entry which is mandatory?

@kodebach

This comment has been minimized.

Copy link
Contributor

kodebach commented Apr 9, 2019

This is basically abusing the error message concept to also use for logging

I agree with both of you. There is no good concept for info messages in Elektra, but integrating them into the error concept is also not a good solution. The problem IMO is that logging in Elektra is disabled by default and even if Elektra is compiled with -DENABLE_LOGGER=ON, the logging is filtered very heavily (only message from files below src/tools are shown), unless you maintain a separate version of log.c.

IMO we should enable logging to syslog (I think we are missing openlog and closelog) by default for message of level INFO and above, no matter were the originate from.

@mpranj

This comment has been minimized.

Copy link
Member

mpranj commented Apr 9, 2019

I agree, but it would be handy to be able to set the log level by configuration (without recompiling).

Edit: I also think that INFO is too verbose as a default setting.

@markus2330

This comment has been minimized.

Copy link
Contributor

markus2330 commented Apr 10, 2019

Yes, let us discuss this tomorrow.

I think we can easily make the logger more verbose on default. It is disabled by default anyway. And if someone passes -DENABLE_LOGGER=ON he/she obviously wants logs.

But this is actually another problem. The info messages I suggested above are messages that should go to the end user (like warnings+error) and not to the loggers.

@markus2330

This comment has been minimized.

Copy link
Contributor

markus2330 commented Apr 14, 2019

I added two points:

  • Write guidelines how to categorize errors.
  • Please put memory allocation errors in its own category so that they are easy to find in future. (They actually need special handling, the macro would need to do something else.)

The guideline should be done asap as there already seem to be inconsistent categorizations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.