superreview requested: [Bug 737153] Enable mounting storage through USB from a host machine : [Attachment 617104] bug-737153 Add the VolumeManager

[bugzill...@mozilla.org]

Dave Hylands [:dhylands] <dhyl...@mozilla.com> has asked Chris Jones [:cjones]
[:warhammer] <jones....@gmail.com> for superreview:
Bug 737153: Enable mounting storage through USB from a host machine
https://bugzilla.mozilla.org/show_bug.cgi?id=737153

Attachment 617104: bug-737153 Add the VolumeManager
https://bugzilla.mozilla.org/attachment.cgi?id=617104&action=edit

[bugzill...@mozilla.org]

Dave Hylands [:dhylands] <dhyl...@mozilla.com> has asked Chris Jones [:cjones]
[:warhammer] <jones....@gmail.com> for superreview:
Bug 737153: Enable mounting storage through USB from a host machine
https://bugzilla.mozilla.org/show_bug.cgi?id=737153

Attachment 617115: bug-737153 - Add AutoMounter
https://bugzilla.mozilla.org/attachment.cgi?id=617115&action=edit

[bugzill...@mozilla.org]

Kyle Machulis [:kmachulis] [:qdot] <ky...@nonpolynomial.com> has canceled Dave
Hylands [:dhylands] <dhyl...@mozilla.com>'s request for superreview:

Bug 737153: Enable mounting storage through USB from a host machine
https://bugzilla.mozilla.org/show_bug.cgi?id=737153

Attachment 617115: bug-737153 - Add AutoMounter
https://bugzilla.mozilla.org/attachment.cgi?id=617115&action=edit

------- Additional Comments from Kyle Machulis [:kmachulis] [:qdot]
<ky...@nonpolynomial.com>
Review of attachment 617115:
-----------------------------------------------------------------

Mostly same stuff as Volumemanager here. We'll go deeper on the next round,
since I'm also working on understanding how the two work with each other.

::: dom/system/gonk/AutoMounter.cpp
@@ +87,5 @@
> +static bool
> +ReadSysFile(const char *filename, int *val)
> +{
> + int fd = open(filename, O_RDONLY);
> + ScopedClose autoClose(fd);

Might want to create the ScopedClose /after/ we know there's something to
close?

@@ +221,5 @@
> + bool mEnabled;
> + VolumeManager *mVolMgr;
> +};
> +
> +static AutoMounter *sAutoMounter = NULL;

RefPtr

@@ +252,5 @@
> +
> + ERR("Volume %s: Command '%s' failed: %d '%s'",
> + volume->Name().get(), cmd.CmdStr(), cmd.ResponseCode(),
cmd.ResponseStr().get());
> +
> + if (++mErrorCount < 3) {

MAX_ERROR_COUNT or something similar.

@@ +404,5 @@
> + MOZ_ASSERT(MessageLoop::current() == XRE_GetIOMessageLoop());
> + MOZ_ASSERT(sAutoMounter);
> +
> + delete sAutoMounter;
> + sAutoMounter = NULL;

Let the RefPtr take care of this once it's changed.

::: dom/system/gonk/AutoMounter.h
@@ +18,5 @@
> +
> +/**
> + * Shuts down the automounter.
> + */
> +void ShutdownAutoMounter();

This is much better function exposure than the VolumeManager stuff. This is the
kinda header I'm looking for. :D

[bugzill...@mozilla.org]

Dave Hylands [:dhylands] <dhyl...@mozilla.com> has asked Chris Jones [:cjones]

[:warhammer] <jones....@gmail.com> for superreview:

Bug 737153: Enable mounting storage through USB from a host machine
https://bugzilla.mozilla.org/show_bug.cgi?id=737153

Attachment 617375: bug-737153 Add the VolumeManager
https://bugzilla.mozilla.org/attachment.cgi?id=617375&action=edit

[bugzill...@mozilla.org]

Dave Hylands [:dhylands] <dhyl...@mozilla.com> has asked Chris Jones [:cjones]
[:warhammer] <jones....@gmail.com> for superreview:
Bug 737153: Enable mounting storage through USB from a host machine
https://bugzilla.mozilla.org/show_bug.cgi?id=737153

Attachment 617377: bug-737153 - Add AutoMounter
https://bugzilla.mozilla.org/attachment.cgi?id=617377&action=edit

[bugzill...@mozilla.org]

Dave Hylands [:dhylands] <dhyl...@mozilla.com> has canceled Dave Hylands
[:dhylands] <dhyl...@mozilla.com>'s request for superreview:

Bug 737153: Enable mounting storage through USB from a host machine
https://bugzilla.mozilla.org/show_bug.cgi?id=737153

[bugzill...@mozilla.org]

Dave Hylands [:dhylands] <dhyl...@mozilla.com> has canceled Dave Hylands
[:dhylands] <dhyl...@mozilla.com>'s request for superreview:
Bug 737153: Enable mounting storage through USB from a host machine
https://bugzilla.mozilla.org/show_bug.cgi?id=737153

[bugzill...@mozilla.org]

Dave Hylands [:dhylands] <dhyl...@mozilla.com> has asked Chris Jones [:cjones]

[:warhammer] <jones....@gmail.com> for superreview:

Bug 737153: Enable mounting storage through USB from a host machine
https://bugzilla.mozilla.org/show_bug.cgi?id=737153

Attachment 619999: Bug 737153 - Add the VolumeManager - v3
https://bugzilla.mozilla.org/attachment.cgi?id=619999&action=edit


------- Additional Comments from Dave Hylands [:dhylands]
<dhyl...@mozilla.com>
v3 - After mwu's comments

[bugzill...@mozilla.org]

Dave Hylands [:dhylands] <dhyl...@mozilla.com> has asked Chris Jones [:cjones]
[:warhammer] <jones....@gmail.com> for superreview:
Bug 737153: Enable mounting storage through USB from a host machine
https://bugzilla.mozilla.org/show_bug.cgi?id=737153

Attachment 620000: Bug 737153 - Add the AutoMounter - v3
https://bugzilla.mozilla.org/attachment.cgi?id=620000&action=edit

[bugzill...@mozilla.org]

Chris Jones [:cjones] [:warhammer] <jones....@gmail.com> has canceled Dave
Hylands [:dhylands] <dhyl...@mozilla.com>'s request for superreview:

Bug 737153: Enable mounting storage through USB from a host machine
https://bugzilla.mozilla.org/show_bug.cgi?id=737153

Attachment 619999: Bug 737153 - Add the VolumeManager - v3
https://bugzilla.mozilla.org/attachment.cgi?id=619999&action=edit

------- Additional Comments from Chris Jones [:cjones] [:warhammer]
<jones....@gmail.com>
>diff --git a/ipc/volumemanager/Volume.cpp b/ipc/volumemanager/Volume.cpp

Sorry Dave, hate to nitpick code locations more, this code belongs in
dom/system/gonk/ for now. The ipc/ directory is code that implements
IPC (and related things). This code /uses/ IPC, in service of DOM
interfaces. (For now, just DOM settings.) It's also gonk specific.

A general point about RefPtr: we use this convention to pass
refcounted objects around

class Object : Refcounted<Object> { ... };

void Foo(Object* aO) { ... }

RefPtr<Object> p(new Object(...));
Foo(p);

That is, the caller of |Foo()| "loans" Foo() its reference, and Foo()
can further loan that reference to callees. The reason we do this is
to eliminate a lot of extraneous ref/unref'ing of arguments.

Similarly, we have a helper TemporaryRef<T> to "forward" references to
refcounted objects returned from functions. So if you need to return
a refcounted object, you should do

TemporaryRef<Object> Bar() { ... }

RefPtr<Object> p = Bar();

This is again used to save unnecessary ref/unref.

Most of the time this usage doesn't matter perf-wise, but it's a good
habit to be in. There are places in the code where it does matter.

One more general nit: our coding style uses a stupid
Hungarian-notation-lite style to annotate function parameters, like so

void Function(const Foo& aParameter)

note the "a" prefix, for "argument". This is silly and worthless but
it's best to stay consistent with the rest of the code around DOM.

I won't call out those things specifically below.

>diff --git a/ipc/volumemanager/Volume.cpp b/ipc/volumemanager/Volume.cpp

>+void
>+Volume::CommandDone(const VolumeCommand &cmd, const VolumeResponse &response)

>+{
>+ Volume::Callback *cb = mCallback;
>+ mCallback = NULL;
>+
>+ cb->CommandDone(this, cmd, response);

This is invalid usage of mCallback ... that might be the last
reference. Instead, you want to do

RefPtr<Callback> cb = mCallback;
mCallback = NULL;
//...

It might be the case here that you can guarantee there's always
another reference, but that's a distributed invariant that's hard to
maintain. If the invariant isn't maintained, we have a security
vulnerability here. Not worth it ;).

>diff --git a/ipc/volumemanager/Volume.h b/ipc/volumemanager/Volume.h

>+namespace mozilla {
>+namespace ipc {

When this moves to dom/system/gonk, let's put it in namespace
mozilla::system. And the other classes here.

>+class Volume; // Forward declarations

Drop this comment.

>+class Volume : public RefCounted<Volume>
>+{

>+ void SetMountPoint(const nsCSubstring &mountPoint);

This method needs an explanatory comment about the mount point
parameter. Use |const nsACString&| for the type.

>+ void StartMount(RefPtr<Callback> cb);
>+ void StartUnmount(RefPtr<Callback> cb);
>+ void StartShare(RefPtr<Callback> cb);
>+ void StartUnshare(RefPtr<Callback> cb);

The state machine here needs to be documented, the fact that it's only
legal to have one or zero operations pending.

>diff --git a/ipc/volumemanager/VolumeCommand.h
b/ipc/volumemanager/VolumeCommand.h

>+class VolumeCommand : public RefCounted<VolumeCommand>
>+{

>+ VolumeCommand(const nsACString &cmd)
>+ : mCmd(cmd),
>+ mBytesConsumed(0)
>+ {
>+ // Add a null character. We want this to be included in the length since
>+ // vold uses it to determine the end of the command.

You can drop this duplicated comment and the one below. Or, factor
out this logic into a separate helper and put the comment there.

>+ const char *CmdStr() const { return mCmd.get(); }
>+ const char *Data() const { return mCmd.Data() +
mBytesConsumed; }
>+ void ConsumeBytes( size_t numBytes ) { mBytesConsumed += numBytes; }


Nit: |ConsumeBytes(size_t aNumBytes)|, no margin in parens.

Non-nit: there's nothing ensuring here that mBytesConsumed points
within mCmd. If a caller accidentally does Consume(1e10), then we
have a security vulnerability in Data(). Let's check this always (not
just debug-only).

>diff --git a/ipc/volumemanager/VolumeManager.cpp
b/ipc/volumemanager/VolumeManager.cpp

>+//static
>+RefPtr<Volume>
>+VolumeManager::FindVolumeByName(const nsCSubstring &name)

nsACString&

>+{
>+ if (!sVolumeManager) {
>+ return NULL;
>+ }
>+ for (VolumeArray::iterator volIter = sVolumeManager->mVolumeArray.begin();

CString has operator== so should be able to use std::find() here.

>diff --git a/ipc/volumemanager/VolumeManager.h
b/ipc/volumemanager/VolumeManager.h

>+class VolumeManager : public MessageLoopForIO::Watcher,
>+ public RefCounted<VolumeManager>
>+{
>+public:
>+
>+ typedef std::vector<RefPtr<Volume>> VolumeArray;

Need "> >" or this won't compile (C++ lexing ambiguity).

Nice work! I like this patch :).

I'd like to see the next version with these comments addressed.

[bugzill...@mozilla.org]

Chris Jones [:cjones] [:warhammer] <jones....@gmail.com> has canceled Dave
Hylands [:dhylands] <dhyl...@mozilla.com>'s request for superreview:
Bug 737153: Enable mounting storage through USB from a host machine
https://bugzilla.mozilla.org/show_bug.cgi?id=737153

Attachment 620000: Bug 737153 - Add the AutoMounter - v3
https://bugzilla.mozilla.org/attachment.cgi?id=620000&action=edit

------- Additional Comments from Chris Jones [:cjones] [:warhammer]
<jones....@gmail.com>

>diff --git a/hal/HalTypes.h b/hal/HalTypes.h

> enum SwitchDevice {
> SWITCH_DEVICE_UNKNOWN = -1,
> SWITCH_HEADPHONES,
>+ SWITCH_USB_CONFIGURATION,

Can we call this |SWITCH_USB| without losing important information?

>diff --git a/ipc/volumemanager/AutoMounter.cpp
b/ipc/volumemanager/AutoMounter.cpp

Sorry, same change --- let's stick this guy in dom/system/gonk for
now.

>+static bool
>+IsUsbCablePluggedIn()
>+{
>+#if 0
>+ // Use this code when bug 745078 gets fixed (or use whatever the
>+ // appropriate method is)

Make sure this code makes it over to 745078.

>diff --git a/ipc/volumemanager/AutoMounter.h b/ipc/volumemanager/AutoMounter.h


>+/**
>+ * Initialize the automounter. This causes some of the phone's
>+ * directories to show up on the host when the phone is plugged
>+ * into the host via USB.
>+ */
>+void InitAutoMounter();
>+

Please doc what happens on startup ... if the automounter sees a USB
cable when it starts up, will it auto-mount?

>+/**
>+ * Shuts down the automounter.
>+ */
>+void ShutdownAutoMounter();

Please document here what happens to fs's that have been mounted by
the auto-mounter ... are they shut down or left alone?

Looks good, again :). Would like to take another look after move.

[bugzill...@mozilla.org]

Dave Hylands [:dhylands] <dhyl...@mozilla.com> has asked Chris Jones [:cjones]
[:warhammer] <jones....@gmail.com> for superreview:

Bug 737153: Enable mounting storage through USB from a host machine
https://bugzilla.mozilla.org/show_bug.cgi?id=737153

Attachment 622593: v4 - After review my cjones and mwu
https://bugzilla.mozilla.org/attachment.cgi?id=622593&action=edit

[bugzill...@mozilla.org]

Dave Hylands [:dhylands] <dhyl...@mozilla.com> has asked Chris Jones [:cjones]
[:warhammer] <jones....@gmail.com> for superreview:
Bug 737153: Enable mounting storage through USB from a host machine
https://bugzilla.mozilla.org/show_bug.cgi?id=737153

Attachment 622594: v4 - After review my cjones and mwu
https://bugzilla.mozilla.org/attachment.cgi?id=622594&action=edit

[bugzill...@mozilla.org]

Dave Hylands [:dhylands] <dhyl...@mozilla.com> has asked Chris Jones [:cjones]
[:warhammer] <jones....@gmail.com> for superreview:
Bug 737153: Enable mounting storage through USB from a host machine
https://bugzilla.mozilla.org/show_bug.cgi?id=737153

Attachment 622793: Hookup VolumeManager and AutoMounter into the build
https://bugzilla.mozilla.org/attachment.cgi?id=622793&action=edit

------- Additional Comments from Dave Hylands [:dhylands]
<dhyl...@mozilla.com>

Fixed to only build when targeting the phone

[bugzill...@mozilla.org]

Dave Hylands [:dhylands] <dhyl...@mozilla.com> has asked Chris Jones [:cjones]
[:warhammer] <jones....@gmail.com> for superreview:
Bug 737153: Enable mounting storage through USB from a host machine
https://bugzilla.mozilla.org/show_bug.cgi?id=737153

Attachment 626242: v5 - Add the VolumeManager
https://bugzilla.mozilla.org/attachment.cgi?id=626242&action=edit

------- Additional Comments from Dave Hylands [:dhylands]
<dhyl...@mozilla.com>

Hooks in the AutoMounterSettings

[bugzill...@mozilla.org]

Dave Hylands [:dhylands] <dhyl...@mozilla.com> has canceled Dave Hylands
[:dhylands] <dhyl...@mozilla.com>'s request for superreview:

Bug 737153: Enable mounting storage through USB from a host machine
https://bugzilla.mozilla.org/show_bug.cgi?id=737153

Attachment 622593: v4 - Add VolumeManager
https://bugzilla.mozilla.org/attachment.cgi?id=622593&action=edit

[bugzill...@mozilla.org]

Dave Hylands [:dhylands] <dhyl...@mozilla.com> has asked Chris Jones [:cjones]

[:warhammer] <jones....@gmail.com> for superreview:

Bug 737153: Enable mounting storage through USB from a host machine
https://bugzilla.mozilla.org/show_bug.cgi?id=737153

Attachment 626244: v5 - Add AutoMounter
https://bugzilla.mozilla.org/attachment.cgi?id=626244&action=edit

------- Additional Comments from Dave Hylands [:dhylands]
<dhyl...@mozilla.com>

Adds the AutoMounterSettings.

[bugzill...@mozilla.org]

Dave Hylands [:dhylands] <dhyl...@mozilla.com> has canceled Dave Hylands

[:dhylands] <dhyl...@mozilla.com>'s request for superreview:

Bug 737153: Enable mounting storage through USB from a host machine
https://bugzilla.mozilla.org/show_bug.cgi?id=737153

Attachment 622594: v4 - Add AutoMounter
https://bugzilla.mozilla.org/attachment.cgi?id=622594&action=edit

[bugzill...@mozilla.org]

Dave Hylands [:dhylands] <dhyl...@mozilla.com> has canceled Dave Hylands
[:dhylands] <dhyl...@mozilla.com>'s request for superreview:
Bug 737153: Enable mounting storage through USB from a host machine
https://bugzilla.mozilla.org/show_bug.cgi?id=737153

Attachment 622793: Hookup VolumeManager and AutoMounter into the build
https://bugzilla.mozilla.org/attachment.cgi?id=622793&action=edit

------- Additional Comments from Dave Hylands [:dhylands]
<dhyl...@mozilla.com>

Adds AutoMounterSettings.

[bugzill...@mozilla.org]

Dave Hylands [:dhylands] <dhyl...@mozilla.com> has asked Chris Jones [:cjones]

[:warhammer] <jones....@gmail.com> for superreview:

Bug 737153: Enable mounting storage through USB from a host machine
https://bugzilla.mozilla.org/show_bug.cgi?id=737153

Attachment 626245: v6 - Hook up VolumeManager and AutoMounter
https://bugzilla.mozilla.org/attachment.cgi?id=626245&action=edit

[bugzill...@mozilla.org]

Dave Hylands [:dhylands] <dhyl...@mozilla.com> has canceled Dave Hylands

[:dhylands] <dhyl...@mozilla.com>'s request for superreview:

Bug 737153: Enable mounting storage through USB from a host machine
https://bugzilla.mozilla.org/show_bug.cgi?id=737153

[bugzill...@mozilla.org]

Dave Hylands [:dhylands] <dhyl...@mozilla.com> has asked Chris Jones [:cjones]

[:warhammer] <jones....@gmail.com> for superreview:

Bug 737153: Enable mounting storage through USB from a host machine
https://bugzilla.mozilla.org/show_bug.cgi?id=737153

Attachment 626503: v6 - Add the VolumeManager
https://bugzilla.mozilla.org/attachment.cgi?id=626503&action=edit

[bugzill...@mozilla.org]

Dave Hylands [:dhylands] <dhyl...@mozilla.com> has canceled Dave Hylands

[:dhylands] <dhyl...@mozilla.com>'s request for superreview:

Bug 737153: Enable mounting storage through USB from a host machine
https://bugzilla.mozilla.org/show_bug.cgi?id=737153

[bugzill...@mozilla.org]

Dave Hylands [:dhylands] <dhyl...@mozilla.com> has asked Chris Jones [:cjones]

[:warhammer] <jones....@gmail.com> for superreview:

Bug 737153: Enable mounting storage through USB from a host machine
https://bugzilla.mozilla.org/show_bug.cgi?id=737153

Attachment 626507: v6 - Add AutoMounter
https://bugzilla.mozilla.org/attachment.cgi?id=626507&action=edit

------- Additional Comments from Dave Hylands [:dhylands]
<dhyl...@mozilla.com>

Added AutoMounterSettings (new addition from last review by cjones)

[bugzill...@mozilla.org]

Chris Jones [:cjones] [:warhammer] <jones....@gmail.com> has granted Dave
Hylands [:dhylands] <dhyl...@mozilla.com>'s request for superreview:

Bug 737153: Enable mounting storage through USB from a host machine
https://bugzilla.mozilla.org/show_bug.cgi?id=737153

Attachment 626245: v6 - Hook up VolumeManager and AutoMounter
https://bugzilla.mozilla.org/attachment.cgi?id=626245&action=edit

------- Additional Comments from Chris Jones [:cjones] [:warhammer]
<jones....@gmail.com>
>diff --git a/dom/system/gonk/SystemWorkerManager.cpp
b/dom/system/gonk/SystemWorkerManager.cpp

>+#ifdef MOZ_WIDGET_GONK
>+ mozilla::system::InitAutoMounter();
>+#endif

>+#ifdef MOZ_WIDGET_GONK
>+ mozilla::system::ShutdownAutoMounter();
>+#endif

Let's |using| the explicit namespace references away.

[bugzill...@mozilla.org]

Chris Jones [:cjones] [:warhammer] <jones....@gmail.com> has granted Dave
Hylands [:dhylands] <dhyl...@mozilla.com>'s request for superreview:
Bug 737153: Enable mounting storage through USB from a host machine
https://bugzilla.mozilla.org/show_bug.cgi?id=737153

[bugzill...@mozilla.org]

Chris Jones [:cjones] [:warhammer] <jones....@gmail.com> has granted Dave
Hylands [:dhylands] <dhyl...@mozilla.com>'s request for superreview:
Bug 737153: Enable mounting storage through USB from a host machine
https://bugzilla.mozilla.org/show_bug.cgi?id=737153

[bugzill...@mozilla.org]

Dave Hylands [:dhylands] <dhyl...@mozilla.com> has granted superreview:

Bug 737153: Enable mounting storage through USB from a host machine
https://bugzilla.mozilla.org/show_bug.cgi?id=737153

Attachment 626643: Hook up VolumeManager and AutoMounter
https://bugzilla.mozilla.org/attachment.cgi?id=626643&action=edit

------- Additional Comments from Dave Hylands [:dhylands]
<dhyl...@mozilla.com>

https://bugzilla.mozilla.org/show_bug.cgi?id=737153

Added final changes suggested by cjones

[bugzill...@mozilla.org]

Dave Hylands [:dhylands] <dhyl...@mozilla.com> has granted superreview:
Bug 737153: Enable mounting storage through USB from a host machine
https://bugzilla.mozilla.org/show_bug.cgi?id=737153

Attachment 627040: v7 - Add AutoMounter
https://bugzilla.mozilla.org/attachment.cgi?id=627040&action=edit

------- Additional Comments from Dave Hylands [:dhylands]
<dhyl...@mozilla.com>

Fixed a bug in AutoMounter::SetMode

[bugzill...@mozilla.org]

Dave Hylands [:dhylands] <dhyl...@mozilla.com> has granted superreview:
Bug 737153: Enable mounting storage through USB from a host machine
https://bugzilla.mozilla.org/show_bug.cgi?id=737153

Attachment 627055: v8 - Add AutoMounter
https://bugzilla.mozilla.org/attachment.cgi?id=627055&action=edit

------- Additional Comments from Dave Hylands [:dhylands]
<dhyl...@mozilla.com>

Address Kyle's nits.

Fixed a bug in AutoMounter::SetMode

Fixed a compile problem on the try server