From: Ben Pfaff <b...@nicira.com<mailto:b...@nicira.com>> Date: Wednesday, June 25, 2014 at 4:54 PM To: Saurabh Shah <ssaur...@vmware.com<mailto:ssaur...@vmware.com>> Cc: "dev@openvswitch.org<mailto:dev@openvswitch.org>" <dev@openvswitch.org<mailto:dev@openvswitch.org>>, Guolin Yang <gy...@vmware.com<mailto:gy...@vmware.com>> Subject: Re: [ovs-dev] [PATCH 3/4] dpif-windows: Implement datapath interface for windows.
On Mon, Jun 23, 2014 at 05:34:58PM -0700, Saurabh Shah wrote: Co-Authored-By: Eitan Eliahu <elia...@vmware.com<mailto:elia...@vmware.com>> Signed-off-by: Eitan Eliahu <elia...@vmware.com<mailto:elia...@vmware.com>> Co-Authored-By: Guolin Yang <gy...@vmware.com<mailto:gy...@vmware.com>> Signed-off-by: Guolin Yang <gy...@vmware.com<mailto:gy...@vmware.com>> Co-Authored-By: Linda Sun <l...@vmware.com<mailto:l...@vmware.com>> Signed-off-by: Linda Sun <l...@vmware.com<mailto:l...@vmware.com>> Co-Authored-By: Nithin Raju <nit...@vmware.com<mailto:nit...@vmware.com>> Signed-off-by: Nithin Raju <nit...@vmware.com<mailto:nit...@vmware.com>> Signed-off-by: Saurabh Shah <ssaur...@vmware.com<mailto: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. Thanks for the review, Ben. 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. Yes, I have made an attempt to reduce some of the duplication with lib/netlink-protocol.h, let me know if that is satisfactory. 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. There is probably a smarter way of doing this with VIM, but I just eyeballed the entire file and fixed the indentation. Let me know if I missed anything. 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). Fixed. Some global variables should probably be static: +char *systemType = "system"; +long long int ovs_dp_timestamp_set_time; Fixed. Please use BUILD_ASSERT(_DECL) from util.h instead of defining ASSERT_ON_COMPILE. Fixed. Please use the standard offsetof from <stddef.h> instead of defining OFFSET_OF. Fixed. 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.) It might be useful. Moving it to util.h/c. This file uses // comments more than OVS generally does. Converted to /**/ 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. Ok, done. 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. */ Fixed. I see a number of lines that would be more readable if broken at 79 characters. Fixed. 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. Fixed. I suspect that none of the casts in dpif_windows_ioctl__() are actually needed. Ok, fixed. Also in dpif_windows_ioctl__(): * In order to deal with the situation, we periodicatlly pass down the "periodically" Fixed. 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. */ No, this needs to be further investigated. 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. */ Fixed. Usually we'd write OVS_NOT_REACHED() here: ovs_assert(FALSE); Done. 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. Done. 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)) { Fixed. 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()). Fixed. 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); Fixed. 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); Deleted both comments. 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)); Fixed. In dpif_windows_port_query__(), we don't normally add parentheses around a "return" expression: return (-retval == ENOENT ? ENODEV : -retval); Fixed. and the middle line is not indented correctly here: if (port_name && strncmp(port_name, info.name, strlen(port_name))) { return ENODEV; } Fixed. 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). We store the length of the vport name in kernel, so I am not sure it guarantees null termination. 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 *? Fixed. This comment in dpif_windows_port_add() is mysterious: /* * we may add uplink support later * when we move away from DVS. */ Deleted. dpif_windows_port_del() marks its parameters OVS_UNUSED but does use them. Fixed. 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; Ok, done. 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. Done. In dpif_windows_port_poll(), I would write OVS_NOT_REACHED() instead of /* NOT REACHED */ if anything is really needed at all. Fixed. 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. Do you mean in do_put? 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. Done. 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. Done. 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); Done. It's pretty unusual for dpif_windows_execute() to call malloc() instead of xmalloc(). Any particular reason? Probably an oversight, fixed. 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? Fixed. 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. Fixed, thanks! 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(). I will copy out the code to windows kernel. 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") Yes, to specify the device namespace instead of the file namespace. http://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).aspx 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. Thanks for the detailed review! Much appreciated. Saurabh _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
