Send connman mailing list submissions to
[email protected]
To subscribe or unsubscribe 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. Re: [PATCH] IP Accounting for WiFi Clients (Daniel Wagner)
2. Re: ipv6 test web page down (Daniel Wagner)
3. Re: Wifi AP with a captive portal (Daniel Wagner)
4. Re: [PATCH] IP Accounting for WiFi Clients (Aravind Gunasekaran)
----------------------------------------------------------------------
Date: Wed, 17 Jun 2020 09:18:05 +0200
From: Daniel Wagner <[email protected]>
Subject: Re: [PATCH] IP Accounting for WiFi Clients
To: Aravind Gunasekaran <[email protected]>
Cc: [email protected]
Message-ID: <[email protected]>
Content-Type: text/plain; charset=us-ascii
Hi Aravind,
Please add a commit message explaining why are doing it and what you are
changing.
I noticed this is for iptables only. The iptables code
is a bit dangorous to work with. There is no user land library to use.
Instead we have to do all the work to parse the message from the kernel.
I am pretty sure there are a lot of hidden bugs in this code and it
currently it just happens to work. It would be far better if
you used the nftables code base. For this we have a proper library and
the code is far cleaner and more robust.
Next thing, please split the patch up into different smaller units.
At least the hocking up of the new interfaces should go into a new
patch.
On Wed, May 27, 2020 at 03:33:20PM +0530, Aravind Gunasekaran wrote:
> diff --git a/Makefile.am b/Makefile.am
> old mode 100644
> new mode 100755
> index 5971ca9..44148df
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -12,7 +12,7 @@ include_HEADERS = include/log.h include/plugin.h \
> include/storage.h include/provision.h \
> include/session.h include/ipaddress.h include/agent.h \
> include/inotify.h include/peer.h include/machine.h \
> - include/acd.h include/tethering.h
> + include/acd.h include/tethering.h include/accounting-iptables.h
The header file in the include directory are for the plugins. Unless the
function you define are used in plugins don't add them here. Instead
add them to src/connman.h.
> nodist_include_HEADERS = include/version.h
>
> @@ -152,7 +152,7 @@ src_connmand_wait_online_LDADD = gdbus/
> libgdbus-internal.la \
> @GLIB_LIBS@ @DBUS_LIBS@
>
> if XTABLES
> -src_connmand_SOURCES += src/iptables.c src/firewall-iptables.c
> +src_connmand_SOURCES += src/iptables.c src/firewall-iptables.c
> src/accounting-iptables.c
The line should be more indented.
> src_connmand_LDADD += @XTABLES_LIBS@
> endif
>
> diff --git a/include/accounting-iptables.h b/include/accounting-iptables.h
> new file mode 100755
> index 0000000..3a9cc30
> --- /dev/null
> +++ b/include/accounting-iptables.h
> @@ -0,0 +1,49 @@
> +/*
> + *
> + * Connection Manager
> + *
> + * Copyright (C) 2007-2013 Intel Corporation. All rights reserved.
This copyright statment is wrong.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301
> USA
> + *
> + */
> +
> +#ifndef __CONNMAN_ACCOUNTING_IPTABLES_H
> +#define __CONNMAN_ACCOUNTING_IPTABLES_H
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +struct counters {
> + __u64 pcnt, bcnt; /* Packet and byte counters */
> +};
> +
> +struct ip_accounting_info {
> + char *ipv4_address; /* IP address of the client */
> + struct counters egress; /* Transmitted bytes information of the client */
> + struct counters ingress; /* Received bytes information of the client */
> +};
Use tabs, see our codying style document. You can also use the checkpatch.pl
script from the Linux kernel to check for style issues.
> +
> +int __connman_get_ip_acc_info(struct ip_accounting_info *ip_acc);
> +int __connman_add_ip_acc_rules(char *ip_addrs);
> +int __connman_delete_ip_acc_rules(char *ip_addrs);
> +int __connman_create_ip_acc_chain(char *ifname);
> +int __connman_destroy_ip_acc_chain(char *ifname);
Function starting with '__' are onley for the ConnMan core, therefore they
should not be in this header file.
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif /* __CONNMAN_ACCOUNTING_IPTABLES_H */
> diff --git a/src/accounting-iptables.c b/src/accounting-iptables.c
> new file mode 100755
> index 0000000..e5b45d9
> --- /dev/null
> +++ b/src/accounting-iptables.c
> @@ -0,0 +1,367 @@
> +/*
> + *
> + * Connection Manager
> + *
> + * Copyright (C) 2007-2013 Intel Corporation. All rights reserved.
Copyright statement is wrong.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301
> USA
> + *
> + */
> +
> +#include <errno.h>
> +#include <getopt.h>
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <unistd.h>
> +#include <errno.h>
> +#include <sys/socket.h>
> +#include <xtables.h>
> +#include <inttypes.h>
> +#include <setjmp.h>
> +
> +#include <linux/netfilter_ipv4/ip_tables.h>
> +#include <linux/netfilter_ipv6/ip6_tables.h>
> +
> +#include "connman.h"
> +#include "src/shared/util.h"
> +
> +#define CHAIN_PREFIX "connman-"
> +#define INGRESS_IP_ACC (CHAIN_PREFIX"INGRESS_IP_ACC")
> +#define EGRESS_IP_ACC (CHAIN_PREFIX"EGRESS_IP_ACC")
> +
> +struct iptables_entry {
> + int type;
> + unsigned int offset;
> + int builtin;
> + int counter_idx;
> + struct ipt_entry *entry;
> + struct ip6t_entry *entry6;
> +};
> +
> +static int get_ingress_ip_acc_info(int type, const char *table_name, const
> char *chain, struct ip_accounting_info *ip_acc)
> +{
> + GList *chain_head, *chain_tail, *list, *next;
> + char src_ip_addrs[INET_ADDRSTRLEN], dst_ip_addrs[INET_ADDRSTRLEN];
> + struct iptables_entry *entry;
> + int err = 0;
> +
> + DBG("%d -t %s -L %s", type, table_name, chain);
> +
> + err = __connman_iptables_get_chain(type, table_name, chain, &chain_head,
> &chain_tail);
> +
> + if (err < 0) {
> + return err;
> + }
Lot's of style issues. Please see our documentation on it. I stop to
comment on the style things but please give checkpatch.pl a go
it will report many of those here. Please take it with a grain of salt
since checkpatch.pl sometimes is a bit overeager and leads to worse code.
Thanks,
Daniel
------------------------------
Date: Wed, 17 Jun 2020 09:21:25 +0200
From: Daniel Wagner <[email protected]>
Subject: Re: ipv6 test web page down
To: KeithG <[email protected]>
Cc: [email protected]
Message-ID: <[email protected]>
Content-Type: text/plain; charset=us-ascii
Hi KeithG,
[cc Marcel]
On Thu, Jun 04, 2020 at 10:11:08AM -0500, KeithG wrote:
> David,
>
> I think there is some mis-configuration with something at the
> server/registration. Shouldn't it only have an ipv6 addres? ipv6.google.com
> does.
>
> If this is a regular test of link functionality at service start, shouldn't
> it be disable-able? How do I do that?
>
> # host ipv6.google.com
> ipv6.google.com is an alias for ipv6.l.google.com.
> ipv6.l.google.com has IPv6 address 2607:f8b0:4009:810::200e
>
> # nslookup ipv6.google.com
> Server: 192.168.2.1
> Address: 192.168.2.1#53
>
> Non-authoritative answer:
> ipv6.google.com canonical name = ipv6.l.google.com.
> Name: ipv6.l.google.com
> Address: 2607:f8b0:4009:810::200e
>
> Versus:
>
> # host ipv6.connman.net
> ipv6.connman.net has address 87.106.208.187
> ipv6.connman.net has IPv6 address 2001:8d8:8b4:c861:5826:fa5f:6690:0
>
> # nslookup ipv6.connman.net
> Server: 192.168.2.1
> Address: 192.168.2.1#53
>
> Non-authoritative answer:
> Name: ipv6.connman.net
> Address: 87.106.208.187
> Name: ipv6.connman.net
> Address: 2001:8d8:8b4:c861:5826:fa5f:6690:0
>
> I cannot ping it.
> # ping ipv6.connman.net
> PING ipv6.connman.net(connman.com (2001:8d8:8b4:c861:5826:fa5f:6690:0)) 56
> data bytes
> ^C
> --- ipv6.connman.net ping statistics ---
> 4 packets transmitted, 0 received, 100% packet loss, time 88ms
>
> but can its ipv4 address...
> # ping -4 ipv6.connman.net
> PING ipv6.connman.net (87.106.208.187) 56(84) bytes of data.
> 64 bytes from senator.holtmann.net (87.106.208.187): icmp_seq=1 ttl=43
> time=126 ms
> 64 bytes from senator.holtmann.net (87.106.208.187): icmp_seq=2 ttl=43
> time=125 ms
> 64 bytes from senator.holtmann.net (87.106.208.187): icmp_seq=3 ttl=43
> time=125 ms
> ^C
> --- ipv6.connman.net ping statistics ---
> 3 packets transmitted, 3 received, 0% packet loss, time 3ms
> rtt min/avg/max/mdev = 124.676/125.263/126.122/0.684 ms
>
> pinging ipv6 does work...
> # ping ipv6.google.com
> PING ipv6.google.com(ord30s31-in-x0e.1e100.net (2607:f8b0:4009:801::200e))
> 56 data bytes
> 64 bytes from ord30s31-in-x0e.1e100.net (2607:f8b0:4009:801::200e):
> icmp_seq=1 ttl=54 time=20.6 ms
> 64 bytes from ord30s31-in-x0e.1e100.net (2607:f8b0:4009:801::200e):
> icmp_seq=2 ttl=54 time=12.3 ms
> ^C
> --- ipv6.google.com ping statistics ---
> 2 packets transmitted, 2 received, 0% packet loss, time 5ms
> rtt min/avg/max/mdev = 12.262/16.431/20.600/4.169 ms
>
> On Thu, Jun 4, 2020 at 8:50 AM David Woodhouse <[email protected]> wrote:
>
> > On Wed, 2020-05-27 at 15:12 -0500, KeithG wrote:
> > > DOn't know if this is the right place or not, but since moving to
> > > connman, I have noticed that the ipv6 test page is down. the ipv4 one
> > > works fine, but I get a mention in the log:
> > > "connmand[254]: Failed to find URL:
> > > http://ipv6.connman.net/online/status.html";
> > >
> > > If I try to get to it from a browser, I get a '502 bad gateway error'
> > >
> > > If I use curl on the cli, I get this:
> > > # curl ipv4.connman.net/online/status.html
> > > <html>
> > > <head>
> > > </head>
> > > <body>
> > > </body>
> > > </html>
> > > # curl ipv6.connman.net/online/status.html
> > > <html>
> > > <head><title>502 Bad Gateway</title></head>
> > > <body bgcolor="white">
> > > <center><h1>502 Bad Gateway</h1></center>
> > > <hr><center>nginx</center>
> > > </body>
> > > </html>
> > >
> > > I think something is not set right on the server...
> >
> > Amusingly it works for me but only over Legacy IP.
> >
> > $ curl -v http://ipv6.connman.net/online/status.html
> > * Trying 2001:8d8:8b4:c861:5826:fa5f:6690:0:80...
> > * Trying 87.106.208.187:80...
> > * Connected to ipv6.connman.net (87.106.208.187) port 80 (#0)
> > > GET /online/status.html HTTP/1.1
> > > Host: ipv6.connman.net
> > > User-Agent: curl/7.69.1
> > > Accept: */*
> > >
> > * Mark bundle as not supporting multiuse
> > < HTTP/1.1 200 OK
> > < Server: nginx
> > < Date: Thu, 04 Jun 2020 13:48:57 GMT
> > < Content-Type: text/html
> > < Connection: close
> > < X-ConnMan-Status: online
> > <
> > <html>
> > <head>
> > </head>
> > <body>
> > </body>
> > </html>
> > * Closing connection 0
> >
> _______________________________________________
> connman mailing list -- [email protected]
> To unsubscribe send an email to [email protected]
------------------------------
Date: Wed, 17 Jun 2020 09:24:52 +0200
From: Daniel Wagner <[email protected]>
Subject: Re: Wifi AP with a captive portal
To: Stavros Vagionitis <[email protected]>
Cc: [email protected]
Message-ID: <[email protected]>
Content-Type: text/plain; charset=us-ascii
Hi Stavros,
On Fri, May 29, 2020 at 04:28:00PM +0100, Stavros Vagionitis wrote:
> Hi all,
>
> I have a question regarding the agent API RequestBrowser (
> https://git.kernel.org/pub/scm/network/connman/connman.git/tree/doc/agent-api.txt#n36
> ).
>
> I use connman v1.37 and I am trying to connect to a wifi AP with a
> captive portal, like the ones at airports or cafes. When I do the
> following using connmanctl, I don't get any URL or the URL is empty.
>
> ```
> connmanctl> agent on
> Agent registered
> connmanctl> connect wifi_304511e7f961_67756573746e6574_managed_none
> connmanctl> [6667749.907750] wlansta0: authenticate with
> 58:0a:20:5b:ec:cf
> [6667749.942790] wlansta0: send auth to 58:0a:20:5b:ec:cf (try 1/3)
> [6667750.115858] wlansta0: authenticated
> [6667750.130032] wlansta0: associate with 58:0a:20:5b:ec:cf (try 1/3)
> [6667750.139199] wlansta0: RX AssocResp from 58:0a:20:5b:ec:cf
> (capab=0x101 status=0 aid=1)
> [6667750.161632] wlansta0: associated
> [6667750.171616] wlcore: Association completed.
> [6667750.178102] IPv6: ADDRCONF(NETDEV_CHANGE): wlansta0: link becomes
> ready
> Connected wifi_304511e7f961_67756573746e6574_managed_none
> Agent RequestBrowser wifi_304511e7f961_67756573746e6574_managed_none
>
> connmanctl> Connected (yes/no)? no
> ```
>
> Is this a normal behavior not to get a URL? If the URL is not provided,
> is there another way to get the URL and add it to the browser?
I am not too familiar with the hotspot protocol. I don't think it is
normal. No idea what's going wrong. I also don't have the corresponding
spec to see what's going on. Best changes it propably to use tcpdump/wireshark
to record the interaction and try to figure out what's going on.
Thanks,
Daniel
------------------------------
Date: Wed, 17 Jun 2020 12:57:56 +0530
From: Aravind Gunasekaran <[email protected]>
Subject: Re: [PATCH] IP Accounting for WiFi Clients
To: Daniel Wagner <[email protected]>
Cc: [email protected]
Message-ID:
<CAJ0PF7c2E9AMgUdaddxuVDsrc=D8RnmwNNMywt=va7_crty...@mail.gmail.com>
Content-Type: multipart/alternative;
boundary="000000000000ef949a05a84299e9"
--000000000000ef949a05a84299e9
Content-Type: text/plain; charset="UTF-8"
HI Daniel,
Thanks for your comments, I will do the needful as you explained.
thanks,
Aravind
On Wed, Jun 17, 2020 at 12:48 PM Daniel Wagner <[email protected]> wrote:
> Hi Aravind,
>
> Please add a commit message explaining why are doing it and what you are
> changing.
>
> I noticed this is for iptables only. The iptables code
> is a bit dangorous to work with. There is no user land library to use.
> Instead we have to do all the work to parse the message from the kernel.
> I am pretty sure there are a lot of hidden bugs in this code and it
> currently it just happens to work. It would be far better if
> you used the nftables code base. For this we have a proper library and
> the code is far cleaner and more robust.
>
> Next thing, please split the patch up into different smaller units.
> At least the hocking up of the new interfaces should go into a new
> patch.
>
> On Wed, May 27, 2020 at 03:33:20PM +0530, Aravind Gunasekaran wrote:
> > diff --git a/Makefile.am b/Makefile.am
> > old mode 100644
> > new mode 100755
> > index 5971ca9..44148df
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -12,7 +12,7 @@ include_HEADERS = include/log.h include/plugin.h \
> > include/storage.h include/provision.h \
> > include/session.h include/ipaddress.h include/agent.h \
> > include/inotify.h include/peer.h include/machine.h \
> > - include/acd.h include/tethering.h
> > + include/acd.h include/tethering.h include/accounting-iptables.h
>
> The header file in the include directory are for the plugins. Unless the
> function you define are used in plugins don't add them here. Instead
> add them to src/connman.h.
>
> > nodist_include_HEADERS = include/version.h
> >
> > @@ -152,7 +152,7 @@ src_connmand_wait_online_LDADD = gdbus/
> > libgdbus-internal.la \
> > @GLIB_LIBS@ @DBUS_LIBS@
> >
> > if XTABLES
> > -src_connmand_SOURCES += src/iptables.c src/firewall-iptables.c
> > +src_connmand_SOURCES += src/iptables.c src/firewall-iptables.c
> > src/accounting-iptables.c
>
> The line should be more indented.
>
> > src_connmand_LDADD += @XTABLES_LIBS@
> > endif
> >
> > diff --git a/include/accounting-iptables.h
> b/include/accounting-iptables.h
> > new file mode 100755
> > index 0000000..3a9cc30
> > --- /dev/null
> > +++ b/include/accounting-iptables.h
> > @@ -0,0 +1,49 @@
> > +/*
> > + *
> > + * Connection Manager
> > + *
> > + * Copyright (C) 2007-2013 Intel Corporation. All rights reserved.
>
> This copyright statment is wrong.
>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
> 02110-1301
> > USA
> > + *
> > + */
> > +
> > +#ifndef __CONNMAN_ACCOUNTING_IPTABLES_H
> > +#define __CONNMAN_ACCOUNTING_IPTABLES_H
> > +
> > +#ifdef __cplusplus
> > +extern "C" {
> > +#endif
> > +
> > +struct counters {
> > + __u64 pcnt, bcnt; /* Packet and byte counters */
> > +};
> > +
> > +struct ip_accounting_info {
> > + char *ipv4_address; /* IP address of the client */
> > + struct counters egress; /* Transmitted bytes information of the client
> */
> > + struct counters ingress; /* Received bytes information of the client */
> > +};
>
> Use tabs, see our codying style document. You can also use the
> checkpatch.pl
> script from the Linux kernel to check for style issues.
>
> > +
> > +int __connman_get_ip_acc_info(struct ip_accounting_info *ip_acc);
> > +int __connman_add_ip_acc_rules(char *ip_addrs);
> > +int __connman_delete_ip_acc_rules(char *ip_addrs);
> > +int __connman_create_ip_acc_chain(char *ifname);
> > +int __connman_destroy_ip_acc_chain(char *ifname);
>
> Function starting with '__' are onley for the ConnMan core, therefore they
> should not be in this header file.
>
> > +
> > +#ifdef __cplusplus
> > +}
> > +#endif
> > +
> > +#endif /* __CONNMAN_ACCOUNTING_IPTABLES_H */
> > diff --git a/src/accounting-iptables.c b/src/accounting-iptables.c
> > new file mode 100755
> > index 0000000..e5b45d9
> > --- /dev/null
> > +++ b/src/accounting-iptables.c
> > @@ -0,0 +1,367 @@
> > +/*
> > + *
> > + * Connection Manager
> > + *
> > + * Copyright (C) 2007-2013 Intel Corporation. All rights reserved.
>
> Copyright statement is wrong.
>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
> 02110-1301
> > USA
> > + *
> > + */
> > +
> > +#include <errno.h>
> > +#include <getopt.h>
> > +#include <stdlib.h>
> > +#include <stdio.h>
> > +#include <string.h>
> > +#include <unistd.h>
> > +#include <errno.h>
> > +#include <sys/socket.h>
> > +#include <xtables.h>
> > +#include <inttypes.h>
> > +#include <setjmp.h>
> > +
> > +#include <linux/netfilter_ipv4/ip_tables.h>
> > +#include <linux/netfilter_ipv6/ip6_tables.h>
> > +
> > +#include "connman.h"
> > +#include "src/shared/util.h"
> > +
> > +#define CHAIN_PREFIX "connman-"
> > +#define INGRESS_IP_ACC (CHAIN_PREFIX"INGRESS_IP_ACC")
> > +#define EGRESS_IP_ACC (CHAIN_PREFIX"EGRESS_IP_ACC")
> > +
> > +struct iptables_entry {
> > + int type;
> > + unsigned int offset;
> > + int builtin;
> > + int counter_idx;
> > + struct ipt_entry *entry;
> > + struct ip6t_entry *entry6;
> > +};
> > +
> > +static int get_ingress_ip_acc_info(int type, const char *table_name,
> const
> > char *chain, struct ip_accounting_info *ip_acc)
> > +{
> > + GList *chain_head, *chain_tail, *list, *next;
> > + char src_ip_addrs[INET_ADDRSTRLEN], dst_ip_addrs[INET_ADDRSTRLEN];
> > + struct iptables_entry *entry;
> > + int err = 0;
> > +
> > + DBG("%d -t %s -L %s", type, table_name, chain);
> > +
> > + err = __connman_iptables_get_chain(type, table_name, chain,
> &chain_head,
> > &chain_tail);
> > +
> > + if (err < 0) {
> > + return err;
> > + }
>
> Lot's of style issues. Please see our documentation on it. I stop to
> comment on the style things but please give checkpatch.pl a go
> it will report many of those here. Please take it with a grain of salt
> since checkpatch.pl sometimes is a bit overeager and leads to worse code.
>
> Thanks,
> Daniel
>
--
Thanks,
Aravind
--000000000000ef949a05a84299e9
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
<div dir=3D"ltr">HI Daniel,<div><br></div><div>Thanks for your comments, I =
will do the needful as you explained.</div><div><br></div><div>thanks,</div=
><div>Aravind</div></div><br><div class=3D"gmail_quote"><div dir=3D"ltr" cl=
ass=3D"gmail_attr">On Wed, Jun 17, 2020 at 12:48 PM Daniel Wagner <<a hr=
ef=3D"mailto:[email protected]";>[email protected]</a>> wrote:<br></div><blockq=
uote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1p=
x solid rgb(204,204,204);padding-left:1ex">Hi Aravind,<br>
<br>
Please add a commit message explaining why are doing it and what you are<br=
>
changing.<br>
<br>
I noticed this is for iptables only. The iptables code<br>
is a bit dangorous to work with. There is no user land library to use.<br>
Instead we have to do all the work to parse the message from the kernel.<br=
>
I am pretty sure there are a lot of hidden bugs in this code and it<br>
currently it just happens to work. It would be far better if<br>
you used the nftables code base. For this we have a proper library and<br>
the code is far cleaner and more robust.<br>
<br>
Next thing, please split the patch up into different smaller units.<br>
At least the hocking up of the new interfaces should go into a new<br>
patch.<br>
<br>
On Wed, May 27, 2020 at 03:33:20PM +0530, Aravind Gunasekaran wrote:<br>
>=C2=A0 diff --git a/Makefile.am b/Makefile.am<br>
> old mode 100644<br>
> new mode 100755<br>
> index 5971ca9..44148df<br>
> --- a/Makefile.am<br>
> +++ b/Makefile.am<br>
> @@ -12,7 +12,7 @@ include_HEADERS =3D include/log.h include/plugin.h \=
<br>
>=C2=A0 =C2=A0include/storage.h include/provision.h \<br>
>=C2=A0 =C2=A0include/session.h include/ipaddress.h include/agent.h \<br=
>
>=C2=A0 =C2=A0include/inotify.h include/peer.h include/machine.h \<br>
> - include/acd.h include/tethering.h<br>
> + include/acd.h include/tethering.h include/accounting-iptables.h<br>
<br>
The header file in the include directory are for the plugins. Unless the<br=
>
function you define are used in plugins don't add them here. Instead<br=
>
add them to src/connman.h.<br>
<br>
>=C2=A0 nodist_include_HEADERS =3D include/version.h<br>
> <br>
> @@ -152,7 +152,7 @@ src_connmand_wait_online_LDADD =3D gdbus/<br>
> <a href=3D"http://libgdbus-internal.la"; rel=3D"noreferrer" target=3D"_=
blank">libgdbus-internal.la</a> \<br>
>=C2=A0 =C2=A0@GLIB_LIBS@ @DBUS_LIBS@<br>
> <br>
>=C2=A0 if XTABLES<br>
> -src_connmand_SOURCES +=3D src/iptables.c src/firewall-iptables.c<br>
> +src_connmand_SOURCES +=3D src/iptables.c src/firewall-iptables.c<br>
> src/accounting-iptables.c<br>
<br>
The line should be more indented.<br>
<br>
>=C2=A0 src_connmand_LDADD +=3D @XTABLES_LIBS@<br>
>=C2=A0 endif<br>
> <br>
> diff --git a/include/accounting-iptables.h b/include/accounting-iptabl=
es.h<br>
> new file mode 100755<br>
> index 0000000..3a9cc30<br>
> --- /dev/null<br>
> +++ b/include/accounting-iptables.h<br>
> @@ -0,0 +1,49 @@<br>
> +/*<br>
> + *<br>
> + *=C2=A0 Connection Manager<br>
> + *<br>
> + *=C2=A0 Copyright (C) 2007-2013=C2=A0 Intel Corporation. All rights =
reserved.<br>
<br>
This copyright statment is wrong.<br>
<br>
> + *<br>
> + *=C2=A0 This program is free software; you can redistribute it and/o=
r modify<br>
> + *=C2=A0 it under the terms of the GNU General Public License version=
2 as<br>
> + *=C2=A0 published by the Free Software Foundation.<br>
> + *<br>
> + *=C2=A0 This program is distributed in the hope that it will be usef=
ul,<br>
> + *=C2=A0 but WITHOUT ANY WARRANTY; without even the implied warranty =
of<br>
> + *=C2=A0 MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.=C2=A0 S=
ee the<br>
> + *=C2=A0 GNU General Public License for more details.<br>
> + *<br>
> + *=C2=A0 You should have received a copy of the GNU General Public Li=
cense<br>
> + *=C2=A0 along with this program; if not, write to the Free Software<=
br>
> + *=C2=A0 Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA=C2=
=A0 02110-1301<br>
>=C2=A0 USA<br>
> + *<br>
> + */<br>
> +<br>
> +#ifndef __CONNMAN_ACCOUNTING_IPTABLES_H<br>
> +#define __CONNMAN_ACCOUNTING_IPTABLES_H<br>
> +<br>
> +#ifdef __cplusplus<br>
> +extern "C" {<br>
> +#endif<br>
> +<br>
> +struct counters {<br>
> + __u64 pcnt, bcnt;=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/* Packet =
and byte counters */<br>
> +};<br>
> +<br>
> +struct ip_accounting_info {<br>
> + char *ipv4_address; /* IP address of the client */<br>
> + struct counters egress; /* Transmitted bytes information of the clie=
nt */<br>
> + struct counters ingress; /* Received bytes information of the client=
*/<br>
> +};<br>
<br>
Use tabs, see our codying style document. You can also use the <a href=3D"h=
ttp://checkpatch.pl" rel=3D"noreferrer" target=3D"_blank">checkpatch.pl</a>=
<br>
script from the Linux kernel to check for style issues.<br>
<br>
> +<br>
> +int __connman_get_ip_acc_info(struct ip_accounting_info *ip_acc);<br>
> +int __connman_add_ip_acc_rules(char *ip_addrs);<br>
> +int __connman_delete_ip_acc_rules(char *ip_addrs);<br>
> +int __connman_create_ip_acc_chain(char *ifname);<br>
> +int __connman_destroy_ip_acc_chain(char *ifname);<br>
<br>
Function starting with '__' are onley for the ConnMan core, therefo=
re they<br>
should not be in this header file.<br>
<br>
> +<br>
> +#ifdef __cplusplus<br>
> +}<br>
> +#endif<br>
> +<br>
> +#endif /* __CONNMAN_ACCOUNTING_IPTABLES_H */<br>
> diff --git a/src/accounting-iptables.c b/src/accounting-iptables.c<br>
> new file mode 100755<br>
> index 0000000..e5b45d9<br>
> --- /dev/null<br>
> +++ b/src/accounting-iptables.c<br>
> @@ -0,0 +1,367 @@<br>
> +/*<br>
> + *<br>
> + *=C2=A0 Connection Manager<br>
> + *<br>
> + *=C2=A0 Copyright (C) 2007-2013=C2=A0 Intel Corporation. All rights =
reserved.<br>
<br>
Copyright statement is wrong.<br>
<br>
> + *<br>
> + *=C2=A0 This program is free software; you can redistribute it and/o=
r modify<br>
> + *=C2=A0 it under the terms of the GNU General Public License version=
2 as<br>
> + *=C2=A0 published by the Free Software Foundation.<br>
> + *<br>
> + *=C2=A0 This program is distributed in the hope that it will be usef=
ul,<br>
> + *=C2=A0 but WITHOUT ANY WARRANTY; without even the implied warranty =
of<br>
> + *=C2=A0 MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.=C2=A0 S=
ee the<br>
> + *=C2=A0 GNU General Public License for more details.<br>
> + *<br>
> + *=C2=A0 You should have received a copy of the GNU General Public Li=
cense<br>
> + *=C2=A0 along with this program; if not, write to the Free Software<=
br>
> + *=C2=A0 Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA=C2=
=A0 02110-1301<br>
>=C2=A0 USA<br>
> + *<br>
> + */<br>
> +<br>
> +#include <errno.h><br>
> +#include <getopt.h><br>
> +#include <stdlib.h><br>
> +#include <stdio.h><br>
> +#include <string.h><br>
> +#include <unistd.h><br>
> +#include <errno.h><br>
> +#include <sys/socket.h><br>
> +#include <xtables.h><br>
> +#include <inttypes.h><br>
> +#include <setjmp.h><br>
> +<br>
> +#include <linux/netfilter_ipv4/ip_tables.h><br>
> +#include <linux/netfilter_ipv6/ip6_tables.h><br>
> +<br>
> +#include "connman.h"<br>
> +#include "src/shared/util.h"<br>
> +<br>
> +#define CHAIN_PREFIX "connman-"<br>
> +#define INGRESS_IP_ACC (CHAIN_PREFIX"INGRESS_IP_ACC")<br>
> +#define EGRESS_IP_ACC (CHAIN_PREFIX"EGRESS_IP_ACC")<br>
> +<br>
> +struct iptables_entry {<br>
> + int type;<br>
> + unsigned int offset;<br>
> + int builtin;<br>
> + int counter_idx;<br>
> + struct ipt_entry *entry;<br>
> + struct ip6t_entry *entry6;<br>
> +};<br>
> +<br>
> +static int get_ingress_ip_acc_info(int type, const char *table_name, =
const<br>
> char *chain, struct ip_accounting_info *ip_acc)<br>
> +{<br>
> + GList *chain_head, *chain_tail, *list, *next;<br>
> + char src_ip_addrs[INET_ADDRSTRLEN], dst_ip_addrs[INET_ADDRSTRLEN];<b=
r>
> + struct iptables_entry *entry;<br>
> + int err =3D 0;<br>
> +<br>
> + DBG("%d -t %s -L %s", type, table_name, chain);<br>
> +<br>
> + err =3D=C2=A0 __connman_iptables_get_chain(type, table_name, chain, =
&chain_head,<br>
> &chain_tail);<br>
> +<br>
> + if (err < 0) {<br>
> + return err;<br>
> + }<br>
<br>
Lot's of style issues. Please see our documentation on it. I stop to<br=
>
comment on the style things but please give <a href=3D"http://checkpatch.pl=
" rel=3D"noreferrer" target=3D"_blank">checkpatch.pl</a> a go<br>
it will report many of those here. Please take it with a grain of salt<br>
since <a href=3D"http://checkpatch.pl"; rel=3D"noreferrer" target=3D"_blank"=
>checkpatch.pl</a> sometimes is a bit overeager and leads to worse code.<br=
>
<br>
Thanks,<br>
Daniel<br>
</blockquote></div><br clear=3D"all"><div><br></div>-- <br><div dir=3D"ltr"=
class=3D"gmail_signature"><div dir=3D"ltr">Thanks,<div>Aravind</div></div>=
</div>
--000000000000ef949a05a84299e9--
------------------------------
Subject: Digest Footer
_______________________________________________
connman mailing list -- [email protected]
To unsubscribe send an email to [email protected]
------------------------------
End of connman Digest, Vol 56, Issue 5
**************************************
