Thanks for the review Ben,
> 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
>
Fixed,
> 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.
>
Thanks for the good suggestion, I'll adjust accordingly.
>
> 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;
> }
>
Fixed,
> 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').
>
I'll change the 'n_pids' to 'n_upcall_pids'. And move it on to of the
'upcall_pids'
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
