890144 - [Gonk-JB] Nexus 4: Find a way to enable DeviceStorage because there is not internal/external storage for AutoMounter.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Marco Chen [:mchen]]

Currently we do the gonk-jb porting on Nexus 4.
According to that Nexus 4 doesn't have external SD Card interface and internal partition for vold, deviceStorage will always be unavailable. This will effect the testing or development on media or camera stuffs.

Comment 1

[Dave Hylands [:dhylands]]

Once bug 865347 lands, we'll have the ability to add arbitrary volumes using whatever directories we'd like.

I think we should create a configuration file which is read on startup and causes the mount points/volume names listed to be added.

This configuration file should be device specific.

Comment 2

[viral [:viralwang]]

(In reply to Dave Hylands [:dhylands] from comment #1)
> Once bug 865347 lands, we'll have the ability to add arbitrary volumes using
> whatever directories we'd like.
> 
> I think we should create a configuration file which is read on startup and
> causes the mount points/volume names listed to be added.
> 
> This configuration file should be device specific.

We can used the patch in bug 865347 to add volumes.
In Nexus 4 case, there's a path "/storage/emulated/legacy" set by fuse as sdcard and not using vold anymore.
I would like to raise one solution to have two different ways for necessary device storage usage:
1. vold: control by vold.fstab, we will used the config to mount internal patition and external SD card.
2. fuse: we will create a device specfic configuration file included mountpoint/fsname
and see if we need to create extra volumes by CreateFakeVolume.
So far I would like to check the configuration file in InitAutoMounter() but not sure it's the best location for device storage architecture.

Comment 3

[Dave Hylands [:dhylands]]

(In reply to vwang from comment #2)
> 2. fuse: we will create a device specfic configuration file included
> mountpoint/fsname
> and see if we need to create extra volumes by CreateFakeVolume.
> So far I would like to check the configuration file in InitAutoMounter() but
> not sure it's the best location for device storage architecture.

Reading the configuration file in InitAutoMounter feels like a reasonable place to put it.

At least that puts all of the volumes into one place (they all come into being through the AutoMounter, either from vold, or from a configuration file).

I also like this because it gives us the opportunity to have the configuration file also affect the vold volumes.

For example, we could add an attribute to a vold volume to indicate if its user-removable or not (some SD cards require the battery to be removed in order to insert/remove an sd card).

Comment 4

[viral [:viralwang]]

Attachment: Support fuse volume by configuration file in AutoMounter

we will take /etc/fuse.xml as configuration file to see if we need additional volume

format in fuse.xml will like this:
sdcard /storage/emulated/legacy

it means sdcard will be fsname and /storage/emulated/legacy is the mountpoint.
For devices like Nexus 4 without internal partition and external sdcard, we only need to add fuse.xml for device storage usage.

Comment 5

[Dave Hylands [:dhylands]]

Comment on attachment 773863 [details] [diff] [review]
Support fuse volume by configuration file in AutoMounter

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

I think that this is the right idea.

Since I made so many comments, I'm going to r- this for now. I want to see the next version.

::: dom/system/gonk/AutoMounter.cpp
@@ +535,5 @@
>  static StaticRefPtr<UsbCableObserver> sUsbCableObserver;
>  static StaticRefPtr<AutoMounterSetting> sAutoMounterSetting;
>  
>  void
> +InitFuseVolume()

The fact that the OS happens to use Fuse isn't really relevant.

Lets call this InitVolumeConfig().

This should also be a static function.

I would also like to see a brief comment that explains what the contents of volume.cfg is supposed to be (so that I don't have to actually read the code to figure it out).

@@ +541,5 @@
> +    FILE *fp;
> +    int n = 0;
> +    char line[255];
> +    char *vol_label, *mount_point, *save_ptr;
> +    if (!(fp = fopen("/etc/fuse.xml", "r"))) {

Why does this file have a .xml extension if it isn't an xml file?

I also don't like the name fuse. I'd rather call this something like volume.cfg.

I also think that it should be from /system/etc rather than /etc (/system/etc will always exist on an android device, /etc may not).

Since we may want to add additional things to this configuration file in the future, lets put a command at the beginning of the line. Let's make the current command be create, so that the line looks like:

create sdcard /path/to/sdcard/mount/point

@@ +542,5 @@
> +    int n = 0;
> +    char line[255];
> +    char *vol_label, *mount_point, *save_ptr;
> +    if (!(fp = fopen("/etc/fuse.xml", "r"))) {
> +        ERR("failed to open");

Failed to open what? You should at least give the name of the file that failed to open. You should also report errno and probably strerror(errno)

@@ +550,5 @@
> +        n++;
> +        line[strlen(line)-1] = '\0';
> +        if (line[0] == '#' || line[0] == '\0')
> +            continue;
> +        if (!(vol_label = strtok_r(line, " ", &save_ptr))) {

I'm glad to see you using strtok_r (rather than strtok).

Create a variable for the delimiters. It should allow spaces, tabs, and newlines.

char *delim = " \t\n";

Then you can remove the line "line[strlen(line)-1] = '\0';"

@@ +551,5 @@
> +        line[strlen(line)-1] = '\0';
> +        if (line[0] == '#' || line[0] == '\0')
> +            continue;
> +        if (!(vol_label = strtok_r(line, " ", &save_ptr))) {
> +            ERR("Error parsing vol_label");

Rather than considering this to be an error, you can remove || line[0] == '\0' from earlier, and if this particular call to strtok_r returns null, then that means we got a blank line. So just continue.

@@ +552,5 @@
> +        if (line[0] == '#' || line[0] == '\0')
> +            continue;
> +        if (!(vol_label = strtok_r(line, " ", &save_ptr))) {
> +            ERR("Error parsing vol_label");
> +            return;

These early returns are causing leaked file handles (fp).

We should add ScopedCloseFILETraits and ScopedFILEClose to xpcom/glue/FileUtils.h and use that to ensure that we don't leak the FILE *

Put the changes to FileUtils.h into a separate patch, since it will need a different reviewer (I can do the initial review, but I'll need to get an xpcom peer to do the final review).

@@ +555,5 @@
> +            ERR("Error parsing vol_label");
> +            return;
> +        }
> +        if (!(mount_point = strtok_r(NULL, " ", &save_ptr))) {
> +            ERR("Error parsing mount_point");

This should give the name of the file being parsed, and the line number. It should also give the name of the volume that was missing a mount point. So I'd do something like:

ERR("No mount point for volume '%s'. %s line %d", vol_name_cstr, filename, n);

@@ +559,5 @@
> +            ERR("Error parsing mount_point");
> +            return;
> +        }
> +    }
> +    nsCOMPtr<nsIVolumeService> vs = do_GetService(NS_VOLUMESERVICE_CONTRACTID);

You should check that vs is non-null before calling SetFakeVolume. I'd probably move the call to initialize vs to before the while loop and return early if vs is null.

Also this code from here to the end of the function should be inside the while loop.

@@ +560,5 @@
> +            return;
> +        }
> +    }
> +    nsCOMPtr<nsIVolumeService> vs = do_GetService(NS_VOLUMESERVICE_CONTRACTID);
> +    LOG("vol_label: %s, mount_point: %s\n", NS_strdup(vol_label), NS_strdup(mount_point));

You've got memory leaks with these calls to NS_strdup since you're not freeing the memory. You should be passing vol_label and mount_point directly.

@@ +562,5 @@
> +    }
> +    nsCOMPtr<nsIVolumeService> vs = do_GetService(NS_VOLUMESERVICE_CONTRACTID);
> +    LOG("vol_label: %s, mount_point: %s\n", NS_strdup(vol_label), NS_strdup(mount_point));
> +    nsString fusepath = NS_ConvertUTF8toUTF16(NS_strdup(mount_point));
> +    nsString fusename = NS_ConvertUTF8toUTF16(NS_strdup(vol_label));

Same thing here. You should just remove these calls to NS_strdup. They aren't doing anything useful and you're introducing memory leaks.

Let's use some names like mount_point_cstr for the char *, and mount_point for the nsString. Similarly, vol_name_cstr for the char * and vol_name for the nsString.

@@ +563,5 @@
> +    nsCOMPtr<nsIVolumeService> vs = do_GetService(NS_VOLUMESERVICE_CONTRACTID);
> +    LOG("vol_label: %s, mount_point: %s\n", NS_strdup(vol_label), NS_strdup(mount_point));
> +    nsString fusepath = NS_ConvertUTF8toUTF16(NS_strdup(mount_point));
> +    nsString fusename = NS_ConvertUTF8toUTF16(NS_strdup(vol_label));
> +    vs->CreateFakeVolume(fusename, fusepath);

We should verify that the CreateFakeVolume actually suceeded (i.e. no duplicate name) and we should verify that the mount point actually exists.

@@ +572,4 @@
>  InitAutoMounter()
>  {
>    InitVolumeManager();
> +  InitFuseVolume();

And since we're going to call this InitVolumeConfig, let's put this before the call to InitVolumeManager, in case we decide to add stuff later.

Comment 6

[Dave Hylands [:dhylands]]

Comment on attachment 773863 [details] [diff] [review]
Support fuse volume by configuration file in AutoMounter

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

::: dom/system/gonk/AutoMounter.cpp
@@ +541,5 @@
> +    FILE *fp;
> +    int n = 0;
> +    char line[255];
> +    char *vol_label, *mount_point, *save_ptr;
> +    if (!(fp = fopen("/etc/fuse.xml", "r"))) {

For the time being, the volume.cfg file is optional. So the error shouldn't really be an error, but rather just informational. So it should use LOG rather than ERR.

It should also be worded something along the lines of:

LOG("Unable to open volume configuration file '%s' - ignoring", filename);

So that it doesn't look like an error in the log.

Comment 7

[viral [:viralwang]]

Attachment: Use InitVolumeConfig to add volume by configuration file (v2)

- used more genereic naming
- change the configuration file to "/system/etc/volume.cfg"
- add error handler and useful log
- reduce memory leak

Comment 8

[viral [:viralwang]]

Attachment: add copedCloseFile to reduce memory leak

add copedCloseFile to avoid memory leak for FILE *

Comment 9

[Dave Hylands [:dhylands]]

Comment on attachment 774477 [details] [diff] [review]
Use InitVolumeConfig to add volume by configuration file (v2)

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

r=me with nits addressed.

::: dom/system/gonk/AutoMounter.cpp
@@ +537,5 @@
>  
> +static void
> +InitVolumeConfig()
> +{
> +  // This function is used /system/etc/volume.cfg to add additional volume

nit: s/is used/uses/
nit: add a period to the end of the sentence.

@@ +540,5 @@
> +{
> +  // This function is used /system/etc/volume.cfg to add additional volume
> +  // The format of volume.cfg will be:
> +  // create sdcard /path/to/sdcard/mountpoint
> +    nsCOMPtr<nsIVolumeService> vs = do_GetService(NS_VOLUMESERVICE_CONTRACTID);

nit: There seems to be an indentation change in the function here. The rest of the function should be at the same indentation as the commments.

@@ +559,5 @@
> +
> +        if (line[0] == '#')
> +            continue;
> +        if (!(command = strtok_r(line, delim, &save_ptr))) {
> +            ERR("No command in %s line %d",  filename, n);

nit: This shouldn't be an error. This will happen on blank lines, which we should just ignore. So just replace the ERR with a comment like:

// Blank line - ignore

@@ +575,5 @@
> +            nsString  mount_point = NS_ConvertUTF8toUTF16(mount_point_cstr);
> +            nsString vol_name = NS_ConvertUTF8toUTF16(vol_name_cstr);
> +            nsresult rv;
> +            rv = vs->CreateFakeVolume(vol_name, mount_point);
> +            NS_SUCCEEDED(rv);

nit: This should ne NS_ENSURE_SUCCESS_VOID(rv);

@@ +578,5 @@
> +            rv = vs->CreateFakeVolume(vol_name, mount_point);
> +            NS_SUCCEEDED(rv);
> +            rv = vs->SetFakeVolumeState(vol_name, nsIVolume::STATE_MOUNTED);
> +            NS_SUCCEEDED(rv);
> +            LOG("vol_name: %s, mount_point: %s\n", vol_name_cstr, mount_point_cstr);

Do we need this log? Doesn't the volume manager already produce a log when the volume changes to the mounted state?

Comment 10

[Dave Hylands [:dhylands]]

Comment on attachment 774481 [details] [diff] [review]
add copedCloseFile to reduce memory leak

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

The indentation on the new function doesn't match the rest of the file.
Passing off to a module peer to give the final review.

::: xpcom/glue/FileUtils.h
@@ +70,5 @@
>  
> +/* RAII wrapper for FILE descriptors */
> +struct ScopedCloseFileTraits
> +{
> +    typedef FILE *type;

nit: use 2 space indentation

Comment 11

[Justin Lebar (not reading bugmail)]

I'm in the middle of some critical b2g work; it might be a week or more before I can look at this (even though it's very small).  I'm really sorry.  :(

Comment 12

[Marco Chen [:mchen]]

Hi Viral,

It seems that the work on Gecko part is almost ready here.
May I know the plan for adding "/system/etc/volume.cfg" into mako (Nexus 4) device folder?

Thanks.

Comment 13

[viral [:viralwang]]

Hi Marco,

I will prepare a patch in device/lge/mako later to add the volume.cfg for Nexus 4.
Thanks.

Comment 14

[viral [:viralwang]]

Attachment: Add the volume.cfg file to add additional volume for mako

Hi Michael,

this patch is for device/lge/mako/
we will used this configuration file to enable an additional volume
for those applications need device storage will need this function to develop.
Thanks.

Comment 15

[viral [:viralwang]]

Attachment: add copedCloseFile to reduce memory leak (v2)

Hi Justin,

I know you may not have time slot to review this patch soon.
Update the patch first since the indentation is not correct.
Thank you!

Comment 16

[viral [:viralwang]]

Hi Dave,

I would like to have some discussion with your suggestion.
Could you please help to review the following description?
Thank you in advance.
(In reply to Dave Hylands [:dhylands] from comment #9)
> ::: dom/system/gonk/AutoMounter.cpp
> @@ +537,5 @@
> >  
> > +static void
> > +InitVolumeConfig()
> > +{
> > +  // This function is used /system/etc/volume.cfg to add additional volume
> 
> nit: s/is used/uses/
> nit: add a period to the end of the sentence.
I add some more comments here to explain the bug and the volume.cfg as below:

  // In some devices with no internal partition and external sdcard, we uses
  // this function to check /system/etc/volume.cfg to add volume
  // The format of volume.cfg will be:
  // create sdcard /path/to/sdcard/mountpoint

> @@ +578,5 @@
> > +            rv = vs->CreateFakeVolume(vol_name, mount_point);
> > +            NS_SUCCEEDED(rv);
> > +            rv = vs->SetFakeVolumeState(vol_name, nsIVolume::STATE_MOUNTED);
> > +            NS_SUCCEEDED(rv);
> > +            LOG("vol_name: %s, mount_point: %s\n", vol_name_cstr, mount_point_cstr);
> 
> Do we need this log? Doesn't the volume manager already produce a log when
> the volume changes to the mounted state?

this log will help us to know the volume is added by the volume.cfg
I think we should have some different log if we can not have device to check where the volume comes from.

Comment 17

[Michael Wu [:mwu]]

Comment on attachment 775473 [details] [diff] [review]
Add the volume.cfg file to add additional volume for mako

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

::: full_mako.mk
@@ +19,5 @@
>  
>  # Get the long list of APNs
> +PRODUCT_COPY_FILES := \
> +  device/sample/etc/apns-full-conf.xml:system/etc/apns-conf.xml \
> +  device/lge/mako/volume.cfg:system/etc/volume.cfg

Please put this new entry in https://github.com/mozilla-b2g/device-mako/blob/b2g-4.2.2_r1/device.mk#L262 where we've put another gecko specific thing.

Comment 18

[Dave Hylands [:dhylands]]

(In reply to vwang from comment #16)
> Hi Dave,
> 
> I would like to have some discussion with your suggestion.
> Could you please help to review the following description?
> Thank you in advance.
> (In reply to Dave Hylands [:dhylands] from comment #9)
> > ::: dom/system/gonk/AutoMounter.cpp
> > @@ +537,5 @@
> > >  
> > > +static void
> > > +InitVolumeConfig()
> > > +{
> > > +  // This function is used /system/etc/volume.cfg to add additional volume
> > 
> > nit: s/is used/uses/
> > nit: add a period to the end of the sentence.
> I add some more comments here to explain the bug and the volume.cfg as below:
> 
>   // In some devices with no internal partition and external sdcard, we uses
>   // this function to check /system/etc/volume.cfg to add volume
>   // The format of volume.cfg will be:
>   // create sdcard /path/to/sdcard/mountpoint

So, I'd probably put the generic description first, and then give the example. So combining them, something like:

// This function is used /system/etc/volume.cfg to add additional volumes
// to the Volume Manager.
//
// This is useful on devices like the Nexus 4, which have no physical sd card
// or dedicated partition.
//
// The format of the volume.cfg file is as follows:
// create volume-name mount-point
// Blank lines and lines starting with the hash character "#" will be ignored.

> > @@ +578,5 @@
> > > +            rv = vs->CreateFakeVolume(vol_name, mount_point);
> > > +            NS_SUCCEEDED(rv);
> > > +            rv = vs->SetFakeVolumeState(vol_name, nsIVolume::STATE_MOUNTED);
> > > +            NS_SUCCEEDED(rv);
> > > +            LOG("vol_name: %s, mount_point: %s\n", vol_name_cstr, mount_point_cstr);
> > 
> > Do we need this log? Doesn't the volume manager already produce a log when
> > the volume changes to the mounted state?
> 
> this log will help us to know the volume is added by the volume.cfg
> I think we should have some different log if we can not have device to check
> where the volume comes from.

I think that you'll also get a log entry like the following when it changes to mounted.

UpdateState: Volume storage is Mounted and inserted @ /data/storage gen 1 locked 0 sharing x

If you still want the log from InitVolumeConfig, then it should at least mention volume.cfg, something along the lines of:

volume.cfg: Added volume '%s' mount-point '%s'

Comment 19

[viral [:viralwang]]

Attachment: Add the volume.cfg file to add additional volume for mako (v2)

Put the new entry to add volume.cfg in device.mk under device-mako

Comment 20

[Justin Lebar (not reading bugmail)]

Comment on attachment 775482 [details] [diff] [review]
add copedCloseFile to reduce memory leak (v2)

r=me, sorry for the delay.

Comment 21

[viral [:viralwang]]

Attachment: Use InitVolumeConfig to add volume by configuration file (v3)

- make the indentation change correctly.
- rewrite the comment/log to have a clear description.

Comment 22

[Dave Hylands [:dhylands]]

Comment on attachment 778266 [details] [diff] [review]
Use InitVolumeConfig to add volume by configuration file (v3)

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

r=me with comments addressed.

::: dom/system/gonk/AutoMounter.cpp
@@ +570,5 @@
>  
> +static void
> +InitVolumeConfig()
> +{
> +  // This function is used /system/etc/volume.cfg to add additional volumes

nit:s/is used/uses/

@@ +578,5 @@
> +  // or dedicated partition.
> +  //
> +  // The format of the volume.cfg file is as follows:
> +  // create volume-name mount-point
> +  // Blank lines and lines starting with the hash character "#" will be ignored.

I'd add a blank line here (after the comment block)

@@ +617,5 @@
> +      rv = vs->CreateFakeVolume(vol_name, mount_point);
> +      NS_ENSURE_SUCCESS_VOID(rv);
> +      rv = vs->SetFakeVolumeState(vol_name, nsIVolume::STATE_MOUNTED);
> +      NS_ENSURE_SUCCESS_VOID(rv);
> +    }

We should report unrecognized commands.

<pre>
else {
  ERR("Unrecognized command: '%s'", command);
}
</pre>

Comment 23

[viral [:viralwang]]

Attachment: Use InitVolumeConfig to add volume by configuration file (v4)

- modify the description and format of comment
- add the error log if the command is unrecognized.is

Comment 24

[viral [:viralwang]]

Attachment: Add the volume.cfg file to add additional volume for mako (final)

- modify the comment of patch for landing.

Comment 25

[viral [:viralwang]]

Attachment: add copedCloseFile to reduce memory leak (final)

- modify the comment of patch for landing.

Comment 26

[viral [:viralwang]]

add try server result
https://tbpl.mozilla.org/?tree=Try&rev=cb58917f81d2
https://tbpl.mozilla.org/?tree=Try&rev=bbc83c9edf06

Comment 27

[viral [:viralwang]]

Gecko part include following two patch already pass try server.
1)Use InitVolumeConfig to add volume by configuration file (v4)
2)add copedCloseFile to reduce memory leak (final)

Another patch also send the pull request for landing.
3)Add the volume.cfg file to add additional volume for mako (final)
https://github.com/mozilla-b2g/device-mako/pull/1

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/projects/birch/rev/8ea49e82b4a6
https://hg.mozilla.org/projects/birch/rev/d5cdac5f280e
https://github.com/mozilla-b2g/device-mako/commit/ab79ce9693322ba39e8c701622af6af64ce553cd

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8ea49e82b4a6
https://hg.mozilla.org/mozilla-central/rev/d5cdac5f280e