Skip to content

Commit 4f02e03

Browse files
DatabeanDatabean
committed
Use Result in SplitRamdiskModules
`SplitRamdiskModules` called many functions that returned `Result`s, and is only called within a `CF_EXPECT` invocation. This means it can forward errors out to its caller as a `Result`, rather than use `CHECK`s or conditionals on the `Result`s it calls. Tested with ``` $ bazel run //cuttlefish/package:cvd -- fetch --default_build=git_main/aosp_cf_x86_64_only_phone-trunk_staging-userdebug --target_directory=$HOME/dl_kernel --kernel_build=aosp_kernel-common-android-mainline/kernel_virt_x86_64 $ bazel run //cuttlefish/package:cvd -- create --host_path=$HOME/dl_kernel --product_path=$HOME/dl_kernel --host_substitutions=bin/assemble_cvd,bin/run_cvd ``` Bug: b/464082938
1 parent 0b3ce48 commit 4f02e03

File tree

2 files changed

+49
-53
lines changed

2 files changed

+49
-53
lines changed

‎base/cvd/cuttlefish/host/commands/assemble_cvd/vendor_dlkm_utils.cc‎

Lines changed: 45 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -423,28 +423,27 @@ std::vector<std::string> Dedup(std::vector<std::string>&& vec) {
423423
return vec;
424424
}
425425

426-
bool SplitRamdiskModules(const std::string& ramdisk_path,
427-
const std::string& ramdisk_stage_dir,
428-
const std::string& vendor_dlkm_build_dir,
429-
const std::string& system_dlkm_build_dir) {
430-
const auto vendor_modules_dir = vendor_dlkm_build_dir + "/lib/modules";
431-
const auto system_modules_dir = system_dlkm_build_dir + "/lib/modules";
432-
auto ret = EnsureDirectoryExists(vendor_modules_dir);
433-
CHECK(ret.ok()) << ret.error().FormatForEnv();
434-
ret = EnsureDirectoryExists(system_modules_dir);
426+
Result<void> SplitRamdiskModules(const std::string& ramdisk_path,
427+
const std::string& ramdisk_stage_dir,
428+
const std::string& vendor_dlkm_build_dir,
429+
const std::string& system_dlkm_build_dir) {
430+
std::string vendor_modules_dir = vendor_dlkm_build_dir + "/lib/modules";
431+
std::string system_modules_dir = system_dlkm_build_dir + "/lib/modules";
432+
433+
CF_EXPECT(EnsureDirectoryExists(vendor_modules_dir));
434+
CF_EXPECT(EnsureDirectoryExists(system_modules_dir));
435+
435436
UnpackRamdisk(ramdisk_path, ramdisk_stage_dir);
436-
auto res = FindFile(ramdisk_stage_dir.c_str(), "modules.load");
437-
if (!res.ok()) {
438-
LOG(ERROR) << "Failed to find modules.dep file in input ramdisk "
439-
<< ramdisk_path;
440-
return false;
441-
}
442-
const auto module_load_file = android::base::Trim(res.value());
443-
if (module_load_file.empty()) {
444-
LOG(ERROR) << "Failed to find modules.dep file in input ramdisk "
445-
<< ramdisk_path;
446-
return false;
447-
}
437+
438+
std::string module_load_file =
439+
CF_EXPECT(FindFile(ramdisk_stage_dir, "modules.load"),
440+
"Failed to find modules.dep file in input ramdisk");
441+
module_load_file = android::base::Trim(module_load_file);
442+
443+
CF_EXPECTF(!module_load_file.empty(),
444+
"Failed to find modules.dep file in input ramdisk '{}'",
445+
ramdisk_path);
446+
448447
LOG(INFO) << "modules.load location " << module_load_file;
449448
const auto module_list =
450449
Dedup(android::base::Tokenize(ReadFile(module_load_file), "\n"));
@@ -469,30 +468,28 @@ bool SplitRamdiskModules(const std::string& ramdisk_path,
469468
if (IsKernelModuleSigned(module_location)) {
470469
const auto system_dlkm_module_location =
471470
fmt::format("{}/{}", system_modules_dir, module_path);
472-
auto res = EnsureDirectoryExists(
473-
android::base::Dirname(system_dlkm_module_location));
474-
CHECK(res.ok()) << res.error().FormatForEnv();
475-
auto ret = RenameFile(module_location, system_dlkm_module_location);
476-
CHECK(ret.ok()) << ret.error().FormatForEnv();
471+
472+
CF_EXPECT(EnsureDirectoryExists(
473+
android::base::Dirname(system_dlkm_module_location)));
474+
CF_EXPECT(RenameFile(module_location, system_dlkm_module_location));
475+
477476
system_dlkm_modules.emplace(module_path);
478477
} else {
479478
const auto vendor_dlkm_module_location =
480479
fmt::format("{}/{}", vendor_modules_dir, module_path);
481-
auto res = EnsureDirectoryExists(
482-
android::base::Dirname(vendor_dlkm_module_location));
483-
CHECK(res.ok()) << res.error().FormatForEnv();
484-
auto ret = RenameFile(module_location, vendor_dlkm_module_location);
485-
CHECK(ret.ok()) << ret.error().FormatForEnv();
480+
481+
CF_EXPECT(EnsureDirectoryExists(
482+
android::base::Dirname(vendor_dlkm_module_location)));
483+
CF_EXPECT(RenameFile(module_location, vendor_dlkm_module_location));
484+
486485
vendor_dlkm_modules.emplace(module_path);
487486
}
488487
}
489488
for (const auto& gki_module : system_dlkm_modules) {
490489
for (const auto& dep : deps.at(gki_module)) {
491-
if (vendor_dlkm_modules.count(dep)) {
492-
LOG(ERROR) << "GKI module " << gki_module
493-
<< " depends on vendor_dlkm module " << dep;
494-
return false;
495-
}
490+
CF_EXPECTF(vendor_dlkm_modules.count(dep) == 0,
491+
"GKI module '{}' depends on vendor_dlkm module '{}'",
492+
gki_module, dep);
496493
}
497494
}
498495
LOG(INFO) << "There are " << ramdisk_modules.size() << " ramdisk modules, "
@@ -507,28 +504,27 @@ bool SplitRamdiskModules(const std::string& ramdisk_path,
507504
if (FileExists(initramfs_blocklist_path)) {
508505
const auto vendor_dlkm_blocklist_path =
509506
fmt::format("{}/{}", vendor_modules_dir, "modules.blocklist");
510-
auto ret = RenameFile(initramfs_blocklist_path, vendor_dlkm_blocklist_path);
511-
CHECK(ret.ok()) << ret.error().FormatForEnv();
507+
CF_EXPECT(RenameFile(initramfs_blocklist_path, vendor_dlkm_blocklist_path));
512508
}
513509

514510
// Write updated modules.dep and modules.load files
515-
CHECK(WriteDepsToFile(FilterDependencies(deps, ramdisk_modules),
516-
module_base_dir + "/modules.dep"));
517-
CHECK(WriteLinesToFile(ramdisk_modules, module_load_file.c_str()));
511+
CF_EXPECT(WriteDepsToFile(FilterDependencies(deps, ramdisk_modules),
512+
module_base_dir + "/modules.dep"));
513+
CF_EXPECT(WriteLinesToFile(ramdisk_modules, module_load_file.c_str()));
518514

519-
CHECK(WriteDepsToFile(
515+
CF_EXPECT(WriteDepsToFile(
520516
UpdateGKIModulePaths(FilterOutDependencies(deps, ramdisk_modules),
521517
system_dlkm_modules),
522518
vendor_modules_dir + "/modules.dep"));
523-
CHECK(WriteLinesToFile(vendor_dlkm_modules,
524-
(vendor_modules_dir + "/modules.load").c_str()));
519+
CF_EXPECT(WriteLinesToFile(vendor_dlkm_modules,
520+
(vendor_modules_dir + "/modules.load").c_str()));
525521

526-
CHECK(WriteDepsToFile(FilterDependencies(deps, system_dlkm_modules),
527-
system_modules_dir + "/modules.dep"));
528-
CHECK(WriteLinesToFile(system_dlkm_modules,
529-
(system_modules_dir + "/modules.load").c_str()));
522+
CF_EXPECT(WriteDepsToFile(FilterDependencies(deps, system_dlkm_modules),
523+
system_modules_dir + "/modules.dep"));
524+
CF_EXPECT(WriteLinesToFile(system_dlkm_modules,
525+
(system_modules_dir + "/modules.load").c_str()));
530526
PackRamdisk(ramdisk_stage_dir, ramdisk_path);
531-
return true;
527+
return {};
532528
}
533529

534530
bool FileEquals(const std::string& file1, const std::string& file2) {

‎base/cvd/cuttlefish/host/commands/assemble_cvd/vendor_dlkm_utils.h‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -21,10 +21,10 @@
2121

2222
namespace cuttlefish {
2323

24-
bool SplitRamdiskModules(const std::string& ramdisk_path,
25-
const std::string& ramdisk_stage_dir,
26-
const std::string& vendor_dlkm_build_dir,
27-
const std::string& system_dlkm_build_dir);
24+
Result<void> SplitRamdiskModules(const std::string& ramdisk_path,
25+
const std::string& ramdisk_stage_dir,
26+
const std::string& vendor_dlkm_build_dir,
27+
const std::string& system_dlkm_build_dir);
2828

2929
Result<bool> WriteFsConfig(const char* output_path, const std::string& fs_root,
3030
const std::string& mount_point);

0 commit comments

Comments
 (0)