[Spice-devel] [PATCH RFC] Add HACKING file
Christophe Fergeau
cfergeau at redhat.com
Mon Dec 8 05:56:43 PST 2014
This is libvirt's HACKING file with some parts not relevant to SPICE
removed. It contains some git advice, some C coding style rules, and
an attempt at defining what a trivial patch is.
---
Hey,
After the latest thread about trivial patches, I realized that libvirt actually
has a definition of what patches can be pushed without review in their HACKING
file. As it would be useful for us to have an agreed on definition for that, I
went ahead and created a HACKING file. As the libvirt file had more useful bits,
I tried to keep those as well (and also removed quite a bit of stuff).
Christophe
HACKING | 394 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Makefile.am | 1 +
2 files changed, 395 insertions(+)
create mode 100644 HACKING
diff --git a/HACKING b/HACKING
new file mode 100644
index 0000000..2a18038
--- /dev/null
+++ b/HACKING
@@ -0,0 +1,394 @@
+ Contributor guidelines
+ ======================
+
+
+
+General tips for contributing patches
+=====================================
+(1) Discuss any large changes on the mailing list first. Post patches early and
+listen to feedback.
+
+
+
+(2) Official upstream repository is kept in git ("http://cgit.freedesktop.org/spice/spice/")
+and is browsable along with other SPICE-related repositories (e.g.
+spice-protocol) online <http://cgit.freedesktop.org/spice/>.
+
+
+
+(3) Post patches in unified diff format, with git rename detection enabled. You
+need a one-time setup of:
+
+ git config diff.renames true
+
+After that, a command similar to this should work:
+
+ diff -urp spice.orig/ spice.modified/ > spice-myfeature.patch
+
+or:
+
+ git diff > spice-myfeature.patch
+
+Also, for code motion patches, you may find that "git diff --patience"
+provides an easier-to-read patch. However, the usual workflow of spice
+developer is:
+
+ git checkout master
+ git pull
+ git checkout -t origin -b workbranch
+ Hack, committing any changes along the way
+
+When you want to post your patches:
+
+ git pull --rebase
+ (fix any conflicts)
+ git send-email --cover-letter --no-chain-reply-to --annotate \
+ --to=spice-devel at lists.freedesktop.org master
+
+(Note that the "git send-email" subcommand may not be in the main git package
+and using it may require installation of a separate package, for example the
+"git-email" package in Fedora.) For a single patch you can omit
+"--cover-letter", but a series of two or more patches needs a cover letter. If
+you get tired of typing "--to=spice-devel at lists.freedesktop.org" designation you can set
+it in git config:
+
+ git config sendemail.to spice-devel at lists.freedesktop.org
+
+Please follow this as close as you can, especially the rebase and git
+send-email part, as it makes life easier for other developers to review your
+patch set. One should avoid sending patches as attachments, but rather send
+them in email body along with commit message. If a developer is sending
+another version of the patch (e.g. to address review comments), they are advised
+to note differences to previous versions after the "---" line in the patch so
+that it helps reviewers but doesn't become part of git history. Moreover, such
+patch needs to be prefixed correctly with "--subject-prefix=PATCHv2" appended
+to "git send-email" (substitute "v2" with the correct version if needed
+though).
+
+
+
+(4) In your commit message, make the summary line reasonably short (60 characters
+is typical), followed by a blank line, followed by any longer description of
+why your patch makes sense. If the patch fixes a regression, and you know what
+commit introduced the problem, mentioning that is useful. If the patch
+resolves a bugzilla report, mentioning the URL of the bug number is useful;
+but also summarize the issue rather than making all readers follow the link.
+You can use 'git shortlog -30' to get an idea of typical summary lines.
+SPICE does not currently attach any meaning to Signed-off-by: lines, so it
+is up to you if you want to include or omit them in the commit message.
+
+
+
+(5) Split large changes into a series of smaller patches, self-contained if
+possible, with an explanation of each patch and an explanation of how the
+sequence of patches fits together. Moreover, please keep in mind that it's
+required to be able to compile cleanly after each patch. A feature does not
+have to work until the end of a series, but intermediate patches must compile
+and not cause test-suite failures (this is to preserve the usefulness of "git
+bisect", among other things).
+
+
+
+(6) Make sure your patches apply against SPICE GIT. Developers only follow GIT
+and don't care much about released versions.
+
+
+
+There is more on this subject, including lots of links to background reading
+on the subject, on Richard Jones' guide to working with open source projects
+<http://people.redhat.com/rjones/how-to-supply-code-to-open-source-projects/>.
+
+
+Code indentation
+================
+SPICE's C source code generally adheres to some basic code-formatting
+conventions. The existing code base is not totally consistent on this front,
+but we do prefer that contributed code be formatted similarly. In short, use
+spaces-not-TABs for indentation, use 4 spaces for each indentation level, and
+other than that, follow the K&R style.
+
+
+Code formatting (especially for new code)
+=========================================
+
+Most of the code base prefers to stick to C89 syntax unless there is a
+compelling reason otherwise. For example, it is preferable to use "/* */"
+comments rather than "//". Also, when declaring local variables, the prevailing
+style has been to declare them at the beginning of a scope, rather than
+immediately before use.
+
+
+Bracket spacing
+===============
+The keywords "if", "for", "while", and "switch" must have a single space
+following them before the opening bracket. E.g.
+
+ if(foo) // Bad
+ if (foo) // Good
+
+Function implementations must not have any whitespace between the function name
+and the opening bracket. E.g.
+
+ int foo (int wizz) // Bad
+ int foo(int wizz) // Good
+
+Function calls must not have any whitespace between the function name and the
+opening bracket. E.g.
+
+ bar = foo (wizz); // Bad
+ bar = foo(wizz); // Good
+
+Function typedefs must not have any whitespace between the closing bracket of
+the function name and opening bracket of the arg list. E.g.
+
+ typedef int (*foo) (int wizz); // Bad
+ typedef int (*foo)(int wizz); // Good
+
+There must not be any whitespace immediately following any opening bracket, or
+immediately prior to any closing bracket. E.g.
+
+ int foo( int wizz ); // Bad
+ int foo(int wizz); // Good
+
+
+Commas
+======
+Commas should always be followed by a space or end of line, and never have
+leading space.
+
+ call(a,b ,c);// Bad
+ call(a, b, c); // Good
+
+When declaring an enum or using a struct initializer that occupies more than
+one line, use a trailing comma. That way, future edits to extend the list only
+have to add a line, rather than modify an existing line to add the
+intermediate comma.
+
+ enum {
+ VALUE_ONE,
+ VALUE_TWO // Bad
+ };
+ enum {
+ VALUE_THREE,
+ VALUE_FOUR, // Good
+ };
+
+
+Semicolons
+==========
+Semicolons should never have a space beforehand. Inside the condition of a
+"for" loop, there should always be a space or line break after each semicolon,
+except for the special case of an infinite loop (although more infinite loops
+use "while"). While not enforced, loop counters generally use post-increment.
+
+ for (i = 0 ;i < limit ; ++i) { // Bad
+ for (i = 0; i < limit; i++) { // Good
+ for (;;) { // ok
+ while (1) { // Better
+
+Empty loop bodies are better represented with curly braces and a comment,
+although use of a semicolon is not currently rejected.
+
+ while ((rc = waitpid(pid, &st, 0) == -1) &&
+ errno == EINTR); // ok
+ while ((rc = waitpid(pid, &st, 0) == -1) &&
+ errno == EINTR) { // Better
+ /* nothing */
+ }
+
+
+Preprocessor
+============
+Macros defined with an ALL_CAPS name should generally be assumed to be unsafe
+with regards to arguments with side-effects (that is, MAX(a++, b--) might
+increment a or decrement b too many or too few times). Exceptions to this rule
+are explicitly documented for macros in viralloc.h and virstring.h.
+
+For variadic macros, stick with C99 syntax:
+
+ #define vshPrint(_ctl, ...) fprintf(stdout, __VA_ARGS__)
+
+Use parenthesis when checking if a macro is defined, and use indentation to
+track nesting:
+
+ #if defined(HAVE_POSIX_FALLOCATE) && !defined(HAVE_FALLOCATE)
+ # define fallocate(a, ignored, b, c) posix_fallocate(a, b, c)
+ #endif
+
+
+C types
+=======
+Use the right type.
+
+Scalars
+-------
+- If you're using "int" or "long", odds are good that there's a better type.
+
+- If a variable is counting something, be sure to declare it with an unsigned
+type.
+
+- If it's memory-size-related, use "size_t" (use "ssize_t" only if required).
+
+- If it's file-size related, use uintmax_t, or maybe "off_t".
+
+- If it's file-offset related (i.e., signed), use "off_t".
+
+- If it's just counting small numbers use "unsigned int"; (on all but oddball
+embedded systems, you can assume that that type is at least four bytes wide).
+
+- In the unusual event that you require a specific width, use a standard type
+like "int32_t", "uint32_t", "uint64_t", etc.
+
+- While using "bool" is good for readability, it comes with minor caveats:
+
+-- Don't use "bool" in places where the type size must be constant across all
+systems, like public interfaces and on-the-wire protocols.
+
+-- Don't compare a bool variable against the literal, "true", since a value with
+a logical non-false value need not be "1". I.e., don't write "if (seen ==
+true) ...". Rather, write "if (seen)...".
+
+
+
+
+
+Of course, take all of the above with a grain of salt. If you're about to use
+some system interface that requires a type like "size_t", "pid_t" or "off_t",
+use matching types for any corresponding variables.
+
+Also, if you try to use e.g., "unsigned int" as a type, and that conflicts
+with the signedness of a related variable, sometimes it's best just to use the
+*wrong* type, if 'pulling the thread' and fixing all related variables would
+be too invasive.
+
+Finally, while using descriptive types is important, be careful not to go
+overboard. If whatever you're doing causes warnings, or requires casts, then
+reconsider or ask for help.
+
+Pointers
+--------
+Ensure that all of your pointers are 'const-correct'. Unless a pointer is used
+to modify the pointed-to storage, give it the "const" attribute. That way, the
+reader knows up-front that this is a read-only pointer. Perhaps more
+importantly, if we're diligent about this, when you see a non-const pointer,
+you're guaranteed that it is used to modify the storage it points to, or it is
+aliased to another pointer that is.
+
+
+Include files
+=============
+There are now quite a large number of include files, both SPICE internal and
+external, and system includes. To manage all this complexity it's best to
+stick to the following general plan for all *.c source files:
+
+ /*
+ * Copyright notice
+ * ....
+ * ....
+ * ....
+ *
+ */
+
+ #include <config.h> Must come first in every file.
+
+ #include <stdio.h> Any system includes you need.
+ #include <string.h>
+ #include <limits.h>
+
+ #if WITH_NUMACTL Some system includes aren't supported
+ # include <numa.h> everywhere so need these #if guards.
+ #endif
+
+ #include "util.h" Any SPICE internal header files.
+ #include "buf.h"
+
+ static int
+ myInternalFunc() The actual code.
+ {
+ ...
+
+
+Use of goto
+===========
+The use of goto is not forbidden, and goto is widely used throughout SPICE.
+While the uncontrolled use of goto will quickly lead to unmaintainable code,
+there is a place for it in well structured code where its use increases
+readability and maintainability. In general, if goto is used for error
+recovery, it's likely to be ok, otherwise, be cautious or avoid it all
+together.
+
+The typical use of goto is to jump to cleanup code in the case of a long list
+of actions, any of which may fail and cause the entire operation to fail. In
+this case, a function will have a single label at the end of the function.
+It's almost always ok to use this style.
+
+There are a couple of signs that a particular use of goto is not ok:
+
+- You're using multiple labels. If you find yourself using multiple labels,
+you're strongly encouraged to rework your code to eliminate all but one of
+them.
+
+- The goto jumps back up to a point above the current line of code being
+executed. Please use some combination of looping constructs to re-execute code
+instead; it's almost certainly going to be more understandable by others. One
+well-known exception to this rule is restarting an i/o operation following
+EINTR.
+
+- The goto jumps down to an arbitrary place in the middle of a function followed
+by further potentially failing calls. You should almost certainly be using a
+conditional and a block instead of a goto. Perhaps some of your function's
+logic would be better pulled out into a helper function.
+
+
+
+Although SPICE does not encourage the Linux kernel wind/unwind style of
+multiple labels, there's a good general discussion of the issue archived at
+KernelTrap <http://kerneltrap.org/node/553/2131>
+
+When using goto, please use one of these standard labels if it makes sense:
+
+ error: A path only taken upon return with an error code
+ cleanup: A path taken upon return with success code + optional error
+ no_memory: A path only taken upon return with an OOM error code
+ retry: If needing to jump upwards (e.g., retry on EINTR)
+
+Top-level labels should be indented by one space (putting them on the
+beginning of the line confuses function context detection in git):
+
+int foo()
+{
+ /* ... do stuff ... */
+ cleanup:
+ /* ... do other stuff ... */
+}
+
+
+SPICE committer guidelines
+==========================
+The AUTHORS files indicates the list of people with commit access right who
+can actually merge the patches.
+
+The general rule for committing a patch is to make sure it has been reviewed
+properly in the mailing-list first, usually if a couple of people gave an ACK
+or +1 to a patch and nobody raised an objection on the list it should be good
+to go. If the patch touches a part of the code where you're not the main
+maintainer, or where you do not have a very clear idea of how things work,
+it's better to wait for a more authoritative feedback though. Before
+committing, please also rebuild locally, and make sure you don't raise errors.
+Try to look for warnings too; for example, compile with
+
+ make CFLAGS="-Werror"
+
+which adds -Werror to compile flags, so no warnings get missed
+
+An exception to 'review and approval on the list first' is fixing failures to
+build:
+
+- if a recently committed patch breaks compilation on a platform or for a given
+driver, then it's fine to commit a minimal fix directly without getting the
+review feedback first
+
+- fixes for documentation and code comments can be managed in the same way, but
+still make sure they get reviewed if non-trivial.
+
+The patch should still be sent to the list (or tell what the fix was if trivial)
diff --git a/Makefile.am b/Makefile.am
index cada94d..5e552b0 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -15,6 +15,7 @@ DISTCHECK_CONFIGURE_FLAGS = \
$(NULL)
EXTRA_DIST = \
+ HACKING \
build-aux/git-version-gen \
.version \
$(NULL)
--
2.1.0
More information about the Spice-devel
mailing list