src/handlers.rs: explicitly handle INVALIDATE, SHUTDOWN, GETFD* #38
Conversation
flokli
force-pushed
the
invalidate-shutdown
branch
from
9c852c7
to
cf12116
Compare
|
I saw the @geofft @leifwalsh WDYT? |
|
Seems reasonable to me |
In both cases, we want to no-op. The client doesn't expect any response from nscd, but let's at least log a debug message on why we're no-op'ing here, to distinguish from the operations that are not yet implemented.
These will normally send an FD pointing to the internal cache structure, which clients use to look into the cache contents on their own. We don't cache, and we don't want clients to poke around in cache structures either. Luckily clients fall back to explicit queries if no FDs are sent over.
flokli
force-pushed
the
invalidate-shutdown
branch
from
de8e5af
to
da099f3
Compare
flokli
changed the title
There was a problem hiding this comment.
Hmm, not unreasonable but I'm curious about the motivation. I feel like I'd do it the other way around - all of these really should get no response but the rest of them are unimplemented and it might be worth noting if we get one.
Should we (also? instead?) log the unimplemented requests? The enum derives Debug so we can just log the actual request value.
(Also now that I'm looking at this it occurs to me that LASTREQ isn't actually a valid protocol request. :) )
| RequestType::GETHOSTBYADDR | ||
| | RequestType::GETHOSTBYADDRv6 | ||
| | RequestType::GETHOSTBYNAME | ||
| | RequestType::GETHOSTBYNAMEv6 | ||
| | RequestType::SHUTDOWN | ||
| | RequestType::GETSTAT |
There was a problem hiding this comment.
This returns statistics about the daemon - you can also mark this as will-not-implement.
There was a problem hiding this comment.
Why? I've been thinking about exposing some prometheus-style metrics on the number of requests, success rate and timing Information, by request type. That'd of course primarily be exposed via http, but if that getstat call is actually usable to expose some of these numbers, and there's some userspace tool to query and show, why not?
I was mostly concerned about having a distinction in code if something has not been implemented yet, or if we consciously decided it's okay to return no data, and userspace can cope with it.
I think we already
Can you elaborate? It's not something that ever happens to be sent out by the client, or it's something where we should always pretend to have not yet have had any previous request? |
|
Btw, can these workflows be changed to not require approval? It'd be nice to immediately get feedback on the various GH action checks... |
|
It's set to require approval for first time contributors, to prevent something like someone running a crypto miner using actions. Once we get you that CLA (#44) and merge some PRs from you you'll be good. |
|
Is this good to go? |
I like the gist of this: it clarifies which operations we'll never implement and makes it clear which remain to be implemented. We can log the unimplemented ones (at info? warn?) later. I'm gonna merge this for now, we can tweak later (e.g. doing something with |
Codecov ReportBase: 30.04% // Head: 29.22% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #38 +/- ##
==========================================
- Coverage 30.04% 29.22% -0.83%
==========================================
Files 5 5
Lines 213 219 +6
==========================================
Hits 64 64
- Misses 149 155 +6
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
In both cases, we want to no-op. The client doesn't expect any response from nscd, but let's at least log a debug message on why we're no-op'ing here, to distinguish from the operations that are not yet implemented.