On Mon, Jun 23, 2014 at 05:34:58PM -0700, Saurabh Shah wrote:
> Co-Authored-By: Eitan Eliahu <elia...@vmware.com>
> Signed-off-by: Eitan Eliahu <elia...@vmware.com>
> Co-Authored-By: Guolin Yang <gy...@vmware.com>
> Signed-off-by: Guolin Yang <gy...@vmware.com>
> Co-Authored-By: Linda Sun <l...@vmware.com>
> Signed-off-by: Linda Sun <l...@vmware.com>
> Co-Authored-By: Nithin Raju <nit...@vmware.com>
> Signed-off-by: Nithin Raju <nit...@vmware.com>
> Signed-off-by: Saurabh Shah <ssaur...@vmware.com>
This is going to be a pretty superficial review, because I don't know
Windows well. It's just what I notice as I try to read through the
code. I haven't compiled it other than to make sure it doesn't break
the GNU/Linux build.
It looks like datapath-windows/include/OvsNetlink.h is cut-and-pasted
from other OVS header files, especially lib/netlink-protocol.h,
lib/netlink.h, and lib/util.h. Is it possible to reduce the amount of
code duplication? Either way, the copyright notice in that file
should include the original copyright notice years and authors, e.g.
* Copyright (c) 2008, 2009, 2010, 2011, 2013 Nicira, Inc.
for lib/netlink.h, instead of just saying VMware 2014.
dpif-windows.c
--------------
A lot of functions shifted around a bit when I asked Emacs to
re-indent them. Some code seems to randomly use 3-space indents.
This file should #include "dpif-windows.h" just after <config.h>, to
ensure that dpif-windows.h is self-contained (it looks to me like it
isn't, I don't see any #includes in there that would define enum
ovs_vport_type, for example).
Some global variables should probably be static:
+char *systemType = "system";
+long long int ovs_dp_timestamp_set_time;
Please use BUILD_ASSERT(_DECL) from util.h instead of defining
ASSERT_ON_COMPILE.
Please use the standard offsetof from <stddef.h> instead of defining
OFFSET_OF.
Is win_error_to_errno() likely to be more broadly useful, e.g. should
it be moved to a more global location? (If not, it should be static.)
This file uses // comments more than OVS generally does.
Lots of log messages log raw error numbers, e.g. in
dpif_windows_init_handle():
error = GetLastError();
VLOG_ERR("Failed driver version query: %ws error %d\n",
OVS_DEVICE_PATH, error);
I guess you can use ovs_last error_to_string() or ovs_format_message()
to give the user better diagnostics.
Comments should generally begin with a capital letter and end with a
period, e.g.
/* handle specifically used for receiving packets from the kernel */
->
/* Handle specifically used for receiving packets from the kernel. */
I see a number of lines that would be more readable if broken at 79
characters.
The use of __FUNCTION__ in logging in dpif_windows_init_handle()
doesn't look too useful to me, since the log message is pretty much
unique without it.
I suspect that none of the casts in dpif_windows_ioctl__() are
actually needed.
Also in dpif_windows_ioctl__():
* In order to deal with the situation, we periodicatlly pass down the
"periodically"
This is a nasty situation. Do you have any leads on why the idea of
the current time might drift?
/*
* There seems to be a skew between the kernel's version of current time and
* the userspace's version of current time. The skew was seen to
* monotonically increase as well.
*
* In order to deal with the situation, we periodicatlly pass down the
* userspace's version of the timestamp to the kernel, and let the kernel
* calculate the delta. The frequency of this is
* OVS_DP_TIMESTAMP_SET_TIME_INTVL.
*/
Also that comment should line up the *s:
/*
* There seems to be a skew between the kernel's version of current time and
* the userspace's version of current time. The skew was seen to
* monotonically increase as well.
*
* In order to deal with the situation, we periodicatlly pass down the
* userspace's version of the timestamp to the kernel, and let the kernel
* calculate the delta. The frequency of this is
* OVS_DP_TIMESTAMP_SET_TIME_INTVL.
*/
Usually we'd write OVS_NOT_REACHED() here:
ovs_assert(FALSE);
Stylistically it's rare to see a bare {} block in OVS like the one in
the middle of dpif_windows_ioctl__(). Historically we've tended to
declare variables at the top of the nearest block. Now the style
allows declarations mid-block. I think I'd prefer to see either of
those latter two choices here.
This:
if (!DeviceIoControl(handle, type,
(LPVOID)request, (DWORD)request_len,
(LPVOID)reply, (DWORD)reply_len,
&bytes, NULL)) {
should be lined up like this:
if (!DeviceIoControl(handle, type,
(LPVOID)request, (DWORD)request_len,
(LPVOID)reply, (DWORD)reply_len,
&bytes, NULL)) {
dpif_windows_ioctl__() seems to return a negative errno in some cases,
but a positive errno if dpif_windows_init() fails. I think that the
latter is probably a mistake, because some callers treat a positive
return value as success (e.g. dpif_windows_dump_numbers()).
In dpif_windows_ioctl(), this:
return dpif_windows_ioctl__(ovs_ctl_device, type, request, request_len,
reply, reply_len);
should be lined up like this:
return dpif_windows_ioctl__(ovs_ctl_device, type, request, request_len,
reply, reply_len);
In dpif_windows_init_handle(), FILE_READ_ATTRIBUTES | SYNCHRONIZE is
commented out in the call to CreateFile. Should these be included?
I guess that this comment in dpif_windows_close() should be deleted:
// close(dpif->chrdev_fd);
In dpif_windows_get_stats(), we don't usually put parentheses around
the operand to sizeof when they are not required, like here:
memset(stats, 0, sizeof(*stats));
In dpif_windows_port_query__(), we don't normally add parentheses
around a "return" expression:
return (-retval == ENOENT ? ENODEV : -retval);
and the middle line is not indented correctly here:
if (port_name && strncmp(port_name, info.name, strlen(port_name))) {
return ENODEV;
}
On that same line of code
if (port_name && strncmp(port_name, info.name, strlen(port_name))) {
I'm not sure why it uses strncmp(). strncmp() seems to risk accepting
a port name that is the requested name plus some trailing prefix (or
maybe I've got that backward). Maybe info.name isn't necessarily
null-terminated? In that case maybe one should check that
strnlen(info.name, sizeof info.name) == strlen(port_name).
dpif_windows_port_add() declares 'type' as char * instead of const
char * and then needs a cast to char *. Why not just use const char
*?
This comment in dpif_windows_port_add() is mysterious:
/*
* we may add uplink support later
* when we move away from DVS.
*/
dpif_windows_port_del() marks its parameters OVS_UNUSED but
does use them.
In dpif_windows_port_get_pid(), I would write
return (port_no != UINT32_MAX) ? port_no : 0;
as
return port_no != UINT32_MAX ? port_no : 0;
The 'event_status' variable declared at file scope should probably be
static. It could even be moved inside dpif_windows_poll_datapath()
since it appears to be used only inside that function.
In dpif_windows_port_poll(), I would write OVS_NOT_REACHED() instead
of
/* NOT REACHED */
if anything is really needed at all.
odp_perfect_flow_key_to_flow() is cut and paste from
dpif_netdev_flow_from_nlattrs(). I would break out the common code
into odp-util.c.
The logging in dpif_windows_flow_del() seems like a debugging stray.
FLOW_DUMP_SIZE is only used in dpif_windows_flow_dump_thread_create()
so I would probably use an enum there:
enum { FLOW_DUMP_SIZE = 4096 };
to keep it nicely scoped.
dpif_windows_ovs_flow_key_to_flow() should probably memset 'dst' to
all zeros to start out with, to make sure that padding between and
after members is zeroed. Then you can omit the other memsets and
assignments to 0 within the function. Oh, actually I see that its
only caller does the memset. I would move the memset into
dpif_windows_ovs_flow_key_to_flow() to make it clearer.
odp_flow_key_from_flow(&fstate->key_buf, &flow, NULL,
flow.in_port.odp_port, false);
should be indented as:
odp_flow_key_from_flow(&fstate->key_buf, &flow, NULL,
flow.in_port.odp_port, false);
It's pretty unusual for dpif_windows_execute() to call malloc()
instead of xmalloc(). Any particular reason?
I'm not sure why dpif_windows_execute() has a cast to struct
dpif_execute *. Why isn't that parameter to
dpif_execute_to_packetexecute() const?
In dpif_windows_subscribe(), I would write
return (retval < 0)? -errno : 0;
as
return retval < 0 ? -errno : 0;
I suspect that the references to erno in dpif_windows_subscribe()
should be to retval.
jhash.[ch]
----------
I'm not really happy with these changes, let's see if there's a better
way.
I don't understand why this adds a cast to jhash_bytes().
OvsPub.h
--------
The quoted string here appears to have two \\ escapes followed by a
\. escape. Does \. mean something special in Windows?
#define OVS_DEVICE_PATH TEXT("\\\\\.\\OvsIoctl")
I succumbed to "review fatigue" looking at the netdev code, so no
comments on that yet. Tomorrow, I'll try to continue my look through,
and then I'll shift over to looking at the cloudbase implementation.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
