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

locking key name in keyNew does not work #3151

Closed
markus2330 opened this issue Nov 3, 2019 · 12 comments · Fixed by #3179
Labels

Comments

@markus2330
Copy link
Contributor

@markus2330 markus2330 commented Nov 3, 2019

as @manuelm reported in #3133:

locking the key value in the constructor doesn't work. Because in https://github.com/ElektraInitiative/libelektra/blob/master/src/libs/elektra/keyhelpers.c the value gets set after the lock flag has been set (Line 303 vs 349).

@markus2330

This comment has been minimized.

Copy link
Contributor Author

@markus2330 markus2330 commented Nov 3, 2019

Why do you think it is a problem? Can you give a case where it doesn't work. Shouldn't the actions be executed as passed via va_args?

@markus2330

This comment has been minimized.

Copy link
Contributor Author

@markus2330 markus2330 commented Nov 3, 2019

Here the link to the lines:

/* flags without an argument */
case KEY_FLAGS:
flags |= va_arg (va, int); // FALLTHROUGH
case KEY_BINARY:
case KEY_LOCK_NAME:
case KEY_LOCK_VALUE:
case KEY_LOCK_META:
case KEY_CASCADING_NAME:
case KEY_META_NAME:
case KEY_EMPTY_NAME:
if (action != KEY_FLAGS) flags |= action;
if (test_bit (flags, KEY_BINARY)) keySetMeta (key, "binary", "");
if (test_bit (flags, KEY_LOCK_NAME)) elektraKeyLock (key, KEY_LOCK_NAME);
if (test_bit (flags, KEY_LOCK_VALUE)) elektraKeyLock (key, KEY_LOCK_VALUE);
if (test_bit (flags, KEY_LOCK_META)) elektraKeyLock (key, KEY_LOCK_META);
break;

@manuelm

This comment has been minimized.

Copy link
Contributor

@manuelm manuelm commented Nov 4, 2019

There are two issues here. The first one is a simple flow error in keyVInit: the name lock flag is set (line 303) before the actual key name is being set (line 349).

And the current test in tests/ctest/test_key.c is insufficient as it doesn't test check the initial name value. Otherwise someone else would have noticed that.

The second issue is about the lock flags in general. va_args does not work for bindings. It requires a compiler. So all bindings initially create the key with keyNew($name, KEY_FLAGS, $flags) and then modify the default properties according to the passed values. e.g. Key("user/foo", KEY_VALUE, "bar", KEY_END) is actually doing: k = KeyNew("user/foo", KEY_FLAGS, 0, KEY_END); keySetName(k, "bar"). Therefor if the empty key value or meta data gets initially locked they can't bet set anymore.

Since the KEY_LOCK_xxx-flags aren't part of the public API yet I'd recommend not using/removing them in/from keyNew. A dedicated function like elektraKeyLock is obviously fine.

@markus2330

This comment has been minimized.

Copy link
Contributor Author

@markus2330 markus2330 commented Nov 4, 2019

The idea is to make KEY_LOCK_xxx flags public. They can be used either in keyNew or keyLock.

@manuelm

This comment has been minimized.

Copy link
Contributor

@manuelm manuelm commented Nov 4, 2019

As explain I recommend to remove support for these flags in keyNew. Or be ok with the fact that using them in the constructor will throw/fail in the bindings.

@markus2330

This comment has been minimized.

Copy link
Contributor Author

@markus2330 markus2330 commented Nov 5, 2019

The first one is a simple flow error in keyVInit

This order is because of the other name options (KEY_CASCADING_NAME | KEY_META_NAME | KEY_EMPTY_NAME). But as we want to get rid of these options (by @kodebach in #3115) we could also fix this problem that actions are being done before setting the name.

As explain I recommend to remove support for these flags in keyNew. Or be ok with the fact that using them in the constructor will throw/fail in the bindings.

If you translate Key("user/foo", KEY_VALUE, "bar", KEY_END) to: k = KeyNew("user/foo", KEY_END); keySetName(k, "bar"); keyLock(LOCK_FLAGS) it would work. I never really liked keyVInit, why not simply provide a public API where you can do all the calls without any vaargs? If you insist of having a single keySetFlags API (instead of separated keyLock, keySetSync), I am open for discussion but I think having it separated is cleaner for anyone except bindings.

Let us go through the flags:

  • KEY_BINARY: not needed for bindings as they would call keySetBinary
  • KEY_LOCK_NAME: use keyLock instead
  • KEY_LOCK_VALUE: use keyLock instead
  • KEY_LOCK_META: use keyLock instead
  • KEY_CASCADING_NAME: is irrelevant with #3115
  • KEY_META_NAME: is irrelevant with #3115
  • KEY_EMPTY_NAME: is irrelevant with #3115

So why have this keyVInit and KEY_FLAGS at all? There was a reason to introduce it two years ago, but now it would be much cleaner to not have it?

@raphi011 @PhilippGackstatter Have you been in contact with these parts?

@manuelm

This comment has been minimized.

Copy link
Contributor

@manuelm manuelm commented Nov 5, 2019

If you translate Key("user/foo", KEY_VALUE, "bar", KEY_END) to: k = KeyNew("user/foo", KEY_END); keySetName(k, "bar"); keyLock(LOCK_FLAGS) it would work.

Yes. But this means that every time a new flag gets introduced this needs to be filtered and translated for every single binding. I'm already doing this for KEY_VALUE+KEY_BINARY and KEY_META. I'm assuming the other bindings authors are doing the same.
But I do think it's unnecessary for KEY_LOCK_xyz. Doing Key *k = KeyNew(...); keyLock(k, KEY_LOCK_NAME); is just as fine in C.

If you insist of having a single keySetFlags API (instead of separated keyLock, keySetSync), I am open for discussion but I think having it separated is cleaner for anyone except bindings.

I'm not insisting at all. Single or multiple functions are both fine for bindings wrapping. But vaargs are not.

@kodebach

This comment has been minimized.

Copy link
Contributor

@kodebach kodebach commented Nov 5, 2019

I never really liked keyVInit, why not simply provide a public API where you can do all the calls without any vaargs?

I think this is a good idea. If we remove the deprecated and the KEY_LOCK_* flags from keyNew, we are left with KEY_META, KEY_SIZE, KEY_BINARY, KEY_VALUE and KEY_FUNC. KEY_FUNC can be replaced with KEY_BINARY + KEY_SIZE.

If we now ignore binary keys for a moment, we only have KEY_META and basically that is what I proposed in #3112.

For the binary keys we could introduce a second set of functions:

Key * keyCreateBinary (const char * name, size_t size, const void * value, ...);
Key * keyVCreateBinary (const char * name, size_t size, const void * value, va_list va);

I know we decided that implementing keyCreate instead of keyNew is unrealistic, but I already have to update almost all keyNew calls in #3115, so it really isn't that much additional effort.

We should keep the possibility to create a key with metadata in a single function call to allow things like ksNew (2, keyNew(...), keyNew(...), KS_END).

@PhilippGackstatter

This comment has been minimized.

Copy link
Contributor

@PhilippGackstatter PhilippGackstatter commented Nov 5, 2019

@PhilippGackstatter Have you been in contact with these parts?

As Rust does not allow defining variadic functions, I am not touching the va_args parts, and instead require the user to call separate functions to set name, value, ...

I was thinking about providing a macro that would mirror keyNew but I found the builder pattern to be a good replacement for the variadic functions: let key = KeyBuilder::new("user/test/key").unwrap().meta("metakey", "metavalue").unwrap().build();

But I do think it's unnecessary for KEY_LOCK_xyz. Doing Key *k = KeyNew(...); keyLock(k, KEY_LOCK_NAME); is just as fine in C.

I agree. If the KEY_LOCK_* flags become public, I would prefer to only add a single lock method to the builder that takes an argument of what should be locked.

@markus2330

This comment has been minimized.

Copy link
Contributor Author

@markus2330 markus2330 commented Nov 5, 2019

But this means that every time a new flag gets introduced this needs to be filtered and translated for every single binding

Yes, this would be an implication. The 3 proposed lock flags, however, already allow to lock everything useful (why would someone want to lock ref counter or sync flag?), I think we can take the risk.

Single or multiple functions are both fine for bindings wrapping. But vaargs are not.

Yes, so keyLock would be fine.

so it really isn't that much additional effort.

It is not only about the points I wrote in #3112 but also about the timing. Even introducing keyLock now is risky: we need to update several bindings within a short time frame. Such a big change as suggested with keyCreate is really unrealistic now.

I agree. If the KEY_LOCK_* flags become public, I would prefer to only add a single lock method to the builder that takes an argument of what should be locked.

Yes, keyLock would provide that.

@markus2330

This comment has been minimized.

Copy link
Contributor Author

@markus2330 markus2330 commented Nov 5, 2019

Sorry, GitHub gave me a 500 error and suggested to refresh. For every refresh it still showed 500 but it seems like it actually posted.

@markus2330 markus2330 changed the title locking key value in keyNew does not work locking key name in keyNew does not work Nov 7, 2019
@markus2330

This comment has been minimized.

Copy link
Contributor Author

@markus2330 markus2330 commented Nov 7, 2019

Yes, so keyLock would be fine.

Seems like keyLock is not ready for prime, only name locking is implemented: #3170. So better to not make it public for 1.0.

@manuelm manuelm removed their assignment Nov 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.