On Fri, 2023-12-22 at 07:09 +0100, Tobias Hagelborn wrote: > From: Tobias Hagelborn <tobia...@axis.com> > > The purpose of the change is to never sign a package not created by > the build itself. > > sstate_create_package is refactored into Python and re-designed > to handle signing inside the function. Thus, the signing should never > apply to existing sstate packages. The function is therefore renamed > into sstate_create_and_sign_package. > The creation of the archive remains in a separate shellscript function. > > Co-authored-by: Peter Kjellerstedt <peter.kjellerst...@axis.com> > Signed-off-by: Tobias Hagelborn <tobias.hagelb...@axis.com> > --- > meta/classes-global/sstate.bbclass | 128 ++++++++++++++++++++--------- > 1 file changed, 87 insertions(+), 41 deletions(-)
This code is quite critical so review on any change here needs to be very careful and that is why I might seem to be a little hard on this patch. The fact this completely rewrites the critical section worries me a lot and means it isn't an easy to review change. We tried to keep this code simple deliberately due to the number of problems we've had with it in the past. You have to keep in mind it is one of the few shared directories we have where other builds can be reading/writing the files at the same time as us without locks. The fact that you have the force mode of operation is setting off alarm bells since we have seen build failures where an existing file is changed half way though a build. Another example of how this could re-introduce issues is that it looks like it will make it easy to break: https://git.yoctoproject.org/poky/commit/?id=47cc6288280bd38f42851d6423a5ffc95a46eea4 again. The code: + if not sstate_pkg.parent.is_dir(): + sstate_pkg.parent.mkdir(parents=True, exist_ok=True) will happen to work as the previous code should have already created the directories correctly so this line will do nothing. The fact it is there will make it all too easy for someone to break this in future though. When I added that I deliberately didn't rewrite other bits in python because I knew what the race issues were like. The directory creation and then moving files from the directory is also a concern as it is potentially going to be much slower. If you have something like the sstate on the autobuilder where the directory is NFS, creating temporary directories and moving files in/out of them is expensive compared to just renaming files. Another race that the force copy of the sig as written isn't as safe as you might think as it can still race with someone else copying the file at the same time. You probably need to move the file into place, then check that the resulting file is the one you placed there and if it is, only then move the sig in. I'm also not thrilled at the need for copies of the datastore. From a style standpoint, we haven't really used Path() in OE/bitbake so that is a bit out of place but I guess people are going to do this inevitably. The variable naming "verify_sig" isn't really true /clear when you're actually signing it either. Cheers, Richard
-=-=-=-=-=-=-=-=-=-=-=- Links: You receive all messages sent to this group. View/Reply Online (#192866): https://lists.openembedded.org/g/openembedded-core/message/192866 Mute This Topic: https://lists.openembedded.org/mt/103314293/21656 Group Owner: openembedded-core+ow...@lists.openembedded.org Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
