Read-only env variables

Hi u-boot,

I see u-boot supports readonly serial# and ethaddr, but I have a few other variables that I need to be read-only. I'm planning to implement a generic list of variables that cannot be changed or deleted.

I'm interested to know what sort of specification people would find most appropriate. My current thought is to follow the same delimiter as the env itself, i.e. null separated list with double null terminator. This list would then be defined in the board config header. It also seems that you should be able to specify simple access rules for each variable name perhaps using "=<val>" after each var name where <val> could have constants (probably numbers for performance, but could be string literals) to define modes. Right now I'm thinking read-only, create-only, change-default, and change-only (i.e no delete) are the modes that make sense.

I'm also interested if anyone would be opposed to simply using this new specification of readonly vars to implement the serial# and ethaddr protection. The CONFIG_HAS_UID is a bit odd... should that be available in general for read-only vars?

Comments?

Thanks,
-Joe

Dear Joe Hershberger,

Good idea, and thanks in advance!

You might also go one step further and define variable types - numeric, string, MAC addr, IP addr, ...

On the other hand I wonder if a compile-time defined, static list is such a good idea. Why not putting this list in a variable itself? Then users could use this feature to add their own r/o variables, or protect already existing definitiins, etc. etc. [Note that you could still prevent users from editing the list by including a reference to itself.]

If we had such a feature, then all special handling of serial# and ethaddr should go away, of course (this would also allow to fix the inconsistent behaviour of ethaddr (read-only) versus eth<N>addr (writable).

Regarding CONFIG_HAS_UID - I never understood what it was intended for, and it is completely undocumented, and davinci_schmoogie and MVSMR are the only boards that actually use it. It should be removed from common code.

Sergey, Andre - comments?

Best regards,

Wolfgang Denk

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
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
You get a wonderful view from the point of no return.
                                    - Terry Pratchett, _Making_Money_

I'm guessing the reason for this being that the value can be validated on set instead of on use? I can see some interactive usability benefit to this. Also the Linux fw_setenv would be well served with this capability.

This is interesting. I had considered this my self as an option for items (such as ipaddr) which some times the user wants to change and save (static ip) and other times would not like to save (ip set by dhcp). This would call for a dynamic configuration for the variables. Having all of this in an environment variable seems like it would get cumbersome to use rather quickly. I like the elegance of everything being in the env, but I'm not sure that it's practical, especially if each variable name is assigned a type and access control.

As I thought more about the implementation, I like the idea of using bit-masks for each part of the access control that users can OR together in their board config headers. Unfortunately, that doesn't play nicely with initializing default u-boot env variables due to the need to convert the result of some expression into a string literal in the preprocessor. I'm not aware of any way to do that. That also means that changing the configuration in the env on the fly is tedious because the user must remember the masks. One way around this would be to make the default env creation routing a bit more fancy and have it generate the env variable at run-time based on the compiled in masks.

Another possibility that could make things simpler to use at runtime would be to create a separate env variable for each access control bitmask element and allow the user to simply add the variable names to those variables. They get the behavior for each one the name is in.

Another thing that would make dealing with the lists in env variables easier would be to support slightly different behavior for setenv... here's what I have in mind: if an env variable called "myList" contains "varName1;varName2" and you call "setenv myList varName3", then the result is myList contains "varName1;varName2;varName3". If you then call "setenv myList -varName2", then myList would contain "varName1;varName3". The behavior would be available in general by having a list variable type (just like numeric or IP addr). I'm not set on using semi-colons as list delimiters or the "-" to signify remove-from-list in setenv, but those seem natural. I'm interested if there are any foreseeable issues with those selections or if there are more appropriate options.

I see we're on the same page about cleaning up these one-off behaviors. Excellent.

I agree. It really doesn't seem to fit, though it could be made one of the access control types rather simply.

Best regards,
-Joe

Dear Joe Hershberger,

Right, that was my intention.

Well, we could go for a simple type scheme, something like

d - decimal number
i - IP address
m - mac address
s - string
x - hex number
...

plus similarly simple access control codes:

d - no change except reset to default possible
o - one-time setting, no changes afterward r - read-only
(none) - unrestricted access

Then we could do something like:

setenv acl delay:d,address:a,ipaddr:i,ethaddr:mo,serial#:sr

The only disadvantage is that the meaning of the qualifiers is position dependent, but I think this is acceptable for the purposes we have in mind.

I think bitmasks is probably overkill - we probably don't need that many options. The scheme above could be extended for more qualifiers as well.

That would be incompatible to existing use. Not a good idea, IMHO.

What would be your use of such list vatiables?

And what's wrong with

setenv myList "varName1;varName2"
setenv myList "$myList;varName3"

to do the same as above, using only the existing feature set?

OK, the "remove entry" function is not that easy here, but which usage do you have in mind that needs that? I never ran into that before.

Please stop this.

Best regards,

Wolfgang Denk

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
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
If something is different, it's either better or worse,  and  usually
both.                                                    - Larry Wall

l - list delimited by ';' (special behavior for setenv)

That seems pretty reasonable. I think I'd like to see the access control codes more role based than limitation based. Such as:

u - unique board configuration (mac addr, ser#)... set if not
set, default override
s - script in env var... read only, default
r - runtime variable (ip addr)... set in env, not saveable to flash.

I think I will at least use bitmasks to define each of the commands that are available in the acl env var. That will make defining new commands simple.

Specifically for manipulating the acl and any other list that a user may need. Because it will self reference and define its type as a list,

There's nothing wrong with that, but the issue is with removal... and if you are making the behavior special for lists, then this is cleaner than dereferencing the original list every time.

Consider the case where a variable is defined as a runtime variable, but the user would like to change the value and commit that change to flash. It would be much more usable if there were a simple way to remove an element from a list.

Apologies.

Best regards,
-Joe

Dear Joe Hershberger,

Don't do this. ';' has a clear meaning. Using it for other purposes

This is most probably less flexible and more cumbersome to use. It's easier to define what exactly you want to do (don't allow over- writing) that finding a role that happens to include this behaviour. Think also about the end user who wonders why variable "foo" is considered to be a "unique board configuration" when all you wanted is to apply certain behaviour.

Why would that be needed?

See my example above:

setenv acl delay:d,address:a,ipaddr:i,ethaddr:mo,serial#:sr

[Implementation note: the implementation should allow for and ignore repeated or trailing ',' separators - i.e. something like "delay:d,,,address:a,ipaddr:i,ethaddr:mo,,serial#:sr,," should work as well. Reason: see below about incremental building.]

Well, we have the editenv command, which makes editing pretty easy. I fear/feal that adding lists is (1) not really needed and thus most likely more overhead than benefit, and (2) holds a lot of potential for causing confusion and conflicts.

We don't have lists vailables in bash either (or do we?)

What to you mean by "runtime variable" or by "change to flash"? I don;t understand what these terms are supposed to mean. As mentioned before, "editenv" seems a pretty straightforward way to edit the settings.

I feel we should try to make this not overly complicated - don't add bells and whistles without real need. Also keep in mind that a lot of power results from being able to dynamically build such settings from smaller blocks.

I can imagine scripts that run things like

	setenv acl "$acl_net,$acl_hw_id,$acl_foo,$acl_bar"

Best regards,

Wolfgang Denk

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
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
That's the thing about people who think  they  hate  computers.  What
they really hate is lousy programmers.
- Larry Niven and Jerry Pournelle in "Oath of Fealty"

Fair enough... ',' then.

The primary reason for wanting this sort of comes back to the reason I wanted bitmasks in the first place. I want to be able to mix certain access control attributes for different variables.

I've added a command called "defaultenv" which comes in really handy in practice here. Basically what it does is revert everything in memory to a default env except for certain (currently hard coded) variables such as ethaddr and serial#. This means that if I rebuild and change a script or boot command, my people can simply load the new u-boot and run "defaultenv;saveenv" to get the updated scripts but not have to redefine all the board unique variables like they would if they just erased their env sectors. I'd like to specify that a variable should be preserved on reverting to default env independent of being read-only or not. Perhaps you would prefer to have each permutation defined as a sum of attributes instead of a role. It's the same thing with a different name.

The idea was that if you wanted to remove "ethaddr:mo" from the list for instance, you could do so without dealing with the rest of the items in the list. See below.

I agree. It will be tolerant of this.

I haven't used the editenv command, so I'll play with that a bit. It sounds like it will make my concern about removal less of an issue. I'll try it and see how usability feels.

Not that I'm aware of. Arrays, yes, but that's not what I'm going for here.

What I'm referring to here is being able to specify a variable as r/w while running in u-boot, but that that value will not be written to flash. If the value was defined in flash, then the existing value in flash will be retained when saveenv is called. Again, I'm considering the case with "ipaddr" and family. If the user wishes to change the static IP, then that change should be written to flash. If the user wishes to use dhcp to configure all the network settings, those settings should not be saved to flash (and then used as though they were static settings). Currently if you use dhcp and then want to save some other change in the env, you save all the dhcp settings and the next time the board is powered, you use an address that you have no lease for and probably conflict with another machine.

I also don't want it to be complicated, but I'd like it to cover the use cases that I'm up against at the moment in a relatively user friendly way. I feel like we are close to a solution.

That sounds great. I also envision making part of the acl unchangeable and part of it changeable. That would be done by making the the acl reference itself as read-only and also contain a dereference such as:
"acl=$acl_rw,acl:sr,ipaddr:i,ethaddr:mo,serial#:sr". You could then change the value of acl_rw at runtime but not the value of acl.

One thing I've noticed that I don't quite understand is how you can add a '$' to a variable's value from the command line. If I escape it with a '\', the '\' sticks around in the value of the variable, but if I don't escape it, then the '$' dereferences in the setenv. Seems like a bug to me. The '\' should not end up in the value of the var when used in setenv to escape the '$'.

Best regards,
-Joe

I ran into the same situation, where I needed set of environment variables for in house testing, and a different set for when products actually go out to customers. So we were erasing the flash quite often, but in this scenario, things like serial number and mac address stay with the board regardless if we replace all of the memory on the board. I wrote a mirror image of the environment variable handling routines, but used a small serial EEPROM that could hold a 100 bytes.

We modified the fw_env to parse the serial EEPROM as well.

So everything that is in the serial EEPROM is accessed by a mirrored set of commands like "protected_saveenv" and "protected_printenv" that are hidden from the user in the help.

I integrated the "protected environment" into the regular environment by looking through the protected space first before allowing any "setenv" and when a "printenv" is done, it will print the protected variables last.

Unfortunately our u-boot is heavily modified in or to make it production worthy, so we've never been able to enter it into the repository. I can however provide you will all the code necessary if you would like.

-----Original Message-----
From: [email protected] [mailto:u-boot-bounces at lists.denx.de] On Behalf Of Joe Hershberger
Sent: Monday, May 10, 2010 5:34 PM
To: Wolfgang Denk
Cc: u-boot
Subject: Re: [U-Boot] Read-only env variables

Fair enough... ',' then.

The primary reason for wanting this sort of comes back to the reason I wanted bitmasks in the first place. I want to be able to mix certain access control attributes for different variables.

I've added a command called "defaultenv" which comes in really handy in practice here. Basically what it does is revert everything in memory to a default env except for certain (currently hard coded) variables such as ethaddr and serial#. This means that if I rebuild and change a script or boot command, my people can simply load the new u-boot and run "defaultenv;saveenv" to get the updated scripts but not have to redefine all the board unique variables like they would if they just erased their env sectors. I'd like to specify that a variable should be preserved on reverting to default env independent of being read-only or not. Perhaps you would prefer to have each permutation defined as a sum of attributes instead of a role. It's the same thing with a different name.

The idea was that if you wanted to remove "ethaddr:mo" from the list for instance, you could do so without dealing with the rest of the items in the list. See below.

I agree. It will be tolerant of this.

I haven't used the editenv command, so I'll play with that a bit. It sounds like it will make my concern about removal less of an issue. I'll try it and see how usability feels.

Not that I'm aware of. Arrays, yes, but that's not what I'm going for here.

What I'm referring to here is being able to specify a variable as r/w while running in u-boot, but that that value will not be written to flash. If the value was defined in flash, then the existing value in flash will be retained when saveenv is called. Again, I'm considering the case with "ipaddr" and family. If the user wishes to change the static IP, then that change should be written to flash. If the user wishes to use dhcp to configure all the network settings, those settings should not be saved to flash (and then used as though they were static settings). Currently if you use dhcp and then want to save some other change in the env, you save all the dhcp settings and the next time the board is powered, you use an address that you have no lease for and probably conflict with another machine.

I also don't want it to be complicated, but I'd like it to cover the use cases that I'm up against at the moment in a relatively user friendly way. I feel like we are close to a solution.

That sounds great. I also envision making part of the acl unchangeable and part of it changeable. That would be done by making the the acl reference itself as read-only and also contain a dereference such as:
"acl=$acl_rw,acl:sr,ipaddr:i,ethaddr:mo,serial#:sr". You could then change the value of acl_rw at runtime but not the value of acl.

One thing I've noticed that I don't quite understand is how you can add a '$' to a variable's value from the command line. If I escape it with a '\', the '\' sticks around in the value of the variable, but if I don't escape it, then the '$' dereferences in the setenv. Seems like a bug to me. The '\' should not end up in the value of the var when used in setenv to escape the '$'.

Best regards,
-Joe
_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Dear "Craig Millen",

There are other boards doing similar things - see for example board/tqc/tqm8xx/load_sernum_ethaddr.c which can pre-load the MAC address and serial number on all TQM8xxL/TQM8xxM/TQM82xx boards from a dedicated area in flash.

There has been discussion about such a feature in the past. I even tried to define the user interface for such a command set, but unfortunately I didn't have enough time myself and nobody else took the bait either.

Yes, it will make it less an issue - as far as interactive use is concerned. But then I still see why you want such a feature fr example for use in scripts.

We agree that the feature is useful, but I am not convinced that the list approach makes sense. Eventually I'd rather have a scriptable edit command. IIRC there was a vi clone in one of the IOCCC submissions with < 1.5k code... :-)

Ah. But you have this already, when using the hush shell. It has shell variables (which work exactly like your r/w variables), and you have environment variables.

All you eventually need is a script / macro to initialize the shell variables after boot.

Be careful. You are defining policy here, and actually you can do all this already now.

Use "setenv ipaddr ...; saveenv" if the user wishes to change the static IP and write it to flash.

Use "dhcp" (without "saveenv") to use dhcp to configure all the network settings without saving this to flash.

What else do you need?

Well, then just do the saveenv _before_ you use DHCP. Or be careful to delete / reset the respective variables.

I agree, your vision is to make this easier, but in this specific case I doubt the benefits justify the additional overhead.

I can't parse that. Do you have an example?

Best regards,

Wolfgang Denk

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
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 only way to get rid of a temptation is to yield to it.
                                                        - Oscar Wilde

I don't have a separate memory to use for this purpose, but it seems like a good approach if you do.

Ok... if you can dig up the user interface you were thinking of, I can compare it to what I've done and possibly tweak it. As it is now, I intend to use the acl to define which variables are preserved. The command takes no arguments and changes only the in-RAM environment.

Both in scripts and when called directly from other commands.

:) I was going for simple(r).

I'll have to look into this. I've noticed their existence, but haven't used them so far. I can see how that would help for intermediate state storage (as long as they stay around through every context such as from preboot to bootcmd), but not for variables that need to be accessible to u-boot (such as network settings).

I want the acl to be the one defining this policy.

Works great in a vacuum.

In the case of dhcp, the key is that I want to be able to save some unrelated change to the environment... I shouldn't be prevented from doing it because I happened to configure the network already.

Not possible for the use case... I'm using netconsole, so dhcp must be first to make the console accessible... so no change made to the env could ever be saved.

That's tedious. I think it should be better. Making it better is part of the reason for doing this.

All of this stems from the fact that a number of env vars are magical.
Things like "ipaddr" and "stdin" etc. are hooked up to internals of
u-boot. In the case of stdin and family, the behavior (without CONFIG_SYS_CONSOLE_IS_IN_ENV) is that those variables only affect the run-time behavior and are not written to flash. I'm certain that the reason for that is the same as the issue I'm having with the network configuration. I think there should be a simple-to-use way to indicate that a magical env var should be only magical and not in flash or possibly initialized from flash but not saved. Since I'm making an ACL, it seemed reasonable to implement that behavior generically. The list behavior idea was an attempt at making the acl easier to change, since it would need to be changed (by users, scripts, and u-boot code) for several different use cases.

Run this command:

setenv a "\$x";printenv a;setenv b "$x";printenv b

Neither one ends up containing "$x". I believe that the first one should.

Best regards,
-Joe

Dear Joe Hershberger,

In most cases the compiled-in default environment should be sufficient for this purpose.

See here:

http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/36570/focus=36861

Taking this one step further, we should then also rename "editenv" into "env edit".

Re-reading the proposal, I still like the suggested user interface.

You can of course initialize environment variables from values stored in shell variables, i. e.

	setenv ipaddr ${var}

But this is IMHO wrong. The ACl should describe properties, i. e. if a variable can be written or not etc. It should NEVER try to define how the user deploys the properties of a variable.

? What do you mean here?

I don't see your problem. If you want to define a default IP address and want to use this as "ipaddr" environment variable unless DHCP overwrites this value, then you can easily do that:

=> setenv default_ipaddr aa.bb.cc.dd
=> setenv default_netmask ...
...
=> setenv net_default 'setenv ipaddr ${default_ipaddr};setenv netmask ${default_netmask};...' => setenv preboot run net_default

How would you in your szenario, where "dhcp" can change the "ipaddr" environment variable, but a saveenv would not save it, set a new "ipaddr"?

Me thinks you are adding complexity here without need, plus potential for lots of confusion.

OK, then do as described above and consider the variables set by DHCP as volatile and do not attempt to use these to store your default settings that are supposed to be persistent.

No, the reason is more likely that the one who implemented this did just what he actually needed, without a clear concept for such a global scope. The next one who needed different behaviour added CONFIG_SYS_CONSOLE_IS_IN_ENV. Now we have both.

I disagree. Please let the end user decide what we wants to do with variable settings - wether he wants to save them or not.

Ok, se your original intention was a scriptable "env edit" command, which might (or might not) be based on some internal list representation - but ther eis no need to introduce such a list to the user interface.

There is some inconsistency in the environment variable subsituion, indeed. Note that the example works with shell variables, though:

=> a="\$x"; b="$x"
=> echo a=${a} b=${b}
a=$x b=

For environment variables, you should delay/prevent variable substitution using '...' quotes:

=> setenv a '\$x';printenv a;setenv b '$x';printenv b a=$x
b=$x

Best regards,

Wolfgang Denk

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
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
Just about every computer on the market today runs Unix,  except  the
Mac (and nobody cares about it).                   - Bill Joy 6/21/85

Looks like a reasonable user interface as far as the command layout.

Why rename one existing command? Why not rename askenv and the rest? I don't think I would rename any of them or I would have a clear separation and reason for renaming some and not others.

It's a tool for the user to implement his chosen policy. Instead of writing a script to keep track of variables that have changed (that he doesn't want to save) and having to call saveenv through a wrapper script to process that list, it could be as simple as him adding that to the ACL.

Sorry... Nevermind. Things are complicated by netconsole, but aren't addressed by this anyway.

This is a reasonable approach and one that I have considered.

Because the acl is editable, the user could simply change the lifetime of the variable. For best ease of use for this case, I've been thinking of something like having the dhcp command mark the variables it touches as volatile (see the bottom of this email) in the acl when it touches them, and if they are touched by the setenv command line interface, the volatile attribute is removed. I wouldn't want all variables that are marked as volatile to no longer be volatile when accessed by setenv, so I would simply define a separate type called auto-volatile that the dhcp command would utilize instead. If setenv command line sets an auto-volatile variable, then that variable loses its volatile attribute.

It's true that you could currently choose to never call many of these commands directly and always have wrapper scripts, but that's pretty tedious and prevents you from having a familiar interface that comes with using the commands directly. I know I personally would take a while to train myself to type "run saveenvcmd" instead of "saveenv".

While some things can be worked around, it doesn't mean they are a easy to use. Also, all behavior stays unchanged if you don't set the ACL. Like many things in u-boot, if a user is not interested, they can just ignore the feature.

I'm curious what you think will be confusing?

Again, I considered this possibility and have it on the back burner if for some reason this is difficult to implement, but I don't like to commit variables to flash that shouldn't be.

I guess my point is that the same need existed for other sub-systems, and could be implemented generically with this in a consistent way rather than one-off hacks for each case.

Exactly. I'd like to give the user a simple way to chose which he wants to save or not. He should not need to manage it by writing scripts that attempt to save off variables and restore them after the env is written to flash. Right now the user cannot decide.

Fair enough. Since the list management behavior was just to make easier what I envisioned would be tedious, I've tabled that until I try it out.

--------------

I've implemented the acl for type and access specification. It currently looks like this:

The first char in the attributes for a var in the acl is the type. It can be:

s - string (default)
d - decimal
x - hexadecimal
b - boolean
i - ip address
m - ethernet mac address

The second char in the attributes for a var in the acl is the access mode. It can be:

a - any (default, optional)
r - read-only
o - set once (this is create / change default)

I'm envisioning a third char in the attributes that is the lifetime. It can be:

p - precious (not removed on env default)
v - volatile (not saved to flash)
a - auto-volatile (not saved to flash, auto-cleared by setenv from
command line)

The fw_setenv Linux utility and the setenv command have the same behavior and share the same code for implementing the acl.

In my initial playing with it, the experience feels like too much of the detail that we want controlled is put in your face. Things that should just be a given such as the fact that ipaddr and ethaddr and a number of other variables should have a certain type or be set-once or whatever. I think what will make it more reasonable to use is to have a compiled in baseline acl, with a env var based augmentation. That way the user doesn't have to look at all the things that should just always be. It seems to be pretty much required from a usability standpoint.

I'm also interested if you think it should be a compile time option to support the ACL at all or if it should be added as a first class citizen to permanently take the place of the curent hard-coded behaviors. I currently have it disabled at compile time (in u-boot, not fw_env), but I also have not removed the other options it could replace.

Best regards,
-Joe

Dear Joe Hershberger,

Did you in any way start working on this?

FYI: I'm in the process of a bigger rewrite of the envrionment handling code, which will, among other things, perform such a renaming (keeping the old names in place, for compatibility, of course).

I think I'll address this situation from another side; in my new code you will be able to export and import envrionment data; this way you can for example save and restore snapshots of your environment settings, and switch between several configurations easily. Also, you can build your configuration by incrementally adding sets of definitions.

You might want to add 'p' for pointers.

I did not get the concept of "auto-volatile". Please explain once more.

I think we should make this a compile time option - and if it was for the unavoidable additional memory footprint.

Best regards,

Wolfgang Denk

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
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
CAUTION:  The Mass of This Product Contains the Energy Equivalent  of
85 Million Tons of TNT per Net Ounce of Weight.

I did not rename the commands yet... though I did implement 2/3 of the ACL. I never head back on which parts of the env handling should be renamed and why.

So this means all aspects of the env handling would be under "env *" including such things as printenv and setenv? I like this so long as the current commands remain for compatibility.

That sounds interesting. Is there another thread where you've gone into detail about this?

How is this different than hexadecimal or decimal? What would the validation entail?

I'll try to make it more clear with an example... take ipaddr for instance. Your ACL has configured the ipaddr as "auto-volatile". Your static IP address is stored in flash in ipaddr. You boot and decide you want DHCP, so you run "dhcp" to assign the ipaddr variable.
You save your environment and the IP address that the DHCP server
gave you is not saved in the flash, it is only used for network communication during this session. The next time you boot, you get your static IP back, since it was not overwritten in your flash. You then device that you would like to change your static IP address setting. When you call setenv some the command prompt (or a script), the "auto-volatile" setting is removed from the ipaddr variable in the ACL (the acl in RAM, not flash) which gives it the default behavior that all variables have now. Now when you save your environment, the new ipaddr setting is saved into flash and available he next time you boot.

So that means we should not remove the current ethaddr and serial# write protection.

Best regards,
-Joe Hershberger

Dear Joe Hershberger,

That's what I'm doing.

No, not yet. I'm still experimenting with the design; I would like to know a little better myself what I want to do before I start discussing it :-)

For 'd' and 'x' I would expect to see the equivalent of an "unsigned long" as underlying data type; for 'p' we should probably use a "physaddr_t" which might be > 32 bit even on 32 bit systems.

You mean the setenv() called from the DHCP code would behave differently than the setenv() called from the command line? Now that _is_ confusing to me. I doubt many users will understand what's going on on such a system.

Not if it can be avoided easily. OTOH my new code will have commands like "env default" or "env import" which will not care at all about these variables, but simply do what you asked for. Dunno if this will need to be changed - but if so, then I'd rather to this in the context of a new read-only concept.

Best regards,

Wolfgang Denk

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
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
As far as we know, our computer has never had an undetected error.
		                                           -- Weisert

Enable validation of the values given to enviroment variables when calling env set. Variables can be restricted to only decimal, hexadecimal, or boolean. If CONFIG_CMD_NET is also defined, the variables can also be restricted to IP address or MAC address.

The format of the list is:
type_attribute = [s|d|x|b|i|m] access_atribute = [a|r|o]
attributes = type_attribute[access_atribute] entry = variable_name[:attributes] list = entry[,list]

The type attributes are:
s - String (default)
d - Decimal
x - Hexadecimal
b - Boolean
i - IP address
m - MAC address

The access attributes are:
a - Any (default)
r - Read-only
o - Write-once (change default)

- CONFIG_ENV_ACL_DEFAULT
Define this to a list (string) to define the "acl" envirnoment variable in the default or embedded environment.

- CONFIG_ENV_ACL_STATIC
Define this to a list (string) to define validation that should be done if an entry is not found in the "acl" environment variable. To override a setting in the static list, simply add an entry for the same variable name to the "acl" variable.

Joe Hershberger (12):
tools/env: Use a board-specific default env tools/env: Remove unneeded complexity tools/env: Don't call env_init() in fw_getenv() tools/env: Reduce the impact on real-time processes tools/env: Serialize calls to fw_*env
env: Make the "silent" env var take effect immediately
env: Update serial baudrate in env_relocate()
env: Check for NULL pointer in envmatch()
env: Clarify the cases for env set
env: acl: Add environment variable access control list
env: acl: Add support for access control to env ACL
env: cosmetic: Consilidate the default env definition

 README                  |  42 +++++++
 common/Makefile         |   1 +
 common/cmd_nvedit.c     | 171 ++++++++++++++++++++-------
 common/env_acl.c        | 308 ++++++++++++++++++++++++++++++++++++++++++++++++
 common/env_common.c     | 123 +++++--------------
 common/env_embedded.c   | 111 +----------------
 doc/README.silent       |  14 ++-
 include/env_acl.h       |  63 ++++++++++
 include/env_default.h   | 142 ++++++++++++++++++++++
 tools/env/Makefile      |   7 +-
 tools/env/fw_env.c      | 305 ++++++++++++++++++++++++-----------------------
 tools/env/fw_env.h      |  25 ++++
 tools/env/fw_env_main.c |  59 ++++++----
 13 files changed, 943 insertions(+), 428 deletions(-)
 create mode 100644 common/env_acl.c
 create mode 100644 include/env_acl.h
 create mode 100644 include/env_default.h

1.7.11.5

Originally added in aa701b94336b358798d676eef12a7b90bdac23f5

Before this patch, there was a hard-coded env that was used as default if the env in flash is detected as invalid. Now this tool (compiled for a given board) will share the default env with the u-boot for the board.

Fix include of config.h

Need to define "TEXT_BASE" when building the fw_env tool so that the default env will be correct for environments which use it.

Define __ASSEMBLY__ when calling #include <config.h> so that we only get #defines (all we're interested in).

---
 tools/env/Makefile |  5 +++--
 tools/env/fw_env.c | 28 +++++++++++++---------------
 tools/env/fw_env.h | 19 +++++++++++++++++++
 3 files changed, 35 insertions(+), 17 deletions(-)

index 07634bc..ab73c8c 100644
@@ -24,13 +24,14 @@
 include $(TOPDIR)/config.mk
 
 HOSTSRCS := $(SRCTREE)/lib/crc32.c  fw_env.c  fw_env_main.c
-HEADERS	:= fw_env.h
+HEADERS	:= fw_env.h $(OBJTREE)/include/config.h
 
 # Compile for a hosted environment on the target
 HOSTCPPFLAGS  = -idirafter $(SRCTREE)/include \
 		-idirafter $(OBJTREE)/include2 \
 		-idirafter $(OBJTREE)/include \
-		-DUSE_HOSTCC
+		-DUSE_HOSTCC \
+		-DTEXT_BASE=$(TEXT_BASE)
 
 ifeq ($(MTD_VERSION),old)
 HOSTCPPFLAGS += -DMTD_OLD

index e292d2b..e46791d 100644
@@ -45,8 +45,6 @@
 
 #include "fw_env.h"
 
-#include <config.h>
-
 #define WHITESPACE(c) ((c == '\t') || (c == ' '))
 
 #define min(x, y) ({				\

@@ -81,7 +79,7 @@ static int dev_current;
 #define ENVSECTORS(i) envdevices[(i)].env_sectors
 #define DEVTYPE(i)    envdevices[(i)].mtd_type
 
-#define CONFIG_ENV_SIZE ENVSIZE(dev_current)
+#define CUR_ENVSIZE ENVSIZE(dev_current)
 
 #define ENV_SIZE      getenvsize()
 

@@ -218,7 +216,7 @@ static int get_config (char *);
 #endif
 static inline ulong getenvsize (void)
 {
-	ulong rc = CONFIG_ENV_SIZE - sizeof (long);
+	ulong rc = CUR_ENVSIZE - sizeof(long);
 
 	if (HaveRedundEnv)
 		rc -= sizeof (char);

@@ -434,7 +432,7 @@ int fw_env_write(char *name, char *value)
 		++env;
 	/*
 	 * Overflow when:
-	 * "name" + "=" + "val" +"\0\0"  > CONFIG_ENV_SIZE - (env-environment)
+	 * "name" + "=" + "val" +"\0\0"  > CUR_ENVSIZE - (env-environment)
 	 */
 	len = strlen (name) + 2;
 	/* add '=' for first arg, ' ' for all others */

@@ -949,8 +947,8 @@ static int flash_write (int fd_current, int fd_target, int dev_target)
 	printf ("Writing new environment at 0x%lx on %s\n",
 		DEVOFFSET (dev_target), DEVNAME (dev_target));
 #endif
-	rc = flash_write_buf (dev_target, fd_target, environment.image,
-			      CONFIG_ENV_SIZE, DEVOFFSET (dev_target),
+	rc = flash_write_buf(dev_target, fd_target, environment.image,
+			      CUR_ENVSIZE, DEVOFFSET(dev_target),
 			      DEVTYPE(dev_target));
 	if (rc < 0)
 		return rc;

@@ -989,10 +987,10 @@ static int flash_read (int fd)
 
 	DEVTYPE(dev_current) = mtdinfo.type;
 
-	rc = flash_read_buf (dev_current, fd, environment.image, CONFIG_ENV_SIZE,
+	rc = flash_read_buf(dev_current, fd, environment.image, CUR_ENVSIZE,
 			     DEVOFFSET (dev_current), mtdinfo.type);
 
-	return (rc != CONFIG_ENV_SIZE) ? -1 : 0;
+	return (rc != CUR_ENVSIZE) ? -1 : 0;
 }
 
 static int flash_io (int mode)

@@ -1089,11 +1087,11 @@ int fw_env_open(void)
 	if (parse_config ())		/* should fill envdevices */
 		return -1;
 
-	addr0 = calloc (1, CONFIG_ENV_SIZE);
+	addr0 = calloc(1, CUR_ENVSIZE);
 	if (addr0 == NULL) {
-		fprintf (stderr,
+		fprintf(stderr,
 			"Not enough memory for environment (%ld bytes)\n",
-			CONFIG_ENV_SIZE);
+			CUR_ENVSIZE);
 		return -1;
 	}
 

@@ -1128,11 +1126,11 @@ int fw_env_open(void)
 		flag0 = *environment.flags;
 
 		dev_current = 1;
-		addr1 = calloc (1, CONFIG_ENV_SIZE);
+		addr1 = calloc(1, CUR_ENVSIZE);
 		if (addr1 == NULL) {
-			fprintf (stderr,
+			fprintf(stderr,
 				"Not enough memory for environment (%ld bytes)\n",
-				CONFIG_ENV_SIZE);
+				CUR_ENVSIZE);
 			return -1;
 		}
 		redundant = addr1;

index ad32446..a1a6807 100644
@@ -21,6 +21,15 @@
  * MA 02111-1307 USA
  */
 
+/* Pull in the current config to define the default environment */
+#ifndef __ASSEMBLY__
+#define __ASSEMBLY__ /* get only #defines from config.h */
+#include <config.h>
+#undef	__ASSEMBLY__
+#else
+#include <config.h>
+#endif
+
 /*
  * To build the utility with the static configuration
  * comment out the next line.

@@ -29,6 +38,7 @@
  */
 #define CONFIG_FILE     "/etc/fw_env.config"
 
+#ifndef CONFIG_FILE
 #define HAVE_REDUND /* For systems with 2 env sectors */
 #define DEVICE1_NAME      "/dev/mtd1"
 #define DEVICE2_NAME      "/dev/mtd2"

@@ -40,14 +50,23 @@
 #define ENV2_SIZE         0x4000
 #define DEVICE2_ESIZE     0x4000
 #define DEVICE2_ENVSECTORS     2
+#endif
 
+#ifndef CONFIG_BAUDRATE
 #define CONFIG_BAUDRATE		115200
+#endif
+
+#ifndef CONFIG_BOOTDELAY
 #define CONFIG_BOOTDELAY	5	/* autoboot after 5 seconds	*/
+#endif
+
+#ifndef CONFIG_BOOTCOMMAND
 #define CONFIG_BOOTCOMMAND							\
 	"bootp; "								\
 	"setenv bootargs root=/dev/nfs nfsroot=${serverip}:${rootpath} "	\
 	"ip=${ipaddr}:${serverip}:${gatewayip}:${netmask}:${hostname}::off; "	\
 	"bootm"
+#endif
 
 extern int   fw_printenv(int argc, char *argv[]);
 extern char *fw_getenv  (char *name);

1.7.11.5

this doesn't make any sense. CONFIG_FILE is defined literally right above this check.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120822/993f3b22/attachment.pgp>

Hi Mike,

The comment right above that says:

/*
* To build the utility with the static configuration
* comment out the next line.
* See included "fw_env.config" sample file
* for notes on configuration.
*/

...so the reason for the #ifndef is for the case where you comment out that line. The other option is to just delete all of that stuff and the user can add it all back if they want to operate with no config file.

-Joe

The length included the name length, and then it was subtracted back out on each use. Now we don't include it in the first place.

---
 tools/env/fw_env.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

index e46791d..a461dbd 100644
@@ -484,22 +484,23 @@ int fw_setenv(int argc, char *argv[])
 
 	name = argv[1];
 
-	len = strlen(name) + 2;
+	len = 0;
 	for (i = 2; i < argc; ++i)
 		len += strlen(argv[i]) + 1;
 
 	/* Allocate enough place to the data string */
 	for (i = 2; i < argc; ++i) {
 		char *val = argv[i];
+
 		if (!value) {
-			value = (char *)malloc(len - strlen(name));
+			value = (char *)malloc(len);
 			if (!value) {
 				fprintf(stderr,
 				"Cannot malloc %zu bytes: %s\n",
-				len - strlen(name), strerror(errno));
+				len, strerror(errno));
 				return -1;
 			}
-			memset(value, 0, len - strlen(name));
+			memset(value, 0, len);
 			tmpval = value;
 		}
 		if (i != 2)

1.7.11.5

We will only call fw_getenv when the env has already been initialized.

---
 tools/env/fw_env.c | 3 ---
 1 file changed, 3 deletions(-)

index a461dbd..8bb7f9a 100644
@@ -247,9 +247,6 @@ char *fw_getenv (char *name)
 {
 	char *env, *nxt;
 
-	if (fw_env_open())
-		return NULL;
-
 	for (env = environment.data; *env; env = nxt + 1) {
 		char *val;
 

1.7.11.5

Modify fw_printenv to read in chunks of 0x20 at a time.

---
 tools/env/fw_env.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

index 8bb7f9a..9ecc22a 100644
@@ -45,6 +45,8 @@
 
 #include "fw_env.h"
 
+#define MAX_BYTES_PER_READ 0x20
+
 #define WHITESPACE(c) ((c == '\t') || (c == ' '))
 
 #define min(x, y) ({				\

@@ -671,6 +673,7 @@ static int flash_read_buf (int dev, int fd, void *buf, size_t count,
 	size_t blocklen;	/* erase / write length - one block on NAND,
 				   0 on NOR */
 	size_t processed = 0;	/* progress counter */
+	size_t bytesread = 0;	/* bytes read so far */
 	size_t readlen = count;	/* current read length */
 	off_t top_of_range;	/* end of the last block we may use */
 	off_t block_seek;	/* offset inside the current block to the start

@@ -730,11 +733,23 @@ static int flash_read_buf (int dev, int fd, void *buf, size_t count,
 		 */
 		lseek (fd, blockstart + block_seek, SEEK_SET);
 
-		rc = read (fd, buf + processed, readlen);
-		if (rc != readlen) {
-			fprintf (stderr, "Read error on %s: %s\n",
-				 DEVNAME (dev), strerror (errno));
-			return -1;
+		/*
+		* Break reads up into very small chunks so fw_printenv doesn't
+		* block the kernel long enough to starve other kernel tasks.
+		*/
+		bytesread = 0;
+		while (bytesread < readlen) {
+			size_t bytestoread = readlen - bytesread;
+
+			if (bytestoread > MAX_BYTES_PER_READ)
+				bytestoread = MAX_BYTES_PER_READ;
+			rc = read(fd, buf + processed + bytesread, bytestoread);
+			if (rc != bytestoread) {
+				fprintf(stderr, "Read error on %s: %s\n",
+					 DEVNAME(dev), strerror(errno));
+				return -1;
+			}
+			bytesread += bytestoread;
 		}
 #ifdef DEBUG
 		fprintf (stderr, "Read 0x%x bytes at 0x%llx\n",

1.7.11.5

Hi Mike,

The problem is something to do with the mtd driver blocking some resource on large reads that starves USB transfers. We never fully investigated it. It's true that this is a work-around. Is that so bad? Maybe so.

-Joe

i think it's a bad precedent that we shouldn't encourage. you add your wonky hardware workaround, and then the next guy will want to add his hack for their broken driver, and we're left with an ugly unmaintainable pile.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120823/9de1e230/attachment.pgp>

Use a lock file at /var/lock/fw_printenv.lock.
Avoids seriously confusing the MTD driver.

---
 tools/env/fw_env_main.c | 59 +++++++++++++++++++++++++++++++------------------
 1 file changed, 37 insertions(+), 22 deletions(-)

index c654057..c855f4c 100644
@@ -39,10 +39,13 @@
  *		  variable "name"
  */
 
+#include <fcntl.h>
+#include <getopt.h>
 #include <stdio.h>
 #include <string.h>
 #include <stdlib.h>
-#include <getopt.h>
+#include <sys/file.h>
+#include <unistd.h>
 #include "fw_env.h"
 
 #define	CMD_PRINTENV	"fw_printenv"

@@ -81,13 +84,27 @@ void usage(void)
 	);
 }
 
-int
-main(int argc, char *argv[])
+int main(int argc, char *argv[])
 {
 	char *p;
 	char *cmdname = *argv;
 	char *script_file = NULL;
 	int c;
+	const char *lockname = "/var/lock/" CMD_PRINTENV ".lock";
+	int lockfd = -1;
+	int retval = EXIT_SUCCESS;
+
+	lockfd = open(lockname, O_WRONLY | O_CREAT | O_TRUNC);
+	if (-1 == lockfd) {
+		fprintf(stderr, "Error opening lock file %s\n", lockname);
+		return EXIT_FAILURE;
+	}
+
+	if (-1 == flock(lockfd, LOCK_EX)) {
+		fprintf(stderr, "Error locking file %s\n", lockname);
+		close(lockfd);
+		return EXIT_FAILURE;
+	}
 
 	if ((p = strrchr (cmdname, '/')) != NULL) {
 		cmdname = p + 1;

@@ -104,38 +121,36 @@ main(int argc, char *argv[])
 			break;
 		case 'h':
 			usage();
-			return EXIT_SUCCESS;
+			goto exit;
 		default: /* '?' */
 			fprintf(stderr, "Try `%s --help' for more information."
 				"\n", cmdname);
-			return EXIT_FAILURE;
+			retval = EXIT_FAILURE;
+			goto exit;
 		}
 	}
 
-
 	if (strcmp(cmdname, CMD_PRINTENV) == 0) {
-
-		if (fw_printenv (argc, argv) != 0)
-			return EXIT_FAILURE;
-
-		return EXIT_SUCCESS;
-
+		if (fw_printenv(argc, argv) != 0)
+			retval = EXIT_FAILURE;
 	} else if (strcmp(cmdname, CMD_SETENV) == 0) {
 		if (!script_file) {
 			if (fw_setenv(argc, argv) != 0)
-				return EXIT_FAILURE;
+				retval = EXIT_FAILURE;
 		} else {
 			if (fw_parse_script(script_file) != 0)
-				return EXIT_FAILURE;
+				retval = EXIT_FAILURE;
 		}
-
-		return EXIT_SUCCESS;
-
+	} else {
+		fprintf(stderr,
+			"Identity crisis - may be called as `" CMD_PRINTENV
+			"' or as `" CMD_SETENV "' but not as `%s'\n",
+			cmdname);
+		retval = EXIT_FAILURE;
 	}
 
-	fprintf (stderr,
-		"Identity crisis - may be called as `" CMD_PRINTENV
-		"' or as `" CMD_SETENV "' but not as `%s'\n",
-		cmdname);
-	return EXIT_FAILURE;
+exit:
+	flock(lockfd, LOCK_UN);
+	close(lockfd);
+	return retval;
 }

1.7.11.5

the lock should be per-MTD, not per-system.

Hi Mike,

Sorry for the delay... I've been distracted.

I'm not sure that's the case. The idea is to prevent the tool from running more than once at a time. I don't think it matters which mtd is being accessed as we want all the mtd accesses across the whole operation to be atomic.

If you are talking about having two independent sets of environments accessed at the same time, then there are other reasons (such as the config file) that make that not possible for now, and it seems like it's an out-there use-case.

Again.. the thought it to be atomic across all accesses. We want to avoid the read / modify / write problems.

Thanks,
-Joe

The silent variable now updates the global data flag anytime it is changed as well as after the env relocation (in case its value is different from the default env in such cases as NAND env)

---
 common/cmd_nvedit.c | 16 ++++++++++++++++
 common/env_common.c |  8 ++++++++
 doc/README.silent   | 14 ++++++++++----
 3 files changed, 34 insertions(+), 4 deletions(-)

index fd05e72..bd52fa4 100644
@@ -303,9 +303,25 @@ int _do_env_set(int flag, int argc, char * const argv[])
 	/* Delete only ? */
 	if (argc < 3 || argv[2] == NULL) {
 		int rc = hdelete_r(name, &env_htab);
+
+#if defined(CONFIG_SILENT_CONSOLE) && \
+	defined(CONFIG_SILENT_CONSOLE_UPDATE_ON_SET)
+		if (strcmp(name, "silent") == 0) {
+			/* silent is deleted */
+			gd->flags &= ~GD_FLG_SILENT;
+		}
+#endif
 		return !rc;
 	}
 
+#if defined(CONFIG_SILENT_CONSOLE) && \
+	defined(CONFIG_SILENT_CONSOLE_UPDATE_ON_SET)
+	if (strcmp(name, "silent") == 0) {
+		/* silent is added */
+		gd->flags |= GD_FLG_SILENT;
+	}
+#endif
+
 	/*
 	 * Insert / replace new value
 	 */

index d9e990d..0c7e845 100644
@@ -245,6 +245,14 @@ void env_relocate(void)
 	} else {
 		env_relocate_spec();
 	}
+
+#if defined(CONFIG_SILENT_CONSOLE) && \
+	defined(CONFIG_SILENT_CONSOLE_UPDATE_ON_RELOC)
+	if (getenv("silent") != NULL)
+		gd->flags |= GD_FLG_SILENT;
+	else
+		gd->flags &= ~GD_FLG_SILENT;
+#endif
 }
 
 #ifdef CONFIG_AUTO_COMPLETE

index a26e3df..a04961f 100644
@@ -1,9 +1,15 @@
 The config option CONFIG_SILENT_CONSOLE can be used to quiet messages
 on the console.  If the option has been enabled, the output can be
-silenced by setting the environment variable "silent".  The variable
-is latched into the global data at an early stage in the boot process
-so deleting it with "setenv" will not take effect until the system is
-restarted.
+silenced by setting the environment variable "silent".
+
+- CONFIG_SILENT_CONSOLE_UPDATE_ON_SET
+	When the "silent" variable is changed with env set, the change
+	will take effect immediately
+
+- CONFIG_SILENT_CONSOLE_UPDATE_ON_RELOC
+	Some environments are not available until relocation (e.g. NAND)
+	so this will make the value in the flash env take effect at
+	relocation.
 
 The following actions are taken if "silent" is set at boot time:
 

1.7.11.5

After the env is relocated, make sure that the serial baudrate is applied in case it's different (such as the env_nand case).

---
 common/env_common.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

index 0c7e845..e58971d 100644
@@ -232,6 +232,10 @@ int env_import(const char *buf, int check)
 
 void env_relocate(void)
 {
+#ifdef CONFIG_BAUDRATE_UPDATE_ON_RELOC
+	const char *baudrate_str;
+#endif
+
 #if defined(CONFIG_NEEDS_MANUAL_RELOC)
 	env_reloc();
 #endif

@@ -253,6 +257,21 @@ void env_relocate(void)
 	else
 		gd->flags &= ~GD_FLG_SILENT;
 #endif
+
+#ifdef CONFIG_BAUDRATE_UPDATE_ON_RELOC
+	baudrate_str = getenv("baudrate");
+	if (baudrate_str != NULL) {
+		int baudrate = simple_strtoul(baudrate_str, NULL, 10);
+
+		if (baudrate != gd->baudrate) {
+			gd->baudrate = baudrate;
+#if defined(CONFIG_PPC) || defined(CONFIG_MCF52x2)
+			gd->bd->bi_baudrate = baudrate;
+#endif
+			serial_setbrg();
+		}
+	}
+#endif
 }
 
 #ifdef CONFIG_AUTO_COMPLETE

1.7.11.5

If the pointer passed into envmatch() is NULL, return -1 instead of crashing.

---
 common/cmd_nvedit.c | 3 +++
 tools/env/fw_env.c  | 2 ++
 2 files changed, 5 insertions(+)

index bd52fa4..76cdd87 100644
@@ -619,6 +619,9 @@ U_BOOT_CMD(
  */
 int envmatch(uchar *s1, int i2)
 {
+	if (s1 == NULL)
+		return -1;
+
 	while (*s1 == env_get_char(i2++))
 		if (*s1++ == '=')
 			return i2;

index 9ecc22a..23cf241 100644
@@ -1072,6 +1072,8 @@ exit:
 
 static char *envmatch (char * s1, char * s2)
 {
+	if (s1 == NULL || s2 == NULL)
+		return NULL;
 
 	while (*s1 == *s2++)
 		if (*s1++ == '=')

1.7.11.5

so don't pass in NULL ?
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120817/96547e5e/attachment.pgp>

Use variables that define the access type to the env. This will simplify validation of access with the ACL.

If deleting a variable that doesn't exist, do nothing

---
 common/cmd_nvedit.c | 101 +++++++++++++++++++++++++++-------------------------
 tools/env/fw_env.c  |  69 +++++++++++++++++------------------
 2 files changed, 88 insertions(+), 82 deletions(-)

index 76cdd87..0d5b957 100644
@@ -207,6 +207,7 @@ int _do_env_set(int flag, int argc, char * const argv[])
 	int   console = -1;
 	char  *name, *value, *s;
 	ENTRY e, *ep;
+	int deleting, creating, overwriting;
 
 	name = argv[1];
 

@@ -224,6 +225,10 @@ int _do_env_set(int flag, int argc, char * const argv[])
 	e.data = NULL;
 	hsearch_r(e, FIND, &ep, &env_htab);
 
+	deleting = (ep && ((argc < 3) || argv[2] == NULL));
+	creating = (!ep && ((argc >= 3) && argv[2] != NULL));
+	overwriting = (ep && ((argc >= 3) && argv[2] != NULL));
+
 	/* Check for console redirection */
 	if (strcmp(name, "stdin") == 0)
 		console = stdin;

@@ -233,7 +238,7 @@ int _do_env_set(int flag, int argc, char * const argv[])
 		console = stderr;
 
 	if (console != -1) {
-		if (argc < 3) {		/* Cannot delete it! */
+		if (deleting) {		/* Cannot delete it! */
 			printf("Can't delete \"%s\"\n", name);
 			return 1;
 		}

@@ -258,7 +263,7 @@ int _do_env_set(int flag, int argc, char * const argv[])
 	 * Some variables like "ethaddr" and "serial#" can be set only
 	 * once and cannot be deleted; also, "ver" is readonly.
 	 */
-	if (ep) {		/* variable exists */
+	if (deleting || overwriting) {		/* variable exists */
 #ifndef CONFIG_ENV_OVERWRITE
 		if (strcmp(name, "serial#") == 0 ||
 		    (strcmp(name, "ethaddr") == 0

@@ -270,6 +275,9 @@ int _do_env_set(int flag, int argc, char * const argv[])
 			return 1;
 		}
 #endif
+	}
+
+	if (creating || overwriting) {
 		/*
 		 * Switch to new baudrate if new baudrate is supported
 		 */

@@ -300,8 +308,7 @@ int _do_env_set(int flag, int argc, char * const argv[])
 		}
 	}
 
-	/* Delete only ? */
-	if (argc < 3 || argv[2] == NULL) {
+	if (deleting) {
 		int rc = hdelete_r(name, &env_htab);
 
 #if defined(CONFIG_SILENT_CONSOLE) && \

@@ -314,59 +321,57 @@ int _do_env_set(int flag, int argc, char * const argv[])
 		return !rc;
 	}
 
+	if (creating || overwriting) {
 #if defined(CONFIG_SILENT_CONSOLE) && \
 	defined(CONFIG_SILENT_CONSOLE_UPDATE_ON_SET)
-	if (strcmp(name, "silent") == 0) {
-		/* silent is added */
-		gd->flags |= GD_FLG_SILENT;
-	}
+		if (strcmp(name, "silent") == 0)
+			/* silent is added */
+			gd->flags |= GD_FLG_SILENT;
 #endif
 
-	/*
-	 * Insert / replace new value
-	 */
-	for (i = 2, len = 0; i < argc; ++i)
-		len += strlen(argv[i]) + 1;
+		for (i = 2, len = 0; i < argc; ++i)
+			len += strlen(argv[i]) + 1;
 
-	value = malloc(len);
-	if (value == NULL) {
-		printf("## Can't malloc %d bytes\n", len);
-		return 1;
-	}
-	for (i = 2, s = value; i < argc; ++i) {
-		char *v = argv[i];
+		value = malloc(len);
+		if (value == NULL) {
+			printf("## Can't malloc %d bytes\n", len);
+			return 1;
+		}
+		for (i = 2, s = value; i < argc; ++i) {
+			char *v = argv[i];
 
-		while ((*s++ = *v++) != '\0')
-			;
-		*(s - 1) = ' ';
-	}
-	if (s != value)
-		*--s = '\0';
-
-	e.key	= name;
-	e.data	= value;
-	hsearch_r(e, ENTER, &ep, &env_htab);
-	free(value);
-	if (!ep) {
-		printf("## Error inserting \"%s\" variable, errno=%d\n",
-			name, errno);
-		return 1;
-	}
+			while ((*s++ = *v++) != '\0')
+				continue;
+			*(s - 1) = ' ';
+		}
+		if (s != value)
+			*--s = '\0';
 
-	/*
-	 * Some variables should be updated when the corresponding
-	 * entry in the environment is changed
-	 */
-	if (strcmp(argv[1], "loadaddr") == 0) {
-		load_addr = simple_strtoul(argv[2], NULL, 16);
-		return 0;
-	}
+		e.key	= name;
+		e.data	= value;
+		hsearch_r(e, ENTER, &ep, &env_htab);
+		free(value);
+		if (!ep) {
+			printf("## Error inserting \"%s\" variable, errno=%d\n",
+				name, errno);
+			return 1;
+		}
+
+		/*
+		 * Some variables should be updated when the corresponding
+		 * entry in the environment is changed
+		 */
+		if (strcmp(argv[1], "loadaddr") == 0) {
+			load_addr = simple_strtoul(argv[2], NULL, 16);
+			return 0;
+		}
 #if defined(CONFIG_CMD_NET)
-	else if (strcmp(argv[1], "bootfile") == 0) {
-		copy_filename(BootFile, argv[2], sizeof(BootFile));
-		return 0;
-	}
+		else if (strcmp(argv[1], "bootfile") == 0) {
+			copy_filename(BootFile, argv[2], sizeof(BootFile));
+			return 0;
+		}
 #endif
+	}
 	return 0;
 }
 

1.7.11.5

Currently just validates variable types as decimal, hexidecimal, boolean, ip address, and mac address. Call env_acl_validate_setenv_params() from setenv() in both cmd_nvedit.c and fw_env.c.

If the entry is not found in the env ACL, then look in the static one. This allows the env to override the static definitions, but prevents the need to have every definition in the environment distracting you.

Move min macro from fw_env.c to fw_env.h... it is needed by env_acl.c when building for Linux.

Need to build in _ctype for isdigit for Linux.

---
 README                |  31 ++++++
 common/Makefile       |   1 +
 common/cmd_nvedit.c   |   8 ++
 common/env_acl.c      | 270 ++++++++++++++++++++++++++++++++++++++++++++++++++
 common/env_common.c   |   3 +
 common/env_embedded.c |   3 +
 include/env_acl.h     |  49 +++++++++
 tools/env/Makefile    |   2 +
 tools/env/fw_env.c    |  25 +++--
 tools/env/fw_env.h    |   6 ++
 10 files changed, 392 insertions(+), 6 deletions(-)
 create mode 100644 common/env_acl.c
 create mode 100644 include/env_acl.h

index fb9d904..2e34bcd 100644
@@ -2908,6 +2908,37 @@ Configuration Settings:
 	cases. This setting can be used to tune behaviour; see
 	lib/hashtable.c for details.
 
+- CONFIG_ENV_ACL
+	Enable validation of the values given to enviroment variables when
+	calling env set.  Variables can be restricted to only decimal,
+	hexadecimal, or boolean.  If CONFIG_CMD_NET is also defined,
+	the variables can also be restricted to IP address or MAC address.
+
+	The format of the list is:
+		type_attribute = [s|d|x|b|i|m]
+		attributes = type_attribute
+		entry = variable_name[:attributes]
+		list = entry[,list]
+
+	The type attributes are:
+		s - String (default)
+		d - Decimal
+		x - Hexadecimal
+		b - Boolean
+		i - IP address
+		m - MAC address
+
+	- CONFIG_ENV_ACL_DEFAULT
+		Define this to a list (string) to define the "acl" envirnoment
+		variable in the default or embedded environment.
+
+	- CONFIG_ENV_ACL_STATIC
+		Define this to a list (string) to define validation that
+		should be done if an entry is not found in the "acl"
+		environment variable.  To override a setting in the static
+		list, simply add an entry for the same variable name to the
+		"acl" variable.
+
 The following definitions that deal with the placement and management
 of environment data (variable area); in general, we support the
 following configurations:

index 3d62775..64522e1 100644
@@ -44,6 +44,7 @@ COBJS-y += cmd_version.o
 
 # environment
 COBJS-y += env_common.o
+COBJS-$(CONFIG_ENV_ACL) += env_acl.o
 COBJS-$(CONFIG_ENV_IS_IN_DATAFLASH) += env_dataflash.o
 COBJS-$(CONFIG_ENV_IS_IN_EEPROM) += env_eeprom.o
 XCOBJS-$(CONFIG_ENV_IS_EMBEDDED) += env_embedded.o

index 0d5b957..b761b2c 100644
@@ -43,6 +43,9 @@
 #include <common.h>
 #include <command.h>
 #include <environment.h>
+#if defined(CONFIG_ENV_ACL)
+#include <env_acl.h>
+#endif
 #include <search.h>
 #include <errno.h>
 #include <malloc.h>

@@ -217,6 +220,11 @@ int _do_env_set(int flag, int argc, char * const argv[])
 		return 1;
 	}
 
+#ifdef CONFIG_ENV_ACL
+	if (env_acl_validate_env_set_params(argc, argv) < 0)
+		return 1;
+#endif
+
 	env_id++;
 	/*
 	 * search if variable with this name already exists

index e58971d..c32c846 100644
@@ -127,6 +127,9 @@ const uchar default_environment[] = {
 	"soc="		CONFIG_SYS_SOC			"\0"
 #endif
 #endif
+#ifdef	CONFIG_ENV_ACL_DEFAULT
+	"acl="		CONFIG_ENV_ACL_DEFAULT		"\0"
+#endif
 #ifdef	CONFIG_EXTRA_ENV_SETTINGS
 	CONFIG_EXTRA_ENV_SETTINGS
 #endif

index 3872878..949fb95 100644
@@ -190,6 +190,9 @@ env_t environment __PPCENV__ = {
 	"soc="		CONFIG_SYS_SOC			"\0"
 #endif
 #endif
+#ifdef	CONFIG_ENV_ACL_DEFAULT
+	"acl="		CONFIG_ENV_ACL_DEFAULT		"\0"
+#endif
 #ifdef	CONFIG_EXTRA_ENV_SETTINGS
 	CONFIG_EXTRA_ENV_SETTINGS
 #endif

index ab73c8c..82b49a0 100644
@@ -24,12 +24,14 @@
 include $(TOPDIR)/config.mk
 
 HOSTSRCS := $(SRCTREE)/lib/crc32.c  fw_env.c  fw_env_main.c
+HOSTSRCS += $(SRCTREE)/lib/ctype.c $(SRCTREE)/common/env_acl.c
 HEADERS	:= fw_env.h $(OBJTREE)/include/config.h
 
 # Compile for a hosted environment on the target
 HOSTCPPFLAGS  = -idirafter $(SRCTREE)/include \
 		-idirafter $(OBJTREE)/include2 \
 		-idirafter $(OBJTREE)/include \
+		-idirafter $(SRCTREE)/tools/env \
 		-DUSE_HOSTCC \
 		-DTEXT_BASE=$(TEXT_BASE)
 

index a4cd179..f7807b5 100644
@@ -45,16 +45,14 @@
 
 #include "fw_env.h"
 
+#if defined(CONFIG_ENV_ACL)
+#include <env_acl.h>
+#endif
+
 #define MAX_BYTES_PER_READ 0x20
 
 #define WHITESPACE(c) ((c == '\t') || (c == ' '))
 
-#define min(x, y) ({				\
-	typeof(x) _min1 = (x);			\
-	typeof(y) _min2 = (y);			\
-	(void) (&_min1 == &_min2);		\
-	_min1 < _min2 ? _min1 : _min2; })
-
 struct envdev_s {
 	char devname[16];		/* Device name */
 	ulong devoff;			/* Device offset */

@@ -203,6 +201,9 @@ static char default_environment[] = {
 #if defined(CONFIG_PCI_BOOTDELAY) && (CONFIG_PCI_BOOTDELAY > 0)
 	"pcidelay=" MK_STR (CONFIG_PCI_BOOTDELAY) "\0"
 #endif
+#ifdef	CONFIG_ENV_ACL_DEFAULT
+	"acl=" CONFIG_ENV_ACL_DEFAULT "\0"
+#endif
 #ifdef  CONFIG_EXTRA_ENV_SETTINGS
 	CONFIG_EXTRA_ENV_SETTINGS
 #endif

@@ -484,6 +485,11 @@ int fw_setenv(int argc, char *argv[])
 
 	name = argv[1];
 
+#if defined(CONFIG_ENV_ACL)
+	if (env_acl_validate_env_set_params(argc, argv) < 0)
+		return 1;
+#endif
+
 	len = 0;
 	for (i = 2; i < argc; ++i)
 		len += strlen(argv[i]) + 1;

@@ -611,6 +617,13 @@ int fw_parse_script(char *fname)
 			name, val ? val : " removed");
 #endif
 
+#if defined(CONFIG_ENV_ACL)
+		if (env_acl_validate_type(name, val) < 0) {
+			ret = -1;
+			break;
+		}
+#endif
+
 		/*
 		 * If there is an error setting a variable,
 		 * try to save the environment and returns an error

index a1a6807..eca168a 100644
@@ -77,3 +77,9 @@ extern int fw_env_write(char *name, char *value);
 extern int fw_env_close(void);
 
 extern unsigned	long  crc32	 (unsigned long, const unsigned char *, unsigned);
+
+#define min(x, y) ({				\
+	typeof(x) _min1 = (x);			\
+	typeof(y) _min2 = (y);			\
+	(void) (&_min1 == &_min2);		\
+	_min1 < _min2 ? _min1 : _min2; })

1.7.11.5

Dear Joe Hershberger,

I have to admid that I haven't tested your code at all, but here are a few comments anyway:

- It appears you will store all access related information in the ACL variable (or a copy of it), i. e. in linearized form. Are you not concerned that this will make variable access slow?

- Encoding type information in a variable named "acl" doesn;t look really elegant to me. Of course, chosing another name is a trivial thing to do.

- There have been discussions that it would be nice to have a "volatile" variable type, for example for variables like "filesize" or "loadaddr" - such variables would not be stored to the persistent storage by the "saveenv" command. Thi sis something that should be fairly wasy to add to your code.

- It would be nice if we could group variables; for example, assume we could define a group "network" which includes things like ethaddr, ipaddr, netmask, hostname etc. - and we could then do something like "env print -g network" to print all variables of such a group.

- We have the situation that certain variables need specific handling when they get assigned to - baudrate, ethaddr etc. come to mind. But the code to implement this is scattered all over the place, nd doesn't really scale.

I've been thinking about similar features for some time, but the implementation I had in mind was way different. This is what I had in mind:

- Like you, I wanted to use plain environment variables for persistent storage of all such information. The format you chose looks fine for me, too.

- I would like to flag these variables as special by using special names for them. All such special vailables would start with a dot, i. e. instead of "acl" I had planned to call this ".flags".

Normally "env print" etc. would not show such specal variables, but "env print -a" would.

Similarly, grous of variables could be defined in a ".groups" variable, etc.

- Instead of parsing the ".flags" variable again and again when accessing variables, my idea was to extend the hash table: so far, the struct entry (see "include/search.h") holds only pointers to the key (variable name) and data (variable value). I suggest to extend this struct:

* By adding a field "flags" (u32) we could easily encode access permission type information (like read-only, write-once, volatile, ...), and of course also type information (hex, dec, string, IP addr, MAC addr, ...).

* By adding a field "callbacks" (int foo(const char *)) we could register pointers to a function which gets called whenever the respective variable value gets changed.

We would still need a compile time mapping from strings to pointers, but at least this would allow to generalize and unify the existing code. For example, setting up a mapping of string "ethaddr" to function env_set_ethaddr() would allow to create something like a special variable

.callbacks=ethaddr:ethaddr,eth1addr,ath2addr;baudrate:baudrate;...

This way we could even enale user specific callbacks without need to modify any common code.

And any "env set" would just have to check if the callback function pointer is non-NULL ...

What do you think?

Best regards,

Wolfgang Denk

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
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
Too many people are ready to carry the stool when the piano needs  to
be moved.

Hi Wolfgang,

I guess you didn't see this the last time I sent it to you off list...

At least in my uses so far, there are not so many variables that it is noticeably slower.

For sure. I accept that this is only one of the things it does and it's not really access control in the typical sense (with users and what not).

I was one of the proponents of this when we last discussed this some years ago. I left it out because you convinced me I should just use other variables and overwrite the special ones in a script when I need to. I still think supporting a volatile variable would be cleaner than that. I'm glad you changed your mind.

I agree that would be nice.

I agree wholeheartedly.

OK... I like the prepended '.'. I'm not sure that "flags" is a very good name for something so specific from a user point of view, but I see why you chose it based on how you plan to store it in the hash.

That makes sense.

OK.

This seems like a good idea for the most part. Are you concerned that the hast table will take twice the space? Should this be optional?

One of the things I've run into with baudrate and silent specifically is wanting to control if the special callback is made on set and/or on env relocation, and/or on initial load, and/or on import. I think there should be a field in the flags for when to check the callback.

Would the user be able to define other callbacks somehow?

I think is sounds very nice. And it's not too far from what I've done. I'll look into implementing some of it.

Cheers,
-Joe

Dear Joe Hershberger,

I didn't receive any such reply, sorry. Do you have the message ID and/or exact timestanmp of your message?

Did you run any benchmarks?

Note that in the linearized form not only the variable count, but also to total size of their values plays a role.

I always try to listen to what people tell me :-)

The hash table lives in RAM only, which is usually an ample resource these days. I suggest to make the ".flags" support unconditional, but ".groups" support is probably not needed by many users, so this should be optional.

So the default configuration whould have only a 50% increase of the hash table size.

I don't see the need for this distinction, yet. My gut feeling is that the callback should be run when the varibale value in the hash table is being changed - relocation should play no role then, and initial import, "env set", "env import" (including variable deletion) should IMO all be handled the same.

I'd really like to make this code more simple.

Yes. Similar as he can add "private" U-Boot commands in his board specific code, he should be able to add "private" callbacks.

I was hoping for such a reply :-)

Thanks a lot in advance.

Best regards,

Wolfgang Denk

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
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
PLEASE NOTE: Some Quantum Physics Theories Suggest That When the Con-
sumer Is Not Directly Observing This Product, It May Cease  to  Exist
or Will Exist Only in a Vague and Undetermined State.

When a variable with a registered callback is inserted, deleted, or overwritten the callback is called and gives the system an opportunity to do something in response to the change. It also has the opportunuty to reject the change by returning non-zero.

The flags on variables can control their type as well as their allowed access.

The format of the list is:
type_attribute = [s|d|x|b|i|m] attributes = type_attribute entry = variable_name[:attributes] list = entry[,list]

The type attributes are:
s - String (default)
d - Decimal
x - Hexadecimal
b - Boolean ([1yYtT|0nNfF])
i - IP address
m - MAC address

The access attributes are:
a - Any (default)
r - Read-only
o - Write-once
c - Change-default

Changes in v3:
- Rebase onto Gerlando Falauto's env patches
- Refactor himport_r() and hsearch_r()'s parameters
- Split hdelete_r() into the core delete and the validation before delete
- Delete vars on failed insertion
- Use Marek's linker lists instead of implementing it directly
- Rebase onto latest master
- Add flags parameter to callbacks
- Implement reverse search in env_attr_lookup()
- Fix space skipping in env_attr_lookup()
- All errors coming back from hsearch_r() are no longer fatal. Don't abort import on failed ENTER action.
- Removed checkpatch.pl warning

Changes in v2:
- Added much-needed documentation
- Factored out prevch and nextch in env_attr_lookup()

Joe Hershberger (18):
Make linux kernel string funcs available to tools
env: Refactor do_apply to a flag
env: Consolidate common code in hsearch_r()
env: Refactor apply into change_ok
env: Use getenv_yesno() more generally
env: Hide '.' variables in env print by default
env: Add support for callbacks to environment vars
env: Add a command to view callbacks
env: Add a bootfile env handler
env: Add a baudrate env handler
env: Add a loadaddr env handler
env: Add a console env handler
env: Add a silent env handler
env: Add environment variable flags tools/env: Add environment variable flags support
env: Add a command to display details about env flags
env: Add support for access control to .flags
env: Handle write-once ethaddr and serial# generically

 README                          |  80 ++++++
 arch/arm/lib/board.c            |   4 +-
 arch/m68k/lib/board.c           |   3 +-
 arch/microblaze/lib/board.c     |   4 +-
 arch/powerpc/cpu/mpc85xx/mp.c   |   4 +-
 arch/powerpc/lib/board.c        |   9 +-
 arch/sparc/lib/board.c          |   3 +-
 board/Marvell/db64360/db64360.c |  10 +-
 board/Marvell/db64460/db64460.c |  10 +-
 board/esd/cpci750/cpci750.c     |  10 +-
 board/esd/pmc440/cmd_pmc440.c   |   2 +-
 board/gw8260/gw8260.c           |  10 +-
 board/prodrive/p3mx/p3mx.c      |  10 +-
 common/Makefile                 |   6 +
 common/cmd_nvedit.c             | 337 ++++++++++++------------
 common/console.c                |  67 +++++
 common/env_attr.c               | 229 +++++++++++++++++
 common/env_callback.c           | 132 ++++++++++
 common/env_common.c             |  60 +++--
 common/env_dataflash.c          |   2 +-
 common/env_eeprom.c             |   2 +-
 common/env_fat.c                |   2 +-
 common/env_flags.c              | 549 ++++++++++++++++++++++++++++++++++++++++
 common/env_flash.c              |   4 +-
 common/env_mmc.c                |   2 +-
 common/env_nand.c               |   4 +-
 common/env_nvram.c              |   2 +-
 common/env_onenand.c            |   2 +-
 common/env_sf.c                 |   4 +-
 common/image.c                  |  21 +-
 doc/README.silent               |  14 +-
 drivers/serial/serial.c         |  70 +++++
 include/common.h                |   5 +
 include/env_attr.h              |  55 ++++
 include/env_callback.h          |  75 ++++++
 include/env_default.h           |   8 +
 include/env_flags.h             | 172 +++++++++++++
 include/environment.h           |  15 +-
 include/image.h                 |   1 -
 include/linux/linux_string.h    |   8 +
 include/linux/string.h          |   5 +-
 include/search.h                |  37 ++-
 lib/Makefile                    |   1 +
 lib/hashtable.c                 | 244 ++++++++++++------
 lib/linux_string.c              |  51 ++++
 lib/string.c                    |  39 ---
 net/net.c                       |  49 ++--
 tools/env/Makefile              |   3 +
 tools/env/fw_env.c              |  92 +++++--
 49 files changed, 2093 insertions(+), 435 deletions(-)
 create mode 100644 common/env_attr.c
 create mode 100644 common/env_callback.c
 create mode 100644 common/env_flags.c
 create mode 100644 include/env_attr.h
 create mode 100644 include/env_callback.h
 create mode 100644 include/env_flags.h
 create mode 100644 include/linux/linux_string.h
 create mode 100644 lib/linux_string.c

1.7.11.5

isspace() and strim() are not in the typical user-mode string.h, so put them in a separate compilation unit so that they can be built into tools that need them independent of the other common string functions.

This allows code shared by u-boot and the linux user-mode tools to link.

---

 include/linux/linux_string.h |  8 +++++++
 include/linux/string.h       |  5 +----
 lib/Makefile                 |  1 +
 lib/linux_string.c           | 51 ++++++++++++++++++++++++++++++++++++++++++++
 lib/string.c                 | 39 ---------------------------------
 5 files changed, 61 insertions(+), 43 deletions(-)
 create mode 100644 include/linux/linux_string.h
 create mode 100644 lib/linux_string.c

new file mode 100644
index 0000000..192b4c9
@@ -0,0 +1,8 @@
+#ifndef _LINUX_LINUX_STRING_H_
+#define _LINUX_LINUX_STRING_H_
+
+extern char * skip_spaces(const char *);
+
+extern char *strim(char *);
+
+#endif

index 9a8cbc2..2a6e52f 100644
@@ -47,10 +47,7 @@ extern char * strchr(const char *,int);
 #ifndef __HAVE_ARCH_STRRCHR
 extern char * strrchr(const char *,int);
 #endif
-extern char * skip_spaces(const char *);
-
-extern char *strim(char *);
-
+#include <linux/linux_string.h>
 #ifndef __HAVE_ARCH_STRSTR
 extern char * strstr(const char *,const char *);
 #endif

index e44e045..5652986 100644
@@ -68,6 +68,7 @@ endif
 COBJS-y += crc32.o
 COBJS-y += ctype.o
 COBJS-y += div64.o
+COBJS-y += linux_string.o
 COBJS-y += string.o
 COBJS-y += time.o
 COBJS-$(CONFIG_BOOTP_PXE) += uuid.o

index c3ad055..2c4f0ec 100644
@@ -214,45 +214,6 @@ char * strrchr(const char * s, int c)
 }
 #endif
 
-
-/**
- * skip_spaces - Removes leading whitespace from @str.
- * @str: The string to be stripped.
- *
- * Returns a pointer to the first non-whitespace character in @str.
- */
-char *skip_spaces(const char *str)
-{
-	while (isspace(*str))
-		++str;
-	return (char *)str;
-}
-
-/**
- * strim - Removes leading and trailing whitespace from @s.
- * @s: The string to be stripped.
- *
- * Note that the first trailing whitespace is replaced with a %NUL-terminator
- * in the given string @s. Returns a pointer to the first non-whitespace
- * character in @s.
- */
-char *strim(char *s)
-{
-	size_t size;
-	char *end;
-
-	s = skip_spaces(s);
-	size = strlen(s);
-	if (!size)
-		return s;
-
-	end = s + size - 1;
-	while (end >= s && isspace(*end))
-		end--;
-	*(end + 1) = '\0';
-
-	return s;
-}
 #ifndef __HAVE_ARCH_STRLEN
 /**
  * strlen - Find the length of a string

1.7.11.5

Use a flag in hsearch_r for insert mode passed from import to allow the behavior be different based on use.

---
Changes in v3:
- Rebase onto Gerlando Falauto's env patches
- Refactor himport_r() and hsearch_r()'s parameters

 common/cmd_nvedit.c | 17 ++++++++---------
 common/env_common.c | 26 ++++++++------------------
 include/search.h    | 15 ++++++++-------
 lib/hashtable.c     | 37 ++++++++++++++++++++-----------------
 4 files changed, 44 insertions(+), 51 deletions(-)

index 68c38f4..4820008 100644
@@ -116,7 +116,7 @@ static int env_print(char *name)
 
 		e.key = name;
 		e.data = NULL;
-		hsearch_r(e, FIND, &ep, &env_htab);
+		hsearch_r(e, FIND, &ep, &env_htab, 0);
 		if (ep == NULL)
 			return 0;
 		len = printf("%s=%s\n", ep->key, ep->data);

@@ -343,20 +343,19 @@ int _do_env_set(int flag, int argc, char * const argv[])
 	 */
 	e.key = name;
 	e.data = NULL;
-	hsearch_r(e, FIND, &ep, &env_htab);
+	hsearch_r(e, FIND, &ep, &env_htab, 0);
 
 	/*
-	 * Perform requested checks. Notice how since we are overwriting
-	 * a single variable, we need to set H_NOCLEAR
+	 * Perform requested checks.
 	 */
-	if (env_check_apply(name, ep ? ep->data : NULL, value, H_NOCLEAR)) {
+	if (env_check_apply(name, ep ? ep->data : NULL, value, 0)) {
 		debug("check function did not approve, refusing\n");
 		return 1;
 	}
 
 	/* Delete only ? */
 	if (argc < 3 || argv[2] == NULL) {
-		int rc = hdelete_r(name, &env_htab, 0);
+		int rc = hdelete_r(name, &env_htab, H_INTERACTIVE);
 		return !rc;
 	}
 

@@ -383,7 +382,7 @@ int _do_env_set(int flag, int argc, char * const argv[])
 
 	e.key	= name;
 	e.data	= value;
-	hsearch_r(e, ENTER, &ep, &env_htab);
+	hsearch_r(e, ENTER, &ep, &env_htab, H_INTERACTIVE);
 	free(value);
 	if (!ep) {
 		printf("## Error inserting \"%s\" variable, errno=%d\n",

@@ -550,7 +549,7 @@ char *getenv(const char *name)
 
 		e.key	= name;
 		e.data	= NULL;
-		hsearch_r(e, FIND, &ep, &env_htab);
+		hsearch_r(e, FIND, &ep, &env_htab, 0);
 
 		return ep ? ep->data : NULL;
 	}

@@ -948,7 +947,7 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag,
 	}
 
 	if (himport_r(&env_htab, addr, size, sep, del ? 0 : H_NOCLEAR,
-			0, NULL, 0 /* do_apply */) == 0) {
+			0, NULL) == 0) {
 		error("Environment import failed: errno = %d\n", errno);
 		return 1;
 	}

index 3d3cb70..f22f5b9 100644
@@ -83,11 +83,8 @@ const uchar *env_get_addr(int index)
 
 void set_default_env(const char *s)
 {
-	/*
-	 * By default, do not apply changes as they will eventually
-	 * be applied by someone else
-	 */
-	int do_apply = 0;
+	int flags = 0;
+
 	if (sizeof(default_environment) > ENV_SIZE) {
 		puts("*** Error - default environment is too large\n\n");
 		return;

@@ -99,14 +96,7 @@ void set_default_env(const char *s)
 				"using default environment\n\n",
 				s + 1);
 		} else {
-			/*
-			 * This set_to_default was explicitly asked for
-			 * by the user, as opposed to being a recovery
-			 * mechanism.  Therefore we check every single
-			 * variable and apply changes to the system
-			 * right away (e.g. baudrate, console).
-			 */
-			do_apply = 1;
+			flags = H_INTERACTIVE;
 			puts(s);
 		}
 	} else {

@@ -114,8 +104,8 @@ void set_default_env(const char *s)
 	}
 
 	if (himport_r(&env_htab, (char *)default_environment,
-			sizeof(default_environment), '\0', 0,
-			0, NULL, do_apply) == 0)
+			sizeof(default_environment), '\0', flags,
+			0, NULL) == 0)
 		error("Environment import failed: errno = %d\n", errno);
 
 	gd->flags |= GD_FLG_ENV_READY;

@@ -130,8 +120,8 @@ int set_default_vars(int nvars, char * const vars[])
 	 * (and use \0 as a separator)
 	 */
 	return himport_r(&env_htab, (const char *)default_environment,
-				sizeof(default_environment), '\0', H_NOCLEAR,
-				nvars, vars, 1 /* do_apply */);
+				sizeof(default_environment), '\0',
+				H_NOCLEAR | H_INTERACTIVE, nvars, vars);
 }
 
 #ifndef CONFIG_SPL_BUILD

@@ -155,7 +145,7 @@ int env_import(const char *buf, int check)
 	}
 
 	if (himport_r(&env_htab, (char *)ep->data, ENV_SIZE, '\0', 0,
-			0, NULL, 0 /* do_apply */)) {
+			0, NULL)) {
 		gd->flags |= GD_FLG_ENV_READY;
 		return 1;
 	}

index 93e1cbc..f5165b0 100644
@@ -73,7 +73,7 @@ struct hsearch_data {
 extern int hcreate_r(size_t __nel, struct hsearch_data *__htab);
 
 /* Destroy current internal hashing table.  */
-extern void hdestroy_r(struct hsearch_data *__htab, int do_apply);
+extern void hdestroy_r(struct hsearch_data *__htab);
 
 /*
  * Search for entry matching ITEM.key in internal hash table.  If

@@ -82,7 +82,7 @@ extern void hdestroy_r(struct hsearch_data *__htab, int do_apply);
  * ITEM.data.
  * */
 extern int hsearch_r(ENTRY __item, ACTION __action, ENTRY ** __retval,
-		     struct hsearch_data *__htab);
+		     struct hsearch_data *__htab, int __flag);
 
 /*
  * Search for an entry matching `MATCH'.  Otherwise, Same semantics

@@ -99,7 +99,7 @@ extern int hstrstr_r(const char *__match, int __last_idx, ENTRY ** __retval,
 
 /* Search and delete entry matching ITEM.key in internal hash table. */
 extern int hdelete_r(const char *__key, struct hsearch_data *__htab,
-			int do_apply);
+		     int __flag);
 
 extern ssize_t hexport_r(struct hsearch_data *__htab,
 		     const char __sep, char **__resp, size_t __size,

@@ -113,10 +113,11 @@ extern ssize_t hexport_r(struct hsearch_data *__htab,
  */
 extern int himport_r(struct hsearch_data *__htab,
 		     const char *__env, size_t __size, const char __sep,
-		     int __flag, int nvars, char * const vars[], int do_apply);
+		     int __flag, int nvars, char * const vars[]);
 
-/* Flags for himport_r() */
-#define	H_NOCLEAR	(1 << 0) /* do not clear hash table before importing */
-#define	H_FORCE		(1 << 1) /* overwrite read-only/write-once variables */
+/* Flags for himport_r(), hdelete_r(), and hsearch_r() */
+#define H_NOCLEAR	(1 << 0) /* do not clear hash table before importing */
+#define H_FORCE		(1 << 1) /* overwrite read-only/write-once variables */
+#define H_INTERACTIVE	(1 << 2) /* indicate that an import is user directed */
 
 #endif /* search.h */

index 94a7b61..f0056ac 100644
@@ -142,7 +142,7 @@ int hcreate_r(size_t nel, struct hsearch_data *htab)
  * be freed and the local static variable can be marked as not used.
  */
 
-void hdestroy_r(struct hsearch_data *htab, int do_apply)
+void hdestroy_r(struct hsearch_data *htab)
 {
 	int i;
 

@@ -156,10 +156,7 @@ void hdestroy_r(struct hsearch_data *htab, int do_apply)
 	for (i = 1; i <= htab->size; ++i) {
 		if (htab->table[i].used > 0) {
 			ENTRY *ep = &htab->table[i].entry;
-			if (do_apply && htab->apply != NULL) {
-				/* deletion is always forced */
-				htab->apply(ep->key, ep->data, NULL, H_FORCE);
-			}
+
 			free((void *)ep->key);
 			free(ep->data);
 		}

@@ -251,7 +248,7 @@ int hmatch_r(const char *match, int last_idx, ENTRY ** retval,
 }
 
 int hsearch_r(ENTRY item, ACTION action, ENTRY ** retval,
-	      struct hsearch_data *htab)
+	      struct hsearch_data *htab, int flag)
 {
 	unsigned int hval;
 	unsigned int count;

@@ -404,7 +401,7 @@ int hsearch_r(ENTRY item, ACTION action, ENTRY ** retval,
  * do that.
  */
 
-int hdelete_r(const char *key, struct hsearch_data *htab, int do_apply)
+int hdelete_r(const char *key, struct hsearch_data *htab, int flag)
 {
 	ENTRY e, *ep;
 	int idx;

@@ -413,15 +410,21 @@ int hdelete_r(const char *key, struct hsearch_data *htab, int do_apply)
 
 	e.key = (char *)key;
 
-	if ((idx = hsearch_r(e, FIND, &ep, htab)) == 0) {
+	idx = hsearch_r(e, FIND, &ep, htab, 0);
+	if (idx == 0) {
 		__set_errno(ESRCH);
 		return 0;	/* not found */
 	}
 
+	/* Check for permission */
+	if (htab->apply != NULL &&
+	    htab->apply(ep->key, ep->data, NULL, flag)) {
+		__set_errno(EPERM);
+		return 0;
+	}
+
 	/* free used ENTRY */
 	debug("hdelete: DELETING key \"%s\"\n", key);
-	if (do_apply && htab->apply != NULL)
-		htab->apply(ep->key, ep->data, NULL, H_FORCE);
 	free((void *)ep->key);
 	free(ep->data);
 	htab->table[idx].used = -1;

@@ -674,7 +677,7 @@ static int drop_var_from_set(const char *name, int nvars, char * vars[])
 
 int himport_r(struct hsearch_data *htab,
 		const char *env, size_t size, const char sep, int flag,
-		int nvars, char * const vars[], int do_apply)
+		int nvars, char * const vars[])
 {
 	char *data, *sp, *dp, *name, *value;
 	char *localvars[nvars];

@@ -704,7 +707,7 @@ int himport_r(struct hsearch_data *htab,
 		debug("Destroy Hash Table: %p table = %p\n", htab,
 		       htab->table);
 		if (htab->table)
-			hdestroy_r(htab, do_apply);
+			hdestroy_r(htab);
 	}
 
 	/*

@@ -770,7 +773,7 @@ int himport_r(struct hsearch_data *htab,
 			if (!drop_var_from_set(name, nvars, localvars))
 				continue;
 
-			if (hdelete_r(name, htab, do_apply) == 0)
+			if (hdelete_r(name, htab, flag) == 0)
 				debug("DELETE ERROR ##############################\n");
 
 			continue;

@@ -795,14 +798,14 @@ int himport_r(struct hsearch_data *htab,
 		e.data = value;
 
 		/* if there is an apply function, check what it has to say */
-		if (do_apply && htab->apply != NULL) {
+		if (htab->apply != NULL) {
 			debug("searching before calling cb function"
 				" for  %s\n", name);
 			/*
 			 * Search for variable in existing env, so to pass
 			 * its previous value to the apply callback
 			 */
-			hsearch_r(e, FIND, &rv, htab);
+			hsearch_r(e, FIND, &rv, htab, 0);
 			debug("previous value was %s\n", rv ? rv->data : "");
 			if (htab->apply(name, rv ? rv->data : NULL,
 				value, flag)) {

@@ -812,7 +815,7 @@ int himport_r(struct hsearch_data *htab,
 			}
 		}
 
-		hsearch_r(e, ENTER, &rv, htab);
+		hsearch_r(e, ENTER, &rv, htab, flag);
 		if (rv == NULL) {
 			printf("himport_r: can't insert \"%s=%s\" into hash table\n",
 				name, value);

@@ -839,7 +842,7 @@ int himport_r(struct hsearch_data *htab,
 		 * b) if the variable was not present in current env, we notify
 		 *    it might be a typo
 		 */
-		if (hdelete_r(localvars[i], htab, do_apply) == 0)
+		if (hdelete_r(localvars[i], htab, flag) == 0)
 			printf("WARNING: '%s' neither in running nor in imported env!\n", localvars[i]);
 		else
 			printf("WARNING: '%s' not in imported env, deleting it!\n", localvars[i]);

1.7.11.5

The same chunk of code was replicated in two places and the following changes will make that chunk grow a bit, so combine into a static func.

---

 lib/hashtable.c | 71 ++++++++++++++++++++++++++++++---------------------------
 1 file changed, 37 insertions(+), 34 deletions(-)

index f0056ac..f4d5795 100644
@@ -247,6 +247,34 @@ int hmatch_r(const char *match, int last_idx, ENTRY ** retval,
 	return 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().
+ */
+static inline int _compare_and_overwrite_entry(ENTRY item, ACTION action,
+	ENTRY **retval, struct hsearch_data *htab, int flag,
+	unsigned int hval, unsigned int idx)
+{
+	if (htab->table[idx].used == hval
+	    && strcmp(item.key, htab->table[idx].entry.key) == 0) {
+		/* Overwrite existing value? */
+		if ((action == ENTER) && (item.data != NULL)) {
+			free(htab->table[idx].entry.data);
+			htab->table[idx].entry.data = strdup(item.data);
+			if (!htab->table[idx].entry.data) {
+				__set_errno(ENOMEM);
+				*retval = NULL;
+				return 0;
+			}
+		}
+		/* return found entry */
+		*retval = &htab->table[idx].entry;
+		return idx;
+	}
+	/* keep searching */
+	return -1;
+}
+
 int hsearch_r(ENTRY item, ACTION action, ENTRY ** retval,
 	      struct hsearch_data *htab, int flag)
 {

@@ -255,6 +283,7 @@ int hsearch_r(ENTRY item, ACTION action, ENTRY ** retval,
 	unsigned int len = strlen(item.key);
 	unsigned int idx;
 	unsigned int first_deleted = 0;
+	int ret;
 
 	/* Compute an value for the given string. Perhaps use a better method. */
 	hval = len;

@@ -286,23 +315,10 @@ int hsearch_r(ENTRY item, ACTION action, ENTRY ** retval,
 		    && !first_deleted)
 			first_deleted = idx;
 
-		if (htab->table[idx].used == hval
-		    && strcmp(item.key, htab->table[idx].entry.key) == 0) {
-			/* Overwrite existing value? */
-			if ((action == ENTER) && (item.data != NULL)) {
-				free(htab->table[idx].entry.data);
-				htab->table[idx].entry.data =
-					strdup(item.data);
-				if (!htab->table[idx].entry.data) {
-					__set_errno(ENOMEM);
-					*retval = NULL;
-					return 0;
-				}
-			}
-			/* return found entry */
-			*retval = &htab->table[idx].entry;
-			return idx;
-		}
+		ret = _compare_and_overwrite_entry(item, action, retval, htab,
+			flag, hval, idx);
+		if (ret != -1)
+			return ret;
 
 		/*
 		 * Second hash function:

@@ -328,23 +344,10 @@ int hsearch_r(ENTRY item, ACTION action, ENTRY ** retval,
 				break;
 
 			/* If entry is found use it. */
-			if ((htab->table[idx].used == hval)
-			    && strcmp(item.key, htab->table[idx].entry.key) == 0) {
-				/* Overwrite existing value? */
-				if ((action == ENTER) && (item.data != NULL)) {
-					free(htab->table[idx].entry.data);
-					htab->table[idx].entry.data =
-						strdup(item.data);
-					if (!htab->table[idx].entry.data) {
-						__set_errno(ENOMEM);
-						*retval = NULL;
-						return 0;
-					}
-				}
-				/* return found entry */
-				*retval = &htab->table[idx].entry;
-				return idx;
-			}
+			ret = _compare_and_overwrite_entry(item, action, retval,
+				htab, flag, hval, idx);
+			if (ret != -1)
+				return ret;
 		}
 		while (htab->table[idx].used);
 	}

1.7.11.5

Move the read of the old value to inside the check function. In some cases it can be avoided all together and at the least the code is only called from one place.

Also name the function and the callback to more clearly describe what it does.

Pass the ENTRY instead of just the name for direct access to the whole data structure.

Pass an enum to the callback that specifies the operation being approved.

---
Changes in v3:
- Split hdelete_r() into the core delete and the validation before delete
- Delete vars on failed insertion

 common/cmd_nvedit.c   | 34 +++++++++++--------------
 common/env_common.c   |  2 +-
 include/environment.h |  7 +++---
 include/search.h      | 13 +++++++---
 lib/hashtable.c       | 70 +++++++++++++++++++++++++++++++--------------------
 5 files changed, 71 insertions(+), 55 deletions(-)

index 4820008..119796b 100644
@@ -207,10 +207,20 @@ static int do_env_grep(cmd_tbl_t *cmdtp, int flag,
  * overwriting of write-once variables.
  */
 
-int env_check_apply(const char *name, const char *oldval,
-			const char *newval, int flag)
+int env_change_ok(const ENTRY *item, const char *newval, enum env_op op,
+	int flag)
 {
 	int   console = -1;
+	const char *name;
+#if !defined(CONFIG_ENV_OVERWRITE) && defined(CONFIG_OVERWRITE_ETHADDR_ONCE) \
+&& defined(CONFIG_ETHADDR)
+	const char *oldval = NULL;
+
+	if (op != env_op_create)
+		oldval = item->data;
+#endif
+
+	name = item->key;
 
 	/* Default value for NULL to protect string-manipulating functions */
 	newval = newval ? : "";

@@ -241,12 +251,12 @@ int env_check_apply(const char *name, const char *oldval,
 #endif /* CONFIG_CONSOLE_MUX */
 	}
 
+#ifndef CONFIG_ENV_OVERWRITE
 	/*
 	 * Some variables like "ethaddr" and "serial#" can be set only once and
 	 * cannot be deleted, unless CONFIG_ENV_OVERWRITE is defined.
 	 */
-#ifndef CONFIG_ENV_OVERWRITE
-	if (oldval != NULL &&			/* variable exists */
+	if (op != env_op_create &&		/* variable exists */
 		(flag & H_FORCE) == 0) {	/* and we are not forced */
 		if (strcmp(name, "serial#") == 0 ||
 		    (strcmp(name, "ethaddr") == 0

@@ -264,7 +274,7 @@ int env_check_apply(const char *name, const char *oldval,
 	 * (which will erase all variables prior to calling this),
 	 * we want the baudrate to actually change - for real.
 	 */
-	if (oldval != NULL ||			/* variable exists */
+	if (op != env_op_create ||		/* variable exists */
 		(flag & H_NOCLEAR) == 0) {	/* or env is clear */
 		/*
 		 * Switch to new baudrate if new baudrate is supported

@@ -338,20 +348,6 @@ int _do_env_set(int flag, int argc, char * const argv[])
 	}
 
 	env_id++;
-	/*
-	 * search if variable with this name already exists
-	 */
-	e.key = name;
-	e.data = NULL;
-	hsearch_r(e, FIND, &ep, &env_htab, 0);
-
-	/*
-	 * Perform requested checks.
-	 */
-	if (env_check_apply(name, ep ? ep->data : NULL, value, 0)) {
-		debug("check function did not approve, refusing\n");
-		return 1;
-	}
 
 	/* Delete only ? */
 	if (argc < 3 || argv[2] == NULL) {

index f22f5b9..919f535 100644
@@ -40,7 +40,7 @@ DECLARE_GLOBAL_DATA_PTR;
 #include <env_default.h>
 
 struct hsearch_data env_htab = {
-	.apply = env_check_apply,
+	.change_ok = env_change_ok,
 };
 
 static uchar __env_get_char_spec(int index)

index e8ab703..4b19f32 100644
@@ -188,13 +188,12 @@ int set_default_vars(int nvars, char * const vars[]);
 int env_import(const char *buf, int check);
 
 /*
- * Check if variable "name" can be changed from oldval to newval,
- * and if so, apply the changes (e.g. baudrate).
+ * Check if variable "item" can be changed to newval
  * When (flag & H_FORCE) is set, it does not print out any error
  * message and forces overwriting of write-once variables.
  */
-int env_check_apply(const char *name, const char *oldval,
-			const char *newval, int flag);
+int env_change_ok(const ENTRY *item, const char *newval, enum env_op op,
+	int flag);
 
 #endif /* DO_DEPS_ONLY */
 

index f5165b0..fa00ea1 100644
@@ -32,6 +32,12 @@
 
 #define __set_errno(val) do { errno = val; } while (0)
 
+enum env_op {
+	env_op_create,
+	env_op_delete,
+	env_op_overwrite,
+};
+
 /* Action which shall be performed in the call the hsearch.  */
 typedef enum {
 	FIND,

@@ -59,14 +65,13 @@ struct hsearch_data {
 	unsigned int filled;
 /*
  * Callback function which will check whether the given change for variable
- * "name" from "oldval" to "newval" may be applied or not, and possibly apply
- * such change.
+ * "item" to "newval" may be applied or not, and possibly apply such change.
  * When (flag & H_FORCE) is set, it shall not print out any error message and
  * shall force overwriting of write-once variables.
 .* Must return 0 for approval, 1 for denial.
  */
-	int (*apply)(const char *name, const char *oldval,
-			const char *newval, int flag);
+	int (*change_ok)(const ENTRY *__item, const char *newval, enum env_op,
+		int flag);
 };
 
 /* Create a new hashing table which will at most contain NEL elements.  */

1.7.11.5

Move the getenv_yesno() to env_common.c and change most checks for 'y' or 'n' to use this helper.

---

 arch/arm/lib/board.c            |  4 +---
 arch/m68k/lib/board.c           |  3 +--
 arch/microblaze/lib/board.c     |  4 +---
 arch/powerpc/cpu/mpc85xx/mp.c   |  4 +---
 arch/powerpc/lib/board.c        |  9 ++-------
 arch/sparc/lib/board.c          |  3 +--
 board/Marvell/db64360/db64360.c | 10 +++-------
 board/Marvell/db64460/db64460.c | 10 +++-------
 board/esd/cpci750/cpci750.c     | 10 +++-------
 board/gw8260/gw8260.c           | 10 +++-------
 board/prodrive/p3mx/p3mx.c      | 10 +++-------
 common/env_common.c             | 14 ++++++++++++++
 common/image.c                  |  6 ------
 include/common.h                |  5 +++++
 include/image.h                 |  1 -
 net/net.c                       | 32 +++++++++++++++-----------------
 16 files changed, 56 insertions(+), 79 deletions(-)

index 92cad9a..2e7a9b5 100644
@@ -532,15 +532,13 @@ void board_init_r(gd_t *id, ulong dest_addr)
 	flash_size = flash_init();
 	if (flash_size > 0) {
 # ifdef CONFIG_SYS_FLASH_CHECKSUM
-		char *s = getenv("flashchecksum");
-
 		print_size(flash_size, "");
 		/*
 		 * Compute and print flash CRC if flashchecksum is set to 'y'
 		 *
 		 * NOTE: Maybe we should add some WATCHDOG_RESET()? XXX
 		 */
-		if (s && (*s == 'y')) {
+		if (getenv_yesno("flashchecksum") == 1) {
 			printf("  CRC: %08X", crc32(0,
 				(const unsigned char *) CONFIG_SYS_FLASH_BASE,
 				flash_size));

index 02d73fd..794b867 100644
@@ -462,8 +462,7 @@ void board_init_r (gd_t *id, ulong dest_addr)
 		 *
 		 * NOTE: Maybe we should add some WATCHDOG_RESET()? XXX
 		 */
-		s = getenv ("flashchecksum");
-		if (s && (*s == 'y')) {
+		if (getenv_yesno("flashchecksum") == 1) {
 			printf ("  CRC: %08X",
 					crc32 (0,
 						   (const unsigned char *) CONFIG_SYS_FLASH_BASE,

index efd63cd..a7c2f76 100644
@@ -74,7 +74,6 @@ void board_init_f(ulong not_used)
 	gd = (gd_t *) (CONFIG_SYS_SDRAM_BASE + CONFIG_SYS_GBL_DATA_OFFSET);
 	bd = (bd_t *) (CONFIG_SYS_SDRAM_BASE + CONFIG_SYS_GBL_DATA_OFFSET \
 						- GENERATED_BD_INFO_SIZE);
-	__maybe_unused char *s;
 #if defined(CONFIG_CMD_FLASH)
 	ulong flash_size = 0;
 #endif

@@ -143,8 +142,7 @@ void board_init_f(ulong not_used)
 		 *
 		 * NOTE: Maybe we should add some WATCHDOG_RESET()? XXX
 		 */
-		s = getenv ("flashchecksum");
-		if (s && (*s == 'y')) {
+		if (getenv_yesno("flashchecksum") == 1) {
 			printf ("  CRC: %08X",
 				crc32(0, (const u8 *)bd->bi_flashstart,
 							flash_size)

index e1197ac..43d4836 100644
@@ -46,10 +46,8 @@ u32 get_my_id()
  */
 int hold_cores_in_reset(int verbose)
 {
-	const char *s = getenv("mp_holdoff");
-
 	/* Default to no, overriden by 'y', 'yes', 'Y', 'Yes', or '1' */
-	if (s && (*s == 'y' || *s == 'Y' || *s == '1')) {
+	if (getenv_yesno("mp_holdoff") == 1) {
 		if (verbose) {
 			puts("Secondary cores are being held in reset.\n");
 			puts("See 'mp_holdoff' environment variable\n");

index ebf4008..b918c01 100644
@@ -739,16 +739,13 @@ void board_init_r(gd_t *id, ulong dest_addr)
 		flash_size = 0;
 	} else if ((flash_size = flash_init()) > 0) {
 #ifdef CONFIG_SYS_FLASH_CHECKSUM
-		char *s;
-
 		print_size(flash_size, "");
 		/*
 		 * Compute and print flash CRC if flashchecksum is set to 'y'
 		 *
 		 * NOTE: Maybe we should add some WATCHDOG_RESET()? XXX
 		 */
-		s = getenv("flashchecksum");
-		if (s && (*s == 'y')) {
+		if (getenv_yesno("flashchecksum") == 1) {
 			printf("  CRC: %08X",
 			       crc32(0,
 				     (const unsigned char *)

@@ -841,9 +838,7 @@ void board_init_r(gd_t *id, ulong dest_addr)
 	 * "i2cfast" into account
 	 */
 	{
-		char *s = getenv("i2cfast");
-
-		if (s && ((*s == 'y') || (*s == 'Y'))) {
+		if (getenv_yesno("i2cfast") == 1) {
 			bd->bi_iic_fast[0] = 1;
 			bd->bi_iic_fast[1] = 1;
 		}

index 32d025a..1b5e995 100644
@@ -284,8 +284,7 @@ void board_init_f(ulong bootflag)
 		 *
 		 * NOTE: Maybe we should add some WATCHDOG_RESET()? XXX
 		 */
-		s = getenv("flashchecksum");
-		if (s && (*s == 'y')) {
+		if (getenv_yesno("flashchecksum") == 1) {
 			printf("  CRC: %08lX",
 			       crc32(0, (const unsigned char *)CONFIG_SYS_FLASH_BASE,
 				     flash_size)

index 6cae686..38769e0 100644
@@ -834,15 +834,11 @@ int mem_test_walk (void)
 /*********************************************************************/
 int testdram (void)
 {
-	char *s;
 	int rundata, runaddress, runwalk;
 
-	s = getenv ("testdramdata");
-	rundata = (s && (*s == 'y')) ? 1 : 0;
-	s = getenv ("testdramaddress");
-	runaddress = (s && (*s == 'y')) ? 1 : 0;
-	s = getenv ("testdramwalk");
-	runwalk = (s && (*s == 'y')) ? 1 : 0;
+	rundata = getenv_yesno("testdramdata") == 1;
+	runaddress = getenv_yesno("testdramaddress") == 1;
+	runwalk = getenv_yesno("testdramwalk") == 1;
 
 /*    rundata = 1; */
 /*    runaddress = 0; */

index d4f58b3..ddb7ed5 100644
@@ -834,15 +834,11 @@ int mem_test_walk (void)
 /*********************************************************************/
 int testdram (void)
 {
-	char *s;
 	int rundata, runaddress, runwalk;
 
-	s = getenv ("testdramdata");
-	rundata = (s && (*s == 'y')) ? 1 : 0;
-	s = getenv ("testdramaddress");
-	runaddress = (s && (*s == 'y')) ? 1 : 0;
-	s = getenv ("testdramwalk");
-	runwalk = (s && (*s == 'y')) ? 1 : 0;
+	rundata = getenv_yesno("testdramdata") == 1;
+	runaddress = getenv_yesno("testdramaddress") == 1;
+	runwalk = getenv_yesno("testdramwalk") == 1;
 
 /*    rundata = 1; */
 /*    runaddress = 0; */

index 98051fb..d7deae4 100644
@@ -953,22 +953,18 @@ int mem_test_walk (void)
 /*********************************************************************/
 int testdram (void)
 {
-	char *s;
 	int rundata    = 0;
 	int runaddress = 0;
 	int runwalk    = 0;
 
 #ifdef CONFIG_SYS_DRAM_TEST_DATA
-	s = getenv ("testdramdata");
-	rundata = (s && (*s == 'y')) ? 1 : 0;
+	rundata = getenv_yesno("testdramdata") == 1;
 #endif
 #ifdef CONFIG_SYS_DRAM_TEST_ADDRESS
-	s = getenv ("testdramaddress");
-	runaddress = (s && (*s == 'y')) ? 1 : 0;
+	runaddress = getenv_yesno("testdramaddress") == 1;
 #endif
 #ifdef CONFIG_SYS_DRAM_TEST_WALK
-	s = getenv ("testdramwalk");
-	runwalk = (s && (*s == 'y')) ? 1 : 0;
+	runwalk = getenv_yesno("testdramwalk") == 1;
 #endif
 
 	if ((rundata == 1) || (runaddress == 1) || (runwalk == 1)) {

index 77a1e1d..64c54d5 100644
@@ -544,15 +544,11 @@ int mem_test_walk (void)
 /*********************************************************************/
 int testdram (void)
 {
-	char *s;
 	int rundata, runaddress, runwalk;
 
-	s = getenv ("testdramdata");
-	rundata = (s && (*s == 'y')) ? 1 : 0;
-	s = getenv ("testdramaddress");
-	runaddress = (s && (*s == 'y')) ? 1 : 0;
-	s = getenv ("testdramwalk");
-	runwalk = (s && (*s == 'y')) ? 1 : 0;
+	rundata = getenv_yesno("testdramdata") == 1;
+	runaddress = getenv_yesno("testdramaddress") == 1;
+	runwalk = getenv_yesno("testdramwalk") == 1;
 
 	if ((rundata == 1) || (runaddress == 1) || (runwalk == 1)) {
 		printf ("Testing RAM ... ");

index 389affc..c3fd191 100644
@@ -768,22 +768,18 @@ int mem_test_walk (void)
 /*********************************************************************/
 int testdram (void)
 {
-	char *s;
 	int rundata    = 0;
 	int runaddress = 0;
 	int runwalk    = 0;
 
 #ifdef CONFIG_SYS_DRAM_TEST_DATA
-	s = getenv ("testdramdata");
-	rundata = (s && (*s == 'y')) ? 1 : 0;
+	rundata = getenv_yesno("testdramdata") == 1;
 #endif
 #ifdef CONFIG_SYS_DRAM_TEST_ADDRESS
-	s = getenv ("testdramaddress");
-	runaddress = (s && (*s == 'y')) ? 1 : 0;
+	runaddress = getenv_yesno("testdramaddress") == 1;
 #endif
 #ifdef CONFIG_SYS_DRAM_TEST_WALK
-	s = getenv ("testdramwalk");
-	runwalk = (s && (*s == 'y')) ? 1 : 0;
+	runwalk = getenv_yesno("testdramwalk") == 1;
 #endif
 
 	if ((rundata == 1) || (runaddress == 1) || (runwalk == 1))

index 919f535..a86f061 100644
@@ -81,6 +81,20 @@ const uchar *env_get_addr(int index)
 		return &default_environment[index];
 }
 
+/*
+ * Read an environment variable as a boolean
+ * Return -1 if variable does not exist (default to true)
+ */
+int getenv_yesno(const char *var)
+{
+	char *s = getenv(var);
+
+	if (s == NULL)
+		return -1;
+	return (*s == '1' || *s == 'y' || *s == 'Y' || *s == 't' || *s == 'T') ?
+		1 : 0;
+}
+
 void set_default_env(const char *s)
 {
 	int flags = 0;

index 750a98b..60428c7 100644
@@ -416,12 +416,6 @@ static const image_header_t *image_get_ramdisk(ulong rd_addr, uint8_t arch,
 /* Shared dual-format routines */
 /*****************************************************************************/
 #ifndef USE_HOSTCC
-int getenv_yesno(char *var)
-{
-	char *s = getenv(var);
-	return (s && (*s == 'n')) ? 0 : 1;
-}
-
 ulong getenv_bootm_low(void)
 {
 	char *s = getenv("bootm_low");

index b23e90b..ebd4bff 100644
@@ -340,6 +340,11 @@ int	envmatch     (uchar *, int);
 char	*getenv	     (const char *);
 int	getenv_f     (const char *name, char *buf, unsigned len);
 ulong getenv_ulong(const char *name, int base, ulong default_val);
+/*
+ * Read an environment variable as a boolean
+ * Return -1 if variable does not exist (default to true)
+ */
+int getenv_yesno(const char *var);
 int	saveenv	     (void);
 #ifdef CONFIG_PPC		/* ARM version to be fixed! */
 int inline setenv    (const char *, const char *);

index 4e5863f..544334c 100644
@@ -459,7 +459,6 @@ static inline void image_set_name(image_header_t *hdr, const char *name)
 int image_check_hcrc(const image_header_t *hdr);
 int image_check_dcrc(const image_header_t *hdr);
 #ifndef USE_HOSTCC
-int getenv_yesno(char *var);
 ulong getenv_bootm_low(void);
 phys_size_t getenv_bootm_size(void);
 phys_size_t getenv_bootm_mapsize(void);

index 569fec4..c1fe00a 100644
@@ -214,26 +214,24 @@ static int NetTryCount;
  */
 void net_auto_load(void)
 {
+#if defined(CONFIG_CMD_NFS)
 	const char *s = getenv("autoload");
 
-	if (s != NULL) {
-		if (*s == 'n') {
-			/*
-			 * Just use BOOTP/RARP to configure system;
-			 * Do not use TFTP to load the bootfile.
-			 */
-			net_set_state(NETLOOP_SUCCESS);
-			return;
-		}
-#if defined(CONFIG_CMD_NFS)
-		if (strcmp(s, "NFS") == 0) {
-			/*
-			 * Use NFS to load the bootfile.
-			 */
-			NfsStart();
-			return;
-		}
+	if (s != NULL && strcmp(s, "NFS") == 0) {
+		/*
+		 * Use NFS to load the bootfile.
+		 */
+		NfsStart();
+		return;
+	}
 #endif
+	if (getenv_yesno("autoload") == 0) {
+		/*
+		 * Just use BOOTP/RARP to configure system;
+		 * Do not use TFTP to load the bootfile.
+		 */
+		net_set_state(NETLOOP_SUCCESS);
+		return;
 	}
 	TftpStart(TFTPGET);
 }

1.7.11.5

When printing all variables with env print, don't print variables that begin with '.'. If env print is called with a '-a' switch, then include variables that begin with '.' (just like the ls command).

Variables printed explicitly will be printed even without the -a.

---

 board/esd/pmc440/cmd_pmc440.c |  2 +-
 common/cmd_nvedit.c           | 24 ++++++++++++++++--------
 common/env_dataflash.c        |  2 +-
 common/env_eeprom.c           |  2 +-
 common/env_fat.c              |  2 +-
 common/env_flash.c            |  4 ++--
 common/env_mmc.c              |  2 +-
 common/env_nand.c             |  4 ++--
 common/env_nvram.c            |  2 +-
 common/env_onenand.c          |  2 +-
 common/env_sf.c               |  4 ++--
 include/search.h              |  5 +++--
 lib/hashtable.c               |  5 ++++-
 13 files changed, 36 insertions(+), 24 deletions(-)

index f1ffb7b..e9a78a3 100644
@@ -391,7 +391,7 @@ int do_painit(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	nextbase -= ((CONFIG_ENV_SIZE + 4096 - 1) & ~(4096 - 1));
 	envp = (env_t *)nextbase;
 	res = (char *)envp->data;
-	len = hexport_r(&env_htab, '\0', &res, ENV_SIZE, 0, NULL);
+	len = hexport_r(&env_htab, '\0', 0, &res, ENV_SIZE, 0, NULL);
 	if (len < 0) {
 		error("Cannot export environment: errno = %d\n", errno);
 		return 1;

index 119796b..a0cdc4b 100644
@@ -106,7 +106,7 @@ int get_env_id(void)
  *
  * Returns 0 in case of error, or length of printed string
  */
-static int env_print(char *name)
+static int env_print(char *name, int flag)
 {
 	char *res = NULL;
 	size_t len;

@@ -116,7 +116,7 @@ static int env_print(char *name)
 
 		e.key = name;
 		e.data = NULL;
-		hsearch_r(e, FIND, &ep, &env_htab, 0);
+		hsearch_r(e, FIND, &ep, &env_htab, flag);
 		if (ep == NULL)
 			return 0;
 		len = printf("%s=%s\n", ep->key, ep->data);

@@ -124,7 +124,7 @@ static int env_print(char *name)
 	}
 
 	/* print whole list */
-	len = hexport_r(&env_htab, '\n', &res, 0, 0, NULL);
+	len = hexport_r(&env_htab, '\n', flag, &res, 0, 0, NULL);
 
 	if (len > 0) {
 		puts(res);

@@ -140,10 +140,17 @@ int do_env_print (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
 	int i;
 	int rcode = 0;
+	int env_flag = H_HIDE_DOT;
+
+	if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'a') {
+		argc--;
+		argv++;
+		env_flag &= ~H_HIDE_DOT;
+	}
 
 	if (argc == 1) {
 		/* print all env vars */
-		rcode = env_print(NULL);
+		rcode = env_print(NULL, env_flag);
 		if (!rcode)
 			return 1;
 		printf("\nEnvironment size: %d/%ld bytes\n",

@@ -152,8 +159,9 @@ int do_env_print (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	}
 
 	/* print selected env vars */
+	env_flag &= ~H_HIDE_DOT;
 	for (i = 1; i < argc; ++i) {
-		int rc = env_print(argv[i]);
+		int rc = env_print(argv[i], env_flag);
 		if (!rc) {
 			printf("## Error: \"%s\" not defined\n", argv[i]);
 			++rcode;

@@ -804,7 +812,7 @@ NXTARG:		;
 	argv++;
 
 	if (sep) {		/* export as text file */
-		len = hexport_r(&env_htab, sep, &addr, size, argc, argv);
+		len = hexport_r(&env_htab, sep, 0, &addr, size, argc, argv);
 		if (len < 0) {
 			error("Cannot export environment: errno = %d\n", errno);
 			return 1;

@@ -822,7 +830,7 @@ NXTARG:		;
 	else			/* export as raw binary data */
 		res = addr;
 
-	len = hexport_r(&env_htab, '\0', &res, ENV_SIZE, argc, argv);
+	len = hexport_r(&env_htab, '\0', 0, &res, ENV_SIZE, argc, argv);
 	if (len < 0) {
 		error("Cannot export environment: errno = %d\n", errno);
 		return 1;

@@ -1062,7 +1070,7 @@ U_BOOT_CMD_COMPLETE(
 U_BOOT_CMD_COMPLETE(
 	printenv, CONFIG_SYS_MAXARGS, 1,	do_env_print,
 	"print environment variables",
-	"\n    - print values of all environment variables\n"
+	"[-a]\n    - print [all] values of all environment variables\n"
 	"printenv name ...\n"
 	"    - print value of environment variable 'name'",
 	var_complete

index 3c5af37..38c9615 100644
@@ -60,7 +60,7 @@ int saveenv(void)
 	char	*res;
 
 	res = (char *)&env_new.data;
-	len = hexport_r(&env_htab, '\0', &res, ENV_SIZE, 0, NULL);
+	len = hexport_r(&env_htab, '\0', 0, &res, ENV_SIZE, 0, NULL);
 	if (len < 0) {
 		error("Cannot export environment: errno = %d\n", errno);
 		return 1;

index b66bba2..45c935b 100644
@@ -139,7 +139,7 @@ int saveenv(void)
 	BUG_ON(env_ptr != NULL);
 
 	res = (char *)&env_new.data;
-	len = hexport_r(&env_htab, '\0', &res, ENV_SIZE, 0, NULL);
+	len = hexport_r(&env_htab, '\0', 0, &res, ENV_SIZE, 0, NULL);
 	if (len < 0) {
 		error("Cannot export environment: errno = %d\n", errno);
 		return 1;

index 6ef5318..c0f18ab 100644
@@ -61,7 +61,7 @@ int saveenv(void)
 	int err;
 
 	res = (char *)&env_new.data;
-	len = hexport_r(&env_htab, '\0', &res, ENV_SIZE, 0, NULL);
+	len = hexport_r(&env_htab, '\0', 0, &res, ENV_SIZE, 0, NULL);
 	if (len < 0) {
 		error("Cannot export environment: errno = %d\n", errno);
 		return 1;

index aa970d4..e07d336 100644
@@ -142,7 +142,7 @@ int saveenv(void)
 		goto done;
 
 	res = (char *)&env_new.data;
-	len = hexport_r(&env_htab, '\0', &res, ENV_SIZE, 0, NULL);
+	len = hexport_r(&env_htab, '\0', 0, &res, ENV_SIZE, 0, NULL);
 	if (len < 0) {
 		error("Cannot export environment: errno = %d\n", errno);
 		goto done;

@@ -275,7 +275,7 @@ int saveenv(void)
 		goto done;
 
 	res = (char *)&env_new.data;
-	len = hexport_r(&env_htab, '\0', &res, ENV_SIZE, 0, NULL);
+	len = hexport_r(&env_htab, '\0', 0, &res, ENV_SIZE, 0, NULL);
 	if (len < 0) {
 		error("Cannot export environment: errno = %d\n", errno);
 		goto done;

index a2ff90b..ce21671 100644
@@ -130,7 +130,7 @@ int saveenv(void)
 	}
 
 	res = (char *)&env_new->data;
-	len = hexport_r(&env_htab, '\0', &res, ENV_SIZE, 0, NULL);
+	len = hexport_r(&env_htab, '\0', 0, &res, ENV_SIZE, 0, NULL);
 	if (len < 0) {
 		error("Cannot export environment: errno = %d\n", errno);
 		ret = 1;

index 79e8033..22e72a2 100644
@@ -186,7 +186,7 @@ int saveenv(void)
 		return 1;
 
 	res = (char *)&env_new.data;
-	len = hexport_r(&env_htab, '\0', &res, ENV_SIZE, 0, NULL);
+	len = hexport_r(&env_htab, '\0', 0, &res, ENV_SIZE, 0, NULL);
 	if (len < 0) {
 		error("Cannot export environment: errno = %d\n", errno);
 		return 1;

@@ -239,7 +239,7 @@ int saveenv(void)
 		return 1;
 
 	res = (char *)&env_new->data;
-	len = hexport_r(&env_htab, '\0', &res, ENV_SIZE, 0, NULL);
+	len = hexport_r(&env_htab, '\0', 0, &res, ENV_SIZE, 0, NULL);
 	if (len < 0) {
 		error("Cannot export environment: errno = %d\n", errno);
 		return 1;

index 6483db3..eab0e7b 100644
@@ -90,7 +90,7 @@ int saveenv(void)
 	int	rcode = 0;
 
 	res = (char *)&env_new.data;
-	len = hexport_r(&env_htab, '\0', &res, ENV_SIZE, 0, NULL);
+	len = hexport_r(&env_htab, '\0', 0, &res, ENV_SIZE, 0, NULL);
 	if (len < 0) {
 		error("Cannot export environment: errno = %d\n", errno);
 		return 1;

index da35071..faa903d 100644
@@ -95,7 +95,7 @@ int saveenv(void)
 	};
 
 	res = (char *)&env_new.data;
-	len = hexport_r(&env_htab, '\0', &res, ENV_SIZE, 0, NULL);
+	len = hexport_r(&env_htab, '\0', 0, &res, ENV_SIZE, 0, NULL);
 	if (len < 0) {
 		error("Cannot export environment: errno = %d\n", errno);
 		return 1;

index bbd472f..d9e9085 100644
@@ -79,7 +79,7 @@ int saveenv(void)
 	}
 
 	res = (char *)&env_new.data;
-	len = hexport_r(&env_htab, '\0', &res, ENV_SIZE, 0, NULL);
+	len = hexport_r(&env_htab, '\0', 0, &res, ENV_SIZE, 0, NULL);
 	if (len < 0) {
 		error("Cannot export environment: errno = %d\n", errno);
 		return 1;

@@ -277,7 +277,7 @@ int saveenv(void)
 	}
 
 	res = (char *)&env_new.data;
-	len = hexport_r(&env_htab, '\0', &res, ENV_SIZE, 0, NULL);
+	len = hexport_r(&env_htab, '\0', 0, &res, ENV_SIZE, 0, NULL);
 	if (len < 0) {
 		error("Cannot export environment: errno = %d\n", errno);
 		goto done;

index fa00ea1..1e48deb 100644
@@ -107,7 +107,7 @@ extern int hdelete_r(const char *__key, struct hsearch_data *__htab,
 		     int __flag);
 
 extern ssize_t hexport_r(struct hsearch_data *__htab,
-		     const char __sep, char **__resp, size_t __size,
+		     const char __sep, int __flag, char **__resp, size_t __size,
 		     int argc, char * const argv[]);
 
 /*

@@ -120,9 +120,10 @@ extern int himport_r(struct hsearch_data *__htab,
 		     const char *__env, size_t __size, const char __sep,
 		     int __flag, int nvars, char * const vars[]);
 
-/* Flags for himport_r(), hdelete_r(), and hsearch_r() */
+/* Flags for himport_r(), hexport_r(), hdelete_r(), and hsearch_r() */
 #define H_NOCLEAR	(1 << 0) /* do not clear hash table before importing */
 #define H_FORCE		(1 << 1) /* overwrite read-only/write-once variables */
 #define H_INTERACTIVE	(1 << 2) /* indicate that an import is user directed */
+#define H_HIDE_DOT	(1 << 3) /* don't print env vars that begin with '.' */
 
 #endif /* search.h */

index 6861a42..7c6b96c 100644
@@ -522,7 +522,7 @@ static int cmpkey(const void *p1, const void *p2)
 	return (strcmp(e1->key, e2->key));
 }
 
-ssize_t hexport_r(struct hsearch_data *htab, const char sep,
+ssize_t hexport_r(struct hsearch_data *htab, const char sep, int flag,
 		 char **resp, size_t size,
 		 int argc, char * const argv[])
 {