src/qemu/qemu_domain.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
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
© 2016 - 2024 Red Hat, Inc.