Use binder::IntoBinderResult to simplify error handling
In addition to the trait, a local trait `LogResult` is also added for
the case of logging an error while returning it. ex:
```
let x = some_function()
.context("some message")
.with_log() // if error, log the error message
.or_binder_exception(ExceptionCode::INVALID_ARGUMENT)?;
```
Bug: 294348831
Test: m com.android.virt
Change-Id: I461ba6501be66014f73831f4320a50cd9eee0ae7
diff --git a/virtualizationmanager/src/aidl.rs b/virtualizationmanager/src/aidl.rs
index 91bd60b..ae81a17 100644
--- a/virtualizationmanager/src/aidl.rs
+++ b/virtualizationmanager/src/aidl.rs
@@ -53,6 +53,7 @@
use binder::{
self, wait_for_interface, BinderFeatures, ExceptionCode, Interface, ParcelFileDescriptor,
Status, StatusCode, Strong,
+ IntoBinderResult,
};
use disk::QcowFile;
use lazy_static::lazy_static;
@@ -76,6 +77,20 @@
use vsock::VsockStream;
use zip::ZipArchive;
+/// Convenient trait for logging an error while returning it
+trait LogResult<T, E> {
+ fn with_log(self) -> std::result::Result<T, E>;
+}
+
+impl<T, E: std::fmt::Debug> LogResult<T, E> for std::result::Result<T, E> {
+ fn with_log(self) -> std::result::Result<T, E> {
+ self.map_err(|e| {
+ error!("{e:?}");
+ e
+ })
+ }
+}
+
/// The unique ID of a VM used (together with a port number) for vsock communication.
pub type Cid = u32;
@@ -179,7 +194,6 @@
Ok(())
}
}
-
impl IVirtualizationService for VirtualizationService {
/// Creates (but does not start) a new VM with the given configuration, assigning it the next
/// available CID.
@@ -212,27 +226,17 @@
partition_type: PartitionType,
) -> binder::Result<()> {
check_manage_access()?;
- let size_bytes = size_bytes.try_into().map_err(|e| {
- Status::new_exception_str(
- ExceptionCode::ILLEGAL_ARGUMENT,
- Some(format!("Invalid size {}: {:?}", size_bytes, e)),
- )
- })?;
+ let size_bytes = size_bytes
+ .try_into()
+ .with_context(|| format!("Invalid size: {}", size_bytes))
+ .or_binder_exception(ExceptionCode::ILLEGAL_ARGUMENT)?;
let size_bytes = round_up(size_bytes, PARTITION_GRANULARITY_BYTES);
let image = clone_file(image_fd)?;
// initialize the file. Any data in the file will be erased.
- image.set_len(0).map_err(|e| {
- Status::new_service_specific_error_str(
- -1,
- Some(format!("Failed to reset a file: {:?}", e)),
- )
- })?;
- let mut part = QcowFile::new(image, size_bytes).map_err(|e| {
- Status::new_service_specific_error_str(
- -1,
- Some(format!("Failed to create QCOW2 image: {:?}", e)),
- )
- })?;
+ image.set_len(0).context("Failed to reset a file").or_service_specific_exception(-1)?;
+ let mut part = QcowFile::new(image, size_bytes)
+ .context("Failed to create QCOW2 image")
+ .or_service_specific_exception(-1)?;
match partition_type {
PartitionType::RAW => Ok(()),
@@ -243,12 +247,8 @@
format!("Unsupported partition type {:?}", partition_type),
)),
}
- .map_err(|e| {
- Status::new_service_specific_error_str(
- -1,
- Some(format!("Failed to initialize partition as {:?}: {:?}", partition_type, e)),
- )
- })?;
+ .with_context(|| format!("Failed to initialize partition as {:?}", partition_type))
+ .or_service_specific_exception(-1)?;
Ok(())
}
@@ -261,8 +261,7 @@
) -> binder::Result<()> {
check_manage_access()?;
- create_or_update_idsig_file(input_fd, idsig_fd)
- .map_err(|e| Status::new_service_specific_error_str(-1, Some(format!("{:?}", e))))?;
+ create_or_update_idsig_file(input_fd, idsig_fd).or_service_specific_exception(-1)?;
Ok(())
}
@@ -309,10 +308,8 @@
}
}
}
- Err(Status::new_service_specific_error_str(
- -1,
- Some("Too many attempts to create VM context failed."),
- ))
+ Err(anyhow!("Too many attempts to create VM context failed"))
+ .or_service_specific_exception(-1)
}
fn create_vm_internal(
@@ -381,12 +378,12 @@
let (is_app_config, config) = match config {
VirtualMachineConfig::RawConfig(config) => (false, BorrowedOrOwned::Borrowed(config)),
VirtualMachineConfig::AppConfig(config) => {
- let config =
- load_app_config(config, &debug_config, &temporary_directory).map_err(|e| {
+ let config = load_app_config(config, &debug_config, &temporary_directory)
+ .or_service_specific_exception_with(-1, |e| {
*is_protected = config.protectedVm;
let message = format!("Failed to load app config: {:?}", e);
error!("{}", message);
- Status::new_service_specific_error_str(-1, Some(message))
+ message
})?;
(true, BorrowedOrOwned::Owned(config))
}
@@ -410,26 +407,21 @@
}
})
.try_for_each(check_label_for_partition)
- .map_err(|e| Status::new_service_specific_error_str(-1, Some(format!("{:?}", e))))?;
+ .or_service_specific_exception(-1)?;
let kernel = maybe_clone_file(&config.kernel)?;
let initrd = maybe_clone_file(&config.initrd)?;
// In a protected VM, we require custom kernels to come from a trusted source (b/237054515).
if config.protectedVm {
- check_label_for_kernel_files(&kernel, &initrd).map_err(|e| {
- Status::new_service_specific_error_str(-1, Some(format!("{:?}", e)))
- })?;
+ check_label_for_kernel_files(&kernel, &initrd).or_service_specific_exception(-1)?;
}
let zero_filler_path = temporary_directory.join("zero.img");
- write_zero_filler(&zero_filler_path).map_err(|e| {
- error!("Failed to make composite image: {:?}", e);
- Status::new_service_specific_error_str(
- -1,
- Some(format!("Failed to make composite image: {:?}", e)),
- )
- })?;
+ write_zero_filler(&zero_filler_path)
+ .context("Failed to make composite image")
+ .with_log()
+ .or_service_specific_exception(-1)?;
// Assemble disk images if needed.
let disks = config
@@ -450,28 +442,21 @@
CpuTopology::MATCH_HOST => (None, true),
CpuTopology::ONE_CPU => (NonZeroU32::new(1), false),
val => {
- error!("Unexpected value of CPU topology: {:?}", val);
- return Err(Status::new_service_specific_error_str(
- -1,
- Some(format!("Failed to parse CPU topology value: {:?}", val)),
- ));
+ return Err(anyhow!("Failed to parse CPU topology value {:?}", val))
+ .with_log()
+ .or_service_specific_exception(-1);
}
};
let devices_dtbo = if !config.devices.is_empty() {
let mut set = HashSet::new();
for device in config.devices.iter() {
- let path = canonicalize(device).map_err(|e| {
- Status::new_exception_str(
- ExceptionCode::ILLEGAL_ARGUMENT,
- Some(format!("can't canonicalize {device}: {e:?}")),
- )
- })?;
+ let path = canonicalize(device)
+ .with_context(|| format!("can't canonicalize {device}"))
+ .or_service_specific_exception(-1)?;
if !set.insert(path) {
- return Err(Status::new_exception_str(
- ExceptionCode::ILLEGAL_ARGUMENT,
- Some(format!("duplicated device {device}")),
- ));
+ return Err(anyhow!("duplicated device {device}"))
+ .or_binder_exception(ExceptionCode::ILLEGAL_ARGUMENT);
}
}
let dtbo_path = temporary_directory.join("dtbo");
@@ -533,13 +518,9 @@
requester_debug_pid,
vm_context,
)
- .map_err(|e| {
- error!("Failed to create VM with config {:?}: {:?}", config, e);
- Status::new_service_specific_error_str(
- -1,
- Some(format!("Failed to create VM: {:?}", e)),
- )
- })?,
+ .with_context(|| format!("Failed to create VM with config {:?}", config))
+ .with_log()
+ .or_service_specific_exception(-1)?,
);
state.add_vm(Arc::downgrade(&instance));
Ok(VirtualMachine::create(instance))
@@ -590,10 +571,8 @@
let image = if !disk.partitions.is_empty() {
if disk.image.is_some() {
warn!("DiskImage {:?} contains both image and partitions.", disk);
- return Err(Status::new_exception_str(
- ExceptionCode::ILLEGAL_ARGUMENT,
- Some("DiskImage contains both image and partitions."),
- ));
+ return Err(anyhow!("DiskImage contains both image and partitions"))
+ .or_binder_exception(ExceptionCode::ILLEGAL_ARGUMENT);
}
let composite_image_filenames =
@@ -605,13 +584,9 @@
&composite_image_filenames.header,
&composite_image_filenames.footer,
)
- .map_err(|e| {
- error!("Failed to make composite image with config {:?}: {:?}", disk, e);
- Status::new_service_specific_error_str(
- -1,
- Some(format!("Failed to make composite image: {:?}", e)),
- )
- })?;
+ .with_context(|| format!("Failed to make composite disk image with config {:?}", disk))
+ .with_log()
+ .or_service_specific_exception(-1)?;
// Pass the file descriptors for the various partition files to crosvm when it
// is run.
@@ -622,10 +597,8 @@
clone_file(image)?
} else {
warn!("DiskImage {:?} didn't contain image or partitions.", disk);
- return Err(Status::new_exception_str(
- ExceptionCode::ILLEGAL_ARGUMENT,
- Some("DiskImage didn't contain image or partitions."),
- ));
+ return Err(anyhow!("DiskImage didn't contain image or partitions."))
+ .or_binder_exception(ExceptionCode::ILLEGAL_ARGUMENT);
};
Ok(DiskFile { image, writable: disk.writable })
@@ -783,10 +756,8 @@
if perm_svc.checkPermission(perm, calling_pid, calling_uid as i32)? {
Ok(())
} else {
- Err(Status::new_exception_str(
- ExceptionCode::SECURITY,
- Some(format!("does not have the {} permission", perm)),
- ))
+ Err(anyhow!("does not have the {} permission", perm))
+ .or_binder_exception(ExceptionCode::SECURITY)
}
}
@@ -892,40 +863,41 @@
}
fn start(&self) -> binder::Result<()> {
- self.instance.start().map_err(|e| {
- error!("Error starting VM with CID {}: {:?}", self.instance.cid, e);
- Status::new_service_specific_error_str(-1, Some(e.to_string()))
- })
+ self.instance
+ .start()
+ .with_context(|| format!("Error starting VM with CID {}", self.instance.cid))
+ .with_log()
+ .or_service_specific_exception(-1)
}
fn stop(&self) -> binder::Result<()> {
- self.instance.kill().map_err(|e| {
- error!("Error stopping VM with CID {}: {:?}", self.instance.cid, e);
- Status::new_service_specific_error_str(-1, Some(e.to_string()))
- })
+ self.instance
+ .kill()
+ .with_context(|| format!("Error stopping VM with CID {}", self.instance.cid))
+ .with_log()
+ .or_service_specific_exception(-1)
}
fn onTrimMemory(&self, level: MemoryTrimLevel) -> binder::Result<()> {
- self.instance.trim_memory(level).map_err(|e| {
- error!("Error trimming VM with CID {}: {:?}", self.instance.cid, e);
- Status::new_service_specific_error_str(-1, Some(e.to_string()))
- })
+ self.instance
+ .trim_memory(level)
+ .with_context(|| format!("Error trimming VM with CID {}", self.instance.cid))
+ .with_log()
+ .or_service_specific_exception(-1)
}
fn connectVsock(&self, port: i32) -> binder::Result<ParcelFileDescriptor> {
if !matches!(&*self.instance.vm_state.lock().unwrap(), VmState::Running { .. }) {
- return Err(Status::new_service_specific_error_str(-1, Some("VM is not running")));
+ return Err(anyhow!("VM is not running")).or_service_specific_exception(-1);
}
let port = port as u32;
if port < 1024 {
- return Err(Status::new_service_specific_error_str(
- -1,
- Some(format!("Can't connect to privileged port {port}")),
- ));
+ return Err(anyhow!("Can't connect to privileged port {port}"))
+ .or_service_specific_exception(-1);
}
- let stream = VsockStream::connect_with_cid_port(self.instance.cid, port).map_err(|e| {
- Status::new_service_specific_error_str(-1, Some(format!("Failed to connect: {:?}", e)))
- })?;
+ let stream = VsockStream::connect_with_cid_port(self.instance.cid, port)
+ .context("Failed to connect")
+ .or_service_specific_exception(-1)?;
Ok(vsock_stream_to_pfd(stream))
}
}
@@ -1051,17 +1023,15 @@
}
/// Converts a `&ParcelFileDescriptor` to a `File` by cloning the file.
-pub fn clone_file(file: &ParcelFileDescriptor) -> Result<File, Status> {
- file.as_ref().try_clone().map_err(|e| {
- Status::new_exception_str(
- ExceptionCode::BAD_PARCELABLE,
- Some(format!("Failed to clone File from ParcelFileDescriptor: {:?}", e)),
- )
- })
+pub fn clone_file(file: &ParcelFileDescriptor) -> binder::Result<File> {
+ file.as_ref()
+ .try_clone()
+ .context("Failed to clone File from ParcelFileDescriptor")
+ .or_binder_exception(ExceptionCode::BAD_PARCELABLE)
}
/// Converts an `&Option<ParcelFileDescriptor>` to an `Option<File>` by cloning the file.
-fn maybe_clone_file(file: &Option<ParcelFileDescriptor>) -> Result<Option<File>, Status> {
+fn maybe_clone_file(file: &Option<ParcelFileDescriptor>) -> binder::Result<Option<File>> {
file.as_ref().map(clone_file).transpose()
}
@@ -1073,13 +1043,10 @@
}
/// Parses the platform version requirement string.
-fn parse_platform_version_req(s: &str) -> Result<VersionReq, Status> {
- VersionReq::parse(s).map_err(|e| {
- Status::new_exception_str(
- ExceptionCode::BAD_PARCELABLE,
- Some(format!("Invalid platform version requirement {}: {:?}", s, e)),
- )
- })
+fn parse_platform_version_req(s: &str) -> binder::Result<VersionReq> {
+ VersionReq::parse(s)
+ .with_context(|| format!("Invalid platform version requirement {}", s))
+ .or_binder_exception(ExceptionCode::BAD_PARCELABLE)
}
/// Create the empty ramdump file
@@ -1088,13 +1055,10 @@
// VM will emit ramdump to. `ramdump_read` will be sent back to the client (i.e. the VM
// owner) for readout.
let ramdump_path = temporary_directory.join("ramdump");
- let ramdump = File::create(ramdump_path).map_err(|e| {
- error!("Failed to prepare ramdump file: {:?}", e);
- Status::new_service_specific_error_str(
- -1,
- Some(format!("Failed to prepare ramdump file: {:?}", e)),
- )
- })?;
+ let ramdump = File::create(ramdump_path)
+ .context("Failed to prepare ramdump file")
+ .with_log()
+ .or_service_specific_exception(-1)?;
Ok(ramdump)
}
@@ -1107,20 +1071,16 @@
fn check_gdb_allowed(config: &VirtualMachineConfig) -> binder::Result<()> {
if is_protected(config) {
- return Err(Status::new_exception_str(
- ExceptionCode::SECURITY,
- Some("can't use gdb with protected VMs"),
- ));
+ return Err(anyhow!("Can't use gdb with protected VMs"))
+ .or_binder_exception(ExceptionCode::SECURITY);
}
match config {
VirtualMachineConfig::RawConfig(_) => Ok(()),
VirtualMachineConfig::AppConfig(config) => {
if config.debugLevel != DebugLevel::FULL {
- Err(Status::new_exception_str(
- ExceptionCode::SECURITY,
- Some("can't use gdb with non-debuggable VMs"),
- ))
+ Err(anyhow!("Can't use gdb with non-debuggable VMs"))
+ .or_binder_exception(ExceptionCode::SECURITY)
} else {
Ok(())
}
@@ -1150,9 +1110,8 @@
return Ok(None);
};
- let (raw_read_fd, raw_write_fd) = pipe().map_err(|e| {
- Status::new_service_specific_error_str(-1, Some(format!("Failed to create pipe: {:?}", e)))
- })?;
+ let (raw_read_fd, raw_write_fd) =
+ pipe().context("Failed to create pipe").or_service_specific_exception(-1)?;
// SAFETY: We are the sole owner of this FD as we just created it, and it is valid and open.
let mut reader = BufReader::new(unsafe { File::from_raw_fd(raw_read_fd) });
@@ -1212,9 +1171,8 @@
let cid = self.cid;
if let Some(vm) = self.state.lock().unwrap().get_vm(cid) {
info!("VM with CID {} started payload", cid);
- vm.update_payload_state(PayloadState::Started).map_err(|e| {
- Status::new_exception_str(ExceptionCode::ILLEGAL_STATE, Some(e.to_string()))
- })?;
+ vm.update_payload_state(PayloadState::Started)
+ .or_binder_exception(ExceptionCode::ILLEGAL_STATE)?;
vm.callbacks.notify_payload_started(cid);
let vm_start_timestamp = vm.vm_metric.lock().unwrap().start_timestamp;
@@ -1222,10 +1180,7 @@
Ok(())
} else {
error!("notifyPayloadStarted is called from an unknown CID {}", cid);
- Err(Status::new_service_specific_error_str(
- -1,
- Some(format!("cannot find a VM with CID {}", cid)),
- ))
+ Err(anyhow!("cannot find a VM with CID {}", cid)).or_service_specific_exception(-1)
}
}
@@ -1233,17 +1188,13 @@
let cid = self.cid;
if let Some(vm) = self.state.lock().unwrap().get_vm(cid) {
info!("VM with CID {} reported payload is ready", cid);
- vm.update_payload_state(PayloadState::Ready).map_err(|e| {
- Status::new_exception_str(ExceptionCode::ILLEGAL_STATE, Some(e.to_string()))
- })?;
+ vm.update_payload_state(PayloadState::Ready)
+ .or_binder_exception(ExceptionCode::ILLEGAL_STATE)?;
vm.callbacks.notify_payload_ready(cid);
Ok(())
} else {
error!("notifyPayloadReady is called from an unknown CID {}", cid);
- Err(Status::new_service_specific_error_str(
- -1,
- Some(format!("cannot find a VM with CID {}", cid)),
- ))
+ Err(anyhow!("cannot find a VM with CID {}", cid)).or_service_specific_exception(-1)
}
}
@@ -1251,17 +1202,13 @@
let cid = self.cid;
if let Some(vm) = self.state.lock().unwrap().get_vm(cid) {
info!("VM with CID {} finished payload", cid);
- vm.update_payload_state(PayloadState::Finished).map_err(|e| {
- Status::new_exception_str(ExceptionCode::ILLEGAL_STATE, Some(e.to_string()))
- })?;
+ vm.update_payload_state(PayloadState::Finished)
+ .or_binder_exception(ExceptionCode::ILLEGAL_STATE)?;
vm.callbacks.notify_payload_finished(cid, exit_code);
Ok(())
} else {
error!("notifyPayloadFinished is called from an unknown CID {}", cid);
- Err(Status::new_service_specific_error_str(
- -1,
- Some(format!("cannot find a VM with CID {}", cid)),
- ))
+ Err(anyhow!("cannot find a VM with CID {}", cid)).or_service_specific_exception(-1)
}
}
@@ -1269,17 +1216,13 @@
let cid = self.cid;
if let Some(vm) = self.state.lock().unwrap().get_vm(cid) {
info!("VM with CID {} encountered an error", cid);
- vm.update_payload_state(PayloadState::Finished).map_err(|e| {
- Status::new_exception_str(ExceptionCode::ILLEGAL_STATE, Some(e.to_string()))
- })?;
+ vm.update_payload_state(PayloadState::Finished)
+ .or_binder_exception(ExceptionCode::ILLEGAL_STATE)?;
vm.callbacks.notify_error(cid, error_code, message);
Ok(())
} else {
error!("notifyError is called from an unknown CID {}", cid);
- Err(Status::new_service_specific_error_str(
- -1,
- Some(format!("cannot find a VM with CID {}", cid)),
- ))
+ Err(anyhow!("cannot find a VM with CID {}", cid)).or_service_specific_exception(-1)
}
}
@@ -1287,10 +1230,8 @@
let cid = self.cid;
let Some(vm) = self.state.lock().unwrap().get_vm(cid) else {
error!("requestCertificate is called from an unknown CID {cid}");
- return Err(Status::new_service_specific_error_str(
- -1,
- Some(format!("cannot find a VM with CID {}", cid)),
- ));
+ return Err(anyhow!("cannot find a VM with CID {}", cid))
+ .or_service_specific_exception(-1);
};
let instance_img_path = vm.temporary_directory.join("rkpvm_instance.img");
let instance_img = OpenOptions::new()
@@ -1298,13 +1239,9 @@
.read(true)
.write(true)
.open(instance_img_path)
- .map_err(|e| {
- error!("Failed to create rkpvm_instance.img file: {:?}", e);
- Status::new_service_specific_error_str(
- -1,
- Some(format!("Failed to create rkpvm_instance.img file: {:?}", e)),
- )
- })?;
+ .context("Failed to create rkpvm_instance.img file")
+ .with_log()
+ .or_service_specific_exception(-1)?;
GLOBAL_SERVICE.requestCertificate(csr, &ParcelFileDescriptor::new(instance_img))
}
}