Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2230713003:
Store correct read-only flag of mounted disks. (Closed)
|
Created:
4 years, 4 months ago by yamaguchi Modified:
4 years, 4 months ago CC:
chromium-reviews, oshima+watch_chromium.org, hashimoto+watch_chromium.org, fukino Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionStore read-only flag of mounted disks.
Chrome has been using the readonly field returned by GetDeviceProperties in cros_disks API to tell whether a disk device is wriatble or read-only. However, the field is based on the physical writablilty of the block device, not reflecting the "ro" flag passed to the mount command. This will be an issue once we introduce read-only mount policy.
DiskMountManager needs to remember it because neither the "mount completed" and "disk profile got" DBus messages doesn't convey that information at the moment.
TEST=chromeos_unittests, and manually see that Files app shows no-entry icon when you drag a file to an external storage device from Drive. Apply patch 2248033003 locally to simulate the external_storage_read_only option.
BUG=629945, 640579
Committed: https://crrev.com/6594bf7e1c8d13f7d0ee2648698f11835d1a7733
Cr-Commit-Position: refs/heads/master@{#414169}
Patch Set 1 #
Total comments: 13
Patch Set 2 : Minor changes for local variable name and assertions in test. #Patch Set 3 : Use device path as a key because mount_path is not given to DiskMountManager::MountPath #Patch Set 4 : Sync to head. #
Total comments: 4
Patch Set 5 : Use source path of mount info directly to lookup. Comment update. #
Total comments: 1
Patch Set 6 : Provide updated disks_ to observers. #Patch Set 7 : Add test to make sure observers can see updated disks. #
Total comments: 1
Messages
Total messages: 45 (23 generated)
Description was changed from ========== Add test of read-only parameter to disk mount manager. TEST=unittest BUG=629945 ========== to ========== Store correct read-only flag of mounted disks. Cros_disks API returns whether a drive is physically writable or not, but ignores the "ro" flag passed to the mount command. DiskMountManager needs to remember it because neither the "mount completed" and "disk profile got" DBus messages doesn't convey that information. TEST=unittest BUG=629945 ==========
yamaguchi@chromium.org changed reviewers: + hashimoto@chromium.org, satorux@chromium.org
+hashimoto for dbus/ and the file system logic. +satorux for OWNERS review of disks/* .
Description was changed from ========== Store correct read-only flag of mounted disks. Cros_disks API returns whether a drive is physically writable or not, but ignores the "ro" flag passed to the mount command. DiskMountManager needs to remember it because neither the "mount completed" and "disk profile got" DBus messages doesn't convey that information. TEST=unittest BUG=629945 ========== to ========== Store correct read-only flag of mounted disks. Cros_disks API returns whether a drive is physically writable or not, but ignores the "ro" flag passed to the mount command. DiskMountManager needs to remember it because neither the "mount completed" and "disk profile got" DBus messages doesn't convey that information. TEST=chromeos_unittests BUG=629945 ==========
Did you consider changing the cros-disks behavior instead? That might be a better approach. If there is a reason why it wouldn't work, please document in the patch description. https://codereview.chromium.org/2230713003/diff/1/chromeos/disks/disk_mount_m... File chromeos/disks/disk_mount_manager.cc (right): https://codereview.chromium.org/2230713003/diff/1/chromeos/disks/disk_mount_m... chromeos/disks/disk_mount_manager.cc:94: access_modes_.insert(std::make_pair(mount_label, access_mode)); At this point, mount isn't complete yet. How about doing this inside OnMountCompleted()? You can pass 'access_mode' to OnMountCompleted() function. https://codereview.chromium.org/2230713003/diff/1/chromeos/disks/disk_mount_m... chromeos/disks/disk_mount_manager.cc:363: AccessModeMap::iterator it = access_modes_.find(disk->mount_path()); access_modes_'s keys are mount labels, but here, it's accessed with mount_path(). Are they the same thing? Would be nice to add some comment? https://codereview.chromium.org/2230713003/diff/1/chromeos/disks/disk_mount_m... chromeos/disks/disk_mount_manager.cc:367: } If 'access_mode' is passed to OnMountCompleted(), code around this can be as simple as disk->set_read_only(access_mode == chromeos::MOUNT_ACCESS_MODE_READ_ONLY) https://codereview.chromium.org/2230713003/diff/1/chromeos/disks/disk_mount_m... chromeos/disks/disk_mount_manager.cc:485: AccessModeMap::iterator mount_option = mount_option -> access_mode? https://codereview.chromium.org/2230713003/diff/1/chromeos/disks/disk_mount_m... chromeos/disks/disk_mount_manager.cc:490: } another approach: instead of introducing access_modes_, how about relying on the previous disk info, that's deleted on the line 460? https://codereview.chromium.org/2230713003/diff/1/chromeos/disks/disk_mount_m... File chromeos/disks/disk_mount_manager_unittest.cc (right): https://codereview.chromium.org/2230713003/diff/1/chromeos/disks/disk_mount_m... chromeos/disks/disk_mount_manager_unittest.cc:626: ASSERT_FALSE(disks.find(kSourcePath1) == disks.end()); how about: ASSERT_GT(disks.count(kSourcePath1), 0) https://codereview.chromium.org/2230713003/diff/1/chromeos/disks/disk_mount_m... chromeos/disks/disk_mount_manager_unittest.cc:628: ASSERT_FALSE(disks.find(kSourcePath2) == disks.end()); ditto
I'm worried about this CL because the access mode map code is only needed by the test, but it adds runtime cost to the product. Just remembering the access mode value in FakeCrosDisksClient::Mount() isn't enough? (which tests that DiskMountManager passes the access mode value to CrosDisksClient appropriately)
On 2016/08/15 06:45:12, hashimoto wrote: > I'm worried about this CL because the access mode map code is only needed by the > test, but it adds runtime cost to the product. > > Just remembering the access mode value in FakeCrosDisksClient::Mount() isn't > enough? (which tests that DiskMountManager passes the access mode value to > CrosDisksClient appropriately) I'm sorry for confusing you. The summary says "Add test of read-only parameter to disk mount manager." as it was the first and wrong comment when I uploaded the CL, but I updated the description after that. This change actually adds a new logic to handle read-only state of the device correctly after https://codereview.chromium.org/2201023002/ .
On 2016/08/15 08:01:45, yamaguchi wrote: > On 2016/08/15 06:45:12, hashimoto wrote: > > I'm worried about this CL because the access mode map code is only needed by > the > > test, but it adds runtime cost to the product. > > > > Just remembering the access mode value in FakeCrosDisksClient::Mount() isn't > > enough? (which tests that DiskMountManager passes the access mode value to > > CrosDisksClient appropriately) > > I'm sorry for confusing you. The summary says "Add test of read-only parameter > to disk mount manager." as it was the first and wrong comment when I uploaded > the CL, but I updated the description after that. > This change actually adds a new logic to handle read-only state of the device > correctly after https://codereview.chromium.org/2201023002/ . Thanks for the explanation, then Satoru's suggestion sounds better.
Description was changed from ========== Store correct read-only flag of mounted disks. Cros_disks API returns whether a drive is physically writable or not, but ignores the "ro" flag passed to the mount command. DiskMountManager needs to remember it because neither the "mount completed" and "disk profile got" DBus messages doesn't convey that information. TEST=chromeos_unittests BUG=629945 ========== to ========== Store correct read-only flag of mounted disks. Cros_disks API returns whether a drive is physically writable or not, but ignores the "ro" flag passed to the mount command. DiskMountManager needs to remember it because neither the "mount completed" and "disk profile got" DBus messages doesn't convey that information. The logic can be removed after modifying the cros_disks service by adding fields to the messages "MountEntry" (passed by OnMountCompleted) and "DiskInfo" (passed from GetDeviceProperties). TEST=chromeos_unittests BUG=629945 ==========
I agree that the best is to modify the API (cros_disks). I investigated and it'd be doable, if we revise the both messages as I wrote in the description. I will start working on that. Should we cancel this (intermediate) change? https://codereview.chromium.org/2230713003/diff/1/chromeos/disks/disk_mount_m... File chromeos/disks/disk_mount_manager.cc (right): https://codereview.chromium.org/2230713003/diff/1/chromeos/disks/disk_mount_m... chromeos/disks/disk_mount_manager.cc:94: access_modes_.insert(std::make_pair(mount_label, access_mode)); On 2016/08/12 02:47:51, satorux1 wrote: > At this point, mount isn't complete yet. How about doing this inside > OnMountCompleted()? You can pass 'access_mode' to OnMountCompleted() function. OnMountCompleted is invoked by the signal from DBus. So it will be doable when we review the dbus message. https://codereview.chromium.org/2230713003/diff/1/chromeos/disks/disk_mount_m... chromeos/disks/disk_mount_manager.cc:363: AccessModeMap::iterator it = access_modes_.find(disk->mount_path()); On 2016/08/12 02:47:51, satorux1 wrote: > access_modes_'s keys are mount labels, but here, it's accessed with > mount_path(). Are they the same thing? Would be nice to add some comment? Good catch. We need to use device path instead of mount_path, because mount_path is not given when we insert it to the dict. https://codereview.chromium.org/2230713003/diff/1/chromeos/disks/disk_mount_m... chromeos/disks/disk_mount_manager.cc:485: AccessModeMap::iterator mount_option = On 2016/08/12 02:47:52, satorux1 wrote: > mount_option -> access_mode? Done. https://codereview.chromium.org/2230713003/diff/1/chromeos/disks/disk_mount_m... chromeos/disks/disk_mount_manager.cc:490: } On 2016/08/12 02:47:52, satorux1 wrote: > another approach: instead of introducing access_modes_, how about relying on the > previous disk info, that's deleted on the line 460? The previous disk will be available only when this is called from the call path of SuspendUnmountManager::SuspendDone, but not when it come from OnMountEvent (=after mounting a new device). https://codereview.chromium.org/2230713003/diff/1/chromeos/disks/disk_mount_m... File chromeos/disks/disk_mount_manager_unittest.cc (right): https://codereview.chromium.org/2230713003/diff/1/chromeos/disks/disk_mount_m... chromeos/disks/disk_mount_manager_unittest.cc:626: ASSERT_FALSE(disks.find(kSourcePath1) == disks.end()); On 2016/08/12 02:47:52, satorux1 wrote: > how about: > > ASSERT_GT(disks.count(kSourcePath1), 0) Done. https://codereview.chromium.org/2230713003/diff/1/chromeos/disks/disk_mount_m... chromeos/disks/disk_mount_manager_unittest.cc:628: ASSERT_FALSE(disks.find(kSourcePath2) == disks.end()); On 2016/08/12 02:47:52, satorux1 wrote: > ditto Done.
Description was changed from ========== Store correct read-only flag of mounted disks. Cros_disks API returns whether a drive is physically writable or not, but ignores the "ro" flag passed to the mount command. DiskMountManager needs to remember it because neither the "mount completed" and "disk profile got" DBus messages doesn't convey that information. The logic can be removed after modifying the cros_disks service by adding fields to the messages "MountEntry" (passed by OnMountCompleted) and "DiskInfo" (passed from GetDeviceProperties). TEST=chromeos_unittests BUG=629945 ========== to ========== Store correct read-only flag of mounted disks. Cros_disks API returns whether a drive is physically writable or not, but ignores the "ro" flag passed to the mount command. DiskMountManager needs to remember it because neither the "mount completed" and "disk profile got" DBus messages doesn't convey that information. The logic can be removed after modifying the cros_disks service by adding fields to the messages "MountEntry" (passed by OnMountCompleted) and "DiskInfo" (passed from GetDeviceProperties). TEST=chromeos_unittests BUG=629945 ==========
On 2016/08/15 15:32:48, yamaguchi wrote: > I agree that the best is to modify the API (cros_disks). > I investigated and it'd be doable, if we revise the both messages as I wrote in > the description. > I will start working on that. Should we cancel this (intermediate) change? Let's put this on hold for now. Please try to change the cros_disks API first. > > https://codereview.chromium.org/2230713003/diff/1/chromeos/disks/disk_mount_m... > File chromeos/disks/disk_mount_manager.cc (right): > > https://codereview.chromium.org/2230713003/diff/1/chromeos/disks/disk_mount_m... > chromeos/disks/disk_mount_manager.cc:94: > access_modes_.insert(std::make_pair(mount_label, access_mode)); > On 2016/08/12 02:47:51, satorux1 wrote: > > At this point, mount isn't complete yet. How about doing this inside > > OnMountCompleted()? You can pass 'access_mode' to OnMountCompleted() function. > > OnMountCompleted is invoked by the signal from DBus. So it will be doable when > we review the dbus message. > > https://codereview.chromium.org/2230713003/diff/1/chromeos/disks/disk_mount_m... > chromeos/disks/disk_mount_manager.cc:363: AccessModeMap::iterator it = > access_modes_.find(disk->mount_path()); > On 2016/08/12 02:47:51, satorux1 wrote: > > access_modes_'s keys are mount labels, but here, it's accessed with > > mount_path(). Are they the same thing? Would be nice to add some comment? > > Good catch. We need to use device path instead of mount_path, because mount_path > is not given when we insert it to the dict. > > https://codereview.chromium.org/2230713003/diff/1/chromeos/disks/disk_mount_m... > chromeos/disks/disk_mount_manager.cc:485: AccessModeMap::iterator mount_option = > On 2016/08/12 02:47:52, satorux1 wrote: > > mount_option -> access_mode? > > Done. > > https://codereview.chromium.org/2230713003/diff/1/chromeos/disks/disk_mount_m... > chromeos/disks/disk_mount_manager.cc:490: } > On 2016/08/12 02:47:52, satorux1 wrote: > > another approach: instead of introducing access_modes_, how about relying on > the > > previous disk info, that's deleted on the line 460? > > The previous disk will be available only when this is called from the call path > of SuspendUnmountManager::SuspendDone, but not when it come from OnMountEvent > (=after mounting a new device). > > https://codereview.chromium.org/2230713003/diff/1/chromeos/disks/disk_mount_m... > File chromeos/disks/disk_mount_manager_unittest.cc (right): > > https://codereview.chromium.org/2230713003/diff/1/chromeos/disks/disk_mount_m... > chromeos/disks/disk_mount_manager_unittest.cc:626: > ASSERT_FALSE(disks.find(kSourcePath1) == disks.end()); > On 2016/08/12 02:47:52, satorux1 wrote: > > how about: > > > > ASSERT_GT(disks.count(kSourcePath1), 0) > > Done. > > https://codereview.chromium.org/2230713003/diff/1/chromeos/disks/disk_mount_m... > chromeos/disks/disk_mount_manager_unittest.cc:628: > ASSERT_FALSE(disks.find(kSourcePath2) == disks.end()); > On 2016/08/12 02:47:52, satorux1 wrote: > > ditto > > Done.
Description was changed from ========== Store correct read-only flag of mounted disks. Cros_disks API returns whether a drive is physically writable or not, but ignores the "ro" flag passed to the mount command. DiskMountManager needs to remember it because neither the "mount completed" and "disk profile got" DBus messages doesn't convey that information. The logic can be removed after modifying the cros_disks service by adding fields to the messages "MountEntry" (passed by OnMountCompleted) and "DiskInfo" (passed from GetDeviceProperties). TEST=chromeos_unittests BUG=629945 ========== to ========== Store read-only flag of mounted disks. Cros_disks API returns whether a drive is physically writable or not, but ignores the "ro" flag passed to the mount command. DiskMountManager needs to remember it because neither the "mount completed" and "disk profile got" DBus messages doesn't convey that information. The logic can be removed after modifying the cros_disks service to return mount options by the arguments of MountCompleted signal and the return value of EnumerateMountEntries. TEST=chromeos_unittests BUG=629945 ==========
The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Store read-only flag of mounted disks. Cros_disks API returns whether a drive is physically writable or not, but ignores the "ro" flag passed to the mount command. DiskMountManager needs to remember it because neither the "mount completed" and "disk profile got" DBus messages doesn't convey that information. The logic can be removed after modifying the cros_disks service to return mount options by the arguments of MountCompleted signal and the return value of EnumerateMountEntries. TEST=chromeos_unittests BUG=629945 ========== to ========== Store read-only flag of mounted disks. Chrome has been using the readonly flag returned by GetDeviceProperties in cros_disks API. It is based on the physical writablilty of the device, not reflecting the "ro" flag passed to the mount command. This will be an issue once we introduce read-only mount policy. DiskMountManager needs to remember it because neither the "mount completed" and "disk profile got" DBus messages doesn't convey that information at the moment. TEST=chromeos_unittests BUG=629945 ==========
I would like to check in this fix first. Synced to head. PTAL. We are discussing how/if cros_disks should return mount options, but it's not yet finalized. I think this change would be a reasonable tentative fix, and we can update the internal structure (whether to do this on Chrome or cros_disks) later as needed.
Description was changed from ========== Store read-only flag of mounted disks. Chrome has been using the readonly flag returned by GetDeviceProperties in cros_disks API. It is based on the physical writablilty of the device, not reflecting the "ro" flag passed to the mount command. This will be an issue once we introduce read-only mount policy. DiskMountManager needs to remember it because neither the "mount completed" and "disk profile got" DBus messages doesn't convey that information at the moment. TEST=chromeos_unittests BUG=629945 ========== to ========== Store read-only flag of mounted disks. Chrome has been using the readonly field returned by GetDeviceProperties in cros_disks API to tell whether a disk device is wriatble or read-only. However, the field is based on the physical writablilty of the block device, not reflecting the "ro" flag passed to the mount command. This will be an issue once we introduce read-only mount policy. DiskMountManager needs to remember it because neither the "mount completed" and "disk profile got" DBus messages doesn't convey that information at the moment. TEST=chromeos_unittests BUG=629945 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Please also update TEST= line and describe how you can test this manually in Files app UI https://codereview.chromium.org/2230713003/diff/60001/chromeos/disks/disk_mou... File chromeos/disks/disk_mount_manager.cc (right): https://codereview.chromium.org/2230713003/diff/60001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager.cc:369: AccessModeMap::iterator it = access_modes_.find(disk->device_path()); source_path passed to Mount() and disk->device_path() here are compatible, right? Please add a comment here or elsewhere. https://codereview.chromium.org/2230713003/diff/60001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager.cc:491: AccessModeMap::iterator access_mode = maybe just call it |iter|?
Description was changed from ========== Store read-only flag of mounted disks. Chrome has been using the readonly field returned by GetDeviceProperties in cros_disks API to tell whether a disk device is wriatble or read-only. However, the field is based on the physical writablilty of the block device, not reflecting the "ro" flag passed to the mount command. This will be an issue once we introduce read-only mount policy. DiskMountManager needs to remember it because neither the "mount completed" and "disk profile got" DBus messages doesn't convey that information at the moment. TEST=chromeos_unittests BUG=629945 ========== to ========== Store read-only flag of mounted disks. Chrome has been using the readonly field returned by GetDeviceProperties in cros_disks API to tell whether a disk device is wriatble or read-only. However, the field is based on the physical writablilty of the block device, not reflecting the "ro" flag passed to the mount command. This will be an issue once we introduce read-only mount policy. DiskMountManager needs to remember it because neither the "mount completed" and "disk profile got" DBus messages doesn't convey that information at the moment. TEST=chromeos_unittests, and manually see if Files app rejects to copy a file to external storage device. Apply patch 2248033003 locally to simulate the external_storage_read_only option. BUG=629945 ==========
Description was changed from ========== Store read-only flag of mounted disks. Chrome has been using the readonly field returned by GetDeviceProperties in cros_disks API to tell whether a disk device is wriatble or read-only. However, the field is based on the physical writablilty of the block device, not reflecting the "ro" flag passed to the mount command. This will be an issue once we introduce read-only mount policy. DiskMountManager needs to remember it because neither the "mount completed" and "disk profile got" DBus messages doesn't convey that information at the moment. TEST=chromeos_unittests, and manually see if Files app rejects to copy a file to external storage device. Apply patch 2248033003 locally to simulate the external_storage_read_only option. BUG=629945 ========== to ========== Store read-only flag of mounted disks. Chrome has been using the readonly field returned by GetDeviceProperties in cros_disks API to tell whether a disk device is wriatble or read-only. However, the field is based on the physical writablilty of the block device, not reflecting the "ro" flag passed to the mount command. This will be an issue once we introduce read-only mount policy. DiskMountManager needs to remember it because neither the "mount completed" and "disk profile got" DBus messages doesn't convey that information at the moment. TEST=chromeos_unittests, and manually see that Files app shows no-entry icon when you drag a file to an external storage device from Drive. Apply patch 2248033003 locally to simulate the external_storage_read_only option. BUG=629945 ==========
https://codereview.chromium.org/2230713003/diff/60001/chromeos/disks/disk_mou... File chromeos/disks/disk_mount_manager.cc (right): https://codereview.chromium.org/2230713003/diff/60001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager.cc:369: AccessModeMap::iterator it = access_modes_.find(disk->device_path()); On 2016/08/23 05:40:49, satorux1 wrote: > source_path passed to Mount() and disk->device_path() here are compatible, > right? Please add a comment here or elsewhere. Added comments to the 2 places looking up the map. Also changed it to use the mount source path passed from the parameter, instead of the one in |disk|. (disks is keyed by disk.device_path, and the function does lookup by a copy of entry.source_path(). so the two values are always the same) https://codereview.chromium.org/2230713003/diff/60001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager.cc:491: AccessModeMap::iterator access_mode = On 2016/08/23 05:40:49, satorux1 wrote: > maybe just call it |iter|? No, because it collides with the one for DiskMap::iterator in the same function.
I found this change sometimes don't work as intended. 1. Right after plugging a device, Files app doesn't always recognize it as read-only. (note that writing attempt to disks still fail due to the mount option.) 2. However it's recognized as read-only after resuming from suspend. Regarding case #1, so far I confirmed that access_modes_ is looked up and found the disk and the read_only field is overwritten. So the cause of the bug should be in other place. (If we were to commit the change with the current form, I fill file this as a crbug.)
On 2016/08/24 00:41:26, yamaguchi wrote: > I found this change sometimes don't work as intended. > 1. Right after plugging a device, Files app doesn't always recognize it as > read-only. > (note that writing attempt to disks still fail due to the mount option.) > 2. However it's recognized as read-only after resuming from suspend. > > Regarding case #1, so far I confirmed that access_modes_ is looked up and found > the disk and the read_only field is overwritten. So the cause of the bug should > be in other place. > (If we were to commit the change with the current form, I fill file this as a > crbug.) Re 1), how long does it take for Files app to recognize that the disk volume is read-only?
From what I could see so far, it's looks like not reflected to UI until some events happen. Perhaps proper updates are not triggered after OnMountComplete with current logic. I will examine the callback chain of the path. 2016-08-24 10:36 GMT+09:00 <satorux@chromium.org>: > On 2016/08/24 00:41:26, yamaguchi wrote: > > I found this change sometimes don't work as intended. > > 1. Right after plugging a device, Files app doesn't always recognize it > as > > read-only. > > (note that writing attempt to disks still fail due to the mount option.) > > 2. However it's recognized as read-only after resuming from suspend. > > > > Regarding case #1, so far I confirmed that access_modes_ is looked up and > found > > the disk and the read_only field is overwritten. So the cause of the bug > should > > be in other place. > > (If we were to commit the change with the current form, I fill file this > as a > > crbug.) > > Re 1), how long does it take for Files app to recognize that the disk > volume is > read-only? > > https://codereview.chromium.org/2230713003/ > -- ; Tatsuhisa Yamaguchi ; yamaguchi@google.com -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/2230713003/diff/80001/chromeos/disks/disk_mou... File chromeos/disks/disk_mount_manager.cc (right): https://codereview.chromium.org/2230713003/diff/80001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager.cc:343: NotifyMountStatusUpdate(MOUNTING, entry.error_code(), mount_info); This will invoke observer functions. Especially, StorageMonitorCros will call back to this disk_mount_manager and reads values of disks_. So we need to update disks_ before this. https://cs.chromium.org/chromium/src/components/storage_monitor/storage_monit...
On 2016/08/24 06:46:28, yamaguchi wrote: > https://codereview.chromium.org/2230713003/diff/80001/chromeos/disks/disk_mou... > File chromeos/disks/disk_mount_manager.cc (right): > > https://codereview.chromium.org/2230713003/diff/80001/chromeos/disks/disk_mou... > chromeos/disks/disk_mount_manager.cc:343: NotifyMountStatusUpdate(MOUNTING, > entry.error_code(), mount_info); > This will invoke observer functions. Especially, StorageMonitorCros will call > back to this disk_mount_manager and reads values of disks_. So we need to update > disks_ before this. > https://cs.chromium.org/chromium/src/components/storage_monitor/storage_monit... Done.
i'm running out of time today. hashimoto@ could you take a look?
yamaguchi@chromium.org changed reviewers: + kinaba@chromium.org
+kinaba because hashimoto is OOO.
Updated test to cover the change. ptal.
The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Store read-only flag of mounted disks. Chrome has been using the readonly field returned by GetDeviceProperties in cros_disks API to tell whether a disk device is wriatble or read-only. However, the field is based on the physical writablilty of the block device, not reflecting the "ro" flag passed to the mount command. This will be an issue once we introduce read-only mount policy. DiskMountManager needs to remember it because neither the "mount completed" and "disk profile got" DBus messages doesn't convey that information at the moment. TEST=chromeos_unittests, and manually see that Files app shows no-entry icon when you drag a file to an external storage device from Drive. Apply patch 2248033003 locally to simulate the external_storage_read_only option. BUG=629945 ========== to ========== Store read-only flag of mounted disks. Chrome has been using the readonly field returned by GetDeviceProperties in cros_disks API to tell whether a disk device is wriatble or read-only. However, the field is based on the physical writablilty of the block device, not reflecting the "ro" flag passed to the mount command. This will be an issue once we introduce read-only mount policy. DiskMountManager needs to remember it because neither the "mount completed" and "disk profile got" DBus messages doesn't convey that information at the moment. TEST=chromeos_unittests, and manually see that Files app shows no-entry icon when you drag a file to an external storage device from Drive. Apply patch 2248033003 locally to simulate the external_storage_read_only option. BUG=629945,640579 ==========
LGTM https://codereview.chromium.org/2230713003/diff/120001/chromeos/disks/disk_mo... File chromeos/disks/disk_mount_manager_unittest.cc (right): https://codereview.chromium.org/2230713003/diff/120001/chromeos/disks/disk_mo... chromeos/disks/disk_mount_manager_unittest.cc:676: [&]() { ExpectDiskReadOnly(manager, kSourcePath2, true); })); hmm, this is hard to read... Please add some comment here what this code does. I'd like use of GMock removed from this file but it'd be a separate task.
The CQ bit was checked by satorux@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Store read-only flag of mounted disks. Chrome has been using the readonly field returned by GetDeviceProperties in cros_disks API to tell whether a disk device is wriatble or read-only. However, the field is based on the physical writablilty of the block device, not reflecting the "ro" flag passed to the mount command. This will be an issue once we introduce read-only mount policy. DiskMountManager needs to remember it because neither the "mount completed" and "disk profile got" DBus messages doesn't convey that information at the moment. TEST=chromeos_unittests, and manually see that Files app shows no-entry icon when you drag a file to an external storage device from Drive. Apply patch 2248033003 locally to simulate the external_storage_read_only option. BUG=629945,640579 ========== to ========== Store read-only flag of mounted disks. Chrome has been using the readonly field returned by GetDeviceProperties in cros_disks API to tell whether a disk device is wriatble or read-only. However, the field is based on the physical writablilty of the block device, not reflecting the "ro" flag passed to the mount command. This will be an issue once we introduce read-only mount policy. DiskMountManager needs to remember it because neither the "mount completed" and "disk profile got" DBus messages doesn't convey that information at the moment. TEST=chromeos_unittests, and manually see that Files app shows no-entry icon when you drag a file to an external storage device from Drive. Apply patch 2248033003 locally to simulate the external_storage_read_only option. BUG=629945,640579 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Store read-only flag of mounted disks. Chrome has been using the readonly field returned by GetDeviceProperties in cros_disks API to tell whether a disk device is wriatble or read-only. However, the field is based on the physical writablilty of the block device, not reflecting the "ro" flag passed to the mount command. This will be an issue once we introduce read-only mount policy. DiskMountManager needs to remember it because neither the "mount completed" and "disk profile got" DBus messages doesn't convey that information at the moment. TEST=chromeos_unittests, and manually see that Files app shows no-entry icon when you drag a file to an external storage device from Drive. Apply patch 2248033003 locally to simulate the external_storage_read_only option. BUG=629945,640579 ========== to ========== Store read-only flag of mounted disks. Chrome has been using the readonly field returned by GetDeviceProperties in cros_disks API to tell whether a disk device is wriatble or read-only. However, the field is based on the physical writablilty of the block device, not reflecting the "ro" flag passed to the mount command. This will be an issue once we introduce read-only mount policy. DiskMountManager needs to remember it because neither the "mount completed" and "disk profile got" DBus messages doesn't convey that information at the moment. TEST=chromeos_unittests, and manually see that Files app shows no-entry icon when you drag a file to an external storage device from Drive. Apply patch 2248033003 locally to simulate the external_storage_read_only option. BUG=629945,640579 Committed: https://crrev.com/6594bf7e1c8d13f7d0ee2648698f11835d1a7733 Cr-Commit-Position: refs/heads/master@{#414169} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/6594bf7e1c8d13f7d0ee2648698f11835d1a7733 Cr-Commit-Position: refs/heads/master@{#414169} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
