From dd68b191b6d1ff86d35e7b92da2e8f87d0cad65f Mon Sep 17 00:00:00 2001 From: Jiang Liu Date: Thu, 2 Mar 2023 19:30:03 +0800 Subject: [PATCH 1/2] nydus-image: simplify ArtifactWriter::new() to remove the `fifo` arg Simplify ArtifactWriter::new() to remove the argument `fifo`. We can detect whether a file is a FIFO or not, so need to pass flag for it. Signed-off-by: Jiang Liu --- src/bin/nydus-image/builder/directory.rs | 4 ++-- src/bin/nydus-image/builder/stargz.rs | 4 ++-- src/bin/nydus-image/builder/tarball.rs | 4 ++-- src/bin/nydus-image/core/blob_compact.rs | 4 ++-- src/bin/nydus-image/core/context.rs | 29 ++++++++++++------------ src/bin/nydus-image/core/node.rs | 2 +- src/bin/nydus-image/merge.rs | 2 +- 7 files changed, 24 insertions(+), 25 deletions(-) diff --git a/src/bin/nydus-image/builder/directory.rs b/src/bin/nydus-image/builder/directory.rs index 921fadb8296..1f1ba72529b 100644 --- a/src/bin/nydus-image/builder/directory.rs +++ b/src/bin/nydus-image/builder/directory.rs @@ -117,10 +117,10 @@ impl Builder for DirectoryBuilder { bootstrap_mgr: &mut BootstrapManager, blob_mgr: &mut BlobManager, ) -> Result { - let mut bootstrap_ctx = bootstrap_mgr.create_ctx(ctx.blob_inline_meta)?; + let mut bootstrap_ctx = bootstrap_mgr.create_ctx()?; let layer_idx = u16::from(bootstrap_ctx.layered); let mut blob_writer = if let Some(blob_stor) = ctx.blob_storage.clone() { - ArtifactWriter::new(blob_stor, ctx.blob_inline_meta)? + ArtifactWriter::new(blob_stor)? } else { return Err(anyhow!( "target blob path should always be valid for directory builder" diff --git a/src/bin/nydus-image/builder/stargz.rs b/src/bin/nydus-image/builder/stargz.rs index 82b9038cd4a..786f1268587 100644 --- a/src/bin/nydus-image/builder/stargz.rs +++ b/src/bin/nydus-image/builder/stargz.rs @@ -811,10 +811,10 @@ impl Builder for StargzBuilder { bootstrap_mgr: &mut BootstrapManager, blob_mgr: &mut BlobManager, ) -> Result { - let mut bootstrap_ctx = bootstrap_mgr.create_ctx(ctx.blob_inline_meta)?; + let mut bootstrap_ctx = bootstrap_mgr.create_ctx()?; let layer_idx = u16::from(bootstrap_ctx.layered); let mut blob_writer = if let Some(blob_stor) = ctx.blob_storage.clone() { - Some(ArtifactWriter::new(blob_stor, ctx.blob_inline_meta)?) + Some(ArtifactWriter::new(blob_stor)?) } else { return Err(anyhow!("missing configuration for target path")); }; diff --git a/src/bin/nydus-image/builder/tarball.rs b/src/bin/nydus-image/builder/tarball.rs index 86741c0f200..2c52b652713 100644 --- a/src/bin/nydus-image/builder/tarball.rs +++ b/src/bin/nydus-image/builder/tarball.rs @@ -549,7 +549,7 @@ impl Builder for TarballBuilder { bootstrap_mgr: &mut BootstrapManager, blob_mgr: &mut BlobManager, ) -> Result { - let mut bootstrap_ctx = bootstrap_mgr.create_ctx(ctx.blob_inline_meta)?; + let mut bootstrap_ctx = bootstrap_mgr.create_ctx()?; let layer_idx = u16::from(bootstrap_ctx.layered); let mut blob_writer = match self.ty { ConversionType::EStargzToRafs @@ -558,7 +558,7 @@ impl Builder for TarballBuilder { | ConversionType::EStargzToRef | ConversionType::TargzToRef => { if let Some(blob_stor) = ctx.blob_storage.clone() { - ArtifactWriter::new(blob_stor, ctx.blob_inline_meta)? + ArtifactWriter::new(blob_stor)? } else { return Err(anyhow!("missing configuration for target path")); } diff --git a/src/bin/nydus-image/core/blob_compact.rs b/src/bin/nydus-image/core/blob_compact.rs index ca894d5185a..ca190b45c16 100644 --- a/src/bin/nydus-image/core/blob_compact.rs +++ b/src/bin/nydus-image/core/blob_compact.rs @@ -129,7 +129,7 @@ impl ChunkSet { aligned_chunk: bool, backend: &Arc, ) -> Result> { - let mut blob_writer = ArtifactWriter::new(blob_storage, build_ctx.blob_inline_meta)?; + let mut blob_writer = ArtifactWriter::new(blob_storage)?; // sort chunks first, don't break order in original blobs let mut chunks = self.chunks.values().collect::>(); @@ -578,7 +578,7 @@ impl BlobCompactor { ); let mut bootstrap_mgr = BootstrapManager::new(Some(ArtifactStorage::SingleFile(d_bootstrap)), None); - let mut bootstrap_ctx = bootstrap_mgr.create_ctx(false)?; + let mut bootstrap_ctx = bootstrap_mgr.create_ctx()?; let mut ori_blob_mgr = BlobManager::new(rs.meta.get_digester()); ori_blob_mgr.extend_from_blob_table(&build_ctx, rs.superblock.get_blob_infos())?; if let Some(dict) = chunk_dict { diff --git a/src/bin/nydus-image/core/context.rs b/src/bin/nydus-image/core/context.rs index 18645227c91..685cb28bd6e 100644 --- a/src/bin/nydus-image/core/context.rs +++ b/src/bin/nydus-image/core/context.rs @@ -8,12 +8,13 @@ use std::any::Any; use std::borrow::Cow; use std::collections::{HashMap, VecDeque}; use std::convert::TryFrom; -use std::fmt; use std::fs::{remove_file, rename, File, OpenOptions}; use std::io::{BufWriter, Cursor, Read, Seek, Write}; +use std::os::unix::fs::FileTypeExt; use std::path::{Display, Path, PathBuf}; use std::str::FromStr; use std::sync::{Arc, Mutex}; +use std::{fmt, fs}; use anyhow::{Context, Error, Result}; use sha2::{Digest, Sha256}; @@ -245,15 +246,18 @@ impl Write for ArtifactWriter { } impl ArtifactWriter { - pub fn new(storage: ArtifactStorage, fifo: bool) -> Result { + pub fn new(storage: ArtifactStorage) -> Result { match storage { ArtifactStorage::SingleFile(ref p) => { let mut opener = &mut OpenOptions::new(); opener = opener.write(true).create(true); - // Make it as the writer side of FIFO file, no truncate flag because it has - // been created by the reader side. - if !fifo { - opener = opener.truncate(true); + if let Ok(md) = fs::metadata(p) { + let ty = md.file_type(); + // Make it as the writer side of FIFO file, no truncate flag because it has + // been created by the reader side. + if !ty.is_fifo() { + opener = opener.truncate(true); + } } let b = BufWriter::with_capacity( BUF_WRITER_CAPACITY, @@ -938,10 +942,9 @@ pub struct BootstrapContext { } impl BootstrapContext { - pub fn new(storage: Option, layered: bool, fifo: bool) -> Result { + pub fn new(storage: Option, layered: bool) -> Result { let writer = if let Some(storage) = storage { - Box::new(ArtifactFileWriter(ArtifactWriter::new(storage, fifo)?)) - as Box + Box::new(ArtifactFileWriter(ArtifactWriter::new(storage)?)) as Box } else { Box::::default() as Box }; @@ -1013,12 +1016,8 @@ impl BootstrapManager { } } - pub fn create_ctx(&self, fifo: bool) -> Result { - BootstrapContext::new( - self.bootstrap_storage.clone(), - self.f_parent_path.is_some(), - fifo, - ) + pub fn create_ctx(&self) -> Result { + BootstrapContext::new(self.bootstrap_storage.clone(), self.f_parent_path.is_some()) } } diff --git a/src/bin/nydus-image/core/node.rs b/src/bin/nydus-image/core/node.rs index 4587024e40d..47028734760 100644 --- a/src/bin/nydus-image/core/node.rs +++ b/src/bin/nydus-image/core/node.rs @@ -1469,7 +1469,7 @@ mod tests { let bootstrap_path = TempFile::new().unwrap(); let storage = ArtifactStorage::SingleFile(bootstrap_path.as_path().to_path_buf()); - let mut bootstrap_ctx = BootstrapContext::new(Some(storage), false, false).unwrap(); + let mut bootstrap_ctx = BootstrapContext::new(Some(storage), false).unwrap(); bootstrap_ctx.offset = 0; // reg file. diff --git a/src/bin/nydus-image/merge.rs b/src/bin/nydus-image/merge.rs index f2b62f933cf..01ecfd174b4 100644 --- a/src/bin/nydus-image/merge.rs +++ b/src/bin/nydus-image/merge.rs @@ -254,7 +254,7 @@ impl Merger { ctx.chunk_size = chunk_size; } - let mut bootstrap_ctx = BootstrapContext::new(Some(target.clone()), false, false)?; + let mut bootstrap_ctx = BootstrapContext::new(Some(target.clone()), false)?; let mut bootstrap = Bootstrap::new()?; bootstrap.build(ctx, &mut bootstrap_ctx, &mut tree)?; let blob_table = blob_mgr.to_blob_table(ctx)?; From b217101701adc04466fb12a51d7741fb489ac37d Mon Sep 17 00:00:00 2001 From: Jiang Liu Date: Fri, 3 Mar 2023 09:28:32 +0800 Subject: [PATCH 2/2] nydus-image: minor improvement to nydus-image Minor improvement to nydus-image: - better handling of `chunk-size` argument - avoid assert at runtime by returning error code Signed-off-by: Jiang Liu --- src/bin/nydus-image/builder/tarball.rs | 23 ++++++++++++++--------- src/bin/nydus-image/main.rs | 12 +++++++----- 2 files changed, 21 insertions(+), 14 deletions(-) diff --git a/src/bin/nydus-image/builder/tarball.rs b/src/bin/nydus-image/builder/tarball.rs index 2c52b652713..17153d9d502 100644 --- a/src/bin/nydus-image/builder/tarball.rs +++ b/src/bin/nydus-image/builder/tarball.rs @@ -149,7 +149,7 @@ impl<'a> TarballTreeBuilder<'a> { // Generate the root node in advance, it may be overwritten by entries from the tar stream. let mut nodes = Vec::with_capacity(10240); - let root = self.create_directory(Path::new("/"), &nodes)?; + let root = self.create_directory(Path::new("/"), 0)?; nodes.push(root.clone()); // Generate RAFS node for each tar entry, and optionally adding missing parents. @@ -192,10 +192,13 @@ impl<'a> TarballTreeBuilder<'a> { ) -> Result { let header = entry.header(); let entry_type = header.entry_type(); - assert!(!entry_type.is_gnu_longname()); - assert!(!entry_type.is_gnu_longlink()); - assert!(!entry_type.is_pax_local_extensions()); - if entry_type.is_pax_global_extensions() { + if entry_type.is_gnu_longname() { + return Err(anyhow!("unsupported gnu_longname from tar header")); + } else if entry_type.is_gnu_longlink() { + return Err(anyhow!("unsupported gnu_longlink from tar header")); + } else if entry_type.is_pax_local_extensions() { + return Err(anyhow!("unsupported pax_local_extensions from tar header")); + } else if entry_type.is_pax_global_extensions() { return Err(anyhow!("unsupported pax_global_extensions from tar header")); } else if entry_type.is_contiguous() { return Err(anyhow!("unsupported contiguous entry type from tar header")); @@ -455,7 +458,7 @@ impl<'a> TarballTreeBuilder<'a> { if let Some(parent_path) = path.as_ref().parent() { if !self.path_inode_map.contains_key(parent_path) { self.make_lost_dirs(parent_path, nodes)?; - let node = self.create_directory(parent_path, nodes)?; + let node = self.create_directory(parent_path, nodes.len())?; nodes.push(node); } } @@ -463,7 +466,7 @@ impl<'a> TarballTreeBuilder<'a> { Ok(()) } - fn create_directory(&mut self, path: &Path, nodes: &[Node]) -> Result { + fn create_directory(&mut self, path: &Path, nodes_index: usize) -> Result { let ino = (self.path_inode_map.len() + 1) as Inode; let name = Self::get_file_name(path)?; let mut inode = InodeWrapper::new(self.ctx.fs_version); @@ -502,7 +505,7 @@ impl<'a> TarballTreeBuilder<'a> { }; self.path_inode_map - .insert(path.to_path_buf(), (ino, nodes.len())); + .insert(path.to_path_buf(), (ino, nodes_index)); Ok(node) } @@ -530,11 +533,13 @@ impl<'a> TarballTreeBuilder<'a> { } } -pub(crate) struct TarballBuilder { +/// Builder to create RAFS filesystems from tarballs. +pub struct TarballBuilder { ty: ConversionType, } impl TarballBuilder { + /// Create a new instance of [TarballBuilder] to build a RAFS filesystem from a tarball. pub fn new(conversion_type: ConversionType) -> Self { Self { ty: conversion_type, diff --git a/src/bin/nydus-image/main.rs b/src/bin/nydus-image/main.rs index 10917e26d83..05463b6e80d 100644 --- a/src/bin/nydus-image/main.rs +++ b/src/bin/nydus-image/main.rs @@ -869,10 +869,9 @@ impl Command { )?; // Some operations like listing xattr pairs of certain namespace need the process - // to be privileged. Therefore, trace what euid and egid are + // to be privileged. Therefore, trace what euid and egid are. event_tracer!("euid", "{}", geteuid()); event_tracer!("egid", "{}", getegid()); - info!("successfully built RAFS filesystem: \n{}", build_output); OutputSerializer::dump(matches, build_output, build_info) } @@ -1284,9 +1283,12 @@ impl Command { } } Some(v) => { - let param = v.trim_start_matches("0x").trim_start_matches("0X"); - let chunk_size = - u32::from_str_radix(param, 16).context(format!("invalid chunk size {}", v))?; + let chunk_size = if v.starts_with("0x") || v.starts_with("0X") { + u32::from_str_radix(&v[2..], 16).context(format!("invalid chunk size {}", v))? + } else { + v.parse::() + .context(format!("invalid chunk size {}", v))? + }; if chunk_size as u64 > RAFS_MAX_CHUNK_SIZE || chunk_size < 0x1000 || !chunk_size.is_power_of_two()