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 <wagi@monom.org> 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