Hello again,
I would also suggest, in order to improve clarity.
For instance, say if there is a function we need to add, that searches the list
of Hyper-V Nics in a list.
And this list is supposed to be previously locked / synchronized by the caller.
The windows kernel style, as we had seen in the hyper-v switch samples, is to
use an "Unsafe" suffix to
make it clear that the function body does not take into account synchronization.
So a way we could name this function would be:
OvsFindHvNicByNicIndexAndPortIdUnsafe
I believe we all agree that this makes it difficult to read.
1. The "Ovs" prefix pollutes the function name more than helps.
As you all probably know, the windows kernel API uses prefixes for functions
(with a very few exceptions), which help to distinguish both
between their code and ours, and among their components.
Prefixes such as: Ke (kernel), Ex (executive), Ndis, etc.
We should use "OVS_" prefixes for types instead, as we can easily run into
non-prefixed types defined in various
windows kernel header files (e.g. ETHERNET_HEADER in netiodef.h), which creates
confusion.
2. Functions dealing with Hyper-V Nics may be easier to be looked-up in code,
if say, they are prefixed by "HvNic".
So we could have HvNicFindByNicIndexAndPortIdUnsafe
However this is still very long and it may be easy to confuse functions with
similar names.
3. In our project, I used the following (most likely, not quite used style):
HvNic_FindByIndexAndPortId_Unsafe
If we add a "_" after the the function operates upon, and a "_" before the
"Unsafe" suffix, it
would be easier to read and more difficult to confuse functions, and would
nicely separate
the "component" part from the "action" and from the possible suffix.
I am suggesting for this kind of function naming to be allowed :)
Sam
________________________________
From: Samuel Ghinet
Sent: Wednesday, August 06, 2014 12:40 AM
To: dev@openvswitch.org
Subject: RE: [PATCH] datapath-windows: Update CodingStyle
Hello guys,
I sent the email with this patch, as I have these suggestions for updating the
coding style
in the windows kernel driver.
I believe we should improve the coding style & coding style requirements. And,
given the
fact that the windows kernel driver is still fresh in the repository, this
would be a proper time to do it.
I'll be waiting for your feedback. Also, if anybody else has coding style
suggestions, it would be nice :)
Thanks!
Sam
________________________________
From: Samuel Ghinet
Sent: Wednesday, August 06, 2014 12:28 AM
To: dev@openvswitch.org
Subject: [PATCH] datapath-windows: Update CodingStyle
Update the file CodingStyle: add more Windows-style rules.
Also, other rules will make code more clear.
Windows Kernel style rules:
o) Type names (structs, enums), constants, symbols, macros.
o) Braces
o) Code annotations
o) Function suffix: Unsafe
o) Switch Cases
Code clarity rules:
o) OVS_ prefixes for types, constants, symbols, macros.
o) "s_" and "g_" prefixes for static and global variables. Normally, Windows
style uses "g" instead of
o) g_" and does not have a prefix for static variables.
o) "p" or "pp" prefixes for pointer and pointer-to-pointer variables. This
style is used in Windows userspace
code style, but not in kernel. Either way, it is useful to distinguish pointer
variables from other variable types.
o) "_" prefix for private functions.
o) Component-prefixed function names.
---
datapath-windows/CodingStyle | 177 ++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 175 insertions(+), 2 deletions(-)
diff --git a/datapath-windows/CodingStyle b/datapath-windows/CodingStyle
index 006adfd..a8f020d 100644
--- a/datapath-windows/CodingStyle
+++ b/datapath-windows/CodingStyle
@@ -42,8 +42,35 @@ guidelines:
Use upper case to begin the name of a function, enum, file name etc.
- For types, use all upper case for all letters with words separated by '_'. If
- camel casing is preferred, use upper case for the first letter.
+ All types, all enum constants and all symbol definitions must be all upper
case.
+If the name is made of multiple words, separate words by "_".
+
+ All macros, symbols, and custom types (i.e. typedef to enums, structs,
unions) must
+be prefixed by "OVS_". Functions must not be prefixed by "Ovs".
+
+ Enum constants must follow the name style of the enum type.
+
+ Example:
+
+typedef enum _OVS_IPPROTO
+{
+ OVS_IPPROTO_ICMP = 1,
+ //constants here.
+};
+
+ This makes easier lookup of enum constants, knowing the enum type.
+
+ All types must be declared with typedef. Type typedef must always be used.
+
+ Example:
+
+typedef struct _OVS_DATAPATH
+{
+ //fields here
+}OVS_DATAPATH, *POVS_DATAPATH;
+
+ Always use OVS_DATAPATH or POVS_DATAPATH. Do not declare variable by
+ "struct _OVS_DATAPATH dp;"
It is a common practice to define a pointer type by prefixing the letter
'P' to a data type. The same practice can be followed here as well.
@@ -72,6 +99,47 @@ OvsDetectTunnelRxPkt(POVS_FORWARDING_CONTEXT ovsFwdCtx,
return FALSE;
}
+ Builtin types: UINT, INT, UINT16, VOID etc. are preferred over unsigned int,
int,
+unsigned short, void, etc. Do not use typedef-s for builtin types that are
lowercase.
+For example, do not use uint32_t, uint16_t, etc.
+
+ In Visual Studio, ULONG is identical to UINT and with UINT32. The use of
ULONG is
+preferred. Use unsigned types whenever the variable should only have positive
values.
+Use typedef-s for UINT32: BE32, BE64, whenever it is known that the variable
is Big Endian.
+Use UINT32 only when it is important to know that the value is 32 bit long.
Otherwise, use
+ULONG.
+
+ Variables declared in a .c file that are used only within that .c file must
be static and
+prefixed by "s_" (stands for "static"). Global variables, i.e. variables that
are declared in
+a .h file and used in multiple .c files must be prefixed by "g_" (stands for
"global").
+
+ Pointer data types should be prefixed by "p" or "pp".
+
+ Example:
+
+OVS_FLOW* pFlow;
+OVS_FLOW** ppFlow;
+
+ or
+
+POVS_FLOW pFlow;
+POVS_FLOW* ppFlow;
+
+Arrays should be named like this:
+OVS_FLOW* flows;
+OVS_FLOW** pFlows;
+
+ or
+
+POVS_FLOW flows;
+POVS_FLOW* pFlows;
+
+If "*" is preferred in a variable declaration, instead of using P-prefixed
type,
+the "*" must be attached to the type, when possible.
+
+Example:
+
+OVS_FLOW* pFlow; //instead of OVS_FLOW *pFlow;
COMMENTS
@@ -103,12 +171,41 @@ the name of the function or based on the workflow it is
called from.
In the interest of keeping comments describing functions similar in
structure, use the following template.
+ Code annotations are preferred.
+
/*
*----------------------------------------------------------------------------
* Any description of the function, arguments, return types, assumptions and
* side effects.
*----------------------------------------------------------------------------
*/
+
+ All private functions (i.e. functions that are to be used only in one .c
file) should
+be defined as static, and be "_" prefixed.
+
+ Example:
+
+static BOOLEAN _FunctionName(VOID* p)
+{
+ //code
+}
+
+ This makes it clear when studying the code, and when seeing function calls
weather
+the function is supposed to be private or public.
+
+ Public functions that operate on a specific component should have a
component name prefix.
+
+ Example:
+
+OVS_FLOW* FlowCreate(VOID* p)
+{
+ //code
+}
+
+ Functions that operate on global data (such as, functions that add / remove
flows), and expect the
+caller to hold a lock beforehand, should be suffixed with "Unsafe". If the
component function (e.g. Flow)
+does not lock its specific lock (e.g. the flow's lock field), but locks any
other component's lock or
+global lock, it should still be suffixed by "Unsafe".
SOURCE FILES
@@ -137,3 +234,79 @@ quickly figure out where a given module fits into the
larger system.
4. Open vSwitch headers, in alphabetical order. Use "", not <>,
to specify Open vSwitch header names.
+
+BRACES
+
+ Use Windows-style braces:
+ The open brace must be on a separate line.
+
+ Example:
+
+if (condition)
+{
+ //code
+}
+
+typedef struct _OVS_DATAPATH
+{
+ //fields here
+}OVS_DATAPATH, *pOVS_DATAPATH;
+
+ All code executed for an if, else, while, do while, or for, must be
+ enclosed in braces, even if it's only one instruction to be executed.
+ The else part of an if must be on the very next line after the if's closed
+ brace (i.e. there should be no blank line in between)
+
+ Example:
+
+if (condition)
+{
+ //code
+}
+else
+{
+ //code
+}
+
+For all other cases than else, after "}", a blank line must follow
+
+SWITCH CASES
+
+ Each "case", if it is preceded by a "break" of a previous case, there must be
+ a blank line in between:
+switch (c)
+{
+ case v1:
+ //code
+ break;
+
+ case v2:
+ //code
+ break;
+}
+
+ Number of lines per file: try to limit to 1000 lines. Maximum allowed should
be 1500.
+If a file is grown to large in number of lines, consider splitting it into
files, based on
+the components the functions operate upon.
+
+SPACES WITHIN LINES
+
+ Use spaces between operands and operators.
+
+ Example:
+
+x + y * z
+
+instead of:
+
+x+y*z
+
+ Do not use space after "(" or before ")"
+
+ Example:
+
+(x * y * z) + a
+
+ instead of:
+
+( x * y * z ) + a
--
1.8.3.msysgit.0
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
