On Fri, Jul 19, 2019 at 08:58:56AM +0200, Wolfgang Denk wrote: > Dear AKASHI Takahiro, > > In message <20190717082525.891-2-takahiro.aka...@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: w...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot