Skip to content
This repository has been archived by the owner on Oct 15, 2024. It is now read-only.

segfault on missing gopts #2815

Closed
markus2330 opened this issue Jun 27, 2019 · 16 comments
Closed

segfault on missing gopts #2815

markus2330 opened this issue Jun 27, 2019 · 16 comments
Assignees
Milestone

Comments

@markus2330
Copy link
Contributor

as reported in #2772

Expected behavior: error, that a plugin cannot be found.

@kodebach
Copy link
Member

AFAIK this is fixed. If it happens again, please reopen.

@markus2330
Copy link
Contributor Author

@bauhaus93 can you try to reproduce? (Run lcdproc with the gopts plugin (re)moved)

@markus2330 markus2330 reopened this Sep 27, 2019
@bauhaus93
Copy link
Contributor

I get a segfault during loadConfiguration(), when trying to run lcdproc (or LCDd) with gopts disabled.

I installed elektra and lcdproc (from ElektraInitiative/lcdproc#7) using the guide in PR 7, then renamed /usr/local/lib/elektra/libelektra-gopts.so, so that gopts doesn't show up in kdb list.

@markus2330
Copy link
Contributor Author

Thank you for reproducing!

Can you maybe also post the stacktrace? (gdb binary-name core and then type bt)

@bauhaus93
Copy link
Contributor

The stacktrace for lcdproc:

(gdb) bt
#0  0x0000555555561849 in loadConfiguration ()
#1  0x0000555555557706 in main ()

@markus2330
Copy link
Contributor Author

It seems like the binary did not have debug symbols. Is your CMAKE_BUILD_TYPE RelWithDebInfo or Debug?

@kodebach
Copy link
Member

kodebach commented Oct 2, 2019

It seems like the binary did not have debug symbols.

I just tried it myself. The full stack-trace is:

#0  isHelpMode () at elektragen.c:52
#1  loadConfiguration (elektra=0x55555578d988 <elektra>, error=0x7fffffffdbe0) at elektragen.c:300
#2  0x000055555555f781 in process_config () at main.c:290
#3  0x000055555555fc0e in main (argc=<optimized out>, argv=<optimized out>) at main.c:209

So this is a different segfault then the one this issue was created for (isHelpMode didn't exist yet in June).

The fix should be simple, it is just a missing NULL check.

@markus2330
Copy link
Contributor Author

@kodebach Was the fix already included somewhere or is this still open?

@kodebach
Copy link
Member

kodebach commented Oct 7, 2019

AFAIK it is still open

@markus2330 markus2330 added this to the 0.9.1 milestone Oct 7, 2019
@markus2330
Copy link
Contributor Author

This is definitely something we should fix for the next release. What else is missing so that the elektrified LCDproc will work nicely?

@kodebach
Copy link
Member

kodebach commented Oct 7, 2019

I will create a PR for this and #3014.

What else is missing so that the elektrified LCDproc will work nicely?

The specload plugin is still marked as experimental, so it won't be built in the default config (-DPLUGINS=ALL;-EXPERIMENTAL). The documentation mentions the minimal set of plugins however.

Other than that, everything should work. Whether it works "nicely" or not, is something that @haraldg will have to decide. The biggest open problems are:

  • Using the ini plugin doesn't work, because of the multiline description metakeys. (I switched to ni for now, but its file format is not ideal).
  • Parts of the specifications are still commented out because of the max plugins issue.
  • Not all drivers are updated. The specification is based on the old example LCDd.conf; not all config keys for drivers are included. The updated drivers have a full specification.
  • Once all drivers are updated, the rather big specification for LCDd will probably slow down the server start-up time as well as any (cascading) kdb operation that calls the spec plugin on the relevant mountpoint. Dynamically enabling/disabling parts of the specification would be nice, although I don't see any way to do this right now.
  • Improving the spec plugin performance would also be nice, but again, I don't know how.
  • Various known issues with the kdb tool that make using it hard. Especially the fact that kdb set -N user <cascading-key>, may result in a broken configuration (because of the origvalue metadata).

@markus2330 markus2330 mentioned this issue Oct 7, 2019
6 tasks
@markus2330
Copy link
Contributor Author

Thank you!

The specload plugin is still marked as experimental

Any specific reason for this? What about remove the experimental flag?

The biggest open problems are

Thank you, I opened #3041

@kodebach
Copy link
Member

kodebach commented Oct 7, 2019

Any specific reason for this? What about remove the experimental flag?

It is unfinished and most of its features are not well tested in real-world environments. Other than that, I don't see a reason to keep it experimental.

@markus2330
Copy link
Contributor Author

Which are the best tested parts?

@kodebach
Copy link
Member

kodebach commented Oct 7, 2019

Loading from an embedded spec via a stdin/stdout pipe works and loading from a file directly also works. I tested those quite a lot by using LCDproc.

The part that is not so well tested is everything to do with changing the spec. I didn't even remember, that we allowed adding/modifying default and adding type. I can think of problems for both of them (handle the default value specially as unset and type problems if application calls kdbSet). As I said in my thesis, a lot of work needs to be put into this plugin to make it work well.

If we disallow all changes to default and type and leave only description and opt/help (and possibly add any other comment-style metakeys) the plugin should be very stable.

@markus2330
Copy link
Contributor Author

If we disallow all changes to default and type and leave only description and opt/help (and possibly add any other comment-style metakeys) the plugin should be very stable.

Yes, this change sounds very useful.

@kodebach kodebach mentioned this issue Oct 9, 2019
16 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants