On Wed, Mar 19, 2014 at 01:35:06PM -0700, Alex Wang wrote:
> Signed-off-by: Alex Wang <[email protected]>
This is a lot of code, but it looks well crafted. I only have a few
comments.
dpif-linux.c
------------
The comment on dpif_linux_refresh_channels() mentions dpif->channels,
which no longer exists.
It looks like there are always 'n_handlers' of both handlers and
epolls in a given struct dpif_linux. Is there a reason that they're
separate structs? There's a comment that's extra confusing to me
here:
+ int uc_array_size; /* Size of 'handlers' and 'epoll_events'. */
because there appear to be n_handlers handlers, not uc_array_size
handlers.
I think that here at the end of vport_add_channels(), the [i] should
be [j] in the epoll_ctl() call:
for (j = 0; j < i; j++) {
epoll_ctl(dpif->epolls[i].epoll_fd, EPOLL_CTL_DEL,
nl_sock_fd(socksp[j]), NULL);
dpif->handlers[j].channels[port_idx].sock = NULL;
}
dpif-linux.h
------------
I think the comment update here is now wrong because the change to the
name of the enum was dropped a revision or two back:
- const uint32_t *upcall_pid; /* OVS_VPORT_ATTR_UPCALL_PID. */
+ const uint32_t *upcall_pids; /* OVS_VPORT_ATTR_UPCALL_PIDS. */
I would move 'n_pids' next to 'upcall_pids' in struct
dpif_linux_vport, since they are closely related, and probably rename
one or the other (either 'upcall_pids' to just 'pids' or 'n_pids' to
'n_upcall_pids').
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
