| Message ID | 20221115111549.2784927-2-tabba@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Use memfd for guest vm memory allocation | expand |
On Tue, 15 Nov 2022 11:15:33 +0000 Fuad Tabba <tabba@google.com> wrote: Hi, > If none of the bank types match, the function would return an > uninitialized value. > > Signed-off-by: Fuad Tabba <tabba@google.com> Indeed the comment promises to return 0 if there is no error. Reviewed-by: Andre Przywara <andre.przywara@arm.com> Cheers, Andre > --- > 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; > struct kvm_mem_bank *bank; > > list_for_each_entry(bank, &kvm->mem_banks, list) {
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 >
Hi, On Wed, Nov 23, 2022 at 4:08 PM Alexandru Elisei <alexandru.elisei@arm.com> 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@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? 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 > >
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; struct kvm_mem_bank *bank; list_for_each_entry(bank, &kvm->mem_banks, list) {
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(-)