971615 - Implement "storageStatus" API for device storage to report volume status

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Alan Huang]

Currently we cannot format device storage with unrecognized format. But this comes out we can only wipe data on a device storage we can recognize. In the case that the device storage has no file system on it yet, we cannot recognize it so we cannot format it.

This should be something we need to fix :)

Comment 1

[Dave Hylands [:dhylands]]

So the volume manager should still report on the volumes, whether they're formatted or not.

I'm actually surprised that device storage doesn't show the volume as unavilable.

What does:

adb shell vdc volume list

show when this unrecognized volume is present?

Comment 2

[Alan Huang]

Sorry, I realized I didn't describe the case I met precisely in the description.

(In reply to Dave Hylands [:dhylands] from comment #1)
> So the volume manager should still report on the volumes, whether they're
> formatted or not.
> I'm actually surprised that device storage doesn't show the volume as
> unavilable.

One day I accidentally deleted the partition on my SD card, then this came into my mind so I tried this. 

The volume manager DO report on the volume, and device storage show it as "unavailable". User cannot format the volume because now Settings app won't allow user to format a device storage which is not "available", according to the UX spec. 

In case like this, AutoMounter send command 'volume mount sdcard' to vold, and vold finally changes the state of the volume to STATE_IDLE : Pending -> Idle-Unmounted -> Checking (but failed to mount via VFAT) -> Idle-Unmounted. Vold reports OperationFailed to the command.

So, I think device storage should show "idle" or something else like "unrecognized", if the the volume state is STATE_IDLE. If device storage show the volume is "available" or "idle"/"unrecognized", then Settings app could format this volume. 

> What does:
> adb shell vdc volume list
> show when this unrecognized volume is present?

$ adb shell vdc volume list
110 sdcard /mnt/sdcard 1
200 Volumes listed.

Comment 3

[Alan Huang]

Attachment: Add additional device storage file status, v1

This patch add an additional status "idle-unmounted" reported by DeviceStorageFile::GetStatus. This could help gaia apps to distinguish this state from "unavailable".

Comment 4

[Alan Huang]

Attachment: Check whether a volume is idle-unmounted or not in Settings app's media storage function

This is a patch I used to test how attachment 8375371 [details] [diff] [review] can help settings app.

Comment 5

[Dave Hylands [:dhylands]]

(In reply to Alan Huang [:ahuang] from comment #3)
> Created attachment 8375371 [details] [diff] [review]
> Add additional device storage file status, v1
> 
> This patch add an additional status "idle-unmounted" reported by
> DeviceStorageFile::GetStatus. This could help gaia apps to distinguish this
> state from "unavailable".

Its not clear to me why this is even required. And I think it messes up media storage.

We should really allow the settings app to query the volume service to get the real volume status for things like format/mount/unmount.

Comment 6

[Dave Hylands [:dhylands]]

Comment on attachment 8375371 [details] [diff] [review]
Add additional device storage file status, v1

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

At the very least this breaks all of the media apps.

I think that it would be better to add a new function named something like storageStatus that returns the actual volume status, rather than trying to wedge this into available()

Comment 7

[[e-mail], [e-mail]]

Add media team devs to join the discussion during API implementing state.

Comment 8

[Alan Huang]

Attachment: Implement "storageStatus" API for device storage, v1

This patch implement an API "storageStatus", to report the actual volume state.

According to the return value of this API, we can know the volume is currently in one of below state: Init, NoMedia, Idle, Pending, Checking, Mounted, Unmounting, Formatting, Shared and Shared-Mounted. The states are listed in http://hg.mozilla.org/mozilla-central/file/6687d299c464/dom/system/gonk/nsIVolume.idl

This can help gaia app to know more details about the state change.

Comment 9

[Alan Huang]

Attachment: ds-test patch to test attachment 8376118

Comment 10

[Dave Hylands [:dhylands]]

Comment on attachment 8376118 [details] [diff] [review]
Implement "storageStatus" API for device storage, v1

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

r=me with minor issues addressed

::: dom/devicestorage/DeviceStorage.h
@@ +98,5 @@
>                        uint64_t* aMusicSoFar, uint64_t* aTotalSoFar);
>  
>    void GetDiskFreeSpace(int64_t* aSoFar);
>    void GetStatus(nsAString& aStatus);
> +  void GetVolumeState(nsAString& aStatus);

Lets call this GetStorageStatus

::: dom/devicestorage/PDeviceStorageRequest.ipdl
@@ +59,5 @@
>  };
>  
> +struct StorageStatusResponse
> +{
> +  nsString mountState;

I think this should be called storageStatus (I think that mountState from AvailableStorageResponse is badly named too)

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +1413,5 @@
> +    return;
> +  }
> +  aStatus.AssignLiteral("undefined");
> +  if (!typeChecker->IsVolumeBased(mStorageType)) {
> +    return;

For consistency with GetStatus (used for Available) we should return the same thing that we return for Mounted with volume based storages.

For VolumeBased storages, I think its ok to initialize aStatus to "undefined"

Comment 11

[Alan Huang]

Attachment: Implement "storageStatus" API for device storage, v2. r=dhylands

Comment 12

[Alan Huang]

(In reply to Dave Hylands [:dhylands] from comment #10)
> ::: dom/devicestorage/nsDeviceStorage.cpp
> @@ +1413,5 @@
> > +    return;
> > +  }
> > +  aStatus.AssignLiteral("undefined");
> > +  if (!typeChecker->IsVolumeBased(mStorageType)) {
> > +    return;
> 
> For consistency with GetStatus (used for Available) we should return the
> same thing that we return for Mounted with volume based storages.
Hi Dave,

I'm not sure whether I get exactly what you mean here, so I made another patch for review. Do you mean, we return both "available" in both GetStorageStatus and GetStatus if we are handling a device storage which is NOT volume based? Thanks.

Comment 13

[Dave Hylands [:dhylands]]

(In reply to Alan Huang [:ahuang] from comment #12)
> (In reply to Dave Hylands [:dhylands] from comment #10)
> > ::: dom/devicestorage/nsDeviceStorage.cpp
> > @@ +1413,5 @@
> > > +    return;
> > > +  }
> > > +  aStatus.AssignLiteral("undefined");
> > > +  if (!typeChecker->IsVolumeBased(mStorageType)) {
> > > +    return;
> > 
> > For consistency with GetStatus (used for Available) we should return the
> > same thing that we return for Mounted with volume based storages.
> Hi Dave,
> 
> I'm not sure whether I get exactly what you mean here, so I made another
> patch for review. Do you mean, we return both "available" in both
> GetStorageStatus and GetStatus if we are handling a device storage which is
> NOT volume based? Thanks.

Exactly. Since its not volume based, this implies that the storage area is always available (i.e. it's actually a local harddrive.) This only happens when running under desktop or perhaps the windows-based simulator.

Comment 14

[Dave Hylands [:dhylands]]

Comment on attachment 8377373 [details] [diff] [review]
Implement "storageStatus" API for device storage, v2. r=dhylands

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

Sorry - I meant to mark this as r+

Comment 15

[Alan Huang]

https://tbpl.mozilla.org/?tree=Try&rev=3126388fc6da

Ah, it is really weird that my local build passed patch got build break in all platforms. Remove check-in needed first.

Comment 16

[Alan Huang]

Attachment: Implement "storageStatus" API for device storage, v3. r=dhylands

Name space mozilla::system is not used in all platforms. Using this namespace should be enclosed in where MOZ_WIDGET_GONK is defined.

https://tbpl.mozilla.org/?tree=Try&rev=a0c1345d3136
The test results are good this time by removing below:

>diff --git a/dom/devicestorage/nsDeviceStorage.cpp b/dom/devicestorage/nsDeviceStorage.cpp
>--- a/dom/devicestorage/nsDeviceStorage.cpp
>+++ b/dom/devicestorage/nsDeviceStorage.cpp
>@@ -65,16 +65,17 @@
> 
> #define DEVICESTORAGE_PROPERTIES \
>   "chrome://global/content/devicestorage.properties"
> #define DEFAULT_THREAD_TIMEOUT_MS 30000
> 
> using namespace mozilla;
> using namespace mozilla::dom;
> using namespace mozilla::dom::devicestorage;
>+using namespace mozilla::system;
> using namespace mozilla::ipc;

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/a1b0f89f74dd

I guess this will have tests on the Gaia side?

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a1b0f89f74dd