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

["Daniel P. Berrangé" <berrange@redhat.com>]

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 :|