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

[Wolfgang Denk]

Dear Takahiro,

In message <20190719074421.GQ21948 at linaro.org> you wrote:
>
> > >     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.

Effectively this is what you are doing.  All teh names in the code
get changes, without need.

> > >  typedef struct entry {
> > > +	unsigned int context;	/* opaque data for table operations */
> > >  	const char *key;
> > 
> > Please keep the key the first entry.
>
> Why?

Because of the significance, for aligment, etc.

> > >  	    && 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.

This raises an interesting question - do have contexts separate
namespaces, i. e. can we have the variable "foo" both for U-Boot and
for UEFI, with different values?  Looking at the external storage
format, we could, as contets do not share storage location. Looking
at the internal storage, we could, if we make the "key" not only
consist of the name, but of a [name, context] pair.

This mightbe confusing for plain "printf", but this would not hurt, I
think.

I have no strong position on this.  Using a common name space is
maybe a bit easier code-wise, but not much.  In any case, the error
message shoulkd be fixed - as is, the end user would not understand
what the problem was.

> > 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"?

No, but the hash table shoulw eventually use a [name, context] pair
as key, at least when you implement separate name spaces.

> 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.

And using '0' is based on the assumption that U-Boot is the first
enum, which is not documented anywhere.  Please fix.

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
"The pathology is to want control, not that you ever get it,  because
of course you never do."                            - Gregory Bateson