Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix abort-on-error behaviour of transactions #2593

Merged
merged 2 commits into from
Apr 30, 2022

Conversation

smcv
Copy link
Contributor

@smcv smcv commented Apr 30, 2022

  • repo: Factor out _ostree_repo_auto_transaction_new()

    This will allow the direct allocation in
    ostree_repo_prepare_transaction() to be replaced with a call to this
    function, avoiding breaking encapsulation.

  • repo: Correctly initialize refcount of temporary transaction

    Previously, the reference count was left uninitialized as a result of
    bypassing the constructor, and the intended abort-on-error usually
    wouldn't have happened.

    Fixes: 8a9737a "repo/private: move OstreeRepoAutoTransaction to a boxed type"
    Resolves: OstreeRepoAutoTransaction used with uninitialized refcount in ostree_repo_prepare_transaction() #2592

This will allow the direct allocation in
ostree_repo_prepare_transaction() to be replaced with a call to this
function, avoiding breaking encapsulation.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Previously, the reference count was left uninitialized as a result of
bypassing the constructor, and the intended abort-on-error usually
wouldn't have happened.

Fixes: 8a9737a "repo/private: move OstreeRepoAutoTransaction to a boxed type"
Resolves: ostreedev#2592
Signed-off-by: Simon McVittie <smcv@collabora.com>
@openshift-ci
Copy link

openshift-ci bot commented Apr 30, 2022

Hi @smcv. Thanks for your PR.

I'm waiting for a ostreedev member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@smcv
Copy link
Contributor Author

smcv commented Apr 30, 2022

I think this is ready for review, but I'm marking it as a draft while I see whether it fixes the CI for flatpak/flatpak#4326.

@dbnicholson
Copy link
Member

/ok-to-test

@lucab
Copy link
Member

lucab commented Apr 30, 2022

/approve
/ok-to-test

@smcv
Copy link
Contributor Author

smcv commented Apr 30, 2022

I think this is right, but I won't know whether it fixed my failing CI for at least another hour (running the Flatpak test-suite under valgrind is slow). https://github.com/flatpak/flatpak/runs/6240039656?check_suite_focus=true

@smcv smcv marked this pull request as ready for review April 30, 2022 14:54
@smcv
Copy link
Contributor Author

smcv commented Apr 30, 2022

The Flatpak test-suite has finally passed under valgrind, and yes, a backport of these commits to 2022.2 does resolve the test failure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OstreeRepoAutoTransaction used with uninitialized refcount in ostree_repo_prepare_transaction()
3 participants