Additional patches for 9.4.0 dmpd.

Resolves: RHEL-26520 RHEL-26521
This commit is contained in:
Marian Csontos 2024-03-04 13:26:50 +01:00
parent afaf3fd452
commit 1c8142b753
5 changed files with 1672 additions and 1 deletions

View File

@ -0,0 +1,225 @@
From 6f7f70da8341cfe8447c6c70ebb4085629983b0d Mon Sep 17 00:00:00 2001
From: Ming-Hung Tsai <mtsai@redhat.com>
Date: Tue, 20 Feb 2024 04:16:46 +0800
Subject: [PATCH 07/10] [thin_dump] Do not print error messages on BrokenPipe
(EPIPE)
Considering BrokenPipe is an expected result in most use cases,
thin_dump should not print error messages in this situation but should
return a non-zero exit code instead. For instance, lvconvert fetches
only the first line of thin_dump then closes the pipe immediately,
causing BrokenPipe to terminate thin_dump intentionally.
Changes made include removing the unused NodeVisitor abstraction for
MappingVisitor to keep the error type from low-level io, and printing
messages based on the error type returned upward (bz2233177).
(cherry picked from commit d7b91f9f7e9e4118f1f47df19d81fc9bc6965ec5)
---
src/commands/utils.rs | 16 ++++++---
src/thin/dump.rs | 31 +++---------------
tests/common/process.rs | 2 +-
tests/thin_dump.rs | 72 ++++++++++++++++++++++++++++++++++++++++-
4 files changed, 89 insertions(+), 32 deletions(-)
diff --git a/src/commands/utils.rs b/src/commands/utils.rs
index 0be4fa7f..38751603 100644
--- a/src/commands/utils.rs
+++ b/src/commands/utils.rs
@@ -162,10 +162,18 @@ pub fn check_overwrite_metadata(report: &Report, path: &Path) -> Result<()> {
pub fn to_exit_code<T>(report: &Report, result: anyhow::Result<T>) -> exitcode::ExitCode {
if let Err(e) = result {
- if e.chain().len() > 1 {
- report.fatal(&format!("{}: {}", e, e.root_cause()));
- } else {
- report.fatal(&format!("{}", e));
+ let root_cause = e.root_cause();
+ let is_broken_pipe = root_cause
+ .downcast_ref::<Arc<std::io::Error>>() // quick_xml::Error::Io wraps io::Error in Arc
+ .map(|err| err.kind() == std::io::ErrorKind::BrokenPipe)
+ .unwrap_or(false);
+
+ if !is_broken_pipe {
+ if e.chain().len() > 1 {
+ report.fatal(&format!("{}: {}", e, root_cause));
+ } else {
+ report.fatal(&format!("{}", e));
+ }
}
// FIXME: we need a way of getting more meaningful error codes
diff --git a/src/thin/dump.rs b/src/thin/dump.rs
index f6046487..561ea566 100644
--- a/src/thin/dump.rs
+++ b/src/thin/dump.rs
@@ -10,8 +10,7 @@ use crate::checksum;
use crate::commands::engine::*;
use crate::dump_utils::*;
use crate::io_engine::*;
-use crate::pdata::btree::{self, *};
-use crate::pdata::btree_walker::*;
+use crate::pdata::btree::*;
use crate::pdata::space_map::common::*;
use crate::pdata::unpack::*;
use crate::report::*;
@@ -104,9 +103,7 @@ impl<'a> MappingVisitor<'a> {
}),
}
}
-}
-impl<'a> NodeVisitor<BlockTime> for MappingVisitor<'a> {
fn visit(
&self,
_path: &[u64],
@@ -114,39 +111,21 @@ impl<'a> NodeVisitor<BlockTime> for MappingVisitor<'a> {
_h: &NodeHeader,
keys: &[u64],
values: &[BlockTime],
- ) -> btree::Result<()> {
+ ) -> Result<()> {
let mut inner = self.inner.lock().unwrap();
for (k, v) in keys.iter().zip(values.iter()) {
if let Some(run) = inner.builder.next(*k, v.block, v.time) {
- // FIXME: BTreeError should carry more information than a string
- // so the caller could identify the actual root cause,
- // e.g., a broken pipe error or something.
- inner
- .md_out
- .map(&run)
- .map_err(|e| btree::value_err(format!("{}", e)))?;
+ inner.md_out.map(&run)?;
}
}
Ok(())
}
- fn visit_again(&self, _path: &[u64], b: u64) -> btree::Result<()> {
- let mut inner = self.inner.lock().unwrap();
- inner
- .md_out
- .ref_shared(&format!("{}", b))
- .map_err(|e| btree::value_err(format!("{}", e)))?;
- Ok(())
- }
-
- fn end_walk(&self) -> btree::Result<()> {
+ fn end_walk(&self) -> Result<()> {
let mut inner = self.inner.lock().unwrap();
if let Some(run) = inner.builder.complete() {
- inner
- .md_out
- .map(&run)
- .map_err(|e| btree::value_err(format!("{}", e)))?;
+ inner.md_out.map(&run)?;
}
Ok(())
}
diff --git a/tests/common/process.rs b/tests/common/process.rs
index 99f10dc7..ad26c38b 100644
--- a/tests/common/process.rs
+++ b/tests/common/process.rs
@@ -39,7 +39,7 @@ impl Command {
Command { program, args }
}
- fn to_expr(&self) -> duct::Expression {
+ pub fn to_expr(&self) -> duct::Expression {
duct::cmd(&self.program, &self.args)
}
}
diff --git a/tests/thin_dump.rs b/tests/thin_dump.rs
index 28cb237d..6e2cd3db 100644
--- a/tests/thin_dump.rs
+++ b/tests/thin_dump.rs
@@ -129,7 +129,7 @@ fn dump_restore_cycle() -> Result<()> {
// test no stderr with a normal dump
#[test]
-fn no_stderr() -> Result<()> {
+fn no_stderr_on_success() -> Result<()> {
let mut td = TestDir::new()?;
let md = mk_valid_md(&mut td)?;
@@ -139,6 +139,76 @@ fn no_stderr() -> Result<()> {
Ok(())
}
+//------------------------------------------
+// test no stderr on broken pipe errors
+
+#[test]
+fn no_stderr_on_broken_pipe() -> Result<()> {
+ use anyhow::ensure;
+
+ let mut td = TestDir::new()?;
+
+ // use the metadata producing dump more than 64KB (the default pipe buffer size),
+ // such that thin_dump will be blocked on writing to the pipe, then hits EPIPE.
+ let md = prep_metadata(&mut td)?;
+
+ let mut pipefd = [0i32; 2];
+ unsafe {
+ ensure!(libc::pipe2(pipefd.as_mut_slice().as_mut_ptr(), libc::O_CLOEXEC) == 0);
+ }
+
+ let cmd = thin_dump_cmd(args![&md])
+ .to_expr()
+ .stdout_file(pipefd[1])
+ .stderr_capture();
+ let handle = cmd.unchecked().start()?;
+
+ // wait for thin_dump to fill the pipe buffer
+ std::thread::sleep(std::time::Duration::from_millis(1000));
+
+ unsafe {
+ libc::close(pipefd[1]); // close the unused write-end
+ libc::close(pipefd[0]); // causing broken pipe
+ }
+
+ let output = handle.wait()?;
+ ensure!(!output.status.success());
+ ensure!(output.stderr.is_empty());
+
+ Ok(())
+}
+
+#[test]
+fn no_stderr_on_broken_fifo() -> Result<()> {
+ use anyhow::ensure;
+
+ let mut td = TestDir::new()?;
+
+ // use the metadata producing dump more than 64KB (the default pipe buffer size),
+ // such that thin_dump will be blocked on writing to the pipe, then hits EPIPE.
+ let md = prep_metadata(&mut td)?;
+
+ let out_fifo = td.mk_path("out_fifo");
+ unsafe {
+ let c_str = std::ffi::CString::new(out_fifo.as_os_str().as_encoded_bytes()).unwrap();
+ ensure!(libc::mkfifo(c_str.as_ptr(), 0o666) == 0);
+ };
+
+ let cmd = thin_dump_cmd(args![&md, "-o", &out_fifo])
+ .to_expr()
+ .stderr_capture();
+ let handle = cmd.unchecked().start()?;
+
+ let fifo = std::fs::File::open(out_fifo)?;
+ drop(fifo); // causing broken pipe
+
+ let output = handle.wait()?;
+ ensure!(!output.status.success());
+ ensure!(output.stderr.is_empty());
+
+ Ok(())
+}
+
//------------------------------------------
// test dump metadata snapshot from a live metadata
// here we use a corrupted metadata to ensure that "thin_dump -m" reads the
--
2.43.0

View File

@ -0,0 +1,34 @@
From e6659ff342516f1861cdb1f365dfb4f79bb45c54 Mon Sep 17 00:00:00 2001
From: mulhern <amulhern@redhat.com>
Date: Sun, 7 Jan 2024 20:17:13 -0500
Subject: [PATCH 08/10] thin_metadata_pack: Allow long format for input and
output
The options match the man pages that way.
(cherry picked from commit b5e7028effc51ad1c303424a9f1d161d2f7b0c7c)
---
src/commands/thin_metadata_pack.rs | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/commands/thin_metadata_pack.rs b/src/commands/thin_metadata_pack.rs
index 5aad973c..265439c9 100644
--- a/src/commands/thin_metadata_pack.rs
+++ b/src/commands/thin_metadata_pack.rs
@@ -26,11 +26,13 @@ impl ThinMetadataPackCommand {
.help("Specify thinp metadata binary device/file")
.required(true)
.short('i')
+ .long("input")
.value_name("DEV"))
.arg(Arg::new("OUTPUT")
.help("Specify packed output file")
.required(true)
.short('o')
+ .long("output")
.value_name("FILE"))
}
}
--
2.43.0

File diff suppressed because it is too large Load Diff

View File

@ -0,0 +1,109 @@
From a557bc55ef3c136b1fca3f27cd55fdb0014dc6e7 Mon Sep 17 00:00:00 2001
From: Ming-Hung Tsai <mtsai@redhat.com>
Date: Fri, 23 Feb 2024 20:20:01 +0800
Subject: [PATCH 10/10] [thin_dump] Do not print error messages on BrokenPipe
(EPIPE)
Handle the case that doesn't write through quick_xml,
e.g., thin_dump --format human_readable
(cherry picked from commit b3e05f2eb9b704af897f14215536dadde5c13b2d)
---
src/commands/utils.rs | 7 +++++--
tests/thin_dump.rs | 34 +++++++++++++++++++++++++++-------
2 files changed, 32 insertions(+), 9 deletions(-)
diff --git a/src/commands/utils.rs b/src/commands/utils.rs
index 38751603..82b2529e 100644
--- a/src/commands/utils.rs
+++ b/src/commands/utils.rs
@@ -165,8 +165,11 @@ pub fn to_exit_code<T>(report: &Report, result: anyhow::Result<T>) -> exitcode::
let root_cause = e.root_cause();
let is_broken_pipe = root_cause
.downcast_ref::<Arc<std::io::Error>>() // quick_xml::Error::Io wraps io::Error in Arc
- .map(|err| err.kind() == std::io::ErrorKind::BrokenPipe)
- .unwrap_or(false);
+ .map_or_else(
+ || root_cause.downcast_ref::<std::io::Error>(),
+ |err| Some(err.as_ref()),
+ )
+ .map_or(false, |err| err.kind() == std::io::ErrorKind::BrokenPipe);
if !is_broken_pipe {
if e.chain().len() > 1 {
diff --git a/tests/thin_dump.rs b/tests/thin_dump.rs
index 6e2cd3db..81982188 100644
--- a/tests/thin_dump.rs
+++ b/tests/thin_dump.rs
@@ -142,8 +142,7 @@ fn no_stderr_on_success() -> Result<()> {
//------------------------------------------
// test no stderr on broken pipe errors
-#[test]
-fn no_stderr_on_broken_pipe() -> Result<()> {
+fn test_no_stderr_on_broken_pipe(extra_args: &[&std::ffi::OsStr]) -> Result<()> {
use anyhow::ensure;
let mut td = TestDir::new()?;
@@ -157,7 +156,9 @@ fn no_stderr_on_broken_pipe() -> Result<()> {
ensure!(libc::pipe2(pipefd.as_mut_slice().as_mut_ptr(), libc::O_CLOEXEC) == 0);
}
- let cmd = thin_dump_cmd(args![&md])
+ let mut args = args![&md].to_vec();
+ args.extend_from_slice(extra_args);
+ let cmd = thin_dump_cmd(args)
.to_expr()
.stdout_file(pipefd[1])
.stderr_capture();
@@ -179,7 +180,16 @@ fn no_stderr_on_broken_pipe() -> Result<()> {
}
#[test]
-fn no_stderr_on_broken_fifo() -> Result<()> {
+fn no_stderr_on_broken_pipe_xml() -> Result<()> {
+ test_no_stderr_on_broken_pipe(&[])
+}
+
+#[test]
+fn no_stderr_on_broken_pipe_humanreadable() -> Result<()> {
+ test_no_stderr_on_broken_pipe(&args!["--format", "human_readable"])
+}
+
+fn test_no_stderr_on_broken_fifo(extra_args: &[&std::ffi::OsStr]) -> Result<()> {
use anyhow::ensure;
let mut td = TestDir::new()?;
@@ -194,9 +204,9 @@ fn no_stderr_on_broken_fifo() -> Result<()> {
ensure!(libc::mkfifo(c_str.as_ptr(), 0o666) == 0);
};
- let cmd = thin_dump_cmd(args![&md, "-o", &out_fifo])
- .to_expr()
- .stderr_capture();
+ let mut args = args![&md, "-o", &out_fifo].to_vec();
+ args.extend_from_slice(extra_args);
+ let cmd = thin_dump_cmd(args).to_expr().stderr_capture();
let handle = cmd.unchecked().start()?;
let fifo = std::fs::File::open(out_fifo)?;
@@ -209,6 +219,16 @@ fn no_stderr_on_broken_fifo() -> Result<()> {
Ok(())
}
+#[test]
+fn no_stderr_on_broken_fifo_xml() -> Result<()> {
+ test_no_stderr_on_broken_fifo(&[])
+}
+
+#[test]
+fn no_stderr_on_broken_fifo_humanreadable() -> Result<()> {
+ test_no_stderr_on_broken_fifo(&args!["--format", "human_readable"])
+}
+
//------------------------------------------
// test dump metadata snapshot from a live metadata
// here we use a corrupted metadata to ensure that "thin_dump -m" reads the
--
2.43.0

View File

@ -10,7 +10,7 @@
Summary: Device-mapper Persistent Data Tools
Name: device-mapper-persistent-data
Version: 1.0.9
Release: 2%{?dist}%{?release_suffix}
Release: 3%{?dist}%{?release_suffix}
License: GPLv3+
URL: https://github.com/jthornber/thin-provisioning-tools
#Source0: https://github.com/jthornber/thin-provisioning-tools/archive/thin-provisioning-tools-%%{version}.tar.gz
@ -22,6 +22,13 @@ Patch3: 0003-thin_repair-Fix-child-keys-checking-on-the-node-with.patch
Patch4: 0004-space_map-Allow-non-zero-values-in-unused-index-bloc.patch
Patch5: 0005-cache_check-Fix-boundary-check-on-the-bitset-for-cac.patch
Patch6: 0006-thin-cache_check-Print-suggestive-hints-for-improvin.patch
# RHEL-26521:
Patch7: 0007-thin_dump-Do-not-print-error-messages-on-BrokenPipe-.patch
# RHEL-26520:
Patch8: 0008-thin_metadata_pack-Allow-long-format-for-input-and-o.patch
Patch9: 0009-commands-Fix-version-string-compatibility-issue-with.patch
# RHEL-26521:
Patch10: 0010-thin_dump-Do-not-print-error-messages-on-BrokenPipe-.patch
BuildRequires: rust-packaging
BuildRequires: rust >= 1.35
@ -116,6 +123,10 @@ make DESTDIR=%{buildroot} MANDIR=%{_mandir} install
#% {_sbindir}/thin_show_duplicates
%changelog
* Mon Mar 04 2024 Marian Csontos <mcsontos@redhat.com> - 1.0.9-3
- Fix --version string compatibility with LVM tools.
- Fix confusing Broken pipe warning when used in lvconvert --repair.
* Thu Feb 08 2024 Marian Csontos <mcsontos@redhat.com> - 1.0.9-2
- Allow non-zero values in unused index block entries.
- Fix boundary check on the bitset for cached blocks.