Re: [PATCH kvmtool v1 01/17] Initialize the return value in kvm__for_each_mem_bank()

[Alexandru Elisei <alexandru.elisei@arm.com>]

Hi,

On Tue, Nov 15, 2022 at 11:15:33AM +0000, Fuad Tabba wrote:
> If none of the bank types match, the function would return an
> uninitialized value.
> 
> Signed-off-by: Fuad Tabba <tabba@google.com>
> ---
>  kvm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kvm.c b/kvm.c
> index 42b8812..78bc0d8 100644
> --- a/kvm.c
> +++ b/kvm.c
> @@ -387,7 +387,7 @@ int kvm__for_each_mem_bank(struct kvm *kvm, enum kvm_mem_type type,
>  			   int (*fun)(struct kvm *kvm, struct kvm_mem_bank *bank, void *data),
>  			   void *data)
>  {
> -	int ret;
> +	int ret = 0;

Would you consider moving the variable declaration after the 'bank'
variable?

>  	struct kvm_mem_bank *bank;
>  
>  	list_for_each_entry(bank, &kvm->mem_banks, list) {

Shouldn't the function return an error if no memory banks matched the type
specified (initialize ret to -EINVAL instead of 0)? I'm thinking that if
the caller expects a certain type of memory bank to be present, but that
memory type is not present, then somehwere an error occured and the caller
should be made aware of it.

Case in point, kvm__for_each_mem_bank() is used vfio/core.c for
KVM_MEM_TYPE_RAM. If RAM hasn't been created by that point, then
VFIO_IOMMU_MAP_DMA will not be called for guest memory and the assigned
device will not work.

Thanks,
Alex

> -- 
> 2.38.1.431.g37b22c650d-goog
>