[ovs-dev] review of Cloudbase port of OVS userspace (was: Re: Cloudbase - OVS Hyper-V porting availability)

Ben Pfaff blp at nicira.com
Thu Jun 26 22:41:54 UTC 2014


On Thu, Jun 26, 2014 at 09:50:39AM +0000, Alessandro Pilotti wrote:
> The user space code that you are seeing is based on our port (the one
> we submitted some months ago to this ML, before Guru's Windows
> patching work started on master), that's why you see all those
> commits.

OK.

> The good news is that thanks to the way we approached the Windows
> Netlink replacement, rebasing the porting on the current master tip
> will be very fast, max one week as written in a previous email,
> resulting in a way smaller diff since master contains already other
> patches for Windows.

I look forward to the rebased version.

> Before getting there, do you think it could be possible to agree on at
> least some of the general design ideas of the porting by looking at
> both the Cloudbase and VMWare efforts? The user space integration
> (netlink replacement) in particular is one area where the two
> approaches differ quite a bit, please see the comparison that I sent
> earlier to this ML [1].
>
> I'd be happy to participate in a IRC chat or hangout to discuss the
> general Windows kernel port design principles and implementation
> details.

I read a lot of the VMware code yesterday afternoon (I hadn't seen it
before), this morning I looked at the Cloudbase userspace repo, and
later today I'll try to read the Cloudbase kernel repo.  After that I
guess I'll understand the issues better and I'm happy to talk them
through.  I generally prefer IRC, if that works for you.  The
#openvswitch channel on freenode is probably suitable.

Looking at the userspace repo, this is a really big set of changes, and
I can see that Cloudbase developers put tons of effort into them.  I
guess that it will take significant integration work to make it all fit
properly into the upstream codebase.  I expect that most of that work
consists of properly documenting the code, its origin, licensing, and
rationale.  That's going to take some additional effort but I guess that
it pales in comparison to the effort already invested to this point.
Anyway, I'm excited to be getting another operating system working
upstream.
  
Here are the integration issues that I noticed while looking through the
Cloudbase repository.  These rules apply to every submission, and many
of them are written down in CONTRIBUTING.

1. Patches must be based on the tip of master, or close to it.  A week
   or so back is fine, as long as there are no significant conflicts.

   In the Cloudbase repository, I see commits that replace files in
   the repository by snapshots from newer upstreams.  This is not
   acceptable because it is impossible to reasonably merge them.
   Example commits: "Changes to bring up to date vswitchd", "Change
   includes and update with new struct files", "Rebase of
   ofproto. Unfortunately ended up in a lot of file changes due to
   incompatibility between structs"

   Adding commits like this is not a "rebase".  To "rebase" is to
   start changes from a newer upstream commit.  With Git, that's
   usually done with a command like "git rebase origin/master".  (That
   is probably not going to work smoothly with the commits I see in
   the Cloudbase tree now, because it will run into tons of conflicts.
   I would probably start by squashing all of the Cloudbase commits
   into a single one, then dropping the bits that don't make sense
   anymore.)

   I see that the Cloudbase commits reintroduce many files that had
   already been deleted at the point of branching off,
   e.g. lib/leak-checker.[ch], lib/stress.[ch], lib/worker.[ch],
   ovsdb/SPECS, utilities/ovs-controller.c.  This is probably an
   artifact of mixing "git rebase" with adding commits as above.

2. Each patch must make logical sense.

   Many of the Cloudbase commits look like temporary commits written
   during debugging.  That is fine as part of debugging (I write tons
   of code that I never publish, and some of it I commit privately),
   but they are not suitable to include in an official repository.
   For example, commit 2a33a40929a6 "Update the *.at files" deletes
   tons of tests, makes unexplained changes to others, and adds a
   printf statement to dpif.c (which should never call printf()), and
   the change log doesn't say anything about any of it.

3. Commit messages must be properly written, e.g.:

   - Commit messages must explain the reason for the commit, when it
     is not obvious.  Many of the message I see in the Cloudbase tree
     do not, e.g. "Use exit instead of abort on valists", "Update the
     *.at files", "Changes to the ofp-errors".  (That is true even if
     one looks at the commit message and the changes in the patch at
     the same time.)

   - Signed-off-by lines are required.  The Cloudbase commits do not
     have any.

   - Commit messages should be written as full sentences.  Many
     Cloudbase commit messages are only a few words.

   - Limit commit messages to 79 characters in width.  Many longer
     Cloudbase commit messages spill much past this limit.

   Please read CONTRIBUTING.

4. The history overall must make sense.  Commits must be organized to
   make review feasible.  The obvious problem with the commits I see
   here is that there are 216 of them, way too many.  I think that it
   should be possible to bring this down to a handful.

5. Commits must not introduce regressions.  A regression is something
   that worked before a commit but does not after the commit.  New
   functionality, such as a port to Windows, cannot be a regression in
   itself (even if the new functionality is buggy), but it can cause
   regressions as side effects.

   This series of commits introduces regressions for non-Windows
   platforms.  When I tried building from the tip of the Cloudbase
   tree, I got hundreds and hundreds of errors from Automake, GCC,
   Clang, and sparse.  There's no point in listing them specifically.

   One way to avoid regressions when adding a new feature is to avoid
   changing existing code, if it is possible.  It should be possible
   in this case given the changes that Guru has done over the last few
   months to better support Windows and MSVC.  So this point may not
   require any additional effort.

6. Do not add third-party libraries to the source tree.  They cause
   security and maintenance headaches.  For example, if there is a
   single OpenSSL shared library on a system, then security problems
   in OpenSSL can be fixed by fixing that copy.  If there are multiple
   copies of OpenSSL, then every one of those copies has to be fixed.
   If some of the copies of OpenSSL are statically linked, then it
   becomes difficult even to identify all of them.

7. Do not add generated files to the repository.  Examples: config.h,
   vswitch-idl.c, vswitch-idl.h, ofp-errors.inc, ofp-msgs.inc.

   Corollary: do not add binaries to the source tree.

8. Do not add unexplained files to the repository.  Examples:
   ovsdb/conf.db, ovsdb/temp.out.  The commits that add these files do
   not mention them in the commit message.

9. Every file, with few exceptions, must include a copyright and
   license statement.  Many new files in windows/ lack both.

   If a file's license is not Apache 2.0, then the various files that
   describe OVS licensing must be updated.  The license that is used
   must not make OVS undistributable.  If adding the license changes
   licensing of OVS binaries as a whole then that needs to be called
   out for discussion.  Example: windows/getopt.c is licensed under
   LGPL, which means that any OVS binaries built with this file would
   need to be distributed in an LGPL compliant fashion, which requires
   significantly more care than distributing in an Apache 2.0
   compliant fashion.

Thanks,

Ben.



More information about the dev mailing list