[Rpm-maint] [rpm-software-management/rpm] Implement openpgp.cert.d based keystore (PR #3437)

[Michal Domonkos]

@dmnks commented on this pull request.

Overall LGTM, some comments in line.

>      return rc;
 }
 
+rpmRC keystore_openpgp_cert_d::import_key(rpmtxn txn, rpmPubkey key, int replace, rpmFlags flags)
+{
+    rpmRC rc = RPMRC_NOTFOUND;
+
+    if ((rc = acquire_write_lock(txn)) == RPMRC_OK) {

I'd suggest using a "bail out" conditional like in `delete_key()`, it avoids the extra indentation level and would be more consistent with the other method.

> +}
+
+rpmRC keystore_openpgp_cert_d::load_keys(rpmtxn txn, rpmKeyring keyring)
+{
+    return load_keys_from_glob(txn, keyring, "%{_keyringpath}/*/*");
+}
+
+rpmRC keystore_openpgp_cert_d::delete_key(rpmtxn txn, rpmPubkey key)
+{
+    rpmRC rc = RPMRC_NOTFOUND;
+
+    if (acquire_write_lock(txn) != RPMRC_OK)
+	return RPMRC_FAIL;
+
+    string fp = rpmPubkeyFingerprintAsHex(key);
+    string dir = fp.substr(0, 2);

This routine of obtaining the storage location (where the first two hex chars are the directory name) is a specific thing in this keystore implementation and it's currently duplicated in two places so maybe deserves a separate private method.

That said, we can always do that later, when there's the [third use](https://en.wikipedia.org/wiki/Rule_of_three_(computer_programming)), so feel free to ignore.

> @@ -275,6 +275,8 @@ static keystore *getKeystore(rpmts ts)
 	    ts->keystore = new keystore_fs();
 	} else if (rstreq(krtype, "rpmdb")) {
 	    ts->keystore = new keystore_rpmdb();
+	} else if (rstreq(krtype, "openpgp")) {
+	    ts->keystore = new keystore_openpgp_cert_d();

The class name seems a bit verbose to me, how about just `keystore_openpgp`? I suppose this would be the defacto standard openpgp keystore implementation so it doesn't have to be more specific than that.

> +rpmRC keystore_openpgp_cert_d::load_keys(rpmtxn txn, rpmKeyring keyring)
+{
+    return load_keys_from_glob(txn, keyring, "%{_keyringpath}/*/*");
+}
+
+rpmRC keystore_openpgp_cert_d::delete_key(rpmtxn txn, rpmPubkey key)
+{
+    rpmRC rc = RPMRC_NOTFOUND;
+
+    if (acquire_write_lock(txn) != RPMRC_OK)
+	return RPMRC_FAIL;
+
+    string fp = rpmPubkeyFingerprintAsHex(key);
+    string dir = fp.substr(0, 2);
+    string filename = fp.substr(2);
+    char * filepath = rpmGetPath(rpmtxnRootDir(txn), "%{_keyringpath}/", dir.c_str(), "/", filename.c_str(), NULL);

I see a mixed used of `rpmGetPath()` and `rpmGenPath()` in the keystore.cc module. Is there a reason for preferring one over the other in those places?

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

Message ID: <rpm-software-management/rpm/pull/3437/review/2427135821 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rpm.org/pipermail/rpm-maint/attachments/20241111/8c4ab146/attachment.html>