| Message ID | 20170519202357.3772-1-aserdean@cloudbasesolutions.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On Fri, May 19, 2017 at 08:25:10PM +0000, Alin Serdean wrote: > getcwd - is used in lib/util.c. getcwd is deprecated on Windows but has > _getcwd which is defined in <direct.h>: > https://msdn.microsoft.com/en-us/library/sf98bd4y(v=vs.120).aspx > > getpid - is used in several files (i.e. lib/vlog.c). getpid > is also and deprecated and _getpid should be used: > https://msdn.microsoft.com/en-us/library/t2y34y40(v=vs.120).aspx > The problem using _getpid is that the definition is in <process.h>. > A file called process.h also exists in the lib folder. This will mess up > includes. > An option would be to use a wrapper like we use for lib/string.h(.in) but > that would mean to also add it to the automake chain. > A simple solution would be to map it to GetCurrentProcessId > https://msdn.microsoft.com/en-us/library/windows/desktop/ms683180(v=vs.85).aspx > > _getpid uses GetCurrentProcessId behind the scenes, casting the result > is not required. > > Signed-off-by: Alin Gabriel Serdean <aserdean@cloudbasesolutions.com> > Co-authored-by: Ben Pfaff <blp@ovn.org> > --- > v2: used an inline function for getpid to make it POSIX compatible as > suggested by Ben Pfaff > --- > include/windows/unistd.h | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/include/windows/unistd.h b/include/windows/unistd.h > index 8629f7e..513ecba 100644 > --- a/include/windows/unistd.h > +++ b/include/windows/unistd.h > @@ -17,9 +17,12 @@ > #define _UNISTD_H 1 > > #define WIN32_LEAN_AND_MEAN > +#include <config.h> > #include <windows.h> > +#include <direct.h> Thanks for the revised patch. Does #include <config.h> make a difference? Every .c file should already start out with that #include, and so if it makes a difference then it probably indicates that some .c file has forgotten it. (But the Makefile checks for that, so it is unlikely.) Thanks, Ben.
> > #define WIN32_LEAN_AND_MEAN > > +#include <config.h> > > #include <windows.h> > > +#include <direct.h> > > Thanks for the revised patch. > > Does #include <config.h> make a difference? Every .c file should already > start out with that #include, and so if it makes a difference then it probably > indicates that some .c file has forgotten it. (But the Makefile checks for that, > so it is unlikely.) > > Thanks, > > Ben. [Alin Serdean] I did a clean compile and a run of unit tests and everything was ok. I included <config.h> for https://github.com/openvswitch/ovs/blob/master/include/windows/windefs.h#L41 . Should I change <config,h> to <windefs.h> ?
On Fri, May 19, 2017 at 11:16:16PM +0000, Alin Serdean wrote: > > > #define WIN32_LEAN_AND_MEAN > > > +#include <config.h> > > > #include <windows.h> > > > +#include <direct.h> > > > > Thanks for the revised patch. > > > > Does #include <config.h> make a difference? Every .c file should already > > start out with that #include, and so if it makes a difference then it probably > > indicates that some .c file has forgotten it. (But the Makefile checks for that, > > so it is unlikely.) > > > > Thanks, > > > > Ben. > [Alin Serdean] I did a clean compile and a run of unit tests and everything was ok. > I included <config.h> for https://github.com/openvswitch/ovs/blob/master/include/windows/windefs.h#L41 . > Should I change <config,h> to <windefs.h> ? It looks like config.h always include windefs.h, on Windows. Every .c file includes config.h. Thus, there should be no need for anything else to ever include config.h or windefs.h. Right?
> On Fri, May 19, 2017 at 11:16:16PM +0000, Alin Serdean wrote: > > > > #define WIN32_LEAN_AND_MEAN > > > > +#include <config.h> > > > > #include <windows.h> > > > > +#include <direct.h> > > > > > > Thanks for the revised patch. > > > > > > Does #include <config.h> make a difference? Every .c file should > > > already start out with that #include, and so if it makes a > > > difference then it probably indicates that some .c file has > > > forgotten it. (But the Makefile checks for that, so it is > > > unlikely.) > > > > > > Thanks, > > > > > > Ben. > > [Alin Serdean] I did a clean compile and a run of unit tests and everything > was ok. > > I included <config.h> for > https://github.com/openvswitch/ovs/blob/master/include/windows/windef > s.h#L41 . > > Should I change <config,h> to <windefs.h> ? > > It looks like config.h always include windefs.h, on Windows. Every .c file > includes config.h. Thus, there should be no need for anything else to ever > include config.h or windefs.h. Right? [Alin Serdean] Yup, it makes total sense. I was a bit tired when I wrote the reply 😊.
diff --git a/include/windows/unistd.h b/include/windows/unistd.h index 8629f7e..513ecba 100644 --- a/include/windows/unistd.h +++ b/include/windows/unistd.h @@ -17,9 +17,12 @@ #define _UNISTD_H 1 #define WIN32_LEAN_AND_MEAN +#include <config.h> #include <windows.h> +#include <direct.h> #define fsync _commit +#define getcwd _getcwd /* Standard file descriptors. */ #define STDIN_FILENO 0 /* Standard input. */ @@ -33,6 +36,15 @@ #define _SC_NPROCESSORS_ONLN 0x2 #define _SC_PHYS_PAGES 0x4 + +static __inline pid_t getpid(void) +{ + /* Since _getpid: https://msdn.microsoft.com/en-us/library/t2y34y40.aspx + * uses GetCurrentProcessId behind the scenes it is safe to assume no + * casting is required */ + return GetCurrentProcessId(); +} + __inline int GetNumLogicalProcessors(void) { SYSTEM_INFO info_temp;
getcwd - is used in lib/util.c. getcwd is deprecated on Windows but has _getcwd which is defined in <direct.h>: https://msdn.microsoft.com/en-us/library/sf98bd4y(v=vs.120).aspx getpid - is used in several files (i.e. lib/vlog.c). getpid is also and deprecated and _getpid should be used: https://msdn.microsoft.com/en-us/library/t2y34y40(v=vs.120).aspx The problem using _getpid is that the definition is in <process.h>. A file called process.h also exists in the lib folder. This will mess up includes. An option would be to use a wrapper like we use for lib/string.h(.in) but that would mean to also add it to the automake chain. A simple solution would be to map it to GetCurrentProcessId https://msdn.microsoft.com/en-us/library/windows/desktop/ms683180(v=vs.85).aspx _getpid uses GetCurrentProcessId behind the scenes, casting the result is not required. Signed-off-by: Alin Gabriel Serdean <aserdean@cloudbasesolutions.com> Co-authored-by: Ben Pfaff <blp@ovn.org> --- v2: used an inline function for getpid to make it POSIX compatible as suggested by Ben Pfaff --- include/windows/unistd.h | 12 ++++++++++++ 1 file changed, 12 insertions(+)