Send connman mailing list submissions to
[email protected]
To subscribe or unsubscribe via the World Wide Web, visit
https://lists.01.org/mailman/listinfo/connman
or, via email, send a message with subject or body 'help' to
[email protected]
You can reach the person managing the list at
[email protected]
When replying, please edit your Subject line so it is more specific
than "Re: Contents of connman digest..."
Today's Topics:
1. [PATCH 1/1] service: Remove unnecessary NULL check
(Ravi Prasad RK)
2. [PATCH v2] nat: Remember previous IPv4 forwarding value
(Patrik Flykt)
3. Re: [PATCH 1/1] service: Fix memory leak in
__connman_service_nameserver_append (Patrik Flykt)
4. Re: [PATCH 1/1] service: Remove unnecessary NULL check
(Patrik Flykt)
5. Re: [PATCH] firewall: Remove old rules (Patrik Flykt)
6. Re: [PATCH] firewall: Remove old rules (Jose Blanquicet)
7. Re: [PATCH] firewall: Remove old rules (Patrik Flykt)
8. [PATCH] firewall: Remove old rules (Jose Blanquicet)
----------------------------------------------------------------------
Message: 1
Date: Tue, 12 Apr 2016 08:59:55 +0530
From: Ravi Prasad RK <[email protected]>
To: [email protected]
Cc: Ravi Prasad RK <[email protected]>
Subject: [PATCH 1/1] service: Remove unnecessary NULL check
Message-ID: <[email protected]>
g_strdup() returns NULL only if 'nameserver' is NULL.
'nameserver' is checked for NULL at begining of function before
passing to g_strdup() function.
---
src/service.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/src/service.c b/src/service.c
index d9abbc4..1fff483 100644
--- a/src/service.c
+++ b/src/service.c
@@ -1127,11 +1127,6 @@ int __connman_service_nameserver_append(struct
connman_service *service,
return -ENOMEM;
nameservers[len] = g_strdup(nameserver);
- if (!nameservers[len]) {
- g_strfreev(nameservers);
- return -ENOMEM;
- }
-
nameservers[len + 1] = NULL;
if (is_auto) {
--
1.7.9.5
------------------------------
Message: 2
Date: Tue, 12 Apr 2016 16:01:51 +0300
From: Patrik Flykt <[email protected]>
To: [email protected]
Subject: [PATCH v2] nat: Remember previous IPv4 forwarding value
Message-ID:
<[email protected]>
When NAT is enabled, store the previous IPv4 forwarding setting so that
it can be restored to its former value when disabling NAT.
---
v2: Fix err = -errno and fix a few conditionals to be more readable
src/nat.c | 40 ++++++++++++++++++++++++++++++----------
1 file changed, 30 insertions(+), 10 deletions(-)
diff --git a/src/nat.c b/src/nat.c
index 063f085..b739e11 100644
--- a/src/nat.c
+++ b/src/nat.c
@@ -25,7 +25,10 @@
#endif
#include <errno.h>
-#include <stdio.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <unistd.h>
#include "connman.h"
@@ -42,20 +45,37 @@ struct connman_nat {
static int enable_ip_forward(bool enable)
{
- FILE *f;
+ static char value = 0;
+ int f, err = 0;
- f = fopen("/proc/sys/net/ipv4/ip_forward", "r+");
- if (!f)
+ if ((f = open("/proc/sys/net/ipv4/ip_forward", O_CLOEXEC)) < 0)
return -errno;
- if (enable)
- fprintf(f, "1");
- else
- fprintf(f, "0");
+ if (!value) {
+ if (read(f, &value, sizeof(value)) < 0)
+ value = 0;
+ }
- fclose(f);
+ if (enable) {
+ char allow = '1';
- return 0;
+ if (write (f, &allow, sizeof(allow)) < 0)
+ err = -errno;
+ } else {
+ char deny = '0';
+
+ if (value)
+ deny = value;
+
+ if (write(f, &deny, sizeof(deny)) < 0)
+ err = -errno;
+
+ value = 0;
+ }
+
+ close(f);
+
+ return err;
}
static int enable_nat(struct connman_nat *nat)
--
2.8.0.rc3
------------------------------
Message: 3
Date: Tue, 12 Apr 2016 16:33:34 +0300
From: Patrik Flykt <[email protected]>
To: Ravi Prasad RK <[email protected]>, [email protected]
Subject: Re: [PATCH 1/1] service: Fix memory leak in
__connman_service_nameserver_append
Message-ID: <[email protected]>
Content-Type: text/plain; charset="UTF-8"
Hi,
On Mon, 2016-04-11 at 18:03 +0530, Ravi Prasad RK wrote:
> ---
> ?src/service.c |????4 +++-
> ?1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/src/service.c b/src/service.c
> index 8e07337..d9abbc4 100644
> --- a/src/service.c
> +++ b/src/service.c
> @@ -1127,8 +1127,10 @@ int __connman_service_nameserver_append(struct
> connman_service *service,
> ? return -ENOMEM;
> ?
> ? nameservers[len] = g_strdup(nameserver);
> - if (!nameservers[len])
> + if (!nameservers[len]) {
> + g_strfreev(nameservers);
> ? return -ENOMEM;
> + }
> ?
> ? nameservers[len + 1] = NULL;
> ?
If g_strdup() fails to duplicate the string, it will terminate the
program. So this check is useless.
Cheers,
Patrik
------------------------------
Message: 4
Date: Tue, 12 Apr 2016 16:37:02 +0300
From: Patrik Flykt <[email protected]>
To: Ravi Prasad RK <[email protected]>, [email protected]
Subject: Re: [PATCH 1/1] service: Remove unnecessary NULL check
Message-ID: <[email protected]>
Content-Type: text/plain; charset="UTF-8"
On Tue, 2016-04-12 at 08:59 +0530, Ravi Prasad RK wrote:
> g_strdup() returns NULL only if 'nameserver' is NULL.
> 'nameserver' is checked for NULL at begining of function before
> passing to g_strdup() function.
> ---
> ?src/service.c |????5 -----
> ?1 file changed, 5 deletions(-)
>
> diff --git a/src/service.c b/src/service.c
> index d9abbc4..1fff483 100644
> --- a/src/service.c
> +++ b/src/service.c
> @@ -1127,11 +1127,6 @@ int __connman_service_nameserver_append(struct
> connman_service *service,
> ? return -ENOMEM;
> ?
> ? nameservers[len] = g_strdup(nameserver);
> - if (!nameservers[len]) {
> - g_strfreev(nameservers);
> - return -ENOMEM;
> - }
> -
> ? nameservers[len + 1] = NULL;
> ?
> ? if (is_auto) {
Yes, except that this patch does not apply to upstream as it contains a
previous modification.
Patrik
------------------------------
Message: 5
Date: Tue, 12 Apr 2016 16:44:00 +0300
From: Patrik Flykt <[email protected]>
To: Jose Blanquicet <[email protected]>, [email protected]
Subject: Re: [PATCH] firewall: Remove old rules
Message-ID: <[email protected]>
Content-Type: text/plain; charset="UTF-8"
Hi,
On Mon, 2016-04-11 at 15:24 +0200, Jose Blanquicet wrote:
> ---
Please add the cover letter text from 'firewall: Remove old rules' as a
commit message.
> ?src/firewall.c | 8 +++++++-
> ?1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/src/firewall.c b/src/firewall.c
> index c5acc11..0baba6b 100644
> --- a/src/firewall.c
> +++ b/src/firewall.c
> @@ -438,7 +438,13 @@ int __connman_firewall_enable(struct
> firewall_context *ctx)
> ?
> ?int __connman_firewall_disable(struct firewall_context *ctx)
> ?{
> - return __connman_firewall_disable_rule(ctx, FW_ALL_RULES);
> + int err;
> +
> + err = __connman_firewall_disable_rule(ctx, FW_ALL_RULES);
> + if (err < 0)
> + return err;
> +
> + return __connman_firewall_remove_rule(ctx, FW_ALL_RULES);
> ?}
> ?
> ?bool __connman_firewall_is_up(void)
Does it make sense stubbornly calling __connman_firewall_remove_rule()
anyway instead of bailing out if an error is returned
from?__connman_firewall_disable_rule() ?
Cheers,
Patrik
------------------------------
Message: 6
Date: Tue, 12 Apr 2016 16:37:18 +0200
From: Jose Blanquicet <[email protected]>
To: Patrik Flykt <[email protected]>
Cc: [email protected]
Subject: Re: [PATCH] firewall: Remove old rules
Message-ID:
<cafc8ijjdjdqvqxkwcvf8nw6wc4xfxme7zymzxhh_g-1bg+h...@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Hello,
On Tue, Apr 12, 2016 at 3:44 PM, Patrik Flykt
<[email protected]> wrote:
>
> Hi,
>
> On Mon, 2016-04-11 at 15:24 +0200, Jose Blanquicet wrote:
>> ---
>
> Please add the cover letter text from 'firewall: Remove old rules' as a
> commit message.
>
Should I create next time just one email with the description of the
path as commit message together with the patch itself?
>> src/firewall.c | 8 +++++++-
>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/firewall.c b/src/firewall.c
>> index c5acc11..0baba6b 100644
>> --- a/src/firewall.c
>> +++ b/src/firewall.c
>> @@ -438,7 +438,13 @@ int __connman_firewall_enable(struct
>> firewall_context *ctx)
>>
>> int __connman_firewall_disable(struct firewall_context *ctx)
>> {
>> - return __connman_firewall_disable_rule(ctx, FW_ALL_RULES);
>> + int err;
>> +
>> + err = __connman_firewall_disable_rule(ctx, FW_ALL_RULES);
>> + if (err < 0)
>> + return err;
>> +
>> + return __connman_firewall_remove_rule(ctx, FW_ALL_RULES);
>> }
>>
>> bool __connman_firewall_is_up(void)
>
> Does it make sense stubbornly calling __connman_firewall_remove_rule()
> anyway instead of bailing out if an error is returned
> from __connman_firewall_disable_rule() ?
>
Yes, it makes completely sense because in any case the returned error
is not handled and by stubbornly calling the remove function we are
totally ensuring that any rule will re-installed. Do I send a new
patch removing the error check?
> Cheers,
>
> Patrik
>
Best regards,
Jose Blanquicet
------------------------------
Message: 7
Date: Tue, 12 Apr 2016 18:07:13 +0300
From: Patrik Flykt <[email protected]>
To: Jose Blanquicet <[email protected]>
Cc: [email protected]
Subject: Re: [PATCH] firewall: Remove old rules
Message-ID: <[email protected]>
Content-Type: text/plain; charset="UTF-8"
Hi,
On Tue, 2016-04-12 at 16:37 +0200, Jose Blanquicet wrote:
> Hello,
>
>
> On Tue, Apr 12, 2016 at 3:44 PM, Patrik Flykt
> <[email protected]> wrote:
> >
> >
> > ????????Hi,
> >
> > On Mon, 2016-04-11 at 15:24 +0200, Jose Blanquicet wrote:
> > >
> > > ---
> > Please add the cover letter text from 'firewall: Remove old rules'
> > as a
> > commit message.
> >
> Should I create next time just one email with the description of the
> path as commit message together with the patch itself?
Yes, please. With only one patch the explanation should fit into its
commit messages. Of course there are exceptions to every rule, but
they're rare.
>
> >
> > >
> > > ?src/firewall.c | 8 +++++++-
> > > ?1 file changed, 7 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/firewall.c b/src/firewall.c
> > > index c5acc11..0baba6b 100644
> > > --- a/src/firewall.c
> > > +++ b/src/firewall.c
> > > @@ -438,7 +438,13 @@ int __connman_firewall_enable(struct
> > > firewall_context *ctx)
> > >
> > > ?int __connman_firewall_disable(struct firewall_context *ctx)
> > > ?{
> > > -?????return __connman_firewall_disable_rule(ctx, FW_ALL_RULES);
> > > +?????int err;
> > > +
> > > +?????err = __connman_firewall_disable_rule(ctx, FW_ALL_RULES);
> > > +?????if (err < 0)
> > > +?????????????return err;
> > > +
> > > +?????return __connman_firewall_remove_rule(ctx, FW_ALL_RULES);
> > > ?}
> > >
> > > ?bool __connman_firewall_is_up(void)
> > Does it make sense stubbornly calling
> > __connman_firewall_remove_rule()
> > anyway instead of bailing out if an error is returned
> > from __connman_firewall_disable_rule() ?
> >
> Yes, it makes completely sense because in any case the returned error
> is not handled and by stubbornly calling the remove function we are
> totally ensuring that any rule will re-installed. Do I send a new
> patch removing the error check?
That would be fine, please do.
Cheers,
Patrik
------------------------------
Message: 8
Date: Tue, 12 Apr 2016 17:18:08 +0200
From: Jose Blanquicet <[email protected]>
To: [email protected]
Subject: [PATCH] firewall: Remove old rules
Message-ID: <[email protected]>
When a new service becomes ready or online, it is checked if its technology is
more preferred than the one used by the currrent default gateway. If so, this
new service becomes the new default gateway.
When the tethering is enabled, a NAT rule is created to forward the traffic
between the interface playing the AP role and the interface connected to
current default gateway. The problem comes out when the default gateway
changes, because the NAT rule is disabled but not removed from the firewall
rules list. Therefore, when the new rule is installed also the old rule is
installed because it is still in the list. If it changes again, then three
rules will be installed, and so on. They are never removed.
This patch adds a deletion of all the rules from the firewall list exactly
after they are disabled to avoid the described problem.
---
src/firewall.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/firewall.c b/src/firewall.c
index c5acc11..c440df6 100644
--- a/src/firewall.c
+++ b/src/firewall.c
@@ -438,7 +438,8 @@ int __connman_firewall_enable(struct firewall_context *ctx)
int __connman_firewall_disable(struct firewall_context *ctx)
{
- return __connman_firewall_disable_rule(ctx, FW_ALL_RULES);
+ __connman_firewall_disable_rule(ctx, FW_ALL_RULES);
+ return __connman_firewall_remove_rule(ctx, FW_ALL_RULES);
}
bool __connman_firewall_is_up(void)
--
1.9.1
------------------------------
Subject: Digest Footer
_______________________________________________
connman mailing list
[email protected]
https://lists.01.org/mailman/listinfo/connman
------------------------------
End of connman Digest, Vol 6, Issue 7
*************************************
