Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2440443003:
Preserve the hardware read-only flag in Disk object. (Closed)
DescriptionPreserve the hardware read-only flag in Disk object.
The hardware read-only flag is obtained from device enumeration and
stored to Disk objects. Existing code overwrites that field when
applying read-only policy. However, that information will be needed when
we support remounting devices to switch between read-only and read-write
mode upon policy update.
After this change, Disk objects can distinguish whether a device is
mounted read-only because of a read-only hardware or because of the
mount option passed to Mount (for ExternalStorageReadOnly policy).
TEST=Run chromeos_unittest, unit_tests, and browser_tests.
BUG=642247, 655003
Committed: https://crrev.com/eaaad9842eef857da9e5e14b4ee5e6ffe6fe37ef
Cr-Commit-Position: refs/heads/master@{#427034}
Patch Set 1 #Patch Set 2 : Fix build errors. #Patch Set 3 : Fix build error in file_manager_private_api test. #
Total comments: 8
Patch Set 4 : Put the right value at Disk object construction. #
Total comments: 2
Patch Set 5 : Use auto. #
Total comments: 6
Patch Set 6 : Add comments on some private fields. #Patch Set 7 : Adjust comment to match with the field name. #
Total comments: 2
Patch Set 8 : Move comments to the constructor args. #
Messages
Total messages: 41 (24 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_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...
Description was changed from ========== Preserve the hardware read-only flag in Disk object. The hardware read-only flag is obtained from device enmeration and stored to Disk objects. Existing code overwrites that field when applying read-only policy. However, that information will be needed when we support remounting devices to switch between read-only and read-write mode upon policy update. After this change, Disk objects can distinguish whether a device is mounted read-only because of a read-only hardware or because of the mount option passed to Mount (for ExternalStorageReadOnly policy). TEST=run chromeos_unittest BUG=none ========== to ========== Preserve the hardware read-only flag in Disk object. The hardware read-only flag is obtained from device enmeration and stored to Disk objects. Existing code overwrites that field when applying read-only policy. However, that information will be needed when we support remounting devices to switch between read-only and read-write mode upon policy update. After this change, Disk objects can distinguish whether a device is mounted read-only because of a read-only hardware or because of the mount option passed to Mount (for ExternalStorageReadOnly policy). TEST=Run chromeos_unittest, unit_tests, and browser_tests. BUG=642247,655003 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
yamaguchi@chromium.org changed reviewers: + hashimoto@chromium.org
ptal.
Description was changed from ========== Preserve the hardware read-only flag in Disk object. The hardware read-only flag is obtained from device enmeration and stored to Disk objects. Existing code overwrites that field when applying read-only policy. However, that information will be needed when we support remounting devices to switch between read-only and read-write mode upon policy update. After this change, Disk objects can distinguish whether a device is mounted read-only because of a read-only hardware or because of the mount option passed to Mount (for ExternalStorageReadOnly policy). TEST=Run chromeos_unittest, unit_tests, and browser_tests. BUG=642247,655003 ========== to ========== Preserve the hardware read-only flag in Disk object. The hardware read-only flag is obtained from device enumeration and stored to Disk objects. Existing code overwrites that field when applying read-only policy. However, that information will be needed when we support remounting devices to switch between read-only and read-write mode upon policy update. After this change, Disk objects can distinguish whether a device is mounted read-only because of a read-only hardware or because of the mount option passed to Mount (for ExternalStorageReadOnly policy). TEST=Run chromeos_unittest, unit_tests, and browser_tests. BUG=642247,655003 ==========
https://codereview.chromium.org/2440443003/diff/40001/chromeos/disks/disk_mou... File chromeos/disks/disk_mount_manager.cc (right): https://codereview.chromium.org/2440443003/diff/40001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager.cc:474: false, Please make it easy to figure out what this "false" means. https://codereview.chromium.org/2440443003/diff/40001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager.cc:500: chromeos::MOUNT_ACCESS_MODE_READ_ONLY); nit: indent You can run "git cl format" to correctly format affected files. https://codereview.chromium.org/2440443003/diff/40001/chromeos/disks/disk_mou... File chromeos/disks/disk_mount_manager_unittest.cc (right): https://codereview.chromium.org/2440443003/diff/40001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager_unittest.cc:74: false, // write_disabled_by_policy nit: 2 spaces before a comment. https://codereview.chromium.org/2440443003/diff/40001/chromeos/disks/mock_dis... File chromeos/disks/mock_disk_mount_manager.cc (right): https://codereview.chromium.org/2440443003/diff/40001/chromeos/disks/mock_dis... chromeos/disks/mock_disk_mount_manager.cc:76: false, // write_disabled_by_policy nit: 2 spaces before a comment. The same goes for others.
https://codereview.chromium.org/2440443003/diff/40001/chromeos/disks/disk_mou... File chromeos/disks/disk_mount_manager.cc (right): https://codereview.chromium.org/2440443003/diff/40001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager.cc:474: false, On 2016/10/21 07:28:04, hashimoto wrote: > Please make it easy to figure out what this "false" means. Done. https://codereview.chromium.org/2440443003/diff/40001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager.cc:500: chromeos::MOUNT_ACCESS_MODE_READ_ONLY); On 2016/10/21 07:28:04, hashimoto wrote: > nit: indent > You can run "git cl format" to correctly format affected files. Thanks. It looks the existing code differs in the style as that of the command, thus it will produce unrelated changes. For example, the invocation of the Disk constructor above will be compressed by putting as many arguments as possible in each line. So I'd like to keep some part of this file as-is for now. https://codereview.chromium.org/2440443003/diff/40001/chromeos/disks/disk_mou... File chromeos/disks/disk_mount_manager_unittest.cc (right): https://codereview.chromium.org/2440443003/diff/40001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager_unittest.cc:74: false, // write_disabled_by_policy On 2016/10/21 07:28:04, hashimoto wrote: > nit: 2 spaces before a comment. Done. https://codereview.chromium.org/2440443003/diff/40001/chromeos/disks/mock_dis... File chromeos/disks/mock_disk_mount_manager.cc (right): https://codereview.chromium.org/2440443003/diff/40001/chromeos/disks/mock_dis... chromeos/disks/mock_disk_mount_manager.cc:76: false, // write_disabled_by_policy On 2016/10/21 07:28:04, hashimoto wrote: > nit: 2 spaces before a comment. > The same goes for others. Done.
lgtm https://codereview.chromium.org/2440443003/diff/60001/chromeos/disks/disk_mou... File chromeos/disks/disk_mount_manager.cc (right): https://codereview.chromium.org/2440443003/diff/60001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager.cc:477: AccessModeMap::iterator access_mode = nit: You can do auto access_mode = access_modes_.find(...);
https://codereview.chromium.org/2440443003/diff/60001/chromeos/disks/disk_mou... File chromeos/disks/disk_mount_manager.cc (right): https://codereview.chromium.org/2440443003/diff/60001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager.cc:477: AccessModeMap::iterator access_mode = On 2016/10/21 08:39:59, hashimoto wrote: > nit: You can do > auto access_mode = access_modes_.find(...); Done.
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...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
yamaguchi@chromium.org changed reviewers: + satorux@chromium.org
+satorux Will you review and approve the CL, because it contains changes to files under chromeos/disks/ ?
https://codereview.chromium.org/2440443003/diff/80001/chromeos/disks/disk_mou... File chromeos/disks/disk_mount_manager.h (right): https://codereview.chromium.org/2440443003/diff/80001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager.h:59: bool write_disabled_by_policy, any reason why this should be added here? i thought it'd be nicer to add this to a location below where other bool parameters are listed. https://codereview.chromium.org/2440443003/diff/80001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager.h:166: std::string mount_path_; ditto.
https://codereview.chromium.org/2440443003/diff/80001/chromeos/disks/disk_mou... File chromeos/disks/disk_mount_manager.h (right): https://codereview.chromium.org/2440443003/diff/80001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager.h:59: bool write_disabled_by_policy, On 2016/10/24 01:39:55, satorux1 wrote: > any reason why this should be added here? i thought it'd be nicer to add this to > a location below where other bool parameters are listed. I think that the parameter is tied to the mount_path above, as it conveys information that originates from the mount operation by CrOS, as opposed to that the most of other fields are originated from the physical device itself. Also, the field is only valid when mount_path is non-empty.
https://codereview.chromium.org/2440443003/diff/80001/chromeos/disks/disk_mou... File chromeos/disks/disk_mount_manager.h (right): https://codereview.chromium.org/2440443003/diff/80001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager.h:59: bool write_disabled_by_policy, On 2016/10/24 01:53:30, yamaguchi wrote: > On 2016/10/24 01:39:55, satorux1 wrote: > > any reason why this should be added here? i thought it'd be nicer to add this > to > > a location below where other bool parameters are listed. > > I think that the parameter is tied to the mount_path above, as it conveys > information that originates from the mount operation by CrOS, as opposed to that > the most of other fields are originated from the physical device itself. Also, > the field is only valid when mount_path is non-empty. Ah OK. Then please add a comment that explains the relationship between mount_path and this parameter. Then it'll be clear and informative.
https://codereview.chromium.org/2440443003/diff/80001/chromeos/disks/disk_mou... File chromeos/disks/disk_mount_manager.h (right): https://codereview.chromium.org/2440443003/diff/80001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager.h:59: bool write_disabled_by_policy, On 2016/10/24 02:00:11, satorux1 wrote: > On 2016/10/24 01:53:30, yamaguchi wrote: > > On 2016/10/24 01:39:55, satorux1 wrote: > > > any reason why this should be added here? i thought it'd be nicer to add > this > > to > > > a location below where other bool parameters are listed. > > > > I think that the parameter is tied to the mount_path above, as it conveys > > information that originates from the mount operation by CrOS, as opposed to > that > > the most of other fields are originated from the physical device itself. Also, > > the field is only valid when mount_path is non-empty. > > Ah OK. Then please add a comment that explains the relationship between > mount_path and this parameter. Then it'll be clear and informative. Done. https://codereview.chromium.org/2440443003/diff/80001/chromeos/disks/disk_mou... chromeos/disks/disk_mount_manager.h:166: std::string mount_path_; On 2016/10/24 01:39:55, satorux1 wrote: > ditto. Done.
lgtm with a request https://codereview.chromium.org/2440443003/diff/120001/chromeos/disks/disk_mo... File chromeos/disks/disk_mount_manager.h (right): https://codereview.chromium.org/2440443003/diff/120001/chromeos/disks/disk_mo... chromeos/disks/disk_mount_manager.h:172: bool write_disabled_by_policy_; Maybe move these comments to the constructor? I think these comments are useful for developers who use this class, and comments at the constructor are more easily discoverable. Comments for member variables are usually for implementation details.
https://codereview.chromium.org/2440443003/diff/120001/chromeos/disks/disk_mo... File chromeos/disks/disk_mount_manager.h (right): https://codereview.chromium.org/2440443003/diff/120001/chromeos/disks/disk_mo... chromeos/disks/disk_mount_manager.h:172: bool write_disabled_by_policy_; On 2016/10/24 05:07:57, satorux1 wrote: > Maybe move these comments to the constructor? I think these comments are useful > for developers who use this class, and comments at the constructor are more > easily discoverable. > > Comments for member variables are usually for implementation details. Done.
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, satorux@chromium.org Link to the patchset: https://codereview.chromium.org/2440443003/#ps140001 (title: "Move comments to the constructor args.")
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 ========== Preserve the hardware read-only flag in Disk object. The hardware read-only flag is obtained from device enumeration and stored to Disk objects. Existing code overwrites that field when applying read-only policy. However, that information will be needed when we support remounting devices to switch between read-only and read-write mode upon policy update. After this change, Disk objects can distinguish whether a device is mounted read-only because of a read-only hardware or because of the mount option passed to Mount (for ExternalStorageReadOnly policy). TEST=Run chromeos_unittest, unit_tests, and browser_tests. BUG=642247,655003 ========== to ========== Preserve the hardware read-only flag in Disk object. The hardware read-only flag is obtained from device enumeration and stored to Disk objects. Existing code overwrites that field when applying read-only policy. However, that information will be needed when we support remounting devices to switch between read-only and read-write mode upon policy update. After this change, Disk objects can distinguish whether a device is mounted read-only because of a read-only hardware or because of the mount option passed to Mount (for ExternalStorageReadOnly policy). TEST=Run chromeos_unittest, unit_tests, and browser_tests. BUG=642247,655003 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Preserve the hardware read-only flag in Disk object. The hardware read-only flag is obtained from device enumeration and stored to Disk objects. Existing code overwrites that field when applying read-only policy. However, that information will be needed when we support remounting devices to switch between read-only and read-write mode upon policy update. After this change, Disk objects can distinguish whether a device is mounted read-only because of a read-only hardware or because of the mount option passed to Mount (for ExternalStorageReadOnly policy). TEST=Run chromeos_unittest, unit_tests, and browser_tests. BUG=642247,655003 ========== to ========== Preserve the hardware read-only flag in Disk object. The hardware read-only flag is obtained from device enumeration and stored to Disk objects. Existing code overwrites that field when applying read-only policy. However, that information will be needed when we support remounting devices to switch between read-only and read-write mode upon policy update. After this change, Disk objects can distinguish whether a device is mounted read-only because of a read-only hardware or because of the mount option passed to Mount (for ExternalStorageReadOnly policy). TEST=Run chromeos_unittest, unit_tests, and browser_tests. BUG=642247,655003 Committed: https://crrev.com/eaaad9842eef857da9e5e14b4ee5e6ffe6fe37ef Cr-Commit-Position: refs/heads/master@{#427034} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/eaaad9842eef857da9e5e14b4ee5e6ffe6fe37ef Cr-Commit-Position: refs/heads/master@{#427034} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
