[U-Boot] is there any issue with creating and using more than one hashtable?

[Robert P. J. Day]

On Sat, 1 Oct 2016, Wolfgang Denk wrote:

> Dear Robert,
>
> In message <alpine.LFD.2.20.1609301101260.11297 at localhost.localdomain> you wrote:
> >
> >   in misc_init_r(), i create a new hashtable to just grab the
> > contents of that bootline as is. i then create a *second*
> > hashtable and selectively move over just those keys i want from
> > the first hashtable, possibly renaming them in the process.
>
> I'm not sure how you implemented this "move over" thing...

  oh, nothing fancy, it's manual ... if i find an initial string of
"g=a.b.c.d", i translate that to the u-boot required "gatewayip"
variable name, and add that to the second hashtable. here's a snippet
of the routine that populates the second hash table given an ENTRY
corresponding to an original "var=value" -- you can see that it just
checks possible initial keys, and writes the transformed value to the
second table -- pure brute force, and it seems to work just fine:

populate_htab_2(ENTRY* e)
{
        const char*     k = e->key;
        ENTRY*          entry_ptr;

        if (!strcmp(k, "h")) {
                ENTRY he = {
                        .key = "serverip",
                        .data = e->data
                };
                printf("found h, translating to serverip.\n");
                hsearch_r(he, ENTER, &entry_ptr, &bootline_htab_2, 0);
        }

        if (!strcmp(k, "g")) {
                ENTRY ge = {
                        .key = "gatewayip",
                        .data = e->data
                };
                printf("found g, translating to gatewayip.\n");
                hsearch_r(ge, ENTER, &entry_ptr, &bootline_htab_2, 0);
        }

        ... snip ...

> > now here's the thing ... after all that, i manually check whether
> > the CRC for the env table in persistent storage is valid and, if
> > it is, i do *nothing* with all that hashtable content -- that was
> > all wasted cycles, but it should not do any harm.
> >
> >   but it does, because this is what happens:
> >
> >   TFTP from server 10.35.5.37; our IP address is 192.168.1.2; sending
> >   through gateway 192.168.1.1
> >
> > the server address is correct, but the IP address and gateway are
> > totally wrong -- they should have been untouched and been completely
> > different values; instead, they now equal the values in that second
> > hashtable, which i chose to do nothing with.

  one thing i want to emphasize about the above -- the example i was
working with pulled out and transformed just the ip address and
gateway loading them into the second hashtable, and those are the
values that are now "incorrect." there was no server setting in that
original data, hence "serverip" was not loaded into the second
hashtable, so it's fine. it seems that only u-boot variables i put in
the second hashtable somehow "bleed" into my working environment,
whatever that might mean.

> For a test, I would like to ask you to run the following commands
> before the TFTP command:
>
> 	printenv serverip;setenv serverip ${serverip};printenv serverip
> 	printenv ipaddr;setenv ipaddr ${ipaddr};printenv ipaddr
> 	printenv gatewayip;setenv gatewayip ${gatewayip};printenv gatewayip
>
> Does this change anything?

  i'll have to wait until later to try this, but i'm pretty sure i
tried the above yesterday, and everything looked fine ... i would
"print" the variables, they seemed fine, i'd "env save", i'd reset,
check again, all looked good, and yet ... same problem.

  wait, maybe i didn't explicitlt "setenv" in the middle, because it
didn't seem necessary. that might be something i didn't test.

> >   is there some weird issue with re-entrancy here? if i print "ipaddr"
> > and "gatewayip", they seem correct. if i "md" the environment in
> > persistent storage, again, they seem correct. and yet, when TFTP kicks
> > in, it seems to pick up the values from that hashtable.
>
> Actually the TFTP will not access the environment directly to
> determine the boot parameters like bootfile, ipaddr, gatewayip,
> netmask, serverip, etc.; instead, it uses internal variables.  So
> your environment settings for "ipaddr" and "gatewayip" may contain
> totally different values than the variables net_ip resp.
> net_gateway which get used by the TFTP code.

  ah ... would this also apply to other net-related commands like
ping? because, weirdly, even ping doesn't seem to work, unable to ping
even its own gateway, but if that command is *also* quietly picking up
the incorrect "gatewayip" setting from that second hashtable, that
would also explain its failure.

> These variables get set through the U_BOOT_ENV_CALLBACK
> functionality, i. e. as a callback whenever the corresponding
> variable gets set. "setting" here means that the value in the hash
> table gets changed - see function _compare_and_overwrite_entry() in
> "hashtable.c":
>
> 244                         /* If there is a callback, call it */
> 245                         if (htab->table[idx].entry.callback &&
> 246                             htab->table[idx].entry.callback(item.key,
> 247                             item.data, env_op_overwrite, flag)) {
> 248                                 debug("callback() rejected setting variable "
> 249                                         "%s, skipping it!\n", item.key);
> 250                                 __set_errno(EINVAL);
> 251                                 *retval = NULL;
> 252                                 return 0;
> 253                         }
>
>
> So when you insert variables of these registered names into your
> alternative hash table using the common code, then the respective
> callbacks will fire in the same way as if you had changed the
> environment settings through a setenv command.

  oh ... crap ... that would appear to be the issue, then.

> It is obvious that the U-Boot design did not anticipate a situation
> where an alternative hash table with different settings would be
> created.

  i will admit i didn't see any documentation that told me explicitly
that i *could* have additional hashtables; i just assumed, hey, it's
just another data structure, it has some useful functionality, i'll
just create another couple of them to save me some coding.

  i *did* try to verify that there was no shared data; things like
making sure "strdup" was used to avoid sharing things like strings. i
never thought of checking the callback structure.

> A quick but ugly workaround could be to re-set the variables to the
> values stored in the environment by running something like
>
> 	setenv ipaddr ${ipaddr}
>
> etc.
>
>
> Hope this helps.

  massively, thanks. it just means that my attempt at cleverness in
using a couple more hashtables wasn't as clever as i thought.

rday

-- 

========================================================================
Robert P. J. Day                                 Ottawa, Ontario, CANADA
                        http://crashcourse.ca

Twitter:                                       http://twitter.com/rpjday
LinkedIn:                               http://ca.linkedin.com/in/rpjday
========================================================================