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

get rid of kdbprivate.h in bindings/tools #3162

Open
markus2330 opened this issue Nov 5, 2019 · 6 comments
Assignees

Comments

@markus2330
Copy link
Contributor

@markus2330 markus2330 commented Nov 5, 2019

  • src/tools/kdb/get.cpp:#include <kdbprivate.h>
  • src/tools/kdb/mount.cpp:#include <kdbprivate.h>
  • src/bindings/glib/gelektra-keyset.c:#include <kdbprivate.h>
  • src/bindings/glib/gelektra-key.c:#include <kdbprivate.h>
  • src/bindings/intercept/fs/intercept.c:#include <kdbprivate.h>

@manuelm can fix the glib bindings and @PhilippGackstatter the others?

@manuelm

This comment has been minimized.

Copy link
Contributor

@manuelm manuelm commented Nov 5, 2019

src/bindings/glib/gelektra-key.c

The glib binding is splitting object creation and initialization. That's why the glib binding is calling the private keyVInit after creating an empty key. I can remove the call, but every call to gelektra_key_new will in fact create two keys. One empty key that gets immediately freed again. And a second key with the actual arguments. It's not possible to work around this.

src/bindings/glib/gelektra-keyset.c

Doable. But one gelektra_keyset_new call will result in multiple reallocs. One for every key to append. It's calling ksResize internally.

Shall I proceed? I also don't quite understand why it's suddenly forbidden to call private methods for shipped bindings.

@markus2330

This comment has been minimized.

Copy link
Contributor Author

@markus2330 markus2330 commented Nov 6, 2019

One empty key that gets immediately freed again.

Why not provide an init without vaargs? And e.g. only set name in init? (EDIT)

If something like this is not possible, it is mostly about documentation, it should be easy to understand which parts of Elektra needs which internals and why.

There are several ways to fix this:

  1. we write a design decision about keyVInit (why it is needed for GLib)
  2. we document the need directly next to kdbprivate.h header inclusion
  3. we create a new header kdbbinding.h which includes ref counting, locking, and keyVInit (if needed)

If it is possible, however, it would be better to not have keyVInit as it is some additional maintenance burden. Every additional symbol in the core has some cost, both in maintenance as in library size (I would be interested how much the difference is, maybe you can find out in your thesis).

But one gelektra_keyset_new call will result in multiple reallocs.

Without benchmarks it is hard to say if this is a relevant issue or not.

I also don't quite understand why it's suddenly forbidden to call private methods for shipped bindings.

Distributions, afaik, always ship all binary package of one source package together. So the bindings will match the core version. But from the architecture/maintenance point of view it is much better if modules do not depend on internals of other modules.

manuelm added a commit to manuelm/libelektra that referenced this issue Nov 6, 2019
@manuelm

This comment has been minimized.

Copy link
Contributor

@manuelm manuelm commented Nov 6, 2019

If you want to drop va_args for glib, I expect you also drop va_args for libelektra itself. Then the bindings don't have to mimic and workaround the argument passing anymore.

I've created a PR which drops kdbprivate. Feel free to merge it or close it.

@manuelm manuelm referenced this issue Nov 6, 2019
@markus2330

This comment has been minimized.

Copy link
Contributor Author

@markus2330 markus2330 commented Nov 7, 2019

If you want to drop va_args for glib, I expect you also drop va_args for libelektra itself.

Unfortunately, this change is too big.

Then the bindings don't have to mimic and workaround the argument passing anymore.

Some bindings found quite elegant solutions for that. But of course, the va_args do not help the bindings but are rather a source for trouble.

@manuelm manuelm removed their assignment Nov 7, 2019
@PhilippGackstatter

This comment has been minimized.

Copy link
Contributor

@PhilippGackstatter PhilippGackstatter commented Nov 19, 2019

For the intercept plugin, I think I can't remove the dependency on kdbprivate.h.

The only need for the header is here

Plugin * check = elektraPluginOpen (node->exportType, modules, conf, key);
keySetString (key, node->value);
ksRewind (exportKS);
check->kdbSet (check, exportKS, key);

But removing the header results in pointer to incomplete class type is not allowed in line 264 and since the _Plugin type is defined in kdbprivate.h, I don't think we can remove it.

@markus2330

This comment has been minimized.

Copy link
Contributor Author

@markus2330 markus2330 commented Nov 20, 2019

Yes, we would need to replace this call with libinvoke. As the intercept plugin is very experimental it is maybe not worth the effort.

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