Adds USB writing for OS X.

Adds USB writing for OS X.

To support imageWriterPrivate on Mac for the Chromebook Recovery Tool we will have to write to USB drives on Mac.  OS X allows us to open a single FD with elevated privileges through running the authopen command.

Unmounting and claiming the disk are a bit more complicated as they require use of the Disk Arbitration framework.

Disk Arbitration is an asynchronous interface.  In order to simplify the code we use a mutex to block.

The core logic is derived from the previous recovery tool.  See googleclient/chrome/chromeos_recovery/mac/ChromeOSRecovery/AppController.mm in the internal Google repositories.

BUG=376398
BUG=318451
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278194

[Drew Haven, 2014-06-05 18:16:43]

Robert,

Here's the actual writing code we talked about.  It uses a similar pattern of
having an object that manages the thread.  Hopefully I took some the advice from
the other CL so it looks a bit cleaner.

[Robert Sesek, 2014-06-05 19:49:49]

Yes, this is in pretty good shape. It looks like you may have crossed two CLs
though?

https://codereview.chromium.org/294163008/diff/100001/chrome/utility/image_wr...
File chrome/utility/image_writer/image_writer_mac.cc (right):

https://codereview.chromium.org/294163008/diff/100001/chrome/utility/image_wr...
chrome/utility/image_writer/image_writer_mac.cc:63: CFStringRef reason = CFSTR(
Will this message ever be shown to the user? If so, it should be a translated
string.

https://codereview.chromium.org/294163008/diff/100001/chrome/utility/image_wr...
chrome/utility/image_writer/image_writer_mac.cc:87: void
DiskUnmounter::UnmountOnWorker(const std::string& device_path) {
DCHECK(cf_thread_.message_loop() == base::MessageLoop::current()) ?

https://codereview.chromium.org/294163008/diff/100001/chrome/utility/image_wr...
chrome/utility/image_writer/image_writer_mac.cc:120: bool
ImageWriter::IsValidDevice() {
I've seen this function before?

https://codereview.chromium.org/294163008/diff/100001/chrome/utility/image_wr...
chrome/utility/image_writer/image_writer_mac.cc:183: void
ImageWriter::UnmountVolumes(const base::Closure& continuation) {
These are for a different class…?

https://codereview.chromium.org/294163008/diff/100001/chrome/utility/image_wr...
File chrome/utility/image_writer/image_writer_mac.h (right):

https://codereview.chromium.org/294163008/diff/100001/chrome/utility/image_wr...
chrome/utility/image_writer/image_writer_mac.h:8: #include <vector>
nit: C++ system headers come after C system headers, with a blank line between
the two groups.

https://codereview.chromium.org/294163008/diff/100001/chrome/utility/image_wr...
chrome/utility/image_writer/image_writer_mac.h:28: class DiskUnmounter : public
base::SupportsWeakPtr<DiskUnmounter> {
The general preference is to use WeakPtrFactory rather than SupportsWeakPtr, so
that arbitrary clients can't use the object with ill-defined lifetime semantics.

There was a thread "[chromium-dev] Weak ptrs for derived types?" a while ago
that discussed this. WeakPtr is not threadsafe.

https://codereview.chromium.org/294163008/diff/100001/chrome/utility/image_wr...
chrome/utility/image_writer/image_writer_mac.h:36: void Unmount(const
std::string& device_path,
Is this class meant to unmount more than one volume, ever? It's hard to tell.

[Drew Haven, 2014-06-05 22:28:28]

See question about IsValidDevice

https://codereview.chromium.org/294163008/diff/100001/chrome/utility/image_wr...
File chrome/utility/image_writer/image_writer_mac.cc (right):

https://codereview.chromium.org/294163008/diff/100001/chrome/utility/image_wr...
chrome/utility/image_writer/image_writer_mac.cc:63: CFStringRef reason = CFSTR(
On 2014/06/05 19:49:48, rsesek wrote:
> Will this message ever be shown to the user? If so, it should be a translated
> string.

I don't think so.  I actually just copied out what we had in
ChromeOSRecovery/AppController.mm (see internal code search) from the old tool.

The docs don't really seem to make it clear what sort of message goes into the
dissenter.  It appears to be pretty arbitrary and it's up to the other
application to figure out what to do with the string.

We could probably make it less silly, but there's no fun in that.

https://codereview.chromium.org/294163008/diff/100001/chrome/utility/image_wr...
chrome/utility/image_writer/image_writer_mac.cc:87: void
DiskUnmounter::UnmountOnWorker(const std::string& device_path) {
On 2014/06/05 19:49:48, rsesek wrote:
> DCHECK(cf_thread_.message_loop() == base::MessageLoop::current()) ? 

That's reasonable.  It's a private method with a pretty obvious name though, so
unlikely to have a problem.  No harm in a check!

https://codereview.chromium.org/294163008/diff/100001/chrome/utility/image_wr...
chrome/utility/image_writer/image_writer_mac.cc:120: bool
ImageWriter::IsValidDevice() {
On 2014/06/05 19:49:48, rsesek wrote:
> I've seen this function before?

Yes! In fact I wanted to ask you about that.  I'm not sure where a good place to
put a common function that is platform-specific like this would be.  This
function is shared between the drive listing function
(chrome/browser/extensions) and the writing code (chrome/utility), since those
are separate API calls we need to be sure that the string actually passed to the
utility process for writing describes a valid disk.  The two can't import from
each other and I don't know what a good common location is.

https://codereview.chromium.org/294163008/diff/100001/chrome/utility/image_wr...
chrome/utility/image_writer/image_writer_mac.cc:183: void
ImageWriter::UnmountVolumes(const base::Closure& continuation) {
On 2014/06/05 19:49:48, rsesek wrote:
> These are for a different class…?

It's like what I did for the RemovableStorageProvider class.

Basically, we have a bunch of shared logic and just a few functions that are
different.  The pattern I'd gotten used to for that was shared code in class.h
and class.cc and then implement the platform-specific functions in class_mac.cc,
class_win.cc, etc.

In this case I have a helper-class that is used only by one platform, so I
tossed it in image_writer_mac.h and image_writer_mac.cc (which also includes
image_writer.h platform-specific code).  I think perhaps I should just separate
this out into disk_unmounter_mac.h/cc.

https://codereview.chromium.org/294163008/diff/100001/chrome/utility/image_wr...
File chrome/utility/image_writer/image_writer_mac.h (right):

https://codereview.chromium.org/294163008/diff/100001/chrome/utility/image_wr...
chrome/utility/image_writer/image_writer_mac.h:8: #include <vector>
On 2014/06/05 19:49:48, rsesek wrote:
> nit: C++ system headers come after C system headers, with a blank line between
> the two groups.

vector is not actually needed here.  Removed.

https://codereview.chromium.org/294163008/diff/100001/chrome/utility/image_wr...
chrome/utility/image_writer/image_writer_mac.h:28: class DiskUnmounter : public
base::SupportsWeakPtr<DiskUnmounter> {
On 2014/06/05 19:49:48, rsesek wrote:
> The general preference is to use WeakPtrFactory rather than SupportsWeakPtr,
so
> that arbitrary clients can't use the object with ill-defined lifetime
semantics.
> 
> There was a thread "[chromium-dev] Weak ptrs for derived types?" a while ago
> that discussed this. WeakPtr is not threadsafe.

So, I looked at the logic and we don't even need a WeakPtr here.  The cf_thread_
is last to ensure the thread is stopped before any of the data members get
touched, so in the case of destruction the callbacks that reference this class
will all have run or been cancelled by the time this method is destroyed.

The only funny part would be if there were a reference to cf_thread_ as it was
stopping and so was in an invalid state.  The only references to cf_thread_ are
from the thread that owns this object so there is no concurrency there, and the
one DCHECK I just added which should have minimal impact.

https://codereview.chromium.org/294163008/diff/100001/chrome/utility/image_wr...
chrome/utility/image_writer/image_writer_mac.h:36: void Unmount(const
std::string& device_path,
On 2014/06/05 19:49:48, rsesek wrote:
> Is this class meant to unmount more than one volume, ever? It's hard to tell.

No.  The ImageWriter/ImageWriterHandler restrict the thread to one operation at
a time.  This is partially just to keep things simple for the IPCs so they don't
have to track which operation they are talking about.  Multiple requests from
the browser thread should each be spawning their own utility thread and so
should not overlap.

This class is intended to just be an internal implementation detail of the mac
implementation, so I'm assuming that is already handled.  I'll make a note of it
anyway.

[Robert Sesek, 2014-06-06 18:34:06]

https://codereview.chromium.org/294163008/diff/100001/chrome/utility/image_wr...
File chrome/utility/image_writer/image_writer_mac.cc (right):

https://codereview.chromium.org/294163008/diff/100001/chrome/utility/image_wr...
chrome/utility/image_writer/image_writer_mac.cc:120: bool
ImageWriter::IsValidDevice() {
On 2014/06/05 22:28:27, Drew Haven wrote:
> On 2014/06/05 19:49:48, rsesek wrote:
> > I've seen this function before?
> 
> Yes! In fact I wanted to ask you about that.  I'm not sure where a good place
to
> put a common function that is platform-specific like this would be.  This
> function is shared between the drive listing function
> (chrome/browser/extensions) and the writing code (chrome/utility), since those
> are separate API calls we need to be sure that the string actually passed to
the
> utility process for writing describes a valid disk.  The two can't import from
> each other and I don't know what a good common location is.

//{chrome,content}/common is where files shared by multiple processes go. So how
about //chrome/common/extensions/...?

https://codereview.chromium.org/294163008/diff/100001/chrome/utility/image_wr...
chrome/utility/image_writer/image_writer_mac.cc:183: void
ImageWriter::UnmountVolumes(const base::Closure& continuation) {
On 2014/06/05 22:28:27, Drew Haven wrote:
> On 2014/06/05 19:49:48, rsesek wrote:
> > These are for a different class…?
> 
> It's like what I did for the RemovableStorageProvider class.
> 
> Basically, we have a bunch of shared logic and just a few functions that are
> different.  The pattern I'd gotten used to for that was shared code in class.h
> and class.cc and then implement the platform-specific functions in
class_mac.cc,
> class_win.cc, etc.
> 
> In this case I have a helper-class that is used only by one platform, so I
> tossed it in image_writer_mac.h and image_writer_mac.cc (which also includes
> image_writer.h platform-specific code).  I think perhaps I should just
separate
> this out into disk_unmounter_mac.h/cc.

Ah, I see. Yes, I'd not have an image_writer_mac.h and just an
image_writer_mac.cc instead, and then move the DiskUnmounter stuff to its own
file, which you did :).

https://codereview.chromium.org/294163008/diff/140001/chrome/utility/image_wr...
File chrome/utility/image_writer/disk_unmounter_mac.h (right):

https://codereview.chromium.org/294163008/diff/140001/chrome/utility/image_wr...
chrome/utility/image_writer/disk_unmounter_mac.h:25: // class can only handle
one unmount operation at a time and calling Unmount
That's not quite accurate. You can only ever call Unmount() once, otherwise it
will try to start an already-started thread. I'd maybe restructure the
ctor/Unmount in one of these ways:

1. Do all the work in the ctor and remove Unmount()
2. In the ctor, create the thread, but switch to storing original_thread_ and
image_writer in Unmount()
3. Do all the work in Unmount(), including accepting image_writer as a parameter

I think (2) is probably cleanest. And that allows for multiple unmounts more
cleanly in the future.

[Drew Haven, 2014-06-10 01:13:26]

https://codereview.chromium.org/294163008/diff/100001/chrome/utility/image_wr...
File chrome/utility/image_writer/image_writer_mac.cc (right):

https://codereview.chromium.org/294163008/diff/100001/chrome/utility/image_wr...
chrome/utility/image_writer/image_writer_mac.cc:120: bool
ImageWriter::IsValidDevice() {
On 2014/06/06 18:34:06, rsesek wrote:
> On 2014/06/05 22:28:27, Drew Haven wrote:
> > On 2014/06/05 19:49:48, rsesek wrote:
> > > I've seen this function before?
> > 
> > Yes! In fact I wanted to ask you about that.  I'm not sure where a good
place
> to
> > put a common function that is platform-specific like this would be.  This
> > function is shared between the drive listing function
> > (chrome/browser/extensions) and the writing code (chrome/utility), since
those
> > are separate API calls we need to be sure that the string actually passed to
> the
> > utility process for writing describes a valid disk.  The two can't import
from
> > each other and I don't know what a good common location is.
> 
> //{chrome,content}/common is where files shared by multiple processes go. So
how
> about //chrome/common/extensions/...?

I ended up adding a bunch of files over there so I could also move the
unit-test.  Let me know what you think.  I'm not sure if exposing it as just a
plain-old-function is the best way to go, but it seemed the simplest.

https://codereview.chromium.org/294163008/diff/100001/chrome/utility/image_wr...
chrome/utility/image_writer/image_writer_mac.cc:183: void
ImageWriter::UnmountVolumes(const base::Closure& continuation) {
On 2014/06/06 18:34:06, rsesek wrote:
> On 2014/06/05 22:28:27, Drew Haven wrote:
> > On 2014/06/05 19:49:48, rsesek wrote:
> > > These are for a different class…?
> > 
> > It's like what I did for the RemovableStorageProvider class.
> > 
> > Basically, we have a bunch of shared logic and just a few functions that are
> > different.  The pattern I'd gotten used to for that was shared code in
class.h
> > and class.cc and then implement the platform-specific functions in
> class_mac.cc,
> > class_win.cc, etc.
> > 
> > In this case I have a helper-class that is used only by one platform, so I
> > tossed it in image_writer_mac.h and image_writer_mac.cc (which also includes
> > image_writer.h platform-specific code).  I think perhaps I should just
> separate
> > this out into disk_unmounter_mac.h/cc.
> 
> Ah, I see. Yes, I'd not have an image_writer_mac.h and just an
> image_writer_mac.cc instead, and then move the DiskUnmounter stuff to its own
> file, which you did :).

Done.

https://codereview.chromium.org/294163008/diff/140001/chrome/utility/image_wr...
File chrome/utility/image_writer/disk_unmounter_mac.h (right):

https://codereview.chromium.org/294163008/diff/140001/chrome/utility/image_wr...
chrome/utility/image_writer/disk_unmounter_mac.h:25: // class can only handle
one unmount operation at a time and calling Unmount
On 2014/06/06 18:34:06, rsesek wrote:
> That's not quite accurate. You can only ever call Unmount() once, otherwise it
> will try to start an already-started thread. I'd maybe restructure the
> ctor/Unmount in one of these ways:
> 
> 1. Do all the work in the ctor and remove Unmount()
> 2. In the ctor, create the thread, but switch to storing original_thread_ and
> image_writer in Unmount()
> 3. Do all the work in Unmount(), including accepting image_writer as a
parameter
> 
> I think (2) is probably cleanest. And that allows for multiple unmounts more
> cleanly in the future.

I agree.  I followed up on that.  Creation of the DiskUnmounter should create
the thread now, and then the callback info is stored when Unmounting.  I also
just cleaned it up so there is just a failure and success callback and the
unmounter doesn't actually care about its caller.

[Robert Sesek, 2014-06-10 19:47:47]

https://codereview.chromium.org/294163008/diff/180001/chrome/common/extension...
File chrome/common/extensions/image_writer/image_writer_test_util_mac.h (right):

https://codereview.chromium.org/294163008/diff/180001/chrome/common/extension...
chrome/common/extensions/image_writer/image_writer_test_util_mac.h:18: typedef
enum DriveType {
In C++, just |enum DriveType| is enough to declare a type.

https://codereview.chromium.org/294163008/diff/180001/chrome/common/extension...
File chrome/common/extensions/image_writer/image_writer_util_mac.cc (right):

https://codereview.chromium.org/294163008/diff/180001/chrome/common/extension...
chrome/common/extensions/image_writer/image_writer_util_mac.cc:42:
CFEqual(protocol, CFSTR(kIOPropertyPhysicalInterconnectTypeUSB)) &&
Maybe pull out line 42, 43, and 44 into its own bool, since it's common between
the two? bool is_removable_usb_device?

https://codereview.chromium.org/294163008/diff/180001/chrome/common/extension...
File chrome/common/extensions/image_writer/image_writer_util_mac_unittest.cc
(right):

https://codereview.chromium.org/294163008/diff/180001/chrome/common/extension...
chrome/common/extensions/image_writer/image_writer_util_mac_unittest.cc:19:
TEST_F(ImageWriterTestUtilsTest, DiskIsValidTarget) {
You can just use TEST(ImageWriterTestUtilsTest, DiskIsValidTarget) and remove
the subclass above.

https://codereview.chromium.org/294163008/diff/180001/chrome/utility/image_wr...
File chrome/utility/image_writer/disk_unmounter_mac.h (right):

https://codereview.chromium.org/294163008/diff/180001/chrome/utility/image_wr...
chrome/utility/image_writer/disk_unmounter_mac.h:30: virtual
~DiskUnmounterMac();
Need not be virtual.

https://codereview.chromium.org/294163008/diff/180001/chrome/utility/image_wr...
File chrome/utility/image_writer/image_writer_mac.cc (right):

https://codereview.chromium.org/294163008/diff/180001/chrome/utility/image_wr...
chrome/utility/image_writer/image_writer_mac.cc:54: pid_t childPid = fork();
This should probably use base/process/launch.h.

https://codereview.chromium.org/294163008/diff/180001/chrome/utility/image_wr...
chrome/utility/image_writer/image_writer_mac.cc:69: real_device_path =
base::FilePath("/dev").Append(device_path_);
Yikes, this is going to do allocation between fork/exec. You can't trust Libc
right now, so this won't fly. C.f.
https://code.google.com/p/chromium/issues/detail?id=36678

https://codereview.chromium.org/294163008/diff/180001/chrome/utility/image_wr...
chrome/utility/image_writer/image_writer_mac.cc:85: msghdr message = {0};
What is this code supposed to do? Commentary definitely required.

[Robert Sesek, 2014-06-10 19:48:08]

This design is in pretty good shape now, most of the comments are focused on
image_writer_mac.cc.

[Drew Haven, 2014-06-12 02:24:27]

https://codereview.chromium.org/294163008/diff/180001/chrome/common/extension...
File chrome/common/extensions/image_writer/image_writer_test_util_mac.h (right):

https://codereview.chromium.org/294163008/diff/180001/chrome/common/extension...
chrome/common/extensions/image_writer/image_writer_test_util_mac.h:18: typedef
enum DriveType {
On 2014/06/10 19:47:46, rsesek wrote:
> In C++, just |enum DriveType| is enough to declare a type.

Done.  Bad habit.

https://codereview.chromium.org/294163008/diff/180001/chrome/common/extension...
File chrome/common/extensions/image_writer/image_writer_util_mac.cc (right):

https://codereview.chromium.org/294163008/diff/180001/chrome/common/extension...
chrome/common/extensions/image_writer/image_writer_util_mac.cc:42:
CFEqual(protocol, CFSTR(kIOPropertyPhysicalInterconnectTypeUSB)) &&
On 2014/06/10 19:47:46, rsesek wrote:
> Maybe pull out line 42, 43, and 44 into its own bool, since it's common
between
> the two? bool is_removable_usb_device?

Done.

https://codereview.chromium.org/294163008/diff/180001/chrome/common/extension...
File chrome/common/extensions/image_writer/image_writer_util_mac_unittest.cc
(right):

https://codereview.chromium.org/294163008/diff/180001/chrome/common/extension...
chrome/common/extensions/image_writer/image_writer_util_mac_unittest.cc:19:
TEST_F(ImageWriterTestUtilsTest, DiskIsValidTarget) {
On 2014/06/10 19:47:46, rsesek wrote:
> You can just use TEST(ImageWriterTestUtilsTest, DiskIsValidTarget) and remove
> the subclass above.

Done.

https://codereview.chromium.org/294163008/diff/180001/chrome/utility/image_wr...
File chrome/utility/image_writer/disk_unmounter_mac.h (right):

https://codereview.chromium.org/294163008/diff/180001/chrome/utility/image_wr...
chrome/utility/image_writer/disk_unmounter_mac.h:30: virtual
~DiskUnmounterMac();
On 2014/06/10 19:47:46, rsesek wrote:
> Need not be virtual.

Isn't it good practice to make a destructor virtual just in case it ever gets
subclassed?

https://codereview.chromium.org/294163008/diff/180001/chrome/utility/image_wr...
File chrome/utility/image_writer/image_writer_mac.cc (right):

https://codereview.chromium.org/294163008/diff/180001/chrome/utility/image_wr...
chrome/utility/image_writer/image_writer_mac.cc:54: pid_t childPid = fork();
On 2014/06/10 19:47:46, rsesek wrote:
> This should probably use base/process/launch.h.

Done.

https://codereview.chromium.org/294163008/diff/180001/chrome/utility/image_wr...
chrome/utility/image_writer/image_writer_mac.cc:69: real_device_path =
base::FilePath("/dev").Append(device_path_);
On 2014/06/10 19:47:46, rsesek wrote:
> Yikes, this is going to do allocation between fork/exec. You can't trust Libc
> right now, so this won't fly. C.f.
> https://code.google.com/p/chromium/issues/detail?id=36678

With the switch to LaunchProcess I think we should be fine.

https://codereview.chromium.org/294163008/diff/180001/chrome/utility/image_wr...
chrome/utility/image_writer/image_writer_mac.cc:85: msghdr message = {0};
On 2014/06/10 19:47:47, rsesek wrote:
> What is this code supposed to do? Commentary definitely required.

I commented it up a bit.  I couldn't figure out a good way to do this simple
receive with any of the IPC classes because they are built around the idea of
Listeners and Senders and I just want to receive a single known message without
setting up any of that.

[Robert Sesek, 2014-06-12 22:01:35]

https://codereview.chromium.org/294163008/diff/200001/chrome/utility/image_wr...
File chrome/utility/image_writer/image_writer_mac.cc (right):

https://codereview.chromium.org/294163008/diff/200001/chrome/utility/image_wr...
chrome/utility/image_writer/image_writer_mac.cc:20: static const char*
kAuthOpenPath = "/usr/libexec/authopen";
Use const char kAuthOpenPath[] = "..."; instead to avoid creating a static
initializer.

https://codereview.chromium.org/294163008/diff/200001/chrome/utility/image_wr...
chrome/utility/image_writer/image_writer_mac.cc:99: base::CommandLine
cmd_line((base::FilePath(kAuthOpenPath)));
I have two minds about this authopen stuff. On the one hand, it seems fairly
straightforward. On the other hand, the truly correct way to do this would be
with Authorization Services/Security Framework:

https://developer.apple.com/library/mac/documentation/security/Conceptual/aut...

I don't have much familiarity with that framework, though, so I'm a bit hesitant
to send you down an unknown path.

https://codereview.chromium.org/294163008/diff/200001/chrome/utility/image_wr...
chrome/utility/image_writer/image_writer_mac.cc:99: base::CommandLine
cmd_line((base::FilePath(kAuthOpenPath)));
nit: double parens around this

https://codereview.chromium.org/294163008/diff/200001/chrome/utility/image_wr...
chrome/utility/image_writer/image_writer_mac.cc:110: // TODO: close stuff.
This seems like a rather important TODO.

https://codereview.chromium.org/294163008/diff/200001/chrome/utility/image_wr...
chrome/utility/image_writer/image_writer_mac.cc:115: // extension. 
|IPC::Channel| is built around |IPC::Listener|s, and it's
Well, that and the man page for `authopen` explicitly says to use sendmsg() with
-stdoutpipe.

https://codereview.chromium.org/294163008/diff/200001/chrome/utility/image_wr...
chrome/utility/image_writer/image_writer_mac.cc:121: char
dataBuffer[kDataBufferSize];
Naming: use under_scores throughout this function.

https://codereview.chromium.org/294163008/diff/200001/chrome/utility/image_wr...
chrome/utility/image_writer/image_writer_mac.cc:126: message.msg_iovlen = 1;
sizeof(ioVec)

https://codereview.chromium.org/294163008/diff/200001/chrome/utility/image_wr...
chrome/utility/image_writer/image_writer_mac.cc:127: const socklen_t
kCmsgSocketSize = (socklen_t)CMSG_SPACE(sizeof(int));
C style casts are banned, and below.

[Drew Haven, 2014-06-12 22:55:10]

https://codereview.chromium.org/294163008/diff/200001/chrome/utility/image_wr...
File chrome/utility/image_writer/image_writer_mac.cc (right):

https://codereview.chromium.org/294163008/diff/200001/chrome/utility/image_wr...
chrome/utility/image_writer/image_writer_mac.cc:20: static const char*
kAuthOpenPath = "/usr/libexec/authopen";
On 2014/06/12 22:01:34, rsesek wrote:
> Use const char kAuthOpenPath[] = "..."; instead to avoid creating a static
> initializer.

Done.

https://codereview.chromium.org/294163008/diff/200001/chrome/utility/image_wr...
chrome/utility/image_writer/image_writer_mac.cc:99: base::CommandLine
cmd_line((base::FilePath(kAuthOpenPath)));
On 2014/06/12 22:01:35, rsesek wrote:
> nit: double parens around this

That was actually prompted by the compiler because a single-parens is
interpreted as a function declaration.  Maybe we have a policy of always doing
an assignment for variable initialization?

https://codereview.chromium.org/294163008/diff/200001/chrome/utility/image_wr...
chrome/utility/image_writer/image_writer_mac.cc:99: base::CommandLine
cmd_line((base::FilePath(kAuthOpenPath)));
On 2014/06/12 22:01:34, rsesek wrote:
> I have two minds about this authopen stuff. On the one hand, it seems fairly
> straightforward. On the other hand, the truly correct way to do this would be
> with Authorization Services/Security Framework:
> 
>
https://developer.apple.com/library/mac/documentation/security/Conceptual/aut...
> 
> I don't have much familiarity with that framework, though, so I'm a bit
hesitant
> to send you down an unknown path.

At this point I'd like to just go with this implementation since it's fairly
straight forward.  Yet another library to learn might cause my brain to explode.
 I just went from what the recovery tool was doing.

https://codereview.chromium.org/294163008/diff/200001/chrome/utility/image_wr...
chrome/utility/image_writer/image_writer_mac.cc:110: // TODO: close stuff.
On 2014/06/12 22:01:34, rsesek wrote:
> This seems like a rather important TODO.

ha, oops.  I did it by making sure everything was scoped and allocated to local
variables.  I just forgot to go back and remove the TODO.

https://codereview.chromium.org/294163008/diff/200001/chrome/utility/image_wr...
chrome/utility/image_writer/image_writer_mac.cc:115: // extension. 
|IPC::Channel| is built around |IPC::Listener|s, and it's
On 2014/06/12 22:01:35, rsesek wrote:
> Well, that and the man page for `authopen` explicitly says to use sendmsg()
with
> -stdoutpipe.

Done.

https://codereview.chromium.org/294163008/diff/200001/chrome/utility/image_wr...
chrome/utility/image_writer/image_writer_mac.cc:121: char
dataBuffer[kDataBufferSize];
On 2014/06/12 22:01:35, rsesek wrote:
> Naming: use under_scores throughout this function.

Whoops, copy-paste.

https://codereview.chromium.org/294163008/diff/200001/chrome/utility/image_wr...
chrome/utility/image_writer/image_writer_mac.cc:126: message.msg_iovlen = 1;
On 2014/06/12 22:01:35, rsesek wrote:
> sizeof(ioVec)

Done.

https://codereview.chromium.org/294163008/diff/200001/chrome/utility/image_wr...
chrome/utility/image_writer/image_writer_mac.cc:127: const socklen_t
kCmsgSocketSize = (socklen_t)CMSG_SPACE(sizeof(int));
On 2014/06/12 22:01:34, rsesek wrote:
> C style casts are banned, and below.

Done.

[Drew Haven, 2014-06-16 18:23:38]

Ping?

I'm hoping this is getting into good shape and we can get it all behind us. :)

[Robert Sesek, 2014-06-16 19:32:54]

On 2014/06/16 18:23:38, Drew Haven wrote:
> Ping?
> 
> I'm hoping this is getting into good shape and we can get it all behind us. :)

Sorry, I didn't have energy enough to do this review on Friday :).

https://codereview.chromium.org/294163008/diff/200001/chrome/utility/image_wr...
File chrome/utility/image_writer/image_writer_mac.cc (right):

https://codereview.chromium.org/294163008/diff/200001/chrome/utility/image_wr...
chrome/utility/image_writer/image_writer_mac.cc:99: base::CommandLine
cmd_line((base::FilePath(kAuthOpenPath)));
On 2014/06/12 22:55:10, Drew Haven wrote:
> On 2014/06/12 22:01:35, rsesek wrote:
> > nit: double parens around this
> 
> That was actually prompted by the compiler because a single-parens is
> interpreted as a function declaration.  Maybe we have a policy of always doing
> an assignment for variable initialization?

Yes, double parens look weird here. Use assignment instead.

https://codereview.chromium.org/294163008/diff/220001/chrome/utility/image_wr...
File chrome/utility/image_writer/disk_unmounter_mac.cc (right):

https://codereview.chromium.org/294163008/diff/220001/chrome/utility/image_wr...
chrome/utility/image_writer/disk_unmounter_mac.cc:32: const base::Closure&
failure_continuation) {
Maybe DCHECK(!original_thread_)?

https://codereview.chromium.org/294163008/diff/220001/chrome/utility/image_wr...
chrome/utility/image_writer/disk_unmounter_mac.cc:115:
scoped_ptr<base::MessagePump> DiskUnmounterMac::CreateMessagePump() {
nit: implementation order should match header order

https://codereview.chromium.org/294163008/diff/220001/chrome/utility/image_wr...
File chrome/utility/image_writer/image_writer_mac.cc (right):

https://codereview.chromium.org/294163008/diff/220001/chrome/utility/image_wr...
chrome/utility/image_writer/image_writer_mac.cc:5: #include <sys/socket.h>
nit: s > I (assuming capitalization doesn't matter, do whatever PRESUBMIT.py
says).

https://codereview.chromium.org/294163008/diff/220001/chrome/utility/image_wr...
chrome/utility/image_writer/image_writer_mac.cc:77: LOG(ERROR) << "Unable to
allocate socket pair.";
Use PLOG to capture errno in the output.

https://codereview.chromium.org/294163008/diff/220001/chrome/utility/image_wr...
chrome/utility/image_writer/image_writer_mac.cc:116: char
data_buffer[kDataBufferSize];
What is data_buffer used for and where did the value for |kDataBufferSize| come
from?

https://codereview.chromium.org/294163008/diff/220001/chrome/utility/image_wr...
chrome/utility/image_writer/image_writer_mac.cc:118: iovec io_vec[1];
I know it's not necessary in C++, but since this is a C struct, I like using
|struct iovec io_vec|.

https://codereview.chromium.org/294163008/diff/220001/chrome/utility/image_wr...
chrome/utility/image_writer/image_writer_mac.cc:124: char
cmsgSocket[kCmsgSocketSize];
naming: cmsg_socket

https://codereview.chromium.org/294163008/diff/220001/chrome/utility/image_wr...
chrome/utility/image_writer/image_writer_mac.cc:126: msghdr message = {0};
Same, |struct msghdr|.

https://codereview.chromium.org/294163008/diff/220001/chrome/utility/image_wr...
chrome/utility/image_writer/image_writer_mac.cc:134: cmsghdr* cmsgSocketHeader =
CMSG_FIRSTHDR(&message);
Same, |struct smsghdr|.

[Drew Haven, 2014-06-16 21:51:39]

No worries about the energy problems.  It happens.  I find myself feeling the
same way sometimes, particularly about reviews.  However, I'm under a bit of
time pressure since I'm switching to another team in two weeks.

https://codereview.chromium.org/294163008/diff/220001/chrome/utility/image_wr...
File chrome/utility/image_writer/disk_unmounter_mac.cc (right):

https://codereview.chromium.org/294163008/diff/220001/chrome/utility/image_wr...
chrome/utility/image_writer/disk_unmounter_mac.cc:32: const base::Closure&
failure_continuation) {
On 2014/06/16 19:32:53, rsesek wrote:
> Maybe DCHECK(!original_thread_)?

I forget that we use these debug assertions to enforce constraints.  I'm used to
trying to put those in the type system, but this works.

Done.

https://codereview.chromium.org/294163008/diff/220001/chrome/utility/image_wr...
chrome/utility/image_writer/disk_unmounter_mac.cc:115:
scoped_ptr<base::MessagePump> DiskUnmounterMac::CreateMessagePump() {
On 2014/06/16 19:32:53, rsesek wrote:
> nit: implementation order should match header order

Done.

https://codereview.chromium.org/294163008/diff/220001/chrome/utility/image_wr...
File chrome/utility/image_writer/image_writer_mac.cc (right):

https://codereview.chromium.org/294163008/diff/220001/chrome/utility/image_wr...
chrome/utility/image_writer/image_writer_mac.cc:5: #include <sys/socket.h>
On 2014/06/16 19:32:54, rsesek wrote:
> nit: s > I (assuming capitalization doesn't matter, do whatever PRESUBMIT.py
> says).

Done.  I don't think the 'git cl upload' presubmit checks complained either way.
 They might not care about the order of system includes because those may have
ordering constraints (like they do on windows).

https://codereview.chromium.org/294163008/diff/220001/chrome/utility/image_wr...
chrome/utility/image_writer/image_writer_mac.cc:77: LOG(ERROR) << "Unable to
allocate socket pair.";
On 2014/06/16 19:32:53, rsesek wrote:
> Use PLOG to capture errno in the output.

Done.

https://codereview.chromium.org/294163008/diff/220001/chrome/utility/image_wr...
chrome/utility/image_writer/image_writer_mac.cc:116: char
data_buffer[kDataBufferSize];
On 2014/06/16 19:32:53, rsesek wrote:
> What is data_buffer used for and where did the value for |kDataBufferSize|
come
> from?

So, I looked into it.  What we want is the size of the header plus enough room
for the file handle, I've adjusted accordingly and tested on my system to verify
it's true.

https://codereview.chromium.org/294163008/diff/220001/chrome/utility/image_wr...
chrome/utility/image_writer/image_writer_mac.cc:118: iovec io_vec[1];
On 2014/06/16 19:32:54, rsesek wrote:
> I know it's not necessary in C++, but since this is a C struct, I like using
> |struct iovec io_vec|.

Done.

https://codereview.chromium.org/294163008/diff/220001/chrome/utility/image_wr...
chrome/utility/image_writer/image_writer_mac.cc:124: char
cmsgSocket[kCmsgSocketSize];
On 2014/06/16 19:32:54, rsesek wrote:
> naming: cmsg_socket

Done.

https://codereview.chromium.org/294163008/diff/220001/chrome/utility/image_wr...
chrome/utility/image_writer/image_writer_mac.cc:126: msghdr message = {0};
On 2014/06/16 19:32:54, rsesek wrote:
> Same, |struct msghdr|.

Done.

https://codereview.chromium.org/294163008/diff/220001/chrome/utility/image_wr...
chrome/utility/image_writer/image_writer_mac.cc:134: cmsghdr* cmsgSocketHeader =
CMSG_FIRSTHDR(&message);
On 2014/06/16 19:32:53, rsesek wrote:
> Same, |struct smsghdr|.

Done.  Variable name as well.

[Robert Sesek, 2014-06-16 22:15:21]

LGTM w/ a nit

https://codereview.chromium.org/294163008/diff/240001/chrome/utility/image_wr...
File chrome/utility/image_writer/image_writer_mac.cc (right):

https://codereview.chromium.org/294163008/diff/240001/chrome/utility/image_wr...
chrome/utility/image_writer/image_writer_mac.cc:138: fd =
*reinterpret_cast<int*>(CMSG_DATA(cmsg_socket_header));
nit: braces needed around body since the condition spans more than one line

[Drew Haven, 2014-06-17 00:00:45]

https://codereview.chromium.org/294163008/diff/240001/chrome/utility/image_wr...
File chrome/utility/image_writer/image_writer_mac.cc (right):

https://codereview.chromium.org/294163008/diff/240001/chrome/utility/image_wr...
chrome/utility/image_writer/image_writer_mac.cc:138: fd =
*reinterpret_cast<int*>(CMSG_DATA(cmsg_socket_header));
On 2014/06/16 22:15:21, rsesek wrote:
> nit: braces needed around body since the condition spans more than one line

Done.

[Drew Haven, 2014-06-17 01:36:35]

The CQ bit was checked by haven@chromium.org

[commit-bot: I haz the power, 2014-06-17 01:40:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haven@chromium.org/294163008/300001

[commit-bot: I haz the power, 2014-06-17 10:30:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-17 10:30:51]

Try jobs failed on following builders:
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/20960)

[Drew Haven, 2014-06-17 21:49:57]

The CQ bit was checked by haven@chromium.org

[commit-bot: I haz the power, 2014-06-17 21:51:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haven@chromium.org/294163008/320001

[commit-bot: I haz the power, 2014-06-18 09:25:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-18 09:25:35]

Try jobs failed on following builders:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[Drew Haven, 2014-06-18 17:52:34]

The CQ bit was checked by haven@chromium.org

[commit-bot: I haz the power, 2014-06-18 17:54:00]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haven@chromium.org/294163008/320001

[commit-bot: I haz the power, 2014-06-18 23:47:37]

Change committed as 278194