Re: [PATCH 1/2] virdevmapper: Don't error on kernels without DM support

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(a)redhat.com&gt; > Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; > --- > 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 ...

> 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?

> + > + 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.