737153 - Enable mounting storage through USB from a host machine

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

This affects bug 717103, but they're sort of orthogonal.  We need this support after bug 717103 lands, and the implementation of bug 717103 needs to be aware of this.

This is a generally-useful feature, but will only be used in b2g/gonk initially.

Approximately, we need to unmount the SD card and let the USB block driver take it over.  I'm not 100% sure how to do all the dance needed on the device side, but Andreas should be able to help more.  This code exists in android so that can be used as a reference.

Comment 1

[Andreas Gal :gal]

Lets do the following:

1. Make a C++ file in dom/system/gonk that talks to vold using the I/O thread interfaces. Look at the headphones bug to see how to do that.

2. Create a separate file in the same dom/system/gonk directory that is a controller for all this stuff. It will listen for uevents and settings changes and will then poke the other piece to do the mounting/unmounting.

Makes sense?

Comment 2

[Dave Hylands [:dhylands]]

Yep - Adding a reference to bug 736939 (the headphone bug). 

The android command line utility vdc, found in the android sources system/vold/vdc has all of the stuff needed to talk to vold.

Comment 3

[Dave Hylands [:dhylands]]

Alright - I was able to get detection of mozsettings-changed working, but it turns out that there is no settings service, so it isn't possible right now to query the current value of the setting, which makes this approach unworkable right now.

The next logical choice is to do add a "SendCommandToVolD" API which takes a string command and gets back a multi-line string as a response. Since the VolD commands can take quite a few seconds to complete, this would need to be a DOMRequest. The underlying command would do the appropriate socket stuff since the socket support doesn't exist yet for JS.

Comment 4

[Gregor Wagner [:gwagner]]

(In reply to Dave Hylands [:dhylands] from comment #3)
> Alright - I was able to get detection of mozsettings-changed working, but it
> turns out that there is no settings service, so it isn't possible right now
> to query the current value of the setting, which makes this approach
> unworkable right now.

You can get the new value without accessing the database. So in the mozsettings-change event you can ask for the new value.

Comment 5

[Dave Hylands [:dhylands]]

Right, but there isn't anyway to get the value between the time the phone boots up and the user goes into the settings app and changes the setting.

Comment 6

[Dave Hylands [:dhylands]]

OK - I'm going to continue coding/testing using the mozsettings-changed as my trigger. Once the settings API as service is available, then I can have the C++ side query the initial value. I'll add bug 743336 as a blocker.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Yes, assume a reasonable default value (automount == true) and leave a FIXME for the service.

Comment 8

[Dave Hylands [:dhylands]]

Attachment: bug-737153 Add the VolumeManager

The VolumeManager is a front-end for android's vold. The VolumeManager has a collection of Volumes and allows commands (like mount, share) to be performed on those Volumes.

The VolumeManager will also detect if vold goes away and reconnect when it comes back.

Comment 9

[Dave Hylands [:dhylands]]

Attachment: bug-737153 - Add AutoMounter

The AutoMounter detects when all of the conditions are right (stuff is enabled, usb cable is plugged in) to allow the sdcard volume to be mounted on the host computer.
When the conditions are no longer right, then the AutoMounter will unshare the volume and remount it so that b2g can access the volume.

Currently, the AutoMounter assumes that mEnabled is true. This will eventually be tied into a setting.

Comment 10

[Dave Hylands [:dhylands]]

Attachment: bug-737153 Hook up AutoMounter and VolumeManager

Add in calls which start/shutdown the VolumeManager and AutoMounter.

Also fix a compile error in UeventPoller when compiled in debug mode

Comment 11

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 617104 [details] [diff] [review]
bug-737153 Add the VolumeManager

Review of attachment 617104 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, first off, organization: The iothread stuff should be over in the ipc/ directory, since it's talking to an outside process and that's where we expect code that does that to live. Either as ipc/volumemanager or ipc/gonk/volumemanager, since it won't even feasibly work for any other platform.

Also: How much of this really ever need to be public? Obviously Init/Shutdown need exposure, but after that? It might be worth /just/ exposing those functions to the public (i.e. make a header and add EXPORT_ blocks to them for your Makefile.in), then let everything else exist out of the public eye. With as class heavy as this is, it might be nice to break into multiple header/compilation unit pairs and just not EXPORT_ the headers, or you could push everything to compilation unit level that's just going to be utility for the system anyways.

I guess part of this relies on knowing what portions are going to be exposed in XPCOM? But I think that's a better question to ask after a first cleanup pass.

::: dom/system/gonk/VolumeManager.cpp
@@ +87,5 @@
> +    }
> +}
> +#endif
> +
> +static VolumeManager *sVolumeManager = NULL;

Bare pointers are sadness inducing. Make this a RefPtr.

@@ +127,5 @@
> +
> +class VolumeListCommand : public VolumeCommand
> +{
> +public:
> +  VolumeListCommand(VolumeManager *volMgr)

Is there going to be another VolumeManager that's not sVolumeManager? If not, why worry about passing this around? VolumeListCommands can't happen outside this file anyways. Same goes for following classes.

@@ +316,5 @@
> +  return vol;
> +}
> +
> +VolumeManager *
> +VolumeManager::GetInstance()

NOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOO NONONONONONONONONOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOO

NOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOO

NOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOOO

Also: No.

Bare pointer exposure to a static compilation unit scoped instance? Did you mean to do that? I mean, I don't see anywhere this function is actually called, yet, but apparently I have reviews after this. :)

@@ +474,5 @@
> +        nsDependentCString  responseLine(endPtr, &mRcvBuf[mRcvBytes] - endPtr);
> +
> +        DBG("Rcvd: %d '%s'", responseCode, responseLine.Data());
> +
> +        if (responseCode >= 600) {

Same response code limit idea as in the header

::: dom/system/gonk/VolumeManager.h
@@ +16,5 @@
> +
> +//??? This is probably wrong. ResponseCode.h comes from glue/gonk/system/vold/ and
> +//??? the ../.. portion is relative to glue/gonk/system/core/include (which seems
> +//??? to be added as part of TOOLCHAIN_DIRS in the B2G/Makefile.
> +#include <../../vold/ResponseCode.h>

Your comment is the correct idea. We should add the include directory to configure.in (around line 333 currently, where the gonk specific -I flags are), so you don't have to do the weird relative navagation.

@@ +20,5 @@
> +#include <../../vold/ResponseCode.h>
> +
> +namespace mozilla {
> +namespace dom {
> +namespace gonk {

If these move over to IPC, remember to change the namespaces.

@@ +33,5 @@
> +/***************************************************************************
> +*
> +*   Volume class
> +*
> +***************************************************************************/

Shouldn't really need the big comments if we break this out into different files.

@@ +39,5 @@
> +class Volume;               // Forward declarations
> +class VolumeCommand;
> +class VolumeActionCommand;
> +class VolumeListCommand;
> +class VolumeManager;

You forward declare this like 4 times later in the file. Guess that'll make the breaking files up easier. :)

@@ +45,5 @@
> +typedef RefPtr<Volume> VolumePtr;
> +typedef std::vector<VolumePtr> VolumeArray;
> +
> +typedef RefPtr<VolumeCommand>         VolumeCommandPtr;
> +typedef std::queue<VolumeCommandPtr>  VolumeCommandQueue;

The above utility types don't seem to get used outside their respective classes, why not scope them into the classes?

@@ +50,5 @@
> +
> +class Volume : public RefCounted<Volume>
> +{
> +public:
> +

Since RefCounted or the container may deal with the lifetime, does this need a virtual destructor? Not actually sure, myself. You do at least do this in later refcounted classes.

@@ +84,5 @@
> +      case STATE_SHAREDMNT:   return "Shared-Mounted";
> +    }
> +    return "???";
> +  }
> +  const char *StateStr()  { return StateStr(mState); }

If you're going to const correct everything else, shouldn't this also expose as const, since it's calling a const function with a member?

@@ +99,5 @@
> +  class Callback
> +  {
> +  public:
> +    Callback() {}
> +    virtual ~Callback() {}

Callback destructor shouldn't need to be virtual?

@@ +166,5 @@
> +    // vold uses it to determine the end of the command.
> +    mCmd.Append('\0');
> +  }
> +
> +  bool        Done() const                    { return (mResponseCode >= 200) && (mResponseCode < 600); }

The 200/600 magic numbers are used a few places, turning them into static consts would be helpful.

@@ +207,5 @@
> +
> +class VolumeManager : public MessageLoopForIO::Watcher
> +{
> +  friend class VolumeListCommand;
> +

A lot of the observer calls in here should probably be defined in a compilation unit file, since they're gonna inline automatically up here. I'm not sure how heavyweight observer calls are.

@@ +281,5 @@
> +  ScopedClose         mSocket;
> +  VolumeArray         mVolumeArray;
> +  VolumeCommandQueue  mCommands;
> +  bool                mCommandPending;
> +  char                mRcvBuf[1024];

Constantize this size instead of magic numbering it.

Comment 12

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 617119 [details] [diff] [review]
bug-737153 Hook up AutoMounter and VolumeManager

Review of attachment 617119 [details] [diff] [review]:
-----------------------------------------------------------------

Cancelling review here because this is gonna change quite a bit in the light of the last two reviews. But also check comments. :)

::: dom/system/gonk/Makefile.in
@@ +65,5 @@
>  XPCSHELL_TESTS = tests
>  endif
>  
>  include $(topsrcdir)/config/rules.mk
> +include $(topsrcdir)/ipc/chromium/chromium-config.mk

Yeah, this should go in ipc instead. We don't want to vary where we're playing with libevent stuff if we don't have to.

::: hal/gonk/UeventPoller.cpp
@@ +139,5 @@
>  
>  void
>  NetlinkPoller::OnFileCanReadWithoutBlocking(int fd)
>  {
> +  MOZ_ASSERT(fd == mSocket.get());

We're taking care of this in bug 747454, so you can remove it from here. Not really in the context of this patch anyways.

Comment 13

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 617115 [details] [diff] [review]
bug-737153 - Add AutoMounter

Review of attachment 617115 [details] [diff] [review]:
-----------------------------------------------------------------

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

Comment 14

[Dave Hylands [:dhylands]]

Kyle said:

> @@ +50,5 @@
> > +
> > +class Volume : public RefCounted<Volume>
> > +{
> > +public:
> > +
> 
> Since RefCounted or the container may deal with the lifetime, does this need a virtual destructor? Not actually sure, myself. You do at least do this in later refcounted classes.

Fundamentally, you only need a virtual destructor if you have virtual methods. Neither the Volume class nor the RefCounted base class has any virtual methods, so a virtual destructor is unnecessary.

If the RefCounted base class had a virtual destructor, then the virtalness of the destructor is inherited regardless of whether it's declared as virtual in the derived class or not.

Comment 15

[Dave Hylands [:dhylands]]

Kyle said:


> @@ +99,5 @@
> > +  class Callback
> > +  {
> > +  public:
> +    Callback() {}
> +    virtual ~Callback() {}

Callback destructor shouldn't need to be virtual?
(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #11)
> Comment on attachment 617104 [details] [diff] [review]
> bug-737153 Add the VolumeManager
...snip...
> @@ +99,5 @@
> > +  class Callback
> > +  {
> > +  public:
> > +    Callback() {}
> > +    virtual ~Callback() {}
> 
> Callback destructor shouldn't need to be virtual?

Since there is a virtual method in the class, it's almost certainly a requirement that the destructor be virtual. Otherwise if there is a derived class, the derived classes destructor won't get called if you destruct the object using a more derived pointer.

So for example:

class Base
{
public:
    ~Base();
    virtual SomeMethod() = 0;
}

class Derived : public Base
{
public:
    ~Derived();
    virtual SomeMethod();
}

If you delete via a Base *, then Derived's destructor won't get called unless Base declared the destructor as virtual.

Comment 16

[Dave Hylands [:dhylands]]

Attachment: bug-737153 Add the VolumeManager

The VolumeManager is a front-end for android's vold. The VolumeManager has a collection of Volumes and allows commands (like mount, share) to be performed on those Volumes.

The VolumeManager will also detect if vold goes away and reconnect when it comes back.

Comment 17

[Dave Hylands [:dhylands]]

Attachment: bug-737153 - Add AutoMounter

The AutoMounter detects when all of the conditions are right (stuff is enabled, usb cable is plugged in) to allow the sdcard volume to be mounted on the host computer.
When the conditions are no longer right, then the AutoMounter will unshare the volume and remount it so that b2g can access the volume.

Currently, the AutoMounter assumes that mEnabled is true. This will eventually be tied into a setting.

Comment 18

[Dave Hylands [:dhylands]]

Attachment: bug-737153 Hook up AutoMounter and VolumeManager

Add in calls which start/shutdown the VolumeManager and AutoMounter.

Also fix a compile error in UeventPoller when compiled in debug mode

Comment 19

[Dave Hylands [:dhylands]]

Comment on attachment 617375 [details] [diff] [review]
bug-737153 Add the VolumeManager

v2

Comment 20

[Dave Hylands [:dhylands]]

Comment on attachment 617377 [details] [diff] [review]
bug-737153 - Add AutoMounter

v2

Comment 21

[Dave Hylands [:dhylands]]

Comment on attachment 617378 [details] [diff] [review]
bug-737153 Hook up AutoMounter and VolumeManager

v2

Comment 22

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

When you upload new patches that're refinements, you'll want to mark that they obsolete old patches. I went ahead and remarked the old ones.

Comment 23

[Dave Hylands [:dhylands]]

(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #22)
> When you upload new patches that're refinements, you'll want to mark that
> they obsolete old patches. I went ahead and remarked the old ones.

Thanks - How exactly do you do that?

I tried to use git bz attach -e HEAD, but it didn't find the header. So I redid git bz attach 737153 again. I noticed that it didn't automatically mark the old bugs as obsolte, but I also couldn't see exactly where to do that manually.

Comment 24

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Well huh. I was not aware of git bz. That's neat. :D

Looks like git bz does that for you in some cases at least, but to do it manually, it's kinda hidden:

- Go to "Details" on the patch you want to obsolete
- Go to "Edit Details" next to the patch title in the patch detail screen
- There's a "obsolete" radio box there.

There's also the ability to obsolete when adding patches via bugzilla itself, which is the only way I've done it so far.

Comment 25

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Did a really quick cursory glance at the new patches, you're probably ok to bring back cjones as a superreviewer on these now. I'll try to get to my actual reviews tomorrow.

Comment 26

[Dave Hylands [:dhylands]]

Now that I've upgraded my phone to ICS and I started to look through the ICS source code I see that UMS support has changed a bunch.

Do we want to make sure this code works both under GB and ICS?

It also doesn't make sense for this code to run on desktop B2G, so I need to check that Desktop B2G builds with these patches applied. Currently it uses MOZ_B2G_RIL to enable it. What should it use instead?

Comment 27

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

We're starting to review other things (NFC) that only work on ICS, so there's a good question about what exactly we need to support in the future. From what I've been told (in relation to my dbus work for bluetooth), we just need to be able to keep support for building gingerbread (doesn't even have to work. >.> ), without detection for whether or not we're running on ics or gb.

That said, since this gb code is already in reviews, might be good to let the reviews finish (I'm waiting on cjones to srev this right now before I restart my reviews), land this, and work on the ICS stuff in another bug that builds on top of this? That way you can start on ICS now while building on what's learned from the reviews here.

Comment 28

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Also, since this will never build on desktop, we should surround all the calls to this stuff with

#ifdef MOZ_WIDGET_GONK

which means "build whenever we're on phone/simulator". We have MOZ_B2G_RIL/BT because both of those are actually testable on desktop and we turn them on via mozconfig changes to otherwise normal firefox builds.

Comment 29

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

For now, as long as the code builds on ICS, it's OK, doesn't need to work there.  We can figure out a strategy for ICS/GB support in a followup.

Comment 30

[Michael Wu [:mwu]]

Comment on attachment 617375 [details] [diff] [review]
bug-737153 Add the VolumeManager

Review of attachment 617375 [details] [diff] [review]:
-----------------------------------------------------------------

Some general comments -

1. Early returns should be used where possible. Seeing a staircase of right curly braces is often not a good sign IMHO.
2. Too much whitespace. Specifically, too many blank lines. This is making the code hard to read. The blank lines after control statements are especially annoying.

This code reminds me of vendor driver code to some degree.

::: ipc/volumemanager/VolumeManager.cpp
@@ +29,5 @@
> +
> +#if USE_DEBUG
> +//??? Seems like this could be a generic routine, perhaps by passing in an output function
> +static void
> +DumpMem(const char *prefix, unsigned address, const void *inData, unsigned numBytes)

convert this function to two space indent

@@ +125,5 @@
> +//static
> +void
> +VolumeManager::SetState(STATE newState)
> +{
> +  if (sVolumeManager != NULL) {

if (!sVolumeManager)
  return;

here and elsewhere.

@@ +175,5 @@
> +
> +  MOZ_ASSERT(sVolumeManager != NULL);
> +
> +  if (vol == NULL) {
> +    nsCString name2(name);

name2 unused

@@ +264,5 @@
> +
> +void
> +VolumeManager::WriteCommandData()
> +{
> +  if (mCommands.size() > 0) {

invert logic, return early.

@@ +268,5 @@
> +  if (mCommands.size() > 0) {
> +
> +    RefPtr<VolumeCommand>  cmd = mCommands.front();
> +
> +    if (cmd->BytesRemaining() > 0) {

ditto

@@ +419,5 @@
> +
> +void
> +VolumeManager::HandleBroadcast(const VolumeResponse &response)
> +{
> +  if (response.ResponseCode() == ResponseCode::VolumeStateChange) {

return early

@@ +433,5 @@
> +    tokenizer.nextToken();  // The word "Volume"
> +    nsDependentCSubstring volName(tokenizer.nextToken());
> +
> +    RefPtr<Volume> vol = FindVolumeByName(volName);
> +    if (vol != NULL) {

return early

@@ +438,5 @@
> +
> +      const nsCString &responseStr = response.ResponseStr();
> +
> +      PRInt32 toIdx = responseStr.Find(NS_LITERAL_CSTRING(" to "));
> +      if (toIdx > 0) {

return early

@@ +479,5 @@
> +{
> +  MOZ_ASSERT(MessageLoop::current() == XRE_GetIOMessageLoop());
> +
> +  if (sVolumeManager != NULL) {
> +    if (!sVolumeManager->OpenSocket()) {

combine checks

Comment 31

[Dave Hylands [:dhylands]]

Attachment: Bug 737153 - Add the VolumeManager - v3

v3 - After mwu's comments

The VolumeManager is a front-end for android's vold. The VolumeManager has a collection of Volumes and allows commands (like mount, share) to be performed on those Volumes.

The VolumeManager will also detect if vold goes away and reconnect when it comes back.

Comment 32

[Dave Hylands [:dhylands]]

Attachment: Bug 737153 - Add the AutoMounter - v3

v3 - After mwu's comments

The AutoMounter detects when all of the conditions are right (stuff is enabled, usb cable is plugged in) to allow the sdcard volume to be mounted on the host computer.
When the conditions are no longer right, then the AutoMounter will unshare the volume and remount it so that b2g can access the volume.

Currently, the AutoMounter assumes that mEnabled is true. This will eventually be tied into a setting.

Comment 33

[Dave Hylands [:dhylands]]

Attachment: Bug 737153 - Hook up the VolumeManager and AutoMounter to the build - v3

Add in calls which start/shutdown the VolumeManager and AutoMounter.

Now uses ifeq ($(MOZ_WIDGET_TOOLKIT),gonk) to include into the build.

Comment 34

[Dave Hylands [:dhylands]]

Comment on attachment 619999 [details] [diff] [review]
Bug 737153 - Add the VolumeManager - v3

v3 - After mwu's comments

Comment 35

[Dave Hylands [:dhylands]]

Comment on attachment 620000 [details] [diff] [review]
Bug 737153 - Add the AutoMounter - v3

v3 - After mwu's comments

Comment 36

[Dave Hylands [:dhylands]]

Comment on attachment 620001 [details] [diff] [review]
Bug 737153 - Hook up the VolumeManager and AutoMounter to the build - v3

v3 - Now uses ifeq ($(MOZ_WIDGET_TOOLKIT),gonk)

Comment 37

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 619999 [details] [diff] [review]
Bug 737153 - Add the VolumeManager - v3

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

Comment 38

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 620000 [details] [diff] [review]
Bug 737153 - Add the AutoMounter - v3

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

Comment 39

[Michael Wu [:mwu]]

Comment on attachment 619999 [details] [diff] [review]
Bug 737153 - Add the VolumeManager - v3

Review of attachment 619999 [details] [diff] [review]:
-----------------------------------------------------------------

Nice. A lot more readable for me.

::: ipc/volumemanager/Volume.cpp
@@ +40,5 @@
> +
> +void
> +Volume::StartMount(RefPtr<Volume::Callback> cb)
> +{
> +  StartCommand( new VolumeActionCommand(this, "mount", ""), cb);

The space before the new looks weird.

@@ +62,5 @@
> +  StartCommand( new VolumeActionCommand(this, "unshare", "ums"), cb);
> +}
> +
> +void
> +Volume::StartCommand(RefPtr<VolumeCommand> cmd, RefPtr<Volume::Callback> cb)

This should be able to take a plain pointer to VolumeCommand. Same for Volume::Callback. In general, seeing RefPtr in parameter lists is weird. Try to get rid of them elsewhere too.

@@ +77,5 @@
> +  // Set mCallback to NULL before calling CommandDone, so that the
> +  // callback can post further commands
> +
> +  Volume::Callback *cb = mCallback;
> +  mCallback = NULL;

So, this is actually safe because the caller holds on to the callback, but I don't think it should assume that. |RefPtr<Volume::Callback> cb| would be more correct.

And after writing this, I noticed cjones pointed out the same thing. Yay.

::: ipc/volumemanager/Volume.h
@@ +93,5 @@
> +
> +  STATE             mState;
> +  const nsCString   mName;
> +  nsCString         mMountPoint;
> +  RefPtr<Callback>  mCallback;

I noticed that you tend to put two spaces after the type when aligning names. Usually it's just one space unless there's a * or &.

::: ipc/volumemanager/VolumeCommand.cpp
@@ +41,5 @@
> +VolumeActionCommand::VolumeActionCommand(RefPtr<Volume> volume, const char *action, const char *extraArgs)
> +  : VolumeCommand(),
> +    mVolume(volume)
> +{
> +  nsCString cmd;

FWIW, there is a nsAutoCString which can store strings up to 64bytes long on the stack. It's useful when there's you're dealing with a short local string. Don't know if this string qualifies, but I thought I'd mention it since it looks like it might.

::: ipc/volumemanager/VolumeManager.h
@@ +121,5 @@
> +
> +  static RefPtr<Volume> FindVolumeByName(const nsCSubstring &name);
> +  static RefPtr<Volume> FindAddVolumeByName(const nsCSubstring &name);
> +
> +  static void       PostCommand(RefPtr<VolumeCommand> cmd);

Hmm I think you can just take a VolumeCommand pointer here.

Comment 40

[Michael Wu [:mwu]]

(In reply to Michael Wu [:mwu] from comment #39)
> FWIW, there is a nsAutoCString which can store strings up to 64bytes long on
> the stack. It's useful when there's you're dealing with a short local
> string. Don't know if this string qualifies, but I thought I'd mention it
> since it looks like it might.
> 

And by nsAutoCString I meant nsCAutoString.

Comment 41

[Dave Hylands [:dhylands]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #37)
> Comment on attachment 619999 [details] [diff] [review]
> Bug 737153 - Add the VolumeManager - v3
> 
> >diff --git a/ipc/volumemanager/Volume.cpp b/ipc/volumemanager/Volume.cpp

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

Is that just for objects? What about a parameter like "size_t numBytes"

> >diff --git a/ipc/volumemanager/Volume.cpp b/ipc/volumemanager/Volume.cpp
> >+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.

I made SetMountPoint (and SetState) private (and added friend declarations for the couple places that need to use them). It's basically just recording the mount point received from Vold.
The mount point currently isn't used anywhere, but presumably some type of Storage API would need to know this in order to figure out how to access the files.
It isn't clear to me exactly what you're asking me to document.
Do you want me to document what a mount point is?
Or do you just want me to say that it comes from /etc/vold.fstab?
Or just to say that this is the path to access files stored on the volume?
Or something else?

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

You can actually have as many operations pending as you'd like. The callback passed in will be called as each one completes. There is no state machine (at least not as far as the VolumeManager is concerned). The AutoMounter has a state machine, maybe that's what you're referring to?

I added the following comment above the StartXxx routines:
  // The StartXxx functions will queue up a command to the VolumeManager. You can queue up
  // as many commands as you like, and aCallback will be called as each one completes.

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

I'll change it. It does, in fact, compile (I understand the issue though).

Comment 42

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Dave Hylands [:dhylands] from comment #41)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #37)
> > Comment on attachment 619999 [details] [diff] [review]
> > Bug 737153 - Add the VolumeManager - v3
> > 
> > >diff --git a/ipc/volumemanager/Volume.cpp b/ipc/volumemanager/Volume.cpp
> 
> > 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.
> 
> Is that just for objects? What about a parameter like "size_t numBytes"
> 

All function/method parameters.

> > >diff --git a/ipc/volumemanager/Volume.cpp b/ipc/volumemanager/Volume.cpp
> > >+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.
> 
> I made SetMountPoint (and SetState) private (and added friend declarations
> for the couple places that need to use them). It's basically just recording
> the mount point received from Vold.
> The mount point currently isn't used anywhere, but presumably some type of
> Storage API would need to know this in order to figure out how to access the
> files.
> It isn't clear to me exactly what you're asking me to document.
> Do you want me to document what a mount point is?
> Or do you just want me to say that it comes from /etc/vold.fstab?
> Or just to say that this is the path to access files stored on the volume?
> Or something else?
> 

A brief combination of (1) and (3).

> > >+  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.
> 
> You can actually have as many operations pending as you'd like. The callback
> passed in will be called as each one completes. There is no state machine
> (at least not as far as the VolumeManager is concerned). The AutoMounter has
> a state machine, maybe that's what you're referring to?
> 

void
Volume::StartCommand(RefPtr<VolumeCommand> cmd, RefPtr<Volume::Callback> cb)
{
  MOZ_ASSERT(mCallback == NULL);


> I added the following comment above the StartXxx routines:
>   // The StartXxx functions will queue up a command to the VolumeManager.
> You can queue up
>   // as many commands as you like, and aCallback will be called as each one
> completes.
> 

We have a disconnect 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).
> 
> I'll change it. It does, in fact, compile (I understand the issue though).

Yeah, gcc is probably being permissive.  It's also possible the C++11 spec resolved the ambiguity.

Comment 43

[Dave Hylands [:dhylands]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #37)
> Comment on attachment 619999 [details] [diff] [review]
> Bug 737153 - Add the VolumeManager - v3
> 
> >diff --git a/ipc/volumemanager/VolumeManager.cpp b/ipc/volumemanager/VolumeManager.cpp
> 
> >+//static
> >+RefPtr<Volume>
> >+VolumeManager::FindVolumeByName(const nsCSubstring &name)
> 
> nsACString&

In this case, I'm using the nsCWhitespaceTokenizer which returns a nsCSubstring, so I would have to construct a new nsACString rather than using the existing substring.

> >+{
> >+  if (!sVolumeManager) {
> >+    return NULL;
> >+  }
> >+  for (VolumeArray::iterator volIter = sVolumeManager->mVolumeArray.begin();
> 
> CString has operator== so should be able to use std::find() here.

I don't see how this helps. The array is a vector of Volume RefPtrs. I need to call Name() to get the name of the volume. std::find would only work if it was a vector of strings.

Comment 44

[Dave Hylands [:dhylands]]

Attachment: v4 - Add VolumeManager

Comment 45

[Dave Hylands [:dhylands]]

Attachment: v4 - Add AutoMounter

Comment 46

[Dave Hylands [:dhylands]]

Attachment: v4 - Relocated back to dom/system/gonk

Comment 47

[Dave Hylands [:dhylands]]

Attachment: Hookup VolumeManager and AutoMounter into the build

Fixed to only build when targeting the phone

Comment 48

[Dave Hylands [:dhylands]]

Attachment: v5 - Add the VolumeManager

Comment 49

[Dave Hylands [:dhylands]]

Comment on attachment 626242 [details] [diff] [review]
v5 - Add the VolumeManager

Hooks in the AutoMounterSettings

Comment 50

[Dave Hylands [:dhylands]]

Attachment: v5 - Add AutoMounter

Adds the AutoMounterSettings.

Comment 51

[Dave Hylands [:dhylands]]

Attachment: v6 - Hook up VolumeManager and AutoMounter

Adds AutoMounterSettings.

Comment 52

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 626242 [details] [diff] [review]
v5 - Add the VolumeManager

Review of attachment 626242 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/system/gonk/VolumeCommand.h
@@ +40,5 @@
> +  virtual ~VolumeResponseCallback() {}
> +
> +  bool Done() const
> +  {
> +      // Response codes from the 200, 400, and 500 series all indicated that

nit: Indentation differs.

::: dom/system/gonk/VolumeManager.cpp
@@ +205,5 @@
> +  if (!sVolumeManager->mCommandPending) {
> +    // There aren't any commands currently being processed, so go
> +    // ahead and kick this one off.
> +    sVolumeManager->mCommandPending = true;
> +    sVolumeManager->WriteCommandData();

Assuming I'm following this correctly, PostCommand looks to be called from the main thread, but it seems like we want to keep all of our writing out on the IOThread? Shouldn't this just queue a task?

@@ +209,5 @@
> +    sVolumeManager->WriteCommandData();
> +  }
> +}
> +
> +//--------------------------------------------------------------------------

nit: Might just make these comment blocks the same format as the others. Just makes visual scanning easier.

Comment 53

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 626244 [details] [diff] [review]
v5 - Add AutoMounter

Review of attachment 626244 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/system/gonk/AutoMounter.cpp
@@ +382,5 @@
> +static void
> +ShutdownAutoMounterIOThread()
> +{
> +  MOZ_ASSERT(MessageLoop::current() == XRE_GetIOMessageLoop());
> +  MOZ_ASSERT(sAutoMounter);

nit: This is going to assert out sometimes when you don't mean it to, because if something fails in front of it in the SystemWorkerManager initialization function, SystemWorkerManager dumps into its deinit function and you'll shutdown before you've started it up (I just ran into this with bug 756389).

Comment 54

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 626245 [details] [diff] [review]
v6 - Hook up VolumeManager and AutoMounter

Review of attachment 626245 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/system/gonk/SystemWorkerManager.cpp
@@ +224,5 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>  #endif
>  
> +#ifdef MOZ_WIDGET_GONK
> +  mozilla::system::InitAutoMounter();

No success check for automounter initialization?

@@ +248,5 @@
>  
>    mShutdown = true;
>  
> +#ifdef MOZ_WIDGET_GONK
> +  mozilla::system::ShutdownAutoMounter();

Fixing the nit in the prior commit should make this ok.

Comment 55

[Dave Hylands [:dhylands]]

(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #52)
> Comment on attachment 626242 [details] [diff] [review]
> ::: dom/system/gonk/VolumeManager.cpp
> @@ +205,5 @@
> > +  if (!sVolumeManager->mCommandPending) {
> > +    // There aren't any commands currently being processed, so go
> > +    // ahead and kick this one off.
> > +    sVolumeManager->mCommandPending = true;
> > +    sVolumeManager->WriteCommandData();
> 
> Assuming I'm following this correctly, PostCommand looks to be called from
> the main thread, but it seems like we want to keep all of our writing out on
> the IOThread? Shouldn't this just queue a task?

As far as I'm aware, all paths into the instances of the VolumeManager, Volume, and VolumeCommand are on IOThread. Currently, the only client of the VolumeManager is the AutoMounter, and it also runs completely in the IOThread.

The UsbCableObserver in the AutoMounter runs in the main thread, but it does a PostTask to the IOThread.

Comment 56

[Dave Hylands [:dhylands]]

(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #54)
> Comment on attachment 626245 [details] [diff] [review]
> v6 - Hook up VolumeManager and AutoMounter
> 
> Review of attachment 626245 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/SystemWorkerManager.cpp
> @@ +224,5 @@
> >    NS_ENSURE_SUCCESS(rv, rv);
> >  #endif
> >  
> > +#ifdef MOZ_WIDGET_GONK
> > +  mozilla::system::InitAutoMounter();
> 
> No success check for automounter initialization?

No. The InitAutoMounter function returns void since there isn't anything that can fail. InitAutoMounter needs to do a Post so that the real Init takes place on the IOThread, and even then the only initialization done is a new.

I was told that new can never fail.

Comment 57

[Dave Hylands [:dhylands]]

Attachment: v6 - Add the VolumeManager

Comment 58

[Dave Hylands [:dhylands]]

Attachment: v6 - Add AutoMounter

Address Kyle's nits.

Comment 59

[Dave Hylands [:dhylands]]

Comment on attachment 626507 [details] [diff] [review]
v6 - Add AutoMounter

Added AutoMounterSettings (new addition from last review by cjones)

Comment 60

[Dave Hylands [:dhylands]]

From the GUI perspective, I've also created https://github.com/andreasgal/gaia/pull/1467 which adds the UI side of things.

ums.enabled is the user preference.
ums.mode is what actually controlls the AutoMounter, and can have 3 values:
0=disabled, 1=enabled, 2=disable when unplugged.

Comment 61

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 626245 [details] [diff] [review]
v6 - Hook up VolumeManager and AutoMounter

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

Comment 62

[Dave Hylands [:dhylands]]

Attachment: Hook up VolumeManager and AutoMounter

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

Added final changes suggested by cjones

Comment 63

[Dave Hylands [:dhylands]]

Attachment: v7 - Add AutoMounter

Fixed a bug in AutoMounter::SetMode

Comment 64

[Dave Hylands [:dhylands]]

Attachment: v8 - Add AutoMounter

Address Kyle's nits.
Fixed a bug in AutoMounter::SetMode
Fixed a compile problem on the try server

Comment 65

[Dave Hylands [:dhylands]]

Attachment: Bug 737153 - Pull in the toolchain with the system/vold headers

Comment 66

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/d2704688b61b
http://hg.mozilla.org/integration/mozilla-inbound/rev/0773c8788f78
http://hg.mozilla.org/integration/mozilla-inbound/rev/815d53fb27e0
http://hg.mozilla.org/integration/mozilla-inbound/rev/cff8079f2325

Comment 67

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d2704688b61b
https://hg.mozilla.org/mozilla-central/rev/0773c8788f78
https://hg.mozilla.org/mozilla-central/rev/815d53fb27e0
https://hg.mozilla.org/mozilla-central/rev/cff8079f2325