[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