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
