> -----Original Message-----
> From: Hangbin Liu [mailto:[email protected]]
> Sent: Tuesday, July 18, 2017 8:08 PM
> To: Keller, Jacob E <[email protected]>
> Cc: [email protected]; Sushil Kulkarni
> <[email protected]>; Jiri Benc <[email protected]>
> Subject: Re: [Linuxptp-devel] [PATCHv2 4/9] rtnl: add function rtnl_link_info
>
> Hi Keller,
>
> On Tue, Jul 18, 2017 at 08:40:59PM +0000, Keller, Jacob E wrote:
> > > +int rtnl_link_info(struct interface *iface)
> > > +{
> > > + int fd, index;
> > > +
> > > + index = if_nametoindex(iface->name);
> > > +
> > > + if (index == 0) {
> > > + pr_err("failed to get interface %s index: %m", iface->name);
> > > + goto no_fd;
> > > + }
> > > +
> > > + fd = rtnl_open();
> > > + if (fd < 0)
> > > + goto no_fd;
> > > +
> > > + if (rtnl_link_query(fd, index))
> > > + goto no_info;
> > > + if (rtnl_link_status(fd, NULL, iface->ts_iface))
> > > + goto no_info;
> > > +
> > > + /* If we do not have a slave, then use our own interface name
> > > + * as ts_iface
> > > + */
> > > + if (iface->ts_iface[0] == '\0')
> > > + strncpy(iface->ts_iface, iface->name, MAX_IFNAME_SIZE);
> > > +
> >
> > Seems like we're duplicating code here just to maintain a return value?
> >
> > > + rtnl_close(fd);
> > > + return 0;
> > > +
> > > +no_info:
> > > + rtnl_close(fd);
> > > +no_fd:
> > > + if (iface->ts_iface[0] == '\0')
> > > + strncpy(iface->ts_iface, iface->name, MAX_IFNAME_SIZE);
> > > +
> > > + return -1;
> > > +}
>
> hmm, you are right, the code is a little clumsy, how about
>
> fd = rtnl_open();
>
> if (fd > 0 && ! rtnl_link_query(fd, index))
> rtnl_link_status(fd, NULL, iface->ts_iface);
>
> /* If we do not have a slave, then use our own interface name
> * as ts_iface
> */
> if (iface->ts_iface[0] == '\0')
> strncpy(iface->ts_iface, iface->name, MAX_IFNAME_SIZE);
>
> if (fd > 0) {
> rtnl_close(fd);
> return 0;
> } else {
> return -1;
> }
>
I think the gotos are still fine, if you order things correctly, and track an
error value:
int err = 0;
fd = rtnl_open();
if (fd < 0) {
err = -1;
goto no_fd;
}
if (rtnl_link_query(fd, index))
err = -1;
else if (rtnl_link_status(fd, NULL, iface->ts_iface))
err = -1;
rtnl_close(fd);
no_fd:
if (iface->ts_iface[0] == '\0')
strncpy(iface->ts_iface, iface->name, MAX_IFNAME_SIZE);
return err;
IMO this is the most clear and avoids the duplication of the checks.
Thanks,
Jake
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Linuxptp-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
