[PATCH 0/2] virDevMapperGetTargetsImpl: Check for dm major properly
*** BLURB HERE *** Michal Prívozník (2): util: Move virIsDevMapperDevice() to virdevmapper.c virDevMapperGetTargetsImpl: Check for dm major properly src/libvirt_private.syms | 2 +- src/storage/parthelper.c | 2 +- src/storage/storage_backend_disk.c | 1 + src/util/virdevmapper.c | 33 ++++++++++++++++++++++-------- src/util/virdevmapper.h | 3 +++ src/util/virutil.c | 24 ---------------------- src/util/virutil.h | 2 -- 7 files changed, 31 insertions(+), 36 deletions(-) -- 2.26.2
When introducing virdevmapper.c (in v4.3.0-rc1~427) I didn't realize there is a function that calls in devmapper. The function is called virIsDevMapperDevice() and lives in virutil.c. Now that we have a special file for handling devmapper move it there. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 2 +- src/storage/parthelper.c | 2 +- src/storage/storage_backend_disk.c | 1 + src/util/virdevmapper.c | 24 ++++++++++++++++++++++++ src/util/virdevmapper.h | 3 +++ src/util/virutil.c | 24 ------------------------ src/util/virutil.h | 2 -- 7 files changed, 30 insertions(+), 28 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a6af44fe1c..247d7f4741 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1943,6 +1943,7 @@ virDBusSetSharedBus; # util/virdevmapper.h virDevMapperGetTargets; +virIsDevMapperDevice; # util/virdnsmasq.h @@ -3432,7 +3433,6 @@ virGetUserShell; virHostGetDRMRenderNode; virHostHasIOMMU; virIndexToDiskName; -virIsDevMapperDevice; virMemoryLimitIsSet; virMemoryLimitTruncate; virMemoryMaxValue; diff --git a/src/storage/parthelper.c b/src/storage/parthelper.c index 761a7f93fc..812e90d3cb 100644 --- a/src/storage/parthelper.c +++ b/src/storage/parthelper.c @@ -36,10 +36,10 @@ #include <sys/stat.h> #include <unistd.h> -#include "virutil.h" #include "virfile.h" #include "virstring.h" #include "virgettext.h" +#include "virdevmapper.h" /* we don't need to include the full internal.h just for this */ #define STREQ(a, b) (strcmp(a, b) == 0) diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index 35b07abbfe..eae23ec24a 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -32,6 +32,7 @@ #include "virutil.h" #include "configmake.h" #include "virstring.h" +#include "virdevmapper.h" #define VIR_FROM_THIS VIR_FROM_STORAGE diff --git a/src/util/virdevmapper.c b/src/util/virdevmapper.c index 79dbc3d02a..600e1f6322 100644 --- a/src/util/virdevmapper.c +++ b/src/util/virdevmapper.c @@ -212,3 +212,27 @@ virDevMapperGetTargets(const char *path G_GNUC_UNUSED, return -1; } #endif /* ! WITH_DEVMAPPER */ + + +#if WITH_DEVMAPPER +bool +virIsDevMapperDevice(const char *dev_name) +{ + struct stat buf; + + if (!stat(dev_name, &buf) && + S_ISBLK(buf.st_mode) && + dm_is_dm_major(major(buf.st_rdev))) + return true; + + return false; +} + +#else /* ! WITH_DEVMAPPER */ + +bool +virIsDevMapperDevice(const char *dev_name G_GNUC_UNUSED) +{ + return false; +} +#endif /* ! WITH_DEVMAPPER */ diff --git a/src/util/virdevmapper.h b/src/util/virdevmapper.h index 87bbc63cfd..834900692e 100644 --- a/src/util/virdevmapper.h +++ b/src/util/virdevmapper.h @@ -25,3 +25,6 @@ int virDevMapperGetTargets(const char *path, char ***devPaths) G_GNUC_NO_INLINE; + +bool +virIsDevMapperDevice(const char *dev_name) ATTRIBUTE_NONNULL(1); diff --git a/src/util/virutil.c b/src/util/virutil.c index fb46501142..929afb82d0 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -37,10 +37,6 @@ #include <sys/types.h> -#if WITH_DEVMAPPER -# include <libdevmapper.h> -#endif - #ifdef HAVE_GETPWUID_R # include <pwd.h> # include <grp.h> @@ -1340,26 +1336,6 @@ void virWaitForDevices(void) ignore_value(virCommandRun(cmd, &exitstatus)); } -#if WITH_DEVMAPPER -bool -virIsDevMapperDevice(const char *dev_name) -{ - struct stat buf; - - if (!stat(dev_name, &buf) && - S_ISBLK(buf.st_mode) && - dm_is_dm_major(major(buf.st_rdev))) - return true; - - return false; -} -#else -bool virIsDevMapperDevice(const char *dev_name G_GNUC_UNUSED) -{ - return false; -} -#endif - bool virValidateWWN(const char *wwn) { diff --git a/src/util/virutil.h b/src/util/virutil.h index 49b4bf440f..ef01fd9e36 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -114,8 +114,6 @@ bool virDoesUserExist(const char *name); bool virDoesGroupExist(const char *name); -bool virIsDevMapperDevice(const char *dev_name) ATTRIBUTE_NONNULL(1); - bool virValidateWWN(const char *wwn); int virGetDeviceID(const char *path, -- 2.26.2
In v6.4.0-rc1~143 I've introduced a check that is supposed to return from the function early, if given path is not a dm target. While the idea is still valid, the implementation had a flaw. It calls stat() over given path and the uses major(sb.st_dev) to learn the major of the device. This is then passed to dm_is_dm_major() which returns true or false depending whether the device is under devmapper's control or not. The problem with this approach is in how the major of the device is obtained - paths managed by devmapper are special files and thus we want to be using st_rdev instead of st_dev to obtain the major number. Well, that's what virIsDevMapperDevice() does already so might as well us that. Fixes: 01626c668ecfbe465d18799ac4628e6127ea1d47 Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1839992 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virdevmapper.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/src/util/virdevmapper.c b/src/util/virdevmapper.c index 600e1f6322..40a82285f9 100644 --- a/src/util/virdevmapper.c +++ b/src/util/virdevmapper.c @@ -64,7 +64,6 @@ virDevMapperGetTargetsImpl(const char *path, char ***devPaths_ret, unsigned int ttl) { - struct stat sb; struct dm_task *dmt = NULL; struct dm_deps *deps; struct dm_info info; @@ -83,13 +82,7 @@ virDevMapperGetTargetsImpl(const char *path, return ret; } - if (stat(path, &sb) < 0) { - if (errno == ENOENT) - return 0; - return -1; - } - - if (!dm_is_dm_major(major(sb.st_dev))) + if (!virIsDevMapperDevice(path)) return 0; if (!(dmt = dm_task_create(DM_DEVICE_DEPS))) { -- 2.26.2
On Thu, Jun 11, 2020 at 13:44:46 +0200, Michal Privoznik wrote:
*** BLURB HERE ***
Michal Prívozník (2): util: Move virIsDevMapperDevice() to virdevmapper.c virDevMapperGetTargetsImpl: Check for dm major properly
src/libvirt_private.syms | 2 +- src/storage/parthelper.c | 2 +- src/storage/storage_backend_disk.c | 1 + src/util/virdevmapper.c | 33 ++++++++++++++++++++++-------- src/util/virdevmapper.h | 3 +++ src/util/virutil.c | 24 ---------------------- src/util/virutil.h | 2 -- 7 files changed, 31 insertions(+), 36 deletions(-)
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
participants (2)
-
Jiri Denemark -
Michal Privoznik