mbox series

[v2,00/21] W32, W64 msys2/mingw patches

Message ID 20200909094617.1582-1-luoyonggang@gmail.com
Headers show
Series W32, W64 msys2/mingw patches | expand

Message

罗勇刚(Yonggang Luo) Sept. 9, 2020, 9:45 a.m. UTC 
It first introduce msys2 CI on cirrus by fixes nfs, capstone, curses and
disable partial test-char tests.
And then fixes all unit tests failure on msys2/mingw

Yonggang Luo (21):
  block: Fixes nfs compiling error on msys2/mingw
  ci: fixes msys2 build by upgrading capstone to 4.0.2
  configure: Fixes ncursesw detection under msys2/mingw and enable
    curses
  curses: Fixes curses compiling errors.
  tests: disable /char/stdio/* tests in test-char.c on win32
  ci: Enable msys2 ci in cirrus
  tests: Trying fixes test-replication.c on msys2/mingw.
  tests: test-replication disable /replication/secondary/* on
    msys2/mingw.
  osdep: These function are only available on Non-Win32 system.
  meson: Use -b to ignore CR vs. CR-LF issues on Windows
  meson: disable crypto tests are empty under win32
  meson: remove empty else and duplicated gio deps
  vmstate: Fixes test-vmstate.c on msys2/mingw
  cirrus: Building freebsd in a single short
  tests: Convert g_free to g_autofree macro in test-logging.c
  rcu: add uninit destructor for rcu
  tests: Fixes test-io-channel-socket.c tests under msys2/mingw
  tests: fixes aio-win32 about aio_remove_fd_handler, get it consistence
    with aio-posix.c
  tests: Fixes test-io-channel-file by mask only owner file state mask
    bits
  tests: fix test-util-sockets.c
  tests: Fixes test-qdev-global-props.c

 .cirrus.yml                         | 59 ++++++++++++++++-------------
 block/nfs.c                         | 26 ++++++++-----
 capstone                            |  2 +-
 configure                           | 11 ++----
 include/qemu/osdep.h                |  2 +-
 include/qemu/rcu.h                  |  5 +++
 meson.build                         |  6 ---
 scripts/ci/windows/msys2-build.sh   | 28 ++++++++++++++
 scripts/ci/windows/msys2-install.sh | 33 ++++++++++++++++
 tests/meson.build                   |  3 +-
 tests/qapi-schema/meson.build       |  2 +-
 tests/test-char.c                   |  8 ++--
 tests/test-io-channel-file.c        |  4 +-
 tests/test-io-channel-socket.c      |  2 +
 tests/test-logging.c                |  5 ++-
 tests/test-qdev-global-props.c      |  6 +--
 tests/test-replication.c            | 22 +++++++++--
 tests/test-util-sockets.c           |  6 ++-
 tests/test-vmstate.c                |  2 +-
 ui/curses.c                         | 14 +++----
 util/aio-win32.c                    | 11 +++++-
 util/rcu.c                          | 37 +++++++++++++++++-
 22 files changed, 215 insertions(+), 79 deletions(-)
 create mode 100644 scripts/ci/windows/msys2-build.sh
 create mode 100644 scripts/ci/windows/msys2-install.sh

-- 
2.28.0.windows.1

Comments

Daniel P. Berrangé Sept. 9, 2020, 12:24 p.m. UTC | #1 
On Wed, Sep 09, 2020 at 05:45:57PM +0800, Yonggang Luo wrote:
> These compiling errors are fixed:
> ../block/nfs.c:27:10: fatal error: poll.h: No such file or directory
>    27 | #include <poll.h>
>       |          ^~~~~~~~
> compilation terminated.
> 
> ../block/nfs.c:63:5: error: unknown type name 'blkcnt_t'
>    63 |     blkcnt_t st_blocks;
>       |     ^~~~~~~~
> ../block/nfs.c: In function 'nfs_client_open':
> ../block/nfs.c:550:27: error: 'struct _stat64' has no member named 'st_blocks'
>   550 |     client->st_blocks = st.st_blocks;
>       |                           ^
> ../block/nfs.c: In function 'nfs_get_allocated_file_size':
> ../block/nfs.c:751:41: error: 'struct _stat64' has no member named 'st_blocks'
>   751 |     return (task.ret < 0 ? task.ret : st.st_blocks * 512);
>       |                                         ^
> ../block/nfs.c: In function 'nfs_reopen_prepare':
> ../block/nfs.c:805:31: error: 'struct _stat64' has no member named 'st_blocks'
>   805 |         client->st_blocks = st.st_blocks;
>       |                               ^
> ../block/nfs.c: In function 'nfs_get_allocated_file_size':
> ../block/nfs.c:752:1: error: control reaches end of non-void function [-Werror=return-type]
>   752 | }
>       | ^
> 
> On msys2/mingw, there is no st_blocks in struct _stat64, so we use consistence st_size instead.
> 
> Signed-off-by: Yonggang Luo <luoyonggang@gmail.com>
> ---
>  block/nfs.c | 26 +++++++++++++++++---------
>  1 file changed, 17 insertions(+), 9 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
Daniel P. Berrangé Sept. 9, 2020, 12:53 p.m. UTC | #2 
On Wed, Sep 09, 2020 at 05:46:02PM +0800, Yonggang Luo wrote:
> Install msys2 in a proper way refer to https://github.com/cirruslabs/cirrus-ci-docs/issues/699
> The https://wiki.qemu.org/Hosts/W32#Native_builds_with_MSYS2 need to be updated.
> There is no need of --cross-prefix, open mingw64.exe instead of msys2.exe then we don't
> need the --cross-prefix, besides we using environment variable settings:
>     MSYS: winsymlinks:nativestrict
>     MSYSTEM: MINGW64
>     CHERE_INVOKING: 1
> to opening mingw64 native shell.
> We now running tests with make -i check to skip tests errors.
> 
> Signed-off-by: Yonggang Luo <luoyonggang@gmail.com>
> ---
>  .cirrus.yml                         | 24 +++++++++++++++++++++
>  scripts/ci/windows/msys2-build.sh   | 28 ++++++++++++++++++++++++
>  scripts/ci/windows/msys2-install.sh | 33 +++++++++++++++++++++++++++++
>  3 files changed, 85 insertions(+)
>  create mode 100644 scripts/ci/windows/msys2-build.sh
>  create mode 100644 scripts/ci/windows/msys2-install.sh
> 
> diff --git a/.cirrus.yml b/.cirrus.yml
> index 3dd9fcff7f..49335e68c9 100644
> --- a/.cirrus.yml
> +++ b/.cirrus.yml
> @@ -63,3 +63,27 @@ macos_xcode_task:
>                     --enable-werror --cc=clang || { cat config.log; exit 1; }
>      - gmake -j$(sysctl -n hw.ncpu)
>      - gmake check
> +
> +windows_msys2_task:
> +  windows_container:
> +    image: cirrusci/windowsservercore:cmake
> +    os_version: 2019
> +    cpu: 8
> +    memory: 8G
> +  env:
> +    MSYS: winsymlinks:nativestrict
> +    MSYSTEM: MINGW64
> +    CHERE_INVOKING: 1
> +  printenv_script:
> +    - C:\tools\msys64\usr\bin\bash.exe -lc 'printenv'
> +  install_script:
> +    - C:\tools\msys64\usr\bin\bash.exe -lc "cd /c/tools && curl -O http://repo.msys2.org/msys/x86_64/msys2-keyring-r21.b39fb11-1-any.pkg.tar.xz"
> +    - C:\tools\msys64\usr\bin\bash.exe -lc "cd /c/tools && curl -O http://repo.msys2.org/msys/x86_64/msys2-keyring-r21.b39fb11-1-any.pkg.tar.xz.sig"
> +    - C:\tools\msys64\usr\bin\bash.exe -lc "cd /c/tools && pacman -U --noconfirm msys2-keyring-r21.b39fb11-1-any.pkg.tar.xz"
> +    - C:\tools\msys64\usr\bin\bash.exe -lc "pacman -Sy --noconfirm"
> +    - C:\tools\msys64\usr\bin\bash.exe -lc "pacman --needed --noconfirm -S bash pacman pacman-mirrors msys2-runtime"
> +    - taskkill /F /IM gpg-agent.exe
> +    - C:\tools\msys64\usr\bin\bash.exe -lc "pacman --noconfirm -Su"
> +    - C:\tools\msys64\usr\bin\bash.exe -lc "sh scripts/ci/windows/msys2-install.sh"
> +  script:
> +    - C:\tools\msys64\usr\bin\bash.exe -lc "sh scripts/ci/windows/msys2-build.sh"
> diff --git a/scripts/ci/windows/msys2-build.sh b/scripts/ci/windows/msys2-build.sh
> new file mode 100644
> index 0000000000..d9d046b5b0
> --- /dev/null
> +++ b/scripts/ci/windows/msys2-build.sh
> @@ -0,0 +1,28 @@
> +mkdir build
> +cd build
> +../configure \
> +--python=python3 \
> +--ninja=ninja \
> +--enable-stack-protector \
> +--enable-guest-agent \
> +--disable-pie \
> +--enable-gnutls --enable-nettle \
> +--enable-sdl --enable-sdl-image --enable-gtk --disable-vte --enable-curses --enable-iconv \
> +--enable-vnc --enable-vnc-sasl --enable-vnc-jpeg --enable-vnc-png \
> +--enable-slirp=git \
> +--disable-brlapi --enable-curl \
> +--enable-fdt \
> +--disable-kvm --enable-hax --enable-whpx \
> +--enable-libnfs --enable-libusb --enable-live-block-migration --enable-usb-redir \
> +--enable-lzo --enable-snappy --enable-bzip2 --enable-zstd \
> +--enable-membarrier --enable-coroutine-pool \
> +--enable-libssh --enable-libxml2 \
> +--enable-jemalloc --enable-avx2 \
> +--enable-replication \
> +--enable-tools \
> +--enable-bochs --enable-cloop --enable-dmg --enable-qcow1 --enable-vdi --enable-vvfat --enable-qed --enable-parallels \
> +--enable-sheepdog \
> +--enable-capstone=git
> +
> +make -j$NUMBER_OF_PROCESSORS
> +make -i -j$NUMBER_OF_PROCESSORS check

This still needs the changes discussed in v1 to remove all the
configure args and move the commands into the main cirrus.yml


Regards,
Daniel
罗勇刚(Yonggang Luo) Sept. 9, 2020, 12:55 p.m. UTC | #3 
On Wed, Sep 9, 2020 at 8:51 PM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Wed, Sep 09, 2020 at 05:45:59PM +0800, Yonggang Luo wrote:
> > The mingw pkg-config are showing following absolute path and contains :
> as the separator,
> > so we must handling : properly.
> >
> > -D_XOPEN_SOURCE=600 -D_POSIX_C_SOURCE=199506L
> -IC:/CI-Tools/msys64/mingw64/include/ncursesw:-I/usr/include/ncursesw:
> > -DNCURSES_WIDECHAR -D_XOPEN_SOURCE=600 -D_POSIX_C_SOURCE=199506L -IC
> -pipe -lncursesw -lgnurx -ltre -lintl -liconv
> > -DNCURSES_WIDECHAR -D_XOPEN_SOURCE=600 -D_POSIX_C_SOURCE=199506L -IC
> -lncursesw
> > -DNCURSES_WIDECHAR -D_XOPEN_SOURCE=600 -D_POSIX_C_SOURCE=199506L -IC
> -lcursesw
> > -DNCURSES_WIDECHAR /CI-Tools/msys64/mingw64/include/ncursesw -pipe
> -lncursesw -lgnurx -ltre -lintl -liconv
> > -DNCURSES_WIDECHAR /CI-Tools/msys64/mingw64/include/ncursesw -lncursesw
> > -DNCURSES_WIDECHAR /CI-Tools/msys64/mingw64/include/ncursesw -lcursesw
> > -DNCURSES_WIDECHAR -I/usr/include/ncursesw -pipe -lncursesw -lgnurx
> -ltre -lintl -liconv
> > -DNCURSES_WIDECHAR -I/usr/include/ncursesw -lncursesw
> > -DNCURSES_WIDECHAR -I/usr/include/ncursesw -lcursesw
> >
> > msys2/mingw lacks the POSIX-required langinfo.h.
> >
> > gcc test.c -DNCURSES_WIDECHAR -I/mingw64/include/ncursesw -pipe
> -lncursesw -lgnurx -ltre -lintl -liconv
> > test.c:4:10: fatal error: langinfo.h: No such file or directory
> >     4 | #include <langinfo.h>
> >       |          ^~~~~~~~~~~~
> > compilation terminated.
> >
> > So we using g_get_codeset instead of nl_langinfo(CODESET)
> >
> > Signed-off-by: Yonggang Luo <luoyonggang@gmail.com>
> > Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>
> > ---
> >  configure   |  9 +++------
> >  ui/curses.c | 10 +++++-----
> >  2 files changed, 8 insertions(+), 11 deletions(-)
> >
> > diff --git a/configure b/configure
> > index f4f8bc3756..2e6d54e15b 100755
> > --- a/configure
> > +++ b/configure
> > @@ -3653,8 +3653,8 @@ if test "$iconv" = "no" ; then
> >  fi
> >  if test "$curses" != "no" ; then
> >    if test "$mingw32" = "yes" ; then
> > -    curses_inc_list="$($pkg_config --cflags ncurses 2>/dev/null):"
> > -    curses_lib_list="$($pkg_config --libs ncurses
> 2>/dev/null):-lpdcurses"
> > +    curses_inc_list="$($pkg_config --cflags ncursesw
> 2>/dev/null):-I/${MSYSTEM,,}/include/ncursesw:"
> > +    curses_lib_list="$($pkg_config --libs ncursesw
> 2>/dev/null):-lncursesw"
>
> The original code would try  ncurses via pkg-config and if that failed,
> would
> falback to pdcurses.
>
> The new code tries ncursesw via pkg-config and then tries ncursesw again
> via manually specified args, and doesn't try  ncurses or pdcurses at all.
>
Gotcha, Indeed   $pkg_config --cflags ncurses can find curses on mingw32,
the problem is onw mingw32 the include path
have :, so we can not use : as the path sepaerator, for cross-paltform
reason, which is best for path separator?

>
> This fixes your build as you've installed ncursesw, but breaks anyone
> who was using ncurses or pdcurses previously. At least on Fedora only
> pdcurses is available for mingw, so I don't think we should be dropping
> this. It looks like we just need to check all three of ncursesw, ncurses
> and pdcurses.
>
> Copying Samuel who introduced this logic originally in
> commit 8ddc5bf9e5de51c2a4842c01dd3a97f5591776fd
>
> >    else
> >      curses_inc_list="$($pkg_config --cflags ncursesw
> 2>/dev/null):-I/usr/include/ncursesw:"
> >      curses_lib_list="$($pkg_config --libs ncursesw
> 2>/dev/null):-lncursesw:-lcursesw"
> > @@ -3664,17 +3664,14 @@ if test "$curses" != "no" ; then
> >  #include <locale.h>
> >  #include <curses.h>
> >  #include <wchar.h>
> > -#include <langinfo.h>
> >  int main(void) {
> > -  const char *codeset;
> >    wchar_t wch = L'w';
> >    setlocale(LC_ALL, "");
> >    resize_term(0, 0);
> >    addwstr(L"wide chars\n");
> >    addnwstr(&wch, 1);
> >    add_wch(WACS_DEGREE);
> > -  codeset = nl_langinfo(CODESET);
> > -  return codeset != 0;
> > +  return 0;
> >  }
> >  EOF
> >    IFS=:
> > diff --git a/ui/curses.c b/ui/curses.c
> > index a59b23a9cf..12bc682cf9 100644
> > --- a/ui/curses.c
> > +++ b/ui/curses.c
> > @@ -30,7 +30,6 @@
> >  #endif
> >  #include <locale.h>
> >  #include <wchar.h>
> > -#include <langinfo.h>
> >  #include <iconv.h>
> >
> >  #include "qapi/error.h"
> > @@ -526,6 +525,7 @@ static void font_setup(void)
> >      iconv_t nativecharset_to_ucs2;
> >      iconv_t font_conv;
> >      int i;
> > +    g_autofree gchar *local_codeset = g_get_codeset();
> >
> >      /*
> >       * Control characters are normally non-printable, but VGA does have
> > @@ -566,14 +566,14 @@ static void font_setup(void)
> >        0x25bc
> >      };
> >
> > -    ucs2_to_nativecharset = iconv_open(nl_langinfo(CODESET), "UCS-2");
> > +    ucs2_to_nativecharset = iconv_open(local_codeset, "UCS-2");
> >      if (ucs2_to_nativecharset == (iconv_t) -1) {
> >          fprintf(stderr, "Could not convert font glyphs from UCS-2:
> '%s'\n",
> >                          strerror(errno));
> >          exit(1);
> >      }
> >
> > -    nativecharset_to_ucs2 = iconv_open("UCS-2", nl_langinfo(CODESET));
> > +    nativecharset_to_ucs2 = iconv_open("UCS-2", local_codeset);
> >      if (nativecharset_to_ucs2 == (iconv_t) -1) {
> >          iconv_close(ucs2_to_nativecharset);
> >          fprintf(stderr, "Could not convert font glyphs to UCS-2:
> '%s'\n",
> > @@ -581,7 +581,7 @@ static void font_setup(void)
> >          exit(1);
> >      }
> >
> > -    font_conv = iconv_open(nl_langinfo(CODESET), font_charset);
> > +    font_conv = iconv_open(local_codeset, font_charset);
> >      if (font_conv == (iconv_t) -1) {
> >          iconv_close(ucs2_to_nativecharset);
> >          iconv_close(nativecharset_to_ucs2);
> > @@ -602,7 +602,7 @@ static void font_setup(void)
> >      /* DEL */
> >      convert_ucs(0x7F, 0x2302, ucs2_to_nativecharset);
> >
> > -    if (strcmp(nl_langinfo(CODESET), "UTF-8")) {
> > +    if (strcmp(local_codeset, "UTF-8")) {
> >          /* Non-Unicode capable, use termcap equivalents for those
> available */
> >          for (i = 0; i <= 0xFF; i++) {
> >              wchar_t wch[CCHARW_MAX];
> > --
> > 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 :|
>
>
Daniel P. Berrangé Sept. 9, 2020, 12:59 p.m. UTC | #4 
On Wed, Sep 09, 2020 at 05:46:06PM +0800, Yonggang Luo wrote:
> On windows, a difference in line endings causes testsuite failures
> complaining that every single line in files such as
> 'tests/qapi-schemadoc-good.texi' is wrong.  Fix it by adding -b to diff.
> 
> Signed-off-by: Yonggang Luo <luoyonggang@gmail.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  tests/qapi-schema/meson.build | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
Daniel P. Berrangé Sept. 9, 2020, 1 p.m. UTC | #5 
On Wed, Sep 09, 2020 at 05:46:08PM +0800, Yonggang Luo wrote:
> Signed-off-by: Yonggang Luo <luoyonggang@gmail.com>
> ---
>  meson.build | 6 ------
>  1 file changed, 6 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
Daniel P. Berrangé Sept. 9, 2020, 1:08 p.m. UTC | #6 
On Wed, Sep 09, 2020 at 05:46:11PM +0800, Yonggang Luo wrote:
> g_autofree are prefer than g_free when possible.
> 
> Signed-off-by: Yonggang Luo <luoyonggang@gmail.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  tests/test-logging.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
Daniel P. Berrangé Sept. 9, 2020, 1:10 p.m. UTC | #7 
On Wed, Sep 09, 2020 at 05:46:15PM +0800, Yonggang Luo wrote:
> This is the error on msys2/mingw
> Running test test-io-channel-file
> **
> ERROR:../tests/test-io-channel-file.c:59:test_io_channel_file_helper: assertion failed (TEST_MASK & ~mask == st.st_mode & 0777): (384 == 438)
> ERROR test-io-channel-file - Bail out! ERROR:../tests/test-io-channel-file.c:59:test_io_channel_file_helper: assertion failed (TEST_MASK & ~mask == st.st_mode & 0777): (384 == 438)
> 
> Signed-off-by: Yonggang Luo <luoyonggang@gmail.com>
> ---
>  tests/test-io-channel-file.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/test-io-channel-file.c b/tests/test-io-channel-file.c
> index bac2b07562..75aea6450a 100644
> --- a/tests/test-io-channel-file.c
> +++ b/tests/test-io-channel-file.c
> @@ -56,7 +56,9 @@ static void test_io_channel_file_helper(int flags)
>      umask(mask);
>      ret = stat(TEST_FILE, &st);
>      g_assert_cmpint(ret, >, -1);
> -    g_assert_cmpuint(TEST_MASK & ~mask, ==, st.st_mode & 0777);
> +    /* On Windows the stat() function in the C library checks only
> +     the FAT-style READONLY attribute and does not look at the ACL at all. */
> +    g_assert_cmpuint(TEST_MASK & ~mask, ==, st.st_mode & 0700);

I think we will want the stronger check on non-Win32, so better to
ifdef this to use 0700 only on Win32.


Regards,
Daniel
Daniel P. Berrangé Sept. 9, 2020, 1:22 p.m. UTC | #8 
On Wed, Sep 09, 2020 at 08:55:15PM +0800, 罗勇刚(Yonggang Luo) wrote:
> On Wed, Sep 9, 2020 at 8:51 PM Daniel P. Berrangé <berrange@redhat.com>
> wrote:
> 
> > On Wed, Sep 09, 2020 at 05:45:59PM +0800, Yonggang Luo wrote:
> > > The mingw pkg-config are showing following absolute path and contains :
> > as the separator,
> > > so we must handling : properly.
> > >
> > > -D_XOPEN_SOURCE=600 -D_POSIX_C_SOURCE=199506L
> > -IC:/CI-Tools/msys64/mingw64/include/ncursesw:-I/usr/include/ncursesw:
> > > -DNCURSES_WIDECHAR -D_XOPEN_SOURCE=600 -D_POSIX_C_SOURCE=199506L -IC
> > -pipe -lncursesw -lgnurx -ltre -lintl -liconv
> > > -DNCURSES_WIDECHAR -D_XOPEN_SOURCE=600 -D_POSIX_C_SOURCE=199506L -IC
> > -lncursesw
> > > -DNCURSES_WIDECHAR -D_XOPEN_SOURCE=600 -D_POSIX_C_SOURCE=199506L -IC
> > -lcursesw
> > > -DNCURSES_WIDECHAR /CI-Tools/msys64/mingw64/include/ncursesw -pipe
> > -lncursesw -lgnurx -ltre -lintl -liconv
> > > -DNCURSES_WIDECHAR /CI-Tools/msys64/mingw64/include/ncursesw -lncursesw
> > > -DNCURSES_WIDECHAR /CI-Tools/msys64/mingw64/include/ncursesw -lcursesw
> > > -DNCURSES_WIDECHAR -I/usr/include/ncursesw -pipe -lncursesw -lgnurx
> > -ltre -lintl -liconv
> > > -DNCURSES_WIDECHAR -I/usr/include/ncursesw -lncursesw
> > > -DNCURSES_WIDECHAR -I/usr/include/ncursesw -lcursesw
> > >
> > > msys2/mingw lacks the POSIX-required langinfo.h.
> > >
> > > gcc test.c -DNCURSES_WIDECHAR -I/mingw64/include/ncursesw -pipe
> > -lncursesw -lgnurx -ltre -lintl -liconv
> > > test.c:4:10: fatal error: langinfo.h: No such file or directory
> > >     4 | #include <langinfo.h>
> > >       |          ^~~~~~~~~~~~
> > > compilation terminated.
> > >
> > > So we using g_get_codeset instead of nl_langinfo(CODESET)
> > >
> > > Signed-off-by: Yonggang Luo <luoyonggang@gmail.com>
> > > Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>
> > > ---
> > >  configure   |  9 +++------
> > >  ui/curses.c | 10 +++++-----
> > >  2 files changed, 8 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/configure b/configure
> > > index f4f8bc3756..2e6d54e15b 100755
> > > --- a/configure
> > > +++ b/configure
> > > @@ -3653,8 +3653,8 @@ if test "$iconv" = "no" ; then
> > >  fi
> > >  if test "$curses" != "no" ; then
> > >    if test "$mingw32" = "yes" ; then
> > > -    curses_inc_list="$($pkg_config --cflags ncurses 2>/dev/null):"
> > > -    curses_lib_list="$($pkg_config --libs ncurses
> > 2>/dev/null):-lpdcurses"
> > > +    curses_inc_list="$($pkg_config --cflags ncursesw
> > 2>/dev/null):-I/${MSYSTEM,,}/include/ncursesw:"
> > > +    curses_lib_list="$($pkg_config --libs ncursesw
> > 2>/dev/null):-lncursesw"
> >
> > The original code would try  ncurses via pkg-config and if that failed,
> > would
> > falback to pdcurses.
> >
> > The new code tries ncursesw via pkg-config and then tries ncursesw again
> > via manually specified args, and doesn't try  ncurses or pdcurses at all.
> >
> Gotcha, Indeed   $pkg_config --cflags ncurses can find curses on mingw32,
> the problem is onw mingw32 the include path
> have :, so we can not use : as the path sepaerator, for cross-paltform
> reason, which is best for path separator?

I guess it was using ":" because " " might be valid in the file path.

How about using "#" or "%" instead as those should be more unlikely to
clash.


Regards,
Daniel
罗勇刚(Yonggang Luo) Sept. 9, 2020, 3:58 p.m. UTC | #9 
On Wed, Sep 9, 2020 at 9:26 PM Peter Maydell <peter.maydell@linaro.org>
wrote:

> On Wed, 9 Sep 2020 at 10:46, Yonggang Luo <luoyonggang@gmail.com> wrote:
> >
> > This is the compiling error:
> > ../ui/curses.c: In function 'curses_refresh':
> > ../ui/curses.c:256:5: error: 'next_maybe_keycode' may be used
> uninitialized in this function [-Werror=maybe-uninitialized]
> >   256 |     curses2foo(_curses2keycode, _curseskey2keycode, chr,
> maybe_keycode)
> >       |     ^~~~~~~~~~
> > ../ui/curses.c:302:32: note: 'next_maybe_keycode' was declared here
> >   302 |             enum maybe_keycode next_maybe_keycode;
> >       |                                ^~~~~~~~~~~~~~~~~~
> > ../ui/curses.c:256:5: error: 'maybe_keycode' may be used uninitialized
> in this function [-Werror=maybe-uninitialized]
> >   256 |     curses2foo(_curses2keycode, _curseskey2keycode, chr,
> maybe_keycode)
> >       |     ^~~~~~~~~~
> > ../ui/curses.c:265:24: note: 'maybe_keycode' was declared here
> >   265 |     enum maybe_keycode maybe_keycode;
> >       |                        ^~~~~~~~~~~~~
> > cc1.exe: all warnings being treated as errors
> >
> > gcc version 10.2.0 (Rev1, Built by MSYS2 project)
> >
> > Signed-off-by: Yonggang Luo <luoyonggang@gmail.com>
> > Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> > Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>
>
> I never gave this a reviewed-by tag -- can you be more careful
> with your tag handling, please?
>
Sorry, I see you replied the patch, and misunderstand as a review by

>
> thanks
> -- PMM
>
罗勇刚(Yonggang Luo) Sept. 10, 2020, 7:14 a.m. UTC | #10 
On Thu, Sep 10, 2020 at 3:01 PM Peter Lieven <pl@kamp.de> wrote:

>
>
> > Am 09.09.2020 um 11:45 schrieb Yonggang Luo <luoyonggang@gmail.com>:
> >
> > These compiling errors are fixed:
> > ../block/nfs.c:27:10: fatal error: poll.h: No such file or directory
> >   27 | #include <poll.h>
> >      |          ^~~~~~~~
> > compilation terminated.
> >
> > ../block/nfs.c:63:5: error: unknown type name 'blkcnt_t'
> >   63 |     blkcnt_t st_blocks;
> >      |     ^~~~~~~~
> > ../block/nfs.c: In function 'nfs_client_open':
> > ../block/nfs.c:550:27: error: 'struct _stat64' has no member named
> 'st_blocks'
> >  550 |     client->st_blocks = st.st_blocks;
> >      |                           ^
> > ../block/nfs.c: In function 'nfs_get_allocated_file_size':
> > ../block/nfs.c:751:41: error: 'struct _stat64' has no member named
> 'st_blocks'
> >  751 |     return (task.ret < 0 ? task.ret : st.st_blocks * 512);
> >      |                                         ^
> > ../block/nfs.c: In function 'nfs_reopen_prepare':
> > ../block/nfs.c:805:31: error: 'struct _stat64' has no member named
> 'st_blocks'
> >  805 |         client->st_blocks = st.st_blocks;
> >      |                               ^
> > ../block/nfs.c: In function 'nfs_get_allocated_file_size':
> > ../block/nfs.c:752:1: error: control reaches end of non-void function
> [-Werror=return-type]
> >  752 | }
> >      | ^
> >
> > On msys2/mingw, there is no st_blocks in struct _stat64, so we use
> consistence st_size instead.
> >
> > Signed-off-by: Yonggang Luo <luoyonggang@gmail.com>
> > ---
> > block/nfs.c | 26 +++++++++++++++++---------
> > 1 file changed, 17 insertions(+), 9 deletions(-)
> >
> > diff --git a/block/nfs.c b/block/nfs.c
> > index 61a249a9fc..34b2cd5708 100644
> > --- a/block/nfs.c
> > +++ b/block/nfs.c
> > @@ -24,7 +24,9 @@
> >
> > #include "qemu/osdep.h"
> >
> > +#if !defined(_WIN32)
> > #include <poll.h>
> > +#endif
> > #include "qemu/config-file.h"
> > #include "qemu/error-report.h"
> > #include "qapi/error.h"
> > @@ -51,6 +53,12 @@
> > #define QEMU_NFS_MAX_PAGECACHE_SIZE (8388608 / NFS_BLKSIZE)
> > #define QEMU_NFS_MAX_DEBUG_LEVEL 2
> >
> > +#if defined (_WIN32)
> > +#define nfs_stat __stat64
> > +#else
> > +#define nfs_stat stat
> > +#endif
> > +
> > typedef struct NFSClient {
> >     struct nfs_context *context;
> >     struct nfsfh *fh;
> > @@ -58,7 +66,7 @@ typedef struct NFSClient {
> >     bool has_zero_init;
> >     AioContext *aio_context;
> >     QemuMutex mutex;
> > -    blkcnt_t st_blocks;
> > +    int64_t st_size;
> >     bool cache_used;
> >     NFSServer *server;
> >     char *path;
> > @@ -70,7 +78,7 @@ typedef struct NFSRPC {
> >     int ret;
> >     int complete;
> >     QEMUIOVector *iov;
> > -    struct stat *st;
> > +    struct nfs_stat *st;
> >     Coroutine *co;
> >     NFSClient *client;
> > } NFSRPC;
> > @@ -419,7 +427,7 @@ static int64_t nfs_client_open(NFSClient *client,
> BlockdevOptionsNfs *opts,
> >                                int flags, int open_flags, Error **errp)
> > {
> >     int64_t ret = -EINVAL;
> > -    struct stat st;
> > +    struct nfs_stat st;
> >     char *file = NULL, *strp = NULL;
> >
> >     qemu_mutex_init(&client->mutex);
> > @@ -545,7 +553,7 @@ static int64_t nfs_client_open(NFSClient *client,
> BlockdevOptionsNfs *opts,
> >     }
> >
> >     ret = DIV_ROUND_UP(st.st_size, BDRV_SECTOR_SIZE);
> > -    client->st_blocks = st.st_blocks;
> > +    client->st_size = st.st_size;
> >     client->has_zero_init = S_ISREG(st.st_mode);
> >     *strp = '/';
> >     goto out;
> > @@ -729,11 +737,11 @@ static int64_t
> nfs_get_allocated_file_size(BlockDriverState *bs)
> > {
> >     NFSClient *client = bs->opaque;
> >     NFSRPC task = {0};
> > -    struct stat st;
> > +    struct nfs_stat st;
> >
> >     if (bdrv_is_read_only(bs) &&
> >         !(bs->open_flags & BDRV_O_NOCACHE)) {
> > -        return client->st_blocks * 512;
> > +        return client->st_size;
>
I am using   client->st_size instead client->st_blocks * 512, anything
wrong with this?

> >     }
> >
> >     task.bs = bs;
> > @@ -746,7 +754,7 @@ static int64_t
> nfs_get_allocated_file_size(BlockDriverState *bs)
> >     nfs_set_events(client);
> >     BDRV_POLL_WHILE(bs, !task.complete);
> >
> > -    return (task.ret < 0 ? task.ret : st.st_blocks * 512);
> > +    return (task.ret < 0 ? task.ret : st.st_size);
> > }
> >
> > static int coroutine_fn
> > @@ -778,7 +786,7 @@ static int nfs_reopen_prepare(BDRVReopenState *state,
> >                               BlockReopenQueue *queue, Error **errp)
> > {
> >     NFSClient *client = state->bs->opaque;
> > -    struct stat st;
> > +    struct nfs_stat st;
> >     int ret = 0;
> >
> >     if (state->flags & BDRV_O_RDWR && bdrv_is_read_only(state->bs)) {
> > @@ -800,7 +808,7 @@ static int nfs_reopen_prepare(BDRVReopenState *state,
> >                        nfs_get_error(client->context));
> >             return ret;
> >         }
> > -        client->st_blocks = st.st_blocks;
> > +        client->st_size = st.st_size;
> >     }
> >
> >     return 0;
> > --
> > 2.28.0.windows.1
> >
>
>
> NACK. st_blocks and st_size is not the same. st_blocks is the number of
> allocated blocks on disk. st_size is the virtual size of a file as it may
> contain holes.
> I think the appropriate fix is to not implement
> nfs_get_allocated_file_size on WIN32. Its not mandatory.
>
> Please look at the   st_size, it's equivalant to  st_blocks * 512;

Peter
>
>
>
Peter Lieven Sept. 10, 2020, 7:29 a.m. UTC | #11 
> Am 10.09.2020 um 09:14 schrieb 罗勇刚(Yonggang Luo) <luoyonggang@gmail.com>:
> 
> 
> 
> On Thu, Sep 10, 2020 at 3:01 PM Peter Lieven <pl@kamp.de> wrote:
> 
> 
> > Am 09.09.2020 um 11:45 schrieb Yonggang Luo <luoyonggang@gmail.com>:
> > 
> > These compiling errors are fixed:
> > ../block/nfs.c:27:10: fatal error: poll.h: No such file or directory
> >   27 | #include <poll.h>
> >      |          ^~~~~~~~
> > compilation terminated.
> > 
> > ../block/nfs.c:63:5: error: unknown type name 'blkcnt_t'
> >   63 |     blkcnt_t st_blocks;
> >      |     ^~~~~~~~
> > ../block/nfs.c: In function 'nfs_client_open':
> > ../block/nfs.c:550:27: error: 'struct _stat64' has no member named 'st_blocks'
> >  550 |     client->st_blocks = st.st_blocks;
> >      |                           ^
> > ../block/nfs.c: In function 'nfs_get_allocated_file_size':
> > ../block/nfs.c:751:41: error: 'struct _stat64' has no member named 'st_blocks'
> >  751 |     return (task.ret < 0 ? task.ret : st.st_blocks * 512);
> >      |                                         ^
> > ../block/nfs.c: In function 'nfs_reopen_prepare':
> > ../block/nfs.c:805:31: error: 'struct _stat64' has no member named 'st_blocks'
> >  805 |         client->st_blocks = st.st_blocks;
> >      |                               ^
> > ../block/nfs.c: In function 'nfs_get_allocated_file_size':
> > ../block/nfs.c:752:1: error: control reaches end of non-void function [-Werror=return-type]
> >  752 | }
> >      | ^
> > 
> > On msys2/mingw, there is no st_blocks in struct _stat64, so we use consistence st_size instead.
> > 
> > Signed-off-by: Yonggang Luo <luoyonggang@gmail.com>
> > ---
> > block/nfs.c | 26 +++++++++++++++++---------
> > 1 file changed, 17 insertions(+), 9 deletions(-)
> > 
> > diff --git a/block/nfs.c b/block/nfs.c
> > index 61a249a9fc..34b2cd5708 100644
> > --- a/block/nfs.c
> > +++ b/block/nfs.c
> > @@ -24,7 +24,9 @@
> > 
> > #include "qemu/osdep.h"
> > 
> > +#if !defined(_WIN32)
> > #include <poll.h>
> > +#endif
> > #include "qemu/config-file.h"
> > #include "qemu/error-report.h"
> > #include "qapi/error.h"
> > @@ -51,6 +53,12 @@
> > #define QEMU_NFS_MAX_PAGECACHE_SIZE (8388608 / NFS_BLKSIZE)
> > #define QEMU_NFS_MAX_DEBUG_LEVEL 2
> > 
> > +#if defined (_WIN32)
> > +#define nfs_stat __stat64
> > +#else
> > +#define nfs_stat stat
> > +#endif
> > +
> > typedef struct NFSClient {
> >     struct nfs_context *context;
> >     struct nfsfh *fh;
> > @@ -58,7 +66,7 @@ typedef struct NFSClient {
> >     bool has_zero_init;
> >     AioContext *aio_context;
> >     QemuMutex mutex;
> > -    blkcnt_t st_blocks;
> > +    int64_t st_size;
> >     bool cache_used;
> >     NFSServer *server;
> >     char *path;
> > @@ -70,7 +78,7 @@ typedef struct NFSRPC {
> >     int ret;
> >     int complete;
> >     QEMUIOVector *iov;
> > -    struct stat *st;
> > +    struct nfs_stat *st;
> >     Coroutine *co;
> >     NFSClient *client;
> > } NFSRPC;
> > @@ -419,7 +427,7 @@ static int64_t nfs_client_open(NFSClient *client, BlockdevOptionsNfs *opts,
> >                                int flags, int open_flags, Error **errp)
> > {
> >     int64_t ret = -EINVAL;
> > -    struct stat st;
> > +    struct nfs_stat st;
> >     char *file = NULL, *strp = NULL;
> > 
> >     qemu_mutex_init(&client->mutex);
> > @@ -545,7 +553,7 @@ static int64_t nfs_client_open(NFSClient *client, BlockdevOptionsNfs *opts,
> >     }
> > 
> >     ret = DIV_ROUND_UP(st.st_size, BDRV_SECTOR_SIZE);
> > -    client->st_blocks = st.st_blocks;
> > +    client->st_size = st.st_size;
> >     client->has_zero_init = S_ISREG(st.st_mode);
> >     *strp = '/';
> >     goto out;
> > @@ -729,11 +737,11 @@ static int64_t nfs_get_allocated_file_size(BlockDriverState *bs)
> > {
> >     NFSClient *client = bs->opaque;
> >     NFSRPC task = {0};
> > -    struct stat st;
> > +    struct nfs_stat st;
> > 
> >     if (bdrv_is_read_only(bs) &&
> >         !(bs->open_flags & BDRV_O_NOCACHE)) {
> > -        return client->st_blocks * 512;
> > +        return client->st_size;
> I am using   client->st_size instead client->st_blocks * 512, anything wrong with this?
> >     }
> > 
> >     task.bs = bs;
> > @@ -746,7 +754,7 @@ static int64_t nfs_get_allocated_file_size(BlockDriverState *bs)
> >     nfs_set_events(client);
> >     BDRV_POLL_WHILE(bs, !task.complete);
> > 
> > -    return (task.ret < 0 ? task.ret : st.st_blocks * 512);
> > +    return (task.ret < 0 ? task.ret : st.st_size);
> > }
> > 
> > static int coroutine_fn
> > @@ -778,7 +786,7 @@ static int nfs_reopen_prepare(BDRVReopenState *state,
> >                               BlockReopenQueue *queue, Error **errp)
> > {
> >     NFSClient *client = state->bs->opaque;
> > -    struct stat st;
> > +    struct nfs_stat st;
> >     int ret = 0;
> > 
> >     if (state->flags & BDRV_O_RDWR && bdrv_is_read_only(state->bs)) {
> > @@ -800,7 +808,7 @@ static int nfs_reopen_prepare(BDRVReopenState *state,
> >                        nfs_get_error(client->context));
> >             return ret;
> >         }
> > -        client->st_blocks = st.st_blocks;
> > +        client->st_size = st.st_size;
> >     }
> > 
> >     return 0;
> > -- 
> > 2.28.0.windows.1
> > 
> 
> 
> NACK. st_blocks and st_size is not the same. st_blocks is the number of allocated blocks on disk. st_size is the virtual size of a file as it may contain holes.
> I think the appropriate fix is to not implement nfs_get_allocated_file_size on WIN32. Its not mandatory.
> 
> Please look at the   st_size, it's equivalant to  st_blocks * 512;

It is definitely not. Where do you have this info from?

Please have a look at libnfs/lib/nfs_v3.c in function nfs3_stat_1_cb.


        st.st_size    = res->GETATTR3res_u.resok.obj_attributes.size;
#ifndef WIN32
        st.st_blksize = NFS_BLKSIZE;
	st.st_blocks  = (res->GETATTR3res_u.resok.obj_attributes.used + 512 - 1) / 512;
#endif//WIN32


Your patch will eliminate this difference for non WIN32 systems. Please change your patch to not implement nfs_get_allocated_file_size and qemu will handle this case properly.
Put everything that involves st_blocks and nfs_get_allocated_file_size in #ifndef _WIN32 conditions.


Peter