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

Tightening up the eventing publisher #3917

Merged
merged 1 commit into from
Nov 9, 2017

Conversation

obelisk
Copy link
Contributor

@obelisk obelisk commented Nov 9, 2017

Tighten up the event tapping event publisher code. This should hopefully make the code more stable and better equip it to handle edge cases.

@facebook-github-bot facebook-github-bot added the cla signed Automated label: Pull Request author has signed the osquery CLA label Nov 9, 2017
EventFactory::fire<EventTappingEventPublisher>(ec);
return event;
EventFactory::fire<EventTappingEventPublisher>(createEventContext());
/// If you change from listenOnly, return event or you will drop all events
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need /// just //

CFRunLoopRemoveSource(run_loop_, run_loop_source_, kCFRunLoopCommonModes);
CFRelease(run_loop_source_);
run_loop_source_ = nullptr;
if (runLoopSource_ != nullptr) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should use snake case for local member variables, but we don't enforce it.

kCGEventTapOptionListenOnly,
(1 << kCGEventKeyDown),
eventCallback,
NULL);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the NULL be a nullptr?

run_loop_ = nullptr;

if (eventTap_ != nullptr) {
CGEventTapEnable(eventTap_, false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should you do this "disable" before removing the source? Not sure, just curious?

@osqueryer
Copy link

🙅 The commit 6bf2afa (Job results: 2788) failed the code audit or documentation test.

@facebook-github-bot
Copy link

@obelisk has updated the pull request.

@osqueryer
Copy link

🙅 The commit 2dd3222 (Job results: 2789) failed the code audit or documentation test.

@facebook-github-bot
Copy link

@obelisk has updated the pull request.

@osqueryer
Copy link

🙅 The commit 5c8e5cf (Job results: 2790) failed the code audit or documentation test.

@facebook-github-bot
Copy link

@obelisk has updated the pull request.

@osqueryer
Copy link

👎 The commit 2ae754f (Job results: 3041) failed one or more tests (Windows).

@theopolis theopolis added events Related to osquery's evented tables or eventing subsystem macOS labels Nov 9, 2017
@theopolis theopolis merged commit c3a2171 into osquery:master Nov 9, 2017
@osqueryer
Copy link

👎 The commit 2ae754f (Job results: 3042) failed one or more tests (Windows).

trizt pushed a commit to trizt/osquery that referenced this pull request May 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla signed Automated label: Pull Request author has signed the osquery CLA events Related to osquery's evented tables or eventing subsystem macOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants