It's convenient to call iova_tree_remove from a map returned from
iova_tree_find or iova_tree_find_iova. 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;
     const DMAMap *overlap;
 
-    while ((overlap = iova_tree_find(tree, map))) {
+    while ((overlap = iova_tree_find(tree, &needle))) {
         g_tree_remove(tree->tree, overlap);
     }
 }
-- 
2.31.1

在 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.


>       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?

Thanks



>   }

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!