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-...
> + 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.