Hi,
Thanks for the patch! In the end it was a surprisingly small amount of
code nedeed for VLAN support.
A few comments below.
On Sun, 2015-04-12 at 10:37 +0200, Marcus Folkesson wrote:
> The VLAN shows up as an ethernet-service with the name convention
> ethernet_<MAC>_<VLAN ID>_cable
>
> The VLAN ID is represented in a three digit hex-value. For example:
> ethernet_5c260a4bf6a3_00a_cable
>
> Signed-off-by: Marcus Folkesson <[email protected]>
We haven't been using Signed-off-by in commit messages as there is no
maintainer hierarchy in the project. So you can omit this, I'd be
scraping it off from the commit messge anyway.
Please mention Justin Maggard, the patches for this part are virtually
identical.
> ---
> plugins/ethernet.c | 45 ++++++++++++++++++++++++++++++++++++++++++---
> src/rtnl.c | 3 +++
> 2 files changed, 45 insertions(+), 3 deletions(-)
>
> diff --git a/plugins/ethernet.c b/plugins/ethernet.c
> index b8e52ce..04030d4 100644
> --- a/plugins/ethernet.c
> +++ b/plugins/ethernet.c
> @@ -25,6 +25,13 @@
>
> #include <errno.h>
> #include <net/if.h>
> +#include <string.h>
> +#include <sys/ioctl.h>
> +#include <unistd.h>
> +#include <stdio.h>
> +
> +#include <linux/if_vlan.h>
> +#include <linux/sockios.h>
>
> #ifndef IFF_LOWER_UP
> #define IFF_LOWER_UP 0x10000
> @@ -50,6 +57,27 @@ struct ethernet_data {
> struct connman_network *network;
> };
>
> +
> +static int connman_inet_get_vlan_vid(const char *ifname)
This function is in plugins/ethernet.c, so it should not start with
connman_inet_. A static function named for example get_vlan_vid() is
good enough.
> +{
> + struct vlan_ioctl_args vifr;
> + int vid;
> + int sk;
> +
> + memset(&vifr, '\0', sizeof(vifr));
Nitpick: a 0 does as well as the '\0'.
> +
> + sk = socket(AF_INET, SOCK_STREAM, 0);
> + if (sk < 0)
> + return -errno;
> +
> + vifr.cmd = GET_VLAN_VID_CMD;
> + strncpy(vifr.device1, ifname, sizeof(vifr.device1));
> + vid = ioctl(sk, SIOCSIFVLAN, &vifr) ? -errno : vifr.u.VID;
Nitpick: might be easier to read if written using an explicit 'if'
statement.
> + close(sk);
> +
> + return vid;
> +}
> +
> static int eth_network_probe(struct connman_network *network)
> {
> DBG("network %p", network);
> @@ -93,7 +121,8 @@ static void add_network(struct connman_device *device,
> struct ethernet_data *ethernet)
> {
> struct connman_network *network;
> - int index;
> + int index, vid;
> + char *ifname;
>
> network = connman_network_create("carrier",
> CONNMAN_NETWORK_TYPE_ETHERNET);
> @@ -102,6 +131,10 @@ static void add_network(struct connman_device *device,
>
> index = connman_device_get_index(device);
> connman_network_set_index(network, index);
> + ifname = connman_inet_ifname(index);
> + if (!ifname)
> + return;
> + vid = connman_inet_get_vlan_vid(ifname);
>
> connman_network_set_name(network, "Wired");
>
> @@ -110,15 +143,21 @@ static void add_network(struct connman_device *device,
> return;
> }
>
> - if (!eth_tethering)
> + if (!eth_tethering) {
> + char group[10] = "cable";
> /*
> * Prevent service from starting the reconnect
> * procedure as we do not want the DHCP client
> * to run when tethering.
> */
> - connman_network_set_group(network, "cable");
> + if (vid >= 0)
> + snprintf(group, sizeof(group), "%03x_cable", vid);
> +
> + connman_network_set_group(network, group);
> + }
>
> ethernet->network = network;
> + g_free(ifname);
> }
>
> static void remove_network(struct connman_device *device,
> diff --git a/src/rtnl.c b/src/rtnl.c
> index b8b02c4..d1b851f 100644
> --- a/src/rtnl.c
> +++ b/src/rtnl.c
> @@ -170,6 +170,9 @@ static void read_uevent(struct interface_data *interface)
> } else if (strcmp(line + 8, "gadget") == 0) {
> interface->service_type = CONNMAN_SERVICE_TYPE_GADGET;
> interface->device_type = CONNMAN_DEVICE_TYPE_GADGET;
> + } else if (strcmp(line + 8, "vlan") == 0) {
> + interface->service_type = CONNMAN_SERVICE_TYPE_ETHERNET;
> + interface->device_type = CONNMAN_DEVICE_TYPE_ETHERNET;
>
> } else {
> interface->service_type = CONNMAN_SERVICE_TYPE_UNKNOWN;
Sorry it took a bit too long to review, been busy with other areas
lately.
Cheers,
Patrik
_______________________________________________
connman mailing list
[email protected]
https://lists.connman.net/mailman/listinfo/connman
