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

RESOLVED FIXED in 1.4 S2 (28feb)

Status

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: alan.yenlin.huang, Assigned: alan.yenlin.huang)

Tracking

({dev-doc-needed})

unspecified
1.4 S2 (28feb)
ARM
Gonk (Firefox OS)
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

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 :)
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?
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.
This patch add an additional status "idle-unmounted" reported by DeviceStorageFile::GetStatus. This could help gaia apps to distinguish this state from "unavailable".
Attachment #8375371 - Flags: review?(dhylands)
This is a patch I used to test how attachment 8375371 [details] [diff] [review] can help settings app.
Attachment #8375373 - Flags: feedback?(iliu)
(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 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()
Attachment #8375371 - Flags: review?(dhylands) → review-
Summary: Add support of formatting device storage with unrecognized format → Implement "storageStatus" API and for device storage
Attachment #8375373 - Attachment is obsolete: true
Attachment #8375373 - Flags: feedback?(iliu)
Summary: Implement "storageStatus" API and for device storage → Implement "storageStatus" API for device storage to report volume status
Add media team devs to join the discussion during API implementing state.
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.
Attachment #8375371 - Attachment is obsolete: true
Attachment #8376118 - Flags: review?(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"
Attachment #8376118 - Flags: review?(dhylands) → review+
Attachment #8376118 - Attachment is obsolete: true
Attachment #8377373 - Flags: review?(dhylands)
(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.
(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.
Attachment #8377373 - Attachment description: Implement "storageStatus" API for device storage, v2 → Implement "storageStatus" API for device storage, v2. r=dhylands
Attachment #8377373 - Flags: review?(dhylands)
Keywords: checkin-needed
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+
Attachment #8377373 - Flags: review+
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.
Keywords: checkin-needed
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;
Attachment #8377373 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/integration/b2g-inbound/rev/a1b0f89f74dd

I guess this will have tests on the Gaia side?
Flags: in-testsuite?
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a1b0f89f74dd
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.4 S2 (28feb)
You need to log in before you can comment on or make changes to this bug.