nscd -- step1

[david_crossfamilyweb.com]

Summary

This contains the cache cycle symbol changes.

Additionally I refactored the checks for a few things:

1. readability, variables are now used to store comparisons and then used
2. change the cycle breakout flow a bit, previous flow would just bypass the cache lookup and call "method()" which was coded in the cache provider to return NS_UNAVAIL. this just calls continue() to bypass this call entirely. (note this will have the advantage in the future where we can move that cache call INTO "method" for the caching module making the code more similar between methods.

3.moved the check for how to handle the exit code to AFTER the fallback code, it seems logical that even if we had to fallback (for say grouplist), that if we evaluated we should treat that the same as a non-fallback lookup (NOTFOUND, vs SUCCESS, etc).

4. keep track if there is even a cache entry in the srclist, if not, don't even bother trying to save to the cache server, and of course don't write to the cache server inside the cache server

[markj]

change the cycle breakout flow a bit, previous flow would just bypass the cache lookup and call "method()" which was coded in the cache provider to return NS_UNAVAIL. this just calls continue() to bypass this call entirely. (note this will have the advantage in the future where we can move that cache call INTO "method" for the caching module making the code more similar between methods.

I see that you broke out your nss diffs into quite a few different pieces - thank you.

The change referenced above is something I would move into a separate patch. It has no functional effect on its own and it makes control flow more indirect - when looking at the cache lookup call, I have to remember that an earlier check ensures that that code will be skipped if nsdispatch is running within nscd.

I don't insist on moving it to a separate diff this time, but in the future please try to avoid mixing different types of changes in a single patch. If the patch aims to fix a bug, don't also make control flow changes that ease future refactoring. If the patch makes cosmetic improvements and style fixes, please don't also include bug fixes, etc. etc..

lib/libc/net/nsdispatch.c

130

Don't we want the corresponding change in nscd to rename the symbol? That logically belongs in this patch.

645

We use bool types now when applicable. (You'll have to include stdbool.h for that.)

cache_start and is_nscd should be represented by bools.

702

cache_start is a confusing name. It means, "have we seen the cache as a provider at some point during this loop?" seen_cache or similar would be clearer I think.

771

I think the !is_nscd check is redundant, it is implied by cache_data_p != NULL.

[david_crossfamilyweb.com]

In D47143#1075209, @markj wrote:

change the cycle breakout flow a bit, previous flow would just bypass the cache lookup and call "method()" which was coded in the cache provider to return NS_UNAVAIL. this just calls continue() to bypass this call entirely. (note this will have the advantage in the future where we can move that cache call INTO "method" for the caching module making the code more similar between methods.

I see that you broke out your nss diffs into quite a few different pieces - thank you.

The change referenced above is something I would move into a separate patch. It has no functional effect on its own and it makes control flow more indirect - when looking at the cache lookup call, I have to remember that an earlier check ensures that that code will be skipped if nsdispatch is running within nscd.

I don't insist on moving it to a separate diff this time, but in the future please try to avoid mixing different types of changes in a single patch. If the patch aims to fix a bug, don't also make control flow changes that ease future refactoring. If the patch makes cosmetic improvements and style fixes, please don't also include bug fixes, etc. etc..

As for the indirect, yes. But I think it is cleaner. Also it should make later refactors less fragile; I cannot think of a reason this code shouldn't be moved into __nss_cache_handler in nscache.c, we already have a module system with a dispatcher, the if check duplicates and complicates the code. That will be a followup change (once I make sure it works), but it will rewrite this and cache_data_p.

lib/libc/net/nsdispatch.c

130

I tried to order these into file atomic pieces to minimize (or eliminate) cross file changes in my splitting; given that is doesn't even work currently (symbol not exported), I wasn't overly concerned. I can move

645

I will make this change, I wasn't sure the policy on using bools in freebsd code, I didn't wish to introduce "newer" styles, so I defaulted to int.

702

Will change, I am bad at naming and freely admit it!

771

I debated trying to simplify this, but I wasn't sure I had caught all of the possible loop state sets and resets, so I defaulted to overspecifying to be safe. I would still prefer it since it is more explicit and clear the intent and I think less fragile to refactors of the code that may change cache_data_p (I have at least one I want to make).

But I won't die on this hill either,

[david_crossfamilyweb.com]

All changes minus L771 one