[PATCH] qemu: Don't crash when getting targets for a multipath

[Michal Prívozník]

On 19. 3. 2020 17:43, Peter Krempa wrote:
> On Thu, Mar 19, 2020 at 17:22:37 +0100, Michal Privoznik wrote:
>> In one of my previous commits I've introduced code that creates
>> all devices for given (possible) multipath target. But I've made
>> a mistake there - the code accesses src->path without checking if
>> the disk source is local. Note that the path is NULL if the
>> source is not local.
> 
> Well next->path 'may' be NULL if it's not local, but in this case it is
> local because NVMe disks are local, but they don't have the path.

Local as in virStorageSourceIsLocalStorage() == true. And for NVMe we
return false exactly because src->path has to be NULL (it can't be set
to any meaningfull path).

How about this formulation:

Note that the path is NULL if the
source is not local as viewed by
virStorageSourceIsLocalStorage().

> 
>>
>> Fixes: a30078cb832646177defd256e77c632905f1e6d0
>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1814947
>>
>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>> ---
>>  src/qemu/qemu_domain.c | 16 ++++++++--------
>>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> Once the commit message makes sense:
> 
> Reviewed-by: Peter Krempa <pkrempa at redhat.com>
> 
>>
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index 0e2252f6cf..ef17829235 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -15862,19 +15862,19 @@ qemuDomainNamespaceSetupDisk(virDomainObjPtr vm,
>>              }
>>  
>>              tmpPath = g_strdup(next->path);
>> +
>> +            if (virDevMapperGetTargets(next->path, &targetPaths) < 0 &&
>> +                errno != ENOSYS && errno != EBADF) {
>> +                virReportSystemError(errno,
>> +                                     _("Unable to get devmapper targets for %s"),
>> +                                     next->path);
>> +                return -1;
>> +            }
>>          }
>>  
>>          if (virStringListAdd(&paths, tmpPath) < 0)
>>              return -1;
>>  
>> -        if (virDevMapperGetTargets(next->path, &targetPaths) < 0 &&
>> -            errno != ENOSYS && errno != EBADF) {
>> -            virReportSystemError(errno,
>> -                                 _("Unable to get devmapper targets for %s"),
>> -                                 next->path);
>> -            return -1;
>> -        }
>> -
>>          if (virStringListMerge(&paths, &targetPaths) < 0)
>>              return -1;
> 
> Is this supposed to go after 'tmpPath' in the list? Otherwise you might
> want to move it toghether with the call to virDevMapperGetTargets.
> 

Good point, the order doesn't matter.

Michal