Bad hack to improve the current way to do version checking
Needs ReviewPublic
Actions

Authored by aheinecke on Aug 28 2023, 7:25 AM.

Details

Reviewers: None

Summary

I don't want to commit this. This needs to extend KAboutData and properly read information from an ini file.

Test Plan

bla

Diff Detail

Repository

rLIBKLEO Libkleo

Lint

Lint Skipped

Unit

Unit Tests Skipped

Event Timeline

aheinecke requested review of this revision.Aug 28 2023, 7:25 AM

aheinecke created this revision.

aheinecke mentioned this in T6344: Kleopatra, GPG: AboutData ctor on Windows takes too long.

Why don't you create an MR on invent?

src/utils/gnupgrelease.cpp
21	Use `static const char *` instead, maybe with an additional constexpr? I don't really like macro defines in C++ code. And with `constexpr` they lost the "advantage" of compile-time string concatenation.
70	This really belongs in gpgme or at least in qgpgme. I'm trying hard to erradicate any manual calls of gpg executables from kleopatra and libkleo.
77	Please no lambdas that are longer than a couple of lines.
src/utils/gnupgrelease.h
31	`explicit` is superfluous here. Also: why not private?
61	Please use `std::unique_ptr` as we do everywhere else. With C++11 `QScopedPointer` is obsolete.

Thanks for the pointers. I just wanted to paste this as a differential so that it does not get lost in a stash somewhere on my system. I actually do not like this approach anymore. And do not want to commit it in this way. I would rather subclass or extend KAboutData with a verification option and then read from a QSettings style file instead of this Line based thing. For this I really think that an out of process call makes sense because the call is not to gpg but only to gpgv where we can just rely on the return code and even if we just patch it in having a GPGME dependency in KCoreAddons would be bad design IMO.

In one project in the past I added gpgv as a statically linked binary in a QResource, wrote it to a temporary file and then used that together with a PGP key which was also built into my binary as a QResource to verify stuff. This way I could "bootstrap" Authenticity checks based soely on the checksum / codesigning signature of my single binary. I am currently thinking of something similar here. So a user / external process would only need to manually verify that Kleopatra is actually signed with our Windows codesigning key and then Kleopatra can do a super deep integrity check of everything.

Btw. TBH I actually should read again about "explicit" in C++ I never really understood its necessity. :)

CarlSchwan added a subscriber: CarlSchwan.Aug 28 2023, 3:47 PM

CarlSchwan added inline comments.

src/utils/gnupgrelease.cpp
50	I love auto but with QString this is a big trap as auto will be a QStringBuilder and might reference temporary which can cause crashes
60
130	using new for unique_ptr is considered bad practice and can cause some issues in some cases
134–136

ikloecker added inline comments.Aug 28 2023, 6:03 PM

src/utils/gnupgrelease.cpp
50	Shouldn't be a problem for local variables, but it is a problem for "auto" as return type. It took me some time to understand why it crashed, when I ran into this problem. :-)
130	But "std::make_unique" is so much to type. And then it's wants those '<' and '>'. ;-) In any case, it's only a problem if a function takes two or more pointers. See for example https://herbsutter.com/gotw/_102/. The only advantage here would be to avoid "new", so that people who don't know about the exception safety problems with two or more pointers do not ever see a "new" in our code. But we don't catch any allocation-failure exceptions anyway. Kleopatra simply crashes and exception safety is the least of our problems.