[PATCH] virpci: Report an error if virPCIGetVirtualFunctionIndex() fails
From: Michal Privoznik <mprivozn@redhat.com> Either an error should be returned in all error paths in a function or in none (leaving it up to caller). Well, virPCIGetVirtualFunctionIndex() breaks this pattern. Fix it. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virpci.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/util/virpci.c b/src/util/virpci.c index 78c47869ef..ca6f2e8210 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -2870,6 +2870,9 @@ virPCIGetVirtualFunctionIndex(const char *pf_sysfs_device_link, } } + virReportError(VIR_ERR_INTERNAL_ERROR, + _("No virtual function index found for '%1$s'"), + pf_sysfs_device_link); return -1; } -- 2.52.0
On 2/18/26 6:16 AM, Michal Privoznik via Devel wrote:
From: Michal Privoznik <mprivozn@redhat.com>
Either an error should be returned in all error paths in a function or in none (leaving it up to caller). Well, virPCIGetVirtualFunctionIndex() breaks this pattern. Fix it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
And it looks like this bug has been in the code since the function was originally written in commit 17d64cab16cb506fabbc0bdcc3994da5d0307005 in August 2011!! (although it wasn't as obvious prior to the removal of everything jumping to an "out:" label with explicit cleanup code rather than directly doing "return 0|-1;" with implicit cleanup) Was this error actually triggered in some way? It looks like the only way it could happen would be if the caller had sent in mismatched VF and PF devices (or if sysfs was somehow fubar). Reviewed-by: Laine Stump <laine@redhat.com>
--- src/util/virpci.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/src/util/virpci.c b/src/util/virpci.c index 78c47869ef..ca6f2e8210 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -2870,6 +2870,9 @@ virPCIGetVirtualFunctionIndex(const char *pf_sysfs_device_link, } }
+ virReportError(VIR_ERR_INTERNAL_ERROR, + _("No virtual function index found for '%1$s'"), + pf_sysfs_device_link); return -1; }
On 2/20/26 8:14 AM, Laine Stump wrote:
On 2/18/26 6:16 AM, Michal Privoznik via Devel wrote:
From: Michal Privoznik <mprivozn@redhat.com>
Either an error should be returned in all error paths in a function or in none (leaving it up to caller). Well, virPCIGetVirtualFunctionIndex() breaks this pattern. Fix it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
And it looks like this bug has been in the code since the function was originally written in commit 17d64cab16cb506fabbc0bdcc3994da5d0307005 in August 2011!! (although it wasn't as obvious prior to the removal of everything jumping to an "out:" label with explicit cleanup code rather than directly doing "return 0|-1;" with implicit cleanup)
Was this error actually triggered in some way? It looks like the only way it could happen would be if the caller had sent in mismatched VF and PF devices (or if sysfs was somehow fubar).
I think it stems from my experiments with introducing virusbmock to qemuxmlconftest. So maybe our mock (albeit in this case virpcimock) could create insufficient sysfs structure. BUT currently everything's fine. Honestly, I've found this on an old branch and the commit itself is ~ half a year old, so dunno :-D
Reviewed-by: Laine Stump <laine@redhat.com>
Merged, thanks. Michal
participants (3)
-
Laine Stump -
Michal Privoznik -
Michal Prívozník