On Sat, May 2, 2020 at 5:11 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
> On Fri, May 1, 2020 at 10:33 PM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> > On Tue, Apr 28, 2020 at 8:55 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
...
> > > diff --git a/security/selinux/ss/hashtab.h b/security/selinux/ss/hashtab.h
> > > index 31c11511fe10..4885234257d4 100644
> > > --- a/security/selinux/ss/hashtab.h
> > > +++ b/security/selinux/ss/hashtab.h
> > > @@ -13,6 +13,12 @@
> > >
> > > #define HASHTAB_MAX_NODES 0xffffffff
> > >
> > > +struct hashtab_key_params {
> > > + u32 (*hash)(const void *key); /* hash function */
> > > + int (*cmp)(const void *key1, const void *key2);
> > > + /* key comparison function */
> > > +};
> > > +
> > > struct hashtab_node {
> > > void *key;
> > > void *datum;
> > > @@ -23,10 +29,6 @@ struct hashtab {
> > > struct hashtab_node **htable; /* hash table */
> > > u32 size; /* number of slots in hash table */
> > > u32 nel; /* number of elements in hash table */
> > > - u32 (*hash_value)(struct hashtab *h, const void *key);
> > > - /* hash function */
> > > - int (*keycmp)(struct hashtab *h, const void *key1, const void *key2);
> > > - /* key comparison function */
> >
> > I don't like how you've split the hashing and comparison functions out
> > of the hashtab struct and into their own data structure with no
> > explicit linkage between the two. This is a bad design decision in my
> > opinion, and something we should try to avoid.
>
> In general, I would totally agree with you, but in this case this is
> crucial to avoid the indirect calls. Granted, the commit message fails
> to explain this and I need to rewrite it (and the callback separation
> probably deserves a comment in the code as well) ...
Write a proper commit description for the patches and resubmit them,
I'll take a closer look then. Generally when I see a poor commit
description and something odd in the code I stop reviewing the patch
and push back asking for a better explanation, which is what I did
here.
As a reminder, I have yet to reject a patch because the commit
description was too long, or too detailed.
--
paul moore
www.paul-moore.com
