Re: [PATCH 2/4] <nss_action.h>: New abstraction for combining NSS modules and NSS actions

[DJ Delorie <dj@redhat.com>]

"Carlos O'Donell" <carlos@redhat.com> writes:
>> +  /* The next element of the list.x */
>
> s/.x/./g

Done.

> Suggest:
>
> /* Lock covers the nss_actions list.  */

Changed.

>> +/* Returns true if the actions are equal (same module, same actions
>> +   aray).  */
>
> s/aray/array/g

Changed.

>> +
>> +/* Returns true if the COUNT actions at A and B are equal (according
>> +   to actions_equal above).  */
>
> Suggest:
>
> /* Returns true if COUNT actions at A and B are equal (according
>    to actions_equal above). Caller must ensure that A and B have
>    at least COUNT actions.  */

Went with:

/* Returns true if COUNT actions at A and B are equal (according to
   actions_equal above). Caller must ensure that either A or B have at
   least COUNT actions.  */

Like strcmp, a NULL entry will end the search early but safely (as a
mismatch).  The only case that causes problems is if both A and B are
equal, but both shorter than COUNT - then we iterate past the NULL
terminator record.

>> +/* Index in actions.  */
>
> Suggest:
>
> /* Value to add to first nss_status value to get zero.  */
> #define NSS_STATUS_BIAS 2
> /* Number of bits per lookup action.  */
> #define NSS_BPL 2
> #define NSS_BPL_MASK ((1 << NSS_BPL) - 1)
>
> /* Index in actions of an NSS status.
>    Note that in nss/nss.h the status starts at -2, and
>    we shift that up to zero by adding 2.  Thus for example
>    NSS_STATUS_TRYAGAIN, which is -2, would index into the
>    0th bit place as expected.  */

Changed.

>> +static inline int
>> +nss_actions_bits_index (enum nss_status status)
>> +{
>> +  return 2 * (2 + status);
>
> Suggest:
>
> return NSS_BPL * (NSS_STATUS_BIAS + status);

Changed.

>> +nss_action_get (const struct nss_action *action, enum nss_status status)
>> +{
>> +  return (action->action_bits >> nss_actions_bits_index (status)) & 3;
>
> Suggest:
>
> return (action->action_bits >> nss_actions_bits_index (status)) & NSS_BPL_MASK;

Changed.

>> +  int offset = nss_actions_bits_index (status);
>> +  unsigned int mask = 3 << offset;
>
> Use NSS_BPL_MASK and then "mask" makes sense.

Changed.

>> +  unsigned int bits = actions;
>> +  /* Produces 0b00 .. 00 AA AA AA AA AA.  */
>> +  action->action_bits = bits * 0x155;
>
> This needs to be rewritten to be expressed as a function of all the
> nss_status entries. That is to say you need to call nss_action_set
> for all status and actions. I dislike the above magic. It should be
> immediately readable and understandable that we set all the entries.

I went with this:

  unsigned int bits = actions & NSS_BPL_MASK;
  action->action_bits = (   bits
			 | (bits << (NSS_BPL * 1))
			 | (bits << (NSS_BPL * 2))
			 | (bits << (NSS_BPL * 3))
			 | (bits << (NSS_BPL * 4))
			 );

Sadly, GCC does not optimize this to a single multiplication, but at
least it's only called when parsing a new /etc/nsswitch.conf so
performance here isn't critical.