Thanks, both applied.
On Mon, Dec 30, 2013 at 02:07:21PM -0800, Alex Wang wrote: > Thanks for the explanation. It makes sense. > > > On Mon, Dec 30, 2013 at 2:04 PM, Ben Pfaff <[email protected]> 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.) > > _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
