On Thu, Feb 28, 2019 at 07:36:29AM -0800, Richard Cochran wrote:
> On Thu, Feb 28, 2019 at 09:22:22PM +0800, Hangbin Liu wrote:
> > v3: a) Do not make rtnl_buf global as it may be freed by calling
> > rtnl_close()
> > while we are using it in rtnl_link_status()
>
> This fix is wrong. You added a call to rtnl_close() for (AFAICT) no
> good reason. That is the bug.
Cause there is a genl_open() in get_team_active_iface().
>
>
> > int rtnl_link_status(int fd, char *device, rtnl_callback cb, void *ctx)
> > {
>
> > index = if_nametoindex(device);
> > +
> > + rtnl_buf = malloc(BUF_SIZE);
> > if (!rtnl_buf) {
> > - rtnl_len = 4096;
> > - rtnl_buf = malloc(rtnl_len);
> > - if (!rtnl_buf) {
> > - pr_err("rtnl: low memory");
> > - return -1;
>
> The original code avoids calling malloc on every single invocation of
> rtnl_link_status(), but you have removed that. This needs some kind
> of justification.
Oh, I got what you mean. OK, how about just close the fd directly in
get_team_active_iface() since we didn't use rtnl_buf there. Then I
can move this check back.
I will post a new patch for you reviewing.
Thanks
Hangbin
>
> > - }
> > + pr_err("rtnl: low memory");
> > + goto out;
> > }
>
> > +static int get_team_active_iface(int master_index)
> > +{
> > + struct rtattr *tb[TEAM_ATTR_MAX+1];
> > + struct genlmsghdr *gnlh;
> > + struct nlmsghdr *nlh;
> > + char msg[BUF_SIZE];
> > + int fd, gf_id;
> > + int len = -1;
> ...
>
> > +no_info:
> > + rtnl_close(fd);
> > + return len;
> > +}
>
> Here len is -1, as an error flag. Why simply propagate that error
> correct up the call stack?
>
> No need for rtnl_close() here.
>
> Thanks,
> Richard
>
_______________________________________________
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
