Re: [1/2](resend)advapi: break the core of LookupAccountNameW out into helper functions.

[Juan Lang]

Hi Aric,

it strikes me that this could be broken up a bit further.  This'd make
it rather easier to review.  For example,

--- a/dlls/advapi32/advapi32_misc.h
+++ b/dlls/advapi32/advapi32_misc.h
@@ -24,4 +24,15 @@ const char * debugstr_sid(PSID sid);
 BOOL ADVAPI_IsLocalComputer(LPCWSTR ServerName);
 BOOL ADVAPI_GetComputerSid(PSID sid);

+BOOL lookup_local_wellknown_name(LPCWSTR lpAccountName, DWORD ccAccountName,
+                                 PSID Sid, LPDWORD cbSid,
+                                 LPWSTR ReferencedDomainName,
+                                 LPDWORD cchReferencedDomainName,
+                                 PSID_NAME_USE peUse, BOOL *ret);
+BOOL lookup_local_user_name(LPCWSTR lpAccountName, DWORD ccAccountName,
+                            PSID Sid, LPDWORD cbSid,
+                            LPWSTR ReferencedDomainName,
+                            LPDWORD cchReferencedDomainName,
+                            PSID_NAME_USE peUse, BOOL *ret);

In the initial patch, these aren't referenced outside of security.c,
so leaving them static there would make the changes smaller.  A
subsequent patch that does reference them from lsa.c could make them
non-static.

+static inline WCHAR *strnrchrW(const WCHAR *str, WCHAR ch, DWORD *len)

It's not clear to me what this function does from its name, and you
didn't document it.  There's no corresponding strnrchr libc function,
so that doesn't help either.

It appears from reading it that you made a function like strrchrW that
returns the location of the found ch in str, like strrchrW does, but
also sets *len to the position at which it's found?

If that's correct, you could accomplish the same thing by calling
strrchrW and doing pointer arithmetic with its return value.

These suggestions notwithstanding, it seems like introducing this
helper function is one change, and deserving of its own patch.

+
+/*
+ * Helper function for LookupAccountNameW and LookupNames2 in lsa.c

The second part of that comment, that it's a helper for LookupNames2,
isn't correct in this first patch.  It becomes true later, but the
comment should be changed then, not in the first patch.  (Minor nit:
mentioning the file LookupNames2 is in seems superfluous.)

+ * returns TRUE if the AccountName is handled (even if with an error).
+ * returns FALSE if not.
+ */
+
+BOOL lookup_local_wellknown_name(LPCWSTR lpAccountName, DWORD ccAccountName,
+                                 PSID Sid, LPDWORD cbSid,
+                                 LPWSTR ReferencedDomainName,
+                                 LPDWORD cchReferencedDomainName,
+                                 PSID_NAME_USE peUse, BOOL *ret)

The use of two BOOLs is confusing to me.  For one thing, it makes the
patch larger:

-            ret = CreateWellKnownSid(ACCOUNT_SIDS[i].type, NULL,
pSid, &sidLen);
+            *ret = CreateWellKnownSid(ACCOUNT_SIDS[i].type, NULL,
pSid, &sidLen);

This change could be avoided if you returned the old meaning of ret,
and used the BOOL * parameter to indicate whether the AccountName is
handled.

Finally, I'd suggest that breaking out looking up well known names
could be separate from breaking out looking up local names.

Hope that helps,
--Juan