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.
On Mon, 2015-09-28 at 14:48 +0200, Laurent Vaudoit wrote:
> ---
> 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 =
> + 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.
> +
> + /* 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?
> +
> + /* 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>.
> + 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.
> + 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.
> 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.
> + 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
_______________________________________________
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
