Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(54)

Issue 3141038: rootdev: fix -d, add -c and -r. ifdefs for so use (Closed)

Created:
10 years, 4 months ago by Will Drewry
Modified:
9 years, 7 months ago
CC:
chromium-os-reviews_chromium.org, Bill Richardson
Base URL:
http://src.chromium.org/git/rootdev.git
Visibility:
Public.

Description

rootdev: fix -d, add -c and -r. ifdefs for so use Multiple changes: - Ensure the -d flag trims off all of the partition info and only if there is a partition - Add -c, device node creation for platform-agnostic /dev/ROOT, /dev/ROOT0 and /dev/ROOT1 - Add -r, resolve the first slave if the device is a device-mapper device - Add -s, create a symlink from the found device to /dev/ACTIVE_ROOT - Adds short-circuiting if /dev/ACTIVE_ROOT exists to both dm resolution and normal lookups - Add support for building a library with a reusable header and interfaces I'll follow this up with a change to install <rootdev/rootdev.h> and the .so in the ebuild if the interface is at all interesting. If prefered, I can just add: rootdev -s -r to chromeos_startup, then AU can just rely on readlink(/dev/ACTIVE_ROOT). BUG=chromium-os:5988 TEST=built for x86-generic testing with vroot: -r, -s, -c, -d and combos switching to non-vroot to test now can someone test arm for me? Change-Id: Ibab8072afb012ea77d457517f1849e0917d02892

Patch Set 1 #

Patch Set 2 : static-ify internal constants for now #

Total comments: 6

Patch Set 3 : integrate kwaters feedback; retested on a build #

Total comments: 29

Patch Set 4 : complete rewrite :/ #

Patch Set 5 : makefile too #

Patch Set 6 : fix makefile header #

Total comments: 36

Patch Set 7 : integrated feedback; added functional tests #

Unified diffs Side-by-side diffs Delta from patch set Stats (+985 lines, -446 lines) Patch
D COPYING View 1 chunk +0 lines, -340 lines 0 comments Download
D LICENCE View 1 chunk +0 lines, -1 line 0 comments Download
A LICENSE View 1 chunk +27 lines, -0 lines 0 comments Download
M Makefile View 1 2 3 4 5 6 1 chunk +17 lines, -4 lines 0 comments Download
M README.chromium View 1 2 3 1 chunk +11 lines, -12 lines 0 comments Download
A main.c View 4 5 6 1 chunk +157 lines, -0 lines 0 comments Download
A rootdev.h View 1 2 3 4 5 6 1 chunk +104 lines, -0 lines 0 comments Download
M rootdev.c View 1 2 3 4 5 6 1 chunk +378 lines, -89 lines 0 comments Download
A rootdev_test.sh View 1 chunk +291 lines, -0 lines 0 comments Download

Messages

Total messages: 19 (0 generated)
Will Drewry
10 years, 4 months ago (2010-08-24 17:46:17 UTC) #1
Will Drewry
+wfrichar since he has interest in this too.
10 years, 4 months ago (2010-08-24 17:58:52 UTC) #2
Kenneth Waters
Does this need ebuild changes to install the library? http://codereview.chromium.org/3141038/diff/3001/4003 File rootdev.c (right): http://codereview.chromium.org/3141038/diff/3001/4003#newcode64 rootdev.c:64: ...
10 years, 4 months ago (2010-08-24 23:15:39 UTC) #3
Will Drewry
> Does this need ebuild changes to install the library? Yes and the header. If ...
10 years, 4 months ago (2010-08-25 01:14:16 UTC) #4
Will Drewry
On 2010/08/25 01:14:16, Will Drewry wrote: > > Does this need ebuild changes to install ...
10 years, 4 months ago (2010-08-25 18:22:49 UTC) #5
petkov
On 2010/08/25 18:22:49, Will Drewry wrote: > On 2010/08/25 01:14:16, Will Drewry wrote: > > ...
10 years, 4 months ago (2010-08-25 18:28:55 UTC) #6
petkov
Mostly nits. The code is complicated enough now that ideally we would: 1. Rewrite it ...
10 years, 4 months ago (2010-08-25 18:51:49 UTC) #7
adlr
http://codereview.chromium.org/3141038/diff/3001/4004 File rootdev.h (right): http://codereview.chromium.org/3141038/diff/3001/4004#newcode1 rootdev.h:1: /* Taken from util-linux source. GPLv2. (rootdev.c) is it ...
10 years, 4 months ago (2010-08-25 19:11:44 UTC) #8
Will Drewry
On 2010/08/25 18:51:49, petkov wrote: > Mostly nits. The code is complicated enough now that ...
10 years, 4 months ago (2010-08-25 19:25:51 UTC) #9
Will Drewry
http://codereview.chromium.org/3141038/diff/3001/4004 File rootdev.h (right): http://codereview.chromium.org/3141038/diff/3001/4004#newcode1 rootdev.h:1: /* Taken from util-linux source. GPLv2. (rootdev.c) On 2010/08/25 ...
10 years, 4 months ago (2010-08-25 19:26:05 UTC) #10
petkov
On 2010/08/25 19:26:05, Will Drewry wrote: > http://codereview.chromium.org/3141038/diff/3001/4004 > File rootdev.h (right): > > http://codereview.chromium.org/3141038/diff/3001/4004#newcode1 ...
10 years, 4 months ago (2010-08-25 19:50:40 UTC) #11
Will Drewry
On 2010/08/25 19:50:40, petkov wrote: > On 2010/08/25 19:26:05, Will Drewry wrote: > > http://codereview.chromium.org/3141038/diff/3001/4004 ...
10 years, 4 months ago (2010-08-25 20:11:33 UTC) #12
jrbarnette
On Aug 25, 2010, at 1:11 PM, wad@chromium.org wrote: > On 2010/08/25 19:50:40, petkov wrote: ...
10 years, 4 months ago (2010-08-25 20:19:23 UTC) #13
petkov
On 2010/08/25 20:11:33, Will Drewry wrote: > On 2010/08/25 19:50:40, petkov wrote: > > On ...
10 years, 4 months ago (2010-08-25 20:28:58 UTC) #14
Will Drewry
Given all the comments and the timing, I rewrote all the core code so that ...
10 years, 4 months ago (2010-08-26 21:27:23 UTC) #15
petkov
mostly nits, LGTM otherwise... It would be great to have unit tests... It's really hard ...
10 years, 4 months ago (2010-08-26 23:51:15 UTC) #16
Will Drewry
Added a functional test suite and integrated all comments. PTAL! --- (Writing unittests is a ...
10 years, 3 months ago (2010-08-27 21:11:57 UTC) #17
petkov
LGTM http://codereview.chromium.org/3141038/diff/25001/26008 File rootdev.h (right): http://codereview.chromium.org/3141038/diff/25001/26008#newcode20 rootdev.h:20: * @path: char array the result will be ...
10 years, 3 months ago (2010-08-27 21:44:59 UTC) #18
Will Drewry
10 years, 3 months ago (2010-08-27 21:58:06 UTC) #19
Thanks! I'll push bright and early monday, then do the ebuild changes!

On Fri, Aug 27, 2010 at 4:44 PM,  <petkov@chromium.org> wrote:
> LGTM
>
>
>
> http://codereview.chromium.org/3141038/diff/25001/26008
> File rootdev.h (right):
>
> http://codereview.chromium.org/3141038/diff/25001/26008#newcode20
> rootdev.h:20: * @path:   char array the result will be written to
> On 2010/08/27 21:11:57, Will Drewry wrote:
>>
>> On 2010/08/26 23:51:15, petkov wrote:
>> > is the path malloc'ed? does the caller take ownership?
>
>> It can't be  - it is a * not a **.  In addition, size below is not a
>
> *int so it
>>
>> can't be set either.  I'll add a little more info.
>
> Ah, I thought I had removed that comment after reading the rest of the
> code...
>
> http://codereview.chromium.org/3141038/show
>

Powered by Google App Engine
This is Rietveld 408576698