Safer transaction: add End() method and don't use as error#15
Merged
msteinert merged 15 commits intomsteinert:masterfrom Nov 30, 2023
Merged
Safer transaction: add End() method and don't use as error#15msteinert merged 15 commits intomsteinert:masterfrom
msteinert merged 15 commits intomsteinert:masterfrom
Conversation
This was referenced Oct 12, 2023
3v1n0
force-pushed
the
safer-transaction
branch
5 times, most recently
from
01beba4 to
65c9789
Compare
3v1n0
force-pushed
the
safer-transaction
branch
from
65c9789 to
085f3e9
Compare
3v1n0
force-pushed
the
safer-transaction
branch
2 times, most recently
from
05f5108 to
3fd81f1
Compare
Closed
3v1n0
force-pushed
the
safer-transaction
branch
4 times, most recently
from
34f1192 to
3e66e55
Compare
3v1n0
force-pushed
the
safer-transaction
branch
5 times, most recently
from
a0acd6e to
bdc316f
Compare
msteinert
requested changes
Nov 29, 2023
Owner
msteinert
left a comment
msteinert
left a comment
There was a problem hiding this comment.
Hi Marco, sorry for the delay in reviewing! I haven't been able to make much time for open source lately.
This looks really good, I'm excited for the 2.0 release. Let me know what you think. I'm willing to make some of the changes I suggested if you're feeling burned out on this project.
3v1n0
force-pushed
the
safer-transaction
branch
2 times, most recently
from
40c6d38 to
bb76ea3
Compare
msteinert
reviewed
Nov 29, 2023
3v1n0
force-pushed
the
safer-transaction
branch
from
bb76ea3 to
5cf914c
Compare
3v1n0
force-pushed
the
safer-transaction
branch
from
5cf914c to
378f970
Compare
And use them instead of C ones. Given that we have strings for them we can easily implement error interfaces for it too.
Use pam_debug.so to generate pam configurations at test time and check if the returned values expect the ones we want.
If the transaction fails during start, there's no way to get the error detail in a programmatic way, so let's wrap the pam.Error to allow more per-type checks.
…atus All the pam functions return an integer with the status of the operation so instead of duplicating the same code everywhere, that is quite error prone, use an helper function. It would have been nice to make this more dynamic, but cgo doesn't allow us to do much magic here. This is enough though.
Transactions save the status of each operation in a status field, however such field could be written concurrently by various operations, so we need to be sure that: - We always return the status for the current operation - We store the status in a atomic way so that other actions won't create write races In general, in a multi-thread operation one should not rely on Transaction.Error() to get info about the last operation.
While transaction does implement error, it's not a valid error implementer because it may have bogous values since it's not thread-safe and so we may read the result of Error() when it's into an invalid state As per this never return it as an error, while always return the Status unless when not available, where we still return pam.Error.
…more As per previous commit, Transaction can't be used anymore as an error value, but we instead we always return the status code.
A PAM transaction needs to be ended in order to release the associated resources, however this can't be sadly automated as the go finalizers run in goroutines and this could cause problems to modules that we load. In fact a module code may be called back during pam_end (to cleanup data for example) and the module code could not be thread safe. So let's make this more manual, but safer. The transaction status is still preserved in the transaction so end will be automatically called with the last-known status. Closes: msteinert#14
3v1n0
force-pushed
the
safer-transaction
branch
from
378f970 to
75e26f6
Compare
msteinert
reviewed
Nov 30, 2023
Check if a transaction is ended in in tests.
…nd Style types We redefined various PAM constant values for items, flags and style, but only few of them were marked as being Item's or Flag's. This caused go to just consider them as generic integers instead of the actual subtype.
3v1n0
force-pushed
the
safer-transaction
branch
from
75e26f6 to
067f634
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
As per discussions we had, Transaction is not fully thread safe and the fact it could be returned as an error makes things worse because its Error() value, that depends on the state could be changed while it's processed.
So:
End()method.These are both breaking changes, but would simplify #13 too.
Closes: #14
Note that this also includes #16