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

common,mds,mgr,mon,osd: store event only if it's added #16312

Merged
merged 1 commit into from Aug 30, 2017

Conversation

tchaikov
Copy link
Contributor

otherwise

  • we will try to cancel it even it's never been added
  • we will keep a dangling pointer around. which is, well,
    scaring.
  • static analyzer will yell at us:
    Memory - illegal accesses (USE_AFTER_FREE)

Signed-off-by: Kefu Chai kchai@redhat.com

@liewegas
Copy link
Member

This seems right. I'm scared about the number of call sites that have to change, though, and that we'll introduce something subtle. :(

@dillaman
Copy link

Hold this PR for mimic?

@tchaikov
Copy link
Contributor Author

tchaikov commented Jul 14, 2017

@tchaikov
Copy link
Contributor Author

tchaikov commented Jul 14, 2017

i am fine either way. this fix mostly addresses the possible crashes after timer is shutdown, and this only happens when the daemon is being brought down.

@liewegas
Copy link
Member

@jdurgin what do you think?

@jdurgin
Copy link
Member

jdurgin commented Jul 14, 2017

Unless this is causing a bunch of noise in the tests, I'd prefer postponing to mimic

@tchaikov
Copy link
Contributor Author

okay, so far no noises is observed in tests caused by the problem mentioned above. let's hold this PR for mimic!

@tchaikov tchaikov force-pushed the wip-store-event-if-added branch 3 times, most recently from f754b9c to 2449b3a Compare August 20, 2017 06:13
otherwise
* we will try to cancel it even it's never been added
* we will keep a dangling pointer around. which is, well,
  scaring.
* static analyzer will yell at us:
  Memory - illegal accesses  (USE_AFTER_FREE)

Signed-off-by: Kefu Chai <kchai@redhat.com>
@tchaikov
Copy link
Contributor Author

@jdurgin @dillaman could you take a look at this PR again? as luminous has been forked, it's relatively safe to have this change in master now.

@tchaikov
Copy link
Contributor Author

@tchaikov tchaikov merged commit 8f49f7e into ceph:master Aug 30, 2017
@tchaikov tchaikov deleted the wip-store-event-if-added branch August 30, 2017 05:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants