I think Alin posted another patch with a format message. Would you like me to address your comments from that email(attached) and post a final patch?
Saurabh From: Ben Pfaff <b...@nicira.com<mailto:b...@nicira.com>> Date: Wednesday, December 18, 2013 9:44 AM To: Saurabh Shah <ssaur...@vmware.com<mailto:ssaur...@vmware.com>> Cc: Alin Serdean <aserd...@cloudbasesolutions.com<mailto:aserd...@cloudbasesolutions.com>>, "shet...@nicira.com<mailto:shet...@nicira.com>" <shet...@nicira.com<mailto:shet...@nicira.com>>, "dev@openvswitch.org<mailto:dev@openvswitch.org>" <dev@openvswitch.org<mailto:dev@openvswitch.org>> Subject: Re: [ovs-dev] [Windows thread 3] Saurabh, can you post a final patch? On Fri, Dec 13, 2013 at 11:18:17AM -0800, Saurabh Shah wrote: I don't have a strong preference about the structuring. Anything that is intuitive is fine with me. With regards to keeping all pragma comments separate, I actually like keeping them close to the files that use them. However, since these are DLLs, I don't see any major downside in declaring them in a separate file (except that the pragma declare file might get out-of-sync with the actual libraries used by the code). Thanks! Saurabh From: Alin Serdean <aserd...@cloudbasesolutions.com<mailto:aserd...@cloudbasesolutions.com><mailto:aserd...@cloudbasesolutions.com>> Date: Friday, December 13, 2013 10:28 AM To: Saurabh Shah <ssaur...@vmware.com<mailto:ssaur...@vmware.com><mailto:ssaur...@vmware.com>>, "b...@nicira.com<mailto:b...@nicira.com><mailto:b...@nicira.com>" <b...@nicira.com<mailto:b...@nicira.com><mailto:b...@nicira.com>>, "shet...@nicira.com<mailto:shet...@nicira.com><mailto:shet...@nicira.com>" <shet...@nicira.com<mailto:shet...@nicira.com><mailto:shet...@nicira.com>> Cc: "dev@openvswitch.org<mailto:dev@openvswitch.org><mailto:dev@openvswitch.org>" <dev@openvswitch.org<mailto:dev@openvswitch.org><mailto:dev@openvswitch.org>> Subject: RE: [ovs-dev] [Windows thread 3] I was thinking to include all pragmas and other needed defines in a common header used through all of the source files(https://urldefense.proofpoint.com/v1/url?u=http://openvswitch.org/pipermail/dev/2013-December/034983.html&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=pEkjsHfytvHEWufeZPpgqSOJMdMjuZPbesVsNhCUc0E%3D%0A&m=obniPhCF7XqjNymb0SjBmKTiXpqhZiD%2BcTIUHWK2nQU%3D%0A&s=869fb509740d389ad9737b32b07160a8057a9bf3ab5b61125f4642f060379acf), but this is just my thought. The way you want to structure it is up too you :-). Alin. ________________________________ From: Saurabh Shah [ssaur...@vmware.com<mailto:ssaur...@vmware.com><mailto:ssaur...@vmware.com>] Sent: Friday, December 13, 2013 2:41 AM To: Alin Serdean; b...@nicira.com<mailto:b...@nicira.com><mailto:b...@nicira.com>; shet...@nicira.com<mailto:shet...@nicira.com><mailto:shet...@nicira.com> Cc: dev@openvswitch.org<mailto:dev@openvswitch.org><mailto:dev@openvswitch.org> Subject: Re: [ovs-dev] [Windows thread 3] Looks good to me. Just one minor comment. You probably also want to include the following? #include <windows.h> #pragma comment(lib, "advapi32.lib") Btw, printing the error message will require a small change which maps strerror_r to strerror_s. Not sure, if we should just add it to this commit. Saurabh From: Alin Serdean <aserd...@cloudbasesolutions.com<mailto:aserd...@cloudbasesolutions.com><mailto:aserd...@cloudbasesolutions.com>> Date: Thursday, December 12, 2013 1:59 PM To: Saurabh Shah <ssaur...@vmware.com<mailto:ssaur...@vmware.com><mailto:ssaur...@vmware.com>>, "b...@nicira.com<mailto:b...@nicira.com><mailto:b...@nicira.com>" <b...@nicira.com<mailto:b...@nicira.com><mailto:b...@nicira.com>>, "shet...@nicira.com<mailto:shet...@nicira.com><mailto:shet...@nicira.com>" <shet...@nicira.com<mailto:shet...@nicira.com><mailto:shet...@nicira.com>> Cc: "dev@openvswitch.org<mailto:dev@openvswitch.org><mailto:dev@openvswitch.org>" <dev@openvswitch.org<mailto:dev@openvswitch.org><mailto:dev@openvswitch.org>> Subject: RE: [ovs-dev] [Windows thread 3] Updated. Thanks for the feedback :-). Final version would look something like: lib/entropy.c | 13 +++++++++++++ 1 files changed, 13 insertions(+), 0 deletions(-) --- diff --git a/lib/entropy.c b/lib/entropy.c index 02f56e0..45e83ec 100644 --- a/lib/entropy.c +++ b/lib/entropy.c @@ -33,6 +33,7 @@ static const char urandom[] = "/dev/urandom"; int get_entropy(void *buffer, size_t n) { +#ifndef _WIN32 size_t bytes_read; int error; int fd; @@ -49,6 +50,18 @@ get_entropy(void *buffer, size_t n) if (error) { VLOG_ERR("%s: read error (%s)", urandom, ovs_retval_to_string(error)); } +#else + int error = 0; + HCRYPTPROV crypt_prov = 0; + CryptAcquireContext(&crypt_prov, NULL, NULL, PROV_RSA_FULL, CRYPT_VERIFYCONTEXT); + + if (!CryptGenRandom(crypt_prov, n, buffer)) { + error = GetLastError(); + VLOG_ERR("CryptGenRandom: read error (%s)", ovs_retval_to_string(error)); + } + + CryptReleaseContext(crypt_prov, 0); +#endif return error; } --- If there are no further remarks I would post in a patch. Alin. ________________________________ From: Saurabh Shah [ssaur...@vmware.com<mailto:ssaur...@vmware.com><mailto:ssaur...@vmware.com>] Sent: Wednesday, December 11, 2013 9:53 PM To: Alin Serdean; b...@nicira.com<mailto:b...@nicira.com><mailto:b...@nicira.com>; shet...@nicira.com<mailto:shet...@nicira.com><mailto:shet...@nicira.com> Cc: dev@openvswitch.org<mailto:dev@openvswitch.org><mailto:dev@openvswitch.org> Subject: Re: [ovs-dev] [Windows thread 3] Hey, The following is a quick patch for secure pseudorandom number generator on windows. I split the functionality with a brutal ifdef macro. Feedback on the code and suggestions for a nicer implementation is appreciated :). diff --git a/lib/entropy.c b/lib/entropy.c index 02f56e0..ec9d95c 100644 --- a/lib/entropy.c +++ b/lib/entropy.c @@ -20,6 +20,9 @@ #include <errno.h> #include <fcntl.h> #include <unistd.h> +#ifdef _WIN32 +#include <Wincrypt.h> +#endif #include "socket-util.h" #include "vlog.h" @@ -33,6 +36,7 @@ static const char urandom[] = "/dev/urandom"; int get_entropy(void *buffer, size_t n) { +#ifndef _WIN32 size_t bytes_read; int error; int fd; @@ -49,6 +53,20 @@ get_entropy(void *buffer, size_t n) if (error) { VLOG_ERR("%s: read error (%s)", urandom, ovs_retval_to_string(error)); } +#else + int error = 1; + HCRYPTPROV crypt_prov = 0; + CryptAcquireContext(&crypt_prov, NULL, NULL, PROV_RSA_FULL, 0); + Microsoft documentation suggests using CRYPT_VERIFYCONTEXT. Although, I haven't tested to see what sort of an impact this will have. https://urldefense.proofpoint.com/v1/url?u=http://msdn.microsoft.com/en-us/library/windows/desktop/aa379886%28v%3Dvs.85%29.aspx&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=pEkjsHfytvHEWufeZPpgqSOJMdMjuZPbesVsNhCUc0E%3D%0A&m=obniPhCF7XqjNymb0SjBmKTiXpqhZiD%2BcTIUHWK2nQU%3D%0A&s=5c3176f7bb7078dd1b46269b0818fa74c6aa6c3d6b7937f7b4481f2f19e6d857<https://urldefense.proofpoint.com/v1/url?u=http://msdn.microsoft.com/en-us/library/windows/desktop/aa379886(v%3Dvs.85).aspx&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=pEkjsHfytvHEWufeZPpgqSOJMdMjuZPbesVsNhCUc0E%3D%0A&m=zoQn%2BOKvRPDqVVu5wfXEQ7LzlyDiCzTd%2BoRL3B70vuQ%3D%0A&s=bf3a9912332ddbdb2c2109e0613776296f4ecc3c50b51a489f75b67da27a015f> For performance reasons, we recommend that you set the pszContainer parameter to NULL and the dwFlags parameter to CRYPT_VERIFYCONTEXT in all situations where you do not require a persisted key. In particular, consider setting the pszContainer parameter to NULL and the dwFlags parameter to CRYPT_VERIFYCONTEXT for the following scenarios: + if (CryptGenRandom(crypt_prov, n, buffer)) { + error = 0; + } + + if (error) { + VLOG_ERR("CryptGenRandom: read error (%s)", urandom, ovs_retval_to_string(error)); + } How about doing instead - int error = 0; If (! CryptGetRandom(crypt_prov, n, buffer)) { error = GetLastError(); VLOG_ERR("CryptGenRandom: read error (%s)", urandom, ovs_retval_to_string(error)); } + CryptReleaseContext(crypt_prov, 0); +#endif return error; } Kind Regards, Alin. _______________________________________________ dev mailing list dev@openvswitch.org<mailto:dev@openvswitch.org><mailto:dev@openvswitch.org> https://urldefense.proofpoint.com/v1/url?u=http://openvswitch.org/mailman/listinfo/dev&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=pEkjsHfytvHEWufeZPpgqSOJMdMjuZPbesVsNhCUc0E%3D%0A&m=KlUcJXE7spv5Cm%2FmexYFbql6rLI%2BJfpjXWgtb05Lero%3D%0A&s=ccb6c3872370fa5ee60d07509dba3d0a07ebc526274c18553e02ab434de8bcdb
--- Begin Message ---On Mon, Dec 16, 2013 at 05:13:59PM +0000, Alin Serdean wrote: > I can put in the ernno for the error but the problem is CryptGenRandom > does not set the error number. OK. > So it is either just print the value of GetLastError() or I use > FormatMessage function to get the text from that value. Like the > following: I think it would be better to format the error. > + if (!CryptGenRandom(crypt_prov, n, buffer)) { > + error = GetLastError(); > + LPVOID msg_buf; Please put declarations before statements within a block, as CodingStyle requires. > + FormatMessage( FORMAT_MESSAGE_ALLOCATE_BUFFER | > + FORMAT_MESSAGE_FROM_SYSTEM | > + FORMAT_MESSAGE_IGNORE_INSERTS, > + NULL, > + error, > + 0, > + (LPTSTR)&msg_buf, > + 0, > + NULL > + ); > + VLOG_ERR("CryptGenRandom: read error (%s)", msg_buf); > + LocalFree(msg_buf); The caller still needs an errno value, so probably one should pick some reasonable value (e.g. EINVAL?) here and use that one. > + } > + > + CryptReleaseContext(crypt_prov, 0); > +#endif > return error; > } > > Would you like me to set up a helper function like > ovs_retval_to_string (i.e. ovs_getlasterror_to_string) or just leave > it the way it is for the moment? I'm happy either way for the moment, but I guess this will come up again and then a helper would be a good idea. Thanks, Ben. _______________________________________________ dev mailing list dev@openvswitch.org https://urldefense.proofpoint.com/v1/url?u=http://o penvswitch.org/mailman/listinfo/dev&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=pE kjsHfytvHEWufeZPpgqSOJMdMjuZPbesVsNhCUc0E%3D%0A&m=zS3yhoaujMjhn4kPvzZYM29dkq ckKMOPxhEFExRr%2BOs%3D%0A&s=e3ba9b6a0795f73a24201da6427c5a95583a9a108c815c49 876a5483b5d7439d
--- End Message ---
_______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
