On 06/09/11 20:22, Russell Bryant wrote:
On 09/06/2011 01:03 PM, Adam Stokes wrote:
diff --git a/src/lib/utilities_windows.c b/src/lib/utilities_windows.c index 26a3679..c87dfe4 100644 --- a/src/lib/utilities_windows.c +++ b/src/lib/utilities_windows.c @@ -18,9 +18,14 @@
#include "config.h"
+#include<windows.h> +#include<rpc.h> #include "matahari/utilities.h" +#include "matahari/logging.h" #include "utilities_private.h"
+#define MAXUUIDLEN 255
- const char * mh_os_dnsdomainname(void) {
@@ -32,3 +37,47 @@ mh_os_dnsdomainname(void)
return mh_domainname();
}
+const char * +mh_os_uuid(void) +{
- UUID u;
- HKEY hKey;
- DWORD nSize = MAXUUIDLEN;
- wchar_t szData[MAXUUIDLEN];
I know these are the names of the formal parameters, but Win32 uses the *bad* kind of Hungarian notation (Systems Hungarian - see http://blogs.msdn.com/b/larryosterman/archive/2004/06/22/162629.aspx). I'd prefer more descriptive names.
- static char s[MAXUUIDLEN];
- wchar_t *szValue = char2wide("UUID");
Aside from the memory leak, this variable is not required. The parameter is an LPCTSTR (i.e. it's const), so just pass L"UUID".
- unsigned char __RPC_FAR *rs;
From my bit of searching around, it looks like __RPC_FAR is unnecessary, so just remove it.
http://www.tech-archive.net/Archive/VC/microsoft.public.vc.language/2005-11/...
- long lSuccess = RegOpenKey(HKEY_LOCAL_MACHINE,
L"SYSTEM\\CurrentControlSet\\services\\Matahari",
+&hKey);
- if (lSuccess != ERROR_SUCCESS) {
mh_debug("Could not open Matahari key from the registry: %ld",
lSuccess);
goto bail;
- }
- lSuccess = RegQueryValueEx(hKey, szValue, NULL, NULL, (LPBYTE) szData,
+&nSize);
- if (lSuccess == ERROR_SUCCESS) {
wcstombs(s, szData, (size_t) MAXUUIDLEN);
We should probably check the return code to make sure the conversion succeeded. And to make sure it does succeed, we also need to null-terminate szData because it apparently may come out of the registry with a null terminator :(
return s;
- }
- UuidCreate(&u);
- if (UuidToStringA(&u,&rs) == RPC_S_OK) {
strncpy(s,(char __RPC_FAR *)rs, MAXUUIDLEN);
s[MAXUUIDLEN - 1] = '\0';
I would prefer using sizeof(s) instead of MAXUUIDLEN here.
I don't actually have a problem with MAXUUIDLEN myself, but DIMOF() is better than sizeof here. Especially in a function where you have both char and wchar_t arrays, it would be easy to accidentally use sizeof on a wide-char array.
RpcStringFreeA(&rs);
mh_trace("Got uuid: %s", s);
lSuccess = RegSetValueEx(hKey, szValue, 0, REG_SZ, (CONST BYTE *)char2wide(s), strlen(s) * sizeof(wchar_t *));
A few problems here 1) s is technically a multi-byte string, so you can't necessarily get the length of the corresponding wide-char string by multiplying by sizeof(wchar_t). (I see you've already fixed this one in v3 of the patch.) 2) You really wanted sizeof(wchar_t), not sizeof(wchar_t *). 3) You need (wcslen() + 1) to ensure that the null terminator is stored.
There is a memory leak here. char2wide() returns mallocd memory that will need to be freed.
It's probably easier to do everything with wide chars (i.e. use UUIDToString() instead of UIUDToStringA()), then you need only do one conversion (wcstombs() just before you return the result).
As an aside, the tricky thing about design is that a change to any part of it potentially requires re-examination of the assumptions behind any other part. We use abstraction and loose coupling to try to limit the scope of this re-examination, but we still need to do it to ensure that entropy is minimised. (Minimising entropy is, at some level, the point of design.)
e.g. in this case the use of the ANSI version of UuidCreate() was a good design choice at the time, but it made the code more complicated when the value also had to be stored as Unicode in the registry.
...Actually, on closer inspection there are also ANSI RegQueryValueExA()/RegSetValueExA() functions that will do the conversions for you. That's probably even easier.
if (lSuccess != ERROR_SUCCESS) {
goto bail;
}
- }
- return s;
- bail:
- return "";
+} \ No newline at end of file
If there's a way to set up your editor to always put a newline at the end of a file, that would be good.
cheers, Zane.