[PATCH] qemu: Don't crash when getting targets for a multipath
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. Fixes: a30078cb832646177defd256e77c632905f1e6d0 Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1814947 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) 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; } -- 2.24.1
On a Thursday in 2020, 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.
Fixes: a30078cb832646177defd256e77c632905f1e6d0 Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1814947
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
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.
Fixes: a30078cb832646177defd256e77c632905f1e6d0 Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1814947
Signed-off-by: Michal Privoznik <mprivozn@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@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.
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@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@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
On Thu, Mar 19, 2020 at 18:39:58 +0100, Michal Privoznik wrote:
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).
Yes. The meaning of virStorageSourceIsLocalStorage shifted after NVMe disks were added.
How about this formulation:
Note that the path is NULL if the source is not local as viewed by virStorageSourceIsLocalStorage().
That is still misleading, because network disks can have non-null path. I'd prefer: 'next->path' is NULL/doesn't make sense for VIR_STORAGE_TYPE_NVME
participants (4)
-
Ján Tomko -
Michal Privoznik -
Michal Prívozník
-
Peter Krempa