[PATCH] Optimise nfq_queue_cb

[PATCH] Optimise nfq_queue_cb

[Paul Aitken]

ct and myct have both already been checked for non-NULL,
so there's no need to check either of them again later.

Signed-off-by: Paul Aitken <paitken@brocade.com>
---
  src/cthelper.c | 8 +++-----
  1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/src/cthelper.c b/src/cthelper.c
index 15d5126..6537515 100644
--- a/src/cthelper.c
+++ b/src/cthelper.c
@@ -325,14 +325,12 @@ static int nfq_queue_cb(const struct nlmsghdr *nlh, void *data)
  	if (pkt_verdict_issue(helper, myct, queue_num, id, verdict, pktb) < 0)
  		goto err_pktb;
  
-	if (ct != NULL)
-		nfct_destroy(ct);
+	nfct_destroy(ct);
  	if (myct->exp != NULL)
  		nfexp_destroy(myct->exp);
-	if (myct && myct->priv_data != NULL)
+	if (myct->priv_data != NULL)
  		free(myct->priv_data);
-	if (myct != NULL)
-		free(myct);
+	free(myct);
  
  	return MNL_CB_OK;
  err_pktb:
-- 
1.9.1

Re: [PATCH] Optimise nfq_queue_cb

[Pablo Neira Ayuso]

On Mon, Jun 01, 2015 at 10:22:00AM +0100, Paul Aitken wrote:
> ct and myct have both already been checked for non-NULL,
> so there's no need to check either of them again later.
> 
> Signed-off-by: Paul Aitken <paitken@brocade.com>
> ---
>  src/cthelper.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/src/cthelper.c b/src/cthelper.c
> index 15d5126..6537515 100644
> --- a/src/cthelper.c
> +++ b/src/cthelper.c
> @@ -325,14 +325,12 @@ static int nfq_queue_cb(const struct nlmsghdr *nlh, void *data)
>  	if (pkt_verdict_issue(helper, myct, queue_num, id, verdict, pktb) < 0)
>  		goto err_pktb;
> -	if (ct != NULL)
> -		nfct_destroy(ct);
> +	nfct_destroy(ct);

void nfct_destroy(struct nf_conntrack *ct)
{
        assert(ct != NULL);
        ...

the library doesn't allow NULL pointers.

>  	if (myct->exp != NULL)
>  		nfexp_destroy(myct->exp);
> -	if (myct && myct->priv_data != NULL)
> +	if (myct->priv_data != NULL)
>  		free(myct->priv_data);
> -	if (myct != NULL)
> -		free(myct);
> +	free(myct);
>  	return MNL_CB_OK;
>  err_pktb:
> -- 
> 1.9.1
> 

Re: [PATCH] Optimise nfq_queue_cb

[Paul Aitken]

Pablo,

> On Mon, Jun 01, 2015 at 10:22:00AM +0100, Paul Aitken wrote:
>> ct and myct have both already been checked for non-NULL,
>> so there's no need to check either of them again later.
>>
>> Signed-off-by: Paul Aitken <paitken@brocade.com>
>> ---
>>   src/cthelper.c | 8 +++-----
>>   1 file changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/cthelper.c b/src/cthelper.c
>> index 15d5126..6537515 100644
>> --- a/src/cthelper.c
>> +++ b/src/cthelper.c
>> @@ -325,14 +325,12 @@ static int nfq_queue_cb(const struct nlmsghdr *nlh, void *data)
>>   	if (pkt_verdict_issue(helper, myct, queue_num, id, verdict, pktb) < 0)
>>   		goto err_pktb;
>> -	if (ct != NULL)
>> -		nfct_destroy(ct);
>> +	nfct_destroy(ct);
> void nfct_destroy(struct nf_conntrack *ct)
> {
>          assert(ct != NULL);
>          ...
>
> the library doesn't allow NULL pointers.

ct was already checked for non-NULL when it was assigned around line 297:

         ct = nfct_new();
         if (ct == NULL)
                 goto err;

- so ct cannot be NULL at line 325.


P.

>>   	if (myct->exp != NULL)
>>   		nfexp_destroy(myct->exp);
>> -	if (myct && myct->priv_data != NULL)
>> +	if (myct->priv_data != NULL)
>>   		free(myct->priv_data);
>> -	if (myct != NULL)
>> -		free(myct);
>> +	free(myct);
>>   	return MNL_CB_OK;
>>   err_pktb:
>> -- 
>> 1.9.1
>>

Re: [PATCH] Optimise nfq_queue_cb

[Pablo Neira Ayuso]

On Mon, Jun 01, 2015 at 12:19:20PM +0100, Paul Aitken wrote:
> Pablo,
> 
> >On Mon, Jun 01, 2015 at 10:22:00AM +0100, Paul Aitken wrote:
> >>ct and myct have both already been checked for non-NULL,
> >>so there's no need to check either of them again later.
> >>
> >>Signed-off-by: Paul Aitken <paitken@brocade.com>
> >>---
> >>  src/cthelper.c | 8 +++-----
> >>  1 file changed, 3 insertions(+), 5 deletions(-)
> >>
> >>diff --git a/src/cthelper.c b/src/cthelper.c
> >>index 15d5126..6537515 100644
> >>--- a/src/cthelper.c
> >>+++ b/src/cthelper.c
> >>@@ -325,14 +325,12 @@ static int nfq_queue_cb(const struct nlmsghdr *nlh, void *data)
> >>  	if (pkt_verdict_issue(helper, myct, queue_num, id, verdict, pktb) < 0)
> >>  		goto err_pktb;
> >>-	if (ct != NULL)
> >>-		nfct_destroy(ct);
> >>+	nfct_destroy(ct);
> >void nfct_destroy(struct nf_conntrack *ct)
> >{
> >         assert(ct != NULL);
> >         ...
> >
> >the library doesn't allow NULL pointers.
> 
> ct was already checked for non-NULL when it was assigned around line 297:
> 
>         ct = nfct_new();
>         if (ct == NULL)
>                 goto err;
> 
> - so ct cannot be NULL at line 325.

Right, makes sense. However, your patch doesn't apply here for some
reason:

$ git am Optimise-nfq_queue_cb.patch
Applying: Optimise nfq_queue_cb
error: patch failed: src/cthelper.c:325
error: src/cthelper.c: patch does not apply
Patch failed at 0001 Optimise nfq_queue_cb
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort"

Re: [PATCH] Optimise nfq_queue_cb

[Paul Aitken]

Pablo, somehow tabs had been converted to spaces.

Let me try again...

P.


On 01/06/15 18:34, Pablo Neira Ayuso wrote:
> On Mon, Jun 01, 2015 at 12:19:20PM +0100, Paul Aitken wrote:
>> Pablo,
>>
>>> On Mon, Jun 01, 2015 at 10:22:00AM +0100, Paul Aitken wrote:
>>>> ct and myct have both already been checked for non-NULL,
>>>> so there's no need to check either of them again later.
>>>>
>>>> Signed-off-by: Paul Aitken <paitken@brocade.com>
>>>> ---
>>>>   src/cthelper.c | 8 +++-----
>>>>   1 file changed, 3 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/src/cthelper.c b/src/cthelper.c
>>>> index 15d5126..6537515 100644
>>>> --- a/src/cthelper.c
>>>> +++ b/src/cthelper.c
>>>> @@ -325,14 +325,12 @@ static int nfq_queue_cb(const struct nlmsghdr *nlh, void *data)
>>>>   	if (pkt_verdict_issue(helper, myct, queue_num, id, verdict, pktb) < 0)
>>>>   		goto err_pktb;
>>>> -	if (ct != NULL)
>>>> -		nfct_destroy(ct);
>>>> +	nfct_destroy(ct);
>>> void nfct_destroy(struct nf_conntrack *ct)
>>> {
>>>          assert(ct != NULL);
>>>          ...
>>>
>>> the library doesn't allow NULL pointers.
>> ct was already checked for non-NULL when it was assigned around line 297:
>>
>>          ct = nfct_new();
>>          if (ct == NULL)
>>                  goto err;
>>
>> - so ct cannot be NULL at line 325.
> Right, makes sense. However, your patch doesn't apply here for some
> reason:
>
> $ git am Optimise-nfq_queue_cb.patch
> Applying: Optimise nfq_queue_cb
> error: patch failed: src/cthelper.c:325
> error: src/cthelper.c: patch does not apply
> Patch failed at 0001 Optimise nfq_queue_cb
> When you have resolved this problem run "git am --resolved".
> If you would prefer to skip this patch, instead run "git am --skip".
> To restore the original branch and stop patching run "git am --abort"

[PATCH] Optimise nfq_queue_cb

[Paul Aitken]

ct and myct have both already been checked for non-NULL,
so there's no need to check either of them again later.

Signed-off-by: Paul Aitken <paitken@brocade.com>
---
 src/cthelper.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/src/cthelper.c b/src/cthelper.c
index 15d5126..6537515 100644
--- a/src/cthelper.c
+++ b/src/cthelper.c
@@ -325,14 +325,12 @@ static int nfq_queue_cb(const struct nlmsghdr *nlh, void *data)
 	if (pkt_verdict_issue(helper, myct, queue_num, id, verdict, pktb) < 0)
 		goto err_pktb;
 
-	if (ct != NULL)
-		nfct_destroy(ct);
+	nfct_destroy(ct);
 	if (myct->exp != NULL)
 		nfexp_destroy(myct->exp);
-	if (myct && myct->priv_data != NULL)
+	if (myct->priv_data != NULL)
 		free(myct->priv_data);
-	if (myct != NULL)
-		free(myct);
+	free(myct);
 
 	return MNL_CB_OK;
 err_pktb:
-- 
1.9.1

Re: [PATCH] Optimise nfq_queue_cb

[Pablo Neira Ayuso]

On Thu, Jun 04, 2015 at 10:15:00AM +0100, Paul Aitken wrote:
> ct and myct have both already been checked for non-NULL,
> so there's no need to check either of them again later.

Applied, thanks.