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