Skip to content

Commit 805675a

Browse files
Manciukicroypat
authored andcommitted
refactor(test/cgroup): use a TempDir instead of a manually created dir
The test was reimplementing the logic for creating a temporary directory instead of using TempDir, so I've changed it to simplify it. Also, save the PathBuf object instead of the String to be able to do Path operations in a canonical way without formatting strings. Signed-off-by: Riccardo Mancini <mancio@amazon.com>
1 parent 245057f commit 805675a

File tree

2 files changed

+76
-74
lines changed

2 files changed

+76
-74
lines changed

src/jailer/src/cgroup.rs

Lines changed: 47 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -504,13 +504,15 @@ pub mod test_util {
504504
use std::io::Write;
505505
use std::path::{Path, PathBuf};
506506

507-
use vmm_sys_util::rand;
507+
use vmm_sys_util::tempdir::TempDir;
508508

509509
#[derive(Debug)]
510510
pub struct MockCgroupFs {
511511
mounts_file: File,
512-
pub proc_mounts_path: String,
513-
pub sys_cgroups_path: String,
512+
// kept to clean up on Drop
513+
_mock_jailer_dir: TempDir,
514+
pub proc_mounts_path: PathBuf,
515+
pub sys_cgroups_path: PathBuf,
514516
}
515517

516518
// Helper object that simulates the layout of the cgroup file system
@@ -533,17 +535,12 @@ pub mod test_util {
533535
}
534536

535537
pub fn new() -> std::result::Result<MockCgroupFs, std::io::Error> {
536-
let mock_jailer_dir = format!(
537-
"/tmp/firecracker/test/{}/jailer",
538-
rand::rand_alphanumerics(4).into_string().unwrap()
539-
);
540-
let mock_proc_mounts = format!("{}/{}", mock_jailer_dir, "proc/mounts",);
541-
let mock_sys_cgroups = format!("{}/{}", mock_jailer_dir, "sys_cgroup",);
542-
543-
let mock_proc_dir = Path::new(&mock_proc_mounts).parent().unwrap();
538+
let mock_jailer_dir = TempDir::new().unwrap();
539+
let mock_proc_mounts = mock_jailer_dir.as_path().join("proc/mounts");
540+
let mock_sys_cgroups = mock_jailer_dir.as_path().join("sys_cgroup");
544541

545542
// create a mock /proc/mounts file in a temporary directory
546-
fs::create_dir_all(mock_proc_dir)?;
543+
fs::create_dir_all(mock_proc_mounts.parent().unwrap())?;
547544
let file = OpenOptions::new()
548545
.read(true)
549546
.write(true)
@@ -552,6 +549,7 @@ pub mod test_util {
552549
.open(mock_proc_mounts.clone())?;
553550
Ok(MockCgroupFs {
554551
mounts_file: file,
552+
_mock_jailer_dir: mock_jailer_dir,
555553
proc_mounts_path: mock_proc_mounts,
556554
sys_cgroups_path: mock_sys_cgroups,
557555
})
@@ -563,9 +561,9 @@ pub mod test_util {
563561
writeln!(
564562
self.mounts_file,
565563
"cgroupv2 {}/unified cgroup2 rw,nosuid,nodev,noexec,relatime,nsdelegate 0 0",
566-
self.sys_cgroups_path,
564+
self.sys_cgroups_path.to_str().unwrap(),
567565
)?;
568-
let cg_unified_path = PathBuf::from(format!("{}/unified", self.sys_cgroups_path));
566+
let cg_unified_path = self.sys_cgroups_path.join("unified");
569567
fs::create_dir_all(&cg_unified_path)?;
570568
Self::create_file_with_contents(
571569
cg_unified_path.join("cgroup.controllers"),
@@ -589,26 +587,14 @@ pub mod test_util {
589587
writeln!(
590588
self.mounts_file,
591589
"cgroup {}/{} cgroup rw,nosuid,nodev,noexec,relatime,{} 0 0",
592-
self.sys_cgroups_path, c, c,
590+
self.sys_cgroups_path.to_str().unwrap(),
591+
c,
592+
c,
593593
)?;
594594
}
595595
Ok(())
596596
}
597597
}
598-
599-
// Cleanup created files when object goes out of scope
600-
impl Drop for MockCgroupFs {
601-
fn drop(&mut self) {
602-
let tmp_dir = Path::new(self.proc_mounts_path.as_str())
603-
.parent()
604-
.unwrap()
605-
.parent()
606-
.unwrap()
607-
.parent()
608-
.unwrap();
609-
let _ = fs::remove_dir_all(tmp_dir);
610-
}
611-
}
612598
}
613599

614600
#[cfg(test)]
@@ -639,53 +625,60 @@ mod tests {
639625
#[test]
640626
fn test_cgroup_conf_builder_invalid_version() {
641627
let mock_cgroups = MockCgroupFs::new().unwrap();
642-
let builder = CgroupConfigurationBuilder::new(0, mock_cgroups.proc_mounts_path.as_str());
628+
let builder =
629+
CgroupConfigurationBuilder::new(0, mock_cgroups.proc_mounts_path.to_str().unwrap());
643630
builder.unwrap_err();
644631
}
645632

646633
#[test]
647634
fn test_cgroup_conf_builder_no_mounts() {
648635
let mock_cgroups = MockCgroupFs::new().unwrap();
649-
let builder = CgroupConfigurationBuilder::new(1, mock_cgroups.proc_mounts_path.as_str());
636+
let builder =
637+
CgroupConfigurationBuilder::new(1, mock_cgroups.proc_mounts_path.to_str().unwrap());
650638
builder.unwrap_err();
651639
}
652640

653641
#[test]
654642
fn test_cgroup_conf_builder_v1() {
655643
let mut mock_cgroups = MockCgroupFs::new().unwrap();
656644
mock_cgroups.add_v1_mounts().unwrap();
657-
let builder = CgroupConfigurationBuilder::new(1, mock_cgroups.proc_mounts_path.as_str());
645+
let builder =
646+
CgroupConfigurationBuilder::new(1, mock_cgroups.proc_mounts_path.to_str().unwrap());
658647
builder.unwrap();
659648
}
660649

661650
#[test]
662651
fn test_cgroup_conf_builder_v2() {
663652
let mut mock_cgroups = MockCgroupFs::new().unwrap();
664653
mock_cgroups.add_v2_mounts().unwrap();
665-
let builder = CgroupConfigurationBuilder::new(2, mock_cgroups.proc_mounts_path.as_str());
654+
let builder =
655+
CgroupConfigurationBuilder::new(2, mock_cgroups.proc_mounts_path.to_str().unwrap());
666656
builder.unwrap();
667657
}
668658

669659
#[test]
670660
fn test_cgroup_conf_builder_v2_with_v1_mounts() {
671661
let mut mock_cgroups = MockCgroupFs::new().unwrap();
672662
mock_cgroups.add_v1_mounts().unwrap();
673-
let builder = CgroupConfigurationBuilder::new(2, mock_cgroups.proc_mounts_path.as_str());
663+
let builder =
664+
CgroupConfigurationBuilder::new(2, mock_cgroups.proc_mounts_path.to_str().unwrap());
674665
builder.unwrap_err();
675666
}
676667

677668
#[test]
678669
fn test_cgroup_conf_builder_v2_no_mounts() {
679670
let mock_cgroups = MockCgroupFs::new().unwrap();
680-
let builder = CgroupConfigurationBuilder::new(2, mock_cgroups.proc_mounts_path.as_str());
671+
let builder =
672+
CgroupConfigurationBuilder::new(2, mock_cgroups.proc_mounts_path.to_str().unwrap());
681673
builder.unwrap_err();
682674
}
683675

684676
#[test]
685677
fn test_cgroup_conf_builder_v1_with_v2_mounts() {
686678
let mut mock_cgroups = MockCgroupFs::new().unwrap();
687679
mock_cgroups.add_v2_mounts().unwrap();
688-
let builder = CgroupConfigurationBuilder::new(1, mock_cgroups.proc_mounts_path.as_str());
680+
let builder =
681+
CgroupConfigurationBuilder::new(1, mock_cgroups.proc_mounts_path.to_str().unwrap());
689682
builder.unwrap_err();
690683
}
691684

@@ -696,9 +689,11 @@ mod tests {
696689
mock_cgroups.add_v2_mounts().unwrap();
697690

698691
for v in &[1, 2] {
699-
let mut builder =
700-
CgroupConfigurationBuilder::new(*v, mock_cgroups.proc_mounts_path.as_str())
701-
.unwrap();
692+
let mut builder = CgroupConfigurationBuilder::new(
693+
*v,
694+
mock_cgroups.proc_mounts_path.to_str().unwrap(),
695+
)
696+
.unwrap();
702697

703698
builder
704699
.add_cgroup_property(
@@ -719,9 +714,11 @@ mod tests {
719714
mock_cgroups.add_v2_mounts().unwrap();
720715

721716
for v in &[1, 2] {
722-
let mut builder =
723-
CgroupConfigurationBuilder::new(*v, mock_cgroups.proc_mounts_path.as_str())
724-
.unwrap();
717+
let mut builder = CgroupConfigurationBuilder::new(
718+
*v,
719+
mock_cgroups.proc_mounts_path.to_str().unwrap(),
720+
)
721+
.unwrap();
725722
builder
726723
.add_cgroup_property(
727724
"invalid.cg".to_string(),
@@ -739,7 +736,8 @@ mod tests {
739736
mock_cgroups.add_v1_mounts().unwrap();
740737

741738
let mut builder =
742-
CgroupConfigurationBuilder::new(1, mock_cgroups.proc_mounts_path.as_str()).unwrap();
739+
CgroupConfigurationBuilder::new(1, mock_cgroups.proc_mounts_path.to_str().unwrap())
740+
.unwrap();
743741
builder
744742
.add_cgroup_property(
745743
"cpuset.mems".to_string(),
@@ -750,7 +748,7 @@ mod tests {
750748
.unwrap();
751749
let cg_conf = builder.build();
752750

753-
let cg_root = PathBuf::from(format!("{}/cpuset", mock_cgroups.sys_cgroups_path));
751+
let cg_root = mock_cgroups.sys_cgroups_path.join("cpuset");
754752

755753
// with real cgroups these files are created automatically
756754
// since the mock will not do it automatically, we create it here
@@ -773,11 +771,13 @@ mod tests {
773771
fn test_cgroup_conf_v2_write_value() {
774772
let mut mock_cgroups = MockCgroupFs::new().unwrap();
775773
mock_cgroups.add_v2_mounts().unwrap();
776-
let builder = CgroupConfigurationBuilder::new(2, mock_cgroups.proc_mounts_path.as_str());
774+
let builder =
775+
CgroupConfigurationBuilder::new(2, mock_cgroups.proc_mounts_path.to_str().unwrap());
777776
builder.unwrap();
778777

779778
let mut builder =
780-
CgroupConfigurationBuilder::new(2, mock_cgroups.proc_mounts_path.as_str()).unwrap();
779+
CgroupConfigurationBuilder::new(2, mock_cgroups.proc_mounts_path.to_str().unwrap())
780+
.unwrap();
781781
builder
782782
.add_cgroup_property(
783783
"cpuset.mems".to_string(),
@@ -787,7 +787,7 @@ mod tests {
787787
)
788788
.unwrap();
789789

790-
let cg_root = PathBuf::from(format!("{}/unified", mock_cgroups.sys_cgroups_path));
790+
let cg_root = mock_cgroups.sys_cgroups_path.join("unified");
791791

792792
assert_eq!(builder.get_v2_hierarchy_path().unwrap(), &cg_root);
793793

0 commit comments

Comments
 (0)