From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?utf-8?Q?Nicol=C3=A1s_Pernas_Maradei?= Subject: Re: [PATCH 1/3] pcap: utilize underlying real interface properties Date: Wed, 29 Apr 2015 01:30:19 +0100 Message-ID: Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable To: dev-VfR2kkLFssw@public.gmane.org, tero.aho-jSzUOxOKT6dBDgjK7y7TUQ@public.gmane.org Return-path: Content-Disposition: inline List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces-VfR2kkLFssw@public.gmane.org Sender: "dev" Hi Tero, Just a few comments on one of your patches - see inline comments below. I= nteresting features btw. Nico. --=C2=A0 Nicol=C3=A1s Pernas Maradei On 27 =46ebruary 2015 at 13:43:14, dev-request=40dpdk.org (dev-request=40= dpdk.org) wrote: Message: 5=C2=A0 Date: =46ri, 27 =46eb 2015 15:42:38 +0200=C2=A0 =46rom: Tero Aho =C2=A0 To: =C2=A0 Subject: =5Bdpdk-dev=5D =5BPATCH 1/3=5D pcap: utilize underlying real=C2=A0= interface properties=C2=A0 Message-ID: <1425044560-23397-2-git-send-email-tero.aho=40coriant.com>=C2= =A0 Content-Type: text/plain=C2=A0 These changes set pcap interface mac address to the real underlying=C2=A0= interface address instead of the default one. Also real interface link=C2= =A0 status, speed and duplex are reported when eth=5Flink=5Fupdate is called=C2= =A0 for the pcap interface.=C2=A0 Signed-off-by: Tero Aho =C2=A0 ---=C2=A0 lib/librte=5Fpmd=5Fpcap/rte=5Feth=5Fpcap.c =7C 51 +++++++++++++++++++++++= ++++++++++++---=C2=A0 1 file changed, 47 insertions(+), 4 deletions(-)=C2=A0 diff --git a/lib/librte=5Fpmd=5Fpcap/rte=5Feth=5Fpcap.c b/lib/librte=5Fpm= d=5Fpcap/rte=5Feth=5Fpcap.c=C2=A0 index 5e94930..289af28 100644=C2=A0 --- a/lib/librte=5Fpmd=5Fpcap/rte=5Feth=5Fpcap.c=C2=A0 +++ b/lib/librte=5Fpmd=5Fpcap/rte=5Feth=5Fpcap.c=C2=A0 =40=40 -43,6 +43,11 =40=40=C2=A0 =23include =C2=A0 =23include =C2=A0 +=23include =C2=A0 +=23include =C2=A0 +=23include =C2=A0 +=23include =C2=A0 +=23include =C2=A0 =23include =C2=A0 =40=40 -102,6 +107,8 =40=40 struct pmd=5Finternals =7B=C2=A0 unsigned nb=5Ftx=5Fqueues;=C2=A0 int if=5Findex;=C2=A0 int single=5Fiface;=C2=A0 + const char *if=5Fname;=C2=A0 + int if=5Ffd;=C2=A0 =7D;=C2=A0 const char *valid=5Farguments=5B=5D =3D =7B=C2=A0 =40=40 -451,6 +458,26 =40=40 static int=C2=A0 eth=5Flink=5Fupdate(struct rte=5Feth=5Fdev *dev =5F=5Frte=5Funused,=C2=A0= *dev is being used. Remove =5F=5Frte=5Funused int wait=5Fto=5Fcomplete =5F=5Frte=5Funused)=C2=A0 =7B=C2=A0 + struct ifreq ifr;=C2=A0 + struct ethtool=5Fcmd cmd;=C2=A0 + struct pmd=5Finternals *internals =3D dev->data->dev=5Fprivate;=C2=A0 +=C2=A0 + if (internals->if=5Fname && (internals->if=5Ffd =21=3D -1)) =7B=C2=A0 + /* get link status, speed and duplex from the underlying interface */=C2= =A0 +=C2=A0 + strncpy(ifr.ifr=5Fname, internals->if=5Fname, sizeof(ifr.ifr=5Fname)-1)= ;=C2=A0 + ifr.ifr=5Fname=5Bsizeof(ifr.ifr=5Fname)-1=5D =3D 0;=C2=A0 Use snprintf(ifr.ifr=5Fname, sizeof(ifr.ifr=5Fname), =E2=80=9C%s=E2=80=9D= , internals->if=5Fname) instead. It=E2=80=99s safer and cleaner. + if (=21ioctl(internals->if=5Ffd, SIOCGI=46=46LAGS, &ifr))=C2=A0 + dev->data->dev=5Flink.link=5Fstatus =3D (ifr.ifr=5Fflags & I=46=46=5FUP= ) =3F 1 : 0;=C2=A0 +=C2=A0 + cmd.cmd =3D ETHTOOL=5FGSET;=C2=A0 + ifr.ifr=5Fdata =3D (void *)&cmd;=C2=A0 + if (=21ioctl(internals->if=5Ffd, SIOCETHTOOL, &ifr)) =7B=C2=A0 + dev->data->dev=5Flink.link=5Fspeed =3D ethtool=5Fcmd=5Fspeed(&cmd);=C2=A0= + dev->data->dev=5Flink.link=5Fduplex =3D=C2=A0 + cmd.duplex =3F ETH=5FLINK=5F=46ULL=5FDUPLEX : ETH=5FLINK=5FHAL=46=5FDUP= LEX;=C2=A0 + =7D=C2=A0 + =7D=C2=A0 return 0;=C2=A0 =7D=C2=A0 =40=40 -736,11 +763,24 =40=40 rte=5Fpmd=5Finit=5Finternals(const char *na= me, const unsigned nb=5Frx=5Fqueues,=C2=A0 (*internals)->nb=5Frx=5Fqueues =3D nb=5Frx=5Fqueues;=C2=A0 (*internals)->nb=5Ftx=5Fqueues =3D nb=5Ftx=5Fqueues;=C2=A0 - if (pair =3D=3D NULL)=C2=A0 + if (pair =3D=3D NULL) =7B=C2=A0 (*internals)->if=5Findex =3D 0;=C2=A0 - else=C2=A0 + =7D else =7B=C2=A0 + /* use real inteface mac addr, save name and fd for eth=5Flink=5Fupdate= */=C2=A0 (*internals)->if=5Findex =3D if=5Fnametoindex(pair->value);=C2=A0 -=C2=A0 + (*internals)->if=5Fname =3D strdup(pair->value);=C2=A0 + (*internals)->if=5Ffd =3D socket(A=46=5FINET, SOCK=5FDGRAM, 0);=C2=A0 I see you are using a socket and ioctl calls to get the info you need fro= m the interface. I=E2=80=99m not a big fan of opening a socket at this po= int just to get some parameters of the NIC. I=E2=80=99d rather reading th= ose from sysfs. Is there a reason why you=E2=80=99d prefer to open a sock= et=3F These would be the files you=E2=80=99d need to open and read to the get t= he info you are looking for.=C2=A0 =23 cat /sys/class/net/eth0/address =23 cat /sys/class/net/eth0/duplex =23 cat /sys/class/net/eth0/speed In my opinion the code would be cleaner doing it this way. DPDK already m= anipulates=C2=A0sysfs in other places too.=C2=A0 What do you think=3F + if ((*internals)->if=5Ffd =21=3D -1) =7B=C2=A0 + struct ifreq ifr;=C2=A0 + strncpy(ifr.ifr=5Fname, pair->value, sizeof(ifr.ifr=5Fname)-1);=C2=A0 + ifr.ifr=5Fname=5Bsizeof(ifr.ifr=5Fname)-1=5D =3D 0;=C2=A0 Use snprintf() like before. + if (=21ioctl((*internals)->if=5Ffd, SIOCGI=46HWADDR, &ifr)) =7B=C2=A0 + data->mac=5Faddrs =3D rte=5Fzmalloc=5Fsocket(NULL, ETHER=5FADDR=5FLEN, = 0, numa=5Fnode);=C2=A0 + if (data->mac=5Faddrs)=C2=A0 + rte=5Fmemcpy(data->mac=5Faddrs, ifr.ifr=5Faddr.sa=5Fdata, ETHER=5FADDR=5F= LEN);=C2=A0 + =7D=C2=A0 + =7D=C2=A0 + =7D=C2=A0 pci=5Fdev->numa=5Fnode =3D numa=5Fnode;=C2=A0 data->dev=5Fprivate =3D *internals;=C2=A0 =40=40 -749,7 +789,8 =40=40 rte=5Fpmd=5Finit=5Finternals(const char *name= , const unsigned nb=5Frx=5Fqueues,=C2=A0 data->nb=5Frx=5Fqueues =3D (uint16=5Ft)nb=5Frx=5Fqueues;=C2=A0 data->nb=5Ftx=5Fqueues =3D (uint16=5Ft)nb=5Ftx=5Fqueues;=C2=A0 data->dev=5Flink =3D pmd=5Flink;=C2=A0 - data->mac=5Faddrs =3D ð=5Faddr;=C2=A0 + if (data->mac=5Faddrs =3D=3D NULL)=C2=A0 + data->mac=5Faddrs =3D ð=5Faddr;=C2=A0 strncpy(data->name,=C2=A0 (*eth=5Fdev)->data->name, strlen((*eth=5Fdev)->data->name));=C2=A0 =40=40 -758,6 +799,8 =40=40 rte=5Fpmd=5Finit=5Finternals(const char *name= , const unsigned nb=5Frx=5Fqueues,=C2=A0 (*eth=5Fdev)->pci=5Fdev =3D pci=5Fdev;=C2=A0 (*eth=5Fdev)->driver =3D &rte=5Fpcap=5Fpmd;=C2=A0 + eth=5Flink=5Fupdate((*eth=5Fdev), 0);=C2=A0 +=C2=A0 return 0;=C2=A0 error: if (data)=C2=A0 --=C2=A0 1.9.1=C2=A0 =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=C2=A0 The information contained in this message may be privileged=C2=A0 and confidential and protected from disclosure. If the reader=C2=A0 of this message is not the intended recipient, or an employee=C2=A0 or agent responsible for delivering this message to the=C2=A0 intended recipient, you are hereby notified that any reproduction,=C2=A0 dissemination or distribution of this communication is strictly=C2=A0 prohibited. If you have received this communication in error,=C2=A0 please notify us immediately by replying to the message and=C2=A0 deleting it from your computer. Thank you. Coriant-Tellabs=C2=A0 =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=C2=A0 ------------------------------=C2=A0 Subject: Digest =46ooter=C2=A0 =5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F= =5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=5F=C2=A0 dev mailing list=C2=A0 dev=40dpdk.org=C2=A0 http://dpdk.org/ml/listinfo/dev=C2=A0 ------------------------------=C2=A0 End of dev Digest, Vol 29, Issue 99=C2=A0 ***********************************=C2=A0