Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

do not log error in the syscall crate #1973

Merged
merged 6 commits into from
Jun 5, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion crates/libcontainer/src/process/container_init_process.rs
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,6 @@ fn masked_path(path: &Path, mount_label: &Option<String>, syscall: &dyn Syscall)
match err {
SyscallError::Nix(nix::errno::Errno::ENOENT) => {
// ignore error if path is not exist.
tracing::warn!("masked path {:?} not exist", path);
}
SyscallError::Nix(nix::errno::Errno::ENOTDIR) => {
let label = match mount_label {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,10 @@ pub fn container_intermediate_process(
let proc = spec.process().as_ref().ok_or(MissingSpecError::Process)?;
if let Some(rlimits) = proc.rlimits() {
for rlimit in rlimits {
command.set_rlimit(rlimit)?;
command.set_rlimit(rlimit).map_err(|err| {
tracing::error!(?err, ?rlimit, "failed to set rlimit");
err
})?;
}
}

Expand Down
52 changes: 38 additions & 14 deletions crates/libcontainer/src/rootfs/device.rs
Original file line number Diff line number Diff line change
Expand Up @@ -102,9 +102,9 @@ impl Device {
)
.map_err(|err| {
tracing::error!(
"failed to mount bind dev {:?}: {}",
full_container_path,
err
?err,
path = ?full_container_path,
"failed to mount bind dev",
);
err
})?;
Expand All @@ -122,17 +122,41 @@ impl Device {

let full_container_path = create_container_dev_path(rootfs, dev)?;

self.syscall.mknod(
&full_container_path,
to_sflag(dev.typ()),
Mode::from_bits_truncate(dev.file_mode().unwrap_or(0)),
makedev(dev.major(), dev.minor()),
)?;
self.syscall.chown(
&full_container_path,
dev.uid().map(Uid::from_raw),
dev.gid().map(Gid::from_raw),
)?;
self.syscall
.mknod(
&full_container_path,
to_sflag(dev.typ()),
Mode::from_bits_truncate(dev.file_mode().unwrap_or(0)),
makedev(dev.major(), dev.minor()),
)
.map_err(|err| {
tracing::error!(
?err,
path = ?full_container_path,
major = ?dev.major(),
minor = ?dev.minor(),
"failed to mknod device"
);

err
})?;
self.syscall
.chown(
&full_container_path,
dev.uid().map(Uid::from_raw),
dev.gid().map(Gid::from_raw),
)
.map_err(|err| {
tracing::error!(
path = ?full_container_path,
?err,
uid = ?dev.uid(),
gid = ?dev.gid(),
"failed to chown device"
);

err
})?;

Ok(())
}
Expand Down
43 changes: 32 additions & 11 deletions crates/libcontainer/src/rootfs/rootfs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ impl RootFS {
bind_devices: bool,
cgroup_ns: bool,
) -> Result<()> {
tracing::debug!("Prepare rootfs: {:?}", rootfs);
tracing::debug!(?rootfs, "prepare rootfs");
let mut flags = MsFlags::MS_REC;
let linux = spec.linux().as_ref().ok_or(MissingSpecError::Linux)?;

Expand All @@ -52,20 +52,34 @@ impl RootFS {
}

self.syscall
.mount(None, Path::new("/"), None, flags, None)?;
.mount(None, Path::new("/"), None, flags, None)
.map_err(|err| {
tracing::error!(
?err,
?flags,
"failed to change the mount propagation type of the root"
);

err
})?;

let mounter = Mount::new();

mounter.make_parent_mount_private(rootfs)?;

tracing::debug!("mount root fs {:?}", rootfs);
self.syscall.mount(
Some(rootfs),
rootfs,
None,
MsFlags::MS_BIND | MsFlags::MS_REC,
None,
)?;
self.syscall
.mount(
Some(rootfs),
rootfs,
None,
MsFlags::MS_BIND | MsFlags::MS_REC,
None,
)
.map_err(|err| {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example, this call to syscall::mount is inside the rootfs module. Here, mount call failure indicates an actual failure, so we log the error here.

tracing::error!(?rootfs, ?err, "failed to bind mount rootfs");
err
})?;

let global_options = MountOptions {
root: rootfs,
Expand Down Expand Up @@ -108,9 +122,16 @@ impl RootFS {
};

if let Some(flags) = flags {
tracing::debug!("make root mount {:?}", flags);
self.syscall
.mount(None, Path::new("/"), None, flags, None)?;
.mount(None, Path::new("/"), None, flags, None)
.map_err(|err| {
tracing::error!(
?err,
?flags,
"failed to adjust the mount propagation type of the root"
);
err
})?;
}

Ok(())
Expand Down
71 changes: 13 additions & 58 deletions crates/libcontainer/src/syscall/linux.rs
Original file line number Diff line number Diff line change
Expand Up @@ -349,14 +349,10 @@ impl Syscall for LinuxSyscall {
/// Disassociate parts of execution context
// see https://man7.org/linux/man-pages/man2/unshare.2.html for more information
fn unshare(&self, flags: CloneFlags) -> Result<()> {
unshare(flags).map_err(|err| {
tracing::error!(?err, ?flags, "failed to unshare");
err
})?;
unshare(flags)?;

Ok(())
}

/// Set capabilities for container process
fn set_capability(&self, cset: CapSet, value: &CapsHashSet) -> Result<()> {
match cset {
Expand All @@ -382,10 +378,7 @@ impl Syscall for LinuxSyscall {

/// Sets hostname for process
fn set_hostname(&self, hostname: &str) -> Result<()> {
sethostname(hostname).map_err(|err| {
tracing::error!(?hostname, "failed to set hostname");
err
})?;
sethostname(hostname)?;
Ok(())
}

Expand All @@ -396,15 +389,9 @@ impl Syscall for LinuxSyscall {
let len = domainname.len();
match unsafe { setdomainname(ptr, len) } {
0 => Ok(()),
-1 => {
tracing::error!(?domainname, "failed to set domainname");
Err(nix::Error::last())
}
-1 => Err(nix::Error::last()),

_ => {
tracing::error!(?domainname, "failed to set domainname for unknown reason");
Err(nix::Error::UnknownErrno)
}
_ => Err(nix::Error::UnknownErrno),
}?;

Ok(())
Expand All @@ -425,14 +412,8 @@ impl Syscall for LinuxSyscall {

match res {
0 => Ok(()),
-1 => {
tracing::error!(?res, rlimit = ?rlimit.typ(), "failed to set rlimit");
Err(SyscallError::Nix(nix::Error::last()))
}
_ => {
tracing::error!(?res, rlimit = ?rlimit.typ(), "failed to set rlimit for unknown reason");
Err(SyscallError::Nix(nix::Error::UnknownErrno))
}
-1 => Err(SyscallError::Nix(nix::Error::last())),
_ => Err(SyscallError::Nix(nix::Error::UnknownErrno)),
}?;

Ok(())
Expand Down Expand Up @@ -473,10 +454,7 @@ impl Syscall for LinuxSyscall {
}

fn chroot(&self, path: &Path) -> Result<()> {
unistd::chroot(path).map_err(|err| {
tracing::error!(?err, ?path, "failed to chroot");
err
})?;
unistd::chroot(path)?;

Ok(())
}
Expand All @@ -489,41 +467,24 @@ impl Syscall for LinuxSyscall {
flags: MsFlags,
data: Option<&str>,
) -> Result<()> {
mount(source, target, fstype, flags, data).map_err(|err| {
tracing::error!(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any need to bring this tracing to the callers?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, 47bc859

"failed to mount {source:?} to {target:?} with fstype {fstype:?}, flags {flags:?}, data {data:?}: {err}",
);
err
})?;

mount(source, target, fstype, flags, data)?;
Ok(())
}

fn symlink(&self, original: &Path, link: &Path) -> Result<()> {
symlink(original, link).map_err(|err| {
tracing::error!("failed to create symlink from {original:?} to {link:?}: {err}");
err
})?;
symlink(original, link)?;

Ok(())
}

fn mknod(&self, path: &Path, kind: SFlag, perm: Mode, dev: u64) -> Result<()> {
mknod(path, kind, perm, dev).map_err(|errno| {
tracing::error!(
"failed to mknod {path:?}, kind {kind:?}, perm {perm:?}, dev {dev:?}: {errno}"
);
errno
})?;
mknod(path, kind, perm, dev)?;

Ok(())
}

fn chown(&self, path: &Path, owner: Option<Uid>, group: Option<Gid>) -> Result<()> {
chown(path, owner, group).map_err(|errno| {
tracing::error!("failed to chown {path:?} to {owner:?}:{group:?}: {errno}");
errno
})?;
chown(path, owner, group)?;

Ok(())
}
Expand Down Expand Up @@ -552,16 +513,10 @@ impl Syscall for LinuxSyscall {
// kernel 5.11. If the kernel is older we emulate close_range in userspace.
Self::emulate_close_range(preserve_fds)
}
e => {
tracing::error!(errno = ?e, "failed to close_range");
Err(SyscallError::Nix(e))
}
e => Err(SyscallError::Nix(e)),
}
}
_ => {
tracing::error!("failed to close_range unknown failure");
Err(SyscallError::Nix(nix::errno::Errno::UnknownErrno))
}
_ => Err(SyscallError::Nix(nix::errno::Errno::UnknownErrno)),
}?;

Ok(())
Expand Down
2 changes: 1 addition & 1 deletion rust-toolchain.toml
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
[toolchain]
profile="default"
channel="1.69.0"
channel="1.70.0"
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
///! Contains utility functions for testing
///! Similar to https://github.com/opencontainers/runtime-tools/blob/master/validation/util/test.go
//! Contains utility functions for testing
//! Similar to https://github.com/opencontainers/runtime-tools/blob/master/validation/util/test.go
use super::{generate_uuid, prepare_bundle, set_config};
use super::{get_runtime_path, get_runtimetest_path};
use anyhow::{anyhow, bail, Context, Result};
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
///! Contains definition for a tests which should be conditionally run
//! Contains definition for a tests which should be conditionally run
use crate::testable::{TestResult, Testable};

// type aliases for test function signature
Expand Down
2 changes: 1 addition & 1 deletion tests/rust-integration-tests/test_framework/src/test.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
///! Contains definition for a simple and commonly usable test structure
//! Contains definition for a simple and commonly usable test structure
use crate::testable::{TestResult, Testable};

// type alias for the test function
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
///! Contains structure for a test group
//! Contains structure for a test group
use crate::testable::{TestResult, Testable, TestableGroup};
use crossbeam::thread;
use std::collections::BTreeMap;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
///! This exposes the main control wrapper to control the tests
//! This exposes the main control wrapper to control the tests
use crate::testable::{TestResult, TestableGroup};
use anyhow::Result;
use crossbeam::thread;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
//! Contains Basic setup for testing, testable trait and its result type
use std::fmt::Debug;

///! Contains Basic setup for testing, testable trait and its result type
use anyhow::{bail, Error, Result};

#[derive(Debug)]
Expand Down