From 34ed4e980998fb9b5c8a9f690220755480392115 Mon Sep 17 00:00:00 2001 From: Matthias Bolte Date: Fri, 12 Nov 2010 15:36:53 +0100 Subject: [PATCH] Generate HACKING from docs/hacking.html.in --- HACKING | 592 +++++++++++++++++++++++++------------------- Makefile.am | 7 + cfg.mk | 3 + docs/hacking1.xsl | 28 +++ docs/hacking2.xsl | 146 +++++++++++ docs/wrapstring.xsl | 56 +++++ 6 files changed, 579 insertions(+), 253 deletions(-) create mode 100644 docs/hacking1.xsl create mode 100644 docs/hacking2.xsl create mode 100644 docs/wrapstring.xsl diff --git a/HACKING b/HACKING index a9a9b49924..cbd38839f7 100644 --- a/HACKING +++ b/HACKING @@ -1,18 +1,19 @@ -*- buffer-read-only: t -*- vi: set ro: DO NOT EDIT THIS FILE! IT IS GENERATED AUTOMATICALLY! - Libvirt contributor guidelines - ============================== + + + Contributor guidelines + ====================== + General tips for contributing patches ===================================== +(1) Discuss any large changes on the mailing list first. Post patches early and +listen to feedback. -(1) Discuss any large changes on the mailing list first. Post patches -early and listen to feedback. - -(2) Post patches in unified diff format. A command similar to this -should work: +(2) Post patches in unified diff format. A command similar to this should work: diff -urp libvirt.orig/ libvirt.modified/ > libvirt-myfeature.patch @@ -20,15 +21,15 @@ or: git diff > libvirt-myfeature.patch -(3) 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. +(3) 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. -(4) Make sure your patches apply against libvirt GIT. Developers -only follow GIT and don't care much about released versions. +(4) Make sure your patches apply against libvirt GIT. Developers only follow GIT +and don't care much about released versions. -(5) Run the automated tests on your code before submitting any changes. -In particular, configure with compile warnings set to -Werror: +(5) Run the automated tests on your code before submitting any changes. In +particular, configure with compile warnings set to -Werror: ./configure --enable-compile-warnings=error @@ -47,28 +48,29 @@ VIR_TEST_DEBUG may provide larger amounts of information: VIR_TEST_DEBUG=1 make check (or) VIR_TEST_DEBUG=2 make check -Also, individual tests can be run from inside the 'tests/' directory, like: +Also, individual tests can be run from inside the "tests/" directory, like: ./qemuxml2xmltest -(6) Update tests and/or documentation, particularly if you are adding -a new feature or changing the output of a program. +(6) Update tests and/or documentation, particularly if you are adding a new +feature or changing the output of a program. -There is more on this subject, including lots of links to background -reading on the subject, on this page: +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://et.redhat.com/~rjones/how-to-supply-code-to-open-source-projects/ - Code indentation ================ Libvirt'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. +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. If you use Emacs, add the following to one of one of your start-up files (e.g., ~/.emacs), to help ensure that you get indentation right: @@ -82,15 +84,15 @@ If you use Emacs, add the following to one of one of your start-up files (setq c-indent-level 4) (setq c-basic-offset 4)) (add-hook 'c-mode-hook - '(lambda () (if (string-match "/libvirt" (buffer-file-name)) - (libvirt-c-mode)))) + '(lambda () (if (string-match "/libvirt" (buffer-file-name)) + (libvirt-c-mode)))) + Code formatting (especially for new code) ========================================= -With new code, we can be even more strict. -Please apply the following function (using GNU indent) to any new code. -Note that this also gives you an idea of the type of spacing we prefer -around operators and keywords: +With new code, we can be even more strict. Please apply the following function +(using GNU indent) to any new code. Note that this also gives you an idea of +the type of spacing we prefer around operators and keywords: indent-libvirt() { @@ -99,66 +101,63 @@ around operators and keywords: --no-tabs "$@" } -Note that sometimes you'll have to post-process that output further, by -piping it through "expand -i", since some leading TABs can get through. -Usually they're in macro definitions or strings, and should be converted -anyhow. +Note that sometimes you'll have to post-process that output further, by piping +it through "expand -i", since some leading TABs can get through. Usually +they're in macro definitions or strings, and should be converted anyhow. Curly braces ============ Omit the curly braces around an "if", "while", "for" etc. body only when that body occupies a single line. In every other case we require the braces. This -ensures that it is trivially easy to identify a single-*statement* loop: each -has only one *line* in its body. +ensures that it is trivially easy to identify a single-'statement' loop: each +has only one 'line' in its body. Omitting braces with a single-line body is fine: - while (expr) // one-line body -> omitting curly braces is ok - single_line_stmt (); + while (expr) // one-line body -> omitting curly braces is ok + single_line_stmt(); However, the moment your loop/if/else body extends onto a second line, for whatever reason (even if it's just an added comment), then you should add braces. Otherwise, it would be too easy to insert a statement just before that -comment (without adding braces), thinking it is already a multi-statement -loop: +comment (without adding braces), thinking it is already a multi-statement loop: - while (true) // BAD! multi-line body with no braces - /* comment... */ - single_line_stmt (); + while (true) // BAD! multi-line body with no braces + /* comment... */ + single_line_stmt(); Do this instead: - while (true) { // Always put braces around a multi-line body. - /* comment... */ - single_line_stmt (); - } + while (true) { // Always put braces around a multi-line body. + /* comment... */ + single_line_stmt(); + } There is one exception: when the second body line is not at the same indentation level as the first body line: - if (expr) - die ("a diagnostic that would make this line" - " extend past the 80-column limit")); + if (expr) + die("a diagnostic that would make this line" + " extend past the 80-column limit")); It is safe to omit the braces in the code above, since the further-indented second body line makes it obvious that this is still a single-statement body. - To reiterate, don't do this: - if (expr) // BAD: no braces around... - while (expr_2) { // ... a multi-line body - ... - } + if (expr) // BAD: no braces around... + while (expr_2) { // ... a multi-line body + ... + } Do this, instead: - if (expr) { - while (expr_2) { - ... - } - } + if (expr) { + while (expr_2) { + ... + } + } However, there is one exception in the other direction, when even a one-line block should have braces. That occurs when that one-line, brace-less block is @@ -167,47 +166,47 @@ case, either put braces around the "else" block, or negate the "if"-condition and swap the bodies, putting the one-line block first and making the longer, multi-line block be the "else" block. - if (expr) { - ... - ... - } - else - x = y; // BAD: braceless "else" with braced "then" + if (expr) { + ... + ... + } + else + x = y; // BAD: braceless "else" with braced "then" This is preferred, especially when the multi-line body is more than a few -lines long, because it is easier to read and grasp the semantics of an if- -then-else block when the simpler block occurs first, rather than after the +lines long, because it is easier to read and grasp the semantics of an +if-then-else block when the simpler block occurs first, rather than after the more involved block: - if (!expr) - x = y; // putting the smaller block first is more readable - else { - ... - ... - } + if (!expr) + x = y; // putting the smaller block first is more readable + else { + ... + ... + } If you'd rather not negate the condition, then at least add braces: - if (expr) { - ... - ... - } else { - x = y; - } + if (expr) { + ... + ... + } else { + x = y; + } Preprocessor ============ For variadic macros, stick with C99 syntax: -#define vshPrint(_ctl, ...) fprintf(stdout, __VA_ARGS__) + #define vshPrint(_ctl, ...) fprintf(stdout, __VA_ARGS__) -Use parenthesis when checking if a macro is defined, and use -indentation to track nesting: +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 + #if defined(HAVE_POSIX_FALLOCATE) && !defined(HAVE_FALLOCATE) + # define fallocate(a,ignored,b,c) posix_fallocate(a,b,c) + #endif C types @@ -216,199 +215,235 @@ 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). -If a variable has boolean semantics, give it the "bool" type -and use the corresponding "true" and "false" macros. It's ok -to include , since libvirt's use of gnulib ensures -that it exists and is usable. -In the unusual event that you require a specific width, use a -standard type like int32_t, uint32_t, uint64_t, etc. +- If you're using "int" or "long", odds are good that there's a better type. -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. Note - that it would be possible (albeit wasteful) to use "bool" in libvirt's - logical wire protocol, since XDR maps that to its lower-level bool_t - type, which *is* fixed-size. - - 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)...". +- If a variable is counting something, be sure to declare it with an unsigned +type. -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. +- If it's memory-size-related, use "size_t" (use "ssize_t" only if required). -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. +- If it's file-size related, use uintmax_t, or maybe "off_t". -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. +- 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). + +- If a variable has boolean semantics, give it the "bool" type and use the +corresponding "true" and "false" macros. It's ok to include , since +libvirt's use of gnulib ensures that it exists and is usable. + +- 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. Note that it would +be possible (albeit wasteful) to use "bool" in libvirt's logical wire +protocol, since XDR maps that to its lower-level "bool_t" type, which *is* +fixed-size. + +-- 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. +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. Low level memory management =========================== - Use of the malloc/free/realloc/calloc APIs is deprecated in the libvirt -codebase, because they encourage a number of serious coding bugs and do -not enable compile time verification of checks for NULL. Instead of these +codebase, because they encourage a number of serious coding bugs and do not +enable compile time verification of checks for NULL. Instead of these routines, use the macros from memory.h - - eg to allocate a single object: +- e.g. to allocate a single object: - virDomainPtr domain; + virDomainPtr domain; - if (VIR_ALLOC(domain) < 0) { - virReportOOMError(); - return NULL; - } + if (VIR_ALLOC(domain) < 0) { + virReportOOMError(); + return NULL; + } - - eg to allocate an array of objects - virDomainPtr domains; - int ndomains = 10; +- e.g. to allocate an array of objects - if (VIR_ALLOC_N(domains, ndomains) < 0) { - virReportOOMError(); - return NULL; - } + virDomainPtr domains; + int ndomains = 10; - - eg to allocate an array of object pointers + if (VIR_ALLOC_N(domains, ndomains) < 0) { + virReportOOMError(); + return NULL; + } - virDomainPtr *domains; - int ndomains = 10; - if (VIR_ALLOC_N(domains, ndomains) < 0) { - virReportOOMError(); - return NULL; - } - - eg to re-allocate the array of domains to be longer +- e.g. to allocate an array of object pointers + + virDomainPtr *domains; + int ndomains = 10; + + if (VIR_ALLOC_N(domains, ndomains) < 0) { + virReportOOMError(); + return NULL; + } + + + +- e.g. to re-allocate the array of domains to be longer + + ndomains = 20 + + if (VIR_REALLOC_N(domains, ndomains) < 0) { + virReportOOMError(); + return NULL; + } + + + +- e.g. to free the domain + + VIR_FREE(domain); - ndomains = 20 - if (VIR_REALLOC_N(domains, ndomains) < 0) { - virReportOOMError(); - return NULL; - } - - eg to free the domain - VIR_FREE(domain); File handling ============= - Use of the close() API is deprecated in libvirt code base to help avoiding double-closing of a file descriptor. Instead of this API, use the macro from files.h - - eg close a file descriptor +- e.g. close a file descriptor + + if (VIR_CLOSE(fd) < 0) { + virReportSystemError(errno, _("failed to close file")); + } + + + +- eg close a file descriptor in an error path, without losing the previous +"errno" value + + VIR_FORCE_CLOSE(fd); + + - if (VIR_CLOSE(fd) < 0) { - virReportSystemError(errno, _("failed to close file")); - } - - eg close a file descriptor in an error path, without losing the previous - errno value - VIR_FORCE_CLOSE(fd); String comparisons ================== +Do not use the strcmp, strncmp, etc functions directly. Instead use one of the +following semantically named macros -Do not use the strcmp, strncmp, etc functions directly. Instead use -one of the following semantically named macros +- For strict equality: - - For strict equality: + STREQ(a,b) + STRNEQ(a,b) - STREQ(a,b) - STRNEQ(a,b) - - For case insensitive equality: - STRCASEEQ(a,b) - STRCASENEQ(a,b) - - For strict equality of a substring: +- For case insensitive equality: - STREQLEN(a,b,n) - STRNEQLEN(a,b,n) + STRCASEEQ(a,b) + STRCASENEQ(a,b) - - For case insensitive equality of a substring: - STRCASEEQLEN(a,b,n) - STRCASENEQLEN(a,b,n) - - For strict equality of a prefix: +- For strict equality of a substring: + + STREQLEN(a,b,n) + STRNEQLEN(a,b,n) + + + +- For case insensitive equality of a substring: + + STRCASEEQLEN(a,b,n) + STRCASENEQLEN(a,b,n) + + + +- For strict equality of a prefix: + + STRPREFIX(a,b) + + - STRPREFIX(a,b) String copying ============== +Do not use the strncpy function. According to the man page, it does *not* +guarantee a NULL-terminated buffer, which makes it extremely dangerous to use. +Instead, use one of the functionally equivalent functions: -Do not use the strncpy function. According to the man page, it does -*not* guarantee a NULL-terminated buffer, which makes it extremely dangerous -to use. Instead, use one of the functionally equivalent functions: + virStrncpy(char *dest, const char *src, size_t n, size_t destbytes) - - virStrncpy(char *dest, const char *src, size_t n, size_t destbytes) - The first three arguments have the same meaning as for strncpy; namely the - destination, source, and number of bytes to copy, respectively. The last - argument is the number of bytes available in the destination string; if a - copy of the source string (including a \0) will not fit into the - destination, no bytes are copied and the routine returns NULL. - Otherwise, n bytes from the source are copied into the destination and a - trailing \0 is appended. +The first three arguments have the same meaning as for strncpy; namely the +destination, source, and number of bytes to copy, respectively. The last +argument is the number of bytes available in the destination string; if a copy +of the source string (including a \0) will not fit into the destination, no +bytes are copied and the routine returns NULL. Otherwise, n bytes from the +source are copied into the destination and a trailing \0 is appended. - - virStrcpy(char *dest, const char *src, size_t destbytes) - Use this variant if you know you want to copy the entire src string - into dest. Note that this is a macro, so arguments could be - evaluated more than once. This is equivalent to - virStrncpy(dest, src, strlen(src), destbytes) + virStrcpy(char *dest, const char *src, size_t destbytes) - - virStrcpyStatic(char *dest, const char *src) - Use this variant if you know you want to copy the entire src string - into dest *and* you know that your destination string is a static string - (i.e. that sizeof(dest) returns something meaningful). Note that - this is a macro, so arguments could be evaluated more than once. This is - equivalent to virStrncpy(dest, src, strlen(src), sizeof(dest)). +Use this variant if you know you want to copy the entire src string into dest. +Note that this is a macro, so arguments could be evaluated more than once. +This is equivalent to virStrncpy(dest, src, strlen(src), destbytes) + virStrcpyStatic(char *dest, const char *src) + +Use this variant if you know you want to copy the entire src string into dest +*and* you know that your destination string is a static string (i.e. that +sizeof(dest) returns something meaningful). Note that this is a macro, so +arguments could be evaluated more than once. This is equivalent to +virStrncpy(dest, src, strlen(src), sizeof(dest)). Variable length string buffer ============================= - -If there is a need for complex string concatenations, avoid using -the usual sequence of malloc/strcpy/strcat/snprintf functions and -make use of the virBuffer API described in buf.h +If there is a need for complex string concatenations, avoid using the usual +sequence of malloc/strcpy/strcat/snprintf functions and make use of the +virBuffer API described in buf.h eg typical usage is as follows: char * - somefunction(...) { + somefunction(...) + { virBuffer buf = VIR_BUFFER_INITIALIZER; ... @@ -432,11 +467,9 @@ eg typical usage is as follows: Include files ============= - -There are now quite a large number of include files, both libvirt -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: +There are now quite a large number of include files, both libvirt 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 @@ -461,59 +494,112 @@ complexity it's best to stick to the following general plan for all #include "util.h" Any libvirt internal header files. #include "buf.h" - static myInternalFunc () The actual code. + static int + myInternalFunc() The actual code. { - ... + ... -Of particular note: *DO NOT* include libvirt/libvirt.h or -libvirt/virterror.h. It is included by "internal.h" already and there -are some special reasons why you cannot include these files -explicitly. +Of particular note: *Do not* include libvirt/libvirt.h or libvirt/virterror.h. +It is included by "internal.h" already and there are some special reasons why +you cannot include these files explicitly. Printf-style functions ====================== +Whenever you add a new printf-style function, i.e., one with a format string +argument and following "..." in its prototype, be sure to use gcc's printf +attribute directive in the prototype. For example, here's the one for +virAsprintf, in util.h: -Whenever you add a new printf-style function, i.e., one with a format -string argument and following "..." in its prototype, be sure to use -gcc's printf attribute directive in the prototype. For example, here's -the one for virAsprintf, in util.h: + int virAsprintf(char **strp, const char *fmt, ...) + ATTRIBUTE_FORMAT(printf, 2, 3); - int virAsprintf(char **strp, const char *fmt, ...) - ATTRIBUTE_FMT_PRINTF(2, 3); +This makes it so gcc's -Wformat and -Wformat-security options can do their +jobs and cross-check format strings with the number and types of arguments. -This makes it so gcc's -Wformat and -Wformat-security options can do -their jobs and cross-check format strings with the number and types -of arguments. + +Use of goto +=========== +The use of goto is not forbidden, and goto is widely used throughout libvirt. +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. In particular, if the cleanup code +only involves free'ing memory, then having multiple labels is overkill. +VIR_FREE() and every function named XXXFree() in libvirt is required to handle +NULL as its arg. Thus you can safely call free on all the variables even if +they were not yet allocated (yes they have to have been initialized to NULL). +This is much simpler and clearer than having multiple labels. + +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. - Libvirt committer guidelines - ============================ +Although libvirt does not encourage the Linux kernel wind/unwind style of +multiple labels, there's a good general discussion of the issue archived at -The AUTHORS files indicates the list of people with commit access right -who can actually merge the patches. + 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 (eg retry on EINTR) + + +Libvirt 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 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 run 'make check syntax-check' and make sure they -don't raise error. Try to look for warnings too for example configure with - --enable-compile-warnings=error +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, run 'make check syntax-check', and +make sure you don't raise errors. Try to look for warnings too; for example, +configure with + + --enable-compile-warnings=error + which adds -Werror to compile flags, so no warnings get missed -Exceptions to that '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 - - similarly, if make check or make syntax-check breaks, if there is - an obvious fix, it's fine to commit immediately -The patch should still be sent to the list (or tell what the fix was if -trivial) and 'make check syntax-check' should pass too before committing -anything -Similar fixes for documentation and code comments can be managed -in the same way, but still make sure they get reviewed if non-trivial. +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 + +- if make check or make syntax-check breaks, if there is an obvious fix, it's +fine to commit immediately. The patch should still be sent to the list (or +tell what the fix was if trivial), and 'make check syntax-check' should pass +too, before committing anything + +- fixes for documentation and code comments can be managed in the same way, but +still make sure they get reviewed if non-trivial. diff --git a/Makefile.am b/Makefile.am index d05b7da0e6..f3dbd6d5cd 100644 --- a/Makefile.am +++ b/Makefile.am @@ -56,6 +56,13 @@ NEWS: $(top_srcdir)/docs/news.xsl $(top_srcdir)/docs/news.html.in | perl -pe 's/[ \t]+$$//' \ > $@-t && mv $@-t $@ ; fi ); +$(top_srcdir)/HACKING: $(top_srcdir)/docs/hacking1.xsl $(top_srcdir)/docs/hacking2.xsl \ + $(top_srcdir)/docs/wrapstring.xsl $(top_srcdir)/docs/hacking.html.in + -@(if [ -x $(XSLTPROC) ] ; then \ + $(XSLTPROC) --nonet $(top_srcdir)/docs/hacking1.xsl $(top_srcdir)/docs/hacking.html.in | \ + $(XSLTPROC) --nonet $(top_srcdir)/docs/hacking2.xsl - \ + | perl -0777 -pe 's/\n\n+$$/\n/' \ + > $@-t && mv $@-t $@ ; fi ); rpm: clean @(unset CDPATH ; $(MAKE) dist && rpmbuild -ta $(distdir).tar.gz) diff --git a/cfg.mk b/cfg.mk index 16c2ae3c3c..072673e241 100644 --- a/cfg.mk +++ b/cfg.mk @@ -486,3 +486,6 @@ _autogen: # Exempt @...@ uses of these symbols. _makefile_at_at_check_exceptions = ' && !/(SCHEMA|SYSCONF)DIR/' + +# regenerate HACKING as part of the syntax-check +syntax-check: $(top_srcdir)/HACKING diff --git a/docs/hacking1.xsl b/docs/hacking1.xsl new file mode 100644 index 0000000000..982fa8df22 --- /dev/null +++ b/docs/hacking1.xsl @@ -0,0 +1,28 @@ + + + + + + + + + + + + + + + + + + + + + + +** +'' +"" + + diff --git a/docs/hacking2.xsl b/docs/hacking2.xsl new file mode 100644 index 0000000000..89e777b0ea --- /dev/null +++ b/docs/hacking2.xsl @@ -0,0 +1,146 @@ + + + + + + + + + + + + + + + + + + + +-*- buffer-read-only: t -*- vi: set ro: +DO NOT EDIT THIS FILE! IT IS GENERATED AUTOMATICALLY! + + + + + + + + + + + + + + + ====================== + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +() + + +() + + + + + + +- + + + + +-- + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/docs/wrapstring.xsl b/docs/wrapstring.xsl new file mode 100644 index 0000000000..b468a700f5 --- /dev/null +++ b/docs/wrapstring.xsl @@ -0,0 +1,56 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +