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@xxxxxxxxxx> >> --- >> src/qemu/qemu_domain.c | 16 ++++++++-------- >> 1 file changed, 8 insertions(+), 8 deletions(-) > > Once the commit message makes sense: > > Reviewed-by: Peter Krempa <pkrempa@xxxxxxxxxx> > >> >> 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
