Hi,
On Wed, Nov 23, 2022 at 4:08 PM Alexandru Elisei
<alexandru.elisei@xxxxxxx> wrote:
>
> 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@xxxxxxxxxx>
> > ---
> > 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?
Will do.
> > 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.
I was following the behavior specified in the comment. That said, I
agree with you that returning an error should be the correct behavior.
I'll fix that and adjust the comment to reflect that.
Cheers,
/fuad
> Thanks,
> Alex
>
> > --
> > 2.38.1.431.g37b22c650d-goog
> >
