diff mbox series

[U-Boot] tools: env: handle corrupted ubi volumes during sanity check

Message ID 20191112190223.225956-1-martin@geanix.com
State Needs Review / ACK
Delegated to: Tom Rini
Headers show
Series [U-Boot] tools: env: handle corrupted ubi volumes during sanity check | expand

Commit Message

Martin Hundebøll Nov. 12, 2019, 7:02 p.m. UTC
A partially written ubi volume is marked as such by the kernel. Such a
mark causes the initial ubi sanity check to fail instead of falling back
to the redundant environment.

Fix this by special casing the EBADF return code from the UBI_IOCEBISMAP
ioctl, as this is only ever returned in case of a partially written
volume. The CRC checking will decide which environment to use anyways.

Signed-off-by: Martin Hundebøll <martin@geanix.com>
---
 tools/env/fw_env.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Wolfgang Denk Jan. 30, 2020, 8:46 p.m. UTC | #1
Dear Martin,

In message <20191112190223.225956-1-martin@geanix.com> you wrote:
> A partially written ubi volume is marked as such by the kernel. Such a
> mark causes the initial ubi sanity check to fail instead of falling back
> to the redundant environment.
> 
> Fix this by special casing the EBADF return code from the UBI_IOCEBISMAP
> ioctl, as this is only ever returned in case of a partially written
> volume. The CRC checking will decide which environment to use anyways.

I dislike this approach, sorry.

> Signed-off-by: Martin Hundebøll <martin@geanix.com>
> ---
>  tools/env/fw_env.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c
> index cfada0ee11..f7904ae036 100644
> --- a/tools/env/fw_env.c
> +++ b/tools/env/fw_env.c
> @@ -1608,7 +1608,13 @@ static int check_device_config(int dev)
>  
>  	if (IS_UBI(dev)) {
>  		rc = ioctl(fd, UBI_IOCEBISMAP, &lnum);
> -		if (rc < 0) {
> +		if (rc < 0 && errno == EBADF) {
> +			/* EBADF here means we are dealing with a partially
> +			 * written UBI volume, Leave it for now to allow the
> +			 * use of the redundant env. CRC checking will decide
> +			 * which to use */
> +			rc = 0;

No, we have an error here, so we should report that error to the
user at least.  Saying "oh, this error is not an error" is wrong.

Think especially of the case when there is no redundant environment
at all!

If you want to drop through to trying to read the other copy of the
enviropnment (if there is one !!!) this needs to be implemented
differently.

Naked-by: Wolfgang Denk <wd@denx.de>


Best regards,

Wolfgang Denk
diff mbox series

Patch

diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c
index cfada0ee11..f7904ae036 100644
--- a/tools/env/fw_env.c
+++ b/tools/env/fw_env.c
@@ -1608,7 +1608,13 @@  static int check_device_config(int dev)
 
 	if (IS_UBI(dev)) {
 		rc = ioctl(fd, UBI_IOCEBISMAP, &lnum);
-		if (rc < 0) {
+		if (rc < 0 && errno == EBADF) {
+			/* EBADF here means we are dealing with a partially
+			 * written UBI volume, Leave it for now to allow the
+			 * use of the redundant env. CRC checking will decide
+			 * which to use */
+			rc = 0;
+		} else if (rc < 0) {
 			fprintf(stderr, "Cannot get UBI information for %s\n",
 				DEVNAME(dev));
 			goto err;