DeviceStorage returns filenames that only match the filesystem on certain devices. On Flame, which has both internal and external storage, the removal of media files fails. We get a filename such as /sdcard/tests/IMG_0001.jpg but the actual path is /storage/sdcard0/tests/IMG_0001.jpg In bug 1017521 I tried to use the DeviceStorage.delete API to remove only the files found using DeviceStorage. The problem with this is that the call must be from a context that has permission to delete the files. The System app only has readcreate permissions for music, so it was unable to delete these files. Another approach is to translate the path returned from DeviceStorage to the correct path in the file system. We can use |adb shell vdc volume list| to get a list of the volumes. I'm going to put together a patch for b2gpopulate to do this translation.

Comment on attachment 8434035 [details] [diff] [review] Translate the media file path to the actual path in the filesystem. v1.0 Review of attachment 8434035 [details] [diff] [review]: ----------------------------------------------------------------- So after chatting with Dave on irc I'm not entirely sure if this patch is the right way to go, although it might be. f?ing dhylands, who I'm hoping will be able to answer our questions. ::: b2gpopulate/b2gpopulate.py @@ +336,3 @@ > for filename in files: > + # Get the actual location of the file > + parts = filename.strip('/').partition('/') It seems like this will only work for paths of the form /<x>/sdcard/file, which I guess is good enough for the two cases we currently care about (/mnt/sdcard/foo on older devices, /storage/sdcard/foo on the flame) but I worry that there might be devices that have volumes with longer pathnames (e.g. this wouldn't work for /storage/media/card0, /storage/media/card1...). I wonder if there's a better way of processing and using the volume information here? Ultimately we're working with files returned by GaiaData here: https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/gaiatest/gaia_test.py I don't know exactly what format we can expect those to be in. @@ +345,5 @@ > '%s files' % file_type, 0, len(files)) > > + def get_volumes(self): > + vlist = self.device.manager.shellCheckOutput(['vdc', 'volume', 'list']) > + return dict([v.split()[2:4] for v in vlist.splitlines()[:-1]]) So from bug 1018079 it looks like the output here looks like this: 110 sdcard /mnt/sdcard 4 110 extsdcard /mnt/extsdcard 0 And we're using the second field for a key. Are we sure it is always unique? Or might we have multiple volumes of the same type?

The patch takes the paths returned from DeviceStorage, which are currently identical when using unagi/inari/leo/hamachi with files stored on an external sdcard and flame with the internal storage. They are in the format of: /sdcard/foo/bar The actual retrievable location for these however is /mnt/sdcard/foo/bar on unagi/inari/hamachi/leo and /storage/sdcard0/foo/bar on flame (internal storage). I am of the understanding that the initial segment of the path returned by DeviceStorage maps to the second column of the |adb shell vdc volume list| and the third column provides us with the retrievable location for the files. Therefore my assumptions are DeviceStorage returns /<key>/foo/bar where key can be translated using the output of |adb shell vdc volume list| as: 110 <key> <real_path> 4 110 <key> <real_path> 0 Let me know if any of these assumptions are wrong or fragile, and I'm also keen to know if there's a better way to make this translation.

(In reply to William Lachance (:wlach) from comment #2) > Comment on attachment 8434035 [details] [diff] [review] > Translate the media file path to the actual path in the filesystem. v1.0 > > Review of attachment 8434035 [details] [diff] [review]: > ----------------------------------------------------------------- > > So after chatting with Dave on irc I'm not entirely sure if this patch is > the right way to go, although it might be. f?ing dhylands, who I'm hoping > will be able to answer our questions. > > ::: b2gpopulate/b2gpopulate.py > @@ +336,3 @@ > > for filename in files: > > + # Get the actual location of the file > > + parts = filename.strip('/').partition('/') > > It seems like this will only work for paths of the form /<x>/sdcard/file, > which I guess is good enough for the two cases we currently care about > (/mnt/sdcard/foo on older devices, /storage/sdcard/foo on the flame) but I > worry that there might be devices that have volumes with longer pathnames > (e.g. this wouldn't work for /storage/media/card0, /storage/media/card1...). > I wonder if there's a better way of processing and using the volume > information here? For fully-qualified device-stoage filenames, which are of the form /volume-name/relatve-path/filename then parts = filename.strip('/').partition('/') yields ('volume-name', '/', 'relatve-path/filename') and replacing volume-name (parts[0]) with the mount-point volume[parts[0]] and joining it all back up will give a fully-qualified linux pathname. So that looks right to me. I'm assuming volume['volume-name'] = 'mount-point' I'm not quite sure how you arrived at the conclusion: > It seems like this will only work for paths of the form /<x>/sdcard/file, > > Ultimately we're working with files returned by GaiaData here: > https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/ > gaiatest/gaia_test.py > > I don't know exactly what format we can expect those to be in. > > @@ +345,5 @@ > > '%s files' % file_type, 0, len(files)) > > > > + def get_volumes(self): > > + vlist = self.device.manager.shellCheckOutput(['vdc', 'volume', 'list']) > > + return dict([v.split()[2:4] for v in vlist.splitlines()[:-1]]) > > So from bug 1018079 it looks like the output here looks like this: > > 110 sdcard /mnt/sdcard 4 > 110 extsdcard /mnt/extsdcard 0 > > And we're using the second field for a key. Are we sure it is always unique? > Or might we have multiple volumes of the same type? The output of each line is of the form 110 volume-name mount-point state volume-name should be unique. mount-point should also be unique: i.e. there should also only ever be one volume for a given mount-point. Volumes can also originate from the /system/etc/volume.cfg file (on the phone). This is used on phones like the tarako, where the internal volume is shared with the data partition, and thus don't show up in the vdc volume list output.

(In reply to Dave Hunt (:davehunt) from comment #3) > The patch takes the paths returned from DeviceStorage, which are currently > identical when using unagi/inari/leo/hamachi with files stored on an > external sdcard and flame with the internal storage. They are in the format > of: > > /sdcard/foo/bar > > The actual retrievable location for these however is /mnt/sdcard/foo/bar on > unagi/inari/hamachi/leo and /storage/sdcard0/foo/bar on flame (internal > storage). > > I am of the understanding that the initial segment of the path returned by > DeviceStorage maps to the second column of the |adb shell vdc volume list| > and the third column provides us with the retrievable location for the files. Correct > Therefore my assumptions are DeviceStorage returns /<key>/foo/bar where key > can be translated using the output of |adb shell vdc volume list| as: > > 110 <key> <real_path> 4 > 110 <key> <real_path> 0 > > Let me know if any of these assumptions are wrong or fragile, and I'm also > keen to know if there's a better way to make this translation. That's the logic used by the AutoMounter. I'd probably add a parser for /system/etc/volume.cfg, which may add additional volumes/mount points beyond those listed by vdc volume list. IIRC output from vdc volume list trumps volumes included in volume.cfg

Comment on attachment 8434035 [details] [diff] [review] Translate the media file path to the actual path in the filesystem. v1.0 This is certainly a step in the right direction. I think that adding a parser for /system/etc/volume.cfg would make this complete. In Python, I'd parse /system/etc/volume.cfg first and overwrite any duplicates with the results of vdc volume list.

I've been making the assumption that the code in question is for converting device-storage filenames into linux filenames. To go in the reverse direction needs totally different logic. You basically need to scan the volumes, and compare each mount point with a / appended to the filename. If it matches, then you can replace the mount-point followed with /volume-name.

Comment on attachment 8434035 [details] [diff] [review] Translate the media file path to the actual path in the filesystem. v1.0 Review of attachment 8434035 [details] [diff] [review]: ----------------------------------------------------------------- Sounds like this is ok!

Landed in: https://github.com/mozilla/b2gpopulate/commit/c70f1fdf891d0900aa61c9152a1a33ddf0788169 Bumped version: https://github.com/mozilla/b2gpopulate/commit/2a80d9fb6d502bdcc56a52fe98c13de672849296 Released: https://pypi.python.org/pypi/b2gpopulate/0.24 Tagged: https://github.com/mozilla/b2gpopulate/releases/tag/0.24

Will: Do you think it makes sense to enhance mozdevice to offer a volumes property or getter method that returns the dictionary of volume-names to mount-points? We could also add the parsing of /system/etc/volume.cfg as suggested in comment 6.

(In reply to Dave Hunt (:davehunt) from comment #10) > Will: Do you think it makes sense to enhance mozdevice to offer a volumes > property or getter method that returns the dictionary of volume-names to > mount-points? We could also add the parsing of /system/etc/volume.cfg as > suggested in comment 6. Sounds reasonable to me.

Thanks. I raised bug 1020985.