Re: [PATCH v11 2/7] QIOChannelSocket: Implement io_writev zero copy flag & io_flush for CONFIG_LINUX

[Leonardo Bras Soares Passos]

On Thu, May 5, 2022 at 12:55 PM Daniel P. Berrangé  wrote:
>
> On Thu, May 05, 2022 at 12:42:47PM -0300, Leonardo Bras Soares Passos wrote:
> >
> > Hello Daniel,
> >
> > But what if this gets compiled in a Linux system without MSG_ZEROCOPY 
> > support?
> > As qapi will have zero-copy-send as an option we could have this scenario:
> >
> > - User request migration using zero-copy-send
> > - multifd_save_setup() will set write_flags = 
> > QIO_CHANNEL_WRITE_FLAG_ZERO_COPY
> > - In qio_channel_socket_connect_sync(): setsockopt() part will be
> > compiled-out, so no error here
> > - above part in qio_channel_socket_writev() will be commented-out,
> > which means write_flags will be ignored
> > - sflags will not contain MSG_ZEROCOPY, so sendmsg() will use copy-mode
> > - migration will succeed
> >
> > In the above case, the user has all the reason to think migration is
> > using MSG_ZEROCOPY, but in fact it's quietly falling back to
> > copy-mode.
>
> I think we're ok because qio_channel_writev_full() does
>
> if ((flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY) &&
> !qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_WRITE_ZERO_COPY)) {
> error_setg_errno(errp, EINVAL,
>  "Requested Zero Copy feature is not available");
> return -1;
> }
>
> and since there's no way for QIO_CHANNEL_FEATURE_WRITE_ZERO_COPY to
> get set when MSG_ZEROCOPY is compiled out, we'll trigger the error
> condition.

Oh, that's right. It will fail in the first writev(), I was just
considering failing during setup.

>
> > That's why I suggested creating a 'global' config usiing SO_ZEROCOPY &
> > MSG_ZEROCOPY & CONFIG_LINUX so we can use in qapi and have no chance
> > of even offering zero-copy-send if we don't have it.
> >
> > Another local option is to do implement your suggestions, and also
> > change qio_channel_socket_connect_sync() so it returns an error if
> > MSG_ZEROCOPY && SO_ZEROCOPY is not present, such as:
> >
> > +#ifdef CONFIG_LINUX
> > +#if defined(MSG_ZEROCOPY)  && defined(SO_ZEROCOPY)
> > +int ret, v = 1;
> > +ret = setsockopt(fd, SOL_SOCKET, SO_ZEROCOPY, , sizeof(v));
> > +if (ret == 0) {
> > +/* Zero copy available on host */
> > +qio_channel_set_feature(QIO_CHANNEL(ioc),
> > +QIO_CHANNEL_FEATURE_WRITE_ZERO_COPY);
> > +}
> > +#else
> > +error_setg_errno(errp, errno,"MSG_ZEROCOPY not available");
> > +return -1;
> > +#endif
> > +#endif
>
> Do we actually need the ifdef CONFIG_LINUX bit at all ?
>
> Sufficient to just have the check for MSG_ZEROCOPY + SO_ZEROCOPY,
> which will fail on non-Linux anyway.

By some include issue, or by future implementations we can have
MSG_ZEROCOPY or SO_ZEROCOPY getting defined in OS other than Linux,
which would introduce some headaches.

Since you pointed out that migration will fail on writev, the above
piece of code is not necessary.
We could have a local define that equals to (MSG_ZEROCOPY &&
SO_ZEROCOPY && CONFIG_LINUX) so that we can make the code simpler
where needed.

I will work on a v12 and send it here.

Best regards,
Leo

Re: [PULL 0/3] Block patches

[Richard Henderson]

On 5/5/22 03:42, Stefan Hajnoczi wrote:

The following changes since commit 9cf289af47bcfae5c75de37d8e5d6fd23705322c:

   Merge tag 'qga-pull-request' of gitlab.com:marcandre.lureau/qemu into 
staging (2022-05-04 03:42:49 -0700)

are available in the Git repository at:

   https://gitlab.com/stefanha/qemu.git tags/block-pull-request

for you to fetch changes up to bef2e050d6a7feb865854c65570c496ac5a8cf53:

   util/event-loop-base: Introduce options to set the thread pool size 
(2022-05-04 17:02:19 +0100)


Pull request

Add new thread-pool-min/thread-pool-max parameters to control the thread pool
used for async I/O.



Nicolas Saenz Julienne (3):
   Introduce event-loop-base abstract class
   util/main-loop: Introduce the main loop into QOM
   util/event-loop-base: Introduce options to set the thread pool size

  qapi/qom.json|  43 --
  meson.build  |  26 +++---
  include/block/aio.h  |  10 +++
  include/block/thread-pool.h  |   3 +
  include/qemu/main-loop.h |  10 +++
  include/sysemu/event-loop-base.h |  41 +
  include/sysemu/iothread.h|   6 +-
  event-loop-base.c| 140 +++
  iothread.c   |  68 +--
  util/aio-posix.c |   1 +
  util/async.c |  20 +
  util/main-loop.c |  65 ++
  util/thread-pool.c   |  55 +++-
  13 files changed, 419 insertions(+), 69 deletions(-)
  create mode 100644 include/sysemu/event-loop-base.h
  create mode 100644 event-loop-base.c



This appears to introduce a new error on msys2-64bit:


14/85 qemu:unit / test-aio  ERROR   2.14s 
  exit status 3
>>> MALLOC_PERTURB_=82 G_TEST_SRCDIR=C:/GitLab-Runner/builds/qemu-project/qemu/tests/unit 
G_TEST_BUILDDIR=C:/GitLab-Runner/builds/qemu-project/qemu/build/tests/unit 
C:/GitLab-Runner/builds/qemu-project/qemu/build/tests/unit/test-aio.exe --tap -k

- 8< -
stderr:
(test program exited with status code 3)

https://gitlab.com/qemu-project/qemu/-/jobs/2418935125

Are you in a position to test this yourself locally?


r~

Re: iotests and python dependencies

[Daniel P . Berrangé]

On Thu, May 05, 2022 at 05:50:00PM +0200, Paolo Bonzini wrote:
> On 5/5/22 16:13, John Snow wrote:
> > 
> > I would rather keep python/qemu/qmp as a submodule for a longer time,
> > and still go through a virtual environment that installs it together
> > with its pip dependencies.
> > 
> > 
> > A small headache relating fixes to both locations, but if you'd like to
> > see it to prove that the installation mechanism works in general, then
> > OK. I'm willing to deal with the pain until the next release to let us
> > go through a testing cycle. Reluctantly. Maybe.
> > 
> > I'm assuming you mean as a subpackage and not a [git] submodule. If you
> > do mean git, then ... uh. That might be messy.
> 
> Yeah, I meant a git submodule in qemu.git...  It would also be the easiest
> way to build a subpackage in Fedora, since it would be part of the QEMU
> tarballs.

When qemu.qmp is uploaded to PyPi, then Fedora packaging guidelines on
unbundling will expect us to create a dedicated python-qemu.qmp src.rpm,
and use that, not anything QEMU might bundle.

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

Re: [PATCH v11 2/7] QIOChannelSocket: Implement io_writev zero copy flag & io_flush for CONFIG_LINUX

[Daniel P . Berrangé]

On Thu, May 05, 2022 at 12:42:47PM -0300, Leonardo Bras Soares Passos wrote:
> 
> Hello Daniel,
> 
> But what if this gets compiled in a Linux system without MSG_ZEROCOPY support?
> As qapi will have zero-copy-send as an option we could have this scenario:
> 
> - User request migration using zero-copy-send
> - multifd_save_setup() will set write_flags = QIO_CHANNEL_WRITE_FLAG_ZERO_COPY
> - In qio_channel_socket_connect_sync(): setsockopt() part will be
> compiled-out, so no error here
> - above part in qio_channel_socket_writev() will be commented-out,
> which means write_flags will be ignored
> - sflags will not contain MSG_ZEROCOPY, so sendmsg() will use copy-mode
> - migration will succeed
> 
> In the above case, the user has all the reason to think migration is
> using MSG_ZEROCOPY, but in fact it's quietly falling back to
> copy-mode.

I think we're ok because qio_channel_writev_full() does

if ((flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY) &&
!qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_WRITE_ZERO_COPY)) {
error_setg_errno(errp, EINVAL,
 "Requested Zero Copy feature is not available");
return -1;
}

and since there's no way for QIO_CHANNEL_FEATURE_WRITE_ZERO_COPY to
get set when MSG_ZEROCOPY is compiled out, we'll trigger the error
condition.

> That's why I suggested creating a 'global' config usiing SO_ZEROCOPY &
> MSG_ZEROCOPY & CONFIG_LINUX so we can use in qapi and have no chance
> of even offering zero-copy-send if we don't have it.
> 
> Another local option is to do implement your suggestions, and also
> change qio_channel_socket_connect_sync() so it returns an error if
> MSG_ZEROCOPY && SO_ZEROCOPY is not present, such as:
> 
> +#ifdef CONFIG_LINUX
> +#if defined(MSG_ZEROCOPY)  && defined(SO_ZEROCOPY)
> +int ret, v = 1;
> +ret = setsockopt(fd, SOL_SOCKET, SO_ZEROCOPY, , sizeof(v));
> +if (ret == 0) {
> +/* Zero copy available on host */
> +qio_channel_set_feature(QIO_CHANNEL(ioc),
> +QIO_CHANNEL_FEATURE_WRITE_ZERO_COPY);
> +}
> +#else
> +error_setg_errno(errp, errno,"MSG_ZEROCOPY not available");
> +return -1;
> +#endif
> +#endif

Do we actually need the ifdef CONFIG_LINUX bit at all ?

Sufficient to just have the check for MSG_ZEROCOPY + SO_ZEROCOPY,
which will fail on non-Linux anyway.


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

Re: iotests and python dependencies

[Paolo Bonzini]

On 5/5/22 16:13, John Snow wrote:


I would rather keep python/qemu/qmp as a submodule for a longer time,
and still go through a virtual environment that installs it together
with its pip dependencies.


A small headache relating fixes to both locations, but if you'd like to 
see it to prove that the installation mechanism works in general, then 
OK. I'm willing to deal with the pain until the next release to let us 
go through a testing cycle. Reluctantly. Maybe.


I'm assuming you mean as a subpackage and not a [git] submodule. If you 
do mean git, then ... uh. That might be messy.


Yeah, I meant a git submodule in qemu.git...  It would also be the 
easiest way to build a subpackage in Fedora, since it would be part of 
the QEMU tarballs.


Paolo

Re: [PATCH v11 2/7] QIOChannelSocket: Implement io_writev zero copy flag & io_flush for CONFIG_LINUX

[Leonardo Bras Soares Passos]

On Thu, May 5, 2022 at 5:05 AM Daniel P. Berrangé  wrote:
>
> On Wed, May 04, 2022 at 04:18:31PM -0300, Leonardo Bras wrote:
> > For CONFIG_LINUX, implement the new zero copy flag and the optional callback
> > io_flush on QIOChannelSocket, but enables it only when MSG_ZEROCOPY
> > feature is available in the host kernel, which is checked on
> > qio_channel_socket_connect_sync()
> >
> > qio_channel_socket_flush() was implemented by counting how many times
> > sendmsg(...,MSG_ZEROCOPY) was successfully called, and then reading the
> > socket's error queue, in order to find how many of them finished sending.
> > Flush will loop until those counters are the same, or until some error 
> > occurs.
> >
> > Notes on using writev() with QIO_CHANNEL_WRITE_FLAG_ZERO_COPY:
> > 1: Buffer
> > - As MSG_ZEROCOPY tells the kernel to use the same user buffer to avoid 
> > copying,
> > some caution is necessary to avoid overwriting any buffer before it's sent.
> > If something like this happen, a newer version of the buffer may be sent 
> > instead.
> > - If this is a problem, it's recommended to call qio_channel_flush() before 
> > freeing
> > or re-using the buffer.
> >
> > 2: Locked memory
> > - When using MSG_ZERCOCOPY, the buffer memory will be locked after queued, 
> > and
> > unlocked after it's sent.
> > - Depending on the size of each buffer, and how often it's sent, it may 
> > require
> > a larger amount of locked memory than usually available to non-root user.
> > - If the required amount of locked memory is not available, writev_zero_copy
> > will return an error, which can abort an operation like migration,
> > - Because of this, when an user code wants to add zero copy as a feature, it
> > requires a mechanism to disable it, so it can still be accessible to less
> > privileged users.
> >
> > Signed-off-by: Leonardo Bras 
> > Reviewed-by: Peter Xu 
> > Reviewed-by: Daniel P. Berrangé 
> > Reviewed-by: Juan Quintela 
> > ---
> >  include/io/channel-socket.h |   2 +
> >  io/channel-socket.c | 120 ++--
> >  2 files changed, 118 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/io/channel-socket.h b/include/io/channel-socket.h
> > index e747e63514..513c428fe4 100644
> > --- a/include/io/channel-socket.h
> > +++ b/include/io/channel-socket.h
> > @@ -47,6 +47,8 @@ struct QIOChannelSocket {
> >  socklen_t localAddrLen;
> >  struct sockaddr_storage remoteAddr;
> >  socklen_t remoteAddrLen;
> > +ssize_t zero_copy_queued;
> > +ssize_t zero_copy_sent;
> >  };
> >
> >
> > diff --git a/io/channel-socket.c b/io/channel-socket.c
> > index 696a04dc9c..ae756ce166 100644
> > --- a/io/channel-socket.c
> > +++ b/io/channel-socket.c
> > @@ -25,9 +25,25 @@
> >  #include "io/channel-watch.h"
> >  #include "trace.h"
> >  #include "qapi/clone-visitor.h"
> > +#ifdef CONFIG_LINUX
> > +#include 
> > +#include 
> > +#endif
> >
> >  #define SOCKET_MAX_FDS 16
> >
> > +/*
> > + * Zero-copy defines bellow are included to avoid breaking builds on 
> > systems
> > + * that don't support MSG_ZEROCOPY, while keeping the functions more 
> > readable
> > + * (without a lot of ifdefs).
> > + */
> > +#ifndef MSG_ZEROCOPY
> > +#define MSG_ZEROCOPY 0x400
> > +#endif
> > +#ifndef SO_ZEROCOPY
> > +#define SO_ZEROCOPY 60
> > +#endif
>
> Please put these behind CONFIG_LINUX to make it clear to readers that
> this is entirely Linux specific
>
>
> > +
> >  SocketAddress *
> >  qio_channel_socket_get_local_address(QIOChannelSocket *ioc,
> >   Error **errp)
> > @@ -54,6 +70,8 @@ qio_channel_socket_new(void)
> >
> >  sioc = QIO_CHANNEL_SOCKET(object_new(TYPE_QIO_CHANNEL_SOCKET));
> >  sioc->fd = -1;
> > +sioc->zero_copy_queued = 0;
> > +sioc->zero_copy_sent = 0;
> >
> >  ioc = QIO_CHANNEL(sioc);
> >  qio_channel_set_feature(ioc, QIO_CHANNEL_FEATURE_SHUTDOWN);
> > @@ -153,6 +171,16 @@ int qio_channel_socket_connect_sync(QIOChannelSocket 
> > *ioc,
> >  return -1;
> >  }
> >
> > +#ifdef CONFIG_LINUX
> > +int ret, v = 1;
> > +ret = setsockopt(fd, SOL_SOCKET, SO_ZEROCOPY, , sizeof(v));
> > +if (ret == 0) {
> > +/* Zero copy available on host */
> > +qio_channel_set_feature(QIO_CHANNEL(ioc),
> > +QIO_CHANNEL_FEATURE_WRITE_ZERO_COPY);
> > +}
> > +#endif
> > +
> >  return 0;
> >  }
> >
> > @@ -533,6 +561,7 @@ static ssize_t qio_channel_socket_writev(QIOChannel 
> > *ioc,
> >  char control[CMSG_SPACE(sizeof(int) * SOCKET_MAX_FDS)];
> >  size_t fdsize = sizeof(int) * nfds;
> >  struct cmsghdr *cmsg;
> > +int sflags = 0;
> >
> >  memset(control, 0, CMSG_SPACE(sizeof(int) * SOCKET_MAX_FDS));
> >
> > @@ -557,15 +586,27 @@ static ssize_t qio_channel_socket_writev(QIOChannel 
> > *ioc,
> >  memcpy(CMSG_DATA(cmsg), fds, fdsize);
> >  }
> >
> > +if (flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY) {
> > +sflags = MSG_ZEROCOPY;
> > 

Re: [PATCH v2 14/15] qga/wixl: replace QEMU_GA_MSI_MINGW_BIN_PATH with glib bindir

[Konstantin Kostiuk]

Reviewed-by: Konstantin Kostiuk 

On Thu, May 5, 2022 at 11:16 AM  wrote:

> From: Marc-André Lureau 
>
> Use more conventional variables to set the location of pre-built
> DLL/bin.
>
> Signed-off-by: Marc-André Lureau 
> ---
>  configure |  9 ++---
>  meson.build   |  5 -
>  qga/installer/qemu-ga.wxs | 24 
>  qga/meson.build   |  2 +-
>  4 files changed, 23 insertions(+), 17 deletions(-)
>
> diff --git a/configure b/configure
> index 59c43bea05eb..616cd2d0e36c 100755
> --- a/configure
> +++ b/configure
> @@ -2023,6 +2023,11 @@ for i in $glib_modules; do
>  fi
>  done
>
> +glib_bindir="$($pkg_config --variable=bindir glib-2.0)"
> +if test -z "$glib_bindir" ; then
> +   glib_bindir="$($pkg_config --variable=prefix glib-2.0)"/bin
> +fi
> +
>  # This workaround is required due to a bug in pkg-config file for glib as
> it
>  # doesn't define GLIB_STATIC_COMPILATION for pkg-config --static
>
> @@ -2430,8 +2435,6 @@ if test "$QEMU_GA_VERSION" = ""; then
>  QEMU_GA_VERSION=$(cat $source_path/VERSION)
>  fi
>
> -QEMU_GA_MSI_MINGW_BIN_PATH="$($pkg_config --variable=prefix glib-2.0)/bin"
> -
>  # Mac OS X ships with a broken assembler
>  roms=
>  if { test "$cpu" = "i386" || test "$cpu" = "x86_64"; } && \
> @@ -2518,7 +2521,6 @@ if test "$debug_tcg" = "yes" ; then
>  fi
>  if test "$mingw32" = "yes" ; then
>echo "CONFIG_WIN32=y" >> $config_host_mak
> -  echo "QEMU_GA_MSI_MINGW_BIN_PATH=${QEMU_GA_MSI_MINGW_BIN_PATH}" >>
> $config_host_mak
>echo "QEMU_GA_MANUFACTURER=${QEMU_GA_MANUFACTURER}" >> $config_host_mak
>echo "QEMU_GA_DISTRO=${QEMU_GA_DISTRO}" >> $config_host_mak
>echo "QEMU_GA_VERSION=${QEMU_GA_VERSION}" >> $config_host_mak
> @@ -2639,6 +2641,7 @@ echo "QEMU_CXXFLAGS=$QEMU_CXXFLAGS" >>
> $config_host_mak
>  echo "QEMU_OBJCFLAGS=$QEMU_OBJCFLAGS" >> $config_host_mak
>  echo "GLIB_CFLAGS=$glib_cflags" >> $config_host_mak
>  echo "GLIB_LIBS=$glib_libs" >> $config_host_mak
> +echo "GLIB_BINDIR=$glib_bindir" >> $config_host_mak
>  echo "GLIB_VERSION=$(pkg-config --modversion glib-2.0)" >>
> $config_host_mak
>  echo "QEMU_LDFLAGS=$QEMU_LDFLAGS" >> $config_host_mak
>  echo "LD_I386_EMULATION=$ld_i386_emulation" >> $config_host_mak
> diff --git a/meson.build b/meson.build
> index c26aa442d40e..2f68b6cb8634 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -443,7 +443,10 @@
> add_project_arguments(config_host['GLIB_CFLAGS'].split(),
>native: false, language: ['c', 'cpp', 'objc'])
>  glib = declare_dependency(compile_args:
> config_host['GLIB_CFLAGS'].split(),
>link_args: config_host['GLIB_LIBS'].split(),
> -  version: config_host['GLIB_VERSION'])
> +  version: config_host['GLIB_VERSION'],
> +  variables: {
> +'bindir': config_host['GLIB_BINDIR'],
> +  })
>  # override glib dep with the configure results (for subprojects)
>  meson.override_dependency('glib-2.0', glib)
>
> diff --git a/qga/installer/qemu-ga.wxs b/qga/installer/qemu-ga.wxs
> index e5b0958e1898..813d1c6ca6ae 100644
> --- a/qga/installer/qemu-ga.wxs
> +++ b/qga/installer/qemu-ga.wxs
> @@ -58,7 +58,7 @@
>
>
> Guid="{55E737B5-9127-4A11-9FC3-A29367714574}">
> - Source="$(var.Mingw_bin)/libstdc++-6.dll" KeyPath="yes" DiskId="1"/>
> + Source="$(var.BIN_DIR)/libstdc++-6.dll" KeyPath="yes" DiskId="1"/>
>
> Guid="{CB19C453-FABB-4BB1-ABAB-6B74F687BFBB}">
>   Source="$(var.BUILD_DIR)/qga/vss-win32/qga-vss.dll" KeyPath="yes"
> DiskId="1"/>
> @@ -69,40 +69,40 @@
>
>
> Guid="{446185B3-87BE-43D2-96B8-0FEFD9E8696D}">
> - Name="gspawn-win32-helper-console.exe"
> Source="$(var.Mingw_bin)/gspawn-win32-helper-console.exe" KeyPath="yes"
> DiskId="1"/>
> + Name="gspawn-win32-helper-console.exe"
> Source="$(var.BIN_DIR)/gspawn-win32-helper-console.exe" KeyPath="yes"
> DiskId="1"/>
>
> Guid="{CD67A5A3-2DB1-4DA1-A67A-8D71E797B466}">
> - Name="gspawn-win32-helper.exe"
> Source="$(var.Mingw_bin)/gspawn-win32-helper-console.exe" KeyPath="yes"
> DiskId="1"/>
> + Name="gspawn-win32-helper.exe"
> Source="$(var.BIN_DIR)/gspawn-win32-helper-console.exe" KeyPath="yes"
> DiskId="1"/>
>
>
>
> Guid="{9E615A9F-349A-4992-A5C2-C10BAD173660}">
> - Name="gspawn-win64-helper-console.exe"
> Source="$(var.Mingw_bin)/gspawn-win64-helper-console.exe" KeyPath="yes"
> DiskId="1"/>
> + Name="gspawn-win64-helper-console.exe"
> Source="$(var.BIN_DIR)/gspawn-win64-helper-console.exe" KeyPath="yes"
> DiskId="1"/>
>
> Guid="{D201AD22-1846-4E4F-B6E1-C7A908ED2457}">
> - Name="gspawn-win64-helper.exe"
> 

Re: [PATCH v2 13/15] qga/wixl: simplify some pre-processing

[Konstantin Kostiuk]

Reviewed-by: Konstantin Kostiuk 

On Thu, May 5, 2022 at 11:16 AM  wrote:

> From: Marc-André Lureau 
>
> Sadly, wixl doesn't have 'elif'.
>
> Signed-off-by: Marc-André Lureau 
> ---
>  qga/installer/qemu-ga.wxs | 20 +++-
>  1 file changed, 7 insertions(+), 13 deletions(-)
>
> diff --git a/qga/installer/qemu-ga.wxs b/qga/installer/qemu-ga.wxs
> index 651db6e51cda..e5b0958e1898 100644
> --- a/qga/installer/qemu-ga.wxs
> +++ b/qga/installer/qemu-ga.wxs
> @@ -1,21 +1,15 @@
>  
>  http://schemas.microsoft.com/wix/2006/wi;>
> -  
> -
> -  
> -
>
>  
>  
> -  
> -
> -  
> -
> -
> -  
> -
> -  
> -
> +  
> +
> +  
> +  
> +
> +  
> +
>
>
> --
> 2.36.0.44.g0f828332d5ac
>
>

Re: [PATCH v2 12/15] qga/wixl: require Mingw_bin

[Konstantin Kostiuk]

Reviewed-by: Konstantin Kostiuk 

On Thu, May 5, 2022 at 11:16 AM  wrote:

> From: Marc-André Lureau 
>
> No clear reason to make guesses here.
>
> Signed-off-by: Marc-André Lureau 
> ---
>  qga/installer/qemu-ga.wxs | 9 -
>  1 file changed, 9 deletions(-)
>
> diff --git a/qga/installer/qemu-ga.wxs b/qga/installer/qemu-ga.wxs
> index 8a19aa165651..651db6e51cda 100644
> --- a/qga/installer/qemu-ga.wxs
> +++ b/qga/installer/qemu-ga.wxs
> @@ -4,15 +4,6 @@
>  
>
>
> -  
> -
> -  
> -
> -
> -  
> -
> -  
> -
>
>  
>  
> --
> 2.36.0.44.g0f828332d5ac
>
>

Re: iotests and python dependencies

[John Snow]

On Thu, May 5, 2022, 9:16 AM Paolo Bonzini  wrote:

> On 5/5/22 15:10, John Snow wrote:
> >
> >  > Hm, do we need iotests during an rpm build? Is it because of
> > "make check"?
> >
> > Yes, and this is good, because it prevents us from outputting an
> > RPM build that has a broken QEMU in it.
> >
> > Guess this means I need to make a Fedora package too, though. My hubris.
>
> I would rather keep python/qemu/qmp as a submodule for a longer time,
> and still go through a virtual environment that installs it together
> with its pip dependencies.
>

A small headache relating fixes to both locations, but if you'd like to see
it to prove that the installation mechanism works in general, then OK. I'm
willing to deal with the pain until the next release to let us go through a
testing cycle. Reluctantly. Maybe.

I'm assuming you mean as a subpackage and not a [git] submodule. If you do
mean git, then ... uh. That might be messy.

Re: [PATCH v2 11/15] qga/wixl: prefer variables over environment

[Konstantin Kostiuk]

Reviewed-by: Konstantin Kostiuk 

On Thu, May 5, 2022 at 11:16 AM  wrote:

> From: Marc-André Lureau 
>
> No need to setup an environment or to check if the variable is undefined
> manually.
>
> Signed-off-by: Marc-André Lureau 
> ---
>  qga/installer/qemu-ga.wxs | 30 +-
>  qga/meson.build   |  9 -
>  2 files changed, 13 insertions(+), 26 deletions(-)
>
> diff --git a/qga/installer/qemu-ga.wxs b/qga/installer/qemu-ga.wxs
> index 0950e8c6becc..8a19aa165651 100644
> --- a/qga/installer/qemu-ga.wxs
> +++ b/qga/installer/qemu-ga.wxs
> @@ -1,17 +1,5 @@
>  
>  http://schemas.microsoft.com/wix/2006/wi;>
> -  
> -
> -  
> -
> -  
> -
> -  
> -
> -  
> -
> -  
> -
>
>  
>
> @@ -43,20 +31,20 @@
>  Name="QEMU guest agent"
>  Id="*"
>  UpgradeCode="{EB6B8302-C06E-4BEC-ADAC-932C68A3A98D}"
> -Manufacturer="$(env.QEMU_GA_MANUFACTURER)"
> -Version="$(env.QEMU_GA_VERSION)"
> +Manufacturer="$(var.QEMU_GA_MANUFACTURER)"
> +Version="$(var.QEMU_GA_VERSION)"
>  Language="1033">
>  
>  NOT VersionNT64
>  
>   -  Manufacturer="$(env.QEMU_GA_MANUFACTURER)"
> +  Manufacturer="$(var.QEMU_GA_MANUFACTURER)"
>InstallerVersion="200"
>Languages="1033"
>Compressed="yes"
>InstallScope="perMachine"
>/>
> - EmbedCab="yes" />
> + EmbedCab="yes" />
>  1
>  DowngradeErrorMessage="Error: A newer version of QEMU guest agent
> is already installed."
> @@ -66,7 +54,7 @@
>
>  
> Guid="{908B7199-DE2A-4DC6-A8D0-27A5AE444FEA}">
> - Source="$(env.BUILD_DIR)/qga/qemu-ga.exe" KeyPath="yes" DiskId="1"/>
> + Source="$(var.BUILD_DIR)/qga/qemu-ga.exe" KeyPath="yes" DiskId="1"/>
>  Id="ServiceInstaller"
>Type="ownProcess"
> @@ -88,10 +76,10 @@
>   Source="$(var.Mingw_bin)/libstdc++-6.dll" KeyPath="yes" DiskId="1"/>
>
> Guid="{CB19C453-FABB-4BB1-ABAB-6B74F687BFBB}">
> - Source="$(env.BUILD_DIR)/qga/vss-win32/qga-vss.dll" KeyPath="yes"
> DiskId="1"/>
> + Source="$(var.BUILD_DIR)/qga/vss-win32/qga-vss.dll" KeyPath="yes"
> DiskId="1"/>
>
> Guid="{D8D584B1-59C2-4FB7-A91F-636FF7BFA66E}">
> - Source="$(env.BUILD_DIR)/qga/vss-win32/qga-vss.tlb" KeyPath="yes"
> DiskId="1"/>
> + Source="$(var.BUILD_DIR)/qga/vss-win32/qga-vss.tlb" KeyPath="yes"
> DiskId="1"/>
>
>
>
> @@ -133,9 +121,9 @@
>
> Guid="{D075D109-51CA-11E3-9F8B-000C29858960}">
>   -
>  
> Key="Software\$(env.QEMU_GA_MANUFACTURER)\$(env.QEMU_GA_DISTRO)\Tools\QemuGA">
> +
>  
> Key="Software\$(var.QEMU_GA_MANUFACTURER)\$(var.QEMU_GA_DISTRO)\Tools\QemuGA">
> Value="fb0a0d66-c7fb-4e2e-a16b-c4a3bfe8d13b" />
> -   Value="$(env.QEMU_GA_VERSION)" />
> +   Value="$(var.QEMU_GA_VERSION)" />
>  
>
>  
> diff --git a/qga/meson.build b/qga/meson.build
> index 6d9f39bb321b..3ad3bc0260cf 100644
> --- a/qga/meson.build
> +++ b/qga/meson.build
> @@ -121,15 +121,14 @@ if targetos == 'windows'
>  output: 'qemu-ga-@0@.msi'.format(host_arch),
>  depends: deps,
>  command: [
> -  find_program('env'),
> -  'QEMU_GA_VERSION=' +
> config_host['QEMU_GA_VERSION'],
> -  'QEMU_GA_MANUFACTURER=' +
> config_host['QEMU_GA_MANUFACTURER'],
> -  'QEMU_GA_DISTRO=' +
> config_host['QEMU_GA_DISTRO'],
> -  'BUILD_DIR=' + meson.build_root(),
>wixl, '-o', '@OUTPUT0@', '@INPUT0@',
>qemu_ga_msi_arch[cpu],
>qemu_ga_msi_vss,
> +  '-D', 'BUILD_DIR=' + meson.build_root(),
>'-D', 'Mingw_bin=' +
> config_host['QEMU_GA_MSI_MINGW_BIN_PATH'],
> +  '-D', 'QEMU_GA_VERSION=' +
> config_host['QEMU_GA_VERSION'],
> +  '-D', 'QEMU_GA_MANUFACTURER=' +
> config_host['QEMU_GA_MANUFACTURER'],
> +  '-D', 'QEMU_GA_DISTRO=' +
> config_host['QEMU_GA_DISTRO'],
>  ])
>  all_qga += [qga_msi]
>  alias_target('msi', qga_msi)
> --
> 2.36.0.44.g0f828332d5ac
>
>

Re: [PATCH v2 15/15] test/qga: use g_auto wherever sensible

[Markus Armbruster]

Marc-André Lureau  writes:

> Hi
>
> On Thu, May 5, 2022 at 3:47 PM Markus Armbruster  wrote:
>>
>> marcandre.lur...@redhat.com writes:
>>
>> > From: Marc-André Lureau 
>> >
>> > Signed-off-by: Marc-André Lureau 
>> > ---
>> >  tests/unit/test-qga.c | 125 +++---
>> >  1 file changed, 45 insertions(+), 80 deletions(-)
>> >
>> > diff --git a/tests/unit/test-qga.c b/tests/unit/test-qga.c
>> > index ab0b12a2dd16..53cefc2c2649 100644
>> > --- a/tests/unit/test-qga.c
>> > +++ b/tests/unit/test-qga.c
>> > @@ -51,8 +51,11 @@ static void
>> >  fixture_setup(TestFixture *fixture, gconstpointer data, gchar **envp)
>> >  {
>> >  const gchar *extra_arg = data;
>> > -GError *error = NULL;
>> > -gchar *cwd, *path, *cmd, **argv = NULL;
>> > +g_autoptr(GError) error = NULL;
>> > +g_autofree char *cwd = NULL;
>> > +g_autofree char *path = NULL;
>> > +g_autofree char *cmd = NULL;
>> > +g_auto(GStrv) argv = NULL;
>>
>> Arranges five variables to be auto-freed, where ...
>>
>> >
>> >  fixture->loop = g_main_loop_new(NULL, FALSE);
>> >
>> > @@ -78,17 +81,12 @@ fixture_setup(TestFixture *fixture, gconstpointer 
>> > data, gchar **envp)
>> >
>> >  fixture->fd = connect_qga(path);
>> >  g_assert_cmpint(fixture->fd, !=, -1);
>> > -
>> > -g_strfreev(argv);
>> > -g_free(cmd);
>> > -g_free(cwd);
>> > -g_free(path);
>>
>> ... only four were freed before.  The extra one is @error.  Which can
>> only be null here, thanks to g_assert_no_error(), can't it?
>
> Right, but for consistency we can have it. I can drop it too.

If you want to add no-op frees for consistency, then your commit message
should prepare reviewers for that.

And you should perhaps be prepared for reviewers asking you to split
your patch ;)

Dropping seems the easiest path forward.

Re: iotests and python dependencies

[Paolo Bonzini]

On 5/5/22 15:10, John Snow wrote:


 > Hm, do we need iotests during an rpm build? Is it because of
"make check"?

Yes, and this is good, because it prevents us from outputting an
RPM build that has a broken QEMU in it.

Guess this means I need to make a Fedora package too, though. My hubris.


I would rather keep python/qemu/qmp as a submodule for a longer time, 
and still go through a virtual environment that installs it together 
with its pip dependencies.


Paolo

Re: iotests and python dependencies

[John Snow]

On Thu, May 5, 2022, 8:33 AM Daniel P. Berrangé  wrote:

> On Thu, May 05, 2022 at 08:08:42AM -0400, John Snow wrote:
> > On Thu, May 5, 2022, 4:09 AM Daniel P. Berrangé 
> wrote:
> >
> > > On Wed, May 04, 2022 at 03:38:45PM -0400, John Snow wrote:
> > > > Howdy!
> > > >
> > > > So, I want to finally delete python/qemu/qmp from qemu.git, and this
> > > > creates a small problem -- namely, iotests needs access to it in
> order
> > > > to run the python-based tests.
> > > >
> > > > What I think needs to happen is that we create a virtual environment
> > > > that installs python/qemu/. The reason this cannot be done with
> > > > PYTHONPATH alone anymore is because the qmp package itself won't be
> > > > there anymore, we need an installer like `pip` to actually fetch it
> > > > for us and put it somewhere. (i.e., we need to process the
> > > > dependencies of python/qemu now and can't treat it as a pre-installed
> > > > location.)
> > >
> > > Having pip fetch it on the fly creates a problem for RPM builds,
> > > because the koji build env has no network access. We will, however,
> > > have an RPM of python-qemu-qmp installed on the host system though.
> > > IOW we need to be able to run iotests using system python and its
> > > installed libs, not a virtual env.  So if we do anything with a
> > > virtual env, it will need to be optional I believe.
> > >
> >
> > Hm, do we need iotests during an rpm build? Is it because of "make
> check"?
>
> Yes, and this is good, because it prevents us from outputting an
> RPM build that has a broken QEMU in it.


Guess this means I need to make a Fedora package too, though. My hubris.

OK, plenty of work to do.

Re: [PULL 00/13] Block layer patches

[Richard Henderson]

On 5/4/22 09:25, Kevin Wolf wrote:

The following changes since commit 2e3408b3cc7de4e87a9adafc8c19bfce3abec947:

   Merge tag 'misc-pull-request' of gitlab.com:marcandre.lureau/qemu into 
staging (2022-05-03 09:13:17 -0700)

are available in the Git repository at:

   git://repo.or.cz/qemu/kevin.git tags/for-upstream

for you to fetch changes up to c1fe694357a328c807ae3cc6961c19e923448fcc:

   coroutine-win32: use QEMU_DEFINE_STATIC_CO_TLS() (2022-05-04 15:55:23 +0200)


Block layer patches

- Fix and re-enable GLOBAL_STATE_CODE assertions
- vhost-user: Fixes for VHOST_USER_ADD/REM_MEM_REG
- vmdk: Fix reopening bs->file
- coroutine: use QEMU_DEFINE_STATIC_CO_TLS()
- docs/qemu-img: Fix list of formats which implement check


Applied, thanks.  Please update https://wiki.qemu.org/ChangeLog/7.1 as 
appropriate.


r~





Denis V. Lunev (1):
   qemu-img: properly list formats which have consistency check implemented

Hanna Reitz (6):
   block: Classify bdrv_get_flags() as I/O function
   qcow2: Do not reopen data_file in invalidate_cache
   Revert "main-loop: Disable GLOBAL_STATE_CODE() assertions"
   iotests: Add regression test for issue 945
   block/vmdk: Fix reopening bs->file
   iotests/reopen-file: Test reopening file child

Kevin Wolf (3):
   docs/vhost-user: Clarifications for VHOST_USER_ADD/REM_MEM_REG
   libvhost-user: Fix extra vu_add/rem_mem_reg reply
   vhost-user: Don't pass file descriptor for VHOST_USER_REM_MEM_REG

Stefan Hajnoczi (3):
   coroutine-ucontext: use QEMU_DEFINE_STATIC_CO_TLS()
   coroutine: use QEMU_DEFINE_STATIC_CO_TLS()
   coroutine-win32: use QEMU_DEFINE_STATIC_CO_TLS()

  docs/interop/vhost-user.rst|  17 
  docs/tools/qemu-img.rst|   4 +-
  include/block/block-global-state.h |   1 -
  include/block/block-io.h   |   1 +
  include/qemu/main-loop.h   |   3 +-
  block.c|   2 +-
  block/qcow2.c  | 104 -
  block/vmdk.c   |  56 ++-
  hw/virtio/vhost-user.c |   2 +-
  subprojects/libvhost-user/libvhost-user.c  |  17 ++--
  util/coroutine-ucontext.c  |  38 +---
  util/coroutine-win32.c |  18 +++-
  util/qemu-coroutine.c  |  41 
  tests/qemu-iotests/tests/export-incoming-iothread  |  81 
  .../tests/export-incoming-iothread.out |   5 +
  tests/qemu-iotests/tests/reopen-file   |  89 ++
  tests/qemu-iotests/tests/reopen-file.out   |   5 +
  17 files changed, 388 insertions(+), 96 deletions(-)
  create mode 100755 tests/qemu-iotests/tests/export-incoming-iothread
  create mode 100644 tests/qemu-iotests/tests/export-incoming-iothread.out
  create mode 100755 tests/qemu-iotests/tests/reopen-file
  create mode 100644 tests/qemu-iotests/tests/reopen-file.out

Re: [PATCH v11 2/7] QIOChannelSocket: Implement io_writev zero copy flag & io_flush for CONFIG_LINUX

[Peter Xu]

On Thu, May 05, 2022 at 01:20:04AM -0300, Leonardo Bras Soares Passos wrote:
> (2) is already implemented in v11, but I have no issue implementing
> (1) for v12 if it's ok to create this 'global' define.

Dan's suggestion in the other thread sounds good to me with current
approach, on having CONFIG_LINUX to wrap the defines.

But note that it's still prone to change in the future, e.g., when other
qemu modules start to use MSG_ZEROCOPY, we'll probably at least move the
defines into some other headers.  We could probably need to clean things up
when it happens.

But I won't strongly ask for that - we can leave that for later.

Thanks,

-- 
Peter Xu

Re: iotests and python dependencies

[Kevin Wolf]

Am 05.05.2022 um 14:24 hat Paolo Bonzini geschrieben:
> On 5/5/22 12:59, Kevin Wolf wrote:
> > Am 05.05.2022 um 11:28 hat Paolo Bonzini geschrieben:
> > > > Or actually, it could just unconditionally run 'make check-venv' by
> > > > itself, which is probably easier to implement than checking the
> > > > dependencies and more convenient for the user, too.
> > > 
> > > One small complication is that on BSD systems the binary is actually
> > > called "gmake", so you'd have to pass the variable somehow
> > 
> > I guess we could just export $MAKE as an environment variable?
> 
> That would work when invoked by "make", but then that's the case in which
> the venv would be there anyway.
> 
> For the other case, it would have to parse config-host.mak and/or
> reintroduce something like tests/qemu-iotests/common.env.  All in all it
> seems like an unnecessary complication over just printing a clear and polite
> error message.

Or try 'make' and print the error message only if that fails.

Kevin

Re: iotests and python dependencies

[Daniel P . Berrangé]

On Thu, May 05, 2022 at 08:08:42AM -0400, John Snow wrote:
> On Thu, May 5, 2022, 4:09 AM Daniel P. Berrangé  wrote:
> 
> > On Wed, May 04, 2022 at 03:38:45PM -0400, John Snow wrote:
> > > Howdy!
> > >
> > > So, I want to finally delete python/qemu/qmp from qemu.git, and this
> > > creates a small problem -- namely, iotests needs access to it in order
> > > to run the python-based tests.
> > >
> > > What I think needs to happen is that we create a virtual environment
> > > that installs python/qemu/. The reason this cannot be done with
> > > PYTHONPATH alone anymore is because the qmp package itself won't be
> > > there anymore, we need an installer like `pip` to actually fetch it
> > > for us and put it somewhere. (i.e., we need to process the
> > > dependencies of python/qemu now and can't treat it as a pre-installed
> > > location.)
> >
> > Having pip fetch it on the fly creates a problem for RPM builds,
> > because the koji build env has no network access. We will, however,
> > have an RPM of python-qemu-qmp installed on the host system though.
> > IOW we need to be able to run iotests using system python and its
> > installed libs, not a virtual env.  So if we do anything with a
> > virtual env, it will need to be optional I believe.
> >
> 
> Hm, do we need iotests during an rpm build? Is it because of "make check"?

Yes, and this is good, because it prevents us from outputting an
RPM build that has a broken QEMU in it. 

> It's possible to create a venv and run pip in no-network mode, too. If the
> package we want is installed on the system or otherwise in pip's cache,
> it'll succeed without network. If the dependencies require a qemu.qmp
> that's too new, the pip install will just fail instead.
> 
> I have to test a way to craft a pip statement that's network *optional*
> though. i.e. try to fetch and fall back to local otherwise. I think it's
> worth trying to keep the environment setup code unified, and always use a
> venv.

As long as it is no-network, that's good enough.

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

Re: iotests and python dependencies

[Paolo Bonzini]

On 5/5/22 12:59, Kevin Wolf wrote:

Am 05.05.2022 um 11:28 hat Paolo Bonzini geschrieben:

Or actually, it could just unconditionally run 'make check-venv' by
itself, which is probably easier to implement than checking the
dependencies and more convenient for the user, too.


One small complication is that on BSD systems the binary is actually
called "gmake", so you'd have to pass the variable somehow


I guess we could just export $MAKE as an environment variable?


That would work when invoked by "make", but then that's the case in 
which the venv would be there anyway.


For the other case, it would have to parse config-host.mak and/or 
reintroduce something like tests/qemu-iotests/common.env.  All in all it 
seems like an unnecessary complication over just printing a clear and 
polite error message.


Paolo

Re: iotests and python dependencies

[John Snow]

On Thu, May 5, 2022, 4:09 AM Daniel P. Berrangé  wrote:

> On Wed, May 04, 2022 at 03:38:45PM -0400, John Snow wrote:
> > Howdy!
> >
> > So, I want to finally delete python/qemu/qmp from qemu.git, and this
> > creates a small problem -- namely, iotests needs access to it in order
> > to run the python-based tests.
> >
> > What I think needs to happen is that we create a virtual environment
> > that installs python/qemu/. The reason this cannot be done with
> > PYTHONPATH alone anymore is because the qmp package itself won't be
> > there anymore, we need an installer like `pip` to actually fetch it
> > for us and put it somewhere. (i.e., we need to process the
> > dependencies of python/qemu now and can't treat it as a pre-installed
> > location.)
>
> Having pip fetch it on the fly creates a problem for RPM builds,
> because the koji build env has no network access. We will, however,
> have an RPM of python-qemu-qmp installed on the host system though.
> IOW we need to be able to run iotests using system python and its
> installed libs, not a virtual env.  So if we do anything with a
> virtual env, it will need to be optional I believe.
>

Hm, do we need iotests during an rpm build? Is it because of "make check"?

It's possible to create a venv and run pip in no-network mode, too. If the
package we want is installed on the system or otherwise in pip's cache,
it'll succeed without network. If the dependencies require a qemu.qmp
that's too new, the pip install will just fail instead.

I have to test a way to craft a pip statement that's network *optional*
though. i.e. try to fetch and fall back to local otherwise. I think it's
worth trying to keep the environment setup code unified, and always use a
venv.

I think this is tractable, though.

Re: iotests and python dependencies

[John Snow]

On Thu, May 5, 2022, 4:51 AM Kevin Wolf  wrote:

> Am 04.05.2022 um 21:38 hat John Snow geschrieben:
> > Howdy!
> >
> > So, I want to finally delete python/qemu/qmp from qemu.git, and this
> > creates a small problem -- namely, iotests needs access to it in order
> > to run the python-based tests.
> >
> > What I think needs to happen is that we create a virtual environment
> > that installs python/qemu/. The reason this cannot be done with
> > PYTHONPATH alone anymore is because the qmp package itself won't be
> > there anymore, we need an installer like `pip` to actually fetch it
> > for us and put it somewhere. (i.e., we need to process the
> > dependencies of python/qemu now and can't treat it as a pre-installed
> > location.)
> >
> > Avocado tests are already creating a venv for the purposes of
> > installing and running Avocado. We can amend e.g. "../../python" to
> > tests/requirements.txt and the Avocado environment is A-OK good-to-go.
> > The Makefile magic for avocado tests creates a venv-per-build.
>
> "per build" sounded pretty bad, because I thought it meant building a
> new venv every time I run either 'make' or the tests. This would
> obviously be very noticable for short-running tests like some iotests.
> But fortunately this is not what it does, it keeps the venv around in
> the build directory. So it's only per build directory really, which I
> guess is fine.
>

Whoops, yeah. I meant per build directory. In contrast to the Python unit
tests themselves, which create some test venvs tied directly to the source
directory and are build-agnostic.


> > It seems to work well enough. One thing to note here is that the
> > supported invocation for avocado tests is only through the Makefile,
> > which handles creating and entering the venv to make the command
> > seamless.
> >
> > iotests, however, manages its own execution environment with
> > testenv.py, and we support running iotests from outside of the
> > Makefile, for example by going to $build/tests/qemu-iotests and
> > running ./check.
>
> Yes, and I agree that this is important.
>

Figured as much. Not plotting to take this away, I promise. Just getting my
requirements straight before I spend time concocting something.


> > Now ... I could update testenv.py to be smart enough to create and
> > enter a python venv, and have even prototyped this. It seems to work
> > pretty well! This approach seemed like the least invasive to how
> > iotests are expected to be run and used. But a downside with this
> > approach is that now avocado tests and iotests are each managing their
> > own python venv. Worse, vm-tests and device-crash-test are still
> > unhandled entirely.
>
> Is there a reason why testenv.py couldn't enter just the shared venv in
> build/tests/venv?
>

It can, but it'd have to be made first so it can enter it. I figured this
would only happen "on-demand", like how check-avocado works, so I'd need
some way for iotests and the Makefile to share the venv creation code,
which is certainly an option.


> If not, I guess it would be enough if iotests just checks that the venv
> exists and all of the dependencies are there in the right version and
> error out if not, telling the user to run 'make check-venv'.
>

I will spend unreasonable engineering hours avoiding this, if only for
pride. I want everything to be as seamless and easy as it's always been.


> Or actually, it could just unconditionally run 'make check-venv' by
> itself, which is probably easier to implement than checking the
> dependencies and more convenient for the user, too.
>

Oh, that's one way to get them to share the venv-creation pathway. Hadn't
occurred to me, but this might be easy to do.


> (One more detail: 'make check-venv GIT_SUBMODULES_ACTION=ignore' seems
> to make it pretty much instantaneous if the venv is current, but leaving
> the submodule mechanism enabled adds a noticable delay.)
>

Noted.


> > I'd like to find a solution where I create a unified python testing
> > venv tied to the build shared by avocado, iotests, vm-tests and
> > device-crash-test. I'm not completely sure how exactly I'll manage
> > that right now, but I wanted to throw this out there in case there are
> > some requirements I might be overlooking.
> >
> > I think vm-tests and avocado-tests can both have a venv created for
> > them and activated before the test runs. device-crash-test I believe
> > will need a script change in the gitlab ci yaml. iotests is somewhat
> > unique in that it needs to run both by manual invocation and from
> > makefile invocations. If I want a shared VM between all of these, I'll
> > need to isolate the create-and-enter-venv logic somewhere where it can
> > be shared both inside and outside of a Makefile.
>
> If just calling 'make' isn't an option, then moving that part out into a
> separate script probably wouldn't be too hard either. But 'make' has the
> advantage that it already contains the check if the venv is already
> there and requirements.txt 

Re: [PATCH v2 09/15] qga: replace qemu_open_old() with qemu_open_cloexec()

[Marc-André Lureau]

Hi

On Thu, May 5, 2022 at 3:43 PM Markus Armbruster  wrote:

> marcandre.lur...@redhat.com writes:
>
> > From: Marc-André Lureau 
> >
> > qemu_open_old() uses qemu_open_internal() which handles special
> > "/dev/fdset/" path for monitor fd sets, set CLOEXEC, and uses Error
> > reporting (and some O_DIRECT special error casing).
> >
> > The monitor fdset handling is unnecessary for qga, use
> > qemu_open_cloexec() instead.
> >
> > Signed-off-by: Marc-André Lureau 
> > ---
> >  qga/channel-posix.c  | 14 +-
> >  qga/commands-posix.c | 24 
> >  2 files changed, 21 insertions(+), 17 deletions(-)
> >
> > diff --git a/qga/channel-posix.c b/qga/channel-posix.c
> > index 0ce594bc36c2..a1262b130145 100644
> > --- a/qga/channel-posix.c
> > +++ b/qga/channel-posix.c
> > @@ -1,4 +1,5 @@
> >  #include "qemu/osdep.h"
> > +#include "qemu/cutils.h"
> >  #include 
> >  #include "qapi/error.h"
> >  #include "qemu/sockets.h"
> > @@ -128,11 +129,15 @@ static gboolean ga_channel_open(GAChannel *c,
> const gchar *path,
> >  switch (c->method) {
> >  case GA_CHANNEL_VIRTIO_SERIAL: {
> >  assert(fd < 0);
> > -fd = qemu_open_old(path, O_RDWR | O_NONBLOCK
> > +fd = qemu_open_cloexec(
> > +path,
> >  #ifndef CONFIG_SOLARIS
> > -   | O_ASYNC
> > +O_ASYNC |
> >  #endif
> > -   );
> > +O_RDWR | O_NONBLOCK,
> > +0,
> > +errp
> > +);
> >  if (fd == -1) {
> >  error_setg_errno(errp, errno, "error opening channel");
> >  return false;
> > @@ -157,9 +162,8 @@ static gboolean ga_channel_open(GAChannel *c, const
> gchar *path,
> >  struct termios tio;
> >
> >  assert(fd < 0);
> > -fd = qemu_open_old(path, O_RDWR | O_NOCTTY | O_NONBLOCK);
> > +fd = qemu_open_cloexec(path, O_RDWR | O_NOCTTY | O_NONBLOCK, 0,
> errp);
> >  if (fd == -1) {
> > -error_setg_errno(errp, errno, "error opening channel");
> >  return false;
> >  }
> >  tcgetattr(fd, );
> > diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> > index 8ebc327c5e02..f82205e25813 100644
> > --- a/qga/commands-posix.c
> > +++ b/qga/commands-posix.c
> > @@ -1392,6 +1392,7 @@ static GuestDiskInfoList *get_disk_partitions(
> >
> >  static void get_nvme_smart(GuestDiskInfo *disk)
> >  {
> > +Error *err = NULL;
> >  int fd;
> >  GuestNVMeSmart *smart;
> >  NvmeSmartLog log = {0};
> > @@ -1404,9 +1405,10 @@ static void get_nvme_smart(GuestDiskInfo *disk)
> >   | (((sizeof(log) >> 2) - 1) << 16)
> >  };
> >
> > -fd = qemu_open_old(disk->name, O_RDONLY);
> > +fd = qemu_open_cloexec(disk->name, O_RDONLY, 0, );
> >  if (fd == -1) {
> > -g_debug("Failed to open device: %s: %s", disk->name,
> g_strerror(errno));
> > +g_debug("Failed to open device: %s: %s", disk->name,
> error_get_pretty(err));
> > +error_free(err);
> >  return;
> >  }
> >
> > @@ -1737,9 +1739,8 @@ int64_t qmp_guest_fsfreeze_freeze_list(bool
> has_mountpoints,
> >  }
> >  }
> >
> > -fd = qemu_open_old(mount->dirname, O_RDONLY);
> > +fd = qemu_open_cloexec(mount->dirname, O_RDONLY, 0, errp);
> >  if (fd == -1) {
> > -error_setg_errno(errp, errno, "failed to open %s",
> mount->dirname);
> >  goto error;
> >  }
> >
> > @@ -1804,7 +1805,7 @@ int64_t qmp_guest_fsfreeze_thaw(Error **errp)
> >
> >  QTAILQ_FOREACH(mount, , next) {
> >  logged = false;
> > -fd = qemu_open_old(mount->dirname, O_RDONLY);
> > +fd = qemu_open_cloexec(mount->dirname, O_RDONLY, 0, NULL);
> >  if (fd == -1) {
> >  continue;
> >  }
> > @@ -1864,21 +1865,20 @@ static void guest_fsfreeze_cleanup(void)
> >  GuestFilesystemTrimResponse *
> >  qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
> >  {
> > +ERRP_GUARD();
> >  GuestFilesystemTrimResponse *response;
> >  GuestFilesystemTrimResult *result;
> >  int ret = 0;
> >  FsMountList mounts;
> >  struct FsMount *mount;
> >  int fd;
> > -Error *local_err = NULL;
> >  struct fstrim_range r;
> >
> >  slog("guest-fstrim called");
> >
> >  QTAILQ_INIT();
> > -build_fs_mount_list(, _err);
> > -if (local_err) {
> > -error_propagate(errp, local_err);
> > +build_fs_mount_list(, errp);
> > +if (*errp) {
>
> Suggest to change build_fs_mount_list() to return bool, in accordance
> with the guidance under = Rules = in include/qapi/error.h, then do
>
>if (!build_fs_mount_list(, errp)) {
>
>
ack


> No need for ERRP_GUARD() then.
>
>
This is not a demand.
>
> >  return NULL;
> >  }
> >
> > @@ -1890,11 +1890,11 @@ qmp_guest_fstrim(bool has_minimum, int64_t
> minimum, Error **errp)
> >
> >  

Re: [PATCH v2] hw/block/fdc-sysbus: Always mark sysbus floppy controllers as not having DMA

[Markus Armbruster]

Peter Maydell  writes:

> The sysbus floppy controllers (devices sysbus-fdc and sun-fdtwo)
> don't support DMA.  The core floppy controller code expects this to
> be indicated by setting FDCtrl::dma_chann to -1.  This used to be
> done in the device instance_init functions sysbus_fdc_initfn() and
> sun4m_fdc_initfn(), but in commit 1430759ec3e we refactored this code
> and accidentally lost the setting of dma_chann.

Worth a

  Fixes: 1430759ec3e4cb92da224d739c914a0e8d78d786

tag?

>
> For sysbus-fdc this has no ill effects because we were redundantly
> also setting dma_chann in fdctrl_init_sysbus(), but for sun-fdtwo
> this means that guests which try to enable DMA on the floppy
> controller will cause QEMU to crash because FDCtrl::dma is NULL.
>
> Set dma_chann to -1 in the common instance init, and remove the
> redundant code in fdctrl_init_sysbus() that is also setting it.
>
> There is a six-year-old FIXME comment in the jazz board code to the
> effect that in theory it should support doing DMA via a custom DMA
> controller.  If anybody ever chooses to fix that they can do it by
> adding support for setting both FDCtrl::dma_chann and FDCtrl::dma.
> (A QOM link property 'dma-controller' on the sysbus device which can
> be set to an instance of IsaDmaClass is probably the way to go.)
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/958
> Signed-off-by: Peter Maydell 
> Reviewed-by: Philippe Mathieu-Daudé 
> Reviewed-by: Mark Cave-Ayland 

Re: [PATCH v2 15/15] test/qga: use g_auto wherever sensible

[Marc-André Lureau]

Hi

On Thu, May 5, 2022 at 3:47 PM Markus Armbruster  wrote:
>
> marcandre.lur...@redhat.com writes:
>
> > From: Marc-André Lureau 
> >
> > Signed-off-by: Marc-André Lureau 
> > ---
> >  tests/unit/test-qga.c | 125 +++---
> >  1 file changed, 45 insertions(+), 80 deletions(-)
> >
> > diff --git a/tests/unit/test-qga.c b/tests/unit/test-qga.c
> > index ab0b12a2dd16..53cefc2c2649 100644
> > --- a/tests/unit/test-qga.c
> > +++ b/tests/unit/test-qga.c
> > @@ -51,8 +51,11 @@ static void
> >  fixture_setup(TestFixture *fixture, gconstpointer data, gchar **envp)
> >  {
> >  const gchar *extra_arg = data;
> > -GError *error = NULL;
> > -gchar *cwd, *path, *cmd, **argv = NULL;
> > +g_autoptr(GError) error = NULL;
> > +g_autofree char *cwd = NULL;
> > +g_autofree char *path = NULL;
> > +g_autofree char *cmd = NULL;
> > +g_auto(GStrv) argv = NULL;
>
> Arranges five variables to be auto-freed, where ...
>
> >
> >  fixture->loop = g_main_loop_new(NULL, FALSE);
> >
> > @@ -78,17 +81,12 @@ fixture_setup(TestFixture *fixture, gconstpointer data, 
> > gchar **envp)
> >
> >  fixture->fd = connect_qga(path);
> >  g_assert_cmpint(fixture->fd, !=, -1);
> > -
> > -g_strfreev(argv);
> > -g_free(cmd);
> > -g_free(cwd);
> > -g_free(path);
>
> ... only four were freed before.  The extra one is @error.  Which can
> only be null here, thanks to g_assert_no_error(), can't it?

Right, but for consistency we can have it. I can drop it too.

Re: [PATCH v2 15/15] test/qga: use g_auto wherever sensible

[Markus Armbruster]

marcandre.lur...@redhat.com writes:

> From: Marc-André Lureau 
>
> Signed-off-by: Marc-André Lureau 
> ---
>  tests/unit/test-qga.c | 125 +++---
>  1 file changed, 45 insertions(+), 80 deletions(-)
>
> diff --git a/tests/unit/test-qga.c b/tests/unit/test-qga.c
> index ab0b12a2dd16..53cefc2c2649 100644
> --- a/tests/unit/test-qga.c
> +++ b/tests/unit/test-qga.c
> @@ -51,8 +51,11 @@ static void
>  fixture_setup(TestFixture *fixture, gconstpointer data, gchar **envp)
>  {
>  const gchar *extra_arg = data;
> -GError *error = NULL;
> -gchar *cwd, *path, *cmd, **argv = NULL;
> +g_autoptr(GError) error = NULL;
> +g_autofree char *cwd = NULL;
> +g_autofree char *path = NULL;
> +g_autofree char *cmd = NULL;
> +g_auto(GStrv) argv = NULL;

Arranges five variables to be auto-freed, where ...

>  
>  fixture->loop = g_main_loop_new(NULL, FALSE);
>  
> @@ -78,17 +81,12 @@ fixture_setup(TestFixture *fixture, gconstpointer data, 
> gchar **envp)
>  
>  fixture->fd = connect_qga(path);
>  g_assert_cmpint(fixture->fd, !=, -1);
> -
> -g_strfreev(argv);
> -g_free(cmd);
> -g_free(cwd);
> -g_free(path);

... only four were freed before.  The extra one is @error.  Which can
only be null here, thanks to g_assert_no_error(), can't it?

>  }

Didn't look further.

[...]

Re: [PATCH v2 08/15] qga: throw an Error in ga_channel_open()

[Marc-André Lureau]

Hi

On Thu, May 5, 2022 at 3:41 PM Markus Armbruster  wrote:

> marcandre.lur...@redhat.com writes:
>
> > From: Marc-André Lureau 
> >
> > Allow for a single point of error reporting, and further refactoring.
>
> This sounds like there is no behavioral change intended.  But it looks
> like there is change; see below.
>
> > Signed-off-by: Marc-André Lureau 
> > ---
> >  qga/channel-posix.c | 42 +-
> >  1 file changed, 17 insertions(+), 25 deletions(-)
> >
> > diff --git a/qga/channel-posix.c b/qga/channel-posix.c
> > index a996858e2492..0ce594bc36c2 100644
> > --- a/qga/channel-posix.c
> > +++ b/qga/channel-posix.c
> > @@ -119,8 +119,9 @@ static int ga_channel_client_add(GAChannel *c, int
> fd)
> >  }
> >
> >  static gboolean ga_channel_open(GAChannel *c, const gchar *path,
> > -GAChannelMethod method, int fd)
> > +GAChannelMethod method, int fd, Error
> **errp)
> >  {
> > +ERRP_GUARD();
> >  int ret;
> >  c->method = method;
> >
> > @@ -133,21 +134,20 @@ static gboolean ga_channel_open(GAChannel *c,
> const gchar *path,
> >  #endif
> > );
> >  if (fd == -1) {
> > -g_critical("error opening channel: %s", strerror(errno));
> > +error_setg_errno(errp, errno, "error opening channel");
> >  return false;
> >  }
> >  #ifdef CONFIG_SOLARIS
> >  ret = ioctl(fd, I_SETSIG, S_OUTPUT | S_INPUT | S_HIPRI);
> >  if (ret == -1) {
> > -g_critical("error setting event mask for channel: %s",
> > -   strerror(errno));
> > +error_setg_errno(errp, errno, "error setting event mask for
> channel");
> >  close(fd);
> >  return false;
> >  }
> >  #endif
> >  ret = ga_channel_client_add(c, fd);
> >  if (ret) {
> > -g_critical("error adding channel to main loop");
> > +error_setg(errp, "error adding channel to main loop");
> >  close(fd);
> >  return false;
> >  }
> > @@ -159,7 +159,7 @@ static gboolean ga_channel_open(GAChannel *c, const
> gchar *path,
> >  assert(fd < 0);
> >  fd = qemu_open_old(path, O_RDWR | O_NOCTTY | O_NONBLOCK);
> >  if (fd == -1) {
> > -g_critical("error opening channel: %s", strerror(errno));
> > +error_setg_errno(errp, errno, "error opening channel");
> >  return false;
> >  }
> >  tcgetattr(fd, );
> > @@ -180,7 +180,7 @@ static gboolean ga_channel_open(GAChannel *c, const
> gchar *path,
> >  tcsetattr(fd, TCSANOW, );
> >  ret = ga_channel_client_add(c, fd);
> >  if (ret) {
> > -g_critical("error adding channel to main loop");
> > +error_setg(errp, "error adding channel to main loop");
> >  close(fd);
> >  return false;
> >  }
> > @@ -188,12 +188,8 @@ static gboolean ga_channel_open(GAChannel *c, const
> gchar *path,
> >  }
> >  case GA_CHANNEL_UNIX_LISTEN: {
> >  if (fd < 0) {
> > -Error *local_err = NULL;
> > -
> > -fd = unix_listen(path, _err);
> > -if (local_err != NULL) {
> > -g_critical("%s", error_get_pretty(local_err));
> > -error_free(local_err);
> > +fd = unix_listen(path, errp);
> > +if (fd < 0) {
> >  return false;
> >  }
> >  }
> > @@ -202,24 +198,19 @@ static gboolean ga_channel_open(GAChannel *c,
> const gchar *path,
> >  }
> >  case GA_CHANNEL_VSOCK_LISTEN: {
> >  if (fd < 0) {
> > -Error *local_err = NULL;
> >  SocketAddress *addr;
> >  char *addr_str;
> >
> >  addr_str = g_strdup_printf("vsock:%s", path);
> > -addr = socket_parse(addr_str, _err);
> > +addr = socket_parse(addr_str, errp);
> >  g_free(addr_str);
> > -if (local_err != NULL) {
> > -g_critical("%s", error_get_pretty(local_err));
> > -error_free(local_err);
> > +if (*errp) {
>
> Recommend
>
>if (!addr) {
>
>
ok


> >  return false;
> >  }
> >
> > -fd = socket_listen(addr, 1, _err);
> > +fd = socket_listen(addr, 1, errp);
> >  qapi_free_SocketAddress(addr);
> > -if (local_err != NULL) {
> > -g_critical("%s", error_get_pretty(local_err));
> > -error_free(local_err);
> > +if (*errp) {
>
> Recommend
>
>if (fd < 0) {
>
>
ok


> Do you still need ERRP_GUARD() then?
>

It's more future-proof, but could be dropped then.


>
> >  return false;
> >  }
> >  }
> > @@ -227,7 +218,7 @@ static gboolean ga_channel_open(GAChannel *c, const
> gchar *path,
> >  

Re: [PATCH v2 09/15] qga: replace qemu_open_old() with qemu_open_cloexec()

[Markus Armbruster]

marcandre.lur...@redhat.com writes:

> From: Marc-André Lureau 
>
> qemu_open_old() uses qemu_open_internal() which handles special
> "/dev/fdset/" path for monitor fd sets, set CLOEXEC, and uses Error
> reporting (and some O_DIRECT special error casing).
>
> The monitor fdset handling is unnecessary for qga, use
> qemu_open_cloexec() instead.
>
> Signed-off-by: Marc-André Lureau 
> ---
>  qga/channel-posix.c  | 14 +-
>  qga/commands-posix.c | 24 
>  2 files changed, 21 insertions(+), 17 deletions(-)
>
> diff --git a/qga/channel-posix.c b/qga/channel-posix.c
> index 0ce594bc36c2..a1262b130145 100644
> --- a/qga/channel-posix.c
> +++ b/qga/channel-posix.c
> @@ -1,4 +1,5 @@
>  #include "qemu/osdep.h"
> +#include "qemu/cutils.h"
>  #include 
>  #include "qapi/error.h"
>  #include "qemu/sockets.h"
> @@ -128,11 +129,15 @@ static gboolean ga_channel_open(GAChannel *c, const 
> gchar *path,
>  switch (c->method) {
>  case GA_CHANNEL_VIRTIO_SERIAL: {
>  assert(fd < 0);
> -fd = qemu_open_old(path, O_RDWR | O_NONBLOCK
> +fd = qemu_open_cloexec(
> +path,
>  #ifndef CONFIG_SOLARIS
> -   | O_ASYNC
> +O_ASYNC |
>  #endif
> -   );
> +O_RDWR | O_NONBLOCK,
> +0,
> +errp
> +);
>  if (fd == -1) {
>  error_setg_errno(errp, errno, "error opening channel");
>  return false;
> @@ -157,9 +162,8 @@ static gboolean ga_channel_open(GAChannel *c, const gchar 
> *path,
>  struct termios tio;
>  
>  assert(fd < 0);
> -fd = qemu_open_old(path, O_RDWR | O_NOCTTY | O_NONBLOCK);
> +fd = qemu_open_cloexec(path, O_RDWR | O_NOCTTY | O_NONBLOCK, 0, 
> errp);
>  if (fd == -1) {
> -error_setg_errno(errp, errno, "error opening channel");
>  return false;
>  }
>  tcgetattr(fd, );
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 8ebc327c5e02..f82205e25813 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -1392,6 +1392,7 @@ static GuestDiskInfoList *get_disk_partitions(
>  
>  static void get_nvme_smart(GuestDiskInfo *disk)
>  {
> +Error *err = NULL;
>  int fd;
>  GuestNVMeSmart *smart;
>  NvmeSmartLog log = {0};
> @@ -1404,9 +1405,10 @@ static void get_nvme_smart(GuestDiskInfo *disk)
>   | (((sizeof(log) >> 2) - 1) << 16)
>  };
>  
> -fd = qemu_open_old(disk->name, O_RDONLY);
> +fd = qemu_open_cloexec(disk->name, O_RDONLY, 0, );
>  if (fd == -1) {
> -g_debug("Failed to open device: %s: %s", disk->name, 
> g_strerror(errno));
> +g_debug("Failed to open device: %s: %s", disk->name, 
> error_get_pretty(err));
> +error_free(err);
>  return;
>  }
>  
> @@ -1737,9 +1739,8 @@ int64_t qmp_guest_fsfreeze_freeze_list(bool 
> has_mountpoints,
>  }
>  }
>  
> -fd = qemu_open_old(mount->dirname, O_RDONLY);
> +fd = qemu_open_cloexec(mount->dirname, O_RDONLY, 0, errp);
>  if (fd == -1) {
> -error_setg_errno(errp, errno, "failed to open %s", 
> mount->dirname);
>  goto error;
>  }
>  
> @@ -1804,7 +1805,7 @@ int64_t qmp_guest_fsfreeze_thaw(Error **errp)
>  
>  QTAILQ_FOREACH(mount, , next) {
>  logged = false;
> -fd = qemu_open_old(mount->dirname, O_RDONLY);
> +fd = qemu_open_cloexec(mount->dirname, O_RDONLY, 0, NULL);
>  if (fd == -1) {
>  continue;
>  }
> @@ -1864,21 +1865,20 @@ static void guest_fsfreeze_cleanup(void)
>  GuestFilesystemTrimResponse *
>  qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
>  {
> +ERRP_GUARD();
>  GuestFilesystemTrimResponse *response;
>  GuestFilesystemTrimResult *result;
>  int ret = 0;
>  FsMountList mounts;
>  struct FsMount *mount;
>  int fd;
> -Error *local_err = NULL;
>  struct fstrim_range r;
>  
>  slog("guest-fstrim called");
>  
>  QTAILQ_INIT();
> -build_fs_mount_list(, _err);
> -if (local_err) {
> -error_propagate(errp, local_err);
> +build_fs_mount_list(, errp);
> +if (*errp) {

Suggest to change build_fs_mount_list() to return bool, in accordance
with the guidance under = Rules = in include/qapi/error.h, then do

   if (!build_fs_mount_list(, errp)) {

No need for ERRP_GUARD() then.

This is not a demand.

>  return NULL;
>  }
>  
> @@ -1890,11 +1890,11 @@ qmp_guest_fstrim(bool has_minimum, int64_t minimum, 
> Error **errp)
>  
>  QAPI_LIST_PREPEND(response->paths, result);
>  
> -fd = qemu_open_old(mount->dirname, O_RDONLY);
> +fd = qemu_open_cloexec(mount->dirname, O_RDONLY, 0, errp);
>  if (fd == -1) {
> -result->error = g_strdup_printf("failed to open: %s",
> -strerror(errno));
> +

Re: [PATCH v2 08/15] qga: throw an Error in ga_channel_open()

[Markus Armbruster]

marcandre.lur...@redhat.com writes:

> From: Marc-André Lureau 
>
> Allow for a single point of error reporting, and further refactoring.

This sounds like there is no behavioral change intended.  But it looks
like there is change; see below.

> Signed-off-by: Marc-André Lureau 
> ---
>  qga/channel-posix.c | 42 +-
>  1 file changed, 17 insertions(+), 25 deletions(-)
>
> diff --git a/qga/channel-posix.c b/qga/channel-posix.c
> index a996858e2492..0ce594bc36c2 100644
> --- a/qga/channel-posix.c
> +++ b/qga/channel-posix.c
> @@ -119,8 +119,9 @@ static int ga_channel_client_add(GAChannel *c, int fd)
>  }
>  
>  static gboolean ga_channel_open(GAChannel *c, const gchar *path,
> -GAChannelMethod method, int fd)
> +GAChannelMethod method, int fd, Error **errp)
>  {
> +ERRP_GUARD();
>  int ret;
>  c->method = method;
>  
> @@ -133,21 +134,20 @@ static gboolean ga_channel_open(GAChannel *c, const 
> gchar *path,
>  #endif
> );
>  if (fd == -1) {
> -g_critical("error opening channel: %s", strerror(errno));
> +error_setg_errno(errp, errno, "error opening channel");
>  return false;
>  }
>  #ifdef CONFIG_SOLARIS
>  ret = ioctl(fd, I_SETSIG, S_OUTPUT | S_INPUT | S_HIPRI);
>  if (ret == -1) {
> -g_critical("error setting event mask for channel: %s",
> -   strerror(errno));
> +error_setg_errno(errp, errno, "error setting event mask for 
> channel");
>  close(fd);
>  return false;
>  }
>  #endif
>  ret = ga_channel_client_add(c, fd);
>  if (ret) {
> -g_critical("error adding channel to main loop");
> +error_setg(errp, "error adding channel to main loop");
>  close(fd);
>  return false;
>  }
> @@ -159,7 +159,7 @@ static gboolean ga_channel_open(GAChannel *c, const gchar 
> *path,
>  assert(fd < 0);
>  fd = qemu_open_old(path, O_RDWR | O_NOCTTY | O_NONBLOCK);
>  if (fd == -1) {
> -g_critical("error opening channel: %s", strerror(errno));
> +error_setg_errno(errp, errno, "error opening channel");
>  return false;
>  }
>  tcgetattr(fd, );
> @@ -180,7 +180,7 @@ static gboolean ga_channel_open(GAChannel *c, const gchar 
> *path,
>  tcsetattr(fd, TCSANOW, );
>  ret = ga_channel_client_add(c, fd);
>  if (ret) {
> -g_critical("error adding channel to main loop");
> +error_setg(errp, "error adding channel to main loop");
>  close(fd);
>  return false;
>  }
> @@ -188,12 +188,8 @@ static gboolean ga_channel_open(GAChannel *c, const 
> gchar *path,
>  }
>  case GA_CHANNEL_UNIX_LISTEN: {
>  if (fd < 0) {
> -Error *local_err = NULL;
> -
> -fd = unix_listen(path, _err);
> -if (local_err != NULL) {
> -g_critical("%s", error_get_pretty(local_err));
> -error_free(local_err);
> +fd = unix_listen(path, errp);
> +if (fd < 0) {
>  return false;
>  }
>  }
> @@ -202,24 +198,19 @@ static gboolean ga_channel_open(GAChannel *c, const 
> gchar *path,
>  }
>  case GA_CHANNEL_VSOCK_LISTEN: {
>  if (fd < 0) {
> -Error *local_err = NULL;
>  SocketAddress *addr;
>  char *addr_str;
>  
>  addr_str = g_strdup_printf("vsock:%s", path);
> -addr = socket_parse(addr_str, _err);
> +addr = socket_parse(addr_str, errp);
>  g_free(addr_str);
> -if (local_err != NULL) {
> -g_critical("%s", error_get_pretty(local_err));
> -error_free(local_err);
> +if (*errp) {

Recommend

   if (!addr) {

>  return false;
>  }
>  
> -fd = socket_listen(addr, 1, _err);
> +fd = socket_listen(addr, 1, errp);
>  qapi_free_SocketAddress(addr);
> -if (local_err != NULL) {
> -g_critical("%s", error_get_pretty(local_err));
> -error_free(local_err);
> +if (*errp) {

Recommend

   if (fd < 0) {

Do you still need ERRP_GUARD() then?

>  return false;
>  }
>  }
> @@ -227,7 +218,7 @@ static gboolean ga_channel_open(GAChannel *c, const gchar 
> *path,
>  break;
>  }
>  default:
> -g_critical("error binding/listening to specified socket");
> +error_setg(errp, "error binding/listening to specified socket");
>  return false;
>  }
>  
> @@ -272,12 +263,13 @@ GIOStatus ga_channel_read(GAChannel *c, gchar *buf, 
> gsize size, gsize *count)
>  GAChannel 

Re: [PATCH v2 07/15] qga: use qemu_open_cloexec() for safe_open_or_create()

[Markus Armbruster]

marcandre.lur...@redhat.com writes:

> From: Marc-André Lureau 
>
> The function takes care of setting CLOEXEC, and reporting error.
>
> Signed-off-by: Marc-André Lureau 
> ---
>  qga/commands-posix.c | 11 +++
>  1 file changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 0ef049650e31..8ebc327c5e02 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -370,21 +370,16 @@ safe_open_or_create(const char *path, const char *mode, 
> Error **errp)
>   * open() is decisive and its third argument is ignored, and the second
>   * open() and the fchmod() are never called.
>   */
> -fd = open(path, oflag | ((oflag & O_CREAT) ? O_EXCL : 0), 0);
> +fd = qemu_open_cloexec(path, oflag | ((oflag & O_CREAT) ? O_EXCL : 0), 
> 0, errp);
>  if (fd == -1 && errno == EEXIST) {
> +g_clear_pointer(errp, error_free);

Aha, this is where you really need ERRP_GUARD().

Elsewhere, we use

   error_free(errp);
   errp = NULL;

If one of these two ways is superior, we should use the superior one
everywhere.

If it's a wash, we should stick to the prevalent way.

>  oflag &= ~(unsigned)O_CREAT;
> -fd = open(path, oflag);
> +fd = qemu_open_cloexec(path, oflag, 0, errp);
>  }
>  if (fd == -1) {
> -error_setg_errno(errp, errno,
> - "failed to open file '%s' "
> - "(mode: '%s')",
> - path, mode);

This changes the error message, doesn't it?

Not an objection, just something that might be worth noting in the
commit message.

>  goto end;
>  }
>  
> -qemu_set_cloexec(fd);
> -
>  if ((oflag & O_CREAT) && fchmod(fd, DEFAULT_NEW_FILE_MODE) == -1) {
>  error_setg_errno(errp, errno,
>   "failed to set permission 0%03o on new file '%s' 
> (mode: '%s')",

Re: [PATCH v2 05/15] qga: flatten safe_open_or_create()

[Marc-André Lureau]

Hi

On Thu, May 5, 2022 at 3:20 PM Markus Armbruster  wrote:

> marcandre.lur...@redhat.com writes:
>
> > From: Marc-André Lureau 
> >
> > There is a bit too much branching in the function, this can be
> > simplified a bit, and have a common exit point thanks to ERRP_PROPAGATE.
>
> Do you mean ERRP_GUARD()?
>

yes


>
> I'm not sure the commit reduces "branching", but it certainly reduces
> nesting, which I find improves readability.
>

ok


>
> > This also helps with the following error handling changes.
> >
> > Signed-off-by: Marc-André Lureau 
> > ---
> >  qga/commands-posix.c | 124 ++-
> >  1 file changed, 63 insertions(+), 61 deletions(-)
>
> I think the diff is easier to review with space change ignored:
>
> | diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> | index 78f2f21001..b4b19d3eb4 100644
> | --- a/qga/commands-posix.c
> | +++ b/qga/commands-posix.c
> | @@ -315,12 +315,14 @@ find_open_flag(const char *mode_str, Error **errp)
> |  static FILE *
> |  safe_open_or_create(const char *path, const char *mode, Error **errp)
> |  {
> | -Error *local_err = NULL;
> | -int oflag;
> | +ERRP_GUARD();
> | +int oflag, fd = -1;
>
> I'd prefer
>
>   +ERRP_GUARD();
>int oflag;
>   +int fd = -1;
>
>
ok


> because it's slightly less churn, and I dislike variables with and
> without initializer in the same declaration.  Matter of taste.
>
> | +FILE *f = NULL;
> |
> | -oflag = find_open_flag(mode, _err);
> | -if (local_err == NULL) {
> | -int fd;
> | +oflag = find_open_flag(mode, errp);
> | +if (*errp) {
>
> I'd prefer
>
>if (oflag < 0) {
>
> No need for ERRP_GUARD() then, as far as I can tell.
>
>
"qga: use qemu_open_cloexec() for safe_open_or_create()" expects it to be
non-null though, I can move it there.


> | +goto end;
> | +}
> |
> |  /* If the caller wants / allows creation of a new file, we
> implement it
> |   * with a two step process: open() + (open() / fchmod()).
> | @@ -349,39 +351,39 @@ safe_open_or_create(const char *path, const char
> *mode, Error **errp)
> |  oflag &= ~(unsigned)O_CREAT;
> |  fd = open(path, oflag);
> |  }
> | -
> |  if (fd == -1) {
> | -error_setg_errno(_err, errno, "failed to open file
> '%s' "
> | - "(mode: '%s')", path, mode);
> | -} else {
> | +error_setg_errno(errp, errno,
> | + "failed to open file '%s' "
> | + "(mode: '%s')",
> | + path, mode);
> | +goto end;
> | +}
> | +
> |  qemu_set_cloexec(fd);
> |
> |  if ((oflag & O_CREAT) && fchmod(fd, DEFAULT_NEW_FILE_MODE) == -1) {
> | -error_setg_errno(_err, errno, "failed to set
> permission "
> | - "0%03o on new file '%s' (mode: '%s')",
> | +error_setg_errno(errp, errno,
> | + "failed to set permission 0%03o on new file
> '%s' (mode: '%s')",
> |   (unsigned)DEFAULT_NEW_FILE_MODE, path, mode);
> | -} else {
> | -FILE *f;
> | +goto end;
> | +}
> |
> |  f = fdopen(fd, mode);
> |  if (f == NULL) {
> | -error_setg_errno(_err, errno, "failed to
> associate "
> | - "stdio stream with file descriptor
> %d, "
> | - "file '%s' (mode: '%s')", fd,
> path, mode);
> | -} else {
> | -return f;
> | -}
> | +error_setg_errno(errp, errno,
> | + "failed to associate stdio stream with file
> descriptor %d, "
> | + "file '%s' (mode: '%s')",
> | + fd, path, mode);
> |  }
> |
> | +end:
> | +if (f == NULL && fd != -1) {
> |  close(fd);
> |  if (oflag & O_CREAT) {
> |  unlink(path);
> |  }
> |  }
> | -}
> | -
> | -error_propagate(errp, local_err);
> | -return NULL;
> | +return f;
> |  }
> |
> |  int64_t qmp_guest_file_open(const char *path, bool has_mode, const char
> *mode,
>
>
>

-- 
Marc-André Lureau

Re: [PATCH v2 05/15] qga: flatten safe_open_or_create()

[Markus Armbruster]

marcandre.lur...@redhat.com writes:

> From: Marc-André Lureau 
>
> There is a bit too much branching in the function, this can be
> simplified a bit, and have a common exit point thanks to ERRP_PROPAGATE.

Do you mean ERRP_GUARD()?

I'm not sure the commit reduces "branching", but it certainly reduces
nesting, which I find improves readability.

> This also helps with the following error handling changes.
>
> Signed-off-by: Marc-André Lureau 
> ---
>  qga/commands-posix.c | 124 ++-
>  1 file changed, 63 insertions(+), 61 deletions(-)

I think the diff is easier to review with space change ignored:

| diff --git a/qga/commands-posix.c b/qga/commands-posix.c
| index 78f2f21001..b4b19d3eb4 100644
| --- a/qga/commands-posix.c
| +++ b/qga/commands-posix.c
| @@ -315,12 +315,14 @@ find_open_flag(const char *mode_str, Error **errp)
|  static FILE *
|  safe_open_or_create(const char *path, const char *mode, Error **errp)
|  {
| -Error *local_err = NULL;
| -int oflag;
| +ERRP_GUARD();
| +int oflag, fd = -1;

I'd prefer

  +ERRP_GUARD();
   int oflag;
  +int fd = -1;
  
because it's slightly less churn, and I dislike variables with and
without initializer in the same declaration.  Matter of taste.

| +FILE *f = NULL;
| 
| -oflag = find_open_flag(mode, _err);
| -if (local_err == NULL) {
| -int fd;
| +oflag = find_open_flag(mode, errp);
| +if (*errp) {

I'd prefer

   if (oflag < 0) {

No need for ERRP_GUARD() then, as far as I can tell.

| +goto end;
| +}
| 
|  /* If the caller wants / allows creation of a new file, we implement it
|   * with a two step process: open() + (open() / fchmod()).
| @@ -349,39 +351,39 @@ safe_open_or_create(const char *path, const char *mode, 
Error **errp)
|  oflag &= ~(unsigned)O_CREAT;
|  fd = open(path, oflag);
|  }
| -
|  if (fd == -1) {
| -error_setg_errno(_err, errno, "failed to open file '%s' "
| - "(mode: '%s')", path, mode);
| -} else {
| +error_setg_errno(errp, errno,
| + "failed to open file '%s' "
| + "(mode: '%s')",
| + path, mode);
| +goto end;
| +}
| +
|  qemu_set_cloexec(fd);
| 
|  if ((oflag & O_CREAT) && fchmod(fd, DEFAULT_NEW_FILE_MODE) == -1) {
| -error_setg_errno(_err, errno, "failed to set 
permission "
| - "0%03o on new file '%s' (mode: '%s')",
| +error_setg_errno(errp, errno,
| + "failed to set permission 0%03o on new file '%s' 
(mode: '%s')",
|   (unsigned)DEFAULT_NEW_FILE_MODE, path, mode);
| -} else {
| -FILE *f;
| +goto end;
| +}
| 
|  f = fdopen(fd, mode);
|  if (f == NULL) {
| -error_setg_errno(_err, errno, "failed to associate 
"
| - "stdio stream with file descriptor %d, "
| - "file '%s' (mode: '%s')", fd, path, 
mode);
| -} else {
| -return f;
| -}
| +error_setg_errno(errp, errno,
| + "failed to associate stdio stream with file 
descriptor %d, "
| + "file '%s' (mode: '%s')",
| + fd, path, mode);
|  }
| 
| +end:
| +if (f == NULL && fd != -1) {
|  close(fd);
|  if (oflag & O_CREAT) {
|  unlink(path);
|  }
|  }
| -}
| -
| -error_propagate(errp, local_err);
| -return NULL;
| +return f;
|  }
| 
|  int64_t qmp_guest_file_open(const char *path, bool has_mode, const char 
*mode,

Re: iotests and python dependencies

[Kevin Wolf]

Am 05.05.2022 um 11:28 hat Paolo Bonzini geschrieben:
> On 5/5/22 10:51, Kevin Wolf wrote:
> > If not, I guess it would be enough if iotests just checks that the venv
> > exists and all of the dependencies are there in the right version and
> > error out if not, telling the user to run 'make check-venv'.
> > 
> > Or actually, it could just unconditionally run 'make check-venv' by
> > itself, which is probably easier to implement than checking the
> > dependencies and more convenient for the user, too.
> 
> Note that you would still have to add a 'check-block: check-venv'
> dependency in the Makefile, otherwise two "instances" of check-venv
> could run in parallel.

Good point. I only considered manual invocations, but for 'make
check-block' that sounds right.

> One small complication is that on BSD systems the binary is actually
> called "gmake", so you'd have to pass the variable somehow

I guess we could just export $MAKE as an environment variable?

Kevin

Re: [PATCH v2 03/15] tests: make libqmp buildable for win32

[Marc-André Lureau]

Hi

On Thu, May 5, 2022 at 2:52 PM Markus Armbruster  wrote:
>
> marcandre.lur...@redhat.com writes:
>
> > From: Marc-André Lureau 
> >
> > Signed-off-by: Marc-André Lureau 
> > Reviewed-by: Thomas Huth 
> > ---
> >  tests/qtest/libqmp.h |  2 ++
> >  tests/qtest/libqmp.c | 35 +--
> >  2 files changed, 31 insertions(+), 6 deletions(-)
> >
> > diff --git a/tests/qtest/libqmp.h b/tests/qtest/libqmp.h
> > index 94aa97328a17..772f18b73ba3 100644
> > --- a/tests/qtest/libqmp.h
> > +++ b/tests/qtest/libqmp.h
> > @@ -20,8 +20,10 @@
> >  #include "qapi/qmp/qdict.h"
> >
> >  QDict *qmp_fd_receive(int fd);
> > +#ifndef G_OS_WIN32
>
> What's the difference between G_OS_WIN32 and _WIN32?
>
> We have 10 of the former, but >250 of the latter.  If they are
> effectively the same, we should pick one and stick to it.

There are some subtle differences when compiling for cygwin, in which
case G_OS_WIN32 is not defined.

I usually pick G_OS_{UNIX,WIN32} defines, mostly for consistency, but
in many situation _WIN32/WIN32 is fine.

(and we also have CONFIG_WIN32)

Re: [PATCH v2 03/15] tests: make libqmp buildable for win32

[Markus Armbruster]

marcandre.lur...@redhat.com writes:

> From: Marc-André Lureau 
>
> Signed-off-by: Marc-André Lureau 
> Reviewed-by: Thomas Huth 
> ---
>  tests/qtest/libqmp.h |  2 ++
>  tests/qtest/libqmp.c | 35 +--
>  2 files changed, 31 insertions(+), 6 deletions(-)
>
> diff --git a/tests/qtest/libqmp.h b/tests/qtest/libqmp.h
> index 94aa97328a17..772f18b73ba3 100644
> --- a/tests/qtest/libqmp.h
> +++ b/tests/qtest/libqmp.h
> @@ -20,8 +20,10 @@
>  #include "qapi/qmp/qdict.h"
>  
>  QDict *qmp_fd_receive(int fd);
> +#ifndef G_OS_WIN32

What's the difference between G_OS_WIN32 and _WIN32?

We have 10 of the former, but >250 of the latter.  If they are
effectively the same, we should pick one and stick to it.

[...]

Re: [PATCH v2 04/15] include: adjust header guards after renaming

[Markus Armbruster]

marcandre.lur...@redhat.com writes:

> From: Marc-André Lureau 

Pointing to commit 49f9522193 "include: rename qemu-common.h
qemu/help-texts.h" wouldn't hurt.

> Signed-off-by: Marc-André Lureau 
> Reviewed-by: Stefan Weil 
> ---
>  include/qemu/help-texts.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/qemu/help-texts.h b/include/qemu/help-texts.h
> index ba32cc8b1f39..4f265fed8df1 100644
> --- a/include/qemu/help-texts.h
> +++ b/include/qemu/help-texts.h
> @@ -1,5 +1,5 @@
> -#ifndef QEMU_COMMON_H
> -#define QEMU_COMMON_H
> +#ifndef QEMU_HELP_TEXTS_H
> +#define QEMU_HELP_TEXTS_H
>  
>  /* Copyright string for -version arguments, About dialogs, etc */
>  #define QEMU_COPYRIGHT "Copyright (c) 2003-2022 " \

Reviewed-by: Markus Armbruster 

Time for a re-run of scripts/clean-header-guards.pl.

[PATCH] qemu-iotests: inline common.config into common.rc

[Paolo Bonzini]

common.rc has some complicated logic to find the common.config that
dates back to xfstests and is completely unnecessary now.  Just include
the contents of the file.

Signed-off-by: Paolo Bonzini 
---
 tests/qemu-iotests/common.config | 41 
 tests/qemu-iotests/common.rc | 31 ++--
 2 files changed, 19 insertions(+), 53 deletions(-)
 delete mode 100644 tests/qemu-iotests/common.config

diff --git a/tests/qemu-iotests/common.config b/tests/qemu-iotests/common.config
deleted file mode 100644
index 9bd1a5a6fc..00
--- a/tests/qemu-iotests/common.config
+++ /dev/null
@@ -1,41 +0,0 @@
-#!/usr/bin/env bash
-#
-# Copyright (C) 2009 Red Hat, Inc.
-# Copyright (c) 2000-2003,2006 Silicon Graphics, Inc.  All Rights Reserved.
-#
-# This program is free software; you can redistribute it and/or
-# modify it under the terms of the GNU General Public License as
-# published by the Free Software Foundation.
-#
-# This program is distributed in the hope that it would be useful,
-# but WITHOUT ANY WARRANTY; without even the implied warranty of
-# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-# GNU General Public License for more details.
-#
-# You should have received a copy of the GNU General Public License
-# along with this program.  If not, see .
-#
-# all tests should use a common language setting to prevent golden
-# output mismatches.
-export LANG=C
-
-PATH=".:$PATH"
-
-HOSTOS=$(uname -s)
-arch=$(uname -m)
-[[ "$arch" =~ "ppc64" ]] && qemu_arch=ppc64 || qemu_arch="$arch"
-
-# make sure we have a standard umask
-umask 022
-
-_optstr_add()
-{
-if [ -n "$1" ]; then
-echo "$1,$2"
-else
-echo "$2"
-fi
-}
-
-# make sure this script returns success
-true
diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 227e0a5be9..165b54a61e 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -17,6 +17,17 @@
 # along with this program.  If not, see .
 #
 
+export LANG=C
+
+PATH=".:$PATH"
+
+HOSTOS=$(uname -s)
+arch=$(uname -m)
+[[ "$arch" =~ "ppc64" ]] && qemu_arch=ppc64 || qemu_arch="$arch"
+
+# make sure we have a standard umask
+umask 022
+
 # bail out, setting up .notrun file
 _notrun()
 {
@@ -120,18 +131,14 @@ peek_file_raw()
 dd if="$1" bs=1 skip="$2" count="$3" status=none
 }
 
-config=common.config
-test -f $config || config=../common.config
-if ! test -f $config
-then
-echo "$0: failed to find common.config"
-exit 1
-fi
-if ! . $config
-then
-echo "$0: failed to source common.config"
-exit 1
-fi
+_optstr_add()
+{
+if [ -n "$1" ]; then
+echo "$1,$2"
+else
+echo "$2"
+fi
+}
 
 # Set the variables to the empty string to turn Valgrind off
 # for specific processes, e.g.
-- 
2.35.1

[PATCH v2] hw/block/fdc-sysbus: Always mark sysbus floppy controllers as not having DMA

[Peter Maydell]

The sysbus floppy controllers (devices sysbus-fdc and sun-fdtwo)
don't support DMA.  The core floppy controller code expects this to
be indicated by setting FDCtrl::dma_chann to -1.  This used to be
done in the device instance_init functions sysbus_fdc_initfn() and
sun4m_fdc_initfn(), but in commit 1430759ec3e we refactored this code
and accidentally lost the setting of dma_chann.

For sysbus-fdc this has no ill effects because we were redundantly
also setting dma_chann in fdctrl_init_sysbus(), but for sun-fdtwo
this means that guests which try to enable DMA on the floppy
controller will cause QEMU to crash because FDCtrl::dma is NULL.

Set dma_chann to -1 in the common instance init, and remove the
redundant code in fdctrl_init_sysbus() that is also setting it.

There is a six-year-old FIXME comment in the jazz board code to the
effect that in theory it should support doing DMA via a custom DMA
controller.  If anybody ever chooses to fix that they can do it by
adding support for setting both FDCtrl::dma_chann and FDCtrl::dma.
(A QOM link property 'dma-controller' on the sysbus device which can
be set to an instance of IsaDmaClass is probably the way to go.)

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/958
Signed-off-by: Peter Maydell 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Mark Cave-Ayland 
---
Changes v1->v2: remove now-unused 'fdctrl' local variable
 from fdctrl_init_sysbus()
---
 include/hw/block/fdc.h |  3 +--
 hw/block/fdc-sysbus.c  | 16 +++-
 hw/mips/jazz.c |  2 +-
 3 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/include/hw/block/fdc.h b/include/hw/block/fdc.h
index 1ecca7cac7f..35248c08379 100644
--- a/include/hw/block/fdc.h
+++ b/include/hw/block/fdc.h
@@ -10,8 +10,7 @@
 #define TYPE_ISA_FDC "isa-fdc"
 
 void isa_fdc_init_drives(ISADevice *fdc, DriveInfo **fds);
-void fdctrl_init_sysbus(qemu_irq irq, int dma_chann,
-hwaddr mmio_base, DriveInfo **fds);
+void fdctrl_init_sysbus(qemu_irq irq, hwaddr mmio_base, DriveInfo **fds);
 void sun4m_fdctrl_init(qemu_irq irq, hwaddr io_base,
DriveInfo **fds, qemu_irq *fdc_tc);
 
diff --git a/hw/block/fdc-sysbus.c b/hw/block/fdc-sysbus.c
index 57fc8773f12..86ea51d0034 100644
--- a/hw/block/fdc-sysbus.c
+++ b/hw/block/fdc-sysbus.c
@@ -94,18 +94,14 @@ static void fdctrl_handle_tc(void *opaque, int irq, int 
level)
 trace_fdctrl_tc_pulse(level);
 }
 
-void fdctrl_init_sysbus(qemu_irq irq, int dma_chann,
-hwaddr mmio_base, DriveInfo **fds)
+void fdctrl_init_sysbus(qemu_irq irq, hwaddr mmio_base, DriveInfo **fds)
 {
-FDCtrl *fdctrl;
 DeviceState *dev;
 SysBusDevice *sbd;
 FDCtrlSysBus *sys;
 
 dev = qdev_new("sysbus-fdc");
 sys = SYSBUS_FDC(dev);
-fdctrl = >state;
-fdctrl->dma_chann = dma_chann; /* FIXME */
 sbd = SYS_BUS_DEVICE(dev);
 sysbus_realize_and_unref(sbd, _fatal);
 sysbus_connect_irq(sbd, 0, irq);
@@ -138,6 +134,16 @@ static void sysbus_fdc_common_instance_init(Object *obj)
 FDCtrlSysBus *sys = SYSBUS_FDC(obj);
 FDCtrl *fdctrl = >state;
 
+/*
+ * DMA is not currently supported for sysbus floppy controllers.
+ * If we wanted to add support then probably the best approach is
+ * to have a QOM link property 'dma-controller' which the board
+ * code can set to an instance of IsaDmaClass, and an integer
+ * property 'dma-channel', so that we can set fdctrl->dma and
+ * fdctrl->dma_chann accordingly.
+ */
+fdctrl->dma_chann = -1;
+
 qdev_set_legacy_instance_id(dev, 0 /* io */, 2); /* FIXME */
 
 memory_region_init_io(>iomem, obj,
diff --git a/hw/mips/jazz.c b/hw/mips/jazz.c
index 6598d75..96dc6ab32dd 100644
--- a/hw/mips/jazz.c
+++ b/hw/mips/jazz.c
@@ -353,7 +353,7 @@ static void mips_jazz_init(MachineState *machine,
 fds[n] = drive_get(IF_FLOPPY, 0, n);
 }
 /* FIXME: we should enable DMA with a custom IsaDma device */
-fdctrl_init_sysbus(qdev_get_gpio_in(rc4030, 1), -1, 0x80003000, fds);
+fdctrl_init_sysbus(qdev_get_gpio_in(rc4030, 1), 0x80003000, fds);
 
 /* Real time clock */
 mc146818_rtc_init(isa_bus, 1980, NULL);
-- 
2.25.1

Re: iotests and python dependencies

[Paolo Bonzini]

On 5/5/22 10:51, Kevin Wolf wrote:

If not, I guess it would be enough if iotests just checks that the venv
exists and all of the dependencies are there in the right version and
error out if not, telling the user to run 'make check-venv'.

Or actually, it could just unconditionally run 'make check-venv' by
itself, which is probably easier to implement than checking the
dependencies and more convenient for the user, too.


Note that you would still have to add a 'check-block: check-venv' 
dependency in the Makefile, otherwise two "instances" of check-venv 
could run in parallel.


One small complication is that on BSD systems the binary is actually 
called "gmake", so you'd have to pass the variable somehow


Paolo

Re: iotests and python dependencies

[Kevin Wolf]

Am 04.05.2022 um 21:38 hat John Snow geschrieben:
> Howdy!
> 
> So, I want to finally delete python/qemu/qmp from qemu.git, and this
> creates a small problem -- namely, iotests needs access to it in order
> to run the python-based tests.
> 
> What I think needs to happen is that we create a virtual environment
> that installs python/qemu/. The reason this cannot be done with
> PYTHONPATH alone anymore is because the qmp package itself won't be
> there anymore, we need an installer like `pip` to actually fetch it
> for us and put it somewhere. (i.e., we need to process the
> dependencies of python/qemu now and can't treat it as a pre-installed
> location.)
> 
> Avocado tests are already creating a venv for the purposes of
> installing and running Avocado. We can amend e.g. "../../python" to
> tests/requirements.txt and the Avocado environment is A-OK good-to-go.
> The Makefile magic for avocado tests creates a venv-per-build.

"per build" sounded pretty bad, because I thought it meant building a
new venv every time I run either 'make' or the tests. This would
obviously be very noticable for short-running tests like some iotests.
But fortunately this is not what it does, it keeps the venv around in
the build directory. So it's only per build directory really, which I
guess is fine.

> It seems to work well enough. One thing to note here is that the
> supported invocation for avocado tests is only through the Makefile,
> which handles creating and entering the venv to make the command
> seamless.
> 
> iotests, however, manages its own execution environment with
> testenv.py, and we support running iotests from outside of the
> Makefile, for example by going to $build/tests/qemu-iotests and
> running ./check.

Yes, and I agree that this is important.

> Now ... I could update testenv.py to be smart enough to create and
> enter a python venv, and have even prototyped this. It seems to work
> pretty well! This approach seemed like the least invasive to how
> iotests are expected to be run and used. But a downside with this
> approach is that now avocado tests and iotests are each managing their
> own python venv. Worse, vm-tests and device-crash-test are still
> unhandled entirely.

Is there a reason why testenv.py couldn't enter just the shared venv in
build/tests/venv?

If not, I guess it would be enough if iotests just checks that the venv
exists and all of the dependencies are there in the right version and
error out if not, telling the user to run 'make check-venv'.

Or actually, it could just unconditionally run 'make check-venv' by
itself, which is probably easier to implement than checking the
dependencies and more convenient for the user, too.

(One more detail: 'make check-venv GIT_SUBMODULES_ACTION=ignore' seems
to make it pretty much instantaneous if the venv is current, but leaving
the submodule mechanism enabled adds a noticable delay.)

> I'd like to find a solution where I create a unified python testing
> venv tied to the build shared by avocado, iotests, vm-tests and
> device-crash-test. I'm not completely sure how exactly I'll manage
> that right now, but I wanted to throw this out there in case there are
> some requirements I might be overlooking.
> 
> I think vm-tests and avocado-tests can both have a venv created for
> them and activated before the test runs. device-crash-test I believe
> will need a script change in the gitlab ci yaml. iotests is somewhat
> unique in that it needs to run both by manual invocation and from
> makefile invocations. If I want a shared VM between all of these, I'll
> need to isolate the create-and-enter-venv logic somewhere where it can
> be shared both inside and outside of a Makefile.

If just calling 'make' isn't an option, then moving that part out into a
separate script probably wouldn't be too hard either. But 'make' has the
advantage that it already contains the check if the venv is already
there and requirements.txt hasn't changed, which you would have to
replicate otherwise.

Kevin

[PULL 2/3] util/main-loop: Introduce the main loop into QOM

[Stefan Hajnoczi]

From: Nicolas Saenz Julienne 

'event-loop-base' provides basic property handling for all 'AioContext'
based event loops. So let's define a new 'MainLoopClass' that inherits
from it. This will permit tweaking the main loop's properties through
qapi as well as through the command line using the '-object' keyword[1].
Only one instance of 'MainLoopClass' might be created at any time.

'EventLoopBaseClass' learns a new callback, 'can_be_deleted()' so as to
mark 'MainLoop' as non-deletable.

[1] For example:
  -object main-loop,id=main-loop,aio-max-batch=

Signed-off-by: Nicolas Saenz Julienne 
Reviewed-by: Stefan Hajnoczi 
Acked-by: Markus Armbruster 
Message-id: 20220425075723.20019-3-nsaen...@redhat.com
Signed-off-by: Stefan Hajnoczi 
---
 qapi/qom.json| 13 
 meson.build  |  3 +-
 include/qemu/main-loop.h | 10 ++
 include/sysemu/event-loop-base.h |  1 +
 event-loop-base.c| 13 
 util/main-loop.c | 56 
 6 files changed, 95 insertions(+), 1 deletion(-)

diff --git a/qapi/qom.json b/qapi/qom.json
index a2439533c5..7d4a2ac1b9 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -540,6 +540,17 @@
 '*poll-grow': 'int',
 '*poll-shrink': 'int' } }
 
+##
+# @MainLoopProperties:
+#
+# Properties for the main-loop object.
+#
+# Since: 7.1
+##
+{ 'struct': 'MainLoopProperties',
+  'base': 'EventLoopBaseProperties',
+  'data': {} }
+
 ##
 # @MemoryBackendProperties:
 #
@@ -830,6 +841,7 @@
 { 'name': 'input-linux',
   'if': 'CONFIG_LINUX' },
 'iothread',
+'main-loop',
 { 'name': 'memory-backend-epc',
   'if': 'CONFIG_LINUX' },
 'memory-backend-file',
@@ -895,6 +907,7 @@
   'input-linux':{ 'type': 'InputLinuxProperties',
   'if': 'CONFIG_LINUX' },
   'iothread':   'IothreadProperties',
+  'main-loop':  'MainLoopProperties',
   'memory-backend-epc': { 'type': 'MemoryBackendEpcProperties',
   'if': 'CONFIG_LINUX' },
   'memory-backend-file':'MemoryBackendFileProperties',
diff --git a/meson.build b/meson.build
index 11e9bd4824..b3d7ed41f9 100644
--- a/meson.build
+++ b/meson.build
@@ -2927,7 +2927,8 @@ libqemuutil = static_library('qemuutil',
  sources: util_ss.sources() + stub_ss.sources() + 
genh,
  dependencies: [util_ss.dependencies(), libm, 
threads, glib, socket, malloc, pixman])
 qemuutil = declare_dependency(link_with: libqemuutil,
-  sources: genh + version_res)
+  sources: genh + version_res,
+  dependencies: [event_loop_base])
 
 if have_system or have_user
   decodetree = generator(find_program('scripts/decodetree.py'),
diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h
index d3750c8e76..20c9387654 100644
--- a/include/qemu/main-loop.h
+++ b/include/qemu/main-loop.h
@@ -26,9 +26,19 @@
 #define QEMU_MAIN_LOOP_H
 
 #include "block/aio.h"
+#include "qom/object.h"
+#include "sysemu/event-loop-base.h"
 
 #define SIG_IPI SIGUSR1
 
+#define TYPE_MAIN_LOOP  "main-loop"
+OBJECT_DECLARE_TYPE(MainLoop, MainLoopClass, MAIN_LOOP)
+
+struct MainLoop {
+EventLoopBase parent_obj;
+};
+typedef struct MainLoop MainLoop;
+
 /**
  * qemu_init_main_loop: Set up the process so that it can run the main loop.
  *
diff --git a/include/sysemu/event-loop-base.h b/include/sysemu/event-loop-base.h
index 8e77d8b69f..fced4c9fea 100644
--- a/include/sysemu/event-loop-base.h
+++ b/include/sysemu/event-loop-base.h
@@ -25,6 +25,7 @@ struct EventLoopBaseClass {
 
 void (*init)(EventLoopBase *base, Error **errp);
 void (*update_params)(EventLoopBase *base, Error **errp);
+bool (*can_be_deleted)(EventLoopBase *base);
 };
 
 struct EventLoopBase {
diff --git a/event-loop-base.c b/event-loop-base.c
index a924c73a7c..e7f99a6ec8 100644
--- a/event-loop-base.c
+++ b/event-loop-base.c
@@ -73,10 +73,23 @@ static void event_loop_base_complete(UserCreatable *uc, 
Error **errp)
 }
 }
 
+static bool event_loop_base_can_be_deleted(UserCreatable *uc)
+{
+EventLoopBaseClass *bc = EVENT_LOOP_BASE_GET_CLASS(uc);
+EventLoopBase *backend = EVENT_LOOP_BASE(uc);
+
+if (bc->can_be_deleted) {
+return bc->can_be_deleted(backend);
+}
+
+return true;
+}
+
 static void event_loop_base_class_init(ObjectClass *klass, void *class_data)
 {
 UserCreatableClass *ucc = USER_CREATABLE_CLASS(klass);
 ucc->complete = event_loop_base_complete;
+ucc->can_be_deleted = event_loop_base_can_be_deleted;
 
 object_class_property_add(klass, "aio-max-batch", "int",
   event_loop_base_get_param,
diff --git a/util/main-loop.c b/util/main-loop.c
index 9afac10dff..e30f034815 100644
--- a/util/main-loop.c
+++ 

[PULL 3/3] util/event-loop-base: Introduce options to set the thread pool size

[Stefan Hajnoczi]

From: Nicolas Saenz Julienne 

The thread pool regulates itself: when idle, it kills threads until
empty, when in demand, it creates new threads until full. This behaviour
doesn't play well with latency sensitive workloads where the price of
creating a new thread is too high. For example, when paired with qemu's
'-mlock', or using safety features like SafeStack, creating a new thread
has been measured take multiple milliseconds.

In order to mitigate this let's introduce a new 'EventLoopBase'
property to set the thread pool size. The threads will be created during
the pool's initialization or upon updating the property's value, remain
available during its lifetime regardless of demand, and destroyed upon
freeing it. A properly characterized workload will then be able to
configure the pool to avoid any latency spikes.

Signed-off-by: Nicolas Saenz Julienne 
Reviewed-by: Stefan Hajnoczi 
Acked-by: Markus Armbruster 
Message-id: 20220425075723.20019-4-nsaen...@redhat.com
Signed-off-by: Stefan Hajnoczi 
---
 qapi/qom.json| 10 +-
 include/block/aio.h  | 10 ++
 include/block/thread-pool.h  |  3 ++
 include/sysemu/event-loop-base.h |  4 +++
 event-loop-base.c| 23 +
 iothread.c   |  3 ++
 util/aio-posix.c |  1 +
 util/async.c | 20 
 util/main-loop.c |  9 ++
 util/thread-pool.c   | 55 +---
 10 files changed, 133 insertions(+), 5 deletions(-)

diff --git a/qapi/qom.json b/qapi/qom.json
index 7d4a2ac1b9..6a653c6636 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -508,10 +508,18 @@
 # 0 means that the engine will use its default.
 # (default: 0)
 #
+# @thread-pool-min: minimum number of threads reserved in the thread pool
+#   (default:0)
+#
+# @thread-pool-max: maximum number of threads the thread pool can contain
+#   (default:64)
+#
 # Since: 7.1
 ##
 { 'struct': 'EventLoopBaseProperties',
-  'data': { '*aio-max-batch': 'int' } }
+  'data': { '*aio-max-batch': 'int',
+'*thread-pool-min': 'int',
+'*thread-pool-max': 'int' } }
 
 ##
 # @IothreadProperties:
diff --git a/include/block/aio.h b/include/block/aio.h
index 5634173b12..d128558f1d 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -192,6 +192,8 @@ struct AioContext {
 QSLIST_HEAD(, Coroutine) scheduled_coroutines;
 QEMUBH *co_schedule_bh;
 
+int thread_pool_min;
+int thread_pool_max;
 /* Thread pool for performing work and receiving completion callbacks.
  * Has its own locking.
  */
@@ -769,4 +771,12 @@ void aio_context_set_poll_params(AioContext *ctx, int64_t 
max_ns,
 void aio_context_set_aio_params(AioContext *ctx, int64_t max_batch,
 Error **errp);
 
+/**
+ * aio_context_set_thread_pool_params:
+ * @ctx: the aio context
+ * @min: min number of threads to have readily available in the thread pool
+ * @min: max number of threads the thread pool can contain
+ */
+void aio_context_set_thread_pool_params(AioContext *ctx, int64_t min,
+int64_t max, Error **errp);
 #endif
diff --git a/include/block/thread-pool.h b/include/block/thread-pool.h
index 7dd7d730a0..2020bcc92d 100644
--- a/include/block/thread-pool.h
+++ b/include/block/thread-pool.h
@@ -20,6 +20,8 @@
 
 #include "block/block.h"
 
+#define THREAD_POOL_MAX_THREADS_DEFAULT 64
+
 typedef int ThreadPoolFunc(void *opaque);
 
 typedef struct ThreadPool ThreadPool;
@@ -33,5 +35,6 @@ BlockAIOCB *thread_pool_submit_aio(ThreadPool *pool,
 int coroutine_fn thread_pool_submit_co(ThreadPool *pool,
 ThreadPoolFunc *func, void *arg);
 void thread_pool_submit(ThreadPool *pool, ThreadPoolFunc *func, void *arg);
+void thread_pool_update_params(ThreadPool *pool, struct AioContext *ctx);
 
 #endif
diff --git a/include/sysemu/event-loop-base.h b/include/sysemu/event-loop-base.h
index fced4c9fea..2748bf6ae1 100644
--- a/include/sysemu/event-loop-base.h
+++ b/include/sysemu/event-loop-base.h
@@ -33,5 +33,9 @@ struct EventLoopBase {
 
 /* AioContext AIO engine parameters */
 int64_t aio_max_batch;
+
+/* AioContext thread pool parameters */
+int64_t thread_pool_min;
+int64_t thread_pool_max;
 };
 #endif
diff --git a/event-loop-base.c b/event-loop-base.c
index e7f99a6ec8..d5be4dc6fc 100644
--- a/event-loop-base.c
+++ b/event-loop-base.c
@@ -14,6 +14,7 @@
 #include "qemu/osdep.h"
 #include "qom/object_interfaces.h"
 #include "qapi/error.h"
+#include "block/thread-pool.h"
 #include "sysemu/event-loop-base.h"
 
 typedef struct {
@@ -21,9 +22,22 @@ typedef struct {
 ptrdiff_t offset; /* field's byte offset in EventLoopBase struct */
 } EventLoopBaseParamInfo;
 
+static void event_loop_base_instance_init(Object *obj)
+{
+EventLoopBase *base = EVENT_LOOP_BASE(obj);
+
+base->thread_pool_max = 

[PULL 1/3] Introduce event-loop-base abstract class

[Stefan Hajnoczi]

From: Nicolas Saenz Julienne 

Introduce the 'event-loop-base' abstract class, it'll hold the
properties common to all event loops and provide the necessary hooks for
their creation and maintenance. Then have iothread inherit from it.

EventLoopBaseClass is defined as user creatable and provides a hook for
its children to attach themselves to the user creatable class 'complete'
function. It also provides an update_params() callback to propagate
property changes onto its children.

The new 'event-loop-base' class will live in the root directory. It is
built on its own using the 'link_whole' option (there are no direct
function dependencies between the class and its children, it all happens
trough 'constructor' magic). And also imposes new compilation
dependencies:

qom <- event-loop-base <- blockdev (iothread.c)

And in subsequent patches:

qom <- event-loop-base <- qemuutil (util/main-loop.c)

All this forced some amount of reordering in meson.build:

 - Moved qom build definition before qemuutil. Doing it the other way
   around (i.e. moving qemuutil after qom) isn't possible as a lot of
   core libraries that live in between the two depend on it.

 - Process the 'hw' subdir earlier, as it introduces files into the
   'qom' source set.

No functional changes intended.

Signed-off-by: Nicolas Saenz Julienne 
Reviewed-by: Stefan Hajnoczi 
Acked-by: Markus Armbruster 
Message-id: 20220425075723.20019-2-nsaen...@redhat.com
Signed-off-by: Stefan Hajnoczi 
---
 qapi/qom.json|  22 +--
 meson.build  |  23 ---
 include/sysemu/event-loop-base.h |  36 +++
 include/sysemu/iothread.h|   6 +-
 event-loop-base.c| 104 +++
 iothread.c   |  65 ++-
 6 files changed, 192 insertions(+), 64 deletions(-)
 create mode 100644 include/sysemu/event-loop-base.h
 create mode 100644 event-loop-base.c

diff --git a/qapi/qom.json b/qapi/qom.json
index eeb5395ff3..a2439533c5 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -499,6 +499,20 @@
 '*repeat': 'bool',
 '*grab-toggle': 'GrabToggleKeys' } }
 
+##
+# @EventLoopBaseProperties:
+#
+# Common properties for event loops
+#
+# @aio-max-batch: maximum number of requests in a batch for the AIO engine,
+# 0 means that the engine will use its default.
+# (default: 0)
+#
+# Since: 7.1
+##
+{ 'struct': 'EventLoopBaseProperties',
+  'data': { '*aio-max-batch': 'int' } }
+
 ##
 # @IothreadProperties:
 #
@@ -516,17 +530,15 @@
 #   algorithm detects it is spending too long polling without
 #   encountering events. 0 selects a default behaviour (default: 0)
 #
-# @aio-max-batch: maximum number of requests in a batch for the AIO engine,
-# 0 means that the engine will use its default
-# (default:0, since 6.1)
+# The @aio-max-batch option is available since 6.1.
 #
 # Since: 2.0
 ##
 { 'struct': 'IothreadProperties',
+  'base': 'EventLoopBaseProperties',
   'data': { '*poll-max-ns': 'int',
 '*poll-grow': 'int',
-'*poll-shrink': 'int',
-'*aio-max-batch': 'int' } }
+'*poll-shrink': 'int' } }
 
 ##
 # @MemoryBackendProperties:
diff --git a/meson.build b/meson.build
index c26aa442d4..11e9bd4824 100644
--- a/meson.build
+++ b/meson.build
@@ -2899,6 +2899,7 @@ subdir('qom')
 subdir('authz')
 subdir('crypto')
 subdir('ui')
+subdir('hw')
 
 
 if enable_modules
@@ -2906,6 +2907,18 @@ if enable_modules
   modulecommon = declare_dependency(link_whole: libmodulecommon, compile_args: 
'-DBUILD_DSO')
 endif
 
+qom_ss = qom_ss.apply(config_host, strict: false)
+libqom = static_library('qom', qom_ss.sources() + genh,
+dependencies: [qom_ss.dependencies()],
+name_suffix: 'fa')
+qom = declare_dependency(link_whole: libqom)
+
+event_loop_base = files('event-loop-base.c')
+event_loop_base = static_library('event-loop-base', sources: event_loop_base + 
genh,
+ build_by_default: true)
+event_loop_base = declare_dependency(link_whole: event_loop_base,
+ dependencies: [qom])
+
 stub_ss = stub_ss.apply(config_all, strict: false)
 
 util_ss.add_all(trace_ss)
@@ -2992,7 +3005,6 @@ subdir('monitor')
 subdir('net')
 subdir('replay')
 subdir('semihosting')
-subdir('hw')
 subdir('tcg')
 subdir('fpu')
 subdir('accel')
@@ -3117,13 +3129,6 @@ qemu_syms = custom_target('qemu.syms', output: 
'qemu.syms',
  capture: true,
  command: [undefsym, nm, '@INPUT@'])
 
-qom_ss = qom_ss.apply(config_host, strict: false)
-libqom = static_library('qom', qom_ss.sources() + genh,
-dependencies: [qom_ss.dependencies()],
-name_suffix: 'fa')
-
-qom = declare_dependency(link_whole: libqom)
-
 authz_ss = 

[PATCH] block/gluster: correctly set max_pdiscard which is int64_t

[Fabian Ebner]

Previously, max_pdiscard would be zero in the following assertion:
qemu-system-x86_64: ../block/io.c:3166: bdrv_co_pdiscard: Assertion
`max_pdiscard >= bs->bl.request_alignment' failed.

Fixes: 0c8022876f ("block: use int64_t instead of int in driver discard 
handlers")
Cc: qemu-sta...@nongnu.org
Signed-off-by: Fabian Ebner 
---
 block/gluster.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/gluster.c b/block/gluster.c
index 398976bc66..592e71b22a 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -891,7 +891,7 @@ out:
 static void qemu_gluster_refresh_limits(BlockDriverState *bs, Error **errp)
 {
 bs->bl.max_transfer = GLUSTER_MAX_TRANSFER;
-bs->bl.max_pdiscard = SIZE_MAX;
+bs->bl.max_pdiscard = INT64_MAX;
 }
 
 static int qemu_gluster_reopen_prepare(BDRVReopenState *state,
@@ -1304,7 +1304,7 @@ static coroutine_fn int 
qemu_gluster_co_pdiscard(BlockDriverState *bs,
 GlusterAIOCB acb;
 BDRVGlusterState *s = bs->opaque;
 
-assert(bytes <= SIZE_MAX); /* rely on max_pdiscard */
+assert(bytes <= INT64_MAX); /* rely on max_pdiscard */
 
 acb.size = 0;
 acb.ret = 0;
-- 
2.30.2

[PULL 0/3] Block patches

[Stefan Hajnoczi]

The following changes since commit 9cf289af47bcfae5c75de37d8e5d6fd23705322c:

  Merge tag 'qga-pull-request' of gitlab.com:marcandre.lureau/qemu into staging 
(2022-05-04 03:42:49 -0700)

are available in the Git repository at:

  https://gitlab.com/stefanha/qemu.git tags/block-pull-request

for you to fetch changes up to bef2e050d6a7feb865854c65570c496ac5a8cf53:

  util/event-loop-base: Introduce options to set the thread pool size 
(2022-05-04 17:02:19 +0100)


Pull request

Add new thread-pool-min/thread-pool-max parameters to control the thread pool
used for async I/O.



Nicolas Saenz Julienne (3):
  Introduce event-loop-base abstract class
  util/main-loop: Introduce the main loop into QOM
  util/event-loop-base: Introduce options to set the thread pool size

 qapi/qom.json|  43 --
 meson.build  |  26 +++---
 include/block/aio.h  |  10 +++
 include/block/thread-pool.h  |   3 +
 include/qemu/main-loop.h |  10 +++
 include/sysemu/event-loop-base.h |  41 +
 include/sysemu/iothread.h|   6 +-
 event-loop-base.c| 140 +++
 iothread.c   |  68 +--
 util/aio-posix.c |   1 +
 util/async.c |  20 +
 util/main-loop.c |  65 ++
 util/thread-pool.c   |  55 +++-
 13 files changed, 419 insertions(+), 69 deletions(-)
 create mode 100644 include/sysemu/event-loop-base.h
 create mode 100644 event-loop-base.c

-- 
2.35.1

[PATCH v2 15/15] test/qga: use g_auto wherever sensible

[marcandre . lureau]

From: Marc-André Lureau 

Signed-off-by: Marc-André Lureau 
---
 tests/unit/test-qga.c | 125 +++---
 1 file changed, 45 insertions(+), 80 deletions(-)

diff --git a/tests/unit/test-qga.c b/tests/unit/test-qga.c
index ab0b12a2dd16..53cefc2c2649 100644
--- a/tests/unit/test-qga.c
+++ b/tests/unit/test-qga.c
@@ -51,8 +51,11 @@ static void
 fixture_setup(TestFixture *fixture, gconstpointer data, gchar **envp)
 {
 const gchar *extra_arg = data;
-GError *error = NULL;
-gchar *cwd, *path, *cmd, **argv = NULL;
+g_autoptr(GError) error = NULL;
+g_autofree char *cwd = NULL;
+g_autofree char *path = NULL;
+g_autofree char *cmd = NULL;
+g_auto(GStrv) argv = NULL;
 
 fixture->loop = g_main_loop_new(NULL, FALSE);
 
@@ -78,17 +81,12 @@ fixture_setup(TestFixture *fixture, gconstpointer data, 
gchar **envp)
 
 fixture->fd = connect_qga(path);
 g_assert_cmpint(fixture->fd, !=, -1);
-
-g_strfreev(argv);
-g_free(cmd);
-g_free(cwd);
-g_free(path);
 }
 
 static void
 fixture_tear_down(TestFixture *fixture, gconstpointer data)
 {
-gchar *tmp;
+g_autofree char *tmp = NULL;
 
 kill(fixture->pid, SIGTERM);
 
@@ -107,7 +105,6 @@ fixture_tear_down(TestFixture *fixture, gconstpointer data)
 
 tmp = g_build_filename(fixture->test_dir, "sock", NULL);
 g_unlink(tmp);
-g_free(tmp);
 
 g_rmdir(fixture->test_dir);
 g_free(fixture->test_dir);
@@ -122,7 +119,7 @@ static void qmp_assertion_message_error(const char 
*domain,
 QDict  *dict)
 {
 const char *class, *desc;
-char *s;
+g_autofree char *s = NULL;
 QDict *error;
 
 error = qdict_get_qdict(dict, "error");
@@ -131,7 +128,6 @@ static void qmp_assertion_message_error(const char 
*domain,
 
 s = g_strdup_printf("assertion failed %s: %s %s", expr, class, desc);
 g_assertion_message(domain, file, line, func, s);
-g_free(s);
 }
 
 #define qmp_assert_no_error(err) do {   \
@@ -146,7 +142,7 @@ static void test_qga_sync_delimited(gconstpointer fix)
 const TestFixture *fixture = fix;
 guint32 v, r = g_test_rand_int();
 unsigned char c;
-QDict *ret;
+g_autoptr(QDict) ret = NULL;
 
 qmp_fd_send_raw(fixture->fd, "\xff");
 qmp_fd_send(fixture->fd,
@@ -180,15 +176,13 @@ static void test_qga_sync_delimited(gconstpointer fix)
 
 v = qdict_get_int(ret, "return");
 g_assert_cmpint(r, ==, v);
-
-qobject_unref(ret);
 }
 
 static void test_qga_sync(gconstpointer fix)
 {
 const TestFixture *fixture = fix;
 guint32 v, r = g_test_rand_int();
-QDict *ret;
+g_autoptr(QDict) ret = NULL;
 
 /*
  * TODO guest-sync is inherently limited: we cannot distinguish
@@ -210,33 +204,27 @@ static void test_qga_sync(gconstpointer fix)
 
 v = qdict_get_int(ret, "return");
 g_assert_cmpint(r, ==, v);
-
-qobject_unref(ret);
 }
 
 static void test_qga_ping(gconstpointer fix)
 {
 const TestFixture *fixture = fix;
-QDict *ret;
+g_autoptr(QDict) ret = NULL;
 
 ret = qmp_fd(fixture->fd, "{'execute': 'guest-ping'}");
 g_assert_nonnull(ret);
 qmp_assert_no_error(ret);
-
-qobject_unref(ret);
 }
 
 static void test_qga_id(gconstpointer fix)
 {
 const TestFixture *fixture = fix;
-QDict *ret;
+g_autoptr(QDict) ret = NULL;
 
 ret = qmp_fd(fixture->fd, "{'execute': 'guest-ping', 'id': 1}");
 g_assert_nonnull(ret);
 qmp_assert_no_error(ret);
 g_assert_cmpint(qdict_get_int(ret, "id"), ==, 1);
-
-qobject_unref(ret);
 }
 
 static void test_qga_invalid_oob(gconstpointer fix)
@@ -253,7 +241,8 @@ static void test_qga_invalid_oob(gconstpointer fix)
 static void test_qga_invalid_args(gconstpointer fix)
 {
 const TestFixture *fixture = fix;
-QDict *ret, *error;
+g_autoptr(QDict) ret = NULL;
+QDict *error;
 const gchar *class, *desc;
 
 ret = qmp_fd(fixture->fd, "{'execute': 'guest-ping', "
@@ -266,14 +255,13 @@ static void test_qga_invalid_args(gconstpointer fix)
 
 g_assert_cmpstr(class, ==, "GenericError");
 g_assert_cmpstr(desc, ==, "Parameter 'foo' is unexpected");
-
-qobject_unref(ret);
 }
 
 static void test_qga_invalid_cmd(gconstpointer fix)
 {
 const TestFixture *fixture = fix;
-QDict *ret, *error;
+g_autoptr(QDict) ret = NULL;
+QDict *error;
 const gchar *class, *desc;
 
 ret = qmp_fd(fixture->fd, "{'execute': 'guest-invalid-cmd'}");
@@ -285,14 +273,13 @@ static void test_qga_invalid_cmd(gconstpointer fix)
 
 g_assert_cmpstr(class, ==, "CommandNotFound");
 g_assert_cmpint(strlen(desc), >, 0);
-
-qobject_unref(ret);
 }
 
 static void test_qga_info(gconstpointer fix)
 {
 const TestFixture *fixture = fix;
-QDict *ret, *val;
+g_autoptr(QDict) ret = NULL;
+QDict *val;
 const gchar *version;
 
 ret = qmp_fd(fixture->fd, "{'execute': 'guest-info'}");
@@ -302,14 +289,12 @@ static 

[PATCH v2 10/15] test/qga: use G_TEST_DIR to locate os-release test file

[marcandre . lureau]

From: Marc-André Lureau 

This a more accurate way to lookup the test data, and will allow to move
the test in a subproject.

Signed-off-by: Marc-André Lureau 
---
 tests/unit/test-qga.c | 11 +--
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/tests/unit/test-qga.c b/tests/unit/test-qga.c
index d6df1ee92ea1..ab0b12a2dd16 100644
--- a/tests/unit/test-qga.c
+++ b/tests/unit/test-qga.c
@@ -914,15 +914,14 @@ static void test_qga_guest_get_osinfo(gconstpointer data)
 {
 TestFixture fixture;
 const gchar *str;
-gchar *cwd, *env[2];
-QDict *ret, *val;
+QDict *ret = NULL;
+char *env[2];
+QDict *val;
 
-cwd = g_get_current_dir();
 env[0] = g_strdup_printf(
-"QGA_OS_RELEASE=%s%ctests%cdata%ctest-qga-os-release",
-cwd, G_DIR_SEPARATOR, G_DIR_SEPARATOR, G_DIR_SEPARATOR);
+"QGA_OS_RELEASE=%s%c..%cdata%ctest-qga-os-release",
+g_test_get_dir(G_TEST_DIST), G_DIR_SEPARATOR, G_DIR_SEPARATOR, 
G_DIR_SEPARATOR);
 env[1] = NULL;
-g_free(cwd);
 fixture_setup(, NULL, env);
 
 ret = qmp_fd(fixture.fd, "{'execute': 'guest-get-osinfo'}");
-- 
2.36.0.44.g0f828332d5ac

[PATCH v2 11/15] qga/wixl: prefer variables over environment

[marcandre . lureau]

From: Marc-André Lureau 

No need to setup an environment or to check if the variable is undefined
manually.

Signed-off-by: Marc-André Lureau 
---
 qga/installer/qemu-ga.wxs | 30 +-
 qga/meson.build   |  9 -
 2 files changed, 13 insertions(+), 26 deletions(-)

diff --git a/qga/installer/qemu-ga.wxs b/qga/installer/qemu-ga.wxs
index 0950e8c6becc..8a19aa165651 100644
--- a/qga/installer/qemu-ga.wxs
+++ b/qga/installer/qemu-ga.wxs
@@ -1,17 +1,5 @@
 
 http://schemas.microsoft.com/wix/2006/wi;>
-  
-
-  
-
-  
-
-  
-
-  
-
-  
-
   
 
   
@@ -43,20 +31,20 @@
 Name="QEMU guest agent"
 Id="*"
 UpgradeCode="{EB6B8302-C06E-4BEC-ADAC-932C68A3A98D}"
-Manufacturer="$(env.QEMU_GA_MANUFACTURER)"
-Version="$(env.QEMU_GA_VERSION)"
+Manufacturer="$(var.QEMU_GA_MANUFACTURER)"
+Version="$(var.QEMU_GA_VERSION)"
 Language="1033">
 
 NOT VersionNT64
 
 
-
+
 1
 
 
   
-
+
 
   
   
-
+
   
   
-
+
   
   
   
@@ -133,9 +121,9 @@
   
   
 
+ 
Key="Software\$(var.QEMU_GA_MANUFACTURER)\$(var.QEMU_GA_DISTRO)\Tools\QemuGA">
   
-  
+  
 
   
 
diff --git a/qga/meson.build b/qga/meson.build
index 6d9f39bb321b..3ad3bc0260cf 100644
--- a/qga/meson.build
+++ b/qga/meson.build
@@ -121,15 +121,14 @@ if targetos == 'windows'
 output: 'qemu-ga-@0@.msi'.format(host_arch),
 depends: deps,
 command: [
-  find_program('env'),
-  'QEMU_GA_VERSION=' + 
config_host['QEMU_GA_VERSION'],
-  'QEMU_GA_MANUFACTURER=' + 
config_host['QEMU_GA_MANUFACTURER'],
-  'QEMU_GA_DISTRO=' + 
config_host['QEMU_GA_DISTRO'],
-  'BUILD_DIR=' + meson.build_root(),
   wixl, '-o', '@OUTPUT0@', '@INPUT0@',
   qemu_ga_msi_arch[cpu],
   qemu_ga_msi_vss,
+  '-D', 'BUILD_DIR=' + meson.build_root(),
   '-D', 'Mingw_bin=' + 
config_host['QEMU_GA_MSI_MINGW_BIN_PATH'],
+  '-D', 'QEMU_GA_VERSION=' + 
config_host['QEMU_GA_VERSION'],
+  '-D', 'QEMU_GA_MANUFACTURER=' + 
config_host['QEMU_GA_MANUFACTURER'],
+  '-D', 'QEMU_GA_DISTRO=' + 
config_host['QEMU_GA_DISTRO'],
 ])
 all_qga += [qga_msi]
 alias_target('msi', qga_msi)
-- 
2.36.0.44.g0f828332d5ac

[PATCH v2 09/15] qga: replace qemu_open_old() with qemu_open_cloexec()

[marcandre . lureau]

From: Marc-André Lureau 

qemu_open_old() uses qemu_open_internal() which handles special
"/dev/fdset/" path for monitor fd sets, set CLOEXEC, and uses Error
reporting (and some O_DIRECT special error casing).

The monitor fdset handling is unnecessary for qga, use
qemu_open_cloexec() instead.

Signed-off-by: Marc-André Lureau 
---
 qga/channel-posix.c  | 14 +-
 qga/commands-posix.c | 24 
 2 files changed, 21 insertions(+), 17 deletions(-)

diff --git a/qga/channel-posix.c b/qga/channel-posix.c
index 0ce594bc36c2..a1262b130145 100644
--- a/qga/channel-posix.c
+++ b/qga/channel-posix.c
@@ -1,4 +1,5 @@
 #include "qemu/osdep.h"
+#include "qemu/cutils.h"
 #include 
 #include "qapi/error.h"
 #include "qemu/sockets.h"
@@ -128,11 +129,15 @@ static gboolean ga_channel_open(GAChannel *c, const gchar 
*path,
 switch (c->method) {
 case GA_CHANNEL_VIRTIO_SERIAL: {
 assert(fd < 0);
-fd = qemu_open_old(path, O_RDWR | O_NONBLOCK
+fd = qemu_open_cloexec(
+path,
 #ifndef CONFIG_SOLARIS
-   | O_ASYNC
+O_ASYNC |
 #endif
-   );
+O_RDWR | O_NONBLOCK,
+0,
+errp
+);
 if (fd == -1) {
 error_setg_errno(errp, errno, "error opening channel");
 return false;
@@ -157,9 +162,8 @@ static gboolean ga_channel_open(GAChannel *c, const gchar 
*path,
 struct termios tio;
 
 assert(fd < 0);
-fd = qemu_open_old(path, O_RDWR | O_NOCTTY | O_NONBLOCK);
+fd = qemu_open_cloexec(path, O_RDWR | O_NOCTTY | O_NONBLOCK, 0, errp);
 if (fd == -1) {
-error_setg_errno(errp, errno, "error opening channel");
 return false;
 }
 tcgetattr(fd, );
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 8ebc327c5e02..f82205e25813 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1392,6 +1392,7 @@ static GuestDiskInfoList *get_disk_partitions(
 
 static void get_nvme_smart(GuestDiskInfo *disk)
 {
+Error *err = NULL;
 int fd;
 GuestNVMeSmart *smart;
 NvmeSmartLog log = {0};
@@ -1404,9 +1405,10 @@ static void get_nvme_smart(GuestDiskInfo *disk)
  | (((sizeof(log) >> 2) - 1) << 16)
 };
 
-fd = qemu_open_old(disk->name, O_RDONLY);
+fd = qemu_open_cloexec(disk->name, O_RDONLY, 0, );
 if (fd == -1) {
-g_debug("Failed to open device: %s: %s", disk->name, 
g_strerror(errno));
+g_debug("Failed to open device: %s: %s", disk->name, 
error_get_pretty(err));
+error_free(err);
 return;
 }
 
@@ -1737,9 +1739,8 @@ int64_t qmp_guest_fsfreeze_freeze_list(bool 
has_mountpoints,
 }
 }
 
-fd = qemu_open_old(mount->dirname, O_RDONLY);
+fd = qemu_open_cloexec(mount->dirname, O_RDONLY, 0, errp);
 if (fd == -1) {
-error_setg_errno(errp, errno, "failed to open %s", mount->dirname);
 goto error;
 }
 
@@ -1804,7 +1805,7 @@ int64_t qmp_guest_fsfreeze_thaw(Error **errp)
 
 QTAILQ_FOREACH(mount, , next) {
 logged = false;
-fd = qemu_open_old(mount->dirname, O_RDONLY);
+fd = qemu_open_cloexec(mount->dirname, O_RDONLY, 0, NULL);
 if (fd == -1) {
 continue;
 }
@@ -1864,21 +1865,20 @@ static void guest_fsfreeze_cleanup(void)
 GuestFilesystemTrimResponse *
 qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
 {
+ERRP_GUARD();
 GuestFilesystemTrimResponse *response;
 GuestFilesystemTrimResult *result;
 int ret = 0;
 FsMountList mounts;
 struct FsMount *mount;
 int fd;
-Error *local_err = NULL;
 struct fstrim_range r;
 
 slog("guest-fstrim called");
 
 QTAILQ_INIT();
-build_fs_mount_list(, _err);
-if (local_err) {
-error_propagate(errp, local_err);
+build_fs_mount_list(, errp);
+if (*errp) {
 return NULL;
 }
 
@@ -1890,11 +1890,11 @@ qmp_guest_fstrim(bool has_minimum, int64_t minimum, 
Error **errp)
 
 QAPI_LIST_PREPEND(response->paths, result);
 
-fd = qemu_open_old(mount->dirname, O_RDONLY);
+fd = qemu_open_cloexec(mount->dirname, O_RDONLY, 0, errp);
 if (fd == -1) {
-result->error = g_strdup_printf("failed to open: %s",
-strerror(errno));
+result->error = g_strdup(error_get_pretty(*errp));
 result->has_error = true;
+g_clear_pointer(errp, error_free);
 continue;
 }
 
-- 
2.36.0.44.g0f828332d5ac

[PATCH v2 08/15] qga: throw an Error in ga_channel_open()

[marcandre . lureau]

From: Marc-André Lureau 

Allow for a single point of error reporting, and further refactoring.

Signed-off-by: Marc-André Lureau 
---
 qga/channel-posix.c | 42 +-
 1 file changed, 17 insertions(+), 25 deletions(-)

diff --git a/qga/channel-posix.c b/qga/channel-posix.c
index a996858e2492..0ce594bc36c2 100644
--- a/qga/channel-posix.c
+++ b/qga/channel-posix.c
@@ -119,8 +119,9 @@ static int ga_channel_client_add(GAChannel *c, int fd)
 }
 
 static gboolean ga_channel_open(GAChannel *c, const gchar *path,
-GAChannelMethod method, int fd)
+GAChannelMethod method, int fd, Error **errp)
 {
+ERRP_GUARD();
 int ret;
 c->method = method;
 
@@ -133,21 +134,20 @@ static gboolean ga_channel_open(GAChannel *c, const gchar 
*path,
 #endif
);
 if (fd == -1) {
-g_critical("error opening channel: %s", strerror(errno));
+error_setg_errno(errp, errno, "error opening channel");
 return false;
 }
 #ifdef CONFIG_SOLARIS
 ret = ioctl(fd, I_SETSIG, S_OUTPUT | S_INPUT | S_HIPRI);
 if (ret == -1) {
-g_critical("error setting event mask for channel: %s",
-   strerror(errno));
+error_setg_errno(errp, errno, "error setting event mask for 
channel");
 close(fd);
 return false;
 }
 #endif
 ret = ga_channel_client_add(c, fd);
 if (ret) {
-g_critical("error adding channel to main loop");
+error_setg(errp, "error adding channel to main loop");
 close(fd);
 return false;
 }
@@ -159,7 +159,7 @@ static gboolean ga_channel_open(GAChannel *c, const gchar 
*path,
 assert(fd < 0);
 fd = qemu_open_old(path, O_RDWR | O_NOCTTY | O_NONBLOCK);
 if (fd == -1) {
-g_critical("error opening channel: %s", strerror(errno));
+error_setg_errno(errp, errno, "error opening channel");
 return false;
 }
 tcgetattr(fd, );
@@ -180,7 +180,7 @@ static gboolean ga_channel_open(GAChannel *c, const gchar 
*path,
 tcsetattr(fd, TCSANOW, );
 ret = ga_channel_client_add(c, fd);
 if (ret) {
-g_critical("error adding channel to main loop");
+error_setg(errp, "error adding channel to main loop");
 close(fd);
 return false;
 }
@@ -188,12 +188,8 @@ static gboolean ga_channel_open(GAChannel *c, const gchar 
*path,
 }
 case GA_CHANNEL_UNIX_LISTEN: {
 if (fd < 0) {
-Error *local_err = NULL;
-
-fd = unix_listen(path, _err);
-if (local_err != NULL) {
-g_critical("%s", error_get_pretty(local_err));
-error_free(local_err);
+fd = unix_listen(path, errp);
+if (fd < 0) {
 return false;
 }
 }
@@ -202,24 +198,19 @@ static gboolean ga_channel_open(GAChannel *c, const gchar 
*path,
 }
 case GA_CHANNEL_VSOCK_LISTEN: {
 if (fd < 0) {
-Error *local_err = NULL;
 SocketAddress *addr;
 char *addr_str;
 
 addr_str = g_strdup_printf("vsock:%s", path);
-addr = socket_parse(addr_str, _err);
+addr = socket_parse(addr_str, errp);
 g_free(addr_str);
-if (local_err != NULL) {
-g_critical("%s", error_get_pretty(local_err));
-error_free(local_err);
+if (*errp) {
 return false;
 }
 
-fd = socket_listen(addr, 1, _err);
+fd = socket_listen(addr, 1, errp);
 qapi_free_SocketAddress(addr);
-if (local_err != NULL) {
-g_critical("%s", error_get_pretty(local_err));
-error_free(local_err);
+if (*errp) {
 return false;
 }
 }
@@ -227,7 +218,7 @@ static gboolean ga_channel_open(GAChannel *c, const gchar 
*path,
 break;
 }
 default:
-g_critical("error binding/listening to specified socket");
+error_setg(errp, "error binding/listening to specified socket");
 return false;
 }
 
@@ -272,12 +263,13 @@ GIOStatus ga_channel_read(GAChannel *c, gchar *buf, gsize 
size, gsize *count)
 GAChannel *ga_channel_new(GAChannelMethod method, const gchar *path,
   int listen_fd, GAChannelCallback cb, gpointer opaque)
 {
+Error *err = NULL;
 GAChannel *c = g_new0(GAChannel, 1);
 c->event_cb = cb;
 c->user_data = opaque;
 
-if (!ga_channel_open(c, path, method, listen_fd)) {
-g_critical("error opening channel");
+if (!ga_channel_open(c, path, method, listen_fd, )) {
+error_report_err(err);
 ga_channel_free(c);
 return NULL;
 }
-- 
2.36.0.44.g0f828332d5ac

[PATCH v2 14/15] qga/wixl: replace QEMU_GA_MSI_MINGW_BIN_PATH with glib bindir

[marcandre . lureau]

From: Marc-André Lureau 

Use more conventional variables to set the location of pre-built
DLL/bin.

Signed-off-by: Marc-André Lureau 
---
 configure |  9 ++---
 meson.build   |  5 -
 qga/installer/qemu-ga.wxs | 24 
 qga/meson.build   |  2 +-
 4 files changed, 23 insertions(+), 17 deletions(-)

diff --git a/configure b/configure
index 59c43bea05eb..616cd2d0e36c 100755
--- a/configure
+++ b/configure
@@ -2023,6 +2023,11 @@ for i in $glib_modules; do
 fi
 done
 
+glib_bindir="$($pkg_config --variable=bindir glib-2.0)"
+if test -z "$glib_bindir" ; then
+   glib_bindir="$($pkg_config --variable=prefix glib-2.0)"/bin
+fi
+
 # This workaround is required due to a bug in pkg-config file for glib as it
 # doesn't define GLIB_STATIC_COMPILATION for pkg-config --static
 
@@ -2430,8 +2435,6 @@ if test "$QEMU_GA_VERSION" = ""; then
 QEMU_GA_VERSION=$(cat $source_path/VERSION)
 fi
 
-QEMU_GA_MSI_MINGW_BIN_PATH="$($pkg_config --variable=prefix glib-2.0)/bin"
-
 # Mac OS X ships with a broken assembler
 roms=
 if { test "$cpu" = "i386" || test "$cpu" = "x86_64"; } && \
@@ -2518,7 +2521,6 @@ if test "$debug_tcg" = "yes" ; then
 fi
 if test "$mingw32" = "yes" ; then
   echo "CONFIG_WIN32=y" >> $config_host_mak
-  echo "QEMU_GA_MSI_MINGW_BIN_PATH=${QEMU_GA_MSI_MINGW_BIN_PATH}" >> 
$config_host_mak
   echo "QEMU_GA_MANUFACTURER=${QEMU_GA_MANUFACTURER}" >> $config_host_mak
   echo "QEMU_GA_DISTRO=${QEMU_GA_DISTRO}" >> $config_host_mak
   echo "QEMU_GA_VERSION=${QEMU_GA_VERSION}" >> $config_host_mak
@@ -2639,6 +2641,7 @@ echo "QEMU_CXXFLAGS=$QEMU_CXXFLAGS" >> $config_host_mak
 echo "QEMU_OBJCFLAGS=$QEMU_OBJCFLAGS" >> $config_host_mak
 echo "GLIB_CFLAGS=$glib_cflags" >> $config_host_mak
 echo "GLIB_LIBS=$glib_libs" >> $config_host_mak
+echo "GLIB_BINDIR=$glib_bindir" >> $config_host_mak
 echo "GLIB_VERSION=$(pkg-config --modversion glib-2.0)" >> $config_host_mak
 echo "QEMU_LDFLAGS=$QEMU_LDFLAGS" >> $config_host_mak
 echo "LD_I386_EMULATION=$ld_i386_emulation" >> $config_host_mak
diff --git a/meson.build b/meson.build
index c26aa442d40e..2f68b6cb8634 100644
--- a/meson.build
+++ b/meson.build
@@ -443,7 +443,10 @@ add_project_arguments(config_host['GLIB_CFLAGS'].split(),
   native: false, language: ['c', 'cpp', 'objc'])
 glib = declare_dependency(compile_args: config_host['GLIB_CFLAGS'].split(),
   link_args: config_host['GLIB_LIBS'].split(),
-  version: config_host['GLIB_VERSION'])
+  version: config_host['GLIB_VERSION'],
+  variables: {
+'bindir': config_host['GLIB_BINDIR'],
+  })
 # override glib dep with the configure results (for subprojects)
 meson.override_dependency('glib-2.0', glib)
 
diff --git a/qga/installer/qemu-ga.wxs b/qga/installer/qemu-ga.wxs
index e5b0958e1898..813d1c6ca6ae 100644
--- a/qga/installer/qemu-ga.wxs
+++ b/qga/installer/qemu-ga.wxs
@@ -58,7 +58,7 @@
   
   
   
-
+
   
   
 
@@ -69,40 +69,40 @@
   
   
   
-
+
   
   
-
+
   
   
   
   
-
+
   
   
-
+
   
   
   
-
+
   
   
-
+
   
   
-
+
   
   
-
+
   
   
-
+
   
   
-
+
   
   
-
+
   
   
 

[PATCH v2 13/15] qga/wixl: simplify some pre-processing

[marcandre . lureau]

From: Marc-André Lureau 

Sadly, wixl doesn't have 'elif'.

Signed-off-by: Marc-André Lureau 
---
 qga/installer/qemu-ga.wxs | 20 +++-
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/qga/installer/qemu-ga.wxs b/qga/installer/qemu-ga.wxs
index 651db6e51cda..e5b0958e1898 100644
--- a/qga/installer/qemu-ga.wxs
+++ b/qga/installer/qemu-ga.wxs
@@ -1,21 +1,15 @@
 
 http://schemas.microsoft.com/wix/2006/wi;>
-  
-
-  
-
   
 
 
-  
-
-  
-
-
-  
-
-  
-
+  
+
+  
+  
+
+  
+
   
 
   

[PATCH v2 12/15] qga/wixl: require Mingw_bin

[marcandre . lureau]

From: Marc-André Lureau 

No clear reason to make guesses here.

Signed-off-by: Marc-André Lureau 
---
 qga/installer/qemu-ga.wxs | 9 -
 1 file changed, 9 deletions(-)

diff --git a/qga/installer/qemu-ga.wxs b/qga/installer/qemu-ga.wxs
index 8a19aa165651..651db6e51cda 100644
--- a/qga/installer/qemu-ga.wxs
+++ b/qga/installer/qemu-ga.wxs
@@ -4,15 +4,6 @@
 
   
 
-  
-
-  
-
-
-  
-
-  
-
   
 
 
-- 
2.36.0.44.g0f828332d5ac

[PATCH v2 01/15] include: move qemu_*_exec_dir() to cutils

[marcandre . lureau]

From: Marc-André Lureau 

The function is required by get_relocated_path() (already in cutils),
and used by qemu-ga and may be generally useful.

Signed-off-by: Marc-André Lureau 
---
 include/qemu/cutils.h|   7 ++
 include/qemu/osdep.h |   8 --
 qemu-io.c|   1 +
 storage-daemon/qemu-storage-daemon.c |   1 +
 tests/qtest/fuzz/fuzz.c  |   1 +
 util/cutils.c| 108 +++
 util/oslib-posix.c   |  81 
 util/oslib-win32.c   |  36 -
 8 files changed, 118 insertions(+), 125 deletions(-)

diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
index 5c6572d44422..40e10e19a7ed 100644
--- a/include/qemu/cutils.h
+++ b/include/qemu/cutils.h
@@ -193,6 +193,13 @@ int uleb128_decode_small(const uint8_t *in, uint32_t *n);
  */
 int qemu_pstrcmp0(const char **str1, const char **str2);
 
+/* Find program directory, and save it for later usage with
+ * qemu_get_exec_dir().
+ * Try OS specific API first, if not working, parse from argv0. */
+void qemu_init_exec_dir(const char *argv0);
+
+/* Get the saved exec dir.  */
+const char *qemu_get_exec_dir(void);
 
 /**
  * get_relocated_path:
diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 1c1e7eca9898..67cc4654166b 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -557,14 +557,6 @@ void qemu_set_cloexec(int fd);
  */
 char *qemu_get_local_state_dir(void);
 
-/* Find program directory, and save it for later usage with
- * qemu_get_exec_dir().
- * Try OS specific API first, if not working, parse from argv0. */
-void qemu_init_exec_dir(const char *argv0);
-
-/* Get the saved exec dir.  */
-const char *qemu_get_exec_dir(void);
-
 /**
  * qemu_getauxval:
  * @type: the auxiliary vector key to lookup
diff --git a/qemu-io.c b/qemu-io.c
index d70d3dd4fde5..2bd7bfb65073 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -16,6 +16,7 @@
 #endif
 
 #include "qemu/help-texts.h"
+#include "qemu/cutils.h"
 #include "qapi/error.h"
 #include "qemu-io.h"
 #include "qemu/error-report.h"
diff --git a/storage-daemon/qemu-storage-daemon.c 
b/storage-daemon/qemu-storage-daemon.c
index 9b8b17f52e48..c104817cdddc 100644
--- a/storage-daemon/qemu-storage-daemon.c
+++ b/storage-daemon/qemu-storage-daemon.c
@@ -44,6 +44,7 @@
 
 #include "qemu/help-texts.h"
 #include "qemu-version.h"
+#include "qemu/cutils.h"
 #include "qemu/config-file.h"
 #include "qemu/error-report.h"
 #include "qemu/help_option.h"
diff --git a/tests/qtest/fuzz/fuzz.c b/tests/qtest/fuzz/fuzz.c
index a7a5e14fa3bc..0ad4ba9e94dc 100644
--- a/tests/qtest/fuzz/fuzz.c
+++ b/tests/qtest/fuzz/fuzz.c
@@ -15,6 +15,7 @@
 
 #include 
 
+#include "qemu/cutils.h"
 #include "qemu/datadir.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/qtest.h"
diff --git a/util/cutils.c b/util/cutils.c
index b2777210e7da..6cc7cc8cde99 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -931,6 +931,114 @@ static inline const char *next_component(const char *dir, 
int *p_len)
 return dir;
 }
 
+static const char *exec_dir;
+
+void qemu_init_exec_dir(const char *argv0)
+{
+#ifdef G_OS_WIN32
+char *p;
+char buf[MAX_PATH];
+DWORD len;
+
+if (exec_dir) {
+return;
+}
+
+len = GetModuleFileName(NULL, buf, sizeof(buf) - 1);
+if (len == 0) {
+return;
+}
+
+buf[len] = 0;
+p = buf + len - 1;
+while (p != buf && *p != '\\') {
+p--;
+}
+*p = 0;
+if (access(buf, R_OK) == 0) {
+exec_dir = g_strdup(buf);
+} else {
+exec_dir = CONFIG_BINDIR;
+}
+#else
+char *p = NULL;
+char buf[PATH_MAX];
+
+if (exec_dir) {
+return;
+}
+
+#if defined(__linux__)
+{
+int len;
+len = readlink("/proc/self/exe", buf, sizeof(buf) - 1);
+if (len > 0) {
+buf[len] = 0;
+p = buf;
+}
+}
+#elif defined(__FreeBSD__) \
+  || (defined(__NetBSD__) && defined(KERN_PROC_PATHNAME))
+{
+#if defined(__FreeBSD__)
+static int mib[4] = {CTL_KERN, KERN_PROC, KERN_PROC_PATHNAME, -1};
+#else
+static int mib[4] = {CTL_KERN, KERN_PROC_ARGS, -1, KERN_PROC_PATHNAME};
+#endif
+size_t len = sizeof(buf) - 1;
+
+*buf = '\0';
+if (!sysctl(mib, ARRAY_SIZE(mib), buf, , NULL, 0) &&
+*buf) {
+buf[sizeof(buf) - 1] = '\0';
+p = buf;
+}
+}
+#elif defined(__APPLE__)
+{
+char fpath[PATH_MAX];
+uint32_t len = sizeof(fpath);
+if (_NSGetExecutablePath(fpath, ) == 0) {
+p = realpath(fpath, buf);
+if (!p) {
+return;
+}
+}
+}
+#elif defined(__HAIKU__)
+{
+image_info ii;
+int32_t c = 0;
+
+*buf = '\0';
+while (get_next_image_info(0, , ) == B_OK) {
+if (ii.type == B_APP_IMAGE) {
+strncpy(buf, ii.name, sizeof(buf));
+   

[PATCH v2 06/15] osdep: export qemu_open_cloexec()

[marcandre . lureau]

From: Marc-André Lureau 

Used in the next patch, to simplify qga code.

Signed-off-by: Marc-André Lureau 
---
 include/qemu/osdep.h |  1 +
 util/osdep.c | 10 --
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 67cc4654166b..64f51cfb7a62 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -489,6 +489,7 @@ void sigaction_invoke(struct sigaction *action,
  */
 int qemu_open_old(const char *name, int flags, ...);
 int qemu_open(const char *name, int flags, Error **errp);
+int qemu_open_cloexec(const char *name, int flags, mode_t mode, Error **errp);
 int qemu_create(const char *name, int flags, mode_t mode, Error **errp);
 int qemu_close(int fd);
 int qemu_unlink(const char *name);
diff --git a/util/osdep.c b/util/osdep.c
index 60fcbbaebe72..67541b7654ef 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -279,9 +279,11 @@ int qemu_lock_fd_test(int fd, int64_t start, int64_t len, 
bool exclusive)
 }
 #endif
 
-static int qemu_open_cloexec(const char *name, int flags, mode_t mode)
+int qemu_open_cloexec(const char *name, int flags, mode_t mode, Error **errp)
 {
+ERRP_GUARD();
 int ret;
+
 #ifdef O_CLOEXEC
 ret = open(name, flags | O_CLOEXEC, mode);
 #else
@@ -290,6 +292,10 @@ static int qemu_open_cloexec(const char *name, int flags, 
mode_t mode)
 qemu_set_cloexec(ret);
 }
 #endif
+if (ret == -1) {
+error_setg_errno(errp, errno, "Could not open '%s'", name);
+}
+
 return ret;
 }
 
@@ -327,7 +333,7 @@ qemu_open_internal(const char *name, int flags, mode_t 
mode, Error **errp)
 }
 #endif
 
-ret = qemu_open_cloexec(name, flags, mode);
+ret = qemu_open_cloexec(name, flags, mode, NULL);
 
 if (ret == -1) {
 const char *action = flags & O_CREAT ? "create" : "open";
-- 
2.36.0.44.g0f828332d5ac

[PATCH v2 04/15] include: adjust header guards after renaming

[marcandre . lureau]

From: Marc-André Lureau 

Signed-off-by: Marc-André Lureau 
Reviewed-by: Stefan Weil 
---
 include/qemu/help-texts.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/qemu/help-texts.h b/include/qemu/help-texts.h
index ba32cc8b1f39..4f265fed8df1 100644
--- a/include/qemu/help-texts.h
+++ b/include/qemu/help-texts.h
@@ -1,5 +1,5 @@
-#ifndef QEMU_COMMON_H
-#define QEMU_COMMON_H
+#ifndef QEMU_HELP_TEXTS_H
+#define QEMU_HELP_TEXTS_H
 
 /* Copyright string for -version arguments, About dialogs, etc */
 #define QEMU_COPYRIGHT "Copyright (c) 2003-2022 " \
-- 
2.36.0.44.g0f828332d5ac

[PATCH v2 07/15] qga: use qemu_open_cloexec() for safe_open_or_create()

[marcandre . lureau]

From: Marc-André Lureau 

The function takes care of setting CLOEXEC, and reporting error.

Signed-off-by: Marc-André Lureau 
---
 qga/commands-posix.c | 11 +++
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 0ef049650e31..8ebc327c5e02 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -370,21 +370,16 @@ safe_open_or_create(const char *path, const char *mode, 
Error **errp)
  * open() is decisive and its third argument is ignored, and the second
  * open() and the fchmod() are never called.
  */
-fd = open(path, oflag | ((oflag & O_CREAT) ? O_EXCL : 0), 0);
+fd = qemu_open_cloexec(path, oflag | ((oflag & O_CREAT) ? O_EXCL : 0), 0, 
errp);
 if (fd == -1 && errno == EEXIST) {
+g_clear_pointer(errp, error_free);
 oflag &= ~(unsigned)O_CREAT;
-fd = open(path, oflag);
+fd = qemu_open_cloexec(path, oflag, 0, errp);
 }
 if (fd == -1) {
-error_setg_errno(errp, errno,
- "failed to open file '%s' "
- "(mode: '%s')",
- path, mode);
 goto end;
 }
 
-qemu_set_cloexec(fd);
-
 if ((oflag & O_CREAT) && fchmod(fd, DEFAULT_NEW_FILE_MODE) == -1) {
 error_setg_errno(errp, errno,
  "failed to set permission 0%03o on new file '%s' 
(mode: '%s')",
-- 
2.36.0.44.g0f828332d5ac

Re: iotests and python dependencies

[Daniel P . Berrangé]

On Wed, May 04, 2022 at 03:38:45PM -0400, John Snow wrote:
> Howdy!
> 
> So, I want to finally delete python/qemu/qmp from qemu.git, and this
> creates a small problem -- namely, iotests needs access to it in order
> to run the python-based tests.
> 
> What I think needs to happen is that we create a virtual environment
> that installs python/qemu/. The reason this cannot be done with
> PYTHONPATH alone anymore is because the qmp package itself won't be
> there anymore, we need an installer like `pip` to actually fetch it
> for us and put it somewhere. (i.e., we need to process the
> dependencies of python/qemu now and can't treat it as a pre-installed
> location.)

Having pip fetch it on the fly creates a problem for RPM builds,
because the koji build env has no network access. We will, however,
have an RPM of python-qemu-qmp installed on the host system though.
IOW we need to be able to run iotests using system python and its
installed libs, not a virtual env.  So if we do anything with a
virtual env, it will need to be optional I believe.


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

[PATCH v2 05/15] qga: flatten safe_open_or_create()

[marcandre . lureau]

From: Marc-André Lureau 

There is a bit too much branching in the function, this can be
simplified a bit, and have a common exit point thanks to ERRP_PROPAGATE.

This also helps with the following error handling changes.

Signed-off-by: Marc-André Lureau 
---
 qga/commands-posix.c | 124 ++-
 1 file changed, 63 insertions(+), 61 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 69f209af87e6..0ef049650e31 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -339,73 +339,75 @@ find_open_flag(const char *mode_str, Error **errp)
 static FILE *
 safe_open_or_create(const char *path, const char *mode, Error **errp)
 {
-Error *local_err = NULL;
-int oflag;
-
-oflag = find_open_flag(mode, _err);
-if (local_err == NULL) {
-int fd;
+ERRP_GUARD();
+int oflag, fd = -1;
+FILE *f = NULL;
+
+oflag = find_open_flag(mode, errp);
+if (*errp) {
+goto end;
+}
+
+/* If the caller wants / allows creation of a new file, we implement it
+ * with a two step process: open() + (open() / fchmod()).
+ *
+ * First we insist on creating the file exclusively as a new file. If
+ * that succeeds, we're free to set any file-mode bits on it. (The
+ * motivation is that we want to set those file-mode bits independently
+ * of the current umask.)
+ *
+ * If the exclusive creation fails because the file already exists
+ * (EEXIST is not possible for any other reason), we just attempt to
+ * open the file, but in this case we won't be allowed to change the
+ * file-mode bits on the preexistent file.
+ *
+ * The pathname should never disappear between the two open()s in
+ * practice. If it happens, then someone very likely tried to race us.
+ * In this case just go ahead and report the ENOENT from the second
+ * open() to the caller.
+ *
+ * If the caller wants to open a preexistent file, then the first
+ * open() is decisive and its third argument is ignored, and the second
+ * open() and the fchmod() are never called.
+ */
+fd = open(path, oflag | ((oflag & O_CREAT) ? O_EXCL : 0), 0);
+if (fd == -1 && errno == EEXIST) {
+oflag &= ~(unsigned)O_CREAT;
+fd = open(path, oflag);
+}
+if (fd == -1) {
+error_setg_errno(errp, errno,
+ "failed to open file '%s' "
+ "(mode: '%s')",
+ path, mode);
+goto end;
+}
 
-/* If the caller wants / allows creation of a new file, we implement it
- * with a two step process: open() + (open() / fchmod()).
- *
- * First we insist on creating the file exclusively as a new file. If
- * that succeeds, we're free to set any file-mode bits on it. (The
- * motivation is that we want to set those file-mode bits independently
- * of the current umask.)
- *
- * If the exclusive creation fails because the file already exists
- * (EEXIST is not possible for any other reason), we just attempt to
- * open the file, but in this case we won't be allowed to change the
- * file-mode bits on the preexistent file.
- *
- * The pathname should never disappear between the two open()s in
- * practice. If it happens, then someone very likely tried to race us.
- * In this case just go ahead and report the ENOENT from the second
- * open() to the caller.
- *
- * If the caller wants to open a preexistent file, then the first
- * open() is decisive and its third argument is ignored, and the second
- * open() and the fchmod() are never called.
- */
-fd = open(path, oflag | ((oflag & O_CREAT) ? O_EXCL : 0), 0);
-if (fd == -1 && errno == EEXIST) {
-oflag &= ~(unsigned)O_CREAT;
-fd = open(path, oflag);
-}
+qemu_set_cloexec(fd);
 
-if (fd == -1) {
-error_setg_errno(_err, errno, "failed to open file '%s' "
- "(mode: '%s')", path, mode);
-} else {
-qemu_set_cloexec(fd);
+if ((oflag & O_CREAT) && fchmod(fd, DEFAULT_NEW_FILE_MODE) == -1) {
+error_setg_errno(errp, errno,
+ "failed to set permission 0%03o on new file '%s' 
(mode: '%s')",
+ (unsigned)DEFAULT_NEW_FILE_MODE, path, mode);
+goto end;
+}
 
-if ((oflag & O_CREAT) && fchmod(fd, DEFAULT_NEW_FILE_MODE) == -1) {
-error_setg_errno(_err, errno, "failed to set permission "
- "0%03o on new file '%s' (mode: '%s')",
- (unsigned)DEFAULT_NEW_FILE_MODE, path, mode);
-} else {
-FILE *f;
-
-f = fdopen(fd, mode);
-if (f == NULL) {
-

[PATCH v2 03/15] tests: make libqmp buildable for win32

[marcandre . lureau]

From: Marc-André Lureau 

Signed-off-by: Marc-André Lureau 
Reviewed-by: Thomas Huth 
---
 tests/qtest/libqmp.h |  2 ++
 tests/qtest/libqmp.c | 35 +--
 2 files changed, 31 insertions(+), 6 deletions(-)

diff --git a/tests/qtest/libqmp.h b/tests/qtest/libqmp.h
index 94aa97328a17..772f18b73ba3 100644
--- a/tests/qtest/libqmp.h
+++ b/tests/qtest/libqmp.h
@@ -20,8 +20,10 @@
 #include "qapi/qmp/qdict.h"
 
 QDict *qmp_fd_receive(int fd);
+#ifndef G_OS_WIN32
 void qmp_fd_vsend_fds(int fd, int *fds, size_t fds_num,
   const char *fmt, va_list ap) G_GNUC_PRINTF(4, 0);
+#endif
 void qmp_fd_vsend(int fd, const char *fmt, va_list ap) G_GNUC_PRINTF(2, 0);
 void qmp_fd_send(int fd, const char *fmt, ...) G_GNUC_PRINTF(2, 3);
 void qmp_fd_send_raw(int fd, const char *fmt, ...) G_GNUC_PRINTF(2, 3);
diff --git a/tests/qtest/libqmp.c b/tests/qtest/libqmp.c
index 0358b8313dc4..93c9b31cd4ca 100644
--- a/tests/qtest/libqmp.c
+++ b/tests/qtest/libqmp.c
@@ -15,9 +15,13 @@
  */
 
 #include "qemu/osdep.h"
-
 #include "libqmp.h"
 
+#ifndef G_OS_WIN32
+#include 
+#endif
+
+#include "qemu/cutils.h"
 #include "qapi/error.h"
 #include "qapi/qmp/json-parser.h"
 #include "qapi/qmp/qjson.h"
@@ -87,6 +91,7 @@ QDict *qmp_fd_receive(int fd)
 return qmp.response;
 }
 
+#ifndef G_OS_WIN32
 /* Sends a message and file descriptors to the socket.
  * It's needed for qmp-commands like getfd/add-fd */
 static void socket_send_fds(int socket_fd, int *fds, size_t fds_num,
@@ -120,17 +125,23 @@ static void socket_send_fds(int socket_fd, int *fds, 
size_t fds_num,
 } while (ret < 0 && errno == EINTR);
 g_assert_cmpint(ret, >, 0);
 }
+#endif
 
 /**
  * Allow users to send a message without waiting for the reply,
  * in the case that they choose to discard all replies up until
  * a particular EVENT is received.
  */
-void qmp_fd_vsend_fds(int fd, int *fds, size_t fds_num,
-  const char *fmt, va_list ap)
+static void
+_qmp_fd_vsend_fds(int fd, int *fds, size_t fds_num,
+  const char *fmt, va_list ap)
 {
 QObject *qobj;
 
+#ifdef G_OS_WIN32
+assert(fds_num == 0);
+#endif
+
 /* Going through qobject ensures we escape strings properly */
 qobj = qobject_from_vjsonf_nofail(fmt, ap);
 
@@ -148,10 +159,14 @@ void qmp_fd_vsend_fds(int fd, int *fds, size_t fds_num,
 if (log) {
 fprintf(stderr, "%s", str->str);
 }
+
+#ifndef G_OS_WIN32
 /* Send QMP request */
 if (fds && fds_num > 0) {
 socket_send_fds(fd, fds, fds_num, str->str, str->len);
-} else {
+} else
+#endif
+{
 socket_send(fd, str->str, str->len);
 }
 
@@ -160,15 +175,23 @@ void qmp_fd_vsend_fds(int fd, int *fds, size_t fds_num,
 }
 }
 
+#ifndef G_OS_WIN32
+void qmp_fd_vsend_fds(int fd, int *fds, size_t fds_num,
+  const char *fmt, va_list ap)
+{
+_qmp_fd_vsend_fds(fd, fds, fds_num, fmt, ap);
+}
+#endif
+
 void qmp_fd_vsend(int fd, const char *fmt, va_list ap)
 {
-qmp_fd_vsend_fds(fd, NULL, 0, fmt, ap);
+_qmp_fd_vsend_fds(fd, NULL, 0, fmt, ap);
 }
 
 
 QDict *qmp_fdv(int fd, const char *fmt, va_list ap)
 {
-qmp_fd_vsend_fds(fd, NULL, 0, fmt, ap);
+_qmp_fd_vsend_fds(fd, NULL, 0, fmt, ap);
 
 return qmp_fd_receive(fd);
 }
-- 
2.36.0.44.g0f828332d5ac

[PATCH v2 02/15] util/win32: simplify qemu_get_local_state_dir()

[marcandre . lureau]

From: Marc-André Lureau 

SHGetFolderPath() is a deprecated API:
https://docs.microsoft.com/en-us/windows/win32/api/shlobj_core/nf-shlobj_core-shgetfolderpatha

It is a wrapper for SHGetKnownFolderPath() and CSIDL_COMMON_PATH is
mapped to FOLDERID_ProgramData:
https://docs.microsoft.com/en-us/windows/win32/shell/csidl

g_get_system_data_dirs() is a suitable replacement, as it will have
FOLDERID_ProgramData in the returned list. However, it follows the XDG
Base Directory Specification, if `XDG_DATA_DIRS` is defined, it will be
returned instead.

Signed-off-by: Marc-André Lureau 
Reviewed-by: Stefan Weil 
---
 util/oslib-win32.c | 17 -
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index 6c818749d2b9..5723d3eb4c5a 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -40,9 +40,6 @@
 #include "qemu/error-report.h"
 #include 
 
-/* this must come after including "trace.h" */
-#include 
-
 static int get_allocation_granularity(void)
 {
 SYSTEM_INFO system_info;
@@ -237,17 +234,11 @@ int qemu_get_thread_id(void)
 char *
 qemu_get_local_state_dir(void)
 {
-HRESULT result;
-char base_path[MAX_PATH+1] = "";
+const char * const *data_dirs = g_get_system_data_dirs();
 
-result = SHGetFolderPath(NULL, CSIDL_COMMON_APPDATA, NULL,
- /* SHGFP_TYPE_CURRENT */ 0, base_path);
-if (result != S_OK) {
-/* misconfigured environment */
-g_critical("CSIDL_COMMON_APPDATA unavailable: %ld", (long)result);
-abort();
-}
-return g_strdup(base_path);
+g_assert(data_dirs && data_dirs[0]);
+
+return g_strdup(data_dirs[0]);
 }
 
 void qemu_set_tty_echo(int fd, bool echo)
-- 
2.36.0.44.g0f828332d5ac

[PATCH v2 00/15] Misc cleanups

[marcandre . lureau]

From: Marc-André Lureau 

Hi,

v2:
- drop "compiler.h: add QEMU_{BEGIN,END}_IGNORE_INITIALIZER_OVERRIDES",
  "qobject/json-lexer: disable -Winitializer-overrides warnings" &
  "qapi/error: add g_autoptr(Error) support" and adjust related code.
- add "test/qga: use g_auto wherever sensible"
- add r-b tags

Marc-André Lureau (15):
  include: move qemu_*_exec_dir() to cutils
  util/win32: simplify qemu_get_local_state_dir()
  tests: make libqmp buildable for win32
  include: adjust header guards after renaming
  qga: flatten safe_open_or_create()
  osdep: export qemu_open_cloexec()
  qga: use qemu_open_cloexec() for safe_open_or_create()
  qga: throw an Error in ga_channel_open()
  qga: replace qemu_open_old() with qemu_open_cloexec()
  test/qga: use G_TEST_DIR to locate os-release test file
  qga/wixl: prefer variables over environment
  qga/wixl: require Mingw_bin
  qga/wixl: simplify some pre-processing
  qga/wixl: replace QEMU_GA_MSI_MINGW_BIN_PATH with glib bindir
  test/qga: use g_auto wherever sensible

 configure|   9 +-
 meson.build  |   5 +-
 include/qemu/cutils.h|   7 ++
 include/qemu/help-texts.h|   4 +-
 include/qemu/osdep.h |   9 +-
 tests/qtest/libqmp.h |   2 +
 qemu-io.c|   1 +
 qga/channel-posix.c  |  54 +-
 qga/commands-posix.c | 145 +--
 storage-daemon/qemu-storage-daemon.c |   1 +
 tests/qtest/fuzz/fuzz.c  |   1 +
 tests/qtest/libqmp.c |  35 +--
 tests/unit/test-qga.c| 134 +
 util/cutils.c| 108 
 util/osdep.c |  10 +-
 util/oslib-posix.c   |  81 ---
 util/oslib-win32.c   |  53 +-
 qga/installer/qemu-ga.wxs|  83 ++-
 qga/meson.build  |  11 +-
 19 files changed, 352 insertions(+), 401 deletions(-)

-- 
2.36.0.44.g0f828332d5ac

Re: [PATCH v11 2/7] QIOChannelSocket: Implement io_writev zero copy flag & io_flush for CONFIG_LINUX

[Daniel P . Berrangé]

On Wed, May 04, 2022 at 04:18:31PM -0300, Leonardo Bras wrote:
> For CONFIG_LINUX, implement the new zero copy flag and the optional callback
> io_flush on QIOChannelSocket, but enables it only when MSG_ZEROCOPY
> feature is available in the host kernel, which is checked on
> qio_channel_socket_connect_sync()
> 
> qio_channel_socket_flush() was implemented by counting how many times
> sendmsg(...,MSG_ZEROCOPY) was successfully called, and then reading the
> socket's error queue, in order to find how many of them finished sending.
> Flush will loop until those counters are the same, or until some error occurs.
> 
> Notes on using writev() with QIO_CHANNEL_WRITE_FLAG_ZERO_COPY:
> 1: Buffer
> - As MSG_ZEROCOPY tells the kernel to use the same user buffer to avoid 
> copying,
> some caution is necessary to avoid overwriting any buffer before it's sent.
> If something like this happen, a newer version of the buffer may be sent 
> instead.
> - If this is a problem, it's recommended to call qio_channel_flush() before 
> freeing
> or re-using the buffer.
> 
> 2: Locked memory
> - When using MSG_ZERCOCOPY, the buffer memory will be locked after queued, and
> unlocked after it's sent.
> - Depending on the size of each buffer, and how often it's sent, it may 
> require
> a larger amount of locked memory than usually available to non-root user.
> - If the required amount of locked memory is not available, writev_zero_copy
> will return an error, which can abort an operation like migration,
> - Because of this, when an user code wants to add zero copy as a feature, it
> requires a mechanism to disable it, so it can still be accessible to less
> privileged users.
> 
> Signed-off-by: Leonardo Bras 
> Reviewed-by: Peter Xu 
> Reviewed-by: Daniel P. Berrangé 
> Reviewed-by: Juan Quintela 
> ---
>  include/io/channel-socket.h |   2 +
>  io/channel-socket.c | 120 ++--
>  2 files changed, 118 insertions(+), 4 deletions(-)
> 
> diff --git a/include/io/channel-socket.h b/include/io/channel-socket.h
> index e747e63514..513c428fe4 100644
> --- a/include/io/channel-socket.h
> +++ b/include/io/channel-socket.h
> @@ -47,6 +47,8 @@ struct QIOChannelSocket {
>  socklen_t localAddrLen;
>  struct sockaddr_storage remoteAddr;
>  socklen_t remoteAddrLen;
> +ssize_t zero_copy_queued;
> +ssize_t zero_copy_sent;
>  };
>  
>  
> diff --git a/io/channel-socket.c b/io/channel-socket.c
> index 696a04dc9c..ae756ce166 100644
> --- a/io/channel-socket.c
> +++ b/io/channel-socket.c
> @@ -25,9 +25,25 @@
>  #include "io/channel-watch.h"
>  #include "trace.h"
>  #include "qapi/clone-visitor.h"
> +#ifdef CONFIG_LINUX
> +#include 
> +#include 
> +#endif
>  
>  #define SOCKET_MAX_FDS 16
>  
> +/*
> + * Zero-copy defines bellow are included to avoid breaking builds on systems
> + * that don't support MSG_ZEROCOPY, while keeping the functions more readable
> + * (without a lot of ifdefs).
> + */
> +#ifndef MSG_ZEROCOPY
> +#define MSG_ZEROCOPY 0x400
> +#endif
> +#ifndef SO_ZEROCOPY
> +#define SO_ZEROCOPY 60
> +#endif

Please put these behind CONFIG_LINUX to make it clear to readers that
this is entirely Linux specific


> +
>  SocketAddress *
>  qio_channel_socket_get_local_address(QIOChannelSocket *ioc,
>   Error **errp)
> @@ -54,6 +70,8 @@ qio_channel_socket_new(void)
>  
>  sioc = QIO_CHANNEL_SOCKET(object_new(TYPE_QIO_CHANNEL_SOCKET));
>  sioc->fd = -1;
> +sioc->zero_copy_queued = 0;
> +sioc->zero_copy_sent = 0;
>  
>  ioc = QIO_CHANNEL(sioc);
>  qio_channel_set_feature(ioc, QIO_CHANNEL_FEATURE_SHUTDOWN);
> @@ -153,6 +171,16 @@ int qio_channel_socket_connect_sync(QIOChannelSocket 
> *ioc,
>  return -1;
>  }
>  
> +#ifdef CONFIG_LINUX
> +int ret, v = 1;
> +ret = setsockopt(fd, SOL_SOCKET, SO_ZEROCOPY, , sizeof(v));
> +if (ret == 0) {
> +/* Zero copy available on host */
> +qio_channel_set_feature(QIO_CHANNEL(ioc),
> +QIO_CHANNEL_FEATURE_WRITE_ZERO_COPY);
> +}
> +#endif
> +
>  return 0;
>  }
>  
> @@ -533,6 +561,7 @@ static ssize_t qio_channel_socket_writev(QIOChannel *ioc,
>  char control[CMSG_SPACE(sizeof(int) * SOCKET_MAX_FDS)];
>  size_t fdsize = sizeof(int) * nfds;
>  struct cmsghdr *cmsg;
> +int sflags = 0;
>  
>  memset(control, 0, CMSG_SPACE(sizeof(int) * SOCKET_MAX_FDS));
>  
> @@ -557,15 +586,27 @@ static ssize_t qio_channel_socket_writev(QIOChannel 
> *ioc,
>  memcpy(CMSG_DATA(cmsg), fds, fdsize);
>  }
>  
> +if (flags & QIO_CHANNEL_WRITE_FLAG_ZERO_COPY) {
> +sflags = MSG_ZEROCOPY;
> +}

Also should be behind CONFIG_LINUX

> +
>   retry:
> -ret = sendmsg(sioc->fd, , 0);
> +ret = sendmsg(sioc->fd, , sflags);
>  if (ret <= 0) {
> -if (errno == EAGAIN) {
> +switch (errno) {
> +case EAGAIN:
>  return QIO_CHANNEL_ERR_BLOCK;
> -}
> -if 

Re: [PATCH 00/16] Misc cleanups

[Paolo Bonzini]

Looks good apart from the two patches I comment on.

Paolo

Il 4 maggio 2022 19:30:09 CEST, marcandre.lur...@redhat.com ha scritto:
>From: Marc-André Lureau 
>
>Hi,
>
>Perhaps the last series of preliminary patches before I propose a longer series
>to add qemu-common & qga meson subprojects. That's why they are mostly
>QGA-related cleanups.
>
>Thanks for the reviews!
>
>Marc-André Lureau (16):
>  include: move qemu_*_exec_dir() to cutils
>  util/win32: simplify qemu_get_local_state_dir()
>  tests: make libqmp buildable for win32
>  compiler.h: add QEMU_{BEGIN,END}_IGNORE_INITIALIZER_OVERRIDES
>  qobject/json-lexer: disable -Winitializer-overrides warnings
>  include: adjust header guards after renaming
>  qga: flatten safe_open_or_create()
>  osdep: export qemu_open_cloexec()
>  qga: use qemu_open_cloexec() for safe_open_or_create()
>  qapi/error: add g_autoptr(Error) support
>  qga: replace qemu_open_old() with qemu_open_cloexec()
>  test/qga: use G_TEST_DIR to locate os-release test file
>  qga/wixl: prefer variables over environment
>  qga/wixl: require Mingw_bin
>  qga/wixl: simplify some pre-processing
>  qga/wixl: replace QEMU_GA_MSI_MINGW_BIN_PATH with glib bindir
>
> configure|   9 +-
> meson.build  |   5 +-
> include/qapi/error.h |   2 +
> include/qemu/compiler.h  |  11 ++
> include/qemu/cutils.h|   7 ++
> include/qemu/help-texts.h|   4 +-
> include/qemu/osdep.h |   9 +-
> tests/qtest/libqmp.h |   2 +
> qemu-io.c|   1 +
> qga/channel-posix.c  |  18 ++--
> qga/commands-posix.c | 146 +--
> qobject/json-lexer.c |   4 +
> storage-daemon/qemu-storage-daemon.c |   1 +
> tests/qtest/fuzz/fuzz.c  |   1 +
> tests/qtest/libqmp.c |  35 +--
> tests/unit/test-qga.c|  11 +-
> util/cutils.c| 108 
> util/osdep.c |  10 +-
> util/oslib-posix.c   |  81 ---
> util/oslib-win32.c   |  53 +-
> qga/installer/qemu-ga.wxs|  83 +--
> qga/meson.build  |  11 +-
> 22 files changed, 313 insertions(+), 299 deletions(-)
>

Re: [PATCH 04/16] compiler.h: add QEMU_{BEGIN, END}_IGNORE_INITIALIZER_OVERRIDES

[Paolo Bonzini]

>If other projects want to borrow bits of QEMU code then
>they need to either (a) abide by our conventions for
>what compiler warnings to enable or disable, or else
>(b) fork the code and fiddle with their own copy.

Agreed, it's not a huge deal if a single add_project_arguments call is 
duplicated across a couple meson subprojects.

Paolo

>
>I don't really want to see QEMU's source code get littered
>with a pile of extra macros hiding diagnostic pragmas.
>(If we stop passing -Wno-initializer-overrides to the
>compiler then we set a bunch of new "built on gcc on the
>developer's machine but fails to build on clang in the
>CI jobs" traps for ourselves, and if we don't stop passing
>that then the places that should be marked up with the
>macros won't reliably be marked up.)
>
>thanks
>-- PMM
>

Re: [PATCH 10/16] qapi/error: add g_autoptr(Error) support

[Paolo Bonzini]

This was rejected before on the grounds that propagating or printing the error 
is usually the right thing to do, and neither needs an autoptr:

https://patchew.org/QEMU/20210912124834.503032-1-pbonz...@redhat.com/

So while I do agree with the patch, for it to be accepted some Error* functions 
would have to be changed to accept Error** instead. This way they can NULL the 
variable containing their argument, and prevent the auto-error_free.

Paolo

Il 4 maggio 2022 19:30:19 CEST, marcandre.lur...@redhat.com ha scritto:
>From: Marc-André Lureau 
>
>Sometime, ERRP_GUARD() isn't what you are looking for, because the
>function doesn't throw errors, yet auto-cleaning is nice to have.
>
>Signed-off-by: Marc-André Lureau 
>---
> include/qapi/error.h | 2 ++
> 1 file changed, 2 insertions(+)
>
>diff --git a/include/qapi/error.h b/include/qapi/error.h
>index d798faeec3e9..9482b6a58ae6 100644
>--- a/include/qapi/error.h
>+++ b/include/qapi/error.h
>@@ -519,6 +519,8 @@ static inline void 
>error_propagator_cleanup(ErrorPropagator *prop)
> 
> G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagator, error_propagator_cleanup);
> 
>+G_DEFINE_AUTOPTR_CLEANUP_FUNC(Error, error_free);
>+
> /*
>  * Special error destination to abort on error.
>  * See error_setg() and error_propagate() for details.