[ipset PATCH 0/2] Two minor code fixes

[ipset PATCH 0/2] Two minor code fixes

[Phil Sutter]

These were identified by Coverity tool, no problems in practice. Still
worth fixing to reduce noise in code checkers.

Phil Sutter (2):
  xlate: Fix for fd leak in error path
  xlate: Drop dead code

 lib/ipset.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

-- 
2.38.0

[ipset PATCH 1/2] xlate: Fix for fd leak in error path

[Phil Sutter]

A rather cosmetic issue though, the program will terminate anyway.

Fixes: 325af556cd3a6 ("add ipset to nftables translation infrastructure")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 lib/ipset.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/ipset.c b/lib/ipset.c
index f57b07413cba5..2e098e435d954 100644
--- a/lib/ipset.c
+++ b/lib/ipset.c
@@ -1999,7 +1999,7 @@ static int ipset_xlate_restore(struct ipset *ipset)
 
 		ret = build_argv(ipset, c);
 		if (ret < 0)
-			return ret;
+			break;
 
 		cmd = ipset_parser(ipset, ipset->newargc, ipset->newargv);
 		if (cmd < 0)
-- 
2.38.0

[ipset PATCH 2/2] xlate: Drop dead code

[Phil Sutter]

Set type is not needed when manipulating elements, the assigned
variable was unused in that case.

Fixes: 325af556cd3a6 ("add ipset to nftables translation infrastructure")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 lib/ipset.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/lib/ipset.c b/lib/ipset.c
index 2e098e435d954..8e63af56d2a51 100644
--- a/lib/ipset.c
+++ b/lib/ipset.c
@@ -1876,9 +1876,6 @@ static int ipset_xlate(struct ipset *ipset, enum ipset_cmd cmd,
 				cmd == IPSET_CMD_DEL ? "delete" : "get",
 		       ipset_xlate_family(family), table, set);
 
-		typename = ipset_data_get(data, IPSET_OPT_TYPENAME);
-		type = ipset_xlate_set_type(typename);
-
 		xlate_set = (struct ipset_xlate_set *)
 				ipset_xlate_set_get(ipset, set);
 		if (xlate_set && xlate_set->interval)
-- 
2.38.0

Re: [ipset PATCH 0/2] Two minor code fixes

[Pablo Neira Ayuso]

On Wed, Feb 22, 2023 at 06:02:39PM +0100, Phil Sutter wrote:
> These were identified by Coverity tool, no problems in practice. Still
> worth fixing to reduce noise in code checkers.

LGTM.

Did you run ipset xlate tests? These should not break those but just
in case.

Re: [ipset PATCH 0/2] Two minor code fixes

[Phil Sutter]

On Wed, Mar 01, 2023 at 11:28:36PM +0100, Pablo Neira Ayuso wrote:
> On Wed, Feb 22, 2023 at 06:02:39PM +0100, Phil Sutter wrote:
> > These were identified by Coverity tool, no problems in practice. Still
> > worth fixing to reduce noise in code checkers.
> 
> LGTM.
> 
> Did you run ipset xlate tests? These should not break those but just
> in case.

I didn't, thanks for the reminder. Testsuite fails, but it does with
HEAD as well. And so does the other testsuite ("make tests"), BtW. I'll
investigate.

Cheers, Phil

Re: [ipset PATCH 0/2] Two minor code fixes

[Pablo Neira Ayuso]

On Thu, Mar 02, 2023 at 10:04:14AM +0100, Phil Sutter wrote:
> On Wed, Mar 01, 2023 at 11:28:36PM +0100, Pablo Neira Ayuso wrote:
> > On Wed, Feb 22, 2023 at 06:02:39PM +0100, Phil Sutter wrote:
> > > These were identified by Coverity tool, no problems in practice. Still
> > > worth fixing to reduce noise in code checkers.
> > 
> > LGTM.
> > 
> > Did you run ipset xlate tests? These should not break those but just
> > in case.
> 
> I didn't, thanks for the reminder. Testsuite fails, but it does with
> HEAD as well. And so does the other testsuite ("make tests"), BtW. I'll
> investigate.

Does this work after your testsuite updates? If so, push them out.

Re: [ipset PATCH 0/2] Two minor code fixes

[Phil Sutter]

On Fri, Mar 10, 2023 at 12:56:29PM +0100, Pablo Neira Ayuso wrote:
> On Thu, Mar 02, 2023 at 10:04:14AM +0100, Phil Sutter wrote:
> > On Wed, Mar 01, 2023 at 11:28:36PM +0100, Pablo Neira Ayuso wrote:
> > > On Wed, Feb 22, 2023 at 06:02:39PM +0100, Phil Sutter wrote:
> > > > These were identified by Coverity tool, no problems in practice. Still
> > > > worth fixing to reduce noise in code checkers.
> > > 
> > > LGTM.
> > > 
> > > Did you run ipset xlate tests? These should not break those but just
> > > in case.
> > 
> > I didn't, thanks for the reminder. Testsuite fails, but it does with
> > HEAD as well. And so does the other testsuite ("make tests"), BtW. I'll
> > investigate.
> 
> Does this work after your testsuite updates? If so, push them out.

Yes, it does. Should I push the testsuite updates, too? I'm uncertain
about the s/vrrp/carp/, don't want to break anyone's test setup.

Cheers, Phil

Re: [ipset PATCH 0/2] Two minor code fixes

[Phil Sutter]

On Wed, Feb 22, 2023 at 06:02:39PM +0100, Phil Sutter wrote:
> These were identified by Coverity tool, no problems in practice. Still
> worth fixing to reduce noise in code checkers.
> 
> Phil Sutter (2):
>   xlate: Fix for fd leak in error path
>   xlate: Drop dead code

Series applied.

Re: [ipset PATCH 0/2] Two minor code fixes

[Pablo Neira Ayuso]

On Fri, Mar 10, 2023 at 01:05:23PM +0100, Phil Sutter wrote:
> On Fri, Mar 10, 2023 at 12:56:29PM +0100, Pablo Neira Ayuso wrote:
> > On Thu, Mar 02, 2023 at 10:04:14AM +0100, Phil Sutter wrote:
> > > On Wed, Mar 01, 2023 at 11:28:36PM +0100, Pablo Neira Ayuso wrote:
> > > > On Wed, Feb 22, 2023 at 06:02:39PM +0100, Phil Sutter wrote:
> > > > > These were identified by Coverity tool, no problems in practice. Still
> > > > > worth fixing to reduce noise in code checkers.
> > > > 
> > > > LGTM.
> > > > 
> > > > Did you run ipset xlate tests? These should not break those but just
> > > > in case.
> > > 
> > > I didn't, thanks for the reminder. Testsuite fails, but it does with
> > > HEAD as well. And so does the other testsuite ("make tests"), BtW. I'll
> > > investigate.
> > 
> > Does this work after your testsuite updates? If so, push them out.
> 
> Yes, it does. Should I push the testsuite updates, too? I'm uncertain
> about the s/vrrp/carp/, don't want to break anyone's test setup.

I can see some distros still use vrrp en /etc/protocols, yes, I'm
ambivalent on this one.

Re: [ipset PATCH 0/2] Two minor code fixes

[Phil Sutter]

On Fri, Mar 10, 2023 at 01:22:16PM +0100, Pablo Neira Ayuso wrote:
> On Fri, Mar 10, 2023 at 01:05:23PM +0100, Phil Sutter wrote:
> > On Fri, Mar 10, 2023 at 12:56:29PM +0100, Pablo Neira Ayuso wrote:
> > > On Thu, Mar 02, 2023 at 10:04:14AM +0100, Phil Sutter wrote:
> > > > On Wed, Mar 01, 2023 at 11:28:36PM +0100, Pablo Neira Ayuso wrote:
> > > > > On Wed, Feb 22, 2023 at 06:02:39PM +0100, Phil Sutter wrote:
> > > > > > These were identified by Coverity tool, no problems in practice. Still
> > > > > > worth fixing to reduce noise in code checkers.
> > > > > 
> > > > > LGTM.
> > > > > 
> > > > > Did you run ipset xlate tests? These should not break those but just
> > > > > in case.
> > > > 
> > > > I didn't, thanks for the reminder. Testsuite fails, but it does with
> > > > HEAD as well. And so does the other testsuite ("make tests"), BtW. I'll
> > > > investigate.
> > > 
> > > Does this work after your testsuite updates? If so, push them out.
> > 
> > Yes, it does. Should I push the testsuite updates, too? I'm uncertain
> > about the s/vrrp/carp/, don't want to break anyone's test setup.
> 
> I can see some distros still use vrrp en /etc/protocols, yes, I'm
> ambivalent on this one.

Maybe better just use a different protocol which didn't get "renamed"
recently? It's for testing purposes only and the actual value doesn't
matter much, right?

Re: [ipset PATCH 0/2] Two minor code fixes

[Pablo Neira Ayuso]

On Fri, Mar 10, 2023 at 01:37:17PM +0100, Phil Sutter wrote:
> On Fri, Mar 10, 2023 at 01:22:16PM +0100, Pablo Neira Ayuso wrote:
> > On Fri, Mar 10, 2023 at 01:05:23PM +0100, Phil Sutter wrote:
> > > On Fri, Mar 10, 2023 at 12:56:29PM +0100, Pablo Neira Ayuso wrote:
> > > > On Thu, Mar 02, 2023 at 10:04:14AM +0100, Phil Sutter wrote:
> > > > > On Wed, Mar 01, 2023 at 11:28:36PM +0100, Pablo Neira Ayuso wrote:
> > > > > > On Wed, Feb 22, 2023 at 06:02:39PM +0100, Phil Sutter wrote:
> > > > > > > These were identified by Coverity tool, no problems in practice. Still
> > > > > > > worth fixing to reduce noise in code checkers.
> > > > > > 
> > > > > > LGTM.
> > > > > > 
> > > > > > Did you run ipset xlate tests? These should not break those but just
> > > > > > in case.
> > > > > 
> > > > > I didn't, thanks for the reminder. Testsuite fails, but it does with
> > > > > HEAD as well. And so does the other testsuite ("make tests"), BtW. I'll
> > > > > investigate.
> > > > 
> > > > Does this work after your testsuite updates? If so, push them out.
> > > 
> > > Yes, it does. Should I push the testsuite updates, too? I'm uncertain
> > > about the s/vrrp/carp/, don't want to break anyone's test setup.
> > 
> > I can see some distros still use vrrp en /etc/protocols, yes, I'm
> > ambivalent on this one.
> 
> Maybe better just use a different protocol which didn't get "renamed"
> recently? It's for testing purposes only and the actual value doesn't
> matter much, right?

I agree that's fine for a test, yes.