Skip to content
This repository has been archived by the owner on Dec 7, 2023. It is now read-only.

fix possible dm snapshot leaks #381

Merged
merged 2 commits into from
Sep 3, 2019

Conversation

chanwit
Copy link
Member

@chanwit chanwit commented Aug 31, 2019

There are some defers that won't be called if an error occurred by ActivateSnapshot.
It's gonna defer to DeactivateSnapshot anyway even if ActivateSnapshot return an error.

Signed-off-by: Chanwit Kaewkasi chanwit@gmail.com

Signed-off-by: Chanwit Kaewkasi <chanwit@gmail.com>
@chanwit chanwit requested a review from twelho as a code owner August 31, 2019 13:30
@chanwit chanwit changed the title fix possible dm leak by re-arranging defers [WIP] fix possible dm leak by re-arranging defers Aug 31, 2019
@chanwit chanwit force-pushed the fix_possible_dm_leak branch 2 times, most recently from 1a37a62 to 3faa209 Compare August 31, 2019 16:09
@chanwit chanwit changed the title [WIP] fix possible dm leak by re-arranging defers [WIP] fix possible dm snapshot leaks Aug 31, 2019
Signed-off-by: Chanwit Kaewkasi <chanwit@gmail.com>
@chanwit
Copy link
Member Author

chanwit commented Sep 1, 2019

Also kindly ping @luxas

@chanwit
Copy link
Member Author

chanwit commented Sep 1, 2019

Also, could we have this for 0.6.x?

@chanwit chanwit changed the title [WIP] fix possible dm snapshot leaks fix possible dm snapshot leaks Sep 1, 2019
@chanwit
Copy link
Member Author

chanwit commented Sep 3, 2019

kindly ping @stealthybox
I think we need to confirm this change a bit to make sure that it won't break the current behavior.

@stealthybox
Copy link
Contributor

I was able to replicate these issues with the dm snapshots mentioned in #365 using a build on master.

This patch appears to fix it.
In general, this is an improvement in code quality.

Misplaced defer statements are a common source of error-handling issues.
Thanks for the thoughtful change 👍

@stealthybox stealthybox merged commit 70fb4a8 into weaveworks:master Sep 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants