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 options: --cache-size, CacheSize #882

Merged
merged 1 commit into from May 17, 2023

Conversation

candrews
Copy link
Contributor

@candrews candrews commented Mar 31, 2023

  • Add new clamd and clamscan option --cache-size

This option allows you to set the number of entries the cache can store.

Additionally, introduce CacheSize as a clamd.conf
synonym for --cache-size.

Fixes #876

The default cache size, 65536, results in tree and nodes values of 256, which were the values in tree without this PR. So the default behavior remains unchanged.

@micahsnyder as discussed in Discord: https://discord.com/channels/636023333074370595/636023333074370597/1091052783161053184

@candrews
Copy link
Contributor Author

I'd love to also add a debug log message when nodes are evicted from the cache.

@micahsnyder (or anyone else), can you please point me to where in cacheset_add

static inline const char *cacheset_add(struct cache_set *cs, unsigned char *md5, size_t size, uint32_t recursion_level)
this log statement can be added? I'm having a very tough time grokking that function.

@candrews
Copy link
Contributor Author

I've set the nodes per tree and number of trees to the same value (the sqrt of the requested cache size, rounded up). I think this is reasonable.

libclamav/cache.c Fixed Show resolved Hide resolved
libclamav/cache.c Fixed Show resolved Hide resolved
libclamav/cache.c Fixed Show resolved Hide resolved
@candrews
Copy link
Contributor Author

candrews commented Apr 1, 2023

I have no idea why the "build-windows" check fails, it doesn't seem related to my changes.

The "clang-format / check (clamonacc, (c-thread-pool|fts|priv_fts))" check fails on formatting of files not changed in this PR.

All other checks pass.

Copy link
Contributor

@micahsnyder micahsnyder left a comment

Choose a reason for hiding this comment

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

Very nice PR! I only have a few things to request.

Also, our Jenkins build pipeline is broken right now because of an issue with switching from openssl 1.1 to 3.1. I want to make sure I get that fixed before I merge any PR's. But it of course does pass the mac/linux github actions pipelines and local test went well for me.

docs/man/clamd.conf.5.in Show resolved Hide resolved
libfreshclam/libfreshclam.c Show resolved Hide resolved
libclamav/cache.c Show resolved Hide resolved
libclamav/cache.c Show resolved Hide resolved
libclamav/cache.c Show resolved Hide resolved
@candrews
Copy link
Contributor Author

I've force pushed an update (I'm not sure if you prefer fixup commits or force push updates, so I took a guess) to this PR that addresses all comments.

@micahsnyder how's this PR look now?

* Add new clamd and clamscan option --cache-size

This option allows you to set the number of entries the cache can store.

Additionally, introduce CacheSize as a clamd.conf
synonym for --cache-size.

Fixes Cisco-Talos#867
@micahsnyder
Copy link
Contributor

It looks great! I just rebased with the latest commit from main. Will watch it go through our test pipelines next.

Copy link
Contributor

@micahsnyder micahsnyder left a comment

Choose a reason for hiding this comment

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

Testing went well. Merging!

@micahsnyder micahsnyder merged commit e70493c into Cisco-Talos:main May 17, 2023
22 of 24 checks passed
@micahsnyder
Copy link
Contributor

Augh! Just realized the issue number in the commit message was wrong. It actually fixes #876

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

Successfully merging this pull request may close these issues.

Cache doesn't work as well as expected
2 participants