resolver: KeySet not updated if file removed

[markus2330]

The kdbGet optimization to do nothing if everything is unchanged seems to have a bug: If we remove the config file, the resolver will return 0. In the case of the optimization (every other config file unchanged or removed), the KeySet will not be updated and still hold the keys of the previous configuration file.

The change in the resolver is trivial:

diff --git a/src/plugins/resolver/resolver.c b/src/plugins/resolver/resolver.c
index e8def29..618a00c 100644
--- a/src/plugins/resolver/resolver.c
+++ b/src/plugins/resolver/resolver.c
@@ -60,6 +60,7 @@ static void resolverInit (resolverHandle * p, const char * path)
        p->dirmode = KDB_FILE_MODE | KDB_DIR_MODE;
        p->removalNeeded = 0;
        p->isMissing = 0;
+       p->hasExisted = 0;
        p->timeFix = 1;
 
        p->filename = 0;
@@ -461,11 +462,19 @@ int ELEKTRA_PLUGIN_FUNCTION (resolver, get) (Plugin * handle, KeySet * returned,
        /* Start file IO with stat() */
        if (stat (pk->filename, &buf) == -1)
        {
+               if (pk->hasExisted)
+               {
+                       pk->hasExisted = 0;
+                       // was not missing before!
+                       return /*return value?*/;
+               }
+
                // no file, so storage has no job
                errno = errnoSave;
                pk->mtime.tv_sec = 0;  // no file, so no time
                pk->mtime.tv_nsec = 0; // no file, so no time
                pk->isMissing = 1;
+
                return 0;
        }
        else
@@ -475,6 +484,7 @@ int ELEKTRA_PLUGIN_FUNCTION (resolver, get) (Plugin * handle, KeySet * returned,
                pk->gid = buf.st_gid;
                pk->uid = buf.st_uid;
                pk->isMissing = 0;
+               pk->hasExisted = 1;
        }
 
        /* Check if update needed */
diff --git a/src/plugins/resolver/resolver.h b/src/plugins/resolver/resolver.h
index 8aba3af..af50d94 100644
--- a/src/plugins/resolver/resolver.h
+++ b/src/plugins/resolver/resolver.h
@@ -31,6 +31,7 @@ struct _resolverHandle
        mode_t dirmode;                 ///< The mode to set for new directories
        unsigned int removalNeeded : 1; ///< Error on freshly created files need removal
        unsigned int isMissing : 1;     ///< when doing kdbGet(), no file was there
+       unsigned int hasExisted : 1;    ///< when doing kdbGet(), last time the file was there
        int timeFix;                    ///< time increment to use for fixing the time
 
        char * dirname;  ///< directory where real+temp file is

But we need some way to communicate to kdbGet that it does not need to execute the plugins (otherwise the resolver would fail), but it must update the KeySet.

A test case to reproduce the problem:

diff --git a/tests/kdb/testkdb_simple.cpp b/tests/kdb/testkdb_simple.cpp
index da5d5b4..4212a76 100644
--- a/tests/kdb/testkdb_simple.cpp
+++ b/tests/kdb/testkdb_simple.cpp
@@ -215,6 +215,39 @@ TEST_F (Simple, RemoveFile)
        ASSERT_EQ (stat (mp->systemConfigFile.c_str (), &buf), -1) << "found wrong file";
 }
 
+TEST_F (Simple, RemoveFileDoubleGet)
+{
+       using namespace kdb;
+       KDB kdb;
+       KeySet ks;
+       kdb.get (ks, testRoot);
+       ks.append (Key ("system" + testRoot + "remove", KEY_END));
+       ASSERT_EQ (ks.size (), 1) << "could not append key\n" << ks;
+       kdb.set (ks, testRoot);
+       ASSERT_EQ (ks.size (), 1) << "key gone after kdb.set?\n" << ks;
+
+       struct stat buf;
+       ASSERT_EQ (stat (mp->systemConfigFile.c_str (), &buf), 0) << "found no file";
+
+       Key parentKey;
+       KDB kdb2;
+       kdb2.open (parentKey);
+
+       KeySet ks2;
+       kdb2.get (ks2, testRoot);
+       ASSERT_EQ (ks2.size (), 1) << "should get one key\n" << ks;
+       ks2.clear ();
+       ASSERT_EQ (ks2.size (), 0) << "keyset should be empty after clearing it\n" << ks;
+       kdb2.set (ks2, testRoot); // file is now removed
+
+       ASSERT_EQ (stat (mp->systemConfigFile.c_str (), &buf), -1) << "found wrong file (should be removed)";
+
+       ASSERT_EQ (ks.size (), 1) << "key is gone?\n" << ks;
+       kdb.get (ks, testRoot);
+       ASSERT_EQ (ks.size (), 0) << "key should now be gone (updated)\n" << ks;
+
+}
+
 
 TEST_F (Simple, DISABLED_GetNothingEmpty)
 {

[stale[bot]]

I mark this issue stale as it did not have any activity for one year. I'll close it in two weeks if no further activity occurs. If you want it to be alive again, ping the issue by writing a message here or create a new issue with the remainder of this issue. Thank you for your contributions :sparkling_heart:

[stale[bot]]

I mark this issue stale as it did not have any activity for one year. I'll close it in two weeks if no further activity occurs. If you want it to be alive again, ping the issue by writing a message here or create a new issue with the remainder of this issue. Thank you for your contributions :sparkling_heart:

[markus2330]

@kodebach is this something that can be fixed more easily within #3693?

[kodebach]

I wouldn't say it is easier in any way, but it could certainly be combined.

[kodebach]

@markus2330 This may be an issue for FLOSS, especially if the patch provided by you actually solves everything.

[markus2330]

@kodebach It does not solve everything, we also need changes in the backend plugin. Can you please describe here how you would do the necessary changes in the backend plugin?

[kodebach]

How would I know what needs to be fixed? Without trying to implement the fix myself, I have no idea what needs to be done.

If I had to guess I'd say either the fix in resolver is enough, or we do need changes in libelektra-kdb (might be covered by COW internal cache). That we need changes in backend seems rather unlikely to me. But since you seem to have that impression, maybe share why you think it needs changes...

[markus2330]

The problem is explained above: without a file, the keys in the keysets won't get removed on repeated use of kdbGet. We probably need either a new return value for resolvers or some other means of communication between resolver <-> backend to handle this situation correctly. I only ask about advice how to design the plugin API for this case, as you know the backend <-> resolver communication best.

[kodebach]

The backend plugin just passes through the return value. So as I suspected, we need changes in libelektra-kdb to handle a new result for the resolver phase.

Also the problem is not exclusive to file-based backends AFAICT. It seems to be a general issue with the API. If the backend in the first kdbGet says "there is data here", there is no way for it to say "the data is gone now" in a later kdbGet. At least not without going through all the phases after resolver and trying to load the nonexistent data.