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

Possible bug in kdbGet #2497

Open
kodebach opened this Issue Mar 14, 2019 · 5 comments

Comments

Projects
None yet
3 participants
@kodebach
Copy link
Contributor

kodebach commented Mar 14, 2019

@markus2330 I think there might be a bug in kdbGet related to global plugins:

/* Now do the real updating,
but not for bypassed keys in split->size-1 */
clearError (parentKey);
if (elektraGetDoUpdate (split, parentKey) == -1)
{
goto error;
}
else
{
copyError (parentKey, oldError);
}
/* Now post-process the updated keysets */
if (splitGet (split, parentKey, handle) == -1)
{
ELEKTRA_ADD_WARNING (108, parentKey, keyName (ksCurrent (ks)));
// continue, because sizes are already updated
}
/* We are finished, now just merge everything to returned */
ksClear (ks);
splitMerge (split, ks);
}
elektraGlobalGet (handle, ks, parentKey, POSTGETSTORAGE, INIT);
elektraGlobalGet (handle, ks, parentKey, POSTGETSTORAGE, MAXONCE);
elektraGlobalGet (handle, ks, parentKey, POSTGETSTORAGE, DEINIT);
ksRewind (ks);
keySetName (parentKey, keyName (initialParent));
splitUpdateFileName (split, handle, parentKey);

static int elektraGetDoUpdate (Split * split, Key * parentKey)
{
const int bypassedSplits = 1;
for (size_t i = 0; i < split->size - bypassedSplits; i++)
{
if (!test_bit (split->syncbits[i], SPLIT_FLAG_SYNC))
{
// skip it, update is not needed
continue;
}
Backend * backend = split->handles[i];
ksRewind (split->keysets[i]);
keySetName (parentKey, keyName (split->parents[i]));
keySetString (parentKey, keyString (split->parents[i]));
for (size_t p = 1; p < NR_OF_PLUGINS; ++p)
{
int ret = 0;
if (backend->getplugins[p] && backend->getplugins[p]->kdbGet)
{
ret = backend->getplugins[p]->kdbGet (backend->getplugins[p], split->keysets[i], parentKey);
}
if (ret == -1)
{
// Ohh, an error occurred,
// lets stop the process.
return -1;
}
}
}
return 0;
}

As you can see, elektraGetDoUpdate is called with parentKey, which is then modified by this function. Later on elektraGlobalGet is called with this parentKey. But if I understand correctly how global plugins should work, the key name should be reset to initalParent beforehand not afterwards (line 903).

@markus2330

This comment has been minimized.

Copy link
Contributor

markus2330 commented Mar 14, 2019

Thank you for reporting this problem!

@mpranj Is this something you will change within your PR?

@mpranj

This comment has been minimized.

Copy link
Member

mpranj commented Mar 14, 2019

This has not changed within my current cache PR.

The global plugins are not fully implemented, so there are definitely even more bugs. To my knowledge the parentKey should not even be initialParent, but actually the name should contain the global position. That way, the global plugins would know where they are currently called from.

@markus2330

This comment has been minimized.

Copy link
Contributor

markus2330 commented Mar 14, 2019

@mpranj Thank you for looking into it. We are already waiting for your implementation of the global plugins 👍

@kodebach do you have a concrete situation where this creates a problem? Is a fix urgent?

@kodebach

This comment has been minimized.

Copy link
Contributor Author

kodebach commented Mar 14, 2019

I was looking into it because of the options plugin. As discussed, ideally we would add a placement procgetstorage between getstorage and postgetstorage which would be used for plugins that write to the proc namespace.

This is also relevant for caching, because those plugin would always have to be executed (if kdbGet is called twice in the same process, we could skip some plugins, e.g. the options plugin).

The options plugin has to know the parent key of the current applications specification. Normally this is the parent key passed to kdbGet. This is why I was looking at what parent key is passed to global plugins, because normal plugins obviously get the parent of their mountpoint not what is passed to kdbGet.

@mpranj

This comment has been minimized.

Copy link
Member

mpranj commented Mar 14, 2019

Considering the current state of the implementation I am absolutely open to suggestions.

I think it might even make sense to put more information in the parentKey for global plugins. We can keep the initialParent as parentKey and add the position and other information as meta-keys.

Another option is to exchange additional information via the global keyset.

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.