rootdev: fix -d, add -c and -r. ifdefs for so use

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

[Will Drewry, 2010-08-24 17:58:52]

+wfrichar since he has interest in this too.

[Kenneth Waters, 2010-08-24 23:15:39]

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: strcpy(dirnamebuf+dirnamelen+1, dir->d_name);
Potential buffer overflow here.  Paths are not gaurenteed to be < PATH_MAX
length.

http://codereview.chromium.org/3141038/diff/3001/4003#newcode114
rootdev.c:114: *part = 0;
Please write this 0 as '\0'.  This doesn't work for mmc devices.  mmcblk0p1 ->
mmblk0.

[Will Drewry, 2010-08-25 01:14:16]

> Does this need ebuild changes to install the library?

Yes and the header. If everyone (for some value of everyone) is happy enough
with these changes, I'll follow-on with the ebuild, and we can pull it in for
the update_engine.  Or the update_engine can use code similar to this code to do
the resolution.

I can also modify chromeos_startup if we'd like to have a /dev/ACTIVE_ROOT
symlink.  It could simplify the AU code and since anyone who can modify that
symlink can disable the update_engine, it doesn't change the robustness
guarantees a lot (at present).

Thanks!
will

http://codereview.chromium.org/3141038/diff/3001/4003
File rootdev.c (right):

http://codereview.chromium.org/3141038/diff/3001/4003#newcode64
rootdev.c:64: strcpy(dirnamebuf+dirnamelen+1, dir->d_name);
On 2010/08/24 23:15:39, Kenneth Waters wrote:
> Potential buffer overflow here.  Paths are not gaurenteed to be < PATH_MAX
> length.

I'm happy clean up while i'm in here.  They do check for overflow on line 61,
but it is subject to a very unlikely integer overflow so I'll fix it up.

http://codereview.chromium.org/3141038/diff/3001/4003#newcode114
rootdev.c:114: *part = 0;
On 2010/08/24 23:15:39, Kenneth Waters wrote:
> Please write this 0 as '\0'.  

Sure.

> This doesn't work for mmc devices.  mmcblk0p1 ->
> mmblk0.

Ah yeah I didn't see they handled that before.   They did incorrectly strip
partitions off of names with out them :?

[Will Drewry, 2010-08-25 18:22:49]

On 2010/08/25 01:14:16, Will Drewry wrote:
> > Does this need ebuild changes to install the library?
> 
> Yes and the header. If everyone (for some value of everyone) is happy enough
> with these changes, I'll follow-on with the ebuild, and we can pull it in for
> the update_engine.  Or the update_engine can use code similar to this code to
do
> the resolution.
> 
> I can also modify chromeos_startup if we'd like to have a /dev/ACTIVE_ROOT
> symlink.  It could simplify the AU code and since anyone who can modify that
> symlink can disable the update_engine, it doesn't change the robustness
> guarantees a lot (at present).
> 
> Thanks!
> will
> 
> http://codereview.chromium.org/3141038/diff/3001/4003
> File rootdev.c (right):
> 
> http://codereview.chromium.org/3141038/diff/3001/4003#newcode64
> rootdev.c:64: strcpy(dirnamebuf+dirnamelen+1, dir->d_name);
> On 2010/08/24 23:15:39, Kenneth Waters wrote:
> > Potential buffer overflow here.  Paths are not gaurenteed to be < PATH_MAX
> > length.
> 
> I'm happy clean up while i'm in here.  They do check for overflow on line 61,
> but it is subject to a very unlikely integer overflow so I'll fix it up.
> 
> http://codereview.chromium.org/3141038/diff/3001/4003#newcode114
> rootdev.c:114: *part = 0;
> On 2010/08/24 23:15:39, Kenneth Waters wrote:
> > Please write this 0 as '\0'.  
> 
> Sure.
> 
> > This doesn't work for mmc devices.  mmcblk0p1 ->
> > mmblk0.
> 
> Ah yeah I didn't see they handled that before.   They did incorrectly strip
> partitions off of names with out them :?

+petkov

Should I just look at mirroring this code in the update_engine or would
librootdev.so work?  It'd be nice to get the breakage fixed before it gets late
in the week :/


thanks!
will

[petkov, 2010-08-25 18:28:55]

On 2010/08/25 18:22:49, Will Drewry wrote:
> On 2010/08/25 01:14:16, Will Drewry wrote:
> > > Does this need ebuild changes to install the library?
> > 
> > Yes and the header. If everyone (for some value of everyone) is happy enough
> > with these changes, I'll follow-on with the ebuild, and we can pull it in
for
> > the update_engine.  Or the update_engine can use code similar to this code
to
> do
> > the resolution.
> > 
> > I can also modify chromeos_startup if we'd like to have a /dev/ACTIVE_ROOT
> > symlink.  It could simplify the AU code and since anyone who can modify that
> > symlink can disable the update_engine, it doesn't change the robustness
> > guarantees a lot (at present).
> > 
> > Thanks!
> > will
> > 
> > http://codereview.chromium.org/3141038/diff/3001/4003
> > File rootdev.c (right):
> > 
> > http://codereview.chromium.org/3141038/diff/3001/4003#newcode64
> > rootdev.c:64: strcpy(dirnamebuf+dirnamelen+1, dir->d_name);
> > On 2010/08/24 23:15:39, Kenneth Waters wrote:
> > > Potential buffer overflow here.  Paths are not gaurenteed to be < PATH_MAX
> > > length.
> > 
> > I'm happy clean up while i'm in here.  They do check for overflow on line
61,
> > but it is subject to a very unlikely integer overflow so I'll fix it up.
> > 
> > http://codereview.chromium.org/3141038/diff/3001/4003#newcode114
> > rootdev.c:114: *part = 0;
> > On 2010/08/24 23:15:39, Kenneth Waters wrote:
> > > Please write this 0 as '\0'.  
> > 
> > Sure.
> > 
> > > This doesn't work for mmc devices.  mmcblk0p1 ->
> > > mmblk0.
> > 
> > Ah yeah I didn't see they handled that before.   They did incorrectly strip
> > partitions off of names with out them :?
> 
> +petkov
> 
> Should I just look at mirroring this code in the update_engine or would
> librootdev.so work?  It'd be nice to get the breakage fixed before it gets
late
> in the week :/

Unless adlr@ thinks otherwise, I'd think it's best to use librootdev in update
engine rather than replicating code and logic. I assume the only change is going
to be in utils::BootDevice?

Just make sure you can "sudo emerge -av1 rootdev", otherwise you will break the
dev/test flow (it seems to work OK now without your changes).

> 
> 
> thanks!
> will

[petkov, 2010-08-25 18:51:49]

Mostly nits. The code is complicated enough now that ideally we would:

1. Rewrite it in C++.
2. Add unit tests.

Given the time pressure, I assume neither will happen though...

http://codereview.chromium.org/3141038/diff/8001/9001
File Makefile (right):

http://codereview.chromium.org/3141038/diff/8001/9001#newcode6
Makefile:6: $(CC) $(CFLAGS) $(LDFLAGS) -shared -fPIC -Wl,-soname,librootdev.so.1
$< -o $@ -DROOTDEV_LIBRARY=1
80 chars

http://codereview.chromium.org/3141038/diff/8001/9003
File rootdev.c (right):

http://codereview.chromium.org/3141038/diff/8001/9003#newcode19
rootdev.c:19: #include "rootdev.h"
This should be the top include, per C++ style.

http://codereview.chromium.org/3141038/diff/8001/9003#newcode21
rootdev.c:21: /* TODO: see if we can grab this from _our_ kernel headers */
TODO(wad):

http://codereview.chromium.org/3141038/diff/8001/9003#newcode22
rootdev.c:22: #define DM_MAJOR 254
static const int kDMMajor = 254?

http://codereview.chromium.org/3141038/diff/8001/9003#newcode34
rootdev.c:34: #define CHROMEOS_PRIMARY_PARTITION 3
kChromeOSPrimaryPartition? Or does that not work inside the switch?

http://codereview.chromium.org/3141038/diff/8001/9003#newcode38
rootdev.c:38: #define CHROMEOS_SECONDARY_PARTITION 5
kChromeOSSecondaryPartition?

http://codereview.chromium.org/3141038/diff/8001/9003#newcode170
rootdev.c:170: int ret=0;
spaces around =

http://codereview.chromium.org/3141038/diff/8001/9003#newcode199
rootdev.c:199: S_IFBLK|S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH,
add spaces around |

http://codereview.chromium.org/3141038/diff/8001/9003#newcode225
rootdev.c:225: int ret=0;
spaces around = -- here and below

http://codereview.chromium.org/3141038/diff/8001/9003#newcode230
rootdev.c:230: char candidate[PATH_MAX+1];
spaces around +

http://codereview.chromium.org/3141038/diff/8001/9003#newcode305
rootdev.c:305: int createDevices=0;
add spaces around =

I'd also change variables to lower_case style...

http://codereview.chromium.org/3141038/diff/8001/9003#newcode311
rootdev.c:311: while ((c = getopt(argc, argv, "hdcrs")) != -1) {
sort the option letters?

http://codereview.chromium.org/3141038/diff/8001/9003#newcode317
rootdev.c:317: resolveDeviceMapper=1;
spaces around = -- here, above and below...

http://codereview.chromium.org/3141038/diff/8001/9004
File rootdev.h (right):

http://codereview.chromium.org/3141038/diff/8001/9004#newcode6
rootdev.h:6: 
This is going to be included by C++ compiles -- I think you need the extern "C"
stuff...

#if defined(__cplusplus)
extern "C" {
#endif

http://codereview.chromium.org/3141038/diff/8001/9004#newcode9
rootdev.h:9: const char *rootdev_get_partition(const char *dst, size_t len);
It might be nice if you move the function documentation to the header file.

Also, are all these really intended to be public?

[adlr, 2010-08-25 19:11:44]

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 really a good idea to have a GPL library?

[Will Drewry, 2010-08-25 19:25:51]

On 2010/08/25 18:51:49, petkov wrote:
> Mostly nits. The code is complicated enough now that ideally we would:
> 
> 1. Rewrite it in C++.
> 2. Add unit tests.
> 

Agreed.  Also, I attempted to keep the style based on the original code though
my LoC now outweights the original.

> Given the time pressure, I assume neither will happen though...

Given the licensing issue adlr noted, I'm not seeing much choice. ugh

> http://codereview.chromium.org/3141038/diff/8001/9001
> File Makefile (right):
> 
> http://codereview.chromium.org/3141038/diff/8001/9001#newcode6
> Makefile:6: $(CC) $(CFLAGS) $(LDFLAGS) -shared -fPIC
-Wl,-soname,librootdev.so.1
> $< -o $@ -DROOTDEV_LIBRARY=1
> 80 chars
> 
> http://codereview.chromium.org/3141038/diff/8001/9003
> File rootdev.c (right):
> 
> http://codereview.chromium.org/3141038/diff/8001/9003#newcode19
> rootdev.c:19: #include "rootdev.h"
> This should be the top include, per C++ style.
> 
> http://codereview.chromium.org/3141038/diff/8001/9003#newcode21
> rootdev.c:21: /* TODO: see if we can grab this from _our_ kernel headers */
> TODO(wad):
> 
> http://codereview.chromium.org/3141038/diff/8001/9003#newcode22
> rootdev.c:22: #define DM_MAJOR 254
> static const int kDMMajor = 254?
> 
> http://codereview.chromium.org/3141038/diff/8001/9003#newcode34
> rootdev.c:34: #define CHROMEOS_PRIMARY_PARTITION 3
> kChromeOSPrimaryPartition? Or does that not work inside the switch?
> 
> http://codereview.chromium.org/3141038/diff/8001/9003#newcode38
> rootdev.c:38: #define CHROMEOS_SECONDARY_PARTITION 5
> kChromeOSSecondaryPartition?
> 
> http://codereview.chromium.org/3141038/diff/8001/9003#newcode170
> rootdev.c:170: int ret=0;
> spaces around =
> 
> http://codereview.chromium.org/3141038/diff/8001/9003#newcode199
> rootdev.c:199: S_IFBLK|S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH,
> add spaces around |
> 
> http://codereview.chromium.org/3141038/diff/8001/9003#newcode225
> rootdev.c:225: int ret=0;
> spaces around = -- here and below
> 
> http://codereview.chromium.org/3141038/diff/8001/9003#newcode230
> rootdev.c:230: char candidate[PATH_MAX+1];
> spaces around +
> 
> http://codereview.chromium.org/3141038/diff/8001/9003#newcode305
> rootdev.c:305: int createDevices=0;
> add spaces around =
> 
> I'd also change variables to lower_case style...
> 
> http://codereview.chromium.org/3141038/diff/8001/9003#newcode311
> rootdev.c:311: while ((c = getopt(argc, argv, "hdcrs")) != -1) {
> sort the option letters?
> 
> http://codereview.chromium.org/3141038/diff/8001/9003#newcode317
> rootdev.c:317: resolveDeviceMapper=1;
> spaces around = -- here, above and below...
> 
> http://codereview.chromium.org/3141038/diff/8001/9004
> File rootdev.h (right):
> 
> http://codereview.chromium.org/3141038/diff/8001/9004#newcode6
> rootdev.h:6: 
> This is going to be included by C++ compiles -- I think you need the extern
"C"
> stuff...
> 
> #if defined(__cplusplus)
> extern "C" {
> #endif
> 
> http://codereview.chromium.org/3141038/diff/8001/9004#newcode9
> rootdev.h:9: const char *rootdev_get_partition(const char *dst, size_t len);
> It might be nice if you move the function documentation to the header file.
> 
> Also, are all these really intended to be public?

[Will Drewry, 2010-08-25 19:26:05]

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 19:11:44, adlr wrote:
> is it really a good idea to have a GPL library?

Ugh. no.  The only original code is the main and dev tree recursion.  We should
just rewrite this from scratch, usefully license it, make it testable, and use
it.

[petkov, 2010-08-25 19:50:40]

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
> rootdev.h:1: /* Taken from util-linux source. GPLv2. (rootdev.c)
> On 2010/08/25 19:11:44, adlr wrote:
> > is it really a good idea to have a GPL library?
> 
> Ugh. no.  The only original code is the main and dev tree recursion.  We
should
> just rewrite this from scratch, usefully license it, make it testable, and use
> it.

We can probably use FileEnumerator from libbase's file_util.h to do the dev tree
recursion.

[Will Drewry, 2010-08-25 20:11:33]

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
> > 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 19:11:44, adlr wrote:
> > > is it really a good idea to have a GPL library?
> > 
> > Ugh. no.  The only original code is the main and dev tree recursion.  We
> should
> > just rewrite this from scratch, usefully license it, make it testable, and
use
> > it.
> 
> We can probably use FileEnumerator from libbase's file_util.h to do the dev
tree
> recursion.

Yup, but I was hoping to avoid libbase because we only statically link it now
and it is not small.  Currently rootdev is under 2 pages which makes it pretty
speedy.

[jrbarnette, 2010-08-25 20:19:23]

On Aug 25, 2010, at 1:11 PM, wad@chromium.org wrote:

> 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
>> > 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 19:11:44, adlr wrote:
>> > > is it really a good idea to have a GPL library?
>> >
>> > Ugh. no.  The only original code is the main and dev tree  
>> recursion.  We
>> should
>> > just rewrite this from scratch, usefully license it, make it  
>> testable, and
> use
>> > it.
>
>> We can probably use FileEnumerator from libbase's file_util.h to do  
>> the dev
> tree
>> recursion.
>
> Yup, but I was hoping to avoid libbase because we only statically  
> link it now
> and it is not small.  Currently rootdev is under 2 pages which makes  
> it pretty
> speedy.
>
Speedy is an important attribute for this executable; it's invoked
during chromeos_startup, and I think a few other places in the
boot critical path.

That said, I think the cost of even a large-ish shared lib is
still only a few dozen milliseconds.  OTOH, if libbase has its
own set of dependencies to pull in, the cost could grow.

One other question:  it looks like you're exporting a .so to
make rootdev accessible as a library.  Are we certain that no
client of the library will ever be in C?  (I note that I can
imagine bootstat needing access to this library, and that's
currently in C).


>
> http://codereview.chromium.org/3141038/show

-- jrb

[petkov, 2010-08-25 20:28:58]

On 2010/08/25 20:11:33, Will Drewry wrote:
> 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
> > > 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 19:11:44, adlr wrote:
> > > > is it really a good idea to have a GPL library?
> > > 
> > > Ugh. no.  The only original code is the main and dev tree recursion.  We
> > should
> > > just rewrite this from scratch, usefully license it, make it testable, and
> use
> > > it.
> > 
> > We can probably use FileEnumerator from libbase's file_util.h to do the dev
> tree
> > recursion.
> 
> Yup, but I was hoping to avoid libbase because we only statically link it now
> and it is not small.  Currently rootdev is under 2 pages which makes it pretty
> speedy.

Yeah, unfortunately libbase does add >270KB. We should switch it to a .so at
some point...

[Will Drewry, 2010-08-26 21:27:23]

Given all the comments and the timing, I rewrote all the core code so that it is
totally original.

That said, it is still C.  I've specified a simple interface: rootdev() which we
can maintain for simple access even after a C++ rewrite.  I kept some
implementation specific interfaces public and I left in features we aren't
actively using yet.  I'm happy to cut them out, but I figured as dead code they
can still be referenced for discussion 

(do we want a /dev/ACTIVE_ROOT or platform independent device nodes created in
chromeos_startup? etc).

In the long term, I think we should implement a class BlockDevice which contains
the information we might need:
 - dev_t, partition number, name, device node on disk, etc
which could be easily used with rootdev.

Comments appreciated.  If all looks good, we can update the ebuild, install the
.h, and then move this out of third_party.

thanks!
will

http://codereview.chromium.org/3141038/diff/8001/9003
File rootdev.c (right):

http://codereview.chromium.org/3141038/diff/8001/9003#newcode19
rootdev.c:19: #include "rootdev.h"
On 2010/08/25 18:51:49, petkov wrote:
> This should be the top include, per C++ style.
> 

Done.

http://codereview.chromium.org/3141038/diff/8001/9003#newcode21
rootdev.c:21: /* TODO: see if we can grab this from _our_ kernel headers */
On 2010/08/25 18:51:49, petkov wrote:
> TODO(wad):

Done.

http://codereview.chromium.org/3141038/diff/8001/9003#newcode22
rootdev.c:22: #define DM_MAJOR 254
On 2010/08/25 18:51:49, petkov wrote:
> static const int kDMMajor = 254?
> 

Deleted.

http://codereview.chromium.org/3141038/diff/8001/9003#newcode34
rootdev.c:34: #define CHROMEOS_PRIMARY_PARTITION 3
On 2010/08/25 18:51:49, petkov wrote:
> kChromeOSPrimaryPartition? Or does that not work inside the switch?


Can't be referenced in the switch.

Anyway, I've left this code in, but only provided noted private access.  It was
unclear if anyone wanted to use it.  I can either just delete it all, or keep it
here so we can point some discussion to it for later.

http://codereview.chromium.org/3141038/diff/8001/9003#newcode38
rootdev.c:38: #define CHROMEOS_SECONDARY_PARTITION 5
On 2010/08/25 18:51:49, petkov wrote:
> kChromeOSSecondaryPartition?
> 

Yeah same deal.

http://codereview.chromium.org/3141038/diff/8001/9003#newcode170
rootdev.c:170: int ret=0;
On 2010/08/25 18:51:49, petkov wrote:
> spaces around =
> 

Done.

http://codereview.chromium.org/3141038/diff/8001/9003#newcode199
rootdev.c:199: S_IFBLK|S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH,
On 2010/08/25 18:51:49, petkov wrote:
> add spaces around |
> 
Done.

But that just feels wrong.  The style guide is unclear as you could argue they
are all factors :)

http://codereview.chromium.org/3141038/diff/8001/9003#newcode225
rootdev.c:225: int ret=0;
On 2010/08/25 18:51:49, petkov wrote:
> spaces around = -- here and below
> 
> 

Function is gone.

http://codereview.chromium.org/3141038/diff/8001/9003#newcode230
rootdev.c:230: char candidate[PATH_MAX+1];
On 2010/08/25 18:51:49, petkov wrote:
> spaces around +
> 

Function is gone.

http://codereview.chromium.org/3141038/diff/8001/9003#newcode305
rootdev.c:305: int createDevices=0;
Function is gone.  It was by the original author.  It, usage(), and find_dev_..
have all been removed.

http://codereview.chromium.org/3141038/diff/8001/9003#newcode311
rootdev.c:311: while ((c = getopt(argc, argv, "hdcrs")) != -1) {
On 2010/08/25 18:51:49, petkov wrote:
> sort the option letters?
> 
> 

n/a

http://codereview.chromium.org/3141038/diff/8001/9003#newcode317
rootdev.c:317: resolveDeviceMapper=1;
On 2010/08/25 18:51:49, petkov wrote:
> spaces around = -- here, above and below...
> 

n/a

FWIW, if you are modifying a file, the style guide specifies that we stick with
their format.  In this case, that's what I attempted to do where it was obvious
what it was.

Now that all that code is gone.  I stuck to a C++ style guide applied to C
approach, but kept the kernel-style comments.

http://codereview.chromium.org/3141038/diff/8001/9004
File rootdev.h (right):

http://codereview.chromium.org/3141038/diff/8001/9004#newcode6
rootdev.h:6: 
On 2010/08/25 18:51:49, petkov wrote:
> This is going to be included by C++ compiles -- I think you need the extern
"C"
> stuff...
> 
> #if defined(__cplusplus)
> extern "C" {
> #endif
> 

Done.

http://codereview.chromium.org/3141038/diff/8001/9004#newcode9
rootdev.h:9: const char *rootdev_get_partition(const char *dst, size_t len);
On 2010/08/25 18:51:49, petkov wrote:
> It might be nice if you move the function documentation to the header file.
> 
> Also, are all these really intended to be public?
> 

Done.  Yup, but I'll add a comment to that affect.

[petkov, 2010-08-26 23:51:15]

mostly nits, LGTM otherwise...

It would be great to have unit tests... It's really hard to follow how all sizes
and arrays propagate, get checked, etc., so I didn't really try to do that.

I assume you've tested both with vroot and without, all options...

http://codereview.chromium.org/3141038/diff/25001/26004
File Makefile (right):

http://codereview.chromium.org/3141038/diff/25001/26004#newcode11
Makefile:11: $(CC) $(CFLAGS) $(LDFLAGS) -shared -fPIC
-Wl,-soname,librootdev.so.1 $< -o $@
80 chars

http://codereview.chromium.org/3141038/diff/25001/26006
File main.c (right):

http://codereview.chromium.org/3141038/diff/25001/26006#newcode21
main.c:21: static void print_help(const char *progname) {
generally, chromium style is stars to the left... :-/

http://codereview.chromium.org/3141038/diff/25001/26006#newcode49
main.c:49: {"d", no_argument, &flag_strip_partition, 1},
sort options

http://codereview.chromium.org/3141038/diff/25001/26006#newcode82
main.c:82: struct stat s;
rename s to something more descriptive

http://codereview.chromium.org/3141038/diff/25001/26006#newcode113
main.c:113: warn("failed to create %s", path);
indent 2 chars only.

http://codereview.chromium.org/3141038/diff/25001/26007
File rootdev.c (right):

http://codereview.chromium.org/3141038/diff/25001/26007#newcode60
rootdev.c:60: fd = open(file, O_NONBLOCK|O_RDONLY);
spaces around |

http://codereview.chromium.org/3141038/diff/25001/26007#newcode62
rootdev.c:62: return 0;
indent by 2 chars only

http://codereview.chromium.org/3141038/diff/25001/26007#newcode68
rootdev.c:68: return 0;
indentation

http://codereview.chromium.org/3141038/diff/25001/26007#newcode225
rootdev.c:225: struct stat s;
rename s to something more descriptive

http://codereview.chromium.org/3141038/diff/25001/26007#newcode232
rootdev.c:232: /* Note, if the link is not fully qualified, this won't be
reduce indentation in this block

http://codereview.chromium.org/3141038/diff/25001/26007#newcode244
rootdev.c:244: fprintf (stderr, "unable to find match\n");
indentation

http://codereview.chromium.org/3141038/diff/25001/26007#newcode256
rootdev.c:256: 
remove extra blank line

http://codereview.chromium.org/3141038/diff/25001/26007#newcode306
rootdev.c:306: return -1;
indentation

http://codereview.chromium.org/3141038/diff/25001/26007#newcode376
rootdev.c:376: struct stat s;
more descriptive name

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
is the path malloc'ed? does the caller take ownership?

http://codereview.chromium.org/3141038/diff/25001/26008#newcode21
rootdev.h:21: * @size: size of @path
either align all or not align... i prefer not align :)

http://codereview.chromium.org/3141038/diff/25001/26008#newcode27
rootdev.h:27: int rootdev(char *path, size_t size, bool full, bool strip);
Hmm, I thought bool is a C++ only type...

http://codereview.chromium.org/3141038/diff/25001/26008#newcode72
rootdev.h:72: * @path:  char array to store the path
align / not align

[Will Drewry, 2010-08-27 21:11:57]

Added a functional test suite and integrated all comments.

PTAL!

---
(Writing unittests is a royal pain given the platform dependencies, so I put in
functional tests with manually managed dev and sys trees.  I'd rather save that
for mocking out the platform in the C++ port.  I didn't want to rush through the
C++ version so we could actually make useful interfaces for rootdev and
update_engine.)

http://codereview.chromium.org/3141038/diff/25001/26004
File Makefile (right):

http://codereview.chromium.org/3141038/diff/25001/26004#newcode11
Makefile:11: $(CC) $(CFLAGS) $(LDFLAGS) -shared -fPIC
-Wl,-soname,librootdev.so.1 $< -o $@
On 2010/08/26 23:51:15, petkov wrote:
> 80 chars
> 

Done.

http://codereview.chromium.org/3141038/diff/25001/26006
File main.c (right):

http://codereview.chromium.org/3141038/diff/25001/26006#newcode21
main.c:21: static void print_help(const char *progname) {
On 2010/08/26 23:51:15, petkov wrote:
> generally, chromium style is stars to the left... :-/
> 

I thought it was dependent on the code base as long as it was consistent.

I can change it to that hideous style if needed, but I don't want to :p

http://codereview.chromium.org/3141038/diff/25001/26006#newcode49
main.c:49: {"d", no_argument, &flag_strip_partition, 1},
On 2010/08/26 23:51:15, petkov wrote:
> sort options
> 

Done.

http://codereview.chromium.org/3141038/diff/25001/26006#newcode82
main.c:82: struct stat s;
On 2010/08/26 23:51:15, petkov wrote:
> rename s to something more descriptive
> 

Done.

http://codereview.chromium.org/3141038/diff/25001/26006#newcode113
main.c:113: warn("failed to create %s", path);
On 2010/08/26 23:51:15, petkov wrote:
> indent 2 chars only.
> 

Done.

http://codereview.chromium.org/3141038/diff/25001/26007
File rootdev.c (right):

http://codereview.chromium.org/3141038/diff/25001/26007#newcode60
rootdev.c:60: fd = open(file, O_NONBLOCK|O_RDONLY);
On 2010/08/26 23:51:15, petkov wrote:
> spaces around |
> 

Done.

http://codereview.chromium.org/3141038/diff/25001/26007#newcode62
rootdev.c:62: return 0;
On 2010/08/26 23:51:15, petkov wrote:
> indent by 2 chars only

Done.

http://codereview.chromium.org/3141038/diff/25001/26007#newcode68
rootdev.c:68: return 0;
On 2010/08/26 23:51:15, petkov wrote:
> indentation

Done.

http://codereview.chromium.org/3141038/diff/25001/26007#newcode225
rootdev.c:225: struct stat s;
On 2010/08/26 23:51:15, petkov wrote:
> rename s to something more descriptive

Done.

http://codereview.chromium.org/3141038/diff/25001/26007#newcode232
rootdev.c:232: /* Note, if the link is not fully qualified, this won't be
On 2010/08/26 23:51:15, petkov wrote:
> reduce indentation in this block
> 

Done.

http://codereview.chromium.org/3141038/diff/25001/26007#newcode244
rootdev.c:244: fprintf (stderr, "unable to find match\n");
On 2010/08/26 23:51:15, petkov wrote:
> indentation
> 

Done.

http://codereview.chromium.org/3141038/diff/25001/26007#newcode256
rootdev.c:256: 
On 2010/08/26 23:51:15, petkov wrote:
> remove extra blank line
> 

Done.

http://codereview.chromium.org/3141038/diff/25001/26007#newcode306
rootdev.c:306: return -1;
On 2010/08/26 23:51:15, petkov wrote:
> indentation
> 

Done.

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/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.

http://codereview.chromium.org/3141038/diff/25001/26008#newcode21
rootdev.h:21: * @size: size of @path
On 2010/08/26 23:51:15, petkov wrote:
> either align all or not align... i prefer not align :)
> 

Done.

http://codereview.chromium.org/3141038/diff/25001/26008#newcode27
rootdev.h:27: int rootdev(char *path, size_t size, bool full, bool strip);
On 2010/08/26 23:51:15, petkov wrote:
> Hmm, I thought bool is a C++ only type...
> 

C99 brought to you by stdbool.h :)

http://codereview.chromium.org/3141038/diff/25001/26008#newcode72
rootdev.h:72: * @path:  char array to store the path
On 2010/08/26 23:51:15, petkov wrote:
> align / not align
> 

Done.

[petkov, 2010-08-27 21:44:59]

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...

[Will Drewry, 2010-08-27 21:58:06]

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
>