Hi Cristiano,
On Thu, Sep 30, 2010 at 04:43:15PM -0300, [email protected] wrote:
> From: Cristiano Fernandes <[email protected]>
>
> Currently connman uses ioctl to set address, netmask and broadcast separately.
> This imples that when a ip address with a non default netmask is set to an
> interface (192.168.1.10/20 for instance), a netlink event will be trigged
> announcing that the address has been configured with default netmask
> (192.168.1.10/24). Connman will then signal an IPv4 change with the wrong
> netmask. When the correct netmask is set with ioctl, a removal netlink event
> will be trigged that connman will signal again as an IPv4 change. Finally,
> the correct address and netmask are set, another netlink event announces an
> address change and connman signals IPv4 change with the correct values.
>
> Using netlink to set and clear ipv4 configuration avoids connman from
> sending multiple IPv4 signals through DBus with the wrong configuration,
> since the configuration are set all at once triggering only one netlink
> event.
Some comments:
> +int connman_inet_modify_address(int cmd, int flags, int index,
> + const char *address,
> + const char *netmask,
> + const char *broadcast);
As this is a routine that is going to be accessible only to ConnMan core
components, the rule is:
- Define it in src/connman.h
- Use the __connman prefix.
> int connman_inet_set_address(int index, struct connman_ipaddress *ipaddress);
> -int connman_inet_clear_address(int index);
> +int connman_inet_clear_address(int index, struct connman_ipaddress
> *ipaddress);
> int connman_inet_add_host_route(int index, const char *host, const char
> *gateway);
> int connman_inet_del_host_route(int index, const char *host);
> int connman_inet_set_gateway_address(int index, const char *gateway);
> diff --git a/include/ipconfig.h b/include/ipconfig.h
> index 28a3d6a..ad4dbf4 100644
> --- a/include/ipconfig.h
> +++ b/include/ipconfig.h
> @@ -74,6 +74,8 @@ struct connman_ipconfig_ops {
> void (*ip_release) (struct connman_ipconfig *ipconfig);
> };
>
> +unsigned char netmask2prefixlen(const char *netmask);
> +
Same applies here, this should be defined in connman.h, and be called e.g.
__connman_ipconfig_netmask_prefix_len().
> struct connman_ipconfig *connman_ipconfig_create(int index);
> struct connman_ipconfig *connman_ipconfig_clone(struct connman_ipconfig
> *ipconfig);
> struct connman_ipconfig *connman_ipconfig_ref(struct connman_ipconfig
> *ipconfig);
> diff --git a/src/inet.c b/src/inet.c
> index 3f1547a..28fc4f8 100644
> --- a/src/inet.c
> +++ b/src/inet.c
> @@ -39,6 +39,104 @@
>
> #include "connman.h"
>
> +#define NLMSG_TAIL(nmsg) \
> + ((struct rtattr *) (((uint8_t*) (nmsg)) + \
> + NLMSG_ALIGN((nmsg)->nlmsg_len)))
> +
> +static int add_rtattr(struct nlmsghdr *n, size_t max_length, int type,
> + const void *data, size_t data_length)
> +{
> + size_t length;
> + struct rtattr *rta;
> +
> + length = RTA_LENGTH(data_length);
> +
> + if (NLMSG_ALIGN(n->nlmsg_len) + RTA_ALIGN(length) > max_length)
> + return -E2BIG;
> +
> + rta = NLMSG_TAIL(n);
> + rta->rta_type = type;
> + rta->rta_len = length;
> + memcpy(RTA_DATA(rta), data, data_length);
> + n->nlmsg_len = NLMSG_ALIGN(n->nlmsg_len) + RTA_ALIGN(length);
> +
> + return 0;
> +}
> +
> +int connman_inet_modify_address(int cmd, int flags, int index,
> + const char *address,
> + const char *netmask,
> + const char *broadcast)
> +{
> + union {
> + struct sockaddr sa;
> + struct sockaddr_nl nl;
> + } sa;
> + union {
> + struct nlmsghdr header;
> + uint8_t buf[NLMSG_ALIGN(sizeof(struct nlmsghdr)) +
> + NLMSG_ALIGN(sizeof(struct ifaddrmsg)) +
> + RTA_LENGTH(sizeof(struct in6_addr))];
> + } request;
> + struct ifaddrmsg *ifaddrmsg;
> + uint32_t addr, nmask, bcast;
> + unsigned char prefixlen;
> + int sk;
> +
> + DBG("");
> +
> + if (address == NULL || netmask == NULL)
> + return -1;
> +
> + sk = socket(AF_NETLINK, SOCK_DGRAM, NETLINK_ROUTE);
> + if (sk < 0)
> + return -1;
> +
> + addr = inet_addr(address);
> + nmask = inet_addr(netmask);
> + prefixlen = netmask2prefixlen(netmask);
> +
> + if (broadcast != NULL)
> + bcast = inet_addr(broadcast);
> + else
> + bcast = addr | ~nmask;
> +
> + memset(&request, 0, sizeof(request));
> +
> + request.header.nlmsg_len = NLMSG_LENGTH(sizeof(struct ifaddrmsg));
> + request.header.nlmsg_type = cmd;
> + request.header.nlmsg_flags = NLM_F_REQUEST|flags;
> + request.header.nlmsg_seq = 1;
> +
> + ifaddrmsg = NLMSG_DATA(&request.header);
This is breaking strict aliasing.
Your code could be a bit simpler by just using a uint8_t pointer for the
request, and casting it to a nlmsghdr pointer where needed. Something along
those lines:
+int connman_inet_modify_address(int cmd, int flags, int index,
+ const char *address,
+ const char *netmask,
+ const char *broadcast)
+{
+ uint8_t request[NLMSG_ALIGN(sizeof(struct nlmsghdr)) +
+ NLMSG_ALIGN(sizeof(struct ifaddrmsg)) +
+ RTA_LENGTH(sizeof(struct in6_addr))];
+
+ struct sockaddr_nl nl_addr;
+ struct ifaddrmsg *ifaddrmsg;
+ struct nlmsghdr *header;
+ uint32_t addr, nmask, bcast;
+ unsigned char prefixlen;
+ int sk;
+
+ DBG("");
+
+ if (address == NULL || netmask == NULL)
+ return -1;
+
+ sk = socket(AF_NETLINK, SOCK_DGRAM, NETLINK_ROUTE);
+ if (sk < 0)
+ return -1;
+
+ addr = inet_addr(address);
+ nmask = inet_addr(netmask);
+ prefixlen = netmask2prefixlen(netmask);
+
+ if (broadcast != NULL)
+ bcast = inet_addr(broadcast);
+ else
+ bcast = addr | ~nmask;
+
+ memset(&request, 0, sizeof(request));
+
+ header = (struct nlmsghdr *)request;
+
+ header->nlmsg_len = NLMSG_LENGTH(sizeof(struct ifaddrmsg));
+ header->nlmsg_type = cmd;
+ header->nlmsg_flags = NLM_F_REQUEST|flags;
+ header->nlmsg_seq = 1;
+
+ ifaddrmsg = NLMSG_DATA(header);
+ ifaddrmsg->ifa_family = AF_INET;
+ ifaddrmsg->ifa_prefixlen = prefixlen;
+ ifaddrmsg->ifa_flags = IFA_F_PERMANENT;
+ ifaddrmsg->ifa_scope = RT_SCOPE_UNIVERSE;
+ ifaddrmsg->ifa_index = index;
+
+ if (add_rtattr(header, sizeof(request), IFA_LOCAL,
+ &addr, sizeof(addr)) < 0)
+ return -1;
+
+ if (add_rtattr(header, sizeof(request), IFA_BROADCAST,
+ &bcast, sizeof(bcast)) < 0)
+ return -1;
+
+ memset(&nl_addr, 0, sizeof(nl_addr));
+ nl_addr.nl_family = AF_NETLINK;
+
+ if (sendto(sk, request, header->nlmsg_len, 0,
+ (struct sockaddr *) &nl_addr, sizeof(nl_addr)))
+ return -1;
+
+ close(sk);
+
+ return 0;
+}
+
Also, could we also use a netlink socket for the IPv6 address settings ?
Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/
_______________________________________________
connman mailing list
[email protected]
http://lists.connman.net/listinfo/connman
