Re: [PATCH 3/7] util: make a copy of iova_tree_remove_parameter

[Eugenio Perez Martin]

On Tue, Aug 23, 2022 at 8:18 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> 在 2022/8/20 00:53, Eugenio Pérez 写道:
> > It's convenient to call iova_tree_remove from a map returned from
> > iova_tree_find or iova_tree_find_iova.
>
>
> The looks like a hint of the defect of current API.
>
>
> >   With the current code this is not
> > possible, since we will free it, and then we will try to search for it
> > again.
> >
> > Fix it making a copy of the argument. Not applying a fixes tag, since
> > there is no use like that at the moment.
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >   util/iova-tree.c | 4 +++-
> >   1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/util/iova-tree.c b/util/iova-tree.c
> > index fee530a579..713e3edd7b 100644
> > --- a/util/iova-tree.c
> > +++ b/util/iova-tree.c
> > @@ -166,9 +166,11 @@ void iova_tree_foreach(IOVATree *tree, 
> > iova_tree_iterator iterator)
> >
> >   void iova_tree_remove(IOVATree *tree, const DMAMap *map)
> >   {
> > +    /* Just in case caller is calling iova_tree_remove from a result of 
> > find */
> > +    const DMAMap needle = *map;
>
>
> Then let's simply make iova_tree_remove() accept const DMAMap instead of
> the pointer to it.
>

Do you mean to accept DMAMap by value, isn't it? (no need for const it
then, isn't it?).

>
> >       const DMAMap *overlap;
> >
> > -    while ((overlap = iova_tree_find(tree, map))) {
> > +    while ((overlap = iova_tree_find(tree, &needle))) {
> >           g_tree_remove(tree->tree, overlap);
> >       }
>
>
> So we had following in iova_tree_insert():
>
>      /* We don't allow to insert range that overlaps with existings */
>      if (iova_tree_find(tree, map)) {
>          return IOVA_ERR_OVERLAP;
>      }
>
> I wonder how overlap can happen?
>

I don't get this part. The problem is that iova_tree_find returns a
pointer to an internal structure, there is no need to insert multiple
times overlapping maps.

Thanks!