Re: [PATCH v9 04/26] win32: Simplify gmtime_r detection direct base on _POSIX_THREAD_SAFE_FUNCTIONS.

[Daniel P . Berrangé]

On Tue, Sep 15, 2020 at 08:12:56PM +0800, Yonggang Luo wrote:
> First, this reduce the size of configure, configure are tending to removal in 
> future,
> and this didn't introduce any new feature or remove any exist feature.
> Second, the current localtime_r detection are conflict with ncursesw 
> detection in
> mingw, when ncursesw detected, it will provide the following compile flags
> pkg-config --cflags ncursesw
> -D_XOPEN_SOURCE=600 -D_POSIX_C_SOURCE=199506L 
> -IC:/CI-Tools/msys64/mingw64/include/ncursesw
> And the compile flag _POSIX_C_SOURCE will always cause 
> _POSIX_THREAD_SAFE_FUNCTIONS to
> be defined, in new version of mingw, that's will cause localtime_r to be 
> defined.
> But the configure script didn't provide _POSIX_C_SOURCE macro, and that's 
> will result
> localtime_r not detected because localtime_r are defined in forceinline 
> manner.

ncursesw is just one of the three curses impls we can select for
building against, so it doesn't feel right to make an assumption
that _POSIX_C_SOURCE is always defined.

> 
> And finally cause conflict between QEMU defined localtime_r
> struct tm *localtime_r(const time_t *timep, struct tm *result);
> with mingw defined localtime_r
> 
> ```
> #if defined(_POSIX_C_SOURCE) && !defined(_POSIX_THREAD_SAFE_FUNCTIONS)
> #define _POSIX_THREAD_SAFE_FUNCTIONS 200112L
> #endif
> 
> #ifdef _POSIX_THREAD_SAFE_FUNCTIONS
> __forceinline struct tm *__CRTDECL localtime_r(const time_t *_Time, struct tm 
> *_Tm) {
>   return localtime_s(_Tm, _Time) ? NULL : _Tm;
> }
> __forceinline struct tm *__CRTDECL gmtime_r(const time_t *_Time, struct tm 
> *_Tm) {
>   return gmtime_s(_Tm, _Time) ? NULL : _Tm;
> }
> __forceinline char *__CRTDECL ctime_r(const time_t *_Time, char *_Str) {
>   return ctime_s(_Str, 0x7fffffff, _Time) ? NULL : _Str;
> }
> __forceinline char *__CRTDECL asctime_r(const struct tm *_Tm, char * _Str) {
>   return asctime_s(_Str, 0x7fffffff, _Tm) ? NULL : _Str;
> }
> #endif
> ```
> 
> So I suggest remove this configure script, and restrict msys2/mingw version 
> to easy to maintain.
> And use _POSIX_THREAD_SAFE_FUNCTIONS to guard the localtime_r and counterpart 
> functions
> 
> Signed-off-by: Yonggang Luo <luoyonggang@gmail.com>
> ---
>  configure                 | 34 ----------------------------------
>  include/sysemu/os-win32.h |  4 ++--
>  util/oslib-win32.c        |  2 +-
>  3 files changed, 3 insertions(+), 37 deletions(-)
> 
> diff --git a/configure b/configure
> index dc4b7a2e55..bac48b5b49 100755
> --- a/configure
> +++ b/configure
> @@ -2496,37 +2496,6 @@ if test "$vhost_net" = ""; then
>    test "$vhost_kernel" = "yes" && vhost_net=yes
>  fi
>  
> -##########################################
> -# MinGW / Mingw-w64 localtime_r/gmtime_r check
> -
> -if test "$mingw32" = "yes"; then
> -    # Some versions of MinGW / Mingw-w64 lack localtime_r
> -    # and gmtime_r entirely.
> -    #
> -    # Some versions of Mingw-w64 define a macro for
> -    # localtime_r/gmtime_r.
> -    #
> -    # Some versions of Mingw-w64 will define functions
> -    # for localtime_r/gmtime_r, but only if you have
> -    # _POSIX_THREAD_SAFE_FUNCTIONS defined. For fun
> -    # though, unistd.h and pthread.h both define
> -    # that for you.
> -    #
> -    # So this #undef localtime_r and #include <unistd.h>
> -    # are not in fact redundant.
> -cat > $TMPC << EOF
> -#include <unistd.h>
> -#include <time.h>
> -#undef localtime_r
> -int main(void) { localtime_r(NULL, NULL); return 0; }
> -EOF
> -    if compile_prog "" "" ; then
> -        localtime_r="yes"
> -    else
> -        localtime_r="no"
> -    fi
> -fi
> -
>  ##########################################
>  # pkg-config probe
>  
> @@ -7088,9 +7057,6 @@ if [ "$bsd" = "yes" ] ; then
>    echo "CONFIG_BSD=y" >> $config_host_mak
>  fi
>  
> -if test "$localtime_r" = "yes" ; then
> -  echo "CONFIG_LOCALTIME_R=y" >> $config_host_mak
> -fi
>  if test "$qom_cast_debug" = "yes" ; then
>    echo "CONFIG_QOM_CAST_DEBUG=y" >> $config_host_mak
>  fi
> diff --git a/include/sysemu/os-win32.h b/include/sysemu/os-win32.h
> index d8978e28c0..3ac8a53bac 100644
> --- a/include/sysemu/os-win32.h
> +++ b/include/sysemu/os-win32.h
> @@ -48,12 +48,12 @@
>  #define siglongjmp(env, val) longjmp(env, val)
>  
>  /* Missing POSIX functions. Don't use MinGW-w64 macros. */
> -#ifndef CONFIG_LOCALTIME_R
> +#ifndef _POSIX_THREAD_SAFE_FUNCTIONS
>  #undef gmtime_r
>  struct tm *gmtime_r(const time_t *timep, struct tm *result);
>  #undef localtime_r
>  struct tm *localtime_r(const time_t *timep, struct tm *result);
> -#endif /* CONFIG_LOCALTIME_R */
> +#endif
>  
>  static inline void os_setup_signal_handling(void) {}
>  static inline void os_daemonize(void) {}
> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> index c654dafd93..f2fa9a3549 100644
> --- a/util/oslib-win32.c
> +++ b/util/oslib-win32.c
> @@ -106,7 +106,7 @@ void qemu_anon_ram_free(void *ptr, size_t size)
>      }
>  }
>  
> -#ifndef CONFIG_LOCALTIME_R
> +#ifndef _POSIX_THREAD_SAFE_FUNCTIONS
>  /* FIXME: add proper locking */
>  struct tm *gmtime_r(const time_t *timep, struct tm *result)
>  {
> -- 
> 2.28.0.windows.1
> 
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|