Re: [ovs-dev] [PATCH v2] extend-table: Fix a bug that iterates wrong table

2018-10-02 Thread Yifeng Sun 
Good idea, let me work out a new version.

Thanks,
Yifeng

On Tue, Oct 2, 2018 at 3:03 PM Ben Pfaff  wrote:

> On Tue, Oct 02, 2018 at 11:37:03AM -0700, Yifeng Sun wrote:
> > This seems to be a copy and paste bug that iterates and frees
> > the wrong table. This commit fixes that.
> >
> > Reported-at: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=10730
> > Signed-off-by: Yifeng Sun 
> > ---
> > v1->v2: Fix email subject by adding [ovs-dev]
>
> Thank you for the fix.  It is valuable.
>
> Do you mind if we eliminate the code duplication, like this?
>
> Thanks,
>
> Ben.
>
> diff --git a/ovn/lib/extend-table.c b/ovn/lib/extend-table.c
> index 511d1a84b0c0..e42822be8ec1 100644
> --- a/ovn/lib/extend-table.c
> +++ b/ovn/lib/extend-table.c
> @@ -33,26 +33,24 @@ ovn_extend_table_init(struct ovn_extend_table *table)
>  hmap_init(>existing);
>  }
>
> +static void
> +ovn_extend_table_info_destroy(struct hmap *target)
> +{
> +struct ovn_extend_table_info *e, *next;
> +HMAP_FOR_EACH_SAFE (e, next, hmap_node, target) {
> +hmap_remove(target, >hmap_node);
> +free(e->name);
> +free(e);
> +}
> +hmap_destroy(target);
> +}
> +
>  void
>  ovn_extend_table_destroy(struct ovn_extend_table *table)
>  {
>  bitmap_free(table->table_ids);
> -
> -struct ovn_extend_table_info *desired, *d_next;
> -HMAP_FOR_EACH_SAFE (desired, d_next, hmap_node, >existing) {
> -hmap_remove(>existing, >hmap_node);
> -free(desired->name);
> -free(desired);
> -}
> -hmap_destroy(>desired);
> -
> -struct ovn_extend_table_info *existing, *e_next;
> -HMAP_FOR_EACH_SAFE (existing, e_next, hmap_node, >existing) {
> -hmap_remove(>existing, >hmap_node);
> -free(existing->name);
> -free(existing);
> -}
> -hmap_destroy(>existing);
> +ovn_extend_table_info_destroy(>desired);
> +ovn_extend_table_info_destroy(>existing);
>  }
>
>  /* Finds and returns a group_info in 'existing' whose key is identical
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2] extend-table: Fix a bug that iterates wrong table

2018-10-02 Thread Ben Pfaff 
On Tue, Oct 02, 2018 at 11:37:03AM -0700, Yifeng Sun wrote:
> This seems to be a copy and paste bug that iterates and frees
> the wrong table. This commit fixes that.
> 
> Reported-at: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=10730
> Signed-off-by: Yifeng Sun 
> ---
> v1->v2: Fix email subject by adding [ovs-dev]

Thank you for the fix.  It is valuable.

Do you mind if we eliminate the code duplication, like this?

Thanks,

Ben.

diff --git a/ovn/lib/extend-table.c b/ovn/lib/extend-table.c
index 511d1a84b0c0..e42822be8ec1 100644
--- a/ovn/lib/extend-table.c
+++ b/ovn/lib/extend-table.c
@@ -33,26 +33,24 @@ ovn_extend_table_init(struct ovn_extend_table *table)
 hmap_init(>existing);
 }
 
+static void
+ovn_extend_table_info_destroy(struct hmap *target)
+{
+struct ovn_extend_table_info *e, *next;
+HMAP_FOR_EACH_SAFE (e, next, hmap_node, target) {
+hmap_remove(target, >hmap_node);
+free(e->name);
+free(e);
+}
+hmap_destroy(target);
+}
+
 void
 ovn_extend_table_destroy(struct ovn_extend_table *table)
 {
 bitmap_free(table->table_ids);
-
-struct ovn_extend_table_info *desired, *d_next;
-HMAP_FOR_EACH_SAFE (desired, d_next, hmap_node, >existing) {
-hmap_remove(>existing, >hmap_node);
-free(desired->name);
-free(desired);
-}
-hmap_destroy(>desired);
-
-struct ovn_extend_table_info *existing, *e_next;
-HMAP_FOR_EACH_SAFE (existing, e_next, hmap_node, >existing) {
-hmap_remove(>existing, >hmap_node);
-free(existing->name);
-free(existing);
-}
-hmap_destroy(>existing);
+ovn_extend_table_info_destroy(>desired);
+ovn_extend_table_info_destroy(>existing);
 }
 
 /* Finds and returns a group_info in 'existing' whose key is identical
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v2] extend-table: Fix a bug that iterates wrong table

2018-10-02 Thread Yifeng Sun 
This seems to be a copy and paste bug that iterates and frees
the wrong table. This commit fixes that.

Reported-at: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=10730
Signed-off-by: Yifeng Sun 
---
v1->v2: Fix email subject by adding [ovs-dev]

 ovn/lib/extend-table.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ovn/lib/extend-table.c b/ovn/lib/extend-table.c
index 511d1a84b0c0..aba0b2e68b2c 100644
--- a/ovn/lib/extend-table.c
+++ b/ovn/lib/extend-table.c
@@ -39,8 +39,8 @@ ovn_extend_table_destroy(struct ovn_extend_table *table)
 bitmap_free(table->table_ids);
 
 struct ovn_extend_table_info *desired, *d_next;
-HMAP_FOR_EACH_SAFE (desired, d_next, hmap_node, >existing) {
-hmap_remove(>existing, >hmap_node);
+HMAP_FOR_EACH_SAFE (desired, d_next, hmap_node, >desired) {
+hmap_remove(>desired, >hmap_node);
 free(desired->name);
 free(desired);
 }
-- 
2.7.4

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev