Hi Jonathan,

As I mentioned in the other mail, it would be interesting to add some more tests. As for those tests, I have a few comments bellow.

On 12.09.2017 10:01, Jonathan Doron wrote:
Signed-off-by: Jonathan Doron <jond@wizery.com>
---
 dlls/kernel32/tests/pipe.c | 241 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 241 insertions(+)

diff --git a/dlls/kernel32/tests/pipe.c b/dlls/kernel32/tests/pipe.c
index 0a6edd6..9b90ce9 100644
--- a/dlls/kernel32/tests/pipe.c
+++ b/dlls/kernel32/tests/pipe.c
@@ -28,6 +28,7 @@
 #include "winternl.h"
 #include "winioctl.h"
 #include "wine/test.h"
+#include "sddl.h"
 
 #define PIPENAME "\\\\.\\PiPe\\tests_pipe.c"
 
@@ -3018,6 +3019,245 @@ static void test_overlapped_transport(BOOL msg_mode, BOOL msg_read_mode)
     CloseHandle(client);
 }
 
+static PSECURITY_DESCRIPTOR GetObjectSecDesc(HANDLE Object)
+{
+    ULONG RequiredLength;
+    PSECURITY_DESCRIPTOR SecDesc = NULL;
+
+    RequiredLength = 0;
+    NtQuerySecurityObject(Object,
+                          GROUP_SECURITY_INFORMATION | OWNER_SECURITY_INFORMATION,
+                          NULL, 0, &RequiredLength);    // Expected c0000023
+    if (!RequiredLength)
+        goto cleanup;
+
+    SecDesc = LocalAlloc(LMEM_FIXED, RequiredLength);

Please use HeapAlloc().

+    if (!SecDesc)
+        goto cleanup;
+    memset(SecDesc, 0, RequiredLength);
+
+    if (NtQuerySecurityObject(Object,
+                              GROUP_SECURITY_INFORMATION | OWNER_SECURITY_INFORMATION,
+                              SecDesc, RequiredLength, &RequiredLength)) {
+        LocalFree(SecDesc);
+        SecDesc = NULL;
+        goto cleanup;
+    }
+
+cleanup:
+    return SecDesc;
+}

Silent failures are not really good for tests. Please test NtQuerySecurityObject result with ok(). Same for other similar cases.

+static ULONG WINAPI ServerPipeThread(PVOID Param)
+{
+    ULONG RetVal;
+
+    RetVal = ConnectNamedPipe((HANDLE)Param, NULL) ? ERROR_SUCCESS : GetLastError();
+    if ((RetVal == ERROR_PIPE_CONNECTED) || (RetVal == ERROR_SUCCESS))
+        RetVal = ERROR_SUCCESS;
+
+    ExitThread(RetVal);
+}

This is not really needed. You may just connect to file after it's created, skipping new thread and ConnectNamedPipe() call.

Also there are styling issues. Please don't use C++ comments (//) in Wine and try to match style of existing code (eg. function names, variable names).

Thanks,
Jacek