Re: [PATCH v3 1/6] vfio/type1: Introduce iova list and add iommu aperture validity check

[Alex Williamson]

On Mon, 19 Feb 2018 09:50:24 +0000
Shameerali Kolothum Thodi <shameerali.kolothum.thodi@xxxxxxxxxx> wrote:
> > -----Original Message-----
> > From: Alex Williamson [mailto:alex.williamson@xxxxxxxxxx]
> > Sent: Friday, February 16, 2018 8:49 PM 
> > On Thu, 15 Feb 2018 09:44:59 +0000
> > Shameer Kolothum <shameerali.kolothum.thodi@xxxxxxxxxx> wrote:
> > > +			node->end = end;
> > > +			continue;
> > > +		}
> > > +		/* Delete nodes after new end */
> > > +		list_del(&node->list);
> > > +		kfree(node);
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int vfio_iommu_get_iova_copy(struct vfio_iommu *iommu,
> > > +				struct list_head *iova_copy)
> > > +{
> > > +
> > > +	struct list_head *iova = &iommu->iova_list;
> > > +	struct vfio_iova *n;
> > > +
> > > +	list_for_each_entry(n, iova, list) {
> > > +		int ret;
> > > +
> > > +		ret = vfio_insert_iova(n->start, n->end, iova_copy);
> > > +		if (ret)
> > > +			return ret;  
> > 
> > Let's delete and free any entries added to the copy here too.  
> 
> Ok. My original thought was caller will free up in case of error.

This comes down to Rusty's suggestions of how to make an API hard to
misuse rather than simply easy to use to me.  Placing the onus on the
caller to cleanup a list sounds simple, but the caller passed an empty
list and the function failed, why should the caller bother to check if
the function left any cruft on the list in the course of failing?  This
is not a hard to misuse interface, in fact it's very easy to forget
that cleanup.  Thanks,

Alex