rpms/libblockdev
7
0
Fork 0
Code Issues Pull Requests Releases Activity

import libblockdev-2.24-5.el8

Browse Source
This commit is contained in:
CentOS Sources 2021-05-18 02:45:56 -04:00 committed by Andrew Lukoshko
parent 196cd314ca
commit 9d848d77af
3 changed files with 1724 additions and 2 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

792
SOURCES/0002-exec-polling-fixes.patch Normal file
View File

@ -0,0 +1,792 @@
From 592820b7a4300cfdc4f85ecd9548f7c2321689fc Mon Sep 17 00:00:00 2001
From: Tomas Bzatek <tbzatek@redhat.com>
Date: Wed, 16 Sep 2020 17:45:07 +0200
Subject: [PATCH 1/5] exec: Fix polling for stdout and stderr
The condition of having both the stdout and the stderr fds ready
may never be satisfied in the case of a full stdout buffer waiting
to be read with no output on stderr yet while both fds being still
open. In such case the old code got stuck in an endless loop and
the spawned process being stuck on writing to stdout/stderr. Let's
read data from any fd once available and only react on EOF/HUP.
This change also makes use of POSIX poll() instead of g_poll()
as it's more clear what the return values mean - Glib docs are
vague in this regard and one can only guess.
---
src/utils/exec.c | 32 ++++++++++++++++++--------------
1 file changed, 18 insertions(+), 14 deletions(-)
diff --git a/src/utils/exec.c b/src/utils/exec.c
index 37bd960..ebbcaf2 100644
--- a/src/utils/exec.c
+++ b/src/utils/exec.c
@@ -22,6 +22,7 @@
#include "extra_arg.h"
#include <syslog.h>
#include <stdlib.h>
+#include <poll.h>
#include <errno.h>
#include <sys/types.h>
#include <sys/wait.h>
@@ -293,7 +294,7 @@ static gboolean _utils_exec_and_report_progress (const gchar **argv, const BDExt
gint poll_status = 0;
guint i = 0;
guint8 completion = 0;
- GPollFD fds[2] = {ZERO_INIT, ZERO_INIT};
+ struct pollfd fds[2] = { ZERO_INIT, ZERO_INIT };
gboolean out_done = FALSE;
gboolean err_done = FALSE;
GString *stdout_data = g_string_new (NULL);
@@ -360,13 +361,16 @@ static gboolean _utils_exec_and_report_progress (const gchar **argv, const BDExt
fds[0].fd = out_fd;
fds[1].fd = err_fd;
- fds[0].events = G_IO_IN | G_IO_HUP | G_IO_ERR;
- fds[1].events = G_IO_IN | G_IO_HUP | G_IO_ERR;
+ fds[0].events = POLLIN | POLLHUP | POLLERR;
+ fds[1].events = POLLIN | POLLHUP | POLLERR;
while (!out_done || !err_done) {
- poll_status = g_poll (fds, 2, -1);
+ poll_status = poll (fds, 2, -1);
+ g_warn_if_fail (poll_status != 0); /* no timeout specified, zero should never be returned */
if (poll_status < 0) {
+ if (errno == EAGAIN || errno == EINTR)
+ continue;
g_set_error (error, BD_UTILS_EXEC_ERROR, BD_UTILS_EXEC_ERROR_FAILED,
- "Failed to poll output FDs.");
+ "Failed to poll output FDs: %m");
bd_utils_report_finished (progress_id, (*error)->message);
g_io_channel_shutdown (out_pipe, FALSE, NULL);
g_io_channel_unref (out_pipe);
@@ -375,12 +379,9 @@ static gboolean _utils_exec_and_report_progress (const gchar **argv, const BDExt
g_string_free (stdout_data, TRUE);
g_string_free (stderr_data, TRUE);
return FALSE;
- } else if (poll_status != 2)
- /* both revents fields were not filled yet */
- continue;
- if (!(fds[0].revents & G_IO_IN))
- out_done = TRUE;
- while (!out_done) {
+ }
+
+ while (!out_done && (fds[0].revents & POLLIN)) {
io_status = g_io_channel_read_line (out_pipe, &line, NULL, NULL, error);
if (io_status == G_IO_STATUS_NORMAL) {
if (prog_extract && prog_extract (line, &completion))
@@ -401,9 +402,10 @@ static gboolean _utils_exec_and_report_progress (const gchar **argv, const BDExt
return FALSE;
}
}
- if (!(fds[1].revents & G_IO_IN))
- err_done = TRUE;
- while (!err_done) {
+ if (fds[0].revents & POLLHUP || fds[0].revents & POLLERR || fds[0].revents & POLLNVAL)
+ out_done = TRUE;
+
+ while (!err_done && (fds[1].revents & POLLIN)) {
io_status = g_io_channel_read_line (err_pipe, &line, NULL, NULL, error);
if (io_status == G_IO_STATUS_NORMAL) {
g_string_append (stderr_data, line);
@@ -421,6 +423,8 @@ static gboolean _utils_exec_and_report_progress (const gchar **argv, const BDExt
return FALSE;
}
}
+ if (fds[1].revents & POLLHUP || fds[1].revents & POLLERR || fds[1].revents & POLLNVAL)
+ err_done = TRUE;
}
child_ret = waitpid (pid, &status, 0);
--
2.26.2
From 3025deda9ab670a454bfe373166e49f2acd1c151 Mon Sep 17 00:00:00 2001
From: Tomas Bzatek <tbzatek@redhat.com>
Date: Fri, 25 Sep 2020 18:19:46 +0200
Subject: [PATCH 2/5] exec: Use non-blocking read and process the buffer
manually
Waiting for data or a newline character on one fd may create a deadlock
while the other fd is being filled with data, exhausting the pipe buffer.
Setting both stdout and stderr fds non-blocking allows us to get indication
of an empty pipe, continuing with the read cycle over remaining watched fds.
This also gets rid of GIOChannel as no extended functionality like GSource
notifications were used, degrading GIOChannel in a simple GObject wrapper
over an fd with just a convenience read_line method that we had to get rid of
anyway. Let's use standard POSIX calls and split the read buffer manually
by matching the newline character. NULL bytes should be handled gracefully
however the stack higher up is not ready for that anyway.
---
src/utils/exec.c | 277 +++++++++++++++++++++++++++--------------------
1 file changed, 159 insertions(+), 118 deletions(-)
diff --git a/src/utils/exec.c b/src/utils/exec.c
index ebbcaf2..317fb55 100644
--- a/src/utils/exec.c
+++ b/src/utils/exec.c
@@ -23,6 +23,7 @@
#include <syslog.h>
#include <stdlib.h>
#include <poll.h>
+#include <fcntl.h>
#include <errno.h>
#include <sys/types.h>
#include <sys/wait.h>
@@ -272,6 +273,87 @@ gboolean bd_utils_exec_and_report_status_error (const gchar **argv, const BDExtr
return TRUE;
}
+/* buffer size in bytes used to read from stdout and stderr */
+#define _EXEC_BUF_SIZE 64*1024
+
+/* similar to g_strstr_len() yet treats 'null' byte as @needle. */
+static gchar *bd_strchr_len_null (const gchar *haystack, gssize haystack_len, const gchar needle) {
+ gchar *ret;
+ gchar *ret_null;
+
+ ret = memchr (haystack, needle, haystack_len);
+ ret_null = memchr (haystack, 0, haystack_len);
+ if (ret && ret_null)
+ return MIN (ret, ret_null);
+ else
+ return MAX (ret, ret_null);
+}
+
+static gboolean
+_process_fd_event (gint fd, struct pollfd *poll_fd, GString *read_buffer, GString *filtered_buffer, gsize *read_buffer_pos, gboolean *done,
+ guint64 progress_id, guint8 *progress, BDUtilsProgExtract prog_extract, GError **error) {
+ gchar buf[_EXEC_BUF_SIZE] = { 0 };
+ ssize_t num_read;
+ gchar *line;
+ gchar *newline_pos;
+ int errno_saved;
+ gboolean eof = FALSE;
+
+ if (! *done && (poll_fd->revents & POLLIN)) {
+ /* read until we get EOF (0) or error (-1), expecting EAGAIN */
+ while ((num_read = read (fd, buf, _EXEC_BUF_SIZE)) > 0)
+ g_string_append_len (read_buffer, buf, num_read);
+ errno_saved = errno;
+
+ /* process the fresh data by lines */
+ if (read_buffer->len > *read_buffer_pos) {
+ gchar *buf_ptr;
+ gsize buf_len;
+
+ while ((buf_ptr = read_buffer->str + *read_buffer_pos,
+ buf_len = read_buffer->len - *read_buffer_pos,
+ newline_pos = bd_strchr_len_null (buf_ptr, buf_len, '\n'))) {
+ line = g_strndup (buf_ptr, newline_pos - buf_ptr + 1);
+ if (prog_extract && prog_extract (line, progress))
+ bd_utils_report_progress (progress_id, *progress, NULL);
+ else
+ g_string_append (filtered_buffer, line);
+ g_free (line);
+ *read_buffer_pos = newline_pos - read_buffer->str + 1;
+ }
+ }
+
+ /* read error */
+ if (num_read < 0 && errno_saved != EAGAIN && errno_saved != EINTR) {
+ g_set_error (error, BD_UTILS_EXEC_ERROR, BD_UTILS_EXEC_ERROR_FAILED,
+ "Error reading from pipe: %s", g_strerror (errno_saved));
+ return FALSE;
+ }
+
+ /* EOF */
+ if (num_read == 0)
+ eof = TRUE;
+ }
+
+ if (poll_fd->revents & POLLHUP || poll_fd->revents & POLLERR || poll_fd->revents & POLLNVAL)
+ eof = TRUE;
+
+ if (eof) {
+ *done = TRUE;
+ /* process the remaining buffer */
+ line = read_buffer->str + *read_buffer_pos;
+ /* GString guarantees the buffer is always NULL-terminated. */
+ if (strlen (line) > 0) {
+ if (prog_extract && prog_extract (line, progress))
+ bd_utils_report_progress (progress_id, *progress, NULL);
+ else
+ g_string_append (filtered_buffer, line);
+ }
+ }
+
+ return TRUE;
+}
+
static gboolean _utils_exec_and_report_progress (const gchar **argv, const BDExtraArg **extra, BDUtilsProgExtract prog_extract, gint *proc_status, gchar **stdout, gchar **stderr, GError **error) {
const gchar **args = NULL;
guint args_len = 0;
@@ -283,24 +365,26 @@ static gboolean _utils_exec_and_report_progress (const gchar **argv, const BDExt
gchar *msg = NULL;
GPid pid = 0;
gint out_fd = 0;
- GIOChannel *out_pipe = NULL;
gint err_fd = 0;
- GIOChannel *err_pipe = NULL;
- gchar *line = NULL;
gint child_ret = -1;
gint status = 0;
gboolean ret = FALSE;
- GIOStatus io_status = G_IO_STATUS_NORMAL;
gint poll_status = 0;
guint i = 0;
guint8 completion = 0;
struct pollfd fds[2] = { ZERO_INIT, ZERO_INIT };
+ int flags;
gboolean out_done = FALSE;
gboolean err_done = FALSE;
- GString *stdout_data = g_string_new (NULL);
- GString *stderr_data = g_string_new (NULL);
+ GString *stdout_data;
+ GString *stdout_buffer;
+ GString *stderr_data;
+ GString *stderr_buffer;
+ gsize stdout_buffer_pos = 0;
+ gsize stderr_buffer_pos = 0;
gchar **old_env = NULL;
gchar **new_env = NULL;
+ gboolean success = TRUE;
/* TODO: share this code between functions */
if (extra) {
@@ -336,15 +420,13 @@ static gboolean _utils_exec_and_report_progress (const gchar **argv, const BDExt
G_SPAWN_DEFAULT|G_SPAWN_SEARCH_PATH|G_SPAWN_DO_NOT_REAP_CHILD,
NULL, NULL, &pid, NULL, &out_fd, &err_fd, error);
+ g_strfreev (new_env);
+
if (!ret) {
/* error is already populated */
- g_string_free (stdout_data, TRUE);
- g_string_free (stderr_data, TRUE);
- g_strfreev (new_env);
g_free (args);
return FALSE;
}
- g_strfreev (new_env);
args_str = g_strjoinv (" ", args ? (gchar **) args : (gchar **) argv);
msg = g_strdup_printf ("Started '%s'", args_str);
@@ -353,18 +435,25 @@ static gboolean _utils_exec_and_report_progress (const gchar **argv, const BDExt
g_free (args);
g_free (msg);
- out_pipe = g_io_channel_unix_new (out_fd);
- err_pipe = g_io_channel_unix_new (err_fd);
+ /* set both fds for non-blocking read */
+ flags = fcntl (out_fd, F_GETFL, 0);
+ if (fcntl (out_fd, F_SETFL, flags | O_NONBLOCK))
+ g_warning ("_utils_exec_and_report_progress: Failed to set out_fd non-blocking: %m");
+ flags = fcntl (err_fd, F_GETFL, 0);
+ if (fcntl (err_fd, F_SETFL, flags | O_NONBLOCK))
+ g_warning ("_utils_exec_and_report_progress: Failed to set err_fd non-blocking: %m");
- g_io_channel_set_encoding (out_pipe, NULL, NULL);
- g_io_channel_set_encoding (err_pipe, NULL, NULL);
+ stdout_data = g_string_new (NULL);
+ stdout_buffer = g_string_new (NULL);
+ stderr_data = g_string_new (NULL);
+ stderr_buffer = g_string_new (NULL);
fds[0].fd = out_fd;
fds[1].fd = err_fd;
fds[0].events = POLLIN | POLLHUP | POLLERR;
fds[1].events = POLLIN | POLLHUP | POLLERR;
- while (!out_done || !err_done) {
- poll_status = poll (fds, 2, -1);
+ while (! (out_done && err_done)) {
+ poll_status = poll (fds, 2, -1 /* timeout */);
g_warn_if_fail (poll_status != 0); /* no timeout specified, zero should never be returned */
if (poll_status < 0) {
if (errno == EAGAIN || errno == EINTR)
@@ -372,140 +461,90 @@ static gboolean _utils_exec_and_report_progress (const gchar **argv, const BDExt
g_set_error (error, BD_UTILS_EXEC_ERROR, BD_UTILS_EXEC_ERROR_FAILED,
"Failed to poll output FDs: %m");
bd_utils_report_finished (progress_id, (*error)->message);
- g_io_channel_shutdown (out_pipe, FALSE, NULL);
- g_io_channel_unref (out_pipe);
- g_io_channel_shutdown (err_pipe, FALSE, NULL);
- g_io_channel_unref (err_pipe);
- g_string_free (stdout_data, TRUE);
- g_string_free (stderr_data, TRUE);
- return FALSE;
+ success = FALSE;
+ break;
}
- while (!out_done && (fds[0].revents & POLLIN)) {
- io_status = g_io_channel_read_line (out_pipe, &line, NULL, NULL, error);
- if (io_status == G_IO_STATUS_NORMAL) {
- if (prog_extract && prog_extract (line, &completion))
- bd_utils_report_progress (progress_id, completion, NULL);
- else
- g_string_append (stdout_data, line);
- g_free (line);
- } else if (io_status == G_IO_STATUS_EOF) {
- out_done = TRUE;
- } else if (error && (*error)) {
+ if (!out_done) {
+ if (! _process_fd_event (out_fd, &fds[0], stdout_buffer, stdout_data, &stdout_buffer_pos, &out_done, progress_id, &completion, prog_extract, error)) {
bd_utils_report_finished (progress_id, (*error)->message);
- g_io_channel_shutdown (out_pipe, FALSE, NULL);
- g_io_channel_unref (out_pipe);
- g_io_channel_shutdown (err_pipe, FALSE, NULL);
- g_io_channel_unref (err_pipe);
- g_string_free (stdout_data, TRUE);
- g_string_free (stderr_data, TRUE);
- return FALSE;
+ success = FALSE;
+ break;
}
}
- if (fds[0].revents & POLLHUP || fds[0].revents & POLLERR || fds[0].revents & POLLNVAL)
- out_done = TRUE;
- while (!err_done && (fds[1].revents & POLLIN)) {
- io_status = g_io_channel_read_line (err_pipe, &line, NULL, NULL, error);
- if (io_status == G_IO_STATUS_NORMAL) {
- g_string_append (stderr_data, line);
- g_free (line);
- } else if (io_status == G_IO_STATUS_EOF) {
- err_done = TRUE;
- } else if (error && (*error)) {
+ if (!err_done) {
+ if (! _process_fd_event (err_fd, &fds[1], stderr_buffer, stderr_data, &stderr_buffer_pos, &err_done, progress_id, &completion, prog_extract, error)) {
bd_utils_report_finished (progress_id, (*error)->message);
- g_io_channel_shutdown (out_pipe, FALSE, NULL);
- g_io_channel_unref (out_pipe);
- g_io_channel_shutdown (err_pipe, FALSE, NULL);
- g_io_channel_unref (err_pipe);
- g_string_free (stdout_data, TRUE);
- g_string_free (stderr_data, TRUE);
- return FALSE;
+ success = FALSE;
+ break;
}
}
- if (fds[1].revents & POLLHUP || fds[1].revents & POLLERR || fds[1].revents & POLLNVAL)
- err_done = TRUE;
}
+ g_string_free (stdout_buffer, TRUE);
+ g_string_free (stderr_buffer, TRUE);
+ close (out_fd);
+ close (err_fd);
+
child_ret = waitpid (pid, &status, 0);
- *proc_status = WEXITSTATUS(status);
- if (child_ret > 0) {
- if (*proc_status != 0) {
- if (stderr_data->str && (g_strcmp0 ("", stderr_data->str) != 0))
- msg = stderr_data->str;
- else
- msg = stdout_data->str;
- g_set_error (error, BD_UTILS_EXEC_ERROR, BD_UTILS_EXEC_ERROR_FAILED,
- "Process reported exit code %d: %s", *proc_status, msg);
- bd_utils_report_finished (progress_id, (*error)->message);
- g_io_channel_shutdown (out_pipe, FALSE, NULL);
- g_io_channel_unref (out_pipe);
- g_io_channel_shutdown (err_pipe, FALSE, NULL);
- g_io_channel_unref (err_pipe);
- g_string_free (stdout_data, TRUE);
- g_string_free (stderr_data, TRUE);
- return FALSE;
- }
- if (WIFSIGNALED(status)) {
- g_set_error (error, BD_UTILS_EXEC_ERROR, BD_UTILS_EXEC_ERROR_FAILED,
- "Process killed with a signal");
- bd_utils_report_finished (progress_id, (*error)->message);
- g_io_channel_shutdown (out_pipe, FALSE, NULL);
- g_io_channel_unref (out_pipe);
- g_io_channel_shutdown (err_pipe, FALSE, NULL);
- g_io_channel_unref (err_pipe);
- g_string_free (stdout_data, TRUE);
- g_string_free (stderr_data, TRUE);
- return FALSE;
- }
- } else if (child_ret == -1) {
- if (errno != ECHILD) {
- errno = 0;
- g_set_error (error, BD_UTILS_EXEC_ERROR, BD_UTILS_EXEC_ERROR_FAILED,
- "Failed to wait for the process");
- bd_utils_report_finished (progress_id, (*error)->message);
- g_io_channel_shutdown (out_pipe, FALSE, NULL);
- g_io_channel_unref (out_pipe);
- g_io_channel_shutdown (err_pipe, FALSE, NULL);
- g_io_channel_unref (err_pipe);
- g_string_free (stdout_data, TRUE);
- g_string_free (stderr_data, TRUE);
- return FALSE;
+ *proc_status = WEXITSTATUS (status);
+ if (success) {
+ if (child_ret > 0) {
+ if (*proc_status != 0) {
+ msg = stderr_data->len > 0 ? stderr_data->str : stdout_data->str;
+ g_set_error (error, BD_UTILS_EXEC_ERROR, BD_UTILS_EXEC_ERROR_FAILED,
+ "Process reported exit code %d: %s", *proc_status, msg);
+ bd_utils_report_finished (progress_id, (*error)->message);
+ success = FALSE;
+ } else if (WIFSIGNALED (status)) {
+ g_set_error (error, BD_UTILS_EXEC_ERROR, BD_UTILS_EXEC_ERROR_FAILED,
+ "Process killed with a signal");
+ bd_utils_report_finished (progress_id, (*error)->message);
+ success = FALSE;
+ }
+ } else if (child_ret == -1) {
+ if (errno != ECHILD) {
+ errno = 0;
+ g_set_error (error, BD_UTILS_EXEC_ERROR, BD_UTILS_EXEC_ERROR_FAILED,
+ "Failed to wait for the process");
+ bd_utils_report_finished (progress_id, (*error)->message);
+ success = FALSE;
+ } else {
+ /* no such process (the child exited before we tried to wait for it) */
+ errno = 0;
+ }
}
- /* no such process (the child exited before we tried to wait for it) */
- errno = 0;
+ if (success)
+ bd_utils_report_finished (progress_id, "Completed");
}
-
- bd_utils_report_finished (progress_id, "Completed");
log_out (task_id, stdout_data->str, stderr_data->str);
log_done (task_id, *proc_status);
- /* we don't care about the status here */
- g_io_channel_shutdown (out_pipe, FALSE, error);
- g_io_channel_unref (out_pipe);
- g_io_channel_shutdown (err_pipe, FALSE, error);
- g_io_channel_unref (err_pipe);
-
- if (stdout)
+ if (success && stdout)
*stdout = g_string_free (stdout_data, FALSE);
else
g_string_free (stdout_data, TRUE);
- if (stderr)
+ if (success && stderr)
*stderr = g_string_free (stderr_data, FALSE);
else
g_string_free (stderr_data, TRUE);
- return TRUE;
+ return success;
}
/**
* bd_utils_exec_and_report_progress:
* @argv: (array zero-terminated=1): the argv array for the call
* @extra: (allow-none) (array zero-terminated=1): extra arguments
- * @prog_extract: (scope notified): function for extracting progress information
+ * @prog_extract: (scope notified) (nullable): function for extracting progress information
* @proc_status: (out): place to store the process exit status
* @error: (out): place to store error (if any)
*
+ * Note that any NULL bytes read from standard output and standard error
+ * output are treated as separators similar to newlines and @prog_extract
+ * will be called with the respective chunk.
+ *
* Returns: whether the @argv was successfully executed (no error and exit code 0) or not
*/
gboolean bd_utils_exec_and_report_progress (const gchar **argv, const BDExtraArg **extra, BDUtilsProgExtract prog_extract, gint *proc_status, GError **error) {
@@ -519,6 +558,9 @@ gboolean bd_utils_exec_and_report_progress (const gchar **argv, const BDExtraArg
* @output: (out): variable to store output to
* @error: (out): place to store error (if any)
*
+ * Note that any NULL bytes read from standard output and standard error
+ * output will be discarded.
+ *
* Returns: whether the @argv was successfully executed capturing the output or not
*/
gboolean bd_utils_exec_and_capture_output (const gchar **argv, const BDExtraArg **extra, gchar **output, GError **error) {
@@ -549,7 +591,6 @@ gboolean bd_utils_exec_and_capture_output (const gchar **argv, const BDExtraArg
g_free (stderr);
return TRUE;
}
-
}
/**
--
2.26.2
From f5eb61c91ffc6b0d1fd709076a9579105655ff17 Mon Sep 17 00:00:00 2001
From: Tomas Bzatek <tbzatek@redhat.com>
Date: Fri, 25 Sep 2020 18:27:02 +0200
Subject: [PATCH 3/5] exec: Clarify the BDUtilsProgExtract callback
documentation
---
src/utils/exec.h | 24 ++++++++++++++++++++++--
1 file changed, 22 insertions(+), 2 deletions(-)
diff --git a/src/utils/exec.h b/src/utils/exec.h
index ad169e4..0e262a2 100644
--- a/src/utils/exec.h
+++ b/src/utils/exec.h
@@ -31,10 +31,30 @@ typedef void (*BDUtilsProgFunc) (guint64 task_id, BDUtilsProgStatus status, guin
/**
* BDUtilsProgExtract:
- * @line: line from extract progress from
+ * @line: line to extract progress from
* @completion: (out): percentage of completion
*
- * Returns: whether the line was a progress reporting line or not
+ * Callback function used to process a line captured from spawned command's standard
+ * output and standard error output. Typically used to extract completion percentage
+ * of a long-running job.
+ *
+ * Note that both outputs are read simultaneously with no guarantees of message order
+ * this function is called with.
+ *
+ * The value the @completion points to may contain value previously returned from
+ * this callback or zero when called for the first time. This is useful for extractors
+ * where only some kind of a tick mark is printed out as a progress and previous value
+ * is needed to compute an incremented value. It's important to keep in mind that this
+ * function is only called over lines, i.e. progress reporting printing out tick marks
+ * (e.g. dots) without a newline character might not work properly.
+ *
+ * The @line string usually contains trailing newline character, which may be absent
+ * however in case the spawned command exits without printing one. It's guaranteed
+ * this function is called over remaining buffer no matter what the trailing
+ * character is.
+ *
+ * Returns: whether the line was a progress reporting line and should be excluded
+ * from the collected standard output string or not.
*/
typedef gboolean (*BDUtilsProgExtract) (const gchar *line, guint8 *completion);
--
2.26.2
From 8a7f0de5f63099a3e8bcaca005c4de04df959113 Mon Sep 17 00:00:00 2001
From: Tomas Bzatek <tbzatek@redhat.com>
Date: Fri, 25 Sep 2020 18:27:41 +0200
Subject: [PATCH 4/5] tests: Add bufferbloat exec tests
This should test pipe buffer exhaustion as well as potential pipe
read starvation while filling the other fd.
---
tests/utils_test.py | 105 +++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 104 insertions(+), 1 deletion(-)
diff --git a/tests/utils_test.py b/tests/utils_test.py
index 2bec5ed..ed13611 100644
--- a/tests/utils_test.py
+++ b/tests/utils_test.py
@@ -1,8 +1,9 @@
import unittest
import re
import os
+import six
import overrides_hack
-from utils import fake_utils, create_sparse_tempfile, create_lio_device, delete_lio_device, run_command, TestTags, tag_test
+from utils import fake_utils, create_sparse_tempfile, create_lio_device, delete_lio_device, run_command, TestTags, tag_test, read_file
from gi.repository import BlockDev, GLib
@@ -65,6 +66,9 @@ class UtilsExecLoggingTest(UtilsTestCase):
succ = BlockDev.utils_exec_and_report_error(["true"])
self.assertTrue(succ)
+ with six.assertRaisesRegex(self, GLib.GError, r"Process reported exit code 1"):
+ succ = BlockDev.utils_exec_and_report_error(["/bin/false"])
+
succ, out = BlockDev.utils_exec_and_capture_output(["echo", "hi"])
self.assertTrue(succ)
self.assertEqual(out, "hi\n")
@@ -178,6 +182,105 @@ class UtilsExecLoggingTest(UtilsTestCase):
self.assertTrue(succ)
self.assertIn("LC_ALL=C", out)
+ @tag_test(TestTags.NOSTORAGE, TestTags.CORE)
+ def test_exec_buffer_bloat(self):
+ """Verify that very large output from a command is handled properly"""
+
+ # easy 64kB of data
+ cnt = 65536
+ succ, out = BlockDev.utils_exec_and_capture_output(["bash", "-c", "for i in {1..%d}; do echo -n .; done" % cnt])
+ self.assertTrue(succ)
+ self.assertEquals(len(out), cnt)
+
+ succ, out = BlockDev.utils_exec_and_capture_output(["bash", "-c", "for i in {1..%d}; do echo -n .; echo -n \# >&2; done" % cnt])
+ self.assertTrue(succ)
+ self.assertEquals(len(out), cnt)
+
+ # now exceed the system pipe buffer size
+ # pipe(7): The maximum size (in bytes) of individual pipes that can be set by users without the CAP_SYS_RESOURCE capability.
+ cnt = int(read_file("/proc/sys/fs/pipe-max-size")) + 100
+ self.assertGreater(cnt, 0)
+
+ succ, out = BlockDev.utils_exec_and_capture_output(["bash", "-c", "for i in {1..%d}; do echo -n .; done" % cnt])
+ self.assertTrue(succ)
+ self.assertEquals(len(out), cnt)
+
+ succ, out = BlockDev.utils_exec_and_capture_output(["bash", "-c", "for i in {1..%d}; do echo -n .; echo -n \# >&2; done" % cnt])
+ self.assertTrue(succ)
+ self.assertEquals(len(out), cnt)
+
+ # make use of some newlines
+ succ, out = BlockDev.utils_exec_and_capture_output(["bash", "-c", "for i in {1..%d}; do echo -n .; if [ $(($i%%500)) -eq 0 ]; then echo ''; fi; done" % cnt])
+ self.assertTrue(succ)
+ self.assertGreater(len(out), cnt)
+
+ succ, out = BlockDev.utils_exec_and_capture_output(["bash", "-c", "for i in {1..%d}; do echo -n .; echo -n \# >&2; if [ $(($i%%500)) -eq 0 ]; then echo ''; echo '' >&2; fi; done" % cnt])
+ self.assertTrue(succ)
+ self.assertGreater(len(out), cnt)
+
+
+ EXEC_PROGRESS_MSG = "Aloha, I'm the progress line you should match."
+
+ def my_exec_progress_func_concat(self, line):
+ """Expect an concatenated string"""
+ s = ""
+ for i in range(10):
+ s += self.EXEC_PROGRESS_MSG
+ self.assertEqual(line, s)
+ self.num_matches += 1
+ return 0
+
+ def my_exec_progress_func(self, line):
+ self.assertTrue(re.match(r".*%s.*" % self.EXEC_PROGRESS_MSG, line))
+ self.num_matches += 1
+ return 0
+
+ def test_exec_buffer_bloat_progress(self):
+ """Verify that progress reporting can handle large output"""
+
+ # no newlines, should match just a single occurrence on EOF
+ cnt = 10
+ self.num_matches = 0
+ status = BlockDev.utils_exec_and_report_progress(["bash", "-c", "for i in {1..10}; do echo -n \"%s\"; done" % self.EXEC_PROGRESS_MSG], None, self.my_exec_progress_func_concat)
+ self.assertTrue(status)
+ self.assertEqual(self.num_matches, 1)
+
+ # ten matches
+ self.num_matches = 0
+ status = BlockDev.utils_exec_and_report_progress(["bash", "-c", "for i in {1..%d}; do echo \"%s\"; done" % (cnt, self.EXEC_PROGRESS_MSG)], None, self.my_exec_progress_func)
+ self.assertTrue(status)
+ self.assertEqual(self.num_matches, cnt)
+
+ # 100k matches
+ cnt = 100000
+ self.num_matches = 0
+ status = BlockDev.utils_exec_and_report_progress(["bash", "-c", "for i in {1..%d}; do echo \"%s\"; done" % (cnt, self.EXEC_PROGRESS_MSG)], None, self.my_exec_progress_func)
+ self.assertTrue(status)
+ self.assertEqual(self.num_matches, cnt)
+
+ # 100k matches on stderr
+ self.num_matches = 0
+ status = BlockDev.utils_exec_and_report_progress(["bash", "-c", "for i in {1..%d}; do echo \"%s\" >&2; done" % (cnt, self.EXEC_PROGRESS_MSG)], None, self.my_exec_progress_func)
+ self.assertTrue(status)
+ self.assertEqual(self.num_matches, cnt)
+
+ # 100k matches on stderr and stdout
+ self.num_matches = 0
+ status = BlockDev.utils_exec_and_report_progress(["bash", "-c", "for i in {1..%d}; do echo \"%s\"; echo \"%s\" >&2; done" % (cnt, self.EXEC_PROGRESS_MSG, self.EXEC_PROGRESS_MSG)], None, self.my_exec_progress_func)
+ self.assertTrue(status)
+ self.assertEqual(self.num_matches, cnt * 2)
+
+ # stdout and stderr output, non-zero return from the command and very long exception message
+ self.num_matches = 0
+ with six.assertRaisesRegex(self, GLib.GError, r"Process reported exit code 66"):
+ status = BlockDev.utils_exec_and_report_progress(["bash", "-c", "for i in {1..%d}; do echo \"%s\"; echo \"%s\" >&2; done; exit 66" % (cnt, self.EXEC_PROGRESS_MSG, self.EXEC_PROGRESS_MSG)], None, self.my_exec_progress_func)
+ self.assertEqual(self.num_matches, cnt * 2)
+
+ # no progress reporting callback given, tests slightly different code path
+ status = BlockDev.utils_exec_and_report_progress(["bash", "-c", "for i in {1..%d}; do echo \"%s\"; echo \"%s\" >&2; done" % (cnt, self.EXEC_PROGRESS_MSG, self.EXEC_PROGRESS_MSG)], None, None)
+ self.assertTrue(status)
+
+
class UtilsDevUtilsTestCase(UtilsTestCase):
@tag_test(TestTags.NOSTORAGE, TestTags.CORE)
def test_resolve_device(self):
--
2.26.2
From 7a3fd3d32dd325fb5188bcba74966e414e33c343 Mon Sep 17 00:00:00 2001
From: Tomas Bzatek <tbzatek@redhat.com>
Date: Wed, 30 Sep 2020 14:52:27 +0200
Subject: [PATCH 5/5] tests: Add null-byte exec tests
Some commands may print out NULL bytes in the middle of their output,
make sure everything works correctly.
---
tests/utils_test.py | 48 +++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 48 insertions(+)
diff --git a/tests/utils_test.py b/tests/utils_test.py
index ed13611..1235be3 100644
--- a/tests/utils_test.py
+++ b/tests/utils_test.py
@@ -280,6 +280,54 @@ class UtilsExecLoggingTest(UtilsTestCase):
status = BlockDev.utils_exec_and_report_progress(["bash", "-c", "for i in {1..%d}; do echo \"%s\"; echo \"%s\" >&2; done" % (cnt, self.EXEC_PROGRESS_MSG, self.EXEC_PROGRESS_MSG)], None, None)
self.assertTrue(status)
+ def test_exec_null_bytes(self):
+ """Verify that null bytes in the output are processed properly"""
+
+ TEST_DATA = ["First line", "Second line", "Third line"]
+
+ status, out = BlockDev.utils_exec_and_capture_output(["bash", "-c", "echo -e \"%s\\0%s\\n%s\"" % (TEST_DATA[0], TEST_DATA[1], TEST_DATA[2])])
+ self.assertTrue(status)
+ self.assertTrue(TEST_DATA[0] in out)
+ self.assertTrue(TEST_DATA[1] in out)
+ self.assertTrue(TEST_DATA[2] in out)
+ self.assertFalse("kuku!" in out)
+
+ # ten matches
+ cnt = 10
+ self.num_matches = 0
+ status = BlockDev.utils_exec_and_report_progress(["bash", "-c", "for i in {1..%d}; do echo -e \"%s\\0%s\"; done" % (cnt, self.EXEC_PROGRESS_MSG, self.EXEC_PROGRESS_MSG)], None, self.my_exec_progress_func)
+ self.assertTrue(status)
+ self.assertEqual(self.num_matches, cnt * 2)
+
+ # 100k matches
+ cnt = 100000
+ self.num_matches = 0
+ status = BlockDev.utils_exec_and_report_progress(["bash", "-c", "for i in {1..%d}; do echo -e \"%s\\0%s\"; done" % (cnt, self.EXEC_PROGRESS_MSG, self.EXEC_PROGRESS_MSG)], None, self.my_exec_progress_func)
+ self.assertTrue(status)
+ self.assertEqual(self.num_matches, cnt * 2)
+
+ # 100k matches on stderr
+ self.num_matches = 0
+ status = BlockDev.utils_exec_and_report_progress(["bash", "-c", "for i in {1..%d}; do echo -e \"%s\\0%s\" >&2; done" % (cnt, self.EXEC_PROGRESS_MSG, self.EXEC_PROGRESS_MSG)], None, self.my_exec_progress_func)
+ self.assertTrue(status)
+ self.assertEqual(self.num_matches, cnt * 2)
+
+ # 100k matches on stderr and stdout
+ self.num_matches = 0
+ status = BlockDev.utils_exec_and_report_progress(["bash", "-c", "for i in {1..%d}; do echo -e \"%s\\0%s\"; echo -e \"%s\\0%s\" >&2; done" % (cnt, self.EXEC_PROGRESS_MSG, self.EXEC_PROGRESS_MSG, self.EXEC_PROGRESS_MSG, self.EXEC_PROGRESS_MSG)], None, self.my_exec_progress_func)
+ self.assertTrue(status)
+ self.assertEqual(self.num_matches, cnt * 4)
+
+ # stdout and stderr output, non-zero return from the command and very long exception message
+ self.num_matches = 0
+ with six.assertRaisesRegex(self, GLib.GError, r"Process reported exit code 66"):
+ status = BlockDev.utils_exec_and_report_progress(["bash", "-c", "for i in {1..%d}; do echo -e \"%s\\0%s\"; echo -e \"%s\\0%s\" >&2; done; exit 66" % (cnt, self.EXEC_PROGRESS_MSG, self.EXEC_PROGRESS_MSG, self.EXEC_PROGRESS_MSG, self.EXEC_PROGRESS_MSG)], None, self.my_exec_progress_func)
+ self.assertEqual(self.num_matches, cnt * 4)
+
+ # no progress reporting callback given, tests slightly different code path
+ status = BlockDev.utils_exec_and_report_progress(["bash", "-c", "for i in {1..%d}; do echo -e \"%s\\0%s\"; echo -e \"%s\\0%s\" >&2; done" % (cnt, self.EXEC_PROGRESS_MSG, self.EXEC_PROGRESS_MSG, self.EXEC_PROGRESS_MSG, self.EXEC_PROGRESS_MSG)], None, None)
+ self.assertTrue(status)
+
class UtilsDevUtilsTestCase(UtilsTestCase):
@tag_test(TestTags.NOSTORAGE, TestTags.CORE)
--
2.26.2

914
SOURCES/0003-LVM-thin-metadata-calculation-fix.patch Normal file
View File

@ -0,0 +1,914 @@
From 2bb371937c7ef73f26717e57a5eb78cafe90a9f7 Mon Sep 17 00:00:00 2001
From: Vojtech Trefny <vtrefny@redhat.com>
Date: Fri, 20 Sep 2019 09:58:01 +0200
Subject: [PATCH 1/5] Mark all GIR file constants as guint64
See 9676585e65f69ec1c309f45ba139035408d59b8e for more information.
---
src/lib/plugin_apis/btrfs.api | 2 +-
src/lib/plugin_apis/crypto.api | 2 +-
src/lib/plugin_apis/lvm.api | 20 ++++++++++----------
src/lib/plugin_apis/mdraid.api | 4 ++--
4 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/src/lib/plugin_apis/btrfs.api b/src/lib/plugin_apis/btrfs.api
index b52fb4ce..ef0f6c28 100644
--- a/src/lib/plugin_apis/btrfs.api
+++ b/src/lib/plugin_apis/btrfs.api
@@ -3,7 +3,7 @@
#include <blockdev/utils.h>
#define BD_BTRFS_MAIN_VOLUME_ID 5
-#define BD_BTRFS_MIN_MEMBER_SIZE (128 MiB)
+#define BD_BTRFS_MIN_MEMBER_SIZE G_GUINT64_CONSTANT (134217728ULL) // 128 MiB
GQuark bd_btrfs_error_quark (void) {
return g_quark_from_static_string ("g-bd-btrfs-error-quark");
diff --git a/src/lib/plugin_apis/crypto.api b/src/lib/plugin_apis/crypto.api
index e3d69986..ef0217fe 100644
--- a/src/lib/plugin_apis/crypto.api
+++ b/src/lib/plugin_apis/crypto.api
@@ -1,7 +1,7 @@
#include <glib.h>
#include <blockdev/utils.h>
-#define BD_CRYPTO_LUKS_METADATA_SIZE (2 MiB)
+#define BD_CRYPTO_LUKS_METADATA_SIZE G_GUINT64_CONSTANT (2097152ULL) // 2 MiB
GQuark bd_crypto_error_quark (void) {
return g_quark_from_static_string ("g-bd-crypto-error-quark");
diff --git a/src/lib/plugin_apis/lvm.api b/src/lib/plugin_apis/lvm.api
index 21c8f74d..ea52263b 100644
--- a/src/lib/plugin_apis/lvm.api
+++ b/src/lib/plugin_apis/lvm.api
@@ -15,18 +15,18 @@
#define BD_LVM_MAX_LV_SIZE G_GUINT64_CONSTANT (9223372036854775808ULL)
-#define BD_LVM_DEFAULT_PE_START (1 MiB)
-#define BD_LVM_DEFAULT_PE_SIZE (4 MiB)
-#define BD_LVM_MIN_PE_SIZE (1 KiB)
-#define BD_LVM_MAX_PE_SIZE (16 GiB)
-#define BD_LVM_MIN_THPOOL_MD_SIZE (2 MiB)
-#define BD_LVM_MAX_THPOOL_MD_SIZE (16 GiB)
-#define BD_LVM_MIN_THPOOL_CHUNK_SIZE (64 KiB)
-#define BD_LVM_MAX_THPOOL_CHUNK_SIZE (1 GiB)
-#define BD_LVM_DEFAULT_CHUNK_SIZE (64 KiB)
+#define BD_LVM_DEFAULT_PE_START G_GUINT64_CONSTANT (1048576ULL) // 1 MiB
+#define BD_LVM_DEFAULT_PE_SIZE G_GUINT64_CONSTANT (4194304ULL) // 4 MiB
+#define BD_LVM_MIN_PE_SIZE G_GUINT64_CONSTANT (1024ULL) // 1 KiB
+#define BD_LVM_MAX_PE_SIZE G_GUINT64_CONSTANT (17179869184ULL) // 16 GiB
+#define BD_LVM_MIN_THPOOL_MD_SIZE G_GUINT64_CONSTANT (2097152ULL) // 2 MiB
+#define BD_LVM_MAX_THPOOL_MD_SIZE G_GUINT64_CONSTANT (17179869184ULL) // 16 GiB
+#define BD_LVM_MIN_THPOOL_CHUNK_SIZE G_GUINT64_CONSTANT (65536ULL) // 64 KiB
+#define BD_LVM_MAX_THPOOL_CHUNK_SIZE G_GUINT64_CONSTANT (1073741824ULL) // 1 GiB
+#define BD_LVM_DEFAULT_CHUNK_SIZE G_GUINT64_CONSTANT (65536ULL) // 64 KiB
/* according to lvmcache (7) */
-#define BD_LVM_MIN_CACHE_MD_SIZE (8 MiB)
+#define BD_LVM_MIN_CACHE_MD_SIZE (8388608ULL) // 8 MiB
GQuark bd_lvm_error_quark (void) {
return g_quark_from_static_string ("g-bd-lvm-error-quark");
diff --git a/src/lib/plugin_apis/mdraid.api b/src/lib/plugin_apis/mdraid.api
index 3bd9eaf2..02ca9530 100644
--- a/src/lib/plugin_apis/mdraid.api
+++ b/src/lib/plugin_apis/mdraid.api
@@ -7,8 +7,8 @@
/* taken from blivet */
// these defaults were determined empirically
-#define BD_MD_SUPERBLOCK_SIZE (2 MiB)
-#define BD_MD_CHUNK_SIZE (512 KiB)
+#define BD_MD_SUPERBLOCK_SIZE G_GUINT64_CONSTANT (2097152ULL) // 2 MiB
+#define BD_MD_CHUNK_SIZE G_GUINT64_CONSTANT (524288ULL) // 512 KiB
GQuark bd_md_error_quark (void) {
return g_quark_from_static_string ("g-bd-md-error-quark");
From aeedce3bcaa8182c9878cc51d3f85a6c6eb6a01f Mon Sep 17 00:00:00 2001
From: Vojtech Trefny <vtrefny@redhat.com>
Date: Thu, 3 Dec 2020 14:41:25 +0100
Subject: [PATCH 2/5] lvm: Set thin metadata limits to match limits LVM uses in
lvcreate
---
src/lib/plugin_apis/lvm.api | 4 ++--
src/plugins/lvm.h | 9 +++++++--
tests/lvm_dbus_tests.py | 7 ++++---
tests/lvm_test.py | 7 ++++---
4 files changed, 17 insertions(+), 10 deletions(-)
diff --git a/src/lib/plugin_apis/lvm.api b/src/lib/plugin_apis/lvm.api
index ea52263b..e843c916 100644
--- a/src/lib/plugin_apis/lvm.api
+++ b/src/lib/plugin_apis/lvm.api
@@ -19,8 +19,8 @@
#define BD_LVM_DEFAULT_PE_SIZE G_GUINT64_CONSTANT (4194304ULL) // 4 MiB
#define BD_LVM_MIN_PE_SIZE G_GUINT64_CONSTANT (1024ULL) // 1 KiB
#define BD_LVM_MAX_PE_SIZE G_GUINT64_CONSTANT (17179869184ULL) // 16 GiB
-#define BD_LVM_MIN_THPOOL_MD_SIZE G_GUINT64_CONSTANT (2097152ULL) // 2 MiB
-#define BD_LVM_MAX_THPOOL_MD_SIZE G_GUINT64_CONSTANT (17179869184ULL) // 16 GiB
+#define BD_LVM_MIN_THPOOL_MD_SIZE G_GUINT64_CONSTANT (4194304ULL) // 4 MiB
+#define BD_LVM_MAX_THPOOL_MD_SIZE G_GUINT64_CONSTANT (33957085184ULL) // 31.62 GiB
#define BD_LVM_MIN_THPOOL_CHUNK_SIZE G_GUINT64_CONSTANT (65536ULL) // 64 KiB
#define BD_LVM_MAX_THPOOL_CHUNK_SIZE G_GUINT64_CONSTANT (1073741824ULL) // 1 GiB
#define BD_LVM_DEFAULT_CHUNK_SIZE G_GUINT64_CONSTANT (65536ULL) // 64 KiB
diff --git a/src/plugins/lvm.h b/src/plugins/lvm.h
index 4b970f2e..01c06ca4 100644
--- a/src/plugins/lvm.h
+++ b/src/plugins/lvm.h
@@ -1,5 +1,6 @@
#include <glib.h>
#include <blockdev/utils.h>
+#include <libdevmapper.h>
#ifndef BD_LVM
#define BD_LVM
@@ -21,8 +22,12 @@
#define USE_DEFAULT_PE_SIZE 0
#define RESOLVE_PE_SIZE(size) ((size) == USE_DEFAULT_PE_SIZE ? BD_LVM_DEFAULT_PE_SIZE : (size))
-#define BD_LVM_MIN_THPOOL_MD_SIZE (2 MiB)
-#define BD_LVM_MAX_THPOOL_MD_SIZE (16 GiB)
+/* lvm constants for thin pool metadata size are actually half of these
+ but when they calculate the actual metadata size they double the limits
+ so lets just double the limits here too */
+#define BD_LVM_MIN_THPOOL_MD_SIZE (4 MiB)
+#define BD_LVM_MAX_THPOOL_MD_SIZE (DM_THIN_MAX_METADATA_SIZE KiB)
+
#define BD_LVM_MIN_THPOOL_CHUNK_SIZE (64 KiB)
#define BD_LVM_MAX_THPOOL_CHUNK_SIZE (1 GiB)
#define BD_LVM_DEFAULT_CHUNK_SIZE (64 KiB)
diff --git a/tests/lvm_dbus_tests.py b/tests/lvm_dbus_tests.py
index 47402fc3..b517aae9 100644
--- a/tests/lvm_dbus_tests.py
+++ b/tests/lvm_dbus_tests.py
@@ -160,12 +160,13 @@ def test_get_thpool_meta_size(self):
def test_is_valid_thpool_md_size(self):
"""Verify that is_valid_thpool_md_size works as expected"""
- self.assertTrue(BlockDev.lvm_is_valid_thpool_md_size(2 * 1024**2))
- self.assertTrue(BlockDev.lvm_is_valid_thpool_md_size(3 * 1024**2))
+ self.assertTrue(BlockDev.lvm_is_valid_thpool_md_size(4 * 1024**2))
+ self.assertTrue(BlockDev.lvm_is_valid_thpool_md_size(5 * 1024**2))
self.assertTrue(BlockDev.lvm_is_valid_thpool_md_size(16 * 1024**3))
self.assertFalse(BlockDev.lvm_is_valid_thpool_md_size(1 * 1024**2))
- self.assertFalse(BlockDev.lvm_is_valid_thpool_md_size(17 * 1024**3))
+ self.assertFalse(BlockDev.lvm_is_valid_thpool_md_size(3 * 1024**2))
+ self.assertFalse(BlockDev.lvm_is_valid_thpool_md_size(32 * 1024**3))
@tag_test(TestTags.NOSTORAGE)
def test_is_valid_thpool_chunk_size(self):
diff --git a/tests/lvm_test.py b/tests/lvm_test.py
index 149cf54a..d0085651 100644
--- a/tests/lvm_test.py
+++ b/tests/lvm_test.py
@@ -153,12 +153,13 @@ def test_get_thpool_meta_size(self):
def test_is_valid_thpool_md_size(self):
"""Verify that is_valid_thpool_md_size works as expected"""
- self.assertTrue(BlockDev.lvm_is_valid_thpool_md_size(2 * 1024**2))
- self.assertTrue(BlockDev.lvm_is_valid_thpool_md_size(3 * 1024**2))
+ self.assertTrue(BlockDev.lvm_is_valid_thpool_md_size(4 * 1024**2))
+ self.assertTrue(BlockDev.lvm_is_valid_thpool_md_size(5 * 1024**2))
self.assertTrue(BlockDev.lvm_is_valid_thpool_md_size(16 * 1024**3))
self.assertFalse(BlockDev.lvm_is_valid_thpool_md_size(1 * 1024**2))
- self.assertFalse(BlockDev.lvm_is_valid_thpool_md_size(17 * 1024**3))
+ self.assertFalse(BlockDev.lvm_is_valid_thpool_md_size(3 * 1024**2))
+ self.assertFalse(BlockDev.lvm_is_valid_thpool_md_size(32 * 1024**3))
@tag_test(TestTags.NOSTORAGE)
def test_is_valid_thpool_chunk_size(self):
From 15fcf1bb62865083a3483fc51f45e11cdc2d7386 Mon Sep 17 00:00:00 2001
From: Vojtech Trefny <vtrefny@redhat.com>
Date: Thu, 3 Dec 2020 14:46:00 +0100
Subject: [PATCH 3/5] lvm: Do not use thin_metadata_size to recommend thin
metadata size
thin_metadata_size calculates the metadata size differently and
recommends smaller metadata than lvcreate so we should use the
lvcreate formula instead.
Resolves: rhbz#1898668
---
dist/libblockdev.spec.in | 4 --
src/lib/plugin_apis/lvm.api | 8 +--
src/plugins/lvm-dbus.c | 78 ++++++-----------------------
src/plugins/lvm.c | 66 +++++++-----------------
src/python/gi/overrides/BlockDev.py | 6 +++
tests/library_test.py | 6 +--
tests/lvm_dbus_tests.py | 20 ++++----
tests/lvm_test.py | 15 ++----
tests/utils.py | 2 +-
9 files changed, 61 insertions(+), 144 deletions(-)
diff --git a/dist/libblockdev.spec.in b/dist/libblockdev.spec.in
index 7c775174..3e0a53c6 100644
--- a/dist/libblockdev.spec.in
+++ b/dist/libblockdev.spec.in
@@ -387,8 +387,6 @@ BuildRequires: device-mapper-devel
Summary: The LVM plugin for the libblockdev library
Requires: %{name}-utils%{?_isa} >= 0.11
Requires: lvm2
-# for thin_metadata_size
-Requires: device-mapper-persistent-data
%description lvm
The libblockdev library plugin (and in the same time a standalone library)
@@ -411,8 +409,6 @@ BuildRequires: device-mapper-devel
Summary: The LVM plugin for the libblockdev library
Requires: %{name}-utils%{?_isa} >= 1.4
Requires: lvm2-dbusd >= 2.02.156
-# for thin_metadata_size
-Requires: device-mapper-persistent-data
%description lvm-dbus
The libblockdev library plugin (and in the same time a standalone library)
diff --git a/src/lib/plugin_apis/lvm.api b/src/lib/plugin_apis/lvm.api
index e843c916..9f25c1ed 100644
--- a/src/lib/plugin_apis/lvm.api
+++ b/src/lib/plugin_apis/lvm.api
@@ -704,11 +704,13 @@ guint64 bd_lvm_get_thpool_padding (guint64 size, guint64 pe_size, gboolean inclu
* bd_lvm_get_thpool_meta_size:
* @size: size of the thin pool
* @chunk_size: chunk size of the thin pool or 0 to use the default (%BD_LVM_DEFAULT_CHUNK_SIZE)
- * @n_snapshots: number of snapshots that will be created in the pool
+ * @n_snapshots: ignored
* @error: (out): place to store error (if any)
*
- * Returns: recommended size of the metadata space for the specified pool or 0
- * in case of error
+ * Note: This function will be changed in 3.0: the @n_snapshots parameter
+ * is currently not used and will be removed.
+ *
+ * Returns: recommended size of the metadata space for the specified pool
*
* Tech category: %BD_LVM_TECH_THIN_CALCS no mode (it is ignored)
*/
diff --git a/src/plugins/lvm-dbus.c b/src/plugins/lvm-dbus.c
index 9f821a99..24d54426 100644
--- a/src/plugins/lvm-dbus.c
+++ b/src/plugins/lvm-dbus.c
@@ -20,7 +20,6 @@
#include <glib.h>
#include <math.h>
#include <string.h>
-#include <libdevmapper.h>
#include <unistd.h>
#include <blockdev/utils.h>
#include <gio/gio.h>
@@ -248,14 +247,6 @@ static volatile guint avail_features = 0;
static volatile guint avail_module_deps = 0;
static GMutex deps_check_lock;
-#define DEPS_THMS 0
-#define DEPS_THMS_MASK (1 << DEPS_THMS)
-#define DEPS_LAST 1
-
-static const UtilDep deps[DEPS_LAST] = {
- {"thin_metadata_size", NULL, NULL, NULL},
-};
-
#define DBUS_DEPS_LVMDBUSD 0
#define DBUS_DEPS_LVMDBUSD_MASK (1 << DBUS_DEPS_LVMDBUSD)
#define DBUS_DEPS_LAST 1
@@ -301,17 +292,6 @@ gboolean bd_lvm_check_deps (void) {
check_ret = check_ret && success;
}
- for (i=0; i < DEPS_LAST; i++) {
- success = bd_utils_check_util_version (deps[i].name, deps[i].version,
- deps[i].ver_arg, deps[i].ver_regexp, &error);
- if (!success)
- g_warning ("%s", error->message);
- else
- g_atomic_int_or (&avail_deps, 1 << i);
- g_clear_error (&error);
- check_ret = check_ret && success;
- }
-
if (!check_ret)
g_warning("Cannot load the LVM plugin");
@@ -386,10 +366,7 @@ gboolean bd_lvm_is_tech_avail (BDLVMTech tech, guint64 mode, GError **error) {
g_set_error (error, BD_LVM_ERROR, BD_LVM_ERROR_TECH_UNAVAIL,
"Only 'query' supported for thin calculations");
return FALSE;
- } else if ((mode & BD_LVM_TECH_MODE_QUERY) &&
- !check_deps (&avail_deps, DEPS_THMS_MASK, deps, DEPS_LAST, &deps_check_lock, error))
- return FALSE;
- else
+ } else
return TRUE;
case BD_LVM_TECH_CALCS:
if (mode & ~BD_LVM_TECH_MODE_QUERY) {
@@ -1303,53 +1280,28 @@ guint64 bd_lvm_get_thpool_padding (guint64 size, guint64 pe_size, gboolean inclu
* bd_lvm_get_thpool_meta_size:
* @size: size of the thin pool
* @chunk_size: chunk size of the thin pool or 0 to use the default (%BD_LVM_DEFAULT_CHUNK_SIZE)
- * @n_snapshots: number of snapshots that will be created in the pool
+ * @n_snapshots: ignored
* @error: (out): place to store error (if any)
*
- * Returns: recommended size of the metadata space for the specified pool or 0
- * in case of error
+ * Note: This function will be changed in 3.0: the @n_snapshots parameter
+ * is currently not used and will be removed.
+ *
+ * Returns: recommended size of the metadata space for the specified pool
*
* Tech category: %BD_LVM_TECH_THIN_CALCS no mode (it is ignored)
*/
-guint64 bd_lvm_get_thpool_meta_size (guint64 size, guint64 chunk_size, guint64 n_snapshots, GError **error) {
- /* ub - output in bytes, n - output just the number */
- const gchar* args[7] = {"thin_metadata_size", "-ub", "-n", NULL, NULL, NULL, NULL};
- gchar *output = NULL;
- gboolean success = FALSE;
- guint64 ret = 0;
-
- if (!check_deps (&avail_deps, DEPS_THMS_MASK, deps, DEPS_LAST, &deps_check_lock, error))
- return 0;
+guint64 bd_lvm_get_thpool_meta_size (guint64 size, guint64 chunk_size, guint64 n_snapshots UNUSED, GError **error UNUSED) {
+ guint64 md_size = 0;
- /* s - total size, b - chunk size, m - number of snapshots */
- args[3] = g_strdup_printf ("-s%"G_GUINT64_FORMAT, size);
- args[4] = g_strdup_printf ("-b%"G_GUINT64_FORMAT,
- chunk_size != 0 ? chunk_size : (guint64) BD_LVM_DEFAULT_CHUNK_SIZE);
- args[5] = g_strdup_printf ("-m%"G_GUINT64_FORMAT, n_snapshots);
-
- success = bd_utils_exec_and_capture_output (args, NULL, &output, error);
- g_free ((gchar*) args[3]);
- g_free ((gchar*) args[4]);
- g_free ((gchar*) args[5]);
-
- if (!success) {
- /* error is already set */
- g_free (output);
- return 0;
- }
-
- ret = g_ascii_strtoull (output, NULL, 0);
- if (ret == 0) {
- g_set_error (error, BD_LVM_ERROR, BD_LVM_ERROR_PARSE,
- "Failed to parse number from thin_metadata_size's output: '%s'",
- output);
- g_free (output);
- return 0;
- }
+ /* based on lvcreate metadata size calculation */
+ md_size = UINT64_C(64) * size / (chunk_size ? chunk_size : BD_LVM_DEFAULT_CHUNK_SIZE);
- g_free (output);
+ if (md_size > BD_LVM_MAX_THPOOL_MD_SIZE)
+ md_size = BD_LVM_MAX_THPOOL_MD_SIZE;
+ else if (md_size < BD_LVM_MIN_THPOOL_MD_SIZE)
+ md_size = BD_LVM_MIN_THPOOL_MD_SIZE;
- return MAX (ret, BD_LVM_MIN_THPOOL_MD_SIZE);
+ return md_size;
}
/**
diff --git a/src/plugins/lvm.c b/src/plugins/lvm.c
index 6bfaa338..74493feb 100644
--- a/src/plugins/lvm.c
+++ b/src/plugins/lvm.c
@@ -20,7 +20,6 @@
#include <glib.h>
#include <math.h>
#include <string.h>
-#include <libdevmapper.h>
#include <unistd.h>
#include <blockdev/utils.h>
@@ -213,13 +212,10 @@ static GMutex deps_check_lock;
#define DEPS_LVM 0
#define DEPS_LVM_MASK (1 << DEPS_LVM)
-#define DEPS_THMS 1
-#define DEPS_THMS_MASK (1 << DEPS_THMS)
-#define DEPS_LAST 2
+#define DEPS_LAST 1
static const UtilDep deps[DEPS_LAST] = {
{"lvm", LVM_MIN_VERSION, "version", "LVM version:\\s+([\\d\\.]+)"},
- {"thin_metadata_size", NULL, NULL, NULL},
};
#define FEATURES_VDO 0
@@ -236,6 +232,8 @@ static const UtilFeatureDep features[FEATURES_LAST] = {
static const gchar*const module_deps[MODULE_DEPS_LAST] = { "kvdo" };
+#define UNUSED __attribute__((unused))
+
/**
* bd_lvm_check_deps:
*
@@ -317,10 +315,7 @@ gboolean bd_lvm_is_tech_avail (BDLVMTech tech, guint64 mode, GError **error) {
g_set_error (error, BD_LVM_ERROR, BD_LVM_ERROR_TECH_UNAVAIL,
"Only 'query' supported for thin calculations");
return FALSE;
- } else if ((mode & BD_LVM_TECH_MODE_QUERY) &&
- !check_deps (&avail_deps, DEPS_THMS_MASK, deps, DEPS_LAST, &deps_check_lock, error))
- return FALSE;
- else
+ } else
return TRUE;
case BD_LVM_TECH_CALCS:
if (mode & ~BD_LVM_TECH_MODE_QUERY) {
@@ -820,53 +815,28 @@ guint64 bd_lvm_get_thpool_padding (guint64 size, guint64 pe_size, gboolean inclu
* bd_lvm_get_thpool_meta_size:
* @size: size of the thin pool
* @chunk_size: chunk size of the thin pool or 0 to use the default (%BD_LVM_DEFAULT_CHUNK_SIZE)
- * @n_snapshots: number of snapshots that will be created in the pool
+ * @n_snapshots: ignored
* @error: (out): place to store error (if any)
*
- * Returns: recommended size of the metadata space for the specified pool or 0
- * in case of error
+ * Note: This function will be changed in 3.0: the @n_snapshots parameter
+ * is currently not used and will be removed.
+ *
+ * Returns: recommended size of the metadata space for the specified pool
*
* Tech category: %BD_LVM_TECH_THIN_CALCS no mode (it is ignored)
*/
-guint64 bd_lvm_get_thpool_meta_size (guint64 size, guint64 chunk_size, guint64 n_snapshots, GError **error) {
- /* ub - output in bytes, n - output just the number */
- const gchar* args[7] = {"thin_metadata_size", "-ub", "-n", NULL, NULL, NULL, NULL};
- gchar *output = NULL;
- gboolean success = FALSE;
- guint64 ret = 0;
-
- if (!check_deps (&avail_deps, DEPS_THMS_MASK, deps, DEPS_LAST, &deps_check_lock, error))
- return 0;
+guint64 bd_lvm_get_thpool_meta_size (guint64 size, guint64 chunk_size, guint64 n_snapshots UNUSED, GError **error UNUSED) {
+ guint64 md_size = 0;
- /* s - total size, b - chunk size, m - number of snapshots */
- args[3] = g_strdup_printf ("-s%"G_GUINT64_FORMAT, size);
- args[4] = g_strdup_printf ("-b%"G_GUINT64_FORMAT,
- chunk_size != 0 ? chunk_size : (guint64) BD_LVM_DEFAULT_CHUNK_SIZE);
- args[5] = g_strdup_printf ("-m%"G_GUINT64_FORMAT, n_snapshots);
+ /* based on lvcreate metadata size calculation */
+ md_size = UINT64_C(64) * size / (chunk_size ? chunk_size : BD_LVM_DEFAULT_CHUNK_SIZE);
- success = bd_utils_exec_and_capture_output (args, NULL, &output, error);
- g_free ((gchar*) args[3]);
- g_free ((gchar*) args[4]);
- g_free ((gchar*) args[5]);
-
- if (!success) {
- /* error is already set */
- g_free (output);
- return 0;
- }
-
- ret = g_ascii_strtoull (output, NULL, 0);
- if (ret == 0) {
- g_set_error (error, BD_LVM_ERROR, BD_LVM_ERROR_PARSE,
- "Failed to parse number from thin_metadata_size's output: '%s'",
- output);
- g_free (output);
- return 0;
- }
-
- g_free (output);
+ if (md_size > BD_LVM_MAX_THPOOL_MD_SIZE)
+ md_size = BD_LVM_MAX_THPOOL_MD_SIZE;
+ else if (md_size < BD_LVM_MIN_THPOOL_MD_SIZE)
+ md_size = BD_LVM_MIN_THPOOL_MD_SIZE;
- return MAX (ret, BD_LVM_MIN_THPOOL_MD_SIZE);
+ return md_size;
}
/**
diff --git a/src/python/gi/overrides/BlockDev.py b/src/python/gi/overrides/BlockDev.py
index d78bfaab..f768c8bd 100644
--- a/src/python/gi/overrides/BlockDev.py
+++ b/src/python/gi/overrides/BlockDev.py
@@ -462,6 +462,12 @@ def lvm_get_thpool_padding(size, pe_size=0, included=False):
return _lvm_get_thpool_padding(size, pe_size, included)
__all__.append("lvm_get_thpool_padding")
+_lvm_get_thpool_meta_size = BlockDev.lvm_get_thpool_meta_size
+@override(BlockDev.lvm_get_thpool_meta_size)
+def lvm_get_thpool_meta_size(size, chunk_size=0, n_snapshots=0):
+ return _lvm_get_thpool_meta_size(size, chunk_size, n_snapshots)
+__all__.append("lvm_get_thpool_meta_size")
+
_lvm_pvcreate = BlockDev.lvm_pvcreate
@override(BlockDev.lvm_pvcreate)
def lvm_pvcreate(device, data_alignment=0, metadata_size=0, extra=None, **kwargs):
diff --git a/tests/library_test.py b/tests/library_test.py
index e8bb175a..08e44fdc 100644
--- a/tests/library_test.py
+++ b/tests/library_test.py
@@ -349,8 +349,7 @@ def test_try_reinit(self):
# try reinitializing with only some utilities being available and thus
# only some plugins able to load
- with fake_path("tests/lib_missing_utils", keep_utils=["swapon", "swapoff", "mkswap", "lvm",
- "thin_metadata_size", "swaplabel"]):
+ with fake_path("tests/lib_missing_utils", keep_utils=["swapon", "swapoff", "mkswap", "lvm", "swaplabel"]):
succ, loaded = BlockDev.try_reinit(self.requested_plugins, True, None)
self.assertFalse(succ)
for plug_name in ("swap", "lvm", "crypto"):
@@ -361,8 +360,7 @@ def test_try_reinit(self):
# now the same with a subset of plugins requested
plugins = BlockDev.plugin_specs_from_names(["lvm", "swap", "crypto"])
- with fake_path("tests/lib_missing_utils", keep_utils=["swapon", "swapoff", "mkswap", "lvm",
- "thin_metadata_size", "swaplabel"]):
+ with fake_path("tests/lib_missing_utils", keep_utils=["swapon", "swapoff", "mkswap", "lvm","swaplabel"]):
succ, loaded = BlockDev.try_reinit(plugins, True, None)
self.assertTrue(succ)
self.assertEqual(set(loaded), set(["swap", "lvm", "crypto"]))
diff --git a/tests/lvm_dbus_tests.py b/tests/lvm_dbus_tests.py
index b517aae9..c06a480c 100644
--- a/tests/lvm_dbus_tests.py
+++ b/tests/lvm_dbus_tests.py
@@ -141,21 +141,19 @@ def test_get_thpool_padding(self):
def test_get_thpool_meta_size(self):
"""Verify that getting recommended thin pool metadata size works as expected"""
- # no idea how thin_metadata_size works, but let's at least check that
- # the function works and returns what thin_metadata_size says
- out1 = subprocess.check_output(["thin_metadata_size", "-ub", "-n", "-b64k", "-s1t", "-m100"])
- self.assertEqual(int(out1), BlockDev.lvm_get_thpool_meta_size (1 * 1024**4, 64 * 1024, 100))
+ # metadata size is calculated as 64 * pool_size / chunk_size
+ self.assertEqual(BlockDev.lvm_get_thpool_meta_size(1 * 1024**4, 64 * 1024), 1 * 1024**3)
- out2 = subprocess.check_output(["thin_metadata_size", "-ub", "-n", "-b128k", "-s1t", "-m100"])
- self.assertEqual(int(out2), BlockDev.lvm_get_thpool_meta_size (1 * 1024**4, 128 * 1024, 100))
+ self.assertEqual(BlockDev.lvm_get_thpool_meta_size(1 * 1024**4, 128 * 1024), 512 * 1024**2)
- # twice the chunk_size -> roughly half the metadata needed
- self.assertAlmostEqual(float(out1) / float(out2), 2, places=2)
-
- # unless thin_metadata_size gives a value that is not valid (too small)
- self.assertEqual(BlockDev.lvm_get_thpool_meta_size (100 * 1024**2, 128 * 1024, 100),
+ # lower limit is 4 MiB
+ self.assertEqual(BlockDev.lvm_get_thpool_meta_size(100 * 1024**2, 128 * 1024),
BlockDev.LVM_MIN_THPOOL_MD_SIZE)
+ # upper limit is 31.62 GiB
+ self.assertEqual(BlockDev.lvm_get_thpool_meta_size(100 * 1024**4, 64 * 1024),
+ BlockDev.LVM_MAX_THPOOL_MD_SIZE)
+
@tag_test(TestTags.NOSTORAGE)
def test_is_valid_thpool_md_size(self):
"""Verify that is_valid_thpool_md_size works as expected"""
diff --git a/tests/lvm_test.py b/tests/lvm_test.py
index d0085651..b84adece 100644
--- a/tests/lvm_test.py
+++ b/tests/lvm_test.py
@@ -134,19 +134,14 @@ def test_get_thpool_padding(self):
def test_get_thpool_meta_size(self):
"""Verify that getting recommended thin pool metadata size works as expected"""
- # no idea how thin_metadata_size works, but let's at least check that
- # the function works and returns what thin_metadata_size says
- out1 = subprocess.check_output(["thin_metadata_size", "-ub", "-n", "-b64k", "-s1t", "-m100"])
- self.assertEqual(int(out1), BlockDev.lvm_get_thpool_meta_size (1 * 1024**4, 64 * 1024, 100))
- out2 = subprocess.check_output(["thin_metadata_size", "-ub", "-n", "-b128k", "-s1t", "-m100"])
- self.assertEqual(int(out2), BlockDev.lvm_get_thpool_meta_size (1 * 1024**4, 128 * 1024, 100))
+ self.assertEqual(BlockDev.lvm_get_thpool_meta_size(1 * 1024**4, 64 * 1024),
+ 1 * 1024**3)
- # twice the chunk_size -> roughly half the metadata needed
- self.assertAlmostEqual(float(out1) / float(out2), 2, places=2)
+ self.assertEqual(BlockDev.lvm_get_thpool_meta_size(1 * 1024**4, 128 * 1024),
+ 512 * 1024**2)
- # unless thin_metadata_size gives a value that is not valid (too small)
- self.assertEqual(BlockDev.lvm_get_thpool_meta_size (100 * 1024**2, 128 * 1024, 100),
+ self.assertEqual(BlockDev.lvm_get_thpool_meta_size(100 * 1024**2, 128 * 1024),
BlockDev.LVM_MIN_THPOOL_MD_SIZE)
@tag_test(TestTags.NOSTORAGE)
diff --git a/tests/utils.py b/tests/utils.py
index 182eda6a..584fde5c 100644
--- a/tests/utils.py
+++ b/tests/utils.py
@@ -70,7 +70,7 @@ def fake_utils(path="."):
finally:
os.environ["PATH"] = old_path
-ALL_UTILS = {"lvm", "thin_metadata_size", "btrfs", "mkswap", "swaplabel", "multipath", "mpathconf", "dmsetup", "mdadm", "make-bcache", "sgdisk", "sfdisk"}
+ALL_UTILS = {"lvm", "btrfs", "mkswap", "swaplabel", "multipath", "mpathconf", "dmsetup", "mdadm", "make-bcache", "sgdisk", "sfdisk"}
@contextmanager
def fake_path(path=None, keep_utils=None, all_but=None):
From 27961a3fcb205ea51f40668d68765dd8d388777b Mon Sep 17 00:00:00 2001
From: Vojtech Trefny <vtrefny@redhat.com>
Date: Thu, 3 Dec 2020 14:48:08 +0100
Subject: [PATCH 4/5] lvm: Use the UNUSED macro instead of
__attribute__((unused))
for unused attributes. It makes the function definition a little
bit more readable.
---
src/plugins/lvm-dbus.c | 24 ++++++++++++------------
src/plugins/lvm.c | 20 ++++++++++----------
2 files changed, 22 insertions(+), 22 deletions(-)
diff --git a/src/plugins/lvm-dbus.c b/src/plugins/lvm-dbus.c
index 24d54426..b7b4480e 100644
--- a/src/plugins/lvm-dbus.c
+++ b/src/plugins/lvm-dbus.c
@@ -959,7 +959,7 @@ static BDLVMPVdata* get_pv_data_from_props (GVariant *props, GError **error) {
return data;
}
-static BDLVMVGdata* get_vg_data_from_props (GVariant *props, GError **error __attribute__((unused))) {
+static BDLVMVGdata* get_vg_data_from_props (GVariant *props, GError **error UNUSED) {
BDLVMVGdata *data = g_new0 (BDLVMVGdata, 1);
GVariantDict dict;
@@ -1061,7 +1061,7 @@ static BDLVMLVdata* get_lv_data_from_props (GVariant *props, GError **error) {
return data;
}
-static BDLVMVDOPooldata* get_vdo_data_from_props (GVariant *props, GError **error __attribute__((unused))) {
+static BDLVMVDOPooldata* get_vdo_data_from_props (GVariant *props, GError **error UNUSED) {
BDLVMVDOPooldata *data = g_new0 (BDLVMVDOPooldata, 1);
GVariantDict dict;
gchar *value = NULL;
@@ -1165,7 +1165,7 @@ static GVariant* create_size_str_param (guint64 size, const gchar *unit) {
*
* Tech category: %BD_LVM_TECH_CALCS no mode (it is ignored)
*/
-gboolean bd_lvm_is_supported_pe_size (guint64 size, GError **error __attribute__((unused))) {
+gboolean bd_lvm_is_supported_pe_size (guint64 size, GError **error UNUSED) {
return (((size % 2) == 0) && (size >= (BD_LVM_MIN_PE_SIZE)) && (size <= (BD_LVM_MAX_PE_SIZE)));
}
@@ -1177,7 +1177,7 @@ gboolean bd_lvm_is_supported_pe_size (guint64 size, GError **error __attribute__
*
* Tech category: %BD_LVM_TECH_CALCS no mode (it is ignored)
*/
-guint64 *bd_lvm_get_supported_pe_sizes (GError **error __attribute__((unused))) {
+guint64 *bd_lvm_get_supported_pe_sizes (GError **error UNUSED) {
guint8 i;
guint64 val = BD_LVM_MIN_PE_SIZE;
guint8 num_items = ((guint8) round (log2 ((double) BD_LVM_MAX_PE_SIZE))) - ((guint8) round (log2 ((double) BD_LVM_MIN_PE_SIZE))) + 2;
@@ -1199,7 +1199,7 @@ guint64 *bd_lvm_get_supported_pe_sizes (GError **error __attribute__((unused)))
*
* Tech category: %BD_LVM_TECH_CALCS no mode (it is ignored)
*/
-guint64 bd_lvm_get_max_lv_size (GError **error __attribute__((unused))) {
+guint64 bd_lvm_get_max_lv_size (GError **error UNUSED) {
return BD_LVM_MAX_LV_SIZE;
}
@@ -1219,7 +1219,7 @@ guint64 bd_lvm_get_max_lv_size (GError **error __attribute__((unused))) {
*
* Tech category: %BD_LVM_TECH_CALCS no mode (it is ignored)
*/
-guint64 bd_lvm_round_size_to_pe (guint64 size, guint64 pe_size, gboolean roundup, GError **error __attribute__((unused))) {
+guint64 bd_lvm_round_size_to_pe (guint64 size, guint64 pe_size, gboolean roundup, GError **error UNUSED) {
pe_size = RESOLVE_PE_SIZE(pe_size);
guint64 delta = size % pe_size;
if (delta == 0)
@@ -1313,7 +1313,7 @@ guint64 bd_lvm_get_thpool_meta_size (guint64 size, guint64 chunk_size, guint64 n
*
* Tech category: %BD_LVM_TECH_THIN_CALCS no mode (it is ignored)
*/
-gboolean bd_lvm_is_valid_thpool_md_size (guint64 size, GError **error __attribute__((unused))) {
+gboolean bd_lvm_is_valid_thpool_md_size (guint64 size, GError **error UNUSED) {
return ((BD_LVM_MIN_THPOOL_MD_SIZE <= size) && (size <= BD_LVM_MAX_THPOOL_MD_SIZE));
}
@@ -1327,7 +1327,7 @@ gboolean bd_lvm_is_valid_thpool_md_size (guint64 size, GError **error __attribut
*
* Tech category: %BD_LVM_TECH_THIN_CALCS no mode (it is ignored)
*/
-gboolean bd_lvm_is_valid_thpool_chunk_size (guint64 size, gboolean discard, GError **error __attribute__((unused))) {
+gboolean bd_lvm_is_valid_thpool_chunk_size (guint64 size, gboolean discard, GError **error UNUSED) {
gdouble size_log2 = 0.0;
if ((size < BD_LVM_MIN_THPOOL_CHUNK_SIZE) || (size > BD_LVM_MAX_THPOOL_CHUNK_SIZE))
@@ -2616,7 +2616,7 @@ gboolean bd_lvm_thsnapshotcreate (const gchar *vg_name, const gchar *origin_name
*
* Tech category: %BD_LVM_TECH_GLOB_CONF no mode (it is ignored)
*/
-gboolean bd_lvm_set_global_config (const gchar *new_config, GError **error __attribute__((unused))) {
+gboolean bd_lvm_set_global_config (const gchar *new_config, GError **error UNUSED) {
/* XXX: the error attribute will likely be used in the future when
some validation comes into the game */
@@ -2641,7 +2641,7 @@ gboolean bd_lvm_set_global_config (const gchar *new_config, GError **error __att
*
* Tech category: %BD_LVM_TECH_GLOB_CONF no mode (it is ignored)
*/
-gchar* bd_lvm_get_global_config (GError **error __attribute__((unused))) {
+gchar* bd_lvm_get_global_config (GError **error UNUSED) {
gchar *ret = NULL;
g_mutex_lock (&global_config_lock);
@@ -2660,7 +2660,7 @@ gchar* bd_lvm_get_global_config (GError **error __attribute__((unused))) {
*
* Tech category: %BD_LVM_TECH_CACHE_CALCS no mode (it is ignored)
*/
-guint64 bd_lvm_cache_get_default_md_size (guint64 cache_size, GError **error __attribute__((unused))) {
+guint64 bd_lvm_cache_get_default_md_size (guint64 cache_size, GError **error UNUSED) {
return MAX ((guint64) cache_size / 1000, BD_LVM_MIN_CACHE_MD_SIZE);
}
@@ -2670,7 +2670,7 @@ guint64 bd_lvm_cache_get_default_md_size (guint64 cache_size, GError **error __a
*
* Get LV type string from flags.
*/
-static const gchar* get_lv_type_from_flags (BDLVMCachePoolFlags flags, gboolean meta, GError **error __attribute__((unused))) {
+static const gchar* get_lv_type_from_flags (BDLVMCachePoolFlags flags, gboolean meta, GError **error UNUSED) {
if (!meta) {
if (flags & BD_LVM_CACHE_POOL_STRIPED)
return "striped";
diff --git a/src/plugins/lvm.c b/src/plugins/lvm.c
index 74493feb..2be1dbdb 100644
--- a/src/plugins/lvm.c
+++ b/src/plugins/lvm.c
@@ -700,7 +700,7 @@ static BDLVMVDOPooldata* get_vdo_data_from_table (GHashTable *table, gboolean fr
*
* Tech category: %BD_LVM_TECH_CALCS no mode (it is ignored)
*/
-gboolean bd_lvm_is_supported_pe_size (guint64 size, GError **error __attribute__((unused))) {
+gboolean bd_lvm_is_supported_pe_size (guint64 size, GError **error UNUSED) {
return (((size % 2) == 0) && (size >= (BD_LVM_MIN_PE_SIZE)) && (size <= (BD_LVM_MAX_PE_SIZE)));
}
@@ -712,7 +712,7 @@ gboolean bd_lvm_is_supported_pe_size (guint64 size, GError **error __attribute__
*
* Tech category: %BD_LVM_TECH_CALCS no mode (it is ignored)
*/
-guint64 *bd_lvm_get_supported_pe_sizes (GError **error __attribute__((unused))) {
+guint64 *bd_lvm_get_supported_pe_sizes (GError **error UNUSED) {
guint8 i;
guint64 val = BD_LVM_MIN_PE_SIZE;
guint8 num_items = ((guint8) round (log2 ((double) BD_LVM_MAX_PE_SIZE))) - ((guint8) round (log2 ((double) BD_LVM_MIN_PE_SIZE))) + 2;
@@ -734,7 +734,7 @@ guint64 *bd_lvm_get_supported_pe_sizes (GError **error __attribute__((unused)))
*
* Tech category: %BD_LVM_TECH_CALCS no mode (it is ignored)
*/
-guint64 bd_lvm_get_max_lv_size (GError **error __attribute__((unused))) {
+guint64 bd_lvm_get_max_lv_size (GError **error UNUSED) {
return BD_LVM_MAX_LV_SIZE;
}
@@ -754,7 +754,7 @@ guint64 bd_lvm_get_max_lv_size (GError **error __attribute__((unused))) {
*
* Tech category: %BD_LVM_TECH_CALCS no mode (it is ignored)
*/
-guint64 bd_lvm_round_size_to_pe (guint64 size, guint64 pe_size, gboolean roundup, GError **error __attribute__((unused))) {
+guint64 bd_lvm_round_size_to_pe (guint64 size, guint64 pe_size, gboolean roundup, GError **error UNUSED) {
pe_size = RESOLVE_PE_SIZE(pe_size);
guint64 delta = size % pe_size;
if (delta == 0)
@@ -848,7 +848,7 @@ guint64 bd_lvm_get_thpool_meta_size (guint64 size, guint64 chunk_size, guint64 n
*
* Tech category: %BD_LVM_TECH_THIN_CALCS no mode (it is ignored)
*/
-gboolean bd_lvm_is_valid_thpool_md_size (guint64 size, GError **error __attribute__((unused))) {
+gboolean bd_lvm_is_valid_thpool_md_size (guint64 size, GError **error UNUSED) {
return ((BD_LVM_MIN_THPOOL_MD_SIZE <= size) && (size <= BD_LVM_MAX_THPOOL_MD_SIZE));
}
@@ -862,7 +862,7 @@ gboolean bd_lvm_is_valid_thpool_md_size (guint64 size, GError **error __attribut
*
* Tech category: %BD_LVM_TECH_THIN_CALCS no mode (it is ignored)
*/
-gboolean bd_lvm_is_valid_thpool_chunk_size (guint64 size, gboolean discard, GError **error __attribute__((unused))) {
+gboolean bd_lvm_is_valid_thpool_chunk_size (guint64 size, gboolean discard, GError **error UNUSED) {
gdouble size_log2 = 0.0;
if ((size < BD_LVM_MIN_THPOOL_CHUNK_SIZE) || (size > BD_LVM_MAX_THPOOL_CHUNK_SIZE))
@@ -1983,7 +1983,7 @@ gboolean bd_lvm_thsnapshotcreate (const gchar *vg_name, const gchar *origin_name
*
* Tech category: %BD_LVM_TECH_GLOB_CONF no mode (it is ignored)
*/
-gboolean bd_lvm_set_global_config (const gchar *new_config, GError **error __attribute__((unused))) {
+gboolean bd_lvm_set_global_config (const gchar *new_config, GError **error UNUSED) {
/* XXX: the error attribute will likely be used in the future when
some validation comes into the game */
@@ -2008,7 +2008,7 @@ gboolean bd_lvm_set_global_config (const gchar *new_config, GError **error __att
*
* Tech category: %BD_LVM_TECH_GLOB_CONF no mode (it is ignored)
*/
-gchar* bd_lvm_get_global_config (GError **error __attribute__((unused))) {
+gchar* bd_lvm_get_global_config (GError **error UNUSED) {
gchar *ret = NULL;
g_mutex_lock (&global_config_lock);
@@ -2027,7 +2027,7 @@ gchar* bd_lvm_get_global_config (GError **error __attribute__((unused))) {
*
* Tech category: %BD_LVM_TECH_CACHE_CALCS no mode (it is ignored)
*/
-guint64 bd_lvm_cache_get_default_md_size (guint64 cache_size, GError **error __attribute__((unused))) {
+guint64 bd_lvm_cache_get_default_md_size (guint64 cache_size, GError **error UNUSED) {
return MAX ((guint64) cache_size / 1000, BD_LVM_MIN_CACHE_MD_SIZE);
}
@@ -2037,7 +2037,7 @@ guint64 bd_lvm_cache_get_default_md_size (guint64 cache_size, GError **error __a
*
* Get LV type string from flags.
*/
-static const gchar* get_lv_type_from_flags (BDLVMCachePoolFlags flags, gboolean meta, GError **error __attribute__((unused))) {
+static const gchar* get_lv_type_from_flags (BDLVMCachePoolFlags flags, gboolean meta, GError **error UNUSED) {
if (!meta) {
if (flags & BD_LVM_CACHE_POOL_STRIPED)
return "striped";
From f106e775d3c73e5f97512dd109627e00539b703a Mon Sep 17 00:00:00 2001
From: Vojtech Trefny <vtrefny@redhat.com>
Date: Tue, 15 Dec 2020 14:53:13 +0100
Subject: [PATCH 5/5] Fix max size limit for LVM thinpool metadata
DM_THIN_MAX_METADATA_SIZE is in 512 sectors, not in KiB so the
upper limit is 15.81 GiB not 31.62 GiB
---
src/lib/plugin_apis/lvm.api | 2 +-
src/plugins/lvm.h | 10 ++++++----
tests/lvm_dbus_tests.py | 3 ++-
tests/lvm_test.py | 3 ++-
4 files changed, 11 insertions(+), 7 deletions(-)
diff --git a/src/lib/plugin_apis/lvm.api b/src/lib/plugin_apis/lvm.api
index 9f25c1ed..563c1041 100644
--- a/src/lib/plugin_apis/lvm.api
+++ b/src/lib/plugin_apis/lvm.api
@@ -20,7 +20,7 @@
#define BD_LVM_MIN_PE_SIZE G_GUINT64_CONSTANT (1024ULL) // 1 KiB
#define BD_LVM_MAX_PE_SIZE G_GUINT64_CONSTANT (17179869184ULL) // 16 GiB
#define BD_LVM_MIN_THPOOL_MD_SIZE G_GUINT64_CONSTANT (4194304ULL) // 4 MiB
-#define BD_LVM_MAX_THPOOL_MD_SIZE G_GUINT64_CONSTANT (33957085184ULL) // 31.62 GiB
+#define BD_LVM_MAX_THPOOL_MD_SIZE G_GUINT64_CONSTANT (16978542592ULL) // 15.81 GiB
#define BD_LVM_MIN_THPOOL_CHUNK_SIZE G_GUINT64_CONSTANT (65536ULL) // 64 KiB
#define BD_LVM_MAX_THPOOL_CHUNK_SIZE G_GUINT64_CONSTANT (1073741824ULL) // 1 GiB
#define BD_LVM_DEFAULT_CHUNK_SIZE G_GUINT64_CONSTANT (65536ULL) // 64 KiB
diff --git a/src/plugins/lvm.h b/src/plugins/lvm.h
index 01c06ca4..2162d769 100644
--- a/src/plugins/lvm.h
+++ b/src/plugins/lvm.h
@@ -22,11 +22,13 @@
#define USE_DEFAULT_PE_SIZE 0
#define RESOLVE_PE_SIZE(size) ((size) == USE_DEFAULT_PE_SIZE ? BD_LVM_DEFAULT_PE_SIZE : (size))
-/* lvm constants for thin pool metadata size are actually half of these
- but when they calculate the actual metadata size they double the limits
- so lets just double the limits here too */
+/* lvm constant for thin pool metadata size is actually half of this
+ but when they calculate the actual metadata size they double the limit
+ so lets just double the limit here too */
#define BD_LVM_MIN_THPOOL_MD_SIZE (4 MiB)
-#define BD_LVM_MAX_THPOOL_MD_SIZE (DM_THIN_MAX_METADATA_SIZE KiB)
+
+/* DM_THIN_MAX_METADATA_SIZE is in 512 sectors */
+#define BD_LVM_MAX_THPOOL_MD_SIZE (DM_THIN_MAX_METADATA_SIZE * 512)
#define BD_LVM_MIN_THPOOL_CHUNK_SIZE (64 KiB)
#define BD_LVM_MAX_THPOOL_CHUNK_SIZE (1 GiB)
diff --git a/tests/lvm_dbus_tests.py b/tests/lvm_dbus_tests.py
index c06a480c..8f2bb95d 100644
--- a/tests/lvm_dbus_tests.py
+++ b/tests/lvm_dbus_tests.py
@@ -160,10 +160,11 @@ def test_is_valid_thpool_md_size(self):
self.assertTrue(BlockDev.lvm_is_valid_thpool_md_size(4 * 1024**2))
self.assertTrue(BlockDev.lvm_is_valid_thpool_md_size(5 * 1024**2))
- self.assertTrue(BlockDev.lvm_is_valid_thpool_md_size(16 * 1024**3))
+ self.assertTrue(BlockDev.lvm_is_valid_thpool_md_size(15 * 1024**3))
self.assertFalse(BlockDev.lvm_is_valid_thpool_md_size(1 * 1024**2))
self.assertFalse(BlockDev.lvm_is_valid_thpool_md_size(3 * 1024**2))
+ self.assertFalse(BlockDev.lvm_is_valid_thpool_md_size(16 * 1024**3))
self.assertFalse(BlockDev.lvm_is_valid_thpool_md_size(32 * 1024**3))
@tag_test(TestTags.NOSTORAGE)
diff --git a/tests/lvm_test.py b/tests/lvm_test.py
index b84adece..6f80a3ba 100644
--- a/tests/lvm_test.py
+++ b/tests/lvm_test.py
@@ -150,10 +150,11 @@ def test_is_valid_thpool_md_size(self):
self.assertTrue(BlockDev.lvm_is_valid_thpool_md_size(4 * 1024**2))
self.assertTrue(BlockDev.lvm_is_valid_thpool_md_size(5 * 1024**2))
- self.assertTrue(BlockDev.lvm_is_valid_thpool_md_size(16 * 1024**3))
+ self.assertTrue(BlockDev.lvm_is_valid_thpool_md_size(15 * 1024**3))
self.assertFalse(BlockDev.lvm_is_valid_thpool_md_size(1 * 1024**2))
self.assertFalse(BlockDev.lvm_is_valid_thpool_md_size(3 * 1024**2))
+ self.assertFalse(BlockDev.lvm_is_valid_thpool_md_size(16 * 1024**3))
self.assertFalse(BlockDev.lvm_is_valid_thpool_md_size(32 * 1024**3))
@tag_test(TestTags.NOSTORAGE)

20
SPECS/libblockdev.spec
View File

@ -125,12 +125,14 @@
Name: libblockdev
Version: 2.24
Release: 2%{?dist}
Release: 5%{?dist}
Summary: A library for low-level manipulation with block devices
License: LGPLv2+
URL: https://github.com/storaged-project/libblockdev
Source0: https://github.com/storaged-project/libblockdev/releases/download/%{version}-%{release}/%{name}-%{version}.tar.gz
Patch0: 0001-exec-Fix-setting-locale-for-util-calls.patch
Patch1: 0002-exec-polling-fixes.patch
Patch2: 0003-LVM-thin-metadata-calculation-fix.patch
BuildRequires: glib2-devel
%if %{with_gi}
@ -687,6 +689,8 @@ A meta-package that pulls all the libblockdev plugins as dependencies.
%prep
%setup -q -n %{name}-%{version}
%patch0 -p1
%patch1 -p1
%patch2 -p1
%build
autoreconf -ivf
@ -990,9 +994,21 @@ find %{buildroot} -type f -name "*.la" | xargs %{__rm}
%files plugins-all
%changelog
* Mon Jan 11 2021 Vojtech Trefny <vtrefny@redhat.com> - 2.24-5
- Fix LVM thin metadata calculation fix
Resolves: rhbz#1901714
* Mon Dec 14 2020 Vojtech Trefny <vtrefny@redhat.com> - 2.24-4
- LVM thin metadata calculation fix
Resolves: rhbz#1901714
* Wed Nov 18 2020 Vojtech Trefny <vtrefny@redhat.com> - 2.24-3
- exec: Polling fixes
Resolves: rhbz#1884689
* Mon Nov 09 2020 Vojtech Trefny <vtrefny@redhat.com> - 2.24-2
- exec: Fix setting locale for util calls
Resolves: rhbz#1895286
Resolves: rhbz#1880031
* Fri May 22 2020 Vojtech Trefny <vtrefny@redhat.com> - 2.24-1
- Rebased to the latest upstream release 2.24