Discussion:
[PATCH] add management of dsa interface
(too old to reply)
Laurent Vaudoit
2015-09-28 12:48:20 UTC
Permalink
Hi,
here is a patch inspired from vlan management, for interface using dsa linux framework
mailing list thread is this one
https://lists.connman.net/pipermail/connman/2015-September/019788.html

Regards
Laurent
Laurent Vaudoit
2015-09-28 12:48:21 UTC
Permalink
---
plugins/ethernet.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 57 insertions(+), 3 deletions(-)

diff --git a/plugins/ethernet.c b/plugins/ethernet.c
index d176508..456d3cc 100644
--- a/plugins/ethernet.c
+++ b/plugins/ethernet.c
@@ -32,6 +32,7 @@

#include <linux/if_vlan.h>
#include <linux/sockios.h>
+#include <linux/ethtool.h>

#ifndef IFF_LOWER_UP
#define IFF_LOWER_UP 0x10000
@@ -83,6 +84,54 @@ static int get_vlan_vid(const char *ifname)
return vid;
}

+static int get_dsa_port(const char *ifname)
+{
+ int sk;
+ int dsaport=-1;
+ struct ifreq ifr;
+ struct ethtool_cmd cmd;
+ struct ethtool_drvinfo drvinfocmd;
+ struct vlan_ioctl_args vifr;
+
+ sk = socket(AF_INET, SOCK_STREAM, 0);
+ if (sk < 0)
+ return -errno;
+
+ memset(&ifr, 0, sizeof(ifr));
+ strcpy(ifr.ifr_name, ifname);
+
+ /* check if it is a vlan and get physical interface name*/
+ vifr.cmd = GET_VLAN_REALDEV_NAME_CMD;
+ strncpy(vifr.device1, ifname, sizeof(vifr.device1));
+
+ if(ioctl(sk, SIOCSIFVLAN, &vifr) >= 0)
+ strcpy(ifr.ifr_name, vifr.u.device2);
+
+ /* get driver info */
+ drvinfocmd.cmd = ETHTOOL_GDRVINFO;
+ ifr.ifr_data = (caddr_t)&drvinfocmd;
+
+ /* ioctl failed: */
+ if (ioctl(sk, SIOCETHTOOL, &ifr) != 0)
+ DBG("Cannot get driver infos\n");
+ else {
+ if(strcmp(drvinfocmd.driver,"dsa") == 0) {
+ /* get dsa port*/
+ cmd.cmd = ETHTOOL_GSET;
+ ifr.ifr_data = (caddr_t)&cmd;
+
+ /* ioctl failed: */
+ if (ioctl(sk, SIOCETHTOOL, &ifr) != 0)
+ DBG("Cannot get device settings\n");
+ else
+ dsaport = cmd.phy_address;
+ }
+ }
+ close(sk);
+
+ return dsaport;
+}
+
static int eth_network_probe(struct connman_network *network)
{
DBG("network %p", network);
@@ -126,7 +175,7 @@ static void add_network(struct connman_device *device,
struct ethernet_data *ethernet)
{
struct connman_network *network;
- int index, vid;
+ int index, vid,dsaport;
char *ifname;

network = connman_network_create("carrier",
@@ -140,6 +189,7 @@ static void add_network(struct connman_device *device,
if (!ifname)
return;
vid = get_vlan_vid(ifname);
+ dsaport = get_dsa_port(ifname);

connman_network_set_name(network, "Wired");

@@ -149,14 +199,18 @@ static void add_network(struct connman_device *device,
}

if (!eth_tethering) {
- char group[10] = "cable";
+ char group[16] = "cable";
/*
* Prevent service from starting the reconnect
* procedure as we do not want the DHCP client
* to run when tethering.
*/
- if (vid >= 0)
+ if((vid >= 0) && (dsaport >= 0))
+ snprintf(group, sizeof(group), "p%02x_%03x_cable", dsaport, vid);
+ else if (vid >= 0)
snprintf(group, sizeof(group), "%03x_cable", vid);
+ else if (dsaport >= 0)
+ snprintf(group, sizeof(group), "p%02x_cable", dsaport);

connman_network_set_group(network, group);
}
--
1.9.1
Patrik Flykt
2015-10-01 07:42:29 UTC
Permalink
Hi,

git am complains of trailing white space errors on lines 47 and 56.
While looking at the patch, all lines end with ^M, please fix.
Post by Laurent Vaudoit
---
plugins/ethernet.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 57 insertions(+), 3 deletions(-)
diff --git a/plugins/ethernet.c b/plugins/ethernet.c
index d176508..456d3cc 100644
--- a/plugins/ethernet.c
+++ b/plugins/ethernet.c
@@ -32,6 +32,7 @@
#include <linux/if_vlan.h>
#include <linux/sockios.h>
+#include <linux/ethtool.h>
#ifndef IFF_LOWER_UP
#define IFF_LOWER_UP 0x10000
@@ -83,6 +84,54 @@ static int get_vlan_vid(const char *ifname)
return vid;
}
+static int get_dsa_port(const char *ifname)
+{
+ int sk;
+ int dsaport=-1;
Spaces before and after =
Post by Laurent Vaudoit
+ struct ifreq ifr;
+ struct ethtool_cmd cmd;
+ struct ethtool_drvinfo drvinfocmd;
+ struct vlan_ioctl_args vifr;
+
+ sk = socket(AF_INET, SOCK_STREAM, 0);
+ if (sk < 0)
+ return -errno;
+
+ memset(&ifr, 0, sizeof(ifr));
+ strcpy(ifr.ifr_name, ifname);
ifr.ifr_name probably has a max lenght and I don't know if it must end
in a \0. If it does not defined to end with a \0 or max length is
specified, a strncpy should be used instead.
Post by Laurent Vaudoit
+
+ /* check if it is a vlan and get physical interface name*/
+ vifr.cmd = GET_VLAN_REALDEV_NAME_CMD;
+ strncpy(vifr.device1, ifname, sizeof(vifr.device1));
+
+ if(ioctl(sk, SIOCSIFVLAN, &vifr) >= 0)
+ strcpy(ifr.ifr_name, vifr.u.device2);
If this failed, can we return here? Is strcpy safe here for all string
lengths as above?
Post by Laurent Vaudoit
+
+ /* get driver info */
+ drvinfocmd.cmd = ETHTOOL_GDRVINFO;
+ ifr.ifr_data = (caddr_t)&drvinfocmd;
+
+ /* ioctl failed: */
+ if (ioctl(sk, SIOCETHTOOL, &ifr) != 0)
+ DBG("Cannot get driver infos\n");
Could this be turned around without the debug, as the more likely case
is that this always works, and with the error returned in the return
value. One can later on see that it worked, as the identifier is
different. And the comment is also a bit unnecessary then.

Compare zero with '!'<expression>.
Post by Laurent Vaudoit
+ else {
+ if(strcmp(drvinfocmd.driver,"dsa") == 0) {
+ /* get dsa port*/
+ cmd.cmd = ETHTOOL_GSET;
+ ifr.ifr_data = (caddr_t)&cmd;
+
+ /* ioctl failed: */
+ if (ioctl(sk, SIOCETHTOOL, &ifr) != 0)
+ DBG("Cannot get device settings\n");
Same here.
Post by Laurent Vaudoit
+ else
+ dsaport = cmd.phy_address;
+ }
+ }
+ close(sk);
+
+ return dsaport;
+}
+
static int eth_network_probe(struct connman_network *network)
{
DBG("network %p", network);
@@ -126,7 +175,7 @@ static void add_network(struct connman_device *device,
struct ethernet_data *ethernet)
{
struct connman_network *network;
- int index, vid;
+ int index, vid,dsaport;
Nitpick: space after comma.
Post by Laurent Vaudoit
char *ifname;
network = connman_network_create("carrier",
@@ -140,6 +189,7 @@ static void add_network(struct connman_device *device,
if (!ifname)
return;
vid = get_vlan_vid(ifname);
+ dsaport = get_dsa_port(ifname);
connman_network_set_name(network, "Wired");
@@ -149,14 +199,18 @@ static void add_network(struct connman_device *device,
}
if (!eth_tethering) {
- char group[10] = "cable";
+ char group[16] = "cable";
/*
* Prevent service from starting the reconnect
* procedure as we do not want the DHCP client
* to run when tethering.
*/
- if (vid >= 0)
+ if((vid >= 0) && (dsaport >= 0))
dsaport need only be assigned here, can you move it down here please. I
just noticed that then vid is also misplaced, but don't worry about
that.
Post by Laurent Vaudoit
+ snprintf(group, sizeof(group), "p%02x_%03x_cable", dsaport, vid);
+ else if (vid >= 0)
snprintf(group, sizeof(group), "%03x_cable", vid);
+ else if (dsaport >= 0)
+ snprintf(group, sizeof(group), "p%02x_cable", dsaport);
connman_network_set_group(network, group);
}
Patrik
laurent vaudoit
2015-10-06 08:11:14 UTC
Permalink
HI,
thank you for your review, i will sned you another patch wit the correction
asked.
Post by Patrik Flykt
Hi,
git am complains of trailing white space errors on lines 47 and 56.
While looking at the patch, all lines end with ^M, please fix.
Post by Laurent Vaudoit
---
plugins/ethernet.c | 60
+++++++++++++++++++++++++++++++++++++++++++++++++++---
Post by Laurent Vaudoit
1 file changed, 57 insertions(+), 3 deletions(-)
diff --git a/plugins/ethernet.c b/plugins/ethernet.c
index d176508..456d3cc 100644
--- a/plugins/ethernet.c
+++ b/plugins/ethernet.c
@@ -32,6 +32,7 @@
#include <linux/if_vlan.h>
#include <linux/sockios.h>
+#include <linux/ethtool.h>
#ifndef IFF_LOWER_UP
#define IFF_LOWER_UP 0x10000
@@ -83,6 +84,54 @@ static int get_vlan_vid(const char *ifname)
return vid;
}
+static int get_dsa_port(const char *ifname)
+{
+ int sk;
+ int dsaport=-1;
Spaces before and after =
Post by Laurent Vaudoit
+ struct ifreq ifr;
+ struct ethtool_cmd cmd;
+ struct ethtool_drvinfo drvinfocmd;
+ struct vlan_ioctl_args vifr;
+
+ sk = socket(AF_INET, SOCK_STREAM, 0);
+ if (sk < 0)
+ return -errno;
+
+ memset(&ifr, 0, sizeof(ifr));
+ strcpy(ifr.ifr_name, ifname);
ifr.ifr_name probably has a max lenght and I don't know if it must end
in a \0. If it does not defined to end with a \0 or max length is
specified, a strncpy should be used instead.
Post by Laurent Vaudoit
+
+ /* check if it is a vlan and get physical interface name*/
+ vifr.cmd = GET_VLAN_REALDEV_NAME_CMD;
+ strncpy(vifr.device1, ifname, sizeof(vifr.device1));
+
+ if(ioctl(sk, SIOCSIFVLAN, &vifr) >= 0)
+ strcpy(ifr.ifr_name, vifr.u.device2);
If this failed, can we return here? Is strcpy safe here for all string
lengths as above?
we do not return her if ioctl fail. In fact, the idea is to check if
interface is a vlan, and if so, get the physical interface on which the
vlan is "plugged", and so check if this interface is a "dsa" one.
Post by Patrik Flykt
Post by Laurent Vaudoit
+
+ /* get driver info */
+ drvinfocmd.cmd = ETHTOOL_GDRVINFO;
+ ifr.ifr_data = (caddr_t)&drvinfocmd;
+
+ /* ioctl failed: */
+ if (ioctl(sk, SIOCETHTOOL, &ifr) != 0)
+ DBG("Cannot get driver infos\n");
Could this be turned around without the debug, as the more likely case
is that this always works, and with the error returned in the return
value. One can later on see that it worked, as the identifier is
different. And the comment is also a bit unnecessary then.
Compare zero with '!'<expression>.
Post by Laurent Vaudoit
+ else {
+ if(strcmp(drvinfocmd.driver,"dsa") == 0) {
+ /* get dsa port*/
+ cmd.cmd = ETHTOOL_GSET;
+ ifr.ifr_data = (caddr_t)&cmd;
+
+ /* ioctl failed: */
+ if (ioctl(sk, SIOCETHTOOL, &ifr) != 0)
+ DBG("Cannot get device settings\n");
Same here.
Post by Laurent Vaudoit
+ else
+ dsaport = cmd.phy_address;
+ }
+ }
+ close(sk);
+
+ return dsaport;
+}
+
static int eth_network_probe(struct connman_network *network)
{
DBG("network %p", network);
@@ -126,7 +175,7 @@ static void add_network(struct connman_device
*device,
Post by Laurent Vaudoit
struct ethernet_data *ethernet)
{
struct connman_network *network;
- int index, vid;
+ int index, vid,dsaport;
Nitpick: space after comma.
Post by Laurent Vaudoit
char *ifname;
network = connman_network_create("carrier",
@@ -140,6 +189,7 @@ static void add_network(struct connman_device
*device,
Post by Laurent Vaudoit
if (!ifname)
return;
vid = get_vlan_vid(ifname);
+ dsaport = get_dsa_port(ifname);
connman_network_set_name(network, "Wired");
@@ -149,14 +199,18 @@ static void add_network(struct connman_device
*device,
Post by Laurent Vaudoit
}
if (!eth_tethering) {
- char group[10] = "cable";
+ char group[16] = "cable";
/*
* Prevent service from starting the reconnect
* procedure as we do not want the DHCP client
* to run when tethering.
*/
- if (vid >= 0)
+ if((vid >= 0) && (dsaport >= 0))
dsaport need only be assigned here, can you move it down here please. I
just noticed that then vid is also misplaced, but don't worry about
that.
Post by Laurent Vaudoit
+ snprintf(group, sizeof(group), "p%02x_%03x_cable",
dsaport, vid);
Post by Laurent Vaudoit
+ else if (vid >= 0)
snprintf(group, sizeof(group), "%03x_cable", vid);
+ else if (dsaport >= 0)
+ snprintf(group, sizeof(group), "p%02x_cable",
dsaport);
Post by Laurent Vaudoit
connman_network_set_group(network, group);
}
Patrik
Laurent
Post by Patrik Flykt
_______________________________________________
connman mailing list
https://lists.connman.net/mailman/listinfo/connman
Loading...