[ovs-dev] [PATCH 2/2] netdev-linux: Simplify get_stats_via_netlink().

[Alex Wang]

Thanks for the explanation.  It makes sense.


On Mon, Dec 30, 2013 at 2:04 PM, Ben Pfaff <blp at nicira.com> wrote:

> On Mon, Dec 23, 2013 at 11:49:40AM -0800, Alex Wang wrote:
> > Both patches look good to me,
> >
> > One comment below,
> >
> >
> > > @@ -4432,57 +4432,38 @@ netdev_stats_from_rtnl_link_stats(struct
> > > netdev_stats *dst,
> > >  static int
> > >  get_stats_via_netlink(const struct netdev *netdev_, struct
> netdev_stats
> > > *stats)
> > >  {
> > > -    /* Policy for RTNLGRP_LINK messages.
> > > -     *
> > > -     * There are *many* more fields in these messages, but currently
> we
> > > only
> > > -     * care about these fields. */
> > > -    static const struct nl_policy rtnlgrp_link_policy[] = {
> > > -        [IFLA_IFNAME] = { .type = NL_A_STRING, .optional = false },
> > > -        [IFLA_STATS] = { .type = NL_A_UNSPEC, .optional = true,
> > > -                         .min_len = sizeof(struct rtnl_link_stats) },
> > > -    };
> > > -
> > >      struct ofpbuf request;
> > >      struct ofpbuf *reply;
> > > -    struct ifinfomsg *ifi;
> > > -    struct nlattr *attrs[ARRAY_SIZE(rtnlgrp_link_policy)];
> > > -    int ifindex;
> > >      int error;
> > >
> > > -    error = get_ifindex(netdev_, &ifindex);
> > > -    if (error) {
> > > -        return error;
> > > -    }
> > > -
> > >      ofpbuf_init(&request, 0);
> > > -    nl_msg_put_nlmsghdr(&request, sizeof *ifi, RTM_GETLINK,
> > > NLM_F_REQUEST);
> > > -    ifi = ofpbuf_put_zeros(&request, sizeof *ifi);
> > > -    ifi->ifi_family = PF_UNSPEC;
> > > -    ifi->ifi_index = ifindex;
> > > +    nl_msg_put_nlmsghdr(&request,
> > > +                        sizeof(struct ifinfomsg) +
> NL_ATTR_SIZE(IFNAMSIZ),
> > > +                        RTM_GETLINK, NLM_F_REQUEST);
> > > +    ofpbuf_put_zeros(&request, sizeof(struct ifinfomsg));
> > > +    nl_msg_put_string(&request, IFLA_IFNAME,
> netdev_get_name(netdev_));
> > >
> >
> >
> > Why do we do "sizeof(struct ifinfomsg) + NL_ATTR_SIZE(IFNAMSIZ)"
> > instead of "sizeof(struct ifinfomsg)"  or  "sizeof(struct ifinfomsg) +
> > sizeof(struct nlattr) + NL_ATTR_SIZE(IFNAMSIZ) " ?
>
> NL_ATTR_SIZE includes the size of a struct nlattr:
>     #define NL_ATTR_SIZE(PAYLOAD_SIZE) (NLA_HDRLEN +
> NLA_ALIGN(PAYLOAD_SIZE))
>
> > The "nl_msg_put_string(&request, IFLA_IFNAME, netdev_get_name(netdev_));"
> > will realloc the "reqest", since we also need to put "struct nlattr".
>
> I think that space is included.
>
> Thanks for the review!  I'm going to push this in a minute.  (If I'm
> wrong about the size calculation, we can fix that later.)
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20131230/104974b4/attachment-0003.html>