@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/[email protected]>_______________________________________________
Rpm-maint mailing list
[email protected]
http://lists.rpm.org/mailman/listinfo/rpm-maint
