public inbox for u-boot@lists.denx.de
 help / color / mirror / Atom feed
From: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
To: u-boot@lists.denx.de
Subject: [U-Boot] [PATCH] env: Provide programmatic equivalent to 'setenv -f'
Date: Wed, 20 Nov 2019 21:21:36 +0100	[thread overview]
Message-ID: <8a392b32-9662-7296-3086-7e9c9f8fcbf9@gmail.com> (raw)
In-Reply-To: <0102016e8a263d9b-81e7eb33-ed84-442a-ae92-7ffa9b3333ff-000000@eu-west-1.amazonses.com>

Am 20.11.2019 um 19:49 schrieb James Byrne:
> On 19/11/2019 21:01, Simon Goldschmidt wrote:
>>
>>
>> Heinrich Schuchardt <xypron.glpk at gmx.de <mailto:xypron.glpk@gmx.de>>
>> 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 <james.byrne@origamienergy.com
>>      <mailto:james.byrne@origamienergy.com>>
>>       >
>>       > 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!

Cool. I wouldn't want to put this as a burdon on you (meaning to NACK 
this patch like it is), btu a cleanup in that direction would certainly 
be appreciated!

Thanks,
Simon

> 
> James
> 

  reply	other threads:[~2019-11-20 20:21 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-19 17:31 [U-Boot] [PATCH] env: Provide programmatic equivalent to 'setenv -f' James Byrne
2019-11-19 20:30 ` Simon Goldschmidt
2019-11-19 20:56   ` Heinrich Schuchardt
2019-11-19 21:01     ` Simon Goldschmidt
2019-11-19 21:34       ` Joe Hershberger
2019-11-20 21:24         ` Tom Rini
2019-11-20 18:49       ` James Byrne
2019-11-20 20:21         ` Simon Goldschmidt [this message]
2019-11-20  0:10     ` AKASHI Takahiro
2019-11-19 21:33 ` Joe Hershberger

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=8a392b32-9662-7296-3086-7e9c9f8fcbf9@gmail.com \
    --to=simon.k.r.goldschmidt@gmail.com \
    --cc=u-boot@lists.denx.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox