[ovs-dev] [PATCH 3/4] dpif-windows: Implement datapath interface for windows.

[Saurabh Shah]

Converted to plain text. Hopefully this will make my inline comments more
apparent. 

>
>
>From: Ben Pfaff <blp at nicira.com<mailto:blp at nicira.com>>
>Date: Wednesday, June 25, 2014 at 4:54 PM
>To: Saurabh Shah <ssaurabh at vmware.com<mailto:ssaurabh at vmware.com>>
>Cc: "dev at openvswitch.org<mailto:dev at openvswitch.org>"
><dev at openvswitch.org<mailto:dev at openvswitch.org>>, Guolin Yang
><gyang at vmware.com<mailto:gyang at 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
><eliahue at vmware.com<mailto:eliahue at vmware.com>>
>Signed-off-by: Eitan Eliahu
><eliahue at vmware.com<mailto:eliahue at vmware.com>>
>Co-Authored-By: Guolin Yang <gyang at vmware.com<mailto:gyang at vmware.com>>
>Signed-off-by: Guolin Yang <gyang at vmware.com<mailto:gyang at vmware.com>>
>Co-Authored-By: Linda Sun <lsun at vmware.com<mailto:lsun at vmware.com>>
>Signed-off-by: Linda Sun <lsun at vmware.com<mailto:lsun at vmware.com>>
>Co-Authored-By: Nithin Raju <nithin at vmware.com<mailto:nithin at vmware.com>>
>Signed-off-by: Nithin Raju <nithin at vmware.com<mailto:nithin at vmware.com>>
>Signed-off-by: Saurabh Shah
><ssaurabh at vmware.com<mailto:ssaurabh at 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 at 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