On 19/11/2019 21:01, Simon Goldschmidt wrote:


Heinrich Schuchardt <[email protected] <mailto:[email protected]>> schrieb am Di., 19. Nov. 2019, 21:56:

    On 11/19/19 9:30 PM, Simon Goldschmidt wrote:
     > Am 19.11.2019 um 18:31 schrieb James Byrne:
     >> Add env_force() to provide an equivalent to 'setenv -f' that can
    be used
     >> programmatically.
     >>
     >> Also tighten up the definition of argv in _do_env_set() so that
     >> 'const char *' pointers are used.
     >>
     >> Signed-off-by: James Byrne <[email protected]
    <mailto:[email protected]>>
     >
     > OK, I'm on CC, so I'll give my two cent:
     >
     > I always thought this code to be messed up a bit: I think it's better
     > programming style to have the "string argument parsing" code call
    real C
     > functions with typed arguments. The env code instead does it the
    other
     > way round (here, you add do_programmatic_env_set() that sets up an
     > argv[] array to call another function).
     >
     > I'm not a maintainer for the ENV code, but maybe that should be
    sorted
     > out instead of adding yet more code that goes this way?

    There is no maintainer for the ENV code. Simon makes a valid point here.
    By creating a function for setting variables and separating it from
    parsing arguments you get the function you need for forcing the value of
    a variable for free.


Right. I thought someone had volunteered but a look at the maintainers file proves me wrong.

In any way, I'd be more open to review a cleanup patch than a patch continuing this messy code flow.

Having looked at it again, I agree. I have now redone it, but I have ended up changing quite a lot more of the underlying code. I will resubmit a revised patch (probably tomorrow) in two parts, one to apply some tidying up to the env code, and one to add the new function. It will be a much bigger patch set though!

James
_______________________________________________
U-Boot mailing list
[email protected]
https://lists.denx.de/listinfo/u-boot

Reply via email to