[PATCH] qemu: Don't crash when getting targets for a multipath
Michal Prívozník
mprivozn at redhat.com
Thu Mar 19 17:39:58 UTC 2020
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
More information about the libvir-list
mailing list