Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2451603002:
Add a method to remount all removable devices in DiskMountManager. (Closed)
|
Created:
4 years, 1 month ago by yamaguchi Modified:
4 years, 1 month ago CC:
chromium-reviews, yamaguchi+watch_chromium.org, oka+watch_chromium.org, rginda+watch_chromium.org, oshima+watch_chromium.org, fukino+watch_chromium.org, davemoore+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd methods to remount removable devices in DiskMountManager.
The methods will be invoked from VolumeManager to flip the read-only mount option upon ExternalStorageReadOnly policy update.
Also allow to flip the read-only status of a Disk object from read-only to read-write, because it can happen when remounting a disk.
BUG=642247
TEST=chromeos_unittests --gtest_filter=DiskMountManagerTest.*
Committed: https://crrev.com/de59ed6bed57372a9c00a7c65554abd47471f225
Cr-Commit-Position: refs/heads/master@{#430564}
Patch Set 1 #Patch Set 2 : Do not pass mount path as mount_label because it causes test failure on some platforms. #
Total comments: 22
Patch Set 3 : Do not lookup dict of disks multiple times. #Patch Set 4 : Add function comment. #Patch Set 5 : Update comment. #
Total comments: 8
Patch Set 6 : Update write_diabled_by_policy field whenever mount is succeded or device is fake mounted. #Patch Set 7 : Fix comment. #
Total comments: 6
Patch Set 8 : Add is_mounted() method. #
Messages
Total messages: 36 (21 generated)
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 ========== Add a method to remount all removable devices in DiskMountManager. BUG=642247 TEST=chromeos_unittests --gtest_filter=DiskMountManagerTest.*, unit_tests --gtest_filter=VolumeManagerTest.* ========== to ========== Add a method to remount all removable devices in DiskMountManager. BUG=642247 TEST=chromeos_unittests --gtest_filter=DiskMountManagerTest.* ==========
Description was changed from ========== Add a method to remount all removable devices in DiskMountManager. BUG=642247 TEST=chromeos_unittests --gtest_filter=DiskMountManagerTest.* ========== to ========== Add methods to remount removable devices in DiskMountManager. BUG=642247 TEST=chromeos_unittests --gtest_filter=DiskMountManagerTest.* ==========
Description was changed from ========== Add methods to remount removable devices in DiskMountManager. BUG=642247 TEST=chromeos_unittests --gtest_filter=DiskMountManagerTest.* ========== to ========== Add methods to remount removable devices in DiskMountManager. The methods will be invoked from VolumeManager to flip the read-only flag upon policy update. BUG=642247 TEST=chromeos_unittests --gtest_filter=DiskMountManagerTest.* ==========
Description was changed from ========== Add methods to remount removable devices in DiskMountManager. The methods will be invoked from VolumeManager to flip the read-only flag upon policy update. BUG=642247 TEST=chromeos_unittests --gtest_filter=DiskMountManagerTest.* ========== to ========== Add methods to remount removable devices in DiskMountManager. The methods will be invoked from VolumeManager to flip the read-only mount option upon ExternalStorageReadOnly policy update. BUG=642247 TEST=chromeos_unittests --gtest_filter=DiskMountManagerTest.* ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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...
yamaguchi@chromium.org changed reviewers: + hashimoto@chromium.org
ptal
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2451603002/diff/20001/chromeos/disks/disk_mou... File chromeos/disks/disk_mount_manager.cc (right): https://codereview.chromium.org/2451603002/diff/20001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager.cc:117: for (auto it = disks_.begin(); it != disks_.end(); ++it) { How about using range-based for? for (const auto& disk : disks_) https://codereview.chromium.org/2451603002/diff/20001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager.cc:122: void RemountRemovableDrive(const std::string& mount_path, This method should be private. https://codereview.chromium.org/2451603002/diff/20001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager.cc:126: LOG(ERROR) << "Mount point with path \"" << mount_path << "\" not found."; Don't we need to call OnMountCompleted with an error code on failures? https://codereview.chromium.org/2451603002/diff/20001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager.cc:130: std::string device_path = mount_point->second.source_path; const std::string&? https://codereview.chromium.org/2451603002/diff/20001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager.cc:131: DiskMap::const_iterator disk = disks_.find(device_path); In RemountAllRemovableDrives(), we already know which Disk object corresponds to the mount point. Why do we need to find the disk again? Also, the read-only check below shouldn't be done earlier with the Disk object? https://codereview.chromium.org/2451603002/diff/20001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager.cc:147: access_modes_[source_path] = access_mode; I'm not familiar with this access mode management code so please also ask satorux@ to review this part. Isn't there a plan to get this info via D-Bus? I'm not sure how robust the current implementation is against unexpected events like Chrome crash which can result in inconsistency between the actual mount state and Chrome's internal state. https://codereview.chromium.org/2451603002/diff/20001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager.cc:407: // Lookup the mount option passed when invoking Mount() by source path. I couldn't understand what this comment means. What does "the mount option passed" mean? What does "when invoking Mount() by source path" mean? https://codereview.chromium.org/2451603002/diff/20001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager.cc:412: if (it != access_modes_.end() && !disk->is_read_only_hardware()) { It's not clear to me what this change is doing (and why this change is needed). Could you put a number of words in the change description to make it clear? https://codereview.chromium.org/2451603002/diff/20001/chromeos/disks/disk_mou... File chromeos/disks/disk_mount_manager.h (right): https://codereview.chromium.org/2451603002/diff/20001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager.h:297: virtual void RemountAllRemovableDrives(chromeos::MountAccessMode mode) = 0; Please add a comment to describe what this method does.
Description was changed from ========== Add methods to remount removable devices in DiskMountManager. The methods will be invoked from VolumeManager to flip the read-only mount option upon ExternalStorageReadOnly policy update. BUG=642247 TEST=chromeos_unittests --gtest_filter=DiskMountManagerTest.* ========== to ========== Add methods to remount removable devices in DiskMountManager. The methods will be invoked from VolumeManager to flip the read-only mount option upon ExternalStorageReadOnly policy update. Also allow to flip the read-only status of a Disk object from read-only to read-write, because it can happen when remounting a disk. BUG=642247 TEST=chromeos_unittests --gtest_filter=DiskMountManagerTest.* ==========
https://codereview.chromium.org/2451603002/diff/20001/chromeos/disks/disk_mou... File chromeos/disks/disk_mount_manager.cc (right): https://codereview.chromium.org/2451603002/diff/20001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager.cc:117: for (auto it = disks_.begin(); it != disks_.end(); ++it) { On 2016/10/26 06:05:12, hashimoto wrote: > How about using range-based for? > for (const auto& disk : disks_) Done. https://codereview.chromium.org/2451603002/diff/20001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager.cc:122: void RemountRemovableDrive(const std::string& mount_path, On 2016/10/26 06:05:12, hashimoto wrote: > This method should be private. Done. https://codereview.chromium.org/2451603002/diff/20001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager.cc:126: LOG(ERROR) << "Mount point with path \"" << mount_path << "\" not found."; On 2016/10/26 06:05:12, hashimoto wrote: > Don't we need to call OnMountCompleted with an error code on failures? Done. https://codereview.chromium.org/2451603002/diff/20001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager.cc:130: std::string device_path = mount_point->second.source_path; On 2016/10/26 06:05:12, hashimoto wrote: > const std::string&? Done. https://codereview.chromium.org/2451603002/diff/20001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager.cc:131: DiskMap::const_iterator disk = disks_.find(device_path); On 2016/10/26 06:05:12, hashimoto wrote: > In RemountAllRemovableDrives(), we already know which Disk object corresponds to > the mount point. > Why do we need to find the disk again? > > Also, the read-only check below shouldn't be done earlier with the Disk object? That's right. Fixed. https://codereview.chromium.org/2451603002/diff/20001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager.cc:147: access_modes_[source_path] = access_mode; On 2016/10/26 06:05:12, hashimoto wrote: > I'm not familiar with this access mode management code so please also ask > satorux@ to review this part. > > Isn't there a plan to get this info via D-Bus? > I'm not sure how robust the current implementation is against unexpected events > like Chrome crash which can result in inconsistency between the actual mount > state and Chrome's internal state. Yes, storing to access_modes_ here is a tentative solution. We haven't surfaced the one in cros_disks as DBus API yet. https://chromium-review.googlesource.com/#/c/373320/ https://codereview.chromium.org/2451603002/diff/20001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager.cc:407: // Lookup the mount option passed when invoking Mount() by source path. On 2016/10/26 06:05:12, hashimoto wrote: > I couldn't understand what this comment means. > What does "the mount option passed" mean? > What does "when invoking Mount() by source path" mean? Revised the comments. Hope it makes more sense now. https://codereview.chromium.org/2451603002/diff/20001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager.cc:412: if (it != access_modes_.end() && !disk->is_read_only_hardware()) { On 2016/10/26 06:05:12, hashimoto wrote: > It's not clear to me what this change is doing (and why this change is needed). > Could you put a number of words in the change description to make it clear? This mechanism to update the Disk object is revised because the older logic assumed that disk object is generated by addition of device and then updated only once for mounting (and also historically, because we didn't have separate fields for RO-by-policy and RO-by-hardware). Added a brief comment about this in the change description. https://codereview.chromium.org/2451603002/diff/20001/chromeos/disks/disk_mou... File chromeos/disks/disk_mount_manager.h (right): https://codereview.chromium.org/2451603002/diff/20001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager.h:297: virtual void RemountAllRemovableDrives(chromeos::MountAccessMode mode) = 0; On 2016/10/26 06:05:12, hashimoto wrote: > Please add a comment to describe what this method does. Done. https://codereview.chromium.org/2451603002/diff/20001/chromeos/disks/disk_mou... File chromeos/disks/disk_mount_manager_unittest.cc (right): https://codereview.chromium.org/2451603002/diff/20001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager_unittest.cc:96: kDevice2MountPath, This should be an empty string, because there is no mount point corresponding to this disk. The older code was emulating a situation where the disk_ and mount_points_ were inconsistent, which wouldn't have been what we meant to. This change will not affect to the other tests.
https://codereview.chromium.org/2451603002/diff/20001/chromeos/disks/disk_mou... File chromeos/disks/disk_mount_manager.cc (right): https://codereview.chromium.org/2451603002/diff/20001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager.cc:147: access_modes_[source_path] = access_mode; On 2016/10/27 06:34:55, yamaguchi wrote: > On 2016/10/26 06:05:12, hashimoto wrote: > > I'm not familiar with this access mode management code so please also ask > > satorux@ to review this part. > > > > Isn't there a plan to get this info via D-Bus? > > I'm not sure how robust the current implementation is against unexpected > events > > like Chrome crash which can result in inconsistency between the actual mount > > state and Chrome's internal state. > > Yes, storing to access_modes_ here is a tentative solution. > > We haven't surfaced the one in cros_disks as DBus API yet. > https://chromium-review.googlesource.com/#/c/373320/ Seems the said patch was already submitted. What else needs to be done? I'm not familiar with this access_modes_ management code, but it looks quite fragile to me. For example, what happens when RemountAllRemovableDrives() gets called twice with different access mode values, quickly enough? For the first OnMountCompleted call, the Disk object should reflect the first access mode, but IIUC it mistakenly reflects the second access mode. https://codereview.chromium.org/2451603002/diff/80001/chromeos/disks/disk_mou... File chromeos/disks/disk_mount_manager.cc (right): https://codereview.chromium.org/2451603002/diff/80001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager.cc:117: for (const auto& it : disks_) { |it| is an inappropriate name as it's no longer an iterator. FYI now |it|'s type is const std::pair<std::string, std::unique_ptr<Disk>>>&. https://codereview.chromium.org/2451603002/diff/80001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager.cc:126: RemountRemovableDrive(*it.second.get(), mode); *it.second or *(it.second)? https://codereview.chromium.org/2451603002/diff/80001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager.cc:416: if (it != access_modes_.end() && !disk->is_read_only_hardware()) { Don't we need to explicitly set false when the is_read_only_hardware value changes? Isn't it safer to always set the value? disk->set_write_disabled_by_policy(it != access_modes_.end() && !disk->is_read_only_hardware() && it->second == MOUNT_ACCESS_MODE_READ_ONLY) https://codereview.chromium.org/2451603002/diff/80001/chromeos/disks/disk_mou... File chromeos/disks/disk_mount_manager_unittest.cc (right): https://codereview.chromium.org/2451603002/diff/80001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager_unittest.cc:906: // Should remount all devices but read-only hardwares. "but read-only hardwares" what this mean? It seems the "is read only" values of device1 and device2 are both false.
https://codereview.chromium.org/2451603002/diff/20001/chromeos/disks/disk_mou... File chromeos/disks/disk_mount_manager.cc (right): https://codereview.chromium.org/2451603002/diff/20001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager.cc:147: access_modes_[source_path] = access_mode; On 2016/10/27 08:31:52, hashimoto wrote: > On 2016/10/27 06:34:55, yamaguchi wrote: > > On 2016/10/26 06:05:12, hashimoto wrote: > > > I'm not familiar with this access mode management code so please also ask > > > satorux@ to review this part. > > > > > > Isn't there a plan to get this info via D-Bus? > > > I'm not sure how robust the current implementation is against unexpected > > events > > > like Chrome crash which can result in inconsistency between the actual mount > > > state and Chrome's internal state. > > > > Yes, storing to access_modes_ here is a tentative solution. > > > > We haven't surfaced the one in cros_disks as DBus API yet. > > https://chromium-review.googlesource.com/#/c/373320/ > > Seems the said patch was already submitted. > What else needs to be done? > > I'm not familiar with this access_modes_ management code, but it looks quite > fragile to me. > For example, what happens when RemountAllRemovableDrives() gets called twice > with different access mode values, quickly enough? > For the first OnMountCompleted call, the Disk object should reflect the first > access mode, but IIUC it mistakenly reflects the second access mode. Sorry for confusing you. That change is part of it, but we still need another CL to update the DBus message schema and fill data there, and it's not done yet. So this change is still based on the tentative solution. https://codereview.chromium.org/2451603002/diff/80001/chromeos/disks/disk_mou... File chromeos/disks/disk_mount_manager.cc (right): https://codereview.chromium.org/2451603002/diff/80001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager.cc:117: for (const auto& it : disks_) { On 2016/10/27 08:31:52, hashimoto wrote: > |it| is an inappropriate name as it's no longer an iterator. > FYI now |it|'s type is const std::pair<std::string, std::unique_ptr<Disk>>>&. Done. https://codereview.chromium.org/2451603002/diff/80001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager.cc:126: RemountRemovableDrive(*it.second.get(), mode); On 2016/10/27 08:31:52, hashimoto wrote: > *it.second or *(it.second)? Done. https://codereview.chromium.org/2451603002/diff/80001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager.cc:416: if (it != access_modes_.end() && !disk->is_read_only_hardware()) { On 2016/10/27 08:31:52, hashimoto wrote: > Don't we need to explicitly set false when the is_read_only_hardware value > changes? > Isn't it safer to always set the value? > disk->set_write_disabled_by_policy(it != access_modes_.end() && > !disk->is_read_only_hardware() && it->second == MOUNT_ACCESS_MODE_READ_ONLY) I don't think we need to fill false, because we don't expect is_read_only_hardware() changes without removing that device. The field is initialized using the profile policy at OnGetDeviceProperties first. However, I'd like to choose that as I think it's much clearer. https://codereview.chromium.org/2451603002/diff/80001/chromeos/disks/disk_mou... File chromeos/disks/disk_mount_manager_unittest.cc (right): https://codereview.chromium.org/2451603002/diff/80001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager_unittest.cc:906: // Should remount all devices but read-only hardwares. On 2016/10/27 08:31:52, hashimoto wrote: > "but read-only hardwares" what this mean? > It seems the "is read only" values of device1 and device2 are both false. It means that "all volumes should be remounted, but those for a device that is read-only by its hardware, should not be remounted". The test has only 1 device remounted, so I changed the comment to avoid the confusion. Also added comment abotut the initial condition used here.
lgtm https://codereview.chromium.org/2451603002/diff/20001/chromeos/disks/disk_mou... File chromeos/disks/disk_mount_manager.cc (right): https://codereview.chromium.org/2451603002/diff/20001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager.cc:147: access_modes_[source_path] = access_mode; On 2016/10/27 09:52:40, yamaguchi wrote: > On 2016/10/27 08:31:52, hashimoto wrote: > > On 2016/10/27 06:34:55, yamaguchi wrote: > > > On 2016/10/26 06:05:12, hashimoto wrote: > > > > I'm not familiar with this access mode management code so please also ask > > > > satorux@ to review this part. > > > > > > > > Isn't there a plan to get this info via D-Bus? > > > > I'm not sure how robust the current implementation is against unexpected > > > events > > > > like Chrome crash which can result in inconsistency between the actual > mount > > > > state and Chrome's internal state. > > > > > > Yes, storing to access_modes_ here is a tentative solution. > > > > > > We haven't surfaced the one in cros_disks as DBus API yet. > > > https://chromium-review.googlesource.com/#/c/373320/ > > > > Seems the said patch was already submitted. > > What else needs to be done? > > > > I'm not familiar with this access_modes_ management code, but it looks quite > > fragile to me. > > For example, what happens when RemountAllRemovableDrives() gets called twice > > with different access mode values, quickly enough? > > For the first OnMountCompleted call, the Disk object should reflect the first > > access mode, but IIUC it mistakenly reflects the second access mode. > > Sorry for confusing you. That change is part of it, but we still need another CL > to update the DBus message schema and fill data there, and it's not done yet. So > this change is still based on the tentative solution. The said change should be made as early as possible. As I said above, this class has race bugs as long as we use this map.
The CQ bit was checked by yamaguchi@chromium.org
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 yamaguchi@chromium.org
On 2016/10/28 08:55:37, hashimoto wrote: > lgtm > > https://codereview.chromium.org/2451603002/diff/20001/chromeos/disks/disk_mou... > File chromeos/disks/disk_mount_manager.cc (right): > > https://codereview.chromium.org/2451603002/diff/20001/chromeos/disks/disk_mou... > chromeos/disks/disk_mount_manager.cc:147: access_modes_[source_path] = > access_mode; > On 2016/10/27 09:52:40, yamaguchi wrote: > > On 2016/10/27 08:31:52, hashimoto wrote: > > > On 2016/10/27 06:34:55, yamaguchi wrote: > > > > On 2016/10/26 06:05:12, hashimoto wrote: > > > > > I'm not familiar with this access mode management code so please also > ask > > > > > satorux@ to review this part. > > > > > > > > > > Isn't there a plan to get this info via D-Bus? > > > > > I'm not sure how robust the current implementation is against unexpected > > > > events > > > > > like Chrome crash which can result in inconsistency between the actual > > mount > > > > > state and Chrome's internal state. > > > > > > > > Yes, storing to access_modes_ here is a tentative solution. > > > > > > > > We haven't surfaced the one in cros_disks as DBus API yet. > > > > https://chromium-review.googlesource.com/#/c/373320/ > > > > > > Seems the said patch was already submitted. > > > What else needs to be done? > > > > > > I'm not familiar with this access_modes_ management code, but it looks quite > > > fragile to me. > > > For example, what happens when RemountAllRemovableDrives() gets called twice > > > with different access mode values, quickly enough? > > > For the first OnMountCompleted call, the Disk object should reflect the > first > > > access mode, but IIUC it mistakenly reflects the second access mode. > > > > Sorry for confusing you. That change is part of it, but we still need another > CL > > to update the DBus message schema and fill data there, and it's not done yet. > So > > this change is still based on the tentative solution. > > The said change should be made as early as possible. > As I said above, this class has race bugs as long as we use this map. I agree. Filed crbug/660338.
yamaguchi@chromium.org changed reviewers: + satorux@chromium.org
+satorux for files under chromeos/disks.
https://codereview.chromium.org/2451603002/diff/120001/chromeos/disks/disk_mo... File chromeos/disks/disk_mount_manager.cc (right): https://codereview.chromium.org/2451603002/diff/120001/chromeos/disks/disk_mo... chromeos/disks/disk_mount_manager.cc:116: // TODO(yamaguchi): Retry for tentative remount failures. file a bug and add a crbug.com link here? https://codereview.chromium.org/2451603002/diff/120001/chromeos/disks/disk_mo... chromeos/disks/disk_mount_manager.cc:123: if (disk.mount_path().empty()) { checking mount path string looks rather strange. would it make sense to have a boolean field like disk.is_mounted()? https://codereview.chromium.org/2451603002/diff/120001/chromeos/disks/mock_di... File chromeos/disks/mock_disk_mount_manager.h (right): https://codereview.chromium.org/2451603002/diff/120001/chromeos/disks/mock_di... chromeos/disks/mock_disk_mount_manager.h:23: // (http://crbug.com/355757) just a suggestion: this may be a good cleanup work item for spare time. :)
https://codereview.chromium.org/2451603002/diff/120001/chromeos/disks/disk_mo... File chromeos/disks/disk_mount_manager.cc (right): https://codereview.chromium.org/2451603002/diff/120001/chromeos/disks/disk_mo... chromeos/disks/disk_mount_manager.cc:116: // TODO(yamaguchi): Retry for tentative remount failures. On 2016/10/31 05:36:00, satorux1 wrote: > file a bug and add a http://crbug.com link here? > Done. https://codereview.chromium.org/2451603002/diff/120001/chromeos/disks/disk_mo... chromeos/disks/disk_mount_manager.cc:123: if (disk.mount_path().empty()) { On 2016/10/31 05:36:00, satorux1 wrote: > checking mount path string looks rather strange. would it make sense to have a > boolean field like disk.is_mounted()? Done. https://codereview.chromium.org/2451603002/diff/120001/chromeos/disks/mock_di... File chromeos/disks/mock_disk_mount_manager.h (right): https://codereview.chromium.org/2451603002/diff/120001/chromeos/disks/mock_di... chromeos/disks/mock_disk_mount_manager.h:23: // (http://crbug.com/355757) On 2016/10/31 05:36:00, satorux1 wrote: > just a suggestion: this may be a good cleanup work item for spare time. :) Agreed. I'll do it in a spared time.
chromeos/disks lgtm
The CQ bit was checked by yamaguchi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hashimoto@chromium.org Link to the patchset: https://codereview.chromium.org/2451603002/#ps140001 (title: "Add is_mounted() method.")
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 ========== Add methods to remount removable devices in DiskMountManager. The methods will be invoked from VolumeManager to flip the read-only mount option upon ExternalStorageReadOnly policy update. Also allow to flip the read-only status of a Disk object from read-only to read-write, because it can happen when remounting a disk. BUG=642247 TEST=chromeos_unittests --gtest_filter=DiskMountManagerTest.* ========== to ========== Add methods to remount removable devices in DiskMountManager. The methods will be invoked from VolumeManager to flip the read-only mount option upon ExternalStorageReadOnly policy update. Also allow to flip the read-only status of a Disk object from read-only to read-write, because it can happen when remounting a disk. BUG=642247 TEST=chromeos_unittests --gtest_filter=DiskMountManagerTest.* ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Add methods to remount removable devices in DiskMountManager. The methods will be invoked from VolumeManager to flip the read-only mount option upon ExternalStorageReadOnly policy update. Also allow to flip the read-only status of a Disk object from read-only to read-write, because it can happen when remounting a disk. BUG=642247 TEST=chromeos_unittests --gtest_filter=DiskMountManagerTest.* ========== to ========== Add methods to remount removable devices in DiskMountManager. The methods will be invoked from VolumeManager to flip the read-only mount option upon ExternalStorageReadOnly policy update. Also allow to flip the read-only status of a Disk object from read-only to read-write, because it can happen when remounting a disk. BUG=642247 TEST=chromeos_unittests --gtest_filter=DiskMountManagerTest.* Committed: https://crrev.com/de59ed6bed57372a9c00a7c65554abd47471f225 Cr-Commit-Position: refs/heads/master@{#430564} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/de59ed6bed57372a9c00a7c65554abd47471f225 Cr-Commit-Position: refs/heads/master@{#430564} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
