AU: Extent Mapper class

AU: Extent Mapper class

Extent Mapper will use the FIBMAP (widely supported) ioctl to find
out, for a given regular file, which extents it takes up on disk.

[Daniel Erat, 2010-02-23 20:08:51]

http://codereview.chromium.org/650199/diff/1/3
File src/platform/update_engine/extent_mapper.cc (right):

http://codereview.chromium.org/650199/diff/1/3#newcode9
src/platform/update_engine/extent_mapper.cc:9: #include <assert.h>
alphabetize these?

http://codereview.chromium.org/650199/diff/1/3#newcode23
src/platform/update_engine/extent_mapper.cc:23: const int kFieMapBufSize = 4096;
FileMap?  FibMap?  (except capitalization is inconsistent with the function
then).  better yet, just delete it, since you're not using it anywhere :-P

http://codereview.chromium.org/650199/diff/1/4
File src/platform/update_engine/extent_mapper.h (right):

http://codereview.chromium.org/650199/diff/1/4#newcode14
src/platform/update_engine/extent_mapper.h:14: bool ExtentsForFileFibmap(const
std::string& path, std::vector<Extent>* out);
Are you planning to move this into an ExtentMapper class later?  Sticking a
single function in a file with a different name like this seems like it'll make
it difficult to find...

http://codereview.chromium.org/650199/diff/1/5
File src/platform/update_engine/extent_mapper_unittest.cc (right):

http://codereview.chromium.org/650199/diff/1/5#newcode29
src/platform/update_engine/extent_mapper_unittest.cc:29: const string kFilename
= "update_engine_unittests";
this requires that the CWD matches the test's location -- do other tests already
depend on this?

[adlr, 2010-02-24 01:03:45]

http://codereview.chromium.org/650199/diff/1/3
File src/platform/update_engine/extent_mapper.cc (right):

http://codereview.chromium.org/650199/diff/1/3#newcode9
src/platform/update_engine/extent_mapper.cc:9: #include <assert.h>
On 2010/02/23 20:08:51, Daniel Erat wrote:
> alphabetize these?

i think they are... at i thought the ordering was:

- corresponding .h file
- sys standard include C files
- non-sys standard include C files
- libs
- local .h files

what order do you suggest?

http://codereview.chromium.org/650199/diff/1/3#newcode23
src/platform/update_engine/extent_mapper.cc:23: const int kFieMapBufSize = 4096;
On 2010/02/23 20:08:51, Daniel Erat wrote:
> FileMap?  FibMap?  (except capitalization is inconsistent with the function
> then).  better yet, just delete it, since you're not using it anywhere :-P

Done.

http://codereview.chromium.org/650199/diff/1/4
File src/platform/update_engine/extent_mapper.h (right):

http://codereview.chromium.org/650199/diff/1/4#newcode14
src/platform/update_engine/extent_mapper.h:14: bool ExtentsForFileFibmap(const
std::string& path, std::vector<Extent>* out);
On 2010/02/23 20:08:51, Daniel Erat wrote:
> Are you planning to move this into an ExtentMapper class later?  Sticking a
> single function in a file with a different name like this seems like it'll
make
> it difficult to find...

I wasn't planning to. Should I namespace it?

http://codereview.chromium.org/650199/diff/1/5
File src/platform/update_engine/extent_mapper_unittest.cc (right):

http://codereview.chromium.org/650199/diff/1/5#newcode29
src/platform/update_engine/extent_mapper_unittest.cc:29: const string kFilename
= "update_engine_unittests";
On 2010/02/23 20:08:51, Daniel Erat wrote:
> this requires that the CWD matches the test's location -- do other tests
already
> depend on this?

changed to always resolve to the exe no matter what cwd is

[Daniel Erat, 2010-02-24 01:37:44]

LGTM

http://codereview.chromium.org/650199/diff/1/3
File src/platform/update_engine/extent_mapper.cc (right):

http://codereview.chromium.org/650199/diff/1/3#newcode9
src/platform/update_engine/extent_mapper.cc:9: #include <assert.h>
On 2010/02/24 01:03:45, adlr wrote:
> On 2010/02/23 20:08:51, Daniel Erat wrote:
> > alphabetize these?
> 
> i think they are... at i thought the ordering was:
> 
> - corresponding .h file
> - sys standard include C files
> - non-sys standard include C files
> - libs
> - local .h files
> 
> what order do you suggest?

Ah, I didn't realize the sys vs. non-sys distinction; I think I mostly just do
corresponding .h, system C++ and C headers (i.e. STL and libc), angle-bracketed
headers, and then local ones.  Your order seems fine, but maybe add blank lines
between the groups to make the separation more distinct?

http://codereview.chromium.org/650199/diff/1/4
File src/platform/update_engine/extent_mapper.h (right):

http://codereview.chromium.org/650199/diff/1/4#newcode14
src/platform/update_engine/extent_mapper.h:14: bool ExtentsForFileFibmap(const
std::string& path, std::vector<Extent>* out);
On 2010/02/24 01:03:45, adlr wrote:
> On 2010/02/23 20:08:51, Daniel Erat wrote:
> > Are you planning to move this into an ExtentMapper class later?  Sticking a
> > single function in a file with a different name like this seems like it'll
> make
> > it difficult to find...
> 
> I wasn't planning to. Should I namespace it?

Your call, but I find that namespacing functions like this one or making them
static methods of a class makes it easier to infer where they're defined.