841660 - sd card format feature not supplied

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Settings, defect)

Description

[leo.bugzilla.gecko]

Dear Mozilla Team,

In Settings - Media storage menu
It does not have sd card initialization(format) function

I'm looking for API that can be used to format the sd card.

Comment 1

[Gregor Wagner [:gwagner]]

Adding random people that might know the answer.

Comment 2

[Dave Hylands [:dhylands]]

Currently, there is no API.

We'd need to add an API to the VolumeService, and potentially device storage.

The API needs to be IPC'd from the content process to the main process.

Do we need any additional permissions?

I'm guessing that we'd add:

void Format();

to nsIVolume https://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/nsIVolume.idl#9

Comment 3

[Marco Chen [:mchen]]

Discussed with Dave and take this.

Comment 4

[Marco Chen [:mchen]]

Attachment: Part 1: New API v1

Hi Dave,

In order to let Gaia app has a way to fire format command, I add format API into nsIDOMDeviceStorage.idl then nsIVolume.idl.

Q1: I think gonk would support FAT32 only because vold support that only too. But do we need to expose a argument to nsIDOMDeviceStorage.idl::format() for more generic usuage? If so then maybe we need to implement a way to report what formats supporting by this backend.

Q2: Do we allow to format internal storage on gonk platform? Or we need a way to tell Gaia whether this devicestorage can be formated or not.

Thanks for your suggestion.

Comment 5

[Dave Hylands [:dhylands]]

(In reply to Marco Chen [:mchen] from comment #4)
> Created attachment 728116 [details] [diff] [review]
> Part 1: New API v1
> 
> Hi Dave,
> 
> In order to let Gaia app has a way to fire format command, I add format API
> into nsIDOMDeviceStorage.idl then nsIVolume.idl.
> 
> Q1: I think gonk would support FAT32 only because vold support that only
> too. But do we need to expose a argument to
> nsIDOMDeviceStorage.idl::format() for more generic usuage? If so then maybe
> we need to implement a way to report what formats supporting by this backend.

I think that device storage having a format is purely to support USB Mass Storage, which only works with FAT32, so I don't see any reason to support anything else right now.

I think that format (from device storage) should fail on non-gonk platforms.

> Q2: Do we allow to format internal storage on gonk platform? Or we need a
> way to tell Gaia whether this devicestorage can be formated or not.

Personally, I don't see any reason not to support formatting internal storage. It really isn't that much different from sharing the volume with the PC and having the PC format it.

Formatting the volume should require write permissions. We need to decide if we want to require any additional permissions.

Comment 6

[Dave Hylands [:dhylands]]

Comment on attachment 728116 [details] [diff] [review]
Part 1: New API v1

Doug Turner is the one we really need feedback from. Changing the request to Doug.

Comment 7

[Doug Turner (:dougt)]

Comment on attachment 728116 [details] [diff] [review]
Part 1: New API v1

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

this is okay -- but david and I were thinking about supporting multiple volumes per device storage.  I suppose it has the same problem as freeSpace and usedSpace.

What sort of permission would be required here

Comment 8

[Dave Hylands [:dhylands]]

With the latest proposal (of returning multiple deviceStorage objects by calling navigator.getDeviceStorages) then I think that this variant of format makes sense.

Comment 9

[Marco Chen [:mchen]]

(In reply to Doug Turner (:dougt) from comment #7)
> this is okay -- but david and I were thinking about supporting multiple
> volumes per device storage.  I suppose it has the same problem as freeSpace
> and usedSpace.

Thanks for your feedback first.
1. If we put multiple volumes into a devicestorage object, then how could user choose which volume to do format from deviceStorage's DOM API?
2. And as your point, the query of freeSpace and usedSpace would be the problem too.
3. So is it more suitalbe to have a one to one mapping between devicestorage & volume?
4. And accroding to external SD is mounted at subdirectory of /sdcard, user can query all files from deviceStorage with volume of internal SD because it's root_directory is "/sdcard".
5. For writing, the old app used getDeviceStorage("picture/video/music/sdcard") would return the internal SD, so it is no harmful. For advanced apps, they can use getDeviceStorages() to query how many deviceStorages (volumes) can be used then expose to user to do selection.

> What sort of permission would be required here

I think the permission as below is enough because the write permission is allowed to delete files and format is some kind of delete all.

  "device-storage:sdcard":{ "access": "readwrite" }

Thanks for Dog and Dave's discussion here.

Comment 10

[Marco Chen [:mchen]]

Hi all,

I want to make sure what conditions we allow user to do fomrat?

  1. When this sd card is not formated yet.

  2. When this sd card is mounted already.

  3. User needs to unmount sdcard first then can do format.

If we want to support 3 then we need the unmount API too.
And I think 2 is not preferred to allow.

So we can do format command only when volume state is on idle only.
Any comment, Thanks very much.

Comment 11

[Dave Hylands [:dhylands]]

The AutoMounter will need to know that it shouldn't try to mount the volume. Right now, if the volume is in the idle state, then the AutoMounter will either try to mount or share the volume.

From the implementation standpoint, we should probably implement it as if an Unmount command is available, then it just becomes a UX choice about exactly when the Unmount is called.

Calling volume->Unmount should change some state in the volume that the AutoMounter would query to see if it should mount/share the volume or leave it unmounted.

Comment 12

[Marco Chen [:mchen]]

Thanks for Alan's help in this bug now.

Comment 13

[Alan Huang]

Attachment: Proposed implementation

Hello Dave,

I have this possible implementation need your feedback. I'm sure I did do everything right here yet, but it would be great if you can give me some feedback in this incomplete version.

Thank you :)

Comment 14

[Alan Huang]

(In reply to Alan Huang [:ahuang] from comment #13)
> I have this possible implementation need your feedback. I'm sure I did do
> everything right here yet, but it would be great if you can give me some
> feedback in this incomplete version.
Hi Dave,
Uh, ... a terrible typo. I was trying to say I "didn't" do everything right yet, sorry  :(

Comment 15

[Alan Huang]

Attachment: DeviceStorage-test

Device Storage test.

Comment 16

[Alan Huang]

Attachment: Proposed implementation

I think this one is much better than previous one :)

Comment 17

[Dave Hylands [:dhylands]]

Comment on attachment 8335949 [details] [diff] [review]
Proposed implementation

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

So overall, this looks really good.

I've made some suggestions for a few name changes, and I really think that the format command belongs in nsIVolume and not in nsIVolumeService

And I think that there is one race condition that we need to deal with (in AutoMounter.cpp).

Great work.

::: dom/system/gonk/AutoMounter.cpp
@@ +528,5 @@
> +          } else if (vol->IsFormattingEnabled()){
> +            // Volume is unmounted. We can go ahead and format.
> +            LOG("UpdateState: Formatting %s", vol->NameStr());
> +            vol->StartFormat(mResponseCallback);
> +            vol->SetFormattingEnabled(false);

UpdateState may get called again before we get the notification from vold that the state has transitioned to from the idle state.

I think that vol->SetFormatRequested(false) shouldn't happen until we detect nsIVolume::STATE_FORMATTING

Otherwise if UpdateState got called twice in a row, we could try to Mount (line 518) before the format even started.

Here's a timeline that describes what I'm talking about:
1 - UpdateState is called, and the volume is IDLE. So it sends the format command to vold
2 - UpdateState is called again, and the volume is still IDLE, so now it sends the mount command to vold
3 - The state change from IDLE to FORMATTING is received from vold

@@ +774,4 @@
>  {
>    XRE_GetIOMessageLoop()->PostTask(
>        FROM_HERE,
>        NewRunnableFunction(SetAutoMounterSharingModeIOThread, 

nit: trailing space (not part of your patch)

::: dom/system/gonk/AutoMounter.h
@@ +59,5 @@
>  void
>  SetAutoMounterSharingMode(const nsCString& aVolumeName, bool aAllowSharing);
>  
> +void
> +SetAutoMounterFormattingMode(const nsCString& aVolumeName);

Add a comment describing the purpose of this function.

I also think it should be renamed to AutoMounterFormatVolume(const nsCString& aVolumeName);

::: dom/system/gonk/Volume.cpp
@@ +61,5 @@
>      mSharingEnabled(false),
>      mCanBeShared(true),
> +    mIsSharing(false),
> +    mFormattingEnabled(false),
> +    mCanBeFormatted(true),

I don't see any where that mCanBeFormatted is set to false.

In fact, I think that you can use CanBeShared in place of mCanBeFormatted (so you might want to create an inline function

bool CanBeFormatted() { return CanBeShared(); }

and use that instead of mCanBeFormatted.

::: dom/system/gonk/Volume.h
@@ +46,5 @@
>    bool IsMountLocked() const          { return mMountLocked; }
>    bool MediaPresent() const           { return mMediaPresent; }
>    bool CanBeShared() const            { return mCanBeShared; }
>    bool IsSharingEnabled() const       { return mCanBeShared && mSharingEnabled; }
> +  bool IsFormattingEnabled() const    { return mCanBeFormatted && mFormattingEnabled; }

I think that this should be renamed to IsFormatRequested

@@ +53,3 @@
>  
>    void SetSharingEnabled(bool aSharingEnabled);
> +  void SetFormattingEnabled(bool aSharingEnabled);

And this should be renamed to SetFormatRequest.

nit: argument should be called aFormatRequest (this appears to be a copy/paste error)

::: dom/system/gonk/VolumeServiceIOThread.cpp
@@ +79,5 @@
>    sVolumeServiceIOThread = nullptr;
>  }
>  
> +void
> +FormatVolume(const nsCString& aVolume)

Once the format command gets moved to nsVolume::Format, then I think that this function should also move into nsVolume (I think that the function will be the same).

We should probably call it FormatVolumeIOThread to signify that it runs on the IOThread, but otherwise I think its just a case of moving the function.

@@ +87,5 @@
> +    return;
> +  }
> +
> +  SetAutoMounterFormattingMode(aVolume);
> +

nit: extraneous blank line

::: dom/system/gonk/nsIVolume.idl
@@ +61,5 @@
>    // device storage to suppress unwanted 'unavailable' status when
>    // transitioning from mounted to sharing and back again.
>    readonly attribute boolean isSharing;
>  
> +  readonly attribute boolean isFormatting;

You need to change the uuid if you change the interface.

Pleade add a comment to describe this attribute (perhaps referring to isSharing since the purpose of isFormatting parallels isSharing.

::: dom/system/gonk/nsIVolumeService.idl
@@ +15,1 @@
>  [scriptable, uuid(a3b110cd-74f2-43cb-84c6-2a87713f2774)]

You need to change the uuid if you change the interface

@@ +19,5 @@
>      nsIVolume getVolumeByPath(in DOMString path);
>      nsIVolume createOrGetVolumeByPath(in DOMString path);
>  
>      void BroadcastVolume(in DOMString volName);
> +    void formatVolumeByName(in DOMString volName);

This feels like the wrong place for this. I think that it would be better to put a format() method in nsIVolume.idl

The volume service is really for things that need to deal with the collection of volumes, and formatting will only every need to deal with a single volume.

BroadcastVolume allows the volume name to be an empty string, which broadcasts all volumes, which is the only reason that function is here.

::: dom/system/gonk/nsVolumeService.cpp
@@ +172,5 @@
>  }
>  
> +NS_IMETHODIMP
> +nsVolumeService::FormatVolumeByName(const nsAString& aVolName)
> +{

I think that this should be moved to nsVolume::Format()

Comment 18

[Alan Huang]

(In reply to Dave Hylands [:dhylands] from comment #17)
> I've made some suggestions for a few name changes, and I really think that
> the format command belongs in nsIVolume and not in nsIVolumeService

Thanks. After look into the logic between nsIVolumeService and nsIVolume more, I think I should move the format command to nsIVolume.

> And I think that there is one race condition that we need to deal with (in
> AutoMounter.cpp).
> 
> UpdateState may get called again before we get the notification from vold
> that the state has transitioned to from the idle state.
> 
> I think that vol->SetFormatRequested(false) shouldn't happen until we detect
> nsIVolume::STATE_FORMATTING

Yeah, I think you are right. I didn't consider the race condition here.

I think I can fix the race condition in this way:
Once the volume detect state change to nsIVolume::STATE_FORMATTING, we set the mFormatRequested to false. This can happen in SetState in Volume.cpp, just like how I set mIsFormatting to false.

Comment 19

[Alan Huang]

Attachment: Implement format interface for volume-based device storage.

Compare to the previous one, this patch:
1. Fix race condition in AutoMounter UpdateState.
2. Move format command from nsIVolumeService to nsIVolume.
3. include other minor changes.

Comment 20

[Alan Huang]

Comment on attachment 8335948 [details] [diff] [review]
DeviceStorage-test

Hi Ian,
This is the test we talked about. You can take a look to see is there anything else you need to discuss :)

Comment 21

[Alan Huang]

Attachment: Implement format interface for volume-based device storage, v1

This version include a change after I discuss with gaia owner. Currently, I'm not going to report a new status called "formatting" in getStatus(). Current gaia apps listen to storage change and take actions with "available", "unavailable" and "shared". They don't know status "formatting". I use "unavailable" for "formatting" in this patch, and we should change this in future.

Comment 22

[Dave Hylands [:dhylands]]

nit: It would be good if you keep the patch description the same each time and just add v2 or change vx to vx+1

Comment 23

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

Comment on attachment 8335948 [details] [diff] [review]
DeviceStorage-test

Hi Alan,

Since we have discussion for "formatting" event, it won't be provided in the v1.3. And Gecko will change the deviceStorage status to be "unavailable" while a user is formatting SD card. For the state transition, all apps would handle formatting case correctly via "unavailable" status.

The test case is working fine for me. And Gaia patch is in reviewing process(https://bugzilla.mozilla.org/show_bug.cgi?id=929860#c2). Thanks for your support.

Comment 24

[Dave Hylands [:dhylands]]

Comment on attachment 8339119 [details] [diff] [review]
Implement format interface for volume-based device storage, v1

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

I was sure I posted this, but apparently I didn't (it was only saved as a draft).

::: dom/system/gonk/AutoMounter.cpp
@@ +262,5 @@
>          vol->NameStr(), (int)aAllowSharing);
>      UpdateState();
>    }
>  
> +  void SetFormattingMode(const nsACString& aVolumeName)

This should be renamed (since there is no mode argument being passed in).

I'd call if something like:

void FormatVolume(const nsACString& aVolumeName)

@@ +268,5 @@
> +    RefPtr<Volume> vol = VolumeManager::FindVolumeByName(aVolumeName);
> +    if (!vol) {
> +      return;
> +    }
> +    if (vol->IsFormatRequested() == true) {

nit: Don't compare against true or false.

Use if (vol->IsFormatRequested()) {

Ref: https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#C.2FC.2B.2B_practices bullet 5

@@ +610,5 @@
>    sAutoMounter->SetSharingMode(aVolumeName, aAllowSharing);
>  }
>  
>  static void
> +SetAutoMounterFormattingModeIOThread(const nsCString& aVolumeName)

Since this is the IOThread variant of AutoMounterFormatVolume it should be called AutoMounterFormatVolumeIOThread

::: dom/system/gonk/Volume.h
@@ +54,3 @@
>  
>    void SetSharingEnabled(bool aSharingEnabled);
> +  void SetFormatRequest(bool aFormatRequest);

nit: Call this SetFormatRequested

@@ +96,5 @@
>    nsCString         mMountPoint;
>    int32_t           mMountGeneration;
>    bool              mMountLocked;
>    bool              mSharingEnabled;
> +  bool              mFormatRequest;

nit: Call this mFormatRequested;

Comment 25

[Alan Huang]

Attachment: Implement format interface for volume-based device storage, v2

I revised may patch according to naming and coding style nit.

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

Comment 26

[Alan Huang]

(In reply to Dave Hylands [:dhylands] from comment #24)
> I was sure I posted this, but apparently I didn't (it was only saved as a
> draft).
> Ref:
> https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#C.2FC.
> 2B.2B_practices bullet 5

Thank you, Dave. You've helped me a lot here. I won't miss this again in the future  :)

Comment 27

[Dave Hylands [:dhylands]]

Comment on attachment 8335948 [details] [diff] [review]
DeviceStorage-test

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

::: test_apps/ds-test/js/ds-test.js
@@ +585,5 @@
>        case 'available':
>          report(new Available());
>          break;
> +      case 'format':
> +        report(new Format());

The only concern I have about this is that it will format all of the volumes if you have more than one.

Comment 28

[Gina Yeh [:gyeh] [:ginayeh]]

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

Comment 29

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

(In reply to Dave Hylands [:dhylands] from comment #27)
> Comment on attachment 8335948 [details] [diff] [review]
> DeviceStorage-test
> 
> Review of attachment 8335948 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: test_apps/ds-test/js/ds-test.js
> @@ +585,5 @@
> >        case 'available':
> >          report(new Available());
> >          break;
> > +      case 'format':
> > +        report(new Format());
> 
> The only concern I have about this is that it will format all of the volumes
> if you have more than one.
Hi Dave,

In the implementation of settings app side, we'll format the selected storage SD card only(https://github.com/mozilla-b2g/gaia/pull/14120/files#diff-2e669326542ce32522201f56dcf9d0cdR421). For the test case, I'm not sure there is an existed mechanism for the selected storage. Or you have other concern that I did not think. Thanks.

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/955cc9bada56

Comment 31

[Alan Huang]

Attachment: Implement format interface for volume-based device storage. Part2, v1

I realized I missed the child/parent send/receive FileSystemUpdate pairs, and ipc part. This is a follow up patch for this part.

Comment 32

[Alan Huang]

https://tbpl.mozilla.org/?tree=Try&rev=1870739b9af6

Comment 33

[Gina Yeh [:gyeh] [:ginayeh]]

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

Note that Part 1 has been landed on m-c. :)

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/878466c105e7