Converted to plain text. Hopefully this will make my inline comments more apparent.
> > >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. https://urldefense.proofpoint.com/v1/url?u=http://msdn.microsoft.com/en-us/ library/windows/desktop/aa365247%28v%3Dvs.85%29.aspx&k=oIvRg1%2BdGAgOoM1BIl LLqw%3D%3D%0A&r=pEkjsHfytvHEWufeZPpgqSOJMdMjuZPbesVsNhCUc0E%3D%0A&m=Mejrvb4 cg1ENUojHe5BEPVM2jvpg2Q04dvlbVfxBDd0%3D%0A&s=54744436b287bcaf52c3a031d3685d 5d9288fb19f00fb20c7e3dbab78eb49e27 > > > >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 >https://urldefense.proofpoint.com/v1/url?u=http://openvswitch.org/mailman/ >listinfo/dev&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=pEkjsHfytvHEWufeZPpgqSO >JMdMjuZPbesVsNhCUc0E%3D%0A&m=Mejrvb4cg1ENUojHe5BEPVM2jvpg2Q04dvlbVfxBDd0%3 >D%0A&s=e28be9ce77e506cf8746e27a4f36df2d7b1e55ea6fca6ec02615b08b89928404 _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
