[U-Boot] [PATCH 10/12] env: acl: Add environment variable access control list

[Wolfgang Denk <wd@denx.de>]

Dear Joe Hershberger,

In message <CANr=Z=b6wJCOxVc+zi6riiU4Smv5tv_5ifS1d9Ue8Ncv+xVLyg@mail.gmail.com> you wrote:
> 
> I guess you didn't see this the last time I sent it to you off list...

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


> > - 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?
> 
> At least in my uses so far, there are not so many variables that it is
> noticeably slower.

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.

> > - 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.
> 
> 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 always try to listen to what people tell me :-)

> > - 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:
> 
> 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?

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.

> >     .callbacks=ethaddr:ethaddr,eth1addr,ath2addr;baudrate:baudrate;...
> 
> 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.

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.

> >     This way we could even enale user specific callbacks without need
> >     to modify any common code.
> 
> Would the user be able to define other callbacks somehow?

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

> 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.

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.