Hi Samuel, On Fri, Oct 1, 2010 at 12:34 PM, Samuel Ortiz <[email protected]> wrote: > 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 ? I'm modifying the code to meet your suggestions and changing it to be used for IPv6 settings also. I don't know if I'll be able to test IPv6 settings, though.
> Cheers, > Samuel. > > -- > Intel Open Source Technology Centre > http://oss.intel.com/ > _______________________________________________ > connman mailing list > [email protected] > http://lists.connman.net/listinfo/connman > Regards, Cristiano _______________________________________________ connman mailing list [email protected] http://lists.connman.net/listinfo/connman
