[PATCH 1/2] virdevmapper: Don't error on kernels without DM support
Michal Privoznik
mprivozn at redhat.com
Mon Aug 17 15:40:04 UTC 2020
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 at redhat.com>
>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>> ---
>> 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
More information about the libvir-list
mailing list