HI,
thank you for your review, i will sned you another patch wit the correction
asked.
On Thu, Oct 1, 2015 at 9:42 AM, Patrik Flykt <patrik.fl...@linux.intel.com>
wrote:
>
> 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?
>
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.
>
> > +
> > + /* 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
>
Laurent
>
>
> _______________________________________________
> connman mailing list
> connman@connman.net
> https://lists.connman.net/mailman/listinfo/connman
>
_______________________________________________
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
