public inbox for u-boot@lists.denx.de
 help / color / mirror / Atom feed
From: Stefano Babic <sbabic@denx.de>
To: u-boot@lists.denx.de
Subject: [U-Boot] [PATCH v1] env: fix crash using default -f -a
Date: Fri, 05 Oct 2012 13:49:43 +0200	[thread overview]
Message-ID: <506EC957.6050507@denx.de> (raw)
In-Reply-To: <506EBD43.2070602@keymile.com>

On 05/10/2012 12:58, Gerlando Falauto wrote:

> My idea here was to "apply" the changes as soon as the environment gets
> changed, like it would happen with a manual "setenv"/"env set".
> So for instance, if you switch from a custom baudrate back to the
> default baudrate, you get a message prompting you to switch to the new
> baudrate (which actually *DOES* change on the console).

This is exactly the reason I am personally not so convinced to do it.
This forbids to reset the environment in a script. In fact, nobody wants
that a script stops waiting for input.

Think this use case. A board come back from field and in the factory the
original status should be restored. It can be easy done with a script
that contains "env default -f -a", plus parts to restore the original
software. The whole script should run, and not wait for input.

My personal opinion is that who issues such commands (as restoring the
environment, or destroying data in flash, or..) knows what is doing and
it should not be forbidden to do what he minds.

> 
> Whether that should also be performed as a "cleanup" action upon
> restoring defaults, well I am not sure...

I explained my opinion above ;-). But this has nothing to do with the
patch, that must fix a crash.

> 
>>> So by adding these extra checks you're subtly canceling those efforts.
>>
>> That is correct, but in any case a check for a NULL pointer should be
>> done before accessing to the pointer, independently what we want to do
>> with it.
> 
> Absolutely. See my proposed patch.
> 
>>> So I would rather provide a default value to prevent the crashes,
>>> instead of skipping settings altogether. Posting a patch in a few
>>> minutes.
>>
>> Ok, I will review it. In any case, we need a fix for the coming release.
> 
> Absolutely. Once again, I cannot apologize and thank you enough for
> reporting and digging into this.
> I assumed providing an own patch would make more sense than asking you
> to review yours, not sure whether it is popular practice to do so.

Never mind. It is only important to fix the bug ;-)

Best regards,
Stefano

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

      reply	other threads:[~2012-10-05 11:49 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-10-04 17:30 [U-Boot] [PATCH v1] env: fix crash using default -f -a Stefano Babic
2012-10-04 21:57 ` Tom Rini
2012-10-05 10:29 ` Gerlando Falauto
2012-10-05 10:41   ` Stefano Babic
2012-10-05 10:46     ` [U-Boot] [PATCH] " Gerlando Falauto
2012-10-05 11:50       ` Stefano Babic
2012-10-05 22:26       ` Tom Rini
2012-10-05 10:58     ` [U-Boot] [PATCH v1] " Gerlando Falauto
2012-10-05 11:49       ` Stefano Babic [this message]

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=506EC957.6050507@denx.de \
    --to=sbabic@denx.de \
    --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