[U-Boot] [RFC, PATCH v4 01/16] hashtable: extend interfaces to handle entries with context

[AKASHI Takahiro]

On Fri, Jul 19, 2019 at 08:58:56AM +0200, Wolfgang Denk wrote:
> Dear AKASHI Takahiro,
> 
> In message <20190717082525.891-2-takahiro.akashi at linaro.org> you wrote:
> > The following interfaces are extended to accept an additional argument,
> > context:
> >     hsearch_r() -> hsearch_ext()
> >     hmatch_r() -> hmatch_ext()
> >     hdelete_r() -> hdelete_ext()
> >     hexport_r() -> hexport_ext()
> >     himport_r() -> himport_ext()
> 
> Please avoid such renaming; keep the exiting names as is, and just
> add new names (with descriptive names) where needed.

As I said in a reply to your previous comment, I have not renamed them.

> > --- a/include/search.h
> > +++ b/include/search.h
> > @@ -31,6 +31,7 @@ typedef enum {
> >  } ACTION;
> >  
> >  typedef struct entry {
> > +	unsigned int context;	/* opaque data for table operations */
> >  	const char *key;
> 
> Please keep the key the first entry.

Why?

> 
> > @@ -230,6 +238,14 @@ static inline int _compare_and_overwrite_entry(ENTRY item, ACTION action,
> >  {
> >  	if (htab->table[idx].used == hval
> >  	    && strcmp(item.key, htab->table[idx].entry.key) == 0) {
> > +		/* No duplicated keys allowed */
> > +		if (item.context != htab->table[idx].entry.context) {
> > +			debug("context mismatch %s\n", item.key);
> > +			__set_errno(EINVAL);
> > +			*retval = NULL;
> > +			return 0;
> > +		}
> 
> If I understand correctly what you are doing here, then this needs a
> different solution.  As is, the code just fails without a way to fix
> this.  And the reason is that the matching is only done on the key,
> while actually he context is important as well.

The intention here is to prevent a context of any entry from
being changed. I believe that this is a reasonable assumption
and restriction.

> The logical
> conclusion seems to be that the context must be part of the key.

Do you mean that an actual name of variable, for example, "foo"
should be, say, "uboot_foo"?

This will also make the implementation of env_save/load a bit ugly again.

> > +int hsearch_r(ENTRY item, ACTION action, ENTRY ** retval,
> > +	      struct hsearch_data *htab, int flag)
> > +{
> > +	ENTRY e;
> > +
> > +	e = item;
> > +	e.context = 0;
> 
> Please do not use '0' here; use the proper enum name.

I intentionally did so because I don't want to pollute "hash table"
functions with env-specific features so that hash functions will be
used for other purposes.
I admit that it is already polluted with flags though.

-Takahiro Akashi

> 
> Best regards,
> 
> Wolfgang Denk
> 
> -- 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
> In the beginning, I was made. I didn't ask to be made. No one consul-
> ted with me or considered my feelings  in  this  matter.  But  if  it
> brought  some  passing fancy to some lowly humans as they haphazardly
> pranced their way through life's mournful jungle, then so be it.
> - Marvin the Paranoid Android