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

Add some indexing to block_devices #8037

Merged
merged 2 commits into from
Sep 26, 2023

Conversation

directionless
Copy link
Member

@directionless directionless commented May 26, 2023

This extends the work originally done in #7209 to the block_devices table. And relates to #8033

With it, I hope to fix an o(n^2) bug.

As the underlying code doesn't support this on darwin, I opted to flag the spec as additional=True and not index=True

(Thanks @uptycs-rmack for talking through this with me)

This extends the work originally done in osquery#7209 to the `block_devices` table.

With it, I hope to fix an o(n^2) bug.

As the underlying code doesn't support this on darwin, I opted to
flag the spec as `additional=True` and not `index=True`
@directionless directionless requested review from a team as code owners May 26, 2023 19:02
@directionless directionless added this to the 5.10.0 milestone Jun 3, 2023
@mike-myers-tob mike-myers-tob added performance macOS Linux ready for review Pull requests that are ready to be reviewed by a maintainer labels Jun 20, 2023
Copy link
Member

@sharvilshah sharvilshah left a comment

Choose a reason for hiding this comment

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

built and ran it locally and on a VM, seems to run great! took a quick look at udev man pages, and I think we ought to check for null/failure return codes?

udev_enumerate_unref(enumerate);
if (runSelectAll) {
struct udev_enumerate* enumerate = udev_enumerate_new(udev);
udev_enumerate_add_match_subsystem(enumerate, "block");
Copy link
Member

Choose a reason for hiding this comment

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

do we need to check if enumerate is not a nullptr?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure? The prior code did not, and of the other places I see this call most don't check.

I can't really find something authoritative, but the code for udev_list_entry_foreach looks like it handles it.


std::map<std::string, std::string> lvm_lv2pv;
struct udev_list_entry *devices, *dev_list_entry;
devices = udev_enumerate_get_list_entry(enumerate);
Copy link
Member

Choose a reason for hiding this comment

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

perhaps we should check if devices is also not null before continuing

Copy link
Member Author

Choose a reason for hiding this comment

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

(as above)

@directionless directionless merged commit b1f3ff6 into osquery:master Sep 26, 2023
16 checks passed
@directionless directionless deleted the seph/block_devices branch September 26, 2023 13:15
directionless added a commit that referenced this pull request Oct 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Linux performance ready for review Pull requests that are ready to be reviewed by a maintainer virtual tables
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants