On 8/17/20 5:16 PM, Peter Krempa wrote:
On Mon, Aug 17, 2020 at 16:26:54 +0200, Michal Privoznik wrote:
In one of my latest patch (v6.6.0~30) I was trying to remove
libdevmapper use in favor of our own implementation. However, the
code did not take into account that device mapper can be not
compiled into the kernel (e.g. be a separate module that's not
loaded) in which case /proc/devices won't have the device-mapper
major number and thus virDevMapperGetTargets() and/or
virIsDevMapperDevice() fails.
Fixes: 22494556542c676d1b9e7f1c1f2ea13ac17e1e3e
Reported-by: Andrea Bolognani <abologna@xxxxxxxxxx>
Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
---
src/util/virdevmapper.c | 41 +++++++++++++++++++++++++++++++----------
1 file changed, 31 insertions(+), 10 deletions(-)
diff --git a/src/util/virdevmapper.c b/src/util/virdevmapper.c
index fe7f611496..cc33d8211e 100644
--- a/src/util/virdevmapper.c
+++ b/src/util/virdevmapper.c
@@ -47,10 +47,11 @@
G_STATIC_ASSERT(BUF_SIZE > sizeof(struct dm_ioctl));
static unsigned int virDMMajor;
+static virMutex virDevMapperInitMutex = VIR_MUTEX_INITIALIZER;
'virDMMajor' should now be initialized explicitly ...
Aren't global static variables initialized to zero automatically
(something about .bss section)?
static int
-virDevMapperOnceInit(void)
+virDevMapperGetMajor(unsigned int *dmmaj)
{
g_autofree char *buf = NULL;
VIR_AUTOSTRINGLIST lines = NULL;
@@ -69,23 +70,34 @@ virDevMapperOnceInit(void)
if (sscanf(lines[i], "%u %ms\n", &maj, &dev) == 2 &&
STREQ(dev, DM_NAME)) {
- virDMMajor = maj;
... since it's not always initialized here.
+ *dmmaj = maj;
break;
}
}
if (!lines[i]) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Unable to find major for %s"),
- DM_NAME);
- return -1;
+ /* Don't error here. Maybe the kernel was built without
+ * devmapper. Let the caller decide. */
+ return -2;
}
return 0;
}
-VIR_ONCE_GLOBAL_INIT(virDevMapper);
+static int
+virDevMapperInit(void)
+{
+ int rc = 0;
+
+ virMutexLock(&virDevMapperInitMutex);
+
+ if (virDMMajor == 0)
I'm not quite persuaded with this caching here. For cases when the
device mapper is loaded we cache it, but in the negative case ...
+ rc = virDevMapperGetMajor(&virDMMajor);
... we always process /proc/devices. Why is caching necessary then?
Are you suggesting to parse /proc/devices every time? My concern is it
will burn CPU cycles needlessly. And I'm not sure how to address the
negative case when DM module is not loaded. It's ineffective, I agree, I
just don't see how to make it better.
+
+ virMutexUnlock(&virDevMapperInitMutex);
+ return rc;
+}
static void *
@@ -220,7 +232,6 @@ virDevMapperGetTargetsImpl(int controlFD,
size_t i;
memset(&dm, 0, sizeof(dm));
- *devPaths_ret = NULL;
if (ttl == 0) {
errno = ELOOP;
@@ -298,14 +309,24 @@ virDevMapperGetTargets(const char *path,
char ***devPaths)
{
VIR_AUTOCLOSE controlFD = -1;
+ int rc;
const unsigned int ttl = 32;
/* Arbitrary limit on recursion level. A devmapper target can
* consist of devices or yet another targets. If that's the
* case, we have to stop recursion somewhere. */
- if (virDevMapperInitialize() < 0)
+ *devPaths = NULL;
+
+ if ((rc = virDevMapperInit()) < 0) {
+ if (rc == -2) {
+ /* Devmapper is not available. Yet. Maybe the module
+ * will be loaded later, but do not error out for now. */
+ return 0;
+ }
+
return -1;
+ }
if ((controlFD = virDMOpen()) < 0)
return -1;
@@ -319,7 +340,7 @@ virIsDevMapperDevice(const char *dev_name)
{
struct stat buf;
- if (virDevMapperInitialize() < 0)
+ if (virDevMapperInit() < 0)
return false;
if (!stat(dev_name, &buf) &&
Code after this hunk reads 'virDMMajor' directly without locks. Since
virDevMapperInit returns it here it should be used from the return value
rather than accessed directly which creates a code-pattern open for race
conditions.
I'm not sure I follow. virDevMapperInit() returns -2, -1, or 0. Not the
major. And reading without lock is fine because there is no way that the
virDMMajor is not set at this point (in which case the
virDevMapperInit() is basically a NOP. But if it makes us feel better I
can put some locking around.
Michal
