New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 733028 link

Starred by 1 user

Issue metadata

Status: Archived
Owner:
Closed: Jun 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug



Sign in to add a comment

update_kernel.sh can't determine $PARTITION_NUM_xxx on old rootfs

Project Member Reported by briannorris@chromium.org, Jun 13 2017

Issue description

PARTITION_NUM_KERN_A and PARTITION_NUM_KERN_B were killed here:

https://chromium-review.googlesource.com/c/439509/

after being "removed" from the update_kernel script here:

https://chromium-review.googlesource.com/451543

But...not all references were removed.

This results in us "learning" that the kernel partition is at, e.g., /dev/mmcblk0p, which is not a device partition at all. So we think we're writing the kernel partition, but we're not.

We should fix up the update_kernel script to get this right, but it'd be nice if there was some validation for this too... I wonder if dd's 'conv=nocreat' is portable enough? I've found it very valuable in the past to present this kind of SNAFU. Or we could just do a simple bash '-e' check before writing.

Also, how is it that no one noticed this for the last few months?
 
The CL defines this function in remote_access.sh:

# Discover partition numbers from the target.
learn_partition_layout() {
  source <(remote_sh_raw cat /usr/sbin/write_gpt.sh)
  load_base_vars
}

Does that not work for your target? I'm finding it difficult to understand how that's possible
Labels: -Pri-1 Pri-2
$ cat /usr/sbin/write_gpt.sh | grep PARTITION_NUM

shows nothing.

I'm updating a pretty old image. If that's the only problem, then this is less important. But is it possible we can retain compatibility? Or at least add some error-checking, so it's more obvious the update isn't working.
(Yes, I know 'cat ... | grep ...' is one 'cat' too many.)

I see where the PARTITION_NUM stuff got into write_gpt.sh:

https://chromium-review.googlesource.com/439508

So yes, I guess I need a relatively new image.
I'll rely on your judgement for how important this is, or if we should just close it.
I'm happy to cook up some kind of fix if necessary.
Owner: ----
Summary: update_kernel.sh can't determine $PARTITION_NUM_xxx on old rootfs (was: update_kernel.sh still uses deprecated (and removed) disk_layout_util.sh variables)
Well, I think at a minimum it would be nice to improve the script so it knows when it's flashing to nowhere. That would have saved me some considerable trouble over the subtle problems of an updated set of modules, with no updated kernel partition. (And no errors; it's perfectly fine to create a new '/dev/mmcblk0p' file.)

Full backward compatibility might be overkill.

Notably, this is still marked 'Available'. I might tackle this myself later in the week if I get time. (Thanks for the pointers so far.)

Comment 6 by vapier@chromium.org, Jun 14 2017

we should error out when we can't detect the partition, but i don't think we want to add backwards compat code.  ToT is largely meant to be used with ToT-ish systems.
Owner: briannorris@chromium.org
Status: Started (was: Available)
Apparently 'dd ... conv=nocreat' isn't POSIX. Oh well. I've added some error checking here, which we'd want anyway:

https://chromium-review.googlesource.com/537140
Project Member

Comment 8 by bugdroid1@chromium.org, Jun 17 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/crosutils/+/9774b0db8b8d69b9bbc36d150a9c6fb1d723999f

commit 9774b0db8b8d69b9bbc36d150a9c6fb1d723999f
Author: Brian Norris <briannorris@chromium.org>
Date: Sat Jun 17 08:12:42 2017

update_kernel: error out if kernel partition isn't found

The kernel partition might not be found for a number of reasons:

 * the '--partition=' argument might be incorrect
 * the DUT might not be compatible with us (e.g., it's using an old
   image, with differences in /usr/sbin/write_gpt.sh)

Currently, rather than noticing this and bailing out, we're happy to
use 'dd' to create a new file instead. That can create unintuitive
behaviors, where the DUT's kernel modules have been updated while the
kernel partition is left unmodified.

Let's detect this situation and error out, instead of leaving the user
to figure out what happened.

BUG= chromium:733028 
TEST=update_kernel on old images (without $PARTITION_NUM... variables)
     and new ones

Change-Id: I47f64ec7fb1175e0e25697a6b55b63366fcc4555
Signed-off-by: Brian Norris <briannorris@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/537140
Reviewed-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/9774b0db8b8d69b9bbc36d150a9c6fb1d723999f/update_kernel.sh

Status: Fixed (was: Started)
Didn't "fix" backward compatibility, but at least the script will error out and make this more obvious now.

Comment 10 by dchan@chromium.org, Jan 22 2018

Status: Archived (was: Fixed)

Sign in to add a comment