You are right on the xzalloc if fails it aborts the process.
> ovs_assert(index <= VPORT_SOCK_POOL_SIZE);
index = (index == VPORT_SOCK_POOL_SIZE - 1) ? 0 : index + 1;
I think that index should never be greater than ( VPORT_SOCK_POOL_SIZE - 1) if
I understand the logic here.
Thanks,
Eitan
-----Original Message-----
From: Nithin Raju
Sent: Wednesday, October 22, 2014 2:42 PM
To: Eitan Eliahu
Cc: dev@openvswitch.org
Subject: Re: [ovs-dev] [PATCH 5/5] lib/dpif-netlink.c: add support for packet
receive on Windows
> struct dpif_handler {
> struct dpif_channel *channels;/* Array of channels for each handler. */
> struct epoll_event *epoll_events;
> int epoll_fd; /* epoll fd that includes channel socks. */
> int n_events; /* Num events returned by epoll_wait(). */
> int event_offset; /* Offset into 'epoll_events'. */
> +
> +#ifdef _WIN32
> [EITAN]
> Is there any reason why not allocating the memory here for the socket pool
I am taking that you mean that I should declare something like:
struct dpif_windows_vport_sock
dpif_win_vport_sock_pool[VPORT_SOCK_POOL_SIZE];
rather than allocating the array later. I don't have any strong reason as such.
It seemed only cleaner to have a pointer to check for validity later on.
For Eg. in the following code:
struct dpif_windows_vport_sock *sock_pool = handler->
dpif_win_vport_sock_pool;
/* A pool of sockets is allocated when the handler is initialized. */
if (sock_pool == NULL) {
*error = EINVAL;
return NULL;
}
It is easier to check for a pointer rather than check the validity based on the
first socket or something like that.
if (sock_pool[0].nl_sock == NULL) {
}
I feel checking the pointer is more intuitive for checking for validity.
> [EITAN]
> Assert(sock_pool[i].nl_sock ->read_ioctl ==OVS_IOCTL_READ_PACKET)
Definition of 'struct nl_sock' is not available in dpif-netlink.c. I can write
an API for checking the type. FWIW, there's no such check after
nl_sock_join_mcgroup().
> + sock_pool = xzalloc(VPORT_SOCK_POOL_SIZE * sizeof *sock_pool);
> [EITAN]
> If ( !sock_pool) return error;
xzalloc() panics if it fails. No need for explit checking for memory allocation
failures. You can refer to usages of xzalloc().
> + for (i = 0; i < VPORT_SOCK_POOL_SIZE; i++) {
> + error = nl_sock_create(NETLINK_GENERIC, &sock_pool[i].nl_sock);
> + if (error) {
> + goto error;
> + }
> +
> + error = nl_sock_subscribe_packets(sock_pool[i].nl_sock);
> + if (error) {
> [EITAN]
> You want to unsubscribe for all socket index < i
Sure, I can add a check for that.
> [EITAN]
> Check sockp != NULL
> [EITAN]
> + /* Pick netlink sockets to use in a round-robin fashion from each
> + * handler's pool of sockets. */
> + for (i = 0; i < n_socks; i++) {
> + struct dpif_handler *handler = &dpif->handlers[i];
> + struct dpif_windows_vport_sock *sock_pool =
> + handler->dpif_win_vport_sock_pool;
> + size_t index = handler->dpif_win_last_used_vport_sock;
> +
> + /* A pool of sockets is allocated when the handler is initialized. */
> + if (sock_pool == NULL) {
> + *error = EINVAL;
> + return NULL;
> + }
> +
> +
> [EITAN]
> < not <=
Did you mean to say for 'i < n_socks'? We are iterating over the array,
starting at index 0. What do you see wrong?
> ovs_assert(index <= VPORT_SOCK_POOL_SIZE);
> + index = (index == VPORT_SOCK_POOL_SIZE - 1) ? 0 : index + 1;
> +
> [EITAN]
> Remove one line
Sure.
> [EITAN] Can we have the number of iterations (50_ as pre compiler variable
> + if (++read_tries > 50) {
> + return EAGAIN;
> + }
Sure. I can create a #define.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
