| Message ID | 20190717082525.891-2-takahiro.akashi@linaro.org |
|---|---|
| State | RFC |
| Delegated to: | Heinrich Schuchardt |
| Headers | show |
| Series | efi_loader: non-volatile variables support | expand |
Dear AKASHI Takahiro, In message <20190717082525.891-2-takahiro.akashi@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. > --- 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. > @@ -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 logical conclusion seems to be that the context must be part of the key. > +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. Best regards, Wolfgang Denk
On Fri, Jul 19, 2019 at 08:58:56AM +0200, Wolfgang Denk wrote: > Dear AKASHI Takahiro, > > In message <20190717082525.891-2-takahiro.akashi@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@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
Dear Takahiro, In message <20190719074421.GQ21948@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
diff --git a/include/search.h b/include/search.h index 5d07b49073cc..9cece695d726 100644 --- 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; char *data; int (*callback)(const char *name, const char *value, enum env_op op, @@ -77,6 +78,8 @@ extern void hdestroy_r(struct hsearch_data *__htab); * */ extern int hsearch_r(ENTRY __item, ACTION __action, ENTRY ** __retval, struct hsearch_data *__htab, int __flag); +extern int hsearch_ext(ENTRY __item, ACTION __action, ENTRY ** __retval, + struct hsearch_data *__htab, int __flag); /* * Search for an entry matching "__match". Otherwise, Same semantics @@ -84,14 +87,22 @@ extern int hsearch_r(ENTRY __item, ACTION __action, ENTRY ** __retval, */ extern int hmatch_r(const char *__match, int __last_idx, ENTRY ** __retval, struct hsearch_data *__htab); +extern int hmatch_ext(const char *__match, int __last_idx, ENTRY ** __retval, + struct hsearch_data *__htab, unsigned int ctx); /* Search and delete entry matching "__key" in internal hash table. */ extern int hdelete_r(const char *__key, struct hsearch_data *__htab, int __flag); +extern int hdelete_ext(const char *__key, struct hsearch_data *__htab, + unsigned int ctx, int __flag); extern ssize_t hexport_r(struct hsearch_data *__htab, const char __sep, int __flag, char **__resp, size_t __size, int argc, char * const argv[]); +extern ssize_t hexport_ext(struct hsearch_data *__htab, unsigned int ctx, + const char __sep, int __flag, + char **__resp, size_t __size, + int argc, char * const argv[]); /* * nvars: length of vars array @@ -101,6 +112,10 @@ extern int himport_r(struct hsearch_data *__htab, const char *__env, size_t __size, const char __sep, int __flag, int __crlf_is_lf, int nvars, char * const vars[]); +extern int himport_ext(struct hsearch_data *__htab, unsigned int ctx, + const char *__env, size_t __size, const char __sep, + int __flag, int __crlf_is_lf, int nvars, + char * const vars[]); /* Walk the whole table calling the callback on each element */ extern int hwalk_r(struct hsearch_data *__htab, int (*callback)(ENTRY *)); diff --git a/lib/hashtable.c b/lib/hashtable.c index 0d288d12d991..d6975d9cafc6 100644 --- a/lib/hashtable.c +++ b/lib/hashtable.c @@ -200,8 +200,8 @@ void hdestroy_r(struct hsearch_data *htab) * example for functions like hdelete(). */ -int hmatch_r(const char *match, int last_idx, ENTRY ** retval, - struct hsearch_data *htab) +int hmatch_ext(const char *match, int last_idx, ENTRY ** retval, + struct hsearch_data *htab, unsigned int ctx) { unsigned int idx; size_t key_len = strlen(match); @@ -209,6 +209,8 @@ int hmatch_r(const char *match, int last_idx, ENTRY ** retval, for (idx = last_idx + 1; idx < htab->size; ++idx) { if (htab->table[idx].used <= 0) continue; + if (htab->table[idx].entry.context != ctx) + continue; if (!strncmp(match, htab->table[idx].entry.key, key_len)) { *retval = &htab->table[idx].entry; return idx; @@ -220,6 +222,12 @@ int hmatch_r(const char *match, int last_idx, ENTRY ** retval, return 0; } +int hmatch_r(const char *match, int last_idx, ENTRY ** retval, + struct hsearch_data *htab) +{ + return hmatch_ext(match, last_idx, retval, htab, 0); +} + /* * Compare an existing entry with the desired key, and overwrite if the action * is ENTER. This is simply a helper function for hsearch_r(). @@ -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; + } + /* Overwrite existing value? */ if ((action == ENTER) && (item.data != NULL)) { /* check for permission */ @@ -270,8 +286,8 @@ static inline int _compare_and_overwrite_entry(ENTRY item, ACTION action, return -1; } -int hsearch_r(ENTRY item, ACTION action, ENTRY ** retval, - struct hsearch_data *htab, int flag) +int hsearch_ext(ENTRY item, ACTION action, ENTRY ** retval, + struct hsearch_data *htab, int flag) { unsigned int hval; unsigned int count; @@ -371,6 +387,7 @@ int hsearch_r(ENTRY item, ACTION action, ENTRY ** retval, idx = first_deleted; htab->table[idx].used = hval; + htab->table[idx].entry.context = item.context; htab->table[idx].entry.key = strdup(item.key); htab->table[idx].entry.data = strdup(item.data); if (!htab->table[idx].entry.key || @@ -420,6 +437,15 @@ int hsearch_r(ENTRY item, ACTION action, ENTRY ** retval, return 0; } +int hsearch_r(ENTRY item, ACTION action, ENTRY ** retval, + struct hsearch_data *htab, int flag) +{ + ENTRY e; + + e = item; + e.context = 0; + return hsearch_ext(e, action, retval, htab, flag); +} /* * hdelete() @@ -445,16 +471,18 @@ static void _hdelete(const char *key, struct hsearch_data *htab, ENTRY *ep, --htab->filled; } -int hdelete_r(const char *key, struct hsearch_data *htab, int flag) +int hdelete_ext(const char *key, struct hsearch_data *htab, unsigned int ctx, + int flag) { ENTRY e, *ep; int idx; debug("hdelete: DELETE key \"%s\"\n", key); + e.context = ctx; e.key = (char *)key; - idx = hsearch_r(e, FIND, &ep, htab, 0); + idx = hsearch_ext(e, FIND, &ep, htab, 0); if (idx == 0) { __set_errno(ESRCH); return 0; /* not found */ @@ -483,6 +511,11 @@ int hdelete_r(const char *key, struct hsearch_data *htab, int flag) return 1; } +int hdelete_r(const char *key, struct hsearch_data *htab, int flag) +{ + return hdelete_ext(key, htab, 0, flag); +} + #if !(defined(CONFIG_SPL_BUILD) && !defined(CONFIG_SPL_SAVEENV)) /* * hexport() @@ -592,9 +625,9 @@ static int match_entry(ENTRY *ep, int flag, return 0; } -ssize_t hexport_r(struct hsearch_data *htab, const char sep, int flag, - char **resp, size_t size, - int argc, char * const argv[]) +ssize_t hexport_ext(struct hsearch_data *htab, unsigned int ctx, + const char sep, int flag, char **resp, size_t size, + int argc, char * const argv[]) { ENTRY *list[htab->size]; char *res, *p; @@ -618,8 +651,12 @@ ssize_t hexport_r(struct hsearch_data *htab, const char sep, int flag, if (htab->table[i].used > 0) { ENTRY *ep = &htab->table[i].entry; - int found = match_entry(ep, flag, argc, argv); + int found; + if (ep->context != ctx) + continue; + + found = match_entry(ep, flag, argc, argv); if ((argc > 0) && (found == 0)) continue; @@ -709,8 +746,14 @@ ssize_t hexport_r(struct hsearch_data *htab, const char sep, int flag, return size; } -#endif +ssize_t hexport_r(struct hsearch_data *htab, const char sep, int flag, + char **resp, size_t size, + int argc, char * const argv[]) +{ + return hexport_ext(htab, 0, sep, flag, resp, size, argc, argv); +} +#endif /* * himport() @@ -782,7 +825,7 @@ static int drop_var_from_set(const char *name, int nvars, char * vars[]) * '\0' and '\n' have really been tested. */ -int himport_r(struct hsearch_data *htab, +int himport_ext(struct hsearch_data *htab, unsigned int ctx, const char *env, size_t size, const char sep, int flag, int crlf_is_lf, int nvars, char * const vars[]) { @@ -898,7 +941,7 @@ int himport_r(struct hsearch_data *htab, if (!drop_var_from_set(name, nvars, localvars)) continue; - if (hdelete_r(name, htab, flag) == 0) + if (hdelete_ext(name, htab, ctx, flag) == 0) debug("DELETE ERROR ##############################\n"); continue; @@ -926,10 +969,11 @@ int himport_r(struct hsearch_data *htab, continue; /* enter into hash table */ + e.context = ctx; e.key = name; e.data = value; - hsearch_r(e, ENTER, &rv, htab, flag); + hsearch_ext(e, ENTER, &rv, htab, flag); if (rv == NULL) printf("himport_r: can't insert \"%s=%s\" into hash table\n", name, value); @@ -968,6 +1012,14 @@ end: return 1; /* everything OK */ } +int himport_r(struct hsearch_data *htab, + const char *env, size_t size, const char sep, int flag, + int crlf_is_lf, int nvars, char * const vars[]) +{ + return himport_ext(htab, 0, env, size, sep, flag, crlf_is_lf, + nvars, vars); +} + /* * hwalk_r() */
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() A context of an entry does not have any specific meaning in hash routines, except that it can be used one of matching parameters. This feature will be used to add multiple U-Boot environment context support in successive commits. Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> --- include/search.h | 15 +++++++++ lib/hashtable.c | 80 +++++++++++++++++++++++++++++++++++++++--------- 2 files changed, 81 insertions(+), 14 deletions(-)