New issue
Advanced search Search tips

Issue 870621 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 9
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug



Sign in to add a comment

Assume baes::FilePath::CharType == char in platform2

Project Member Reported by satorux@chromium.org, Aug 3

Issue description

It's safe to assume FilePath::CharType == char in platform2
where we don't have to worry about Windows.

With this assumption, the code can be simplified:

- constexpr base::FilePath::CharType kRunContainersPath[] =
-     FILE_PATH_LITERAL("/run/containers");
+ constexpr char kRunContainersPath[] = "/run/containers";

Besides, some code in platform2 already assumes FilePath::CharType == char [1], while some code tries not to (thus using FILE_PATH_LITERAL, etc.).

We can remove the inconsistency by assuming  FilePath::CharType == char everywhere in platform2

[1] Many FilePath objects are created without FILE_PATH_LITERAL:
% git grep 'FilePath("' |wc -l 
310

Context: https://groups.google.com/a/chromium.org/forum/#!topic/chromium-os-dev/lMNs2sazeD4
 
Project Member

Comment 1 by bugdroid1@chromium.org, Aug 6

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/repohooks/+/15d17a5e6816ef55cd12806f5fe2c7c16adbc43c

commit 15d17a5e6816ef55cd12806f5fe2c7c16adbc43c
Author: Satoru Takabayashi <satorux@google.com>
Date: Mon Aug 06 20:38:04 2018

pre-upload: Add a check for FilePath::CharType stuff

It's safe to assume FilePath::CharType == char in places like
platform2 where we don't have to worry about Windows.

With this assumption, the code can be simplified:

- constexpr base::FilePath::CharType kRunContainersPath[] =
-     FILE_PATH_LITERAL("/run/containers");
+ constexpr char kRunContainersPath[] = "/run/containers";

BUG= chromium:870621 
TEST=./pre-upload_unittest.py

Change-Id: I6c96fd3d231ba5d91f074d0c84a28b2a11528af5
Reviewed-on: https://chromium-review.googlesource.com/1163354
Commit-Ready: Satoru Takabayashi <satorux@google.com>
Tested-by: Satoru Takabayashi <satorux@google.com>
Reviewed-by: Dan Erat <derat@chromium.org>

[modify] https://crrev.com/15d17a5e6816ef55cd12806f5fe2c7c16adbc43c/pre-upload_unittest.py
[modify] https://crrev.com/15d17a5e6816ef55cd12806f5fe2c7c16adbc43c/pre-upload.py

Project Member

Comment 2 by bugdroid1@chromium.org, Aug 7

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e

commit cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e
Author: Satoru Takabayashi <satorux@google.com>
Date: Tue Aug 07 08:51:23 2018

platform2: Assume FilePath::CharType == char in platform2

It's safe to assume FilePath::CharType == char in platform2
where we don't have to worry about Windows.

This patch removes FilePath::CharType and its friends
to make the code simpler and more consistent (previously
some code was assuming FilePath::CharType == char, while
some other code was trying to to).

Along the way, fix minor lint errors discovered by the
refactoring:

- Missing/duplicate/unsorted #includes.
- std::string -> char for global variables

BUG= chromium:870621 
TEST=make sure that the affected packages in platform2 build:
% cat list
chromeos-base/arc-adbd
chromeos-base/arc-apk-cache
chromeos-base/arc-obb-mounter
chromeos-base/arc-setup
chromeos-base/biod
chromeos-base/buffet
sys-apps/cfm-device-updater
chromeos-base/chaps
chromeos-base/chromeos-dbus-bindings
chromeos-base/chromeos-imageburner
chromeos-base/chromeos-login
chromeos-base/container_utils
chromeos-base/crash-reporter
chromeos-base/cros-disks
chromeos-base/cryptohome
chromeos-base/debugd
chromeos-base/feedback
chromeos-base/hammerd
chromeos-base/libcontainer
chromeos-base/metrics
chromeos-base/midis
chromeos-base/power_manager
chromeos-base/run_oci
chromeos-base/secure-erase-file
chromeos-base/virtual-file-provider
chromeos-base/vm_guest_tools
chromeos-base/vm_host_tools
% cros_workon --board=$BOARD start $(cat list) && ./build_packages

CQ-DEPEND=CL:1163354
Change-Id: I4e99b99914579598ef208a2e013852ecd0af4f78
Reviewed-on: https://chromium-review.googlesource.com/1163355
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Tested-by: Satoru Takabayashi <satorux@google.com>
Reviewed-by: Dan Erat <derat@chromium.org>

[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/cfm-device-updater/bizlink-updater/src/mcdp_chip_ctrl.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/power_manager/powerd/system/pluggable_internal_backlight.h
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/cryptohome/bootlockbox/nvram_boot_lockbox_unittest.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/crash-reporter/kernel_collector.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/cryptohome/mount.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/hammerd/main.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/arc/obb-mounter/arc_obb_mounter.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/power_manager/powerd/daemon_delegate.h
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/cros-disks/mount_manager.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/arc/setup/priv_code_verifier.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/cryptohome/dircrypto_data_migrator/migration_helper.h
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/feedback/feedback_util.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/login_manager/policy_store.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/libcontainer/libcontainer_util.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/run_oci/run_oci.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/power_manager/powerd/system/internal_backlight.h
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/debugd/src/storage_tool.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/login_manager/session_manager_impl.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/power_manager/powerd/main.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/arc/adbd/adbd.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/cryptohome/homedirs.h
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/vm_tools/syslog/collector_unittest.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/container_utils/mount_extension_image.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/cryptohome/arc_disk_quota_unittest.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/login_manager/system_utils_impl.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/cryptohome/stateful_recovery.h
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/cryptohome/homedirs_unittest.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/libcontainer/libcontainer_unittest.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/cryptohome/platform.h
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/cryptohome/bootlockbox/boot_attributes.h
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/power_manager/powerd/system/internal_backlight.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/image-burner/image_burner_impl.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/image-burner/image_burner_main.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/cfm-device-updater/bizlink-updater/src/dp_aux_ctrl.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/power_manager/powerd/system/peripheral_battery_watcher.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/cryptohome/platform.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/metrics/metrics_daemon.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/chaps/object_store_test.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/arc/apk-cache/cache_cleaner.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/crash-reporter/crash_sender.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/chaps/token_file_manager_linux.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/chaps/slot_manager_impl.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/buffet/manager.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/login_manager/policy_service.h
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/login_manager/user_policy_service_factory.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/login_manager/session_manager_impl_unittest.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/secure_erase_file/secure_erase_file.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/PRESUBMIT.cfg
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/power_manager/powerd/system/pluggable_internal_backlight.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/cryptohome/bootlockbox/boot_attributes.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/midis/tests/udev_handler_test.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/feedback/feedback_daemon.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/chromeos-dbus-bindings/proxy_generator.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/crash-reporter/arc_collector.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/cfm-device-updater/bizlink-updater/src/main.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/power_manager/powerd/system/ambient_light_sensor.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/vm_tools/garcon/desktop_file.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/cryptohome/service_unittest.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/virtual_file_provider/util.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/cryptohome/persistent_lookup_table.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/cryptohome/stateful_recovery.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/login_manager/device_local_account_manager.h
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/biod/biod_storage.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/cryptohome/dircrypto_data_migrator/migration_helper.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/cryptohome/bootlockbox/nvram_boot_lockbox.h
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/feedback/feedback_service_unittest.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/cryptohome/mount.h
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/power_manager/powerd/daemon_unittest.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/cros-disks/archive_manager.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/login_manager/device_local_account_manager.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/cryptohome/dircrypto_data_migrator/migration_helper_unittest.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/cryptohome/bootlockbox/boot_lockbox.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/login_manager/resilient_policy_store.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/chaps/isolate_linux.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/cryptohome/service.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/cryptohome/install_attributes.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/cryptohome/homedirs.cc
[modify] https://crrev.com/cf730fb1fe28db1e6377eb6fc4a44e39fb9c326e/libcontainer/cgroup_unittest.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Aug 8

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/repohooks/+/4ca37923df3fd15c576b54e5b3fa858f23ae5e98

commit 4ca37923df3fd15c576b54e5b3fa858f23ae5e98
Author: Satoru Takabayashi <satorux@google.com>
Date: Wed Aug 08 05:43:56 2018

pre-upload: Add a check for FilePath::FromUTF8Unsafe stuff

FromUTF8Unsafe() and AsUTF8Unsafe() are not necessary in places
like platform2 since it's safe to assume file names are UTF-8 on
Chrome OS. We can use FilePath() and value() instead.

BUG= chromium:870621 
TEST=./pre-upload_unittest.py

Change-Id: I688b893a8b7477ec4499dd4b093a7733a3f9dc5d
Reviewed-on: https://chromium-review.googlesource.com/1166273
Commit-Ready: Satoru Takabayashi <satorux@google.com>
Tested-by: Satoru Takabayashi <satorux@google.com>
Reviewed-by: Dan Erat <derat@chromium.org>

[modify] https://crrev.com/4ca37923df3fd15c576b54e5b3fa858f23ae5e98/pre-upload_unittest.py
[modify] https://crrev.com/4ca37923df3fd15c576b54e5b3fa858f23ae5e98/pre-upload.py

Project Member

Comment 4 by bugdroid1@chromium.org, Aug 8

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/8a4e46726c953466d150992318568ff6b1b1d2c2

commit 8a4e46726c953466d150992318568ff6b1b1d2c2
Author: Satoru Takabayashi <satorux@google.com>
Date: Wed Aug 08 11:29:17 2018

platform2: Get rid of FilePath::FromUTF8Unsafe stuff

It's safe to assume file paths are UTF-8 on Chrome OS.

Along the way, fix minor lint errors trigerred by the
changes:

- Remove (c): Do not use (c) in copyright headers in new files
- unsigned long -> ULONG: Use int16/int64/etc, rather than the C type long

The latter is safe because ULONG == unsigned long per clang:

 xULONG ss_percent = MapDbmToPercent(signal_strength);
  ^~~~~~
  ULONG
../../../../../../../usr/include/gobi/types.h:6:28: note: 'ULONG' declared here
typedef unsigned long      ULONG;

BUG= chromium:870621 
TEST=make sure that the affected packages in platform2 build:
% cros_workon --board=$BOARD start arc-obb-mounter gobi-cromo-plugin power_manager virtual-file-provider
% ./build_packages

CQ-DEPEND=CL:1166273

Change-Id: Ifaafefacdbe6b6f377dca53a1a0578eccc44e7b8
Reviewed-on: https://chromium-review.googlesource.com/1166615
Commit-Ready: Satoru Takabayashi <satorux@google.com>
Tested-by: Satoru Takabayashi <satorux@google.com>
Reviewed-by: Ben Chan <benchan@chromium.org>

[modify] https://crrev.com/8a4e46726c953466d150992318568ff6b1b1d2c2/arc/obb-mounter/mount.cc
[modify] https://crrev.com/8a4e46726c953466d150992318568ff6b1b1d2c2/virtual_file_provider/fuse_main.cc
[modify] https://crrev.com/8a4e46726c953466d150992318568ff6b1b1d2c2/power_manager/common/file_prefs_store.cc
[modify] https://crrev.com/8a4e46726c953466d150992318568ff6b1b1d2c2/gobi-cromo-plugin/gobi_cdma_modem.cc

Status: Fixed (was: Assigned)
This is complete in platform2.

The presubmit check can be used elsewhere by putting something like below in PRESUBMIT.cfg

  [Hook Overrides]
  filepath_chartype_check: true

Sign in to add a comment