Add --list and --delete commands to rpmkeys
#2921
Conversation
|
This feels a bit too quick and dirty. Comments welcome. |
tools/rpmkeys.c
Outdated
| ARGV_t gpgargs = argvNew(); | ||
| for (char * const * arg = args; *arg; arg++) { | ||
| if (strncmp(*arg, "gpg-pubkey-", 11)) { | ||
| char * gpgarg = rpmExpand("gpg-pubkey-", *arg, NULL); |
There was a problem hiding this comment.
Why the macro expand? That seems weird/wrong in this context.
tools/rpmkeys.c
Outdated
| @@ -75,7 +74,29 @@ int main(int argc, char *argv[]) | |||
| break; | |||
| /* XXX TODO: actually implement these... */ | |||
There was a problem hiding this comment.
Maybe delete this comment while at it? 😄 A fine example of why comments are so bad - they don't get updated along with the code.
|
I think I initially wanted to add a sane API for these and then have rpmkeys use that, and when that seemed painful I gave up. But the user wont care how it's achieved if it works in a meaningful manner. And, for the user this is far saner than 'rpmkeys --import -> rpm -q -> rpm --erase'. Two things:
|
tools/rpmkeys.c
Outdated
| case MODE_LISTKEY: | ||
| if (args != NULL) { | ||
| argerror(_("--list-keys does not take any arguments")); |
There was a problem hiding this comment.
I think it should (take optional arguments), actually. It could be useful for eg checking whether a particular key is imported, without having to query all and grep.
tools/rpmkeys.c
Outdated
| if (args != NULL) { | ||
| argerror(_("--list-keys does not take any arguments")); | ||
| } | ||
| ARGV_t query = argvSplitString("gpg-pubkey", " ", ARGV_NONE); |
There was a problem hiding this comment.
It's a bit creative way to initialize an argv... I think
ARGV_t query = NULL;
argvAdd(&query, "gpg-pubkey");
...would be a well worth the one extra line in obviousness 😄
|
Well, I stuck to the names that were in the code... but it turns out the "keys" in in the name of the utility already and we don't need to repeat that in the cli parameter. It also uses "--delete" while rpm itself uses "--erase". Not sure if that is good because it is a slightly different operation or bad because it is inconsistent. Opinions? |
I know. My not-so-thoroughly thought up names from 10+ years ago, and a fine example of why not to leave such half-assed pieces laying around in the codebase 😆
I'm quite sure I chose the names for familiarity from gpg, which has --list-keys and --delete-keys. (interestingly --import is just --import in gpg too). I always quite disliked the "erase" terminology in rpm anyhow. At least I associate erasing with emptying content (think fill data with zeros) as opposed to making the whole thing unreachable (think unlink file) |
tools/rpmkeys.c
Outdated
| @@ -42,6 +41,20 @@ static struct poptOption optionsTable[] = { | |||
| POPT_TABLEEND | |||
| }; | |||
|
|
|||
| static ARGV_t gpgkeyargs(ARGV_const_t args) { | |||
| ARGV_t gpgargs = argvNew(); | |||
There was a problem hiding this comment.
argvNew() is wholly unnecessary here (it almost always is), just initialize to NULl and argvAdd() and similar will alloc it as needed.
tools/rpmkeys.c
Outdated
| ARGV_t gpgargs = argvNew(); | ||
| for (char * const * arg = args; *arg; arg++) { | ||
| if (strncmp(*arg, "gpg-pubkey-", 11)) { | ||
| char * gpgarg = rpmExpand("gpg-pubkey-", *arg, NULL); |
There was a problem hiding this comment.
Again, why the macro expand? 'rpmkeys --delete %{somemacro}' seems like a weird use-case to support, I'd think for something like OpenPGP keys you'd rather always be explicit about the key.
There was a problem hiding this comment.
No idea why moving the code elsewhere didn't solve the issue...
This is a bit of a hack as it manipulates the parsed cli parameters to to the "right thing" and then calls rpmcliQuery and rpmErase.
dmnks
changed the title
--list-keys and --delete-key to rpmkeys
dmnks
changed the title
--list-keys and --delete-key to rpmkeys --list and --delete commands to rpmkeys
This is a bit of a hack as it manipulates the parsed cli parameters to
to the "right thing" and then calls rpmcliQuery and rpmErase.