[Rpm-maint] [rpm-software-management/rpm] Implement a separate lock for the keystore (PR #4123)

[Michal Domonkos]

@dmnks commented on this pull request.

A couple of notes inline. Some notes on the commit messages:

The "side-effects" note in the last commit's message is a bit confusing - it sounds as if the side effects (permitting key queries and imports during transaction) are undesired but it's what we're intentionally permitting here, no?

Also, the first commit's message seems to be missing "and" in the "Queries were always ..." paragraph which makes it a bit confusing, too 😅 

>  	ts->lock = rpmlockFree(ts->lock);
     }
 }
 
-rpmtxn rpmtxnBegin(rpmts ts, rpmtxnFlags flags)
+rpmtxn rpmtxnCreate(rpmts ts, rpmtxnFlags flags,

This should be declared static since it's only used in this file.

>  	if (t == NULL || *t == '\0' || *t == '%') {
 	    free(t);
-	    t = xstrdup(RPMLOCK_PATH);
+	    path = t = xstrdup(fallback_path);

Note that this changes the previous fallback logic subtly: If the `%_rpmlock_path` macro is undefined, we'll fall back to `RPMLOCK_PATH` *and* prepend the `rootDir` to it. Previously, we didn't do the latter and just used the host path.

I guess this new behavior is the correct one since we want to lock the target rpmdb, not the one on the host. But it still should be mentioned in the commit message.

> @@ -1127,6 +1129,22 @@ rpmtxn rpmtxnBegin(rpmts ts, rpmtxnFlags flags)
     return txn;
 }
 
+#define RPMLOCK_PATH LOCALSTATEDIR "/rpm/.rpm.lock"
+rpmtxn rpmtxnBegin(rpmts ts, rpmtxnFlags flags)
+{
+    static const char * const rpmlock_path_default = "%{?_rpmlock_path}";
+    return rpmtxnCreate(ts, flags, rpmlock_path_default, RPMLOCK_PATH,
+			_("transaction"), &(ts->lock));
+}
+
+#define KSLOCK_PATH LOCALSTATEDIR "/rpm/.keyring.lock"
+rpmtxn rpmkxnBegin(rpmts ts, rpmtxnFlags flags)
+{
+    static const char * const kslock_path_default = "%{?_keyring_lockpath}";

Cosmetic, but maybe `_keyringlock_path` would be more consistent with `_rpmlock_path`? 🤔 

> @@ -597,6 +597,7 @@ rpmts rpmtsFree(rpmts ts)
     }
     ts->rootDir = _free(ts->rootDir);
     rpmtsLockFree(ts);

I wonder if this call shouldn't be replaced with a plain assignment (like the keyring lock below)? Just for consistency...

-- 
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/4123#pullrequestreview-3966558782
You are receiving this because you are subscribed to this thread.

Message ID: <rpm-software-management/rpm/pull/4123/review/3966558782 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rpm.org/pipermail/rpm-maint/attachments/20260318/a6686678/attachment.htm>