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
