Re: [PATCH] tests/avocado: Move LinuxTest related code into a separate file
On Fri, Jul 19, 2024 at 11:50:31AM +0200, Thomas Huth wrote: > Only some few tests are using the LinuxTest class. Move the related > code into a separate file so that this does not pollute the main > namespace. > > Signed-off-by: Thomas Huth > --- > tests/avocado/avocado_qemu/__init__.py | 239 +- > tests/avocado/avocado_qemu/linuxtest.py | 253 > tests/avocado/boot_linux.py | 3 +- > tests/avocado/hotplug_blk.py| 2 +- > tests/avocado/hotplug_cpu.py| 2 +- > tests/avocado/intel_iommu.py| 2 +- > tests/avocado/replay_linux.py | 2 +- > tests/avocado/smmu.py | 3 +- > 8 files changed, 262 insertions(+), 244 deletions(-) > create mode 100644 tests/avocado/avocado_qemu/linuxtest.py Reviewed-by: Daniel P. Berrangé 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] tests/avocado/mem-addr-space-check: Remove unused "import signal"
On Fri, Jul 19, 2024 at 11:54:08AM +0200, Thomas Huth wrote: > The "signal" module is not used here, so we can remove this import > statement. > > Signed-off-by: Thomas Huth > --- > tests/avocado/mem-addr-space-check.py | 1 - > 1 file changed, 1 deletion(-) Reviewed-by: Daniel P. Berrangé 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: [PULL 3/3] hw/loongarch: Modify flash block size to 256K
On Fri, Jul 19, 2024 at 10:12:20AM +0200, Philippe Mathieu-Daudé wrote: > On 19/7/24 04:26, Song Gao wrote: > > From: Xianglai Li > > > > loongarch added a common library for edk2 to > > parse flash base addresses through fdt. > > For compatibility with other architectures, > > the flash block size in qemu is now changed to 256k. > > > > Signed-off-by: Xianglai Li > > Reviewed-by: Song Gao > > Message-Id: <20240624033319.999631-1-lixiang...@loongson.cn> > > Signed-off-by: Song Gao > > --- > > include/hw/loongarch/virt.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/include/hw/loongarch/virt.h b/include/hw/loongarch/virt.h > > index 8fdfacf268..603c1cebdb 100644 > > --- a/include/hw/loongarch/virt.h > > +++ b/include/hw/loongarch/virt.h > > @@ -20,7 +20,7 @@ > > #define VIRT_FWCFG_BASE 0x1e02UL > > #define VIRT_BIOS_BASE 0x1c00UL > > #define VIRT_BIOS_SIZE (16 * MiB) > > -#define VIRT_FLASH_SECTOR_SIZE (128 * KiB) > > +#define VIRT_FLASH_SECTOR_SIZE (256 * KiB) > > Again, I believe this breaks machine migration. See the recent > example Daniel explained to me: > https://lore.kernel.org/qemu-devel/zn6eq39q57ktm...@redhat.com/ Yes, changing flash size breaks migration compat, but note that loongarch does not have any versioned machine types, so it has zero migration compat right now regardles of this change. IOW, now is the right time to make the change, before someone asks for versioned machines with loongarch. 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 v2] vnc: increase max display size
On Thu, Jul 18, 2024 at 04:58:10PM +0100, Peter Maydell wrote: > On Thu, 30 May 2024 at 12:11, Gerd Hoffmann wrote: > > > > It's 2024. 4k display resolutions are a thing these days. > > Raise width and height limits of the qemu vnc server. > > > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1596 > > Signed-off-by: Gerd Hoffmann > > --- > > ui/vnc.h | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/ui/vnc.h b/ui/vnc.h > > index 4521dc88f792..e5fa2efa3e5d 100644 > > --- a/ui/vnc.h > > +++ b/ui/vnc.h > > @@ -81,8 +81,8 @@ typedef void VncSendHextileTile(VncState *vs, > > > > /* VNC_MAX_WIDTH must be a multiple of VNC_DIRTY_PIXELS_PER_BIT. */ > > > > -#define VNC_MAX_WIDTH ROUND_UP(2560, VNC_DIRTY_PIXELS_PER_BIT) > > -#define VNC_MAX_HEIGHT 2048 > > +#define VNC_MAX_WIDTH ROUND_UP(5120, VNC_DIRTY_PIXELS_PER_BIT) > > +#define VNC_MAX_HEIGHT 2160 > > > > /* VNC_DIRTY_BITS is the number of bits in the dirty bitmap. */ > > #define VNC_DIRTY_BITS (VNC_MAX_WIDTH / VNC_DIRTY_PIXELS_PER_BIT) > > Hi -- somebody on IRC pointed out that this simple patch > had been code-reviewed by Daniel but never made it into git. > Marc-André: you're listed maintainer for ui/ -- do you have > a pullreq planned? > > Alternatively we could take it via qemu-trivial since it's > a pretty tiny patch (cc'd). I'm working on a misc pull request, so I'll just include this patch 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 v5 3/4] docs/interop/firmware.json: convert "Example" section
On Thu, Jul 18, 2024 at 02:21:47PM +0200, Thomas Weißschuh wrote:
> Since commit 3c5f6114d9ff ("qapi: remove "Example" doc section")
> the "Example" section is not valid anymore.
> It has been replaced by the "qmp-example" role.
>
> This was not detected earlier as firmware.json was not validated.
> As this validation is about to be added, adapt firmware.json.
>
> Signed-off-by: Thomas Weißschuh
> ---
> docs/interop/firmware.json | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Daniel P. Berrangé
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 1/2] ci: add gtk-vnc to the deps
On Thu, Jul 18, 2024 at 12:34:13PM +0200, Thomas Huth wrote: > On 18/07/2024 11.41, Daniel P. Berrangé wrote: > > The gtk-vnc package is used by the vnc-display-test qtest > > program. Technically only gvnc is needed, but since we > > already pull in the gtk3 dep, it is harmless to depend > > on gtk-vnc. > > > > Signed-off-by: Daniel P. Berrangé > > --- > > tests/lcitool/projects/qemu.yml | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/tests/lcitool/projects/qemu.yml > > b/tests/lcitool/projects/qemu.yml > > index 0c85784259..252e871f80 100644 > > --- a/tests/lcitool/projects/qemu.yml > > +++ b/tests/lcitool/projects/qemu.yml > > @@ -32,6 +32,7 @@ packages: > >- glusterfs > >- gnutls > >- gtk3 > > + - gtk-vnc > >- hostname > >- json-c > >- libaio > > Reviewed-by: Thomas Huth > > IIRC Alex has a patch in his queue already to refresh the docker images, > maybe he could include this change there, too...? Yep, I included patch 2 just for completeness, assuming Alex would just pick patch 1, and then re-generate. 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 0/2] ci: fix running of vnc-display-test
The vnc-display-test is skipped in CI due to missing gvnc RPMs, fix that. Daniel P. Berrangé (2): ci: add gtk-vnc to the deps ci: refresh package lists with lcitool .gitlab-ci.d/cirrus/freebsd-13.vars | 2 +- .gitlab-ci.d/cirrus/macos-13.vars | 2 +- .gitlab-ci.d/cirrus/macos-14.vars | 2 +- scripts/ci/setup/ubuntu/ubuntu-2204-aarch64.yaml | 1 + scripts/ci/setup/ubuntu/ubuntu-2204-s390x.yaml| 1 + tests/docker/dockerfiles/alpine.docker| 1 + tests/docker/dockerfiles/debian-amd64-cross.docker| 1 + tests/docker/dockerfiles/debian-arm64-cross.docker| 1 + tests/docker/dockerfiles/debian-armel-cross.docker| 1 + tests/docker/dockerfiles/debian-armhf-cross.docker| 1 + tests/docker/dockerfiles/debian-i686-cross.docker | 1 + tests/docker/dockerfiles/debian-mips64el-cross.docker | 1 + tests/docker/dockerfiles/debian-mipsel-cross.docker | 1 + tests/docker/dockerfiles/debian-ppc64el-cross.docker | 1 + tests/docker/dockerfiles/debian-s390x-cross.docker| 1 + tests/docker/dockerfiles/debian.docker| 1 + tests/docker/dockerfiles/fedora-win64-cross.docker| 1 + tests/docker/dockerfiles/fedora.docker| 1 + tests/docker/dockerfiles/opensuse-leap.docker | 1 + tests/docker/dockerfiles/ubuntu2204.docker| 1 + tests/lcitool/projects/qemu.yml | 1 + tests/vm/generated/freebsd.json | 1 + 22 files changed, 22 insertions(+), 3 deletions(-) -- 2.45.2
[PATCH 2/2] ci: refresh package lists with lcitool
Refresh with the newly added gtk-vnc package Signed-off-by: Daniel P. Berrangé --- .gitlab-ci.d/cirrus/freebsd-13.vars | 2 +- .gitlab-ci.d/cirrus/macos-13.vars | 2 +- .gitlab-ci.d/cirrus/macos-14.vars | 2 +- scripts/ci/setup/ubuntu/ubuntu-2204-aarch64.yaml | 1 + scripts/ci/setup/ubuntu/ubuntu-2204-s390x.yaml| 1 + tests/docker/dockerfiles/alpine.docker| 1 + tests/docker/dockerfiles/debian-amd64-cross.docker| 1 + tests/docker/dockerfiles/debian-arm64-cross.docker| 1 + tests/docker/dockerfiles/debian-armel-cross.docker| 1 + tests/docker/dockerfiles/debian-armhf-cross.docker| 1 + tests/docker/dockerfiles/debian-i686-cross.docker | 1 + tests/docker/dockerfiles/debian-mips64el-cross.docker | 1 + tests/docker/dockerfiles/debian-mipsel-cross.docker | 1 + tests/docker/dockerfiles/debian-ppc64el-cross.docker | 1 + tests/docker/dockerfiles/debian-s390x-cross.docker| 1 + tests/docker/dockerfiles/debian.docker| 1 + tests/docker/dockerfiles/fedora-win64-cross.docker| 1 + tests/docker/dockerfiles/fedora.docker| 1 + tests/docker/dockerfiles/opensuse-leap.docker | 1 + tests/docker/dockerfiles/ubuntu2204.docker| 1 + tests/vm/generated/freebsd.json | 1 + 21 files changed, 21 insertions(+), 3 deletions(-) diff --git a/.gitlab-ci.d/cirrus/freebsd-13.vars b/.gitlab-ci.d/cirrus/freebsd-13.vars index 3785afca36..81f44a782c 100644 --- a/.gitlab-ci.d/cirrus/freebsd-13.vars +++ b/.gitlab-ci.d/cirrus/freebsd-13.vars @@ -11,6 +11,6 @@ MAKE='/usr/local/bin/gmake' NINJA='/usr/local/bin/ninja' PACKAGING_COMMAND='pkg' PIP3='/usr/local/bin/pip-3.8' -PKGS='alsa-lib bash bison bzip2 ca_root_nss capstone4 ccache cmocka ctags curl cyrus-sasl dbus diffutils dtc flex fusefs-libs3 gettext git glib gmake gnutls gsed gtk3 json-c libepoxy libffi libgcrypt libjpeg-turbo libnfs libslirp libspice-server libssh libtasn1 llvm lzo2 meson mtools ncurses nettle ninja opencv pixman pkgconf png py39-numpy py39-pillow py39-pip py39-sphinx py39-sphinx_rtd_theme py39-tomli py39-yaml python3 rpm2cpio sdl2 sdl2_image snappy sndio socat spice-protocol tesseract usbredir virglrenderer vte3 xorriso zstd' +PKGS='alsa-lib bash bison bzip2 ca_root_nss capstone4 ccache cmocka ctags curl cyrus-sasl dbus diffutils dtc flex fusefs-libs3 gettext git glib gmake gnutls gsed gtk-vnc gtk3 json-c libepoxy libffi libgcrypt libjpeg-turbo libnfs libslirp libspice-server libssh libtasn1 llvm lzo2 meson mtools ncurses nettle ninja opencv pixman pkgconf png py39-numpy py39-pillow py39-pip py39-sphinx py39-sphinx_rtd_theme py39-tomli py39-yaml python3 rpm2cpio sdl2 sdl2_image snappy sndio socat spice-protocol tesseract usbredir virglrenderer vte3 xorriso zstd' PYPI_PKGS='' PYTHON='/usr/local/bin/python3' diff --git a/.gitlab-ci.d/cirrus/macos-13.vars b/.gitlab-ci.d/cirrus/macos-13.vars index 534f029956..ac3fa3a847 100644 --- a/.gitlab-ci.d/cirrus/macos-13.vars +++ b/.gitlab-ci.d/cirrus/macos-13.vars @@ -11,6 +11,6 @@ MAKE='/opt/homebrew/bin/gmake' NINJA='/opt/homebrew/bin/ninja' PACKAGING_COMMAND='brew' PIP3='/opt/homebrew/bin/pip3' -PKGS='bash bc bison bzip2 capstone ccache cmocka ctags curl dbus diffutils dtc flex gcovr gettext git glib gnu-sed gnutls gtk+3 jemalloc jpeg-turbo json-c libepoxy libffi libgcrypt libiscsi libnfs libpng libslirp libssh libtasn1 libusb llvm lzo make meson mtools ncurses nettle ninja pixman pkg-config python3 rpm2cpio sdl2 sdl2_image snappy socat sparse spice-protocol swtpm tesseract usbredir vde vte3 xorriso zlib zstd' +PKGS='bash bc bison bzip2 capstone ccache cmocka ctags curl dbus diffutils dtc flex gcovr gettext git glib gnu-sed gnutls gtk+3 gtk-vnc jemalloc jpeg-turbo json-c libepoxy libffi libgcrypt libiscsi libnfs libpng libslirp libssh libtasn1 libusb llvm lzo make meson mtools ncurses nettle ninja pixman pkg-config python3 rpm2cpio sdl2 sdl2_image snappy socat sparse spice-protocol swtpm tesseract usbredir vde vte3 xorriso zlib zstd' PYPI_PKGS='PyYAML numpy pillow sphinx sphinx-rtd-theme tomli' PYTHON='/opt/homebrew/bin/python3' diff --git a/.gitlab-ci.d/cirrus/macos-14.vars b/.gitlab-ci.d/cirrus/macos-14.vars index 43070f4a26..24cfec3b89 100644 --- a/.gitlab-ci.d/cirrus/macos-14.vars +++ b/.gitlab-ci.d/cirrus/macos-14.vars @@ -11,6 +11,6 @@ MAKE='/opt/homebrew/bin/gmake' NINJA='/opt/homebrew/bin/ninja' PACKAGING_COMMAND='brew' PIP3='/opt/homebrew/bin/pip3' -PKGS='bash bc bison bzip2 capstone ccache cmocka ctags curl dbus diffutils dtc flex gcovr gettext git glib gnu-sed gnutls gtk+3 jemalloc jpeg-turbo json-c libepoxy libffi libgcrypt libiscsi libnfs libpng libslirp libssh libtasn1 libusb llvm lzo make meson mtools ncurses nettle ninja pixman pkg-config python3 rpm2cpio sdl2 sdl2_image snappy socat sparse spice-protocol swtpm tesseract usbredir vde vte3 xorriso zlib zstd' +PKGS='bash bc
[PATCH 1/2] ci: add gtk-vnc to the deps
The gtk-vnc package is used by the vnc-display-test qtest program. Technically only gvnc is needed, but since we already pull in the gtk3 dep, it is harmless to depend on gtk-vnc. Signed-off-by: Daniel P. Berrangé --- tests/lcitool/projects/qemu.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/lcitool/projects/qemu.yml b/tests/lcitool/projects/qemu.yml index 0c85784259..252e871f80 100644 --- a/tests/lcitool/projects/qemu.yml +++ b/tests/lcitool/projects/qemu.yml @@ -32,6 +32,7 @@ packages: - glusterfs - gnutls - gtk3 + - gtk-vnc - hostname - json-c - libaio -- 2.45.2
Re: [PATCH v3] chardev: add path option for pty backend
On Thu, Jul 18, 2024 at 08:15:01AM +0200, Markus Armbruster wrote:
> Looks like this one fell through the cracks.
>
> Octavian Purdila writes:
>
> > Add path option to the pty char backend which will create a symbolic
> > link to the given path that points to the allocated PTY.
> >
> > This avoids having to make QMP or HMP monitor queries to find out what
> > the new PTY device path is.
>
> QMP commands chardev-add and chardev-change return the information you
> want:
>
> # @pty: name of the slave pseudoterminal device, present if and only
> # if a chardev of type 'pty' was created
>
> So does HMP command chardev-add. HMP chardev apparently doesn't, but
> that could be fixed.
It does print it:
(qemu) chardev-add pty,id=bar
char device redirected to /dev/pts/12 (label bar)
> So, the use case is basically the command line, right?
Also cli prints it
$ qemu-system-x86_64 -chardev pty,id=foo -monitor stdio -display none
char device redirected to /dev/pts/10 (label foo)
> > Based on patch from Paulo Neves:
> >
> > https://patchew.org/QEMU/1548509635-15776-1-git-send-email-ptsne...@gmail.com/
> >
> > Tested with the following invocations that the link is created and
> > removed when qemu stops:
> >
> > qemu-system-x86_64 -nodefaults -mon chardev=compat_monitor \
> > -chardev pty,path=test,id=compat_monitor0
> >
> > qemu-system-x86_64 -nodefaults -monitor pty:test
> >
> > Also tested that when a link path is not passed invocations still work,
> > e.g.:
> >
> > qemu-system-x86_64 -monitor pty
> >
> > Co-authored-by: Paulo Neves
> > Signed-off-by: Paulo Neves
> > [OP: rebase and address original patch review comments]
> > Signed-off-by: Octavian Purdila
> > Reviewed-by: Marc-André Lureau
> > ---
> > Changes since v2:
> >
> > * remove NULL path check, g_strdup() allows NULL inputs
> >
> > Changes since v1:
> >
> > * Keep the original Signed-off-by from Paulo and add one line
> > description with further changes
> >
> > * Update commit message with justification for why the new
> > functionality is useful
> >
> > * Don't close master_fd when symlink creation fails to avoid double
> > close
> >
> > * Update documentation for clarity
> >
> > chardev/char-pty.c | 33 +
> > chardev/char.c | 5 +
> > qapi/char.json | 4 ++--
> > qemu-options.hx| 24 ++--
> > 4 files changed, 58 insertions(+), 8 deletions(-)
> >
> > diff --git a/chardev/char-pty.c b/chardev/char-pty.c
> > index cc2f7617fe..5c6172ddba 100644
> > --- a/chardev/char-pty.c
> > +++ b/chardev/char-pty.c
> > @@ -29,6 +29,7 @@
> > #include "qemu/sockets.h"
> > #include "qemu/error-report.h"
> > #include "qemu/module.h"
> > +#include "qemu/option.h"
> > #include "qemu/qemu-print.h"
> >
> > #include "chardev/char-io.h"
> > @@ -41,6 +42,7 @@ struct PtyChardev {
> >
> > int connected;
> > GSource *timer_src;
> > +char *symlink_path;
> > };
> > typedef struct PtyChardev PtyChardev;
> >
> > @@ -204,6 +206,12 @@ static void char_pty_finalize(Object *obj)
> > Chardev *chr = CHARDEV(obj);
> > PtyChardev *s = PTY_CHARDEV(obj);
> >
> > +/* unlink symlink */
> > +if (s->symlink_path) {
> > +unlink(s->symlink_path);
> > +g_free(s->symlink_path);
> > +}
>
> Runs when the chardev object is finalized.
>
> Doesn't run when QEMU crashes. Stale symlink left behind then. Can't
> see how you could avoid that at reasonable cost. Troublesome all the
> same.
Do we ever guarantee that the finalizer runs ? eg dif we have
error_setg(_exit,
that's a clean exit, not a crash, but I don't think chardev finalizers
will run, as we don't do atexit() hooks for it.
> The feature feels rather doubtful to me, to be honest.
On the one hand I understand the pain - long ago libvirt had to deal
with parsing the console messages
char device redirected to /dev/pts/10 (label foo)
before we switched to using QMP to query this.
On the other hand, in retrospect libvirt should never have used the 'pty'
backend in the first place. The 'unix' socket backend is a choice as it
has predictable filenames, and it has proper connection oriented semantics,
so QEMU can reliably detect when clients disconnect, which has always been
troublesome for the 'pty' backend.
So while I can understand the desire to add a 'path' option to 'pty'
to trigger symlink creation, I think we could choose to tell people
to use the 'unix' socket backend instead if they want a predictable
path. This would avoid us creating the difficult to fix bug for
symlink deletion in error conditions.
What's the key benefit of the 'pty' backend, that 'unix' doesn't
handle ?
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 v4 3/3] docs: add test for firmware.json QAPI
On Thu, Jul 18, 2024 at 10:27:40AM +0200, Thomas Weißschuh wrote: > To make sure that the QAPI description stays valid add a testcase. > > Suggested-by: Philippe Mathieu-Daudé > Link: > https://lore.kernel.org/qemu-devel/d9ce0234-4beb-4b90-b14c-76810d3b8...@linaro.org/ > Signed-off-by: Thomas Weißschuh > --- > docs/meson.build | 5 + > 1 file changed, 5 insertions(+) Reviewed-by: Daniel P. Berrangé 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 v4 2/3] docs/interop/firmware.json: add new enum FirmwareArchitecture
On Thu, Jul 18, 2024 at 10:27:39AM +0200, Thomas Weißschuh wrote: > Only a small subset of all architectures supported by qemu make use of > firmware files. Introduce and use a new enum to represent this. > > This also removes the dependency to machine.json from the global qapi > definitions. > > Suggested-by: Daniel P. Berrangé > Signed-off-by: Thomas Weißschuh > --- > docs/interop/firmware.json | 29 +++-- > 1 file changed, 27 insertions(+), 2 deletions(-) Reviewed-by: Daniel P. Berrangé 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 v4 1/3] docs/interop/firmware.json: add new enum FirmwareFormat
On Thu, Jul 18, 2024 at 10:27:38AM +0200, Thomas Weißschuh wrote: > Only a small subset of all blockdev drivers make sense for firmware > images. Introduce and use a new enum to represent this. > > This also reduces the dependency on firmware.json from the global qapi > definitions. > > Suggested-by: Daniel P. Berrangé > Signed-off-by: Thomas Weißschuh > --- > docs/interop/firmware.json | 17 +++-- > 1 file changed, 15 insertions(+), 2 deletions(-) Reviewed-by: Daniel P. Berrangé 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 v2] scripts/checkpatch: more checks on files imported from Linux
On Thu, Jul 18, 2024 at 09:20:50AM +0200, Stefano Garzarella wrote: > If a file imported from Linux is touched, emit a warning and suggest > using scripts/update-linux-headers.sh. > > Also check that updating imported files from Linux are not mixed with > other changes, in which case emit an error. > > Signed-off-by: Stefano Garzarella > --- > v2: > - added an error when mixing imported files with other changes [Daniel, > Cornelia] > > v1: https://patchew.org/QEMU/20240717093752.50595-1-sgarz...@redhat.com/ > --- > scripts/checkpatch.pl | 24 > 1 file changed, 24 insertions(+) Reviewed-by: Daniel P. Berrangé 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 04/14] qapi: add a 'command-features' pragma
On Tue, Jul 16, 2024 at 08:08:42PM +0200, Markus Armbruster wrote:
> Sorry for the delay; too many distractions, and I needed a good think.
>
> Daniel P. Berrangé writes:
>
> > On Fri, Jul 12, 2024 at 10:50:54AM +0200, Markus Armbruster wrote:
> >> Daniel P. Berrangé writes:
> >>
> >> > On Fri, Jul 12, 2024 at 10:07:34AM +0200, Markus Armbruster wrote:
> >> >> Daniel P. Berrangé writes:
> >> >>
> >> >> > The 'command-features' pragma allows for defining additional
> >> >> > special features that are unique to a particular QAPI schema
> >> >> > instance and its implementation.
> >> >>
> >> >> So far, we have special features (predefined, known to the generator and
> >> >> treated specially), and normal features (user-defined, not known to the
> >> >> generator). You create a new kind in between: user-defined, not known
> >> >> to the generator, yet treated specially (I guess?). Hmm.
> >> >>
> >> >> Could you at least hint at indented use here? What special treatment do
> >> >> you have in mind?
> >> >
> >> > Essentially, these features are a way to attach metadata to commands that
> >> > the server side impl can later query. This eliminates the need to
> >> > hardcode
> >> > lists of commands, such as in QGA which hardcodes a list of commands
> >> > which
> >> > are safe to use when filesystems are frozen. This is illustrated later in
> >> > this series.
> >>
> >> Please update docs/devel/qapi-code-gen.rst section "Pragma directives",
> >> and maybe section "Features".
>
> Second thoughts; see below.
>
> >> I'm not sure conflating the new kind of feature with existing special
> >> features is a good idea. I need to review more of the series before I
> >> can make up my mind.
> >
> > I originally implemented a completely separate 'tags' concept in the
> > QAPI parser, before deciding I was just re-inventing 'features' for
> > no obvious benefit.
> >
> > The other nice thing about using features is that these are exposed
> > in the schema and docs. With the 'fsfreeze' restriction in code,
> > there's no formal docs of what commands are allowed when frozen, and
> > this is also not exposed in QAPI schema to apps. Using 'features'
> > we get all that as standard.
>
> When you need to tack a mark to one or more things for whatever purpose
> *and* expose it to QMP clients, then features make sense. This is the
> case here.
>
> Initially, features were strictly an external interface annotation, and
> were not meant to be used within QEMU. All features were user-defined.
>
> This changed when I created configurable policy for deprecated and
> unstable management interfaces: the policy engine needs to check for
> features 'deprecated' and 'unstable'. Since the policy engine is partly
> implemented in generated code, these two features need to be baked into
> the generator. This makes them special.
>
> You need less than that: a predicate "does have " for
> certain features, ideally without baking them into the generator.
>
> The command registry already tracks each command's special features for
> use by the policy engine. Obvious idea: also track the features you
> want to pass to the predicate.
>
> Your series adds tracking for exactly the features you need:
>
> * Enumerate them in the schema with new pragma command-features
>
> Missing: documentation for the pragma.
>
> * Generate an extension QapiSpecialFeatureCustom of existing enum
> QapiSpecialFeature, which is not generated. The latter is in
> qapi/util.h, the former in ${prefix}qapi-init-commands.h.
>
> * Mark these features special for commands only, so existing registry
> machinery tracks them. Do *not* make them special elsewhere, because
> that would break things.
>
> Feels like a hack. Minor trap: if you use the same feature in
> multiple schemas, multiple generated headers will define the same enum
> constant, possibly with different values. If you manage to include
> the wrong header *and* the value differs there, you'll likely lose
> hair.
>
> * Missing: tests.
>
> I think we can avoid supplying most of the missing bits. The main QAPI
> schema uses five features: deprecated, unstable,
> allow-write-only-overlay, dynamic-auto-read-only, fdset. The QGA QAPI
> schema uses four, namely the four you add in this series. Why not track
> all features, and
Re: [PATCH] scripts/checkpatch: emit a warning if an imported file is touched
On Wed, Jul 17, 2024 at 11:37:52AM +0200, Stefano Garzarella wrote:
> If a file imported from Linux is touched, emit a warning and suggest
> using scripts/update-linux-headers.sh
>
> Signed-off-by: Stefano Garzarella
> ---
> scripts/checkpatch.pl | 14 --
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index ff373a7083..b0e8266fa2 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -1374,6 +1374,7 @@ sub process {
> my $in_header_lines = $file ? 0 : 1;
> my $in_commit_log = 0; #Scanning lines before patch
> my $reported_maintainer_file = 0;
> + my $reported_imported_file = 0;
> my $non_utf8_charset = 0;
>
> our @report = ();
> @@ -1673,8 +1674,17 @@ sub process {
> # ignore non-hunk lines and lines being removed
> next if (!$hunk_line || $line =~ /^-/);
>
> -# ignore files that are being periodically imported from Linux
> - next if ($realfile =~
> /^(linux-headers|include\/standard-headers)\//);
> +# ignore files that are being periodically imported from Linux and emit a
> warning
> + if ($realfile =~
> /^(linux-headers|include\/standard-headers)\//) {
> + if (!$reported_imported_file) {
> + $reported_imported_file = 1;
> + WARN("added, moved or deleted file(s) " .
> + "imported from Linux, are you using " .
> + "scripts/update-linux-headers.sh?\n" .
> + $herecurr);
This is a good hint, but can we add a further check that is a fatal error,
if the headers are changed in the same commit as non-header changes. When
importing headers, they should only ever be in a self-contained patch
with nothing else touched.
> + }
> + next;
> + }
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 v1 00/11] Convert avocado tests to normal Python unittests
On Wed, Jul 17, 2024 at 10:04:19AM +0200, Thomas Huth wrote: > On 16/07/2024 19.03, Thomas Huth wrote: > > On 16/07/2024 18.51, Daniel P. Berrangé wrote: > > > On Tue, Jul 16, 2024 at 01:26:03PM +0200, Thomas Huth wrote: > > ... > > > > So instead of trying to update the python-based test suite in QEMU > > > > to a newer version of Avocado, we should maybe try to better integrate > > > > it with the meson test runner instead. Indeed most tests work quite > > > > nicely without the Avocado framework already, as you can see with > > > > this patch series - it does not convert all tests, just a subset so > > > > far, but this already proves that many tests only need small modifi- > > > > cations to work without Avocado. > > ... > > > > Now if you want to try out these patches: Apply the patches, then > > > > recompile and then run: > > > > > > > > make check-functional > > > > > > > > You can also run single targets e.g. with: > > > > > > > > make check-functional-ppc > > > > > > > > You can also run the tests without any test runner now by > > > > setting the PYTHONPATH environment variable to the "python" folder > > > > of your source tree, and by specifying the build directory via > > > > QEMU_BUILD_ROOT (if autodetection fails) and by specifying the > > > > QEMU binary via QEMU_TEST_QEMU_BINARY. For example: > > > > > > > > export PYTHONPATH=$HOME/qemu/python > > > > export QEMU_TEST_QEMU_BINARY=qemu-system-x86_64 > > > > export PYTHONPATH=$HOME/qemu/build > > > > ~/qemu/tests/functional/test_virtio_version.py > > > > > > For the whole series as is > > > > > > Tested-by: Daniel P. Berrangé > > > > > > as it does what you claim it does here when I tried it. > > > > Thanks! > > > > > > The logs of the tests can be found in the build directory under > > > > tests/functional/ - console log and general logs will > > > > be put in separate files there. > > > > > > As an example, one dir name appears to be: > > > > > > __main__.MemAddrCheck.test_phybits_ok_pentium_pae > > > > > > I'd rather prefer it if the dir name matched the test script > > > file name - in this case test_mem_addr_space.py, as I don't > > > want to have to lookup which class names were defined inside > > > each test script. We could drop the "test_" prefix from the > > > method name too > > > > > > IOW, could we make this dir name be: > > > > > > test_mem_addr_space.phybits_ok_pentium_pae > > > > I can try to change that, indeed ... but the boilerplate code will > > increase a little bit, I guess, since I cannot simply rely on the > > unittest.id() function in that case anymore... > > After looking at this for a while, I think it's maybe best to ditch the idea > of making the .py files directly runnable and run the tests via a simple > pycotap runner instead. Then you get proper module names: I'd really not want to loose that. To me, eliminating the test harness entirely when debugging is the single biggest improvement of this new approach, especially when I want to 'strace' the test without extraneous processes. > $ pyvenv/bin/python3 -m pycotap test_virtio_version > TAP version 13 > ok 1 test_virtio_version.VirtioVersionCheck.test_conventional_devs > ok 2 test_virtio_version.VirtioVersionCheck.test_modern_only_devs > 1.. With the following change, you get the same output with direct execution, by making argv look the same as you'd get when running your pycotap example. diff --git a/tests/functional/qemu_test/__init__.py b/tests/functional/qemu_test/__init__.py index cc49fd4c94..3a3e65252d 100644 --- a/tests/functional/qemu_test/__init__.py +++ b/tests/functional/qemu_test/__init__.py @@ -266,7 +266,10 @@ def fetch_asset(self, url, asset_hash): def main(): tr = pycotap.TAPTestRunner(message_log = pycotap.LogMode.LogToError, test_output_log = pycotap.LogMode.LogToError) -unittest.main(testRunner = tr) +import sys +import os.path +path = os.path.basename(sys.argv[0])[:-3] +unittest.main(module = None, testRunner = tr, argv=["__dummy__", path]) class QemuSystemTest(QemuBaseTest): 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: [RFC PATCH 0/8] Convert avocado tests to normal Python unittests
On Tue, Jul 16, 2024 at 09:34:41PM +0200, Paolo Bonzini wrote: > Il mar 16 lug 2024, 20:10 Daniel P. Berrangé ha > scritto: > > > On Tue, Jul 16, 2024 at 08:03:54PM +0200, Paolo Bonzini wrote: > > > Il mar 16 lug 2024, 18:45 John Snow ha scritto: > > > > > > > My only ask is that we keep the tests running in the custom venv > > > > environment we set up at build time > > > > > > > > > > Yes, they do, however pytest should also be added to pythondeps.toml if > > we > > > go this way. > > > > Done in this patch: > > > > https://lists.nongnu.org/archive/html/qemu-devel/2024-07/msg03596.html > > > That adds pycotap, not pytest. Yep, the next posting of this series uses only pycotap, there's no need for using pytest at all, as meson can be the harness directly when we emit TAP format. > > Yep, that's the part that I am a bit more doubtful about. > > > > Pulling & caching VM images isn't much more than a URL download to > > a local file, not very complex in python. Assuming that's what you > > are refering to, then it is already done in this patch: > > > > https://lists.nongnu.org/archive/html/qemu-devel/2024-07/msg03598.html > > > I think there are also compressed assets that have to be passed through > gzip/xzip/zstd. I am worried that Thomas's patches do 90% of the job but > that is not a good estimation of what's left. 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 11/11] gitlab-ci: Add "check-functional" to the build tests
On Tue, Jul 16, 2024 at 01:26:14PM +0200, Thomas Huth wrote: > Now that we converted many tests from the "check-avocado" test suite > to the "check-functional" test suite, we should make sure that these > also get tested in the CI. I was thinking of suggesting we make 'check-functional' run both avocado and new tests, but that's pointless extra work if we fully convert all the avocado tests to the new approach in a short time frame. > > Signed-off-by: Thomas Huth > --- > .gitlab-ci.d/buildtest-template.yml | 3 +- > .gitlab-ci.d/buildtest.yml | 60 ++--- > 2 files changed, 32 insertions(+), 31 deletions(-) Reviewed-by: Daniel P. Berrangé 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 10/11] tests/functional: Convert the s390x avocado tests into standalone tests
On Tue, Jul 16, 2024 at 01:26:13PM +0200, Thomas Huth wrote:
> These tests use archive.lzma_uncompress() from the Avocado utils,
> so provide a small helper function for this, based on the
> standard lzma module from Python instead.
>
> And while we're at it, replace the MD5 hashes in the topology test
> with proper SHA256 hashes, since MD5 should not be used anymore
> nowadays.
>
> Signed-off-by: Thomas Huth
> ---
> MAINTAINERS | 4 +-
> tests/functional/meson.build | 6 ++
> tests/functional/qemu_test/utils.py | 7 ++
> .../test_s390x_ccw_virtio.py} | 32 -
> .../test_s390x_topology.py} | 70 +++
> 5 files changed, 52 insertions(+), 67 deletions(-)
> rename tests/{avocado/machine_s390_ccw_virtio.py =>
> functional/test_s390x_ccw_virtio.py} (95%)
> mode change 100644 => 100755
> rename tests/{avocado/s390_topology.py => functional/test_s390x_topology.py}
> (90%)
> mode change 100644 => 100755
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 025227954c..cbefb6fb81 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1739,7 +1739,7 @@ S: Supported
> F: hw/s390x/
> F: include/hw/s390x/
> F: configs/devices/s390x-softmmu/default.mak
> -F: tests/avocado/machine_s390_ccw_virtio.py
> +F: tests/functional/test_s390x_ccw_virtio.py
> T: git https://github.com/borntraeger/qemu.git s390-next
> L: qemu-s3...@nongnu.org
>
> @@ -1802,7 +1802,7 @@ F: hw/s390x/cpu-topology.c
> F: target/s390x/kvm/stsi-topology.c
> F: docs/devel/s390-cpu-topology.rst
> F: docs/system/s390x/cpu-topology.rst
> -F: tests/avocado/s390_topology.py
> +F: tests/functional/test_s390x_topology.py
>
> X86 Machines
>
> diff --git a/tests/functional/meson.build b/tests/functional/meson.build
> index c8fc9f6c07..f6de9af8a2 100644
> --- a/tests/functional/meson.build
> +++ b/tests/functional/meson.build
> @@ -13,6 +13,7 @@ endif
> test_timeouts = {
>'netdev_ethtool' : 180,
>'ppc_74xx' : 90,
> + 's390x_ccw_virtio' : 180,
> }
>
> tests_generic = [
> @@ -47,6 +48,11 @@ tests_ppc_thorough = [
>'ppc_bamboo',
> ]
>
> +tests_s390x_thorough = [
> + 's390x_ccw_virtio',
> + 's390x_topology',
> +]
> +
> tests_sparc64_thorough = [
>'sparc64_sun4u',
> ]
> diff --git a/tests/functional/qemu_test/utils.py
> b/tests/functional/qemu_test/utils.py
> index 4eb5e5d5e5..8115d9d1da 100644
> --- a/tests/functional/qemu_test/utils.py
> +++ b/tests/functional/qemu_test/utils.py
> @@ -8,6 +8,8 @@
> # This work is licensed under the terms of the GNU GPL, version 2 or
> # later. See the COPYING file in the top-level directory.
>
> +import lzma
> +import shutil
> import tarfile
>
> def archive_extract(archive, dest_dir, member=None):
> @@ -19,3 +21,8 @@ def archive_extract(archive, dest_dir, member=None):
> tf.extract(member=member, path=dest_dir)
> else:
> tf.extractall(path=dest_dir)
> +
> +def lzma_uncompress(xz_path, output_path):
> +with lzma.open(xz_path, 'rb') as lzma_in:
> +with open(output_path, 'wb') as raw_out:
> +shutil.copyfileobj(lzma_in, raw_out)
Avocado short-circuited if output_path already existed
for speed. Worth doing the same.
The inner 'with' should be surrounded by a try/except
that does os.remove(output_path) on error. Avocado
didn't have this safety net either, but we should add
it.
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 09/11] tests/functional: Set up logging
On Tue, Jul 16, 2024 at 01:26:12PM +0200, Thomas Huth wrote: > Create log files for each test separately, one file that contains > the basic logging and one that contains the console output. > > Signed-off-by: Thomas Huth > --- > tests/functional/qemu_test/__init__.py | 24 +++- > 1 file changed, 23 insertions(+), 1 deletion(-) Reviewed-by: Daniel P. Berrangé 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 08/11] tests/functional: Convert some avocado tests that needed avocado.utils.archive
On Tue, Jul 16, 2024 at 01:26:11PM +0200, Thomas Huth wrote:
> Instead of using the "archive" module from avocado.utils, switch
> these tests to use the new wrapper function that is based on the
> "tarfile" module instead.
>
> Signed-off-by: Thomas Huth
> ---
> MAINTAINERS | 6 ++---
> tests/functional/meson.build | 6 +
> .../test_arm_canona1100.py} | 21 +---
> .../test_ppc_bamboo.py} | 23 -
> .../test_sparc64_sun4u.py}| 25 +++
> 5 files changed, 46 insertions(+), 35 deletions(-)
> rename tests/{avocado/machine_arm_canona1100.py =>
> functional/test_arm_canona1100.py} (71%)
> mode change 100644 => 100755
> rename tests/{avocado/ppc_bamboo.py => functional/test_ppc_bamboo.py} (75%)
> mode change 100644 => 100755
> rename tests/{avocado/machine_sparc64_sun4u.py =>
> functional/test_sparc64_sun4u.py} (60%)
> mode change 100644 => 100755
Reviewed-by: Daniel P. Berrangé
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 07/11] tests/functional: Add a function for extracting files from an archive
On Tue, Jul 16, 2024 at 01:26:10PM +0200, Thomas Huth wrote: > Some Avocado-based tests use the "archive" module from avocado.utils > to extract files from an archive. To be able to use these tests > without Avocado, we have to provide our own function for extracting > files. Fortunately, there is already the tarfile module that will > provide us with this functionality, so let's just add a nice wrapper > function around that. > > Signed-off-by: Thomas Huth > --- > tests/functional/qemu_test/utils.py | 21 + > 1 file changed, 21 insertions(+) > create mode 100644 tests/functional/qemu_test/utils.py Reviewed-by: Daniel P. Berrangé 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 06/11] tests/functional: Convert some tests that download files via fetch_asset()
On Tue, Jul 16, 2024 at 01:26:09PM +0200, Thomas Huth wrote:
> Now that we've got a working fetch_asset() function, we can convert
> some Avocado tests that use this function for downloading their
> required files.
>
> Signed-off-by: Thomas Huth
> ---
> MAINTAINERS | 12 +++
> tests/functional/meson.build | 25 +++
> .../test_arm_n8x0.py} | 25 +++
> .../test_avr_mega2560.py} | 11 ---
> .../test_loongarch64_virt.py} | 16 ++
> .../test_mips64el_loongson3v.py} | 26 +++
> .../test_netdev_ethtool.py} | 32 ++-
> .../ppc_405.py => functional/test_ppc_405.py} | 19 ++-
> 8 files changed, 89 insertions(+), 77 deletions(-)
> rename tests/{avocado/machine_arm_n8x0.py => functional/test_arm_n8x0.py}
> (71%)
> mode change 100644 => 100755
> rename tests/{avocado/machine_avr6.py => functional/test_avr_mega2560.py}
> (90%)
> mode change 100644 => 100755
> rename tests/{avocado/machine_loongarch.py =>
> functional/test_loongarch64_virt.py} (89%)
> mode change 100644 => 100755
> rename tests/{avocado/machine_mips_loongson3v.py =>
> functional/test_mips64el_loongson3v.py} (55%)
> mode change 100644 => 100755
> rename tests/{avocado/netdev-ethtool.py =>
> functional/test_netdev_ethtool.py} (81%)
> mode change 100644 => 100755
> rename tests/{avocado/ppc_405.py => functional/test_ppc_405.py} (73%)
> mode change 100644 => 100755
Reviewed-by: Daniel P. Berrangé
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 05/11] tests/functional: Implement fetch_asset() method for downloading assets
On Tue, Jul 16, 2024 at 01:26:08PM +0200, Thomas Huth wrote:
> In the new python test framework, we cannot use the fetch_asset()
> function from Avocado anymore, so we have to provide our own
> implementation now instead. Thus add such a function based on the
> urllib python module for this purpose.
>
> Signed-off-by: Thomas Huth
> ---
> tests/functional/qemu_test/__init__.py | 36 ++
> 1 file changed, 36 insertions(+)
>
> diff --git a/tests/functional/qemu_test/__init__.py
> b/tests/functional/qemu_test/__init__.py
> index e73705d40a..77c3b14d34 100644
> --- a/tests/functional/qemu_test/__init__.py
> +++ b/tests/functional/qemu_test/__init__.py
> @@ -11,6 +11,8 @@
> # This work is licensed under the terms of the GNU GPL, version 2 or
> # later. See the COPYING file in the top-level directory.
>
> +import hashlib
> +import urllib.request
> import logging
> import os
> import pycotap
> @@ -23,6 +25,7 @@
> import unittest
>
> from pathlib import Path
> +from shutil import copyfileobj
> from qemu.machine import QEMUMachine
> from qemu.utils import (get_info_usernet_hostfwd_port, kvm_available,
> tcg_available)
> @@ -215,6 +218,39 @@ def setUp(self, bin_prefix):
> if not os.path.exists(self.workdir):
> os.makedirs(self.workdir)
>
> +def check_hash(self, file_name, expected_hash):
> +if not expected_hash:
> +return True
> +if len(expected_hash) == 40:
> +sum_prog = 'sha1sum'
> +elif len(expected_hash) == 64:
> +sum_prog = 'sha256sum'
> +elif len(expected_hash) == 128:
> +sum_prog = 'sha512sum'
> +else:
> +raise Exception("unknown hash type")
> +checksum = subprocess.check_output([sum_prog, file_name]).split()[0]
> +return expected_hash == checksum.decode("utf-8")
> +
> +def fetch_asset(self, url, asset_hash):
> +cache_dir = os.path.expanduser("~/.cache/qemu/download")
> +if not os.path.exists(cache_dir):
> +os.makedirs(cache_dir)
> +fname = os.path.join(cache_dir,
> + hashlib.sha256(url.encode("utf-8")).hexdigest())
> +if os.path.exists(fname) and self.check_hash(fname, asset_hash):
> +return fname
> +self.log.info("Downloading %s to %s...", url, fname)
> +dl_fname = fname + ".download"
> +with urllib.request.urlopen(url) as src:
> +with open(dl_fname, "wb+") as dst:
> +copyfileobj(src, dst)
For cleanliness we should probably wrap this and delete the file on
error, eg
try:
with open(dl_fname,) as dst
except:
os.remove(dl_fname)
raise
> +if not self.check_hash(dl_fname, asset_hash):
> +os.remove(dl_fname)
> +raise Exception("Hash of " + url + " does not match")
> +os.rename(dl_fname, fname)
> +return fname
> +
> def main():
> tr = pycotap.TAPTestRunner(message_log = pycotap.LogMode.LogToError,
> test_output_log =
> pycotap.LogMode.LogToError)
> --
> 2.45.2
>
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 03/11] tests/functional: Convert avocado tests that just need a small adjustment
On Tue, Jul 16, 2024 at 01:26:06PM +0200, Thomas Huth wrote:
> These simple tests can be converted to stand-alone tests quite easily,
> e.g. by just setting the machine to 'none' now manually or by adding
> "-cpu" command line parameters, since we don't support the corresponding
> avocado tags in the new python test framework.
>
> Signed-off-by: Thomas Huth
> ---
> .../test_info_usernet.py} | 11 ++-
> .../test_ppc_74xx.py} | 74 ---
> .../version.py => functional/test_version.py} | 13 ++--
> 3 files changed, 47 insertions(+), 51 deletions(-)
> rename tests/{avocado/info_usernet.py => functional/test_info_usernet.py}
> (87%)
> mode change 100644 => 100755
> rename tests/{avocado/ppc_74xx.py => functional/test_ppc_74xx.py} (74%)
> mode change 100644 => 100755
> rename tests/{avocado/version.py => functional/test_version.py} (78%)
> mode change 100644 => 100755
Reviewed-by: Daniel P. Berrangé
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 02/11] tests/functional: Convert simple avocado tests into standalone python tests
On Tue, Jul 16, 2024 at 01:26:05PM +0200, Thomas Huth wrote:
> These test are rather simple and don't need any modifications apart
> from adjusting the "from avocado_qemu" line. To ease debugging, make
> the files executable and add a shebang line and Python '__main__'
> handling, too, so that these tests can now be run by executing them
> directly.
>
> Signed-off-by: Thomas Huth
> ---
> .../test_cpu_queries.py} | 7 ++-
> .../test_empty_cpu_model.py} | 7 ++-
> .../test_mem_addr_space.py} | 53 +++
> .../test_pc_cpu_hotplug_props.py} | 11 ++--
> .../test_virtio_version.py} | 8 +--
> 5 files changed, 29 insertions(+), 57 deletions(-)
> rename tests/{avocado/cpu_queries.py => functional/test_cpu_queries.py} (89%)
> mode change 100644 => 100755
> rename tests/{avocado/empty_cpu_model.py =>
> functional/test_empty_cpu_model.py} (84%)
> mode change 100644 => 100755
> rename tests/{avocado/mem-addr-space-check.py =>
> functional/test_mem_addr_space.py} (92%)
> mode change 100644 => 100755
> rename tests/{avocado/pc_cpu_hotplug_props.py =>
> functional/test_pc_cpu_hotplug_props.py} (90%)
> mode change 100644 => 100755
> rename tests/{avocado/virtio_version.py =>
> functional/test_virtio_version.py} (98%)
> mode change 100644 => 100755
>
> diff --git a/tests/avocado/mem-addr-space-check.py
> b/tests/functional/test_mem_addr_space.py
> old mode 100644
> new mode 100755
> similarity index 92%
> rename from tests/avocado/mem-addr-space-check.py
> rename to tests/functional/test_mem_addr_space.py
> index 85541ea051..bb0cf062ca
> --- a/tests/avocado/mem-addr-space-check.py
> +++ b/tests/functional/test_mem_addr_space.py
> @@ -1,3 +1,5 @@
> +#!/usr/bin/env python3
> +#
> # Check for crash when using memory beyond the available guest processor
> # address space.
> #
> @@ -8,8 +10,7 @@
> #
> # SPDX-License-Identifier: GPL-2.0-or-later
>
> -from avocado_qemu import QemuSystemTest
> -import signal
Nit-pick - cleanup of an unrelated existing bug - 'signal' wasn't
used. Suggest doing it in a separate patch
Aside from that:
Reviewed-by: Daniel P. Berrangé
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: [RFC PATCH 0/8] Convert avocado tests to normal Python unittests
On Tue, Jul 16, 2024 at 08:03:54PM +0200, Paolo Bonzini wrote: > Il mar 16 lug 2024, 18:45 John Snow ha scritto: > > > My only ask is that we keep the tests running in the custom venv > > environment we set up at build time > > > > Yes, they do, however pytest should also be added to pythondeps.toml if we > go this way. Done in this patch: https://lists.nongnu.org/archive/html/qemu-devel/2024-07/msg03596.html > > > If we move to pytest, it's possible we can eliminate that funkiness, which > > would be a win. > > > > There is the pycotap dependency to produce TAP from pytest, but that's > probably something small enough to be vendored. And also it depends on what > the dependencies would be for the assets framework. > > > I'm also not so sure about recreating all of the framework that pulls vm > > images on demand, that sounds like it'd be a lot of work, but maybe I'm > > wrong about that. > > > > Yep, that's the part that I am a bit more doubtful about. Pulling & caching VM images isn't much more than a URL download to a local file, not very complex in python. Assuming that's what you are refering to, then it is already done in this patch: https://lists.nongnu.org/archive/html/qemu-devel/2024-07/msg03598.html 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 04/11] tests/functional: Add python-based tests to the meson build system
On Tue, Jul 16, 2024 at 01:26:07PM +0200, Thomas Huth wrote:
> Integrate the new python-based test framework with the meson build
> system. Since these tests now require the pycotap module, make
> sure that it gets installed in the venv.
>
> The changes to the meson.build files are partly based on an earlier
> patch by Ani Sinha (but heavily modified by Thomas Huth e.g. to use
> pycotap for running the tests instead).
>
> Signed-off-by: Thomas Huth
> ---
> pythondeps.toml | 3 +-
> tests/Makefile.include | 18 -
> tests/functional/meson.build | 75
> tests/meson.build| 1 +
> 4 files changed, 95 insertions(+), 2 deletions(-)
> create mode 100644 tests/functional/meson.build
Strictly speaking this patch probably ought to be #2, otherwise we
have a bisection window where we've converted some tests but not
run them.
>
> diff --git a/pythondeps.toml b/pythondeps.toml
> index f6e590fdd8..c018b4d74a 100644
> --- a/pythondeps.toml
> +++ b/pythondeps.toml
> @@ -26,9 +26,10 @@ meson = { accepted = ">=1.1.0", installed = "1.2.3",
> canary = "meson" }
> sphinx = { accepted = ">=3.4.3", installed = "5.3.0", canary =
> "sphinx-build" }
> sphinx_rtd_theme = { accepted = ">=0.5", installed = "1.1.1" }
>
> -[avocado]
> +[tests]
> # Note that qemu.git/python/ is always implicitly installed.
> # Prefer an LTS version when updating the accepted versions of
> # avocado-framework, for example right now the limit is 92.x.
> avocado-framework = { accepted = "(>=88.1, <93.0)", installed = "88.1",
> canary = "avocado" }
> pycdlib = { accepted = ">=1.11.0" }
> +pycotap = { accepted = ">=1.1.0" }
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index d39d5dd6a4..2bdf607977 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -9,6 +9,8 @@ check-help:
> @echo "Individual test suites:"
> @echo " $(MAKE) check-qtest-TARGET Run qtest tests for given target"
> @echo " $(MAKE) check-qtestRun qtest tests"
> + @echo " $(MAKE) check-functional Run python-based functional
> tests"
> + @echo " $(MAKE) check-functional-TARG Run functional tests for a given
> target"
We could increase whitespace by 2 to fit TARGET, or shorten all
cases to TGT ?
> @echo " $(MAKE) check-unit Run qobject tests"
> @echo " $(MAKE) check-qapi-schema Run QAPI schema tests"
> @echo " $(MAKE) check-blockRun block tests"
> @@ -111,7 +113,7 @@ quiet-venv-pip = $(quiet-@)$(call quiet-command-run, \
>
The above is a minor non-functional point though so
Reviewed-by: Daniel P. Berrangé
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 v1 00/11] Convert avocado tests to normal Python unittests
On Tue, Jul 16, 2024 at 01:26:03PM +0200, Thomas Huth wrote: > The Avocado v88 that we use in QEMU is already on a life support > system: It is not supported by upstream anymore, and with the latest > versions of Python, it won't work anymore since it depends on the > "imp" module that has been removed in Python 3.12. > > There have been several attempts to update the test suite in QEMU > to a newer version of Avocado, but so far no attempt has successfully > been merged yet. > > Additionally, the whole "make check" test suite in QEMU is using the > meson test runner nowadays, so running the python-based tests via the > Avocodo test runner looks and feels quite like an oddball, requiring > the users to deal with the knowledge of multiple test runners in > parallel (e.g. the timeout settings work completely differently). > > So instead of trying to update the python-based test suite in QEMU > to a newer version of Avocado, we should maybe try to better integrate > it with the meson test runner instead. Indeed most tests work quite > nicely without the Avocado framework already, as you can see with > this patch series - it does not convert all tests, just a subset so > far, but this already proves that many tests only need small modifi- > cations to work without Avocado. > > Only tests that use the LinuxTest / LinuxDistro and LinuxSSHMixIn > classes (e.g. based on cloud-init images or using SSH) really depend > on the Avocado framework, so we'd need a solution for those if we > want to continue using them. One solution might be to simply use the > required functions from avocado.utils for these tests, and still run > them via the meson test runner instead, but that needs some further > investigation that will be done later. > > > Now if you want to try out these patches: Apply the patches, then > recompile and then run: > > make check-functional > > You can also run single targets e.g. with: > > make check-functional-ppc > > You can also run the tests without any test runner now by > setting the PYTHONPATH environment variable to the "python" folder > of your source tree, and by specifying the build directory via > QEMU_BUILD_ROOT (if autodetection fails) and by specifying the > QEMU binary via QEMU_TEST_QEMU_BINARY. For example: > > export PYTHONPATH=$HOME/qemu/python > export QEMU_TEST_QEMU_BINARY=qemu-system-x86_64 > export PYTHONPATH=$HOME/qemu/build > ~/qemu/tests/functional/test_virtio_version.py For the whole series as is Tested-by: Daniel P. Berrangé as it does what you claim it does here when I tried it. > The logs of the tests can be found in the build directory under > tests/functional/ - console log and general logs will > be put in separate files there. As an example, one dir name appears to be: __main__.MemAddrCheck.test_phybits_ok_pentium_pae I'd rather prefer it if the dir name matched the test script file name - in this case test_mem_addr_space.py, as I don't want to have to lookup which class names were defined inside each test script. We could drop the "test_" prefix from the method name too IOW, could we make this dir name be: test_mem_addr_space.phybits_ok_pentium_pae 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 v4 1/7] util: Introduce qemu_get_runtime_dir()
On Tue, Jul 16, 2024 at 07:52:00PM +0900, Akihiko Odaki wrote:
> On 2024/07/16 18:53, Daniel P. Berrangé wrote:
> > On Tue, Jul 16, 2024 at 04:27:31PM +0900, Akihiko Odaki wrote:
> > > qemu_get_runtime_dir() returns a dynamically allocated directory path
> > > that is appropriate for storing runtime files. It corresponds to "run"
> > > directory in Unix.
> > >
> > > With a tree-wide search, it was found that there are several cases
> > > where such a functionality is implemented so let's have one as a common
> > > utlity function.
> > >
> > > A notable feature of qemu_get_runtime_dir() is that it uses
> > > $XDG_RUNTIME_DIR if available. While the function is often called by
> > > executables which requires root privileges, it is still possible that
> > > they are called from a user without privilege to write the system
> > > runtime directory. In fact, I decided to write this patch when I ran
> > > virtiofsd in a Linux namespace created by a normal user and realized
> > > it tries to write the system runtime directory, not writable in this
> > > case. $XDG_RUNTIME_DIR should provide a writable directory in such
> > > cases.
> > >
> > > This function does not use qemu_get_local_state_dir() or its logic
> > > for Windows. Actually the implementation of qemu_get_local_state_dir()
> > > for Windows seems not right as it calls g_get_system_data_dirs(),
> > > which refers to $XDG_DATA_DIRS. In Unix terminology, it is basically
> > > "/usr/share", not "/var", which qemu_get_local_state_dir() is intended
> > > to provide. Instead, this function try to use the following in order:
> > > - $XDG_RUNTIME_DIR
> > > - LocalAppData folder
> > > - get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR "/run")
> > >
> > > This function does not use g_get_user_runtime_dir() either as it
> > > falls back to g_get_user_cache_dir() when $XDG_DATA_DIRS is not
> > > available. In the case, we rather use:
> > > get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR "/run")
> > >
> > > Signed-off-by: Akihiko Odaki
> > > Message-Id: <20230921075425.16738-2-akihiko.od...@daynix.com>
> > > ---
> > > include/qemu/osdep.h | 12
> > > util/oslib-posix.c | 11 +++
> > > util/oslib-win32.c | 26 ++
> > > 3 files changed, 49 insertions(+)
> > >
> > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > > index 191916f38e6d..fe8609fc1375 100644
> > > --- a/include/qemu/osdep.h
> > > +++ b/include/qemu/osdep.h
> > > @@ -670,6 +670,18 @@ void qemu_set_cloexec(int fd);
> > >*/
> > > char *qemu_get_local_state_dir(void);
> > > +/**
> > > + * qemu_get_runtime_dir:
> > > + *
> > > + * Return a dynamically allocated directory path that is appropriate for
> > > storing
> > > + * runtime files. It corresponds to "run" directory in Unix, and uses
> > > + * $XDG_RUNTIME_DIR if available.
> > > + *
> > > + * The caller is responsible for releasing the value returned with
> > > g_free()
> > > + * after use.
> > > + */
> > > +char *qemu_get_runtime_dir(void);
> > > +
> > > /**
> > >* qemu_getauxval:
> > >* @type: the auxiliary vector key to lookup
> > > diff --git a/util/oslib-posix.c b/util/oslib-posix.c
> > > index e76441695bdc..9599509a9aa7 100644
> > > --- a/util/oslib-posix.c
> > > +++ b/util/oslib-posix.c
> > > @@ -278,6 +278,17 @@ qemu_get_local_state_dir(void)
> > > return get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR);
> > > }
> > > +char *
> > > +qemu_get_runtime_dir(void)
> > > +{
> > > +char *env = getenv("XDG_RUNTIME_DIR");
> > > +if (env) {
> > > +return g_strdup(env);
> > > +}
> > > +
> > > +return get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR "/run");
> > > +}
> >
> > I'm not convinced this is the correct logic to be following.
> >
> > In the cover letter you mention not using g_get_user_runtime_dir()
> > because it falls back to XDG_CACHE_HOME, and we need to fallback
> > to LOCALSTATEDIR/run. This is not right for normal users though,
> > where falling back to LOCALSTATEDIR/run is always wrong, as it
> > won't be writable - the g_get_user_runtime_dir() fallbac
Re: [PATCH v4 0/7] util: Introduce qemu_get_runtime_dir()
On Tue, Jul 16, 2024 at 11:06:57AM +0300, Michael Tokarev wrote: > 16.07.2024 10:27, Akihiko Odaki wrote: > > qemu_get_runtime_dir() returns a dynamically allocated directory path > > that is appropriate for storing runtime files. It corresponds to "run" > > directory in Unix. > > Since runtime dir is always used with a filename within, how about > > char *qemu_get_runtime_path(const char *filename) > > which return RUNTIME_DIR/filename instead of just RUNTIME_DIR ? Yeah, I agree, every single caller of the function goes on to call g_build_filename with the result. The helper should just be building the filename itself. 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 v4 1/7] util: Introduce qemu_get_runtime_dir()
On Tue, Jul 16, 2024 at 04:27:31PM +0900, Akihiko Odaki wrote:
> qemu_get_runtime_dir() returns a dynamically allocated directory path
> that is appropriate for storing runtime files. It corresponds to "run"
> directory in Unix.
>
> With a tree-wide search, it was found that there are several cases
> where such a functionality is implemented so let's have one as a common
> utlity function.
>
> A notable feature of qemu_get_runtime_dir() is that it uses
> $XDG_RUNTIME_DIR if available. While the function is often called by
> executables which requires root privileges, it is still possible that
> they are called from a user without privilege to write the system
> runtime directory. In fact, I decided to write this patch when I ran
> virtiofsd in a Linux namespace created by a normal user and realized
> it tries to write the system runtime directory, not writable in this
> case. $XDG_RUNTIME_DIR should provide a writable directory in such
> cases.
>
> This function does not use qemu_get_local_state_dir() or its logic
> for Windows. Actually the implementation of qemu_get_local_state_dir()
> for Windows seems not right as it calls g_get_system_data_dirs(),
> which refers to $XDG_DATA_DIRS. In Unix terminology, it is basically
> "/usr/share", not "/var", which qemu_get_local_state_dir() is intended
> to provide. Instead, this function try to use the following in order:
> - $XDG_RUNTIME_DIR
> - LocalAppData folder
> - get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR "/run")
>
> This function does not use g_get_user_runtime_dir() either as it
> falls back to g_get_user_cache_dir() when $XDG_DATA_DIRS is not
> available. In the case, we rather use:
> get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR "/run")
>
> Signed-off-by: Akihiko Odaki
> Message-Id: <20230921075425.16738-2-akihiko.od...@daynix.com>
> ---
> include/qemu/osdep.h | 12
> util/oslib-posix.c | 11 +++
> util/oslib-win32.c | 26 ++
> 3 files changed, 49 insertions(+)
>
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 191916f38e6d..fe8609fc1375 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -670,6 +670,18 @@ void qemu_set_cloexec(int fd);
> */
> char *qemu_get_local_state_dir(void);
>
> +/**
> + * qemu_get_runtime_dir:
> + *
> + * Return a dynamically allocated directory path that is appropriate for
> storing
> + * runtime files. It corresponds to "run" directory in Unix, and uses
> + * $XDG_RUNTIME_DIR if available.
> + *
> + * The caller is responsible for releasing the value returned with g_free()
> + * after use.
> + */
> +char *qemu_get_runtime_dir(void);
> +
> /**
> * qemu_getauxval:
> * @type: the auxiliary vector key to lookup
> diff --git a/util/oslib-posix.c b/util/oslib-posix.c
> index e76441695bdc..9599509a9aa7 100644
> --- a/util/oslib-posix.c
> +++ b/util/oslib-posix.c
> @@ -278,6 +278,17 @@ qemu_get_local_state_dir(void)
> return get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR);
> }
>
> +char *
> +qemu_get_runtime_dir(void)
> +{
> +char *env = getenv("XDG_RUNTIME_DIR");
> +if (env) {
> +return g_strdup(env);
> +}
> +
> +return get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR "/run");
> +}
I'm not convinced this is the correct logic to be following.
In the cover letter you mention not using g_get_user_runtime_dir()
because it falls back to XDG_CACHE_HOME, and we need to fallback
to LOCALSTATEDIR/run. This is not right for normal users though,
where falling back to LOCALSTATEDIR/run is always wrong, as it
won't be writable - the g_get_user_runtime_dir() fallback is
desirable for non-root users.
IMHO we should be doing something more like this
#ifndef WIN32
if (geteuid() == 0) {
return get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR "/run");
} else {
#endif
return g_get_user_runtime_dir();
#ifndef WIN32
}
#endif
> +
> void qemu_set_tty_echo(int fd, bool echo)
> {
> struct termios tty;
> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> index b623830d624f..8c5a02ee881d 100644
> --- a/util/oslib-win32.c
> +++ b/util/oslib-win32.c
> @@ -27,6 +27,8 @@
> */
>
> #include "qemu/osdep.h"
> +#include
> +#include
> #include
> #include "qapi/error.h"
> #include "qemu/main-loop.h"
> @@ -237,6 +239,30 @@ qemu_get_local_state_dir(void)
> return g_strdup(data_dirs[0]);
> }
>
> +char *
> +qemu_get_runtime_dir(void)
> +{
> +size_t size = GetEnvironmentVariableA("XDG_RUNTIME_DIR", NULL, 0);
> +if (size) {
> +char *env = g_malloc(size);
> +GetEnvironmentVariableA("XDG_RUNTIME_DIR", env, size);
> +return env;
> +}
> +
> +PWSTR wpath;
> +const wchar_t *cwpath;
> +if (!SHGetKnownFolderPath(_LocalAppData, KF_FLAG_DEFAULT, NULL,
> )) {
> +cwpath = wpath;
> +size = wcsrtombs(NULL, , 0, &(mbstate_t){0}) + 1;
> +char *path = g_malloc(size);
> +wcsrtombs(path, , size, &(mbstate_t){0});
> +
Re: [PATCH] meson: Update meson-buildoptions.sh
CC qemu-trivial On Mon, Jul 15, 2024 at 09:00:51PM +0800, Zhao Liu wrote: > Hi Daniel, > > On Fri, Jul 05, 2024 at 10:20:27AM +0100, Daniel P. Berrangé wrote: > > Date: Fri, 5 Jul 2024 10:20:27 +0100 > > From: "Daniel P. Berrangé" > > Subject: Re: [PATCH] meson: Update meson-buildoptions.sh > > > > On Fri, Jul 05, 2024 at 01:49:03PM +0800, Zhao Liu wrote: > > > Update meson-buildoptions.sh to stay in sync with meson_options.txt. > > > > > > Signed-off-by: Zhao Liu > > > --- > > > scripts/meson-buildoptions.sh | 14 +++--- > > > 1 file changed, 7 insertions(+), 7 deletions(-) > > > > Reviewed-by: Daniel P. Berrangé > > > > Thanks! > > BTW, could you please help merge this change? Because recently this > script has been "polluting" my git staging workspace during > development. > > Regards, > Zhao > 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 v3 2/5] qdev-properties: Add DEFINE_PROP_ON_OFF_AUTO_BIT64()
On Sun, Jul 14, 2024 at 02:11:02PM +0900, Akihiko Odaki wrote: > DEFINE_PROP_ON_OFF_AUTO_BIT64() corresponds to DEFINE_PROP_ON_OFF_AUTO() > as DEFINE_PROP_BIT64() corresponds to DEFINE_PROP_BOOL(). The difference > is that DEFINE_PROP_ON_OFF_AUTO_BIT64() exposes OnOffAuto instead of > bool. IMHO this shouldn't be implemented in terms of On/Off auto, as it is misleadingly accepting much more than PROP_ON_OFF accepts. Rather it should be just DEFINE_PROP_AUTO_BIT64, implemented in terms of 'bool', with an extra 'auto' value. 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 v3 1/5] qdev-properties: Accept bool for OnOffAuto
On Sun, Jul 14, 2024 at 02:11:01PM +0900, Akihiko Odaki wrote:
> Accept bool literals for OnOffAuto properties for consistency with bool
> properties.
>
> Signed-off-by: Akihiko Odaki
> ---
> hw/core/qdev-properties.c | 17 -
> 1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index 86a583574dd0..f0a270bb4f61 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -491,6 +491,21 @@ const PropertyInfo qdev_prop_string = {
> .set = set_string,
> };
>
> +static void set_on_off_auto(Object *obj, Visitor *v, const char *name,
> +void *opaque, Error **errp)
> +{
> +Property *prop = opaque;
> +int *ptr = object_field_prop_ptr(obj, prop);
> +bool value;
> +
> +if (visit_type_bool(v, name, , NULL)) {
> +*ptr = value ? ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
> +return;
> +}
> +
> +qdev_propinfo_set_enum(obj, v, name, opaque, errp);
> +}
IMHO this is highly undesirable. It is adding redundant new syntax
across countless places in QEMU that use OnOffAuto.
> +
> /* --- on/off/auto --- */
>
> const PropertyInfo qdev_prop_on_off_auto = {
> @@ -498,7 +513,7 @@ const PropertyInfo qdev_prop_on_off_auto = {
> .description = "on/off/auto",
> .enum_table = _lookup,
> .get = qdev_propinfo_get_enum,
> -.set = qdev_propinfo_set_enum,
> +.set = set_on_off_auto,
> .set_default_value = qdev_propinfo_set_default_value_enum,
> };
>
>
> --
> 2.45.2
>
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 00/14] Improve mechanism for configuring allowed commands
On Mon, Jul 15, 2024 at 11:52:10AM +0200, Markus Armbruster wrote: > Hi Daniel, got a public branch I could pull? This particular v1 posting: https://gitlab.com/berrange/qemu/-/tags/qga-features-v1 Or latest git master rebase https://gitlab.com/berrange/qemu/-/tree/qga-features NB, this is on top of my other qga patch series changing build time conditions, so the above branch includes this branch: https://gitlab.com/berrange/qemu/-/tree/qga-conditions 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 v2] osdep: add a qemu_close_all_open_fd() helper
On Tue, Jun 18, 2024 at 01:17:03PM +0200, Clément Léger wrote:
> Since commit 03e471c41d8b ("qemu_init: increase NOFILE soft limit on
> POSIX"), the maximum number of file descriptors that can be opened are
> raised to nofile.rlim_max. On recent debian distro, this yield a maximum
> of 1073741816 file descriptors. Now, when forking to start
> qemu-bridge-helper, this actually calls close() on the full possible file
> descriptor range (more precisely [3 - sysconf(_SC_OPEN_MAX)]) which
> takes a considerable amount of time. In order to reduce that time,
> factorize existing code to close all open files descriptors in a new
> qemu_close_all_open_fd() function. This function uses various methods
> to close all the open file descriptors ranging from the most efficient
> one to the least one. It also accepts an ordered array of file
> descriptors that should not be closed since this is required by the
> callers that calls it after forking.
>
> Signed-off-by: Clément Léger
>
>
>
> v2:
> - Factorize async_teardown.c close_fds implementation as well as tap.c ones
> - Apply checkpatch
> - v1:
> https://lore.kernel.org/qemu-devel/20240617162520.4045016-1-cle...@rivosinc.com/
>
> ---
> include/qemu/osdep.h| 8 +++
> net/tap.c | 31 ++-
> system/async-teardown.c | 37 +
> util/osdep.c| 115
> 4 files changed, 141 insertions(+), 50 deletions(-)
>
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index f61edcfdc2..9369a97d3d 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -755,6 +755,14 @@ static inline void qemu_reset_optind(void)
>
> int qemu_fdatasync(int fd);
>
> +/**
> + * Close all open file descriptors except the ones supplied in the @skip
> array
> + *
> + * @skip: ordered array of distinct file descriptors that should not be
> closed
> + * @nskip: number of entries in the @skip array.
> + */
> +void qemu_close_all_open_fd(const int *skip, unsigned int nskip);
> +
> /**
> * Sync changes made to the memory mapped file back to the backing
> * storage. For POSIX compliant systems this will fallback
> diff --git a/net/tap.c b/net/tap.c
> index 51f7aec39d..6fc3939078 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -385,6 +385,21 @@ static TAPState *net_tap_fd_init(NetClientState *peer,
> return s;
> }
>
> +static void close_all_fds_after_fork(int excluded_fd)
> +{
> +const int skip_fd[] = {0, 1, 2, 3, excluded_fd};
> +unsigned int nskip = ARRAY_SIZE(skip_fd);
> +
> +/*
> + * skip_fd must be an ordered array of distinct fds, exclude
> + * excluded_fd if already included in the [0 - 3] range
> + */
> +if (excluded_fd <= 3) {
> +nskip--;
> +}
> +qemu_close_all_open_fd(skip_fd, nskip);
> +}
This is slightly over-indented - 4 space is QEMU normal style.
> diff --git a/util/osdep.c b/util/osdep.c
> index 5d23bbfbec..f3710710e3 100644
> --- a/util/osdep.c
> +++ b/util/osdep.c
> @@ -625,3 +625,118 @@ int qemu_fdatasync(int fd)
> return fsync(fd);
> #endif
> }
> +
> +#ifdef CONFIG_LINUX
> +static bool qemu_close_all_open_fd_proc(const int *skip, unsigned int nskip)
> +{
> +struct dirent *de;
> +int fd, dfd;
> +bool close_fd;
> +DIR *dir;
> +int i;
> +
> +dir = opendir("/proc/self/fd");
> +if (!dir) {
> +/* If /proc is not mounted, there is nothing that can be done. */
> +return false;
> +}
> +/* Avoid closing the directory. */
> +dfd = dirfd(dir);
> +
> +for (de = readdir(dir); de; de = readdir(dir)) {
Don't we need
if (de->d_name[0] == '.') {
continue;
}
otherwise atoi will fail and we'll try to close(0) multiple
times.
> +fd = atoi(de->d_name);
> +close_fd = true;
> +if (fd == dfd) {
> +close_fd = false;
> +} else {
> +for (i = 0; i < nskip; i++) {
> +if (fd == skip[i]) {
> +close_fd = false;
> +break;
> +}
> +}
> +}
> +if (close_fd) {
> +close(fd);
> +}
> +}
> +closedir(dir);
> +
> +return true;
> +}
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: [RFC PATCH 0/8] Convert avocado tests to normal Python unittests
On Fri, Jul 12, 2024 at 03:25:23PM +0100, Alex Bennée wrote: > Daniel P. Berrangé writes: > > > On Thu, Jul 11, 2024 at 07:44:39PM +0200, Thomas Huth wrote: > >> On 11/07/2024 16.39, Fabiano Rosas wrote: > >> > Thomas Huth writes: > >> ... > >> > > Things that need further attention though: > >> > > > >> > > - All tests that use the LinuxTest / LinuxDistro classes (e.g. based > >> > >on cloud-init images) really depend on the Avocado framework, > >> > >thus we'd need a solution for those if we want to continue with > >> > >this approach > >> > > > >> > > - Same for all tests that require the LinuxSSHMixIn class - we'd > >> > >need to provide a solution for ssh-based tests, too. > >> > > >> > These two seem to be dependent mostly avocado/utils only. Those could > >> > still be used without the whole framework, no? Say we keep importing > >> > avocado.utils, but run everything from meson, would that make sense? > >> > >> Yes ... maybe ... I can give it a try to see whether that works. > > > > We only import about 8 modules from avocado.utils. There are probably a > > few more indirectly used, but worst case we just clone the bits we need > > into the QEMU tree. > > Is Avocado still actively developed? I thought you guys used it quite > widely within RedHat? Yes it is active: https://github.com/avocado-framework/avocado/commits/master/ 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] meson.build: fix libgcrypt detection on system without libgcrypt-config
On Sat, Jul 06, 2024 at 08:12:26PM +, Yao Zi wrote:
> libgcrypt starts providing correct pkg-config configuration and dropping
> libgcrypt-config since 1.11.0. So use auto method for detection of
> libgcrypt, in which meson will try both pkg-config and libgcrypt-config.
The pkg-config file seems to be provided since 1.9 in fact.
Where do you see that ligcrypt-config is dropped ? It still
exists in the gcrypt git repo and in Fedora 1.11.0 packages.
>
> This fixes build failure when libgcrypt is enabled on a system without
> ligcrypt-config. Auto method for libgcrypt is supported by meson since
> 0.49.0, which is higher than the version qemu requires.
>
> Signed-off-by: Yao Zi
> ---
> meson.build | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/meson.build b/meson.build
> index 6a93da48e1..1b71824548 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -1695,7 +1695,6 @@ endif
> if not gnutls_crypto.found()
>if (not get_option('gcrypt').auto() or have_system) and not
> get_option('nettle').enabled()
> gcrypt = dependency('libgcrypt', version: '>=1.8',
> -method: 'config-tool',
> required: get_option('gcrypt'))
> # Debian has removed -lgpg-error from libgcrypt-config
> # as it "spreads unnecessary dependencies" which in
Despite the misleading commit message the change is good and I'll
queue it.
Reviewed-by: Daniel P. Berrangé
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] docs/devel: Add introduction to LUKS volume with detached header
On Tue, Feb 20, 2024 at 12:04:42AM +0800, Hyman Huang wrote: > Signed-off-by: Hyman Huang > --- > MAINTAINERS | 1 + > docs/devel/luks-detached-header.rst | 182 > 2 files changed, 183 insertions(+) > create mode 100644 docs/devel/luks-detached-header.rst > > diff --git a/MAINTAINERS b/MAINTAINERS > index a24c2b51b6..e8b03032ab 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -3422,6 +3422,7 @@ Detached LUKS header > M: Hyman Huang > S: Maintained > F: tests/qemu-iotests/tests/luks-detached-header > +F: docs/devel/luks-detached-header.rst > > D-Bus > M: Marc-André Lureau > diff --git a/docs/devel/luks-detached-header.rst > b/docs/devel/luks-detached-header.rst > new file mode 100644 > index 00..15e9ccde1d > --- /dev/null > +++ b/docs/devel/luks-detached-header.rst The new file neeeds adding to an index. We don't have anywhere for crypto yet, so I'm starting a crypto section thus: diff --git a/docs/devel/crypto.rst b/docs/devel/crypto.rst new file mode 100644 index 00..39b1c910e7 --- /dev/null +++ b/docs/devel/crypto.rst @@ -0,0 +1,10 @@ +.. _crypto-ref: + + +Cryptography in QEMU + + +.. toctree:: + :maxdepth: 2 + + luks-detached-header diff --git a/docs/devel/index-internals.rst b/docs/devel/index-internals.rst index 5636e9cf1d..4ac7725d72 100644 --- a/docs/devel/index-internals.rst +++ b/docs/devel/index-internals.rst @@ -20,3 +20,4 @@ Details about QEMU's various subsystems including how to add features to them. vfio-iommufd writing-monitor-commands virtio-backends + crypto > @@ -0,0 +1,182 @@ > + > +LUKS volume with detached header > + > + > +Introduction > + > + > +This document gives an overview of the design of LUKS volume with detached > +header and how to use it. > + > +Background > +== > + > +The LUKS format has ability to store the header in a separate volume from > +the payload. We could extend the LUKS driver in QEMU to support this use > +case. > + > +Normally a LUKS volume has a layout: > + > +:: > + > + +---+ > + | ||| > + disk| header | key material | disk payload data | > + | ||| > + +---+ > + > +With a detached LUKS header, you need 2 disks so getting: > + > +:: > + > + +--+ > + disk1 | header | key material | > + +--+ > + +-+ > + disk2 | disk payload data | > + +-+ > + > +There are a variety of benefits to doing this: > + > + * Secrecy - the disk2 cannot be identified as containing LUKS > + volume since there's no header > + * Control - if access to the disk1 is restricted, then even > + if someone has access to disk2 they can't unlock > + it. Might be useful if you have disks on NFS but > + want to restrict which host can launch a VM > + instance from it, by dynamically providing access > + to the header to a designated host > + * Flexibility - your application data volume may be a given > + size and it is inconvenient to resize it to > + add encryption.You can store the LUKS header > + separately and use the existing storage > + volume for payload > + * Recovery - corruption of a bit in the header may make the > + entire payload inaccessible. It might be > + convenient to take backups of the header. If > + your primary disk header becomes corrupt, you > + can unlock the data still by pointing to the > + backup detached header > + > +Architecture > + > + > +Take the qcow2 encryption, for example. The architecture of the > +LUKS volume with detached header is shown in the diagram below. > + > +There are two children of the root node: a file and a header. > +Data from the disk payload is stored in the file node. The > +LUKS header and key material are located in the header node, > +as previously mentioned. > + > +:: > + > + +-+ > + Root node| foo[luks] | > + +-+ > + | | > + file |header | > + | | > + +-++--+ > + Child node |payload-format[qcow2]||header-format[raw]| > +
[PATCH v3 10/22] qga: conditionalize schema for commands requiring getifaddrs
Rather than creating stubs for every comamnd that just return
QERR_UNSUPPORTED, use 'if' conditions in the QAPI schema to
fully exclude generation of the network interface command on
POSIX platforms lacking getifaddrs().
The command will be rejected at QMP dispatch time instead,
avoiding reimplementing rejection by blocking the stub commands.
This changes the error message for affected commands from
{"class": "CommandNotFound", "desc": "Command FOO has been disabled"}
to
{"class": "CommandNotFound", "desc": "The command FOO has not been found"}
This has the additional benefit that the QGA protocol reference
now documents what conditions enable use of the command.
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Konstantin Kostiuk
Signed-off-by: Daniel P. Berrangé
---
qga/commands-posix.c | 13 -
qga/qapi-schema.json | 15 ++-
2 files changed, 10 insertions(+), 18 deletions(-)
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 559d71ffae..09d08ee2ca 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1142,14 +1142,6 @@ error:
return NULL;
}
-#else
-
-GuestNetworkInterfaceList *qmp_guest_network_get_interfaces(Error **errp)
-{
-error_setg(errp, QERR_UNSUPPORTED);
-return NULL;
-}
-
#endif /* HAVE_GETIFADDRS */
#if !defined(CONFIG_FSFREEZE)
@@ -1222,11 +1214,6 @@ qmp_guest_fstrim(bool has_minimum, int64_t minimum,
Error **errp)
/* add unsupported commands to the list of blocked RPCs */
GList *ga_command_init_blockedrpcs(GList *blockedrpcs)
{
-#if !defined(HAVE_GETIFADDRS)
-blockedrpcs = g_list_append(blockedrpcs,
- g_strdup("guest-network-get-interfaces"));
-#endif
-
#if !defined(CONFIG_FSFREEZE)
{
const char *list[] = {
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index 38483652ac..79ed4f0e21 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -654,7 +654,8 @@
# Since: 1.1
##
{ 'enum': 'GuestIpAddressType',
- 'data': [ 'ipv4', 'ipv6' ] }
+ 'data': [ 'ipv4', 'ipv6' ],
+ 'if': { 'any': ['CONFIG_WIN32', 'HAVE_GETIFADDRS'] } }
##
# @GuestIpAddress:
@@ -670,7 +671,8 @@
{ 'struct': 'GuestIpAddress',
'data': {'ip-address': 'str',
'ip-address-type': 'GuestIpAddressType',
- 'prefix': 'int'} }
+ 'prefix': 'int'},
+ 'if': { 'any': ['CONFIG_WIN32', 'HAVE_GETIFADDRS'] } }
##
# @GuestNetworkInterfaceStat:
@@ -702,7 +704,8 @@
'tx-packets': 'uint64',
'tx-errs': 'uint64',
'tx-dropped': 'uint64'
- } }
+ },
+ 'if': { 'any': ['CONFIG_WIN32', 'HAVE_GETIFADDRS'] } }
##
# @GuestNetworkInterface:
@@ -722,7 +725,8 @@
'data': {'name': 'str',
'*hardware-address': 'str',
'*ip-addresses': ['GuestIpAddress'],
- '*statistics': 'GuestNetworkInterfaceStat' } }
+ '*statistics': 'GuestNetworkInterfaceStat' },
+ 'if': { 'any': ['CONFIG_WIN32', 'HAVE_GETIFADDRS'] } }
##
# @guest-network-get-interfaces:
@@ -734,7 +738,8 @@
# Since: 1.1
##
{ 'command': 'guest-network-get-interfaces',
- 'returns': ['GuestNetworkInterface'] }
+ 'returns': ['GuestNetworkInterface'],
+ 'if': { 'any': ['CONFIG_WIN32', 'HAVE_GETIFADDRS'] } }
##
# @GuestLogicalProcessor:
--
2.45.1
[PATCH v3 05/22] qga: move linux disk/cpu stats command impls to commands-linux.c
The qmp_guest_{diskstats,cpustats} command impls in
commands-posix.c are surrounded by '#ifdef __linux__' so should
instead live in commands-linux.c
This also removes a "#ifdef CONFIG_LINUX" that was nested inside
a "#ifdef __linux__".
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Konstantin Kostiuk
Signed-off-by: Daniel P. Berrangé
---
qga/commands-linux.c | 195 ++
qga/commands-posix.c | 199 ---
2 files changed, 195 insertions(+), 199 deletions(-)
diff --git a/qga/commands-linux.c b/qga/commands-linux.c
index 084e6c9e85..c0e8bd4062 100644
--- a/qga/commands-linux.c
+++ b/qga/commands-linux.c
@@ -1594,3 +1594,198 @@ int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList
*vcpus, Error **errp)
return processed;
}
+
+#define MAX_NAME_LEN 128
+static GuestDiskStatsInfoList *guest_get_diskstats(Error **errp)
+{
+GuestDiskStatsInfoList *head = NULL, **tail =
+const char *diskstats = "/proc/diskstats";
+FILE *fp;
+size_t n;
+char *line = NULL;
+
+fp = fopen(diskstats, "r");
+if (fp == NULL) {
+error_setg_errno(errp, errno, "open(\"%s\")", diskstats);
+return NULL;
+}
+
+while (getline(, , fp) != -1) {
+g_autofree GuestDiskStatsInfo *diskstatinfo = NULL;
+g_autofree GuestDiskStats *diskstat = NULL;
+char dev_name[MAX_NAME_LEN];
+unsigned int ios_pgr, tot_ticks, rq_ticks, wr_ticks, dc_ticks,
fl_ticks;
+unsigned long rd_ios, rd_merges_or_rd_sec, rd_ticks_or_wr_sec, wr_ios;
+unsigned long wr_merges, rd_sec_or_wr_ios, wr_sec;
+unsigned long dc_ios, dc_merges, dc_sec, fl_ios;
+unsigned int major, minor;
+int i;
+
+i = sscanf(line, "%u %u %s %lu %lu %lu"
+ "%lu %lu %lu %lu %u %u %u %u"
+ "%lu %lu %lu %u %lu %u",
+ , , dev_name,
+ _ios, _merges_or_rd_sec, _sec_or_wr_ios,
+ _ticks_or_wr_sec, _ios, _merges, _sec,
+ _ticks, _pgr, _ticks, _ticks,
+ _ios, _merges, _sec, _ticks,
+ _ios, _ticks);
+
+if (i < 7) {
+continue;
+}
+
+diskstatinfo = g_new0(GuestDiskStatsInfo, 1);
+diskstatinfo->name = g_strdup(dev_name);
+diskstatinfo->major = major;
+diskstatinfo->minor = minor;
+
+diskstat = g_new0(GuestDiskStats, 1);
+if (i == 7) {
+diskstat->has_read_ios = true;
+diskstat->read_ios = rd_ios;
+diskstat->has_read_sectors = true;
+diskstat->read_sectors = rd_merges_or_rd_sec;
+diskstat->has_write_ios = true;
+diskstat->write_ios = rd_sec_or_wr_ios;
+diskstat->has_write_sectors = true;
+diskstat->write_sectors = rd_ticks_or_wr_sec;
+}
+if (i >= 14) {
+diskstat->has_read_ios = true;
+diskstat->read_ios = rd_ios;
+diskstat->has_read_sectors = true;
+diskstat->read_sectors = rd_sec_or_wr_ios;
+diskstat->has_read_merges = true;
+diskstat->read_merges = rd_merges_or_rd_sec;
+diskstat->has_read_ticks = true;
+diskstat->read_ticks = rd_ticks_or_wr_sec;
+diskstat->has_write_ios = true;
+diskstat->write_ios = wr_ios;
+diskstat->has_write_sectors = true;
+diskstat->write_sectors = wr_sec;
+diskstat->has_write_merges = true;
+diskstat->write_merges = wr_merges;
+diskstat->has_write_ticks = true;
+diskstat->write_ticks = wr_ticks;
+diskstat->has_ios_pgr = true;
+diskstat->ios_pgr = ios_pgr;
+diskstat->has_total_ticks = true;
+diskstat->total_ticks = tot_ticks;
+diskstat->has_weight_ticks = true;
+diskstat->weight_ticks = rq_ticks;
+}
+if (i >= 18) {
+diskstat->has_discard_ios = true;
+diskstat->discard_ios = dc_ios;
+diskstat->has_discard_merges = true;
+diskstat->discard_merges = dc_merges;
+diskstat->has_discard_sectors = true;
+diskstat->discard_sectors = dc_sec;
+diskstat->has_discard_ticks = true;
+diskstat->discard_ticks = dc_ticks;
+}
+if (i >= 20) {
+diskstat->has_flush_ios = true;
+diskstat->flush_ios = fl_ios;
+diskstat->has_flush_ticks = true;
+diskstat->flush_ticks = fl_ticks;
+}
+
+diskstatinfo->stats = g_steal_pointer();
+QAPI_LIST_APPEND(tail, diskstatinfo);
+diskstatin
[PATCH v3 06/22] qga: move linux memory block command impls to commands-linux.c
The qmp_guest_{set,get}_{memory_blocks,block_info} command impls in
commands-posix.c are surrounded by '#ifdef __linux__' so should
instead live in commands-linux.c
This also removes a "#ifdef CONFIG_LINUX" that was nested inside
a "#ifdef __linux__".
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Konstantin Kostiuk
Signed-off-by: Daniel P. Berrangé
---
qga/commands-linux.c | 308 ++
qga/commands-posix.c | 311 +--
2 files changed, 309 insertions(+), 310 deletions(-)
diff --git a/qga/commands-linux.c b/qga/commands-linux.c
index c0e8bd4062..73b13fbaf6 100644
--- a/qga/commands-linux.c
+++ b/qga/commands-linux.c
@@ -1595,6 +1595,314 @@ int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList
*vcpus, Error **errp)
return processed;
}
+
+static void ga_read_sysfs_file(int dirfd, const char *pathname, char *buf,
+ int size, Error **errp)
+{
+int fd;
+int res;
+
+errno = 0;
+fd = openat(dirfd, pathname, O_RDONLY);
+if (fd == -1) {
+error_setg_errno(errp, errno, "open sysfs file \"%s\"", pathname);
+return;
+}
+
+res = pread(fd, buf, size, 0);
+if (res == -1) {
+error_setg_errno(errp, errno, "pread sysfs file \"%s\"", pathname);
+} else if (res == 0) {
+error_setg(errp, "pread sysfs file \"%s\": unexpected EOF", pathname);
+}
+close(fd);
+}
+
+static void ga_write_sysfs_file(int dirfd, const char *pathname,
+const char *buf, int size, Error **errp)
+{
+int fd;
+
+errno = 0;
+fd = openat(dirfd, pathname, O_WRONLY);
+if (fd == -1) {
+error_setg_errno(errp, errno, "open sysfs file \"%s\"", pathname);
+return;
+}
+
+if (pwrite(fd, buf, size, 0) == -1) {
+error_setg_errno(errp, errno, "pwrite sysfs file \"%s\"", pathname);
+}
+
+close(fd);
+}
+
+/* Transfer online/offline status between @mem_blk and the guest system.
+ *
+ * On input either @errp or *@errp must be NULL.
+ *
+ * In system-to-@mem_blk direction, the following @mem_blk fields are accessed:
+ * - R: mem_blk->phys_index
+ * - W: mem_blk->online
+ * - W: mem_blk->can_offline
+ *
+ * In @mem_blk-to-system direction, the following @mem_blk fields are accessed:
+ * - R: mem_blk->phys_index
+ * - R: mem_blk->online
+ *- R: mem_blk->can_offline
+ * Written members remain unmodified on error.
+ */
+static void transfer_memory_block(GuestMemoryBlock *mem_blk, bool sys2memblk,
+ GuestMemoryBlockResponse *result,
+ Error **errp)
+{
+char *dirpath;
+int dirfd;
+char *status;
+Error *local_err = NULL;
+
+if (!sys2memblk) {
+DIR *dp;
+
+if (!result) {
+error_setg(errp, "Internal error, 'result' should not be NULL");
+return;
+}
+errno = 0;
+dp = opendir("/sys/devices/system/memory/");
+ /* if there is no 'memory' directory in sysfs,
+ * we think this VM does not support online/offline memory block,
+ * any other solution?
+ */
+if (!dp) {
+if (errno == ENOENT) {
+result->response =
+GUEST_MEMORY_BLOCK_RESPONSE_TYPE_OPERATION_NOT_SUPPORTED;
+}
+goto out1;
+}
+closedir(dp);
+}
+
+dirpath = g_strdup_printf("/sys/devices/system/memory/memory%" PRId64 "/",
+ mem_blk->phys_index);
+dirfd = open(dirpath, O_RDONLY | O_DIRECTORY);
+if (dirfd == -1) {
+if (sys2memblk) {
+error_setg_errno(errp, errno, "open(\"%s\")", dirpath);
+} else {
+if (errno == ENOENT) {
+result->response = GUEST_MEMORY_BLOCK_RESPONSE_TYPE_NOT_FOUND;
+} else {
+result->response =
+GUEST_MEMORY_BLOCK_RESPONSE_TYPE_OPERATION_FAILED;
+}
+}
+g_free(dirpath);
+goto out1;
+}
+g_free(dirpath);
+
+status = g_malloc0(10);
+ga_read_sysfs_file(dirfd, "state", status, 10, _err);
+if (local_err) {
+/* treat with sysfs file that not exist in old kernel */
+if (errno == ENOENT) {
+error_free(local_err);
+if (sys2memblk) {
+mem_blk->online = true;
+mem_blk->can_offline = false;
+} else if (!mem_blk->online) {
+result->response =
+GUEST_MEMORY_BLOCK_RESPONSE_TYPE_OPERATION_NOT_SUPPORTED;
+}
+} else {
+if (sys2memblk) {
+error_propagate(errp, local_err);
+
[PATCH v3 13/22] qga: conditionalize schema for commands requiring fsfreeze
Rather than creating stubs for every command that just return
QERR_UNSUPPORTED, use 'if' conditions in the schema to fully
exclude generation of the filesystem freezing commands on POSIX
platforms lacking the required APIs.
The command will be rejected at QMP dispatch time instead,
avoiding reimplementing rejection by blocking the stub commands.
This changes the error message for affected commands from
{"class": "CommandNotFound", "desc": "Command FOO has been disabled"}
to
{"class": "CommandNotFound", "desc": "The command FOO has not been found"}
This has the additional benefit that the QGA protocol reference
now documents what conditions enable use of the command.
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Konstantin Kostiuk
Signed-off-by: Daniel P. Berrangé
---
qga/commands-posix.c | 47
qga/qapi-schema.json | 15 +-
2 files changed, 10 insertions(+), 52 deletions(-)
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index b7f96aa005..9207cb7a8f 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1144,39 +1144,6 @@ error:
#endif /* HAVE_GETIFADDRS */
-#if !defined(CONFIG_FSFREEZE)
-
-GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **errp)
-{
-error_setg(errp, QERR_UNSUPPORTED);
-
-return 0;
-}
-
-int64_t qmp_guest_fsfreeze_freeze(Error **errp)
-{
-error_setg(errp, QERR_UNSUPPORTED);
-
-return 0;
-}
-
-int64_t qmp_guest_fsfreeze_freeze_list(bool has_mountpoints,
- strList *mountpoints,
- Error **errp)
-{
-error_setg(errp, QERR_UNSUPPORTED);
-
-return 0;
-}
-
-int64_t qmp_guest_fsfreeze_thaw(Error **errp)
-{
-error_setg(errp, QERR_UNSUPPORTED);
-
-return 0;
-}
-#endif /* CONFIG_FSFREEZE */
-
#if !defined(CONFIG_FSTRIM)
GuestFilesystemTrimResponse *
qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
@@ -1189,20 +1156,6 @@ qmp_guest_fstrim(bool has_minimum, int64_t minimum,
Error **errp)
/* add unsupported commands to the list of blocked RPCs */
GList *ga_command_init_blockedrpcs(GList *blockedrpcs)
{
-#if !defined(CONFIG_FSFREEZE)
-{
-const char *list[] = {
-"guest-fsfreeze-status",
-"guest-fsfreeze-freeze", "guest-fsfreeze-freeze-list",
-"guest-fsfreeze-thaw", NULL};
-char **p = (char **)list;
-
-while (*p) {
-blockedrpcs = g_list_append(blockedrpcs, g_strdup(*p++));
-}
-}
-#endif
-
#if !defined(CONFIG_FSTRIM)
blockedrpcs = g_list_append(blockedrpcs, g_strdup("guest-fstrim"));
#endif
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index de3fc46d2e..62462f092c 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -412,7 +412,8 @@
# Since: 0.15.0
##
{ 'enum': 'GuestFsfreezeStatus',
- 'data': [ 'thawed', 'frozen' ] }
+ 'data': [ 'thawed', 'frozen' ],
+ 'if': { 'any': ['CONFIG_WIN32', 'CONFIG_FSFREEZE'] } }
##
# @guest-fsfreeze-status:
@@ -429,7 +430,8 @@
# Since: 0.15.0
##
{ 'command': 'guest-fsfreeze-status',
- 'returns': 'GuestFsfreezeStatus' }
+ 'returns': 'GuestFsfreezeStatus',
+ 'if': { 'any': ['CONFIG_WIN32', 'CONFIG_FSFREEZE'] } }
##
# @guest-fsfreeze-freeze:
@@ -451,7 +453,8 @@
# Since: 0.15.0
##
{ 'command': 'guest-fsfreeze-freeze',
- 'returns': 'int' }
+ 'returns': 'int',
+ 'if': { 'any': ['CONFIG_WIN32', 'CONFIG_FSFREEZE'] } }
##
# @guest-fsfreeze-freeze-list:
@@ -471,7 +474,8 @@
##
{ 'command': 'guest-fsfreeze-freeze-list',
'data':{ '*mountpoints': ['str'] },
- 'returns': 'int' }
+ 'returns': 'int',
+ 'if': { 'any': ['CONFIG_WIN32', 'CONFIG_FSFREEZE'] } }
##
# @guest-fsfreeze-thaw:
@@ -488,7 +492,8 @@
# Since: 0.15.0
##
{ 'command': 'guest-fsfreeze-thaw',
- 'returns': 'int' }
+ 'returns': 'int',
+ 'if': { 'any': ['CONFIG_WIN32', 'CONFIG_FSFREEZE'] } }
##
# @GuestFilesystemTrimResult:
--
2.45.1
[PATCH v3 16/22] qga: conditionalize schema for commands requiring utmpx
Rather than creating stubs for every command that just return
QERR_UNSUPPORTED, use 'if' conditions in the QAPI schema to
fully exclude generation of the get-users command on POSIX
platforms lacking required APIs.
The command will be rejected at QMP dispatch time instead,
avoiding reimplementing rejection by blocking the stub commands.
This changes the error message for affected commands from
{"class": "CommandNotFound", "desc": "Command FOO has been disabled"}
to
{"class": "CommandNotFound", "desc": "The command FOO has not been found"}
This has the additional benefit that the QGA protocol reference
now documents what conditions enable use of the command.
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Konstantin Kostiuk
Signed-off-by: Daniel P. Berrangé
---
qga/commands-posix.c | 10 +-
qga/qapi-schema.json | 6 --
2 files changed, 5 insertions(+), 11 deletions(-)
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index d92fa0ec87..a353f64ae6 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1212,15 +1212,7 @@ GuestUserList *qmp_guest_get_users(Error **errp)
return head;
}
-#else
-
-GuestUserList *qmp_guest_get_users(Error **errp)
-{
-error_setg(errp, QERR_UNSUPPORTED);
-return NULL;
-}
-
-#endif
+#endif /* HAVE_UTMPX */
/* Replace escaped special characters with their real values. The replacement
* is done in place -- returned value is in the original string.
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index cf1ad42519..0662a68c43 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -1414,7 +1414,8 @@
# Since: 2.10
##
{ 'struct': 'GuestUser',
- 'data': { 'user': 'str', 'login-time': 'number', '*domain': 'str' } }
+ 'data': { 'user': 'str', 'login-time': 'number', '*domain': 'str' },
+ 'if': { 'any': ['CONFIG_WIN32', 'HAVE_UTMPX' ] } }
##
# @guest-get-users:
@@ -1426,7 +1427,8 @@
# Since: 2.10
##
{ 'command': 'guest-get-users',
- 'returns': ['GuestUser'] }
+ 'returns': ['GuestUser'],
+ 'if': { 'any': ['CONFIG_WIN32', 'HAVE_UTMPX' ] } }
##
# @GuestTimezone:
--
2.45.1
[PATCH v3 08/22] qga: conditionalize schema for commands unsupported on Windows
Rather than creating stubs for every command that just return
QERR_UNSUPPORTED, use 'if' conditions in the QAPI schema to
fully exclude generation of the commands on Windows.
The command will be rejected at QMP dispatch time instead,
avoiding reimplementing rejection by blocking the stub commands.
This changes the error message for affected commands from
{"class": "CommandNotFound", "desc": "Command FOO has been disabled"}
to
{"class": "CommandNotFound", "desc": "The command FOO has not been found"}
This also fixes an accidental inconsistency where some commands
(guest-get-diskstats & guest-get-cpustats) are implemented as
stubs, yet not added to the blockedrpc list. Those change their
error message from
{"class": "GenericError, "desc": "this feature or command is not currently
supported"}
to
{"class": "CommandNotFound", "desc": "The command FOO has not been found"}
The final additional benefit is that the QGA protocol reference
now documents what conditions enable use of the command.
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Konstantin Kostiuk
Signed-off-by: Daniel P. Berrangé
---
qga/commands-posix.c | 2 +-
qga/commands-win32.c | 56 +---
qga/qapi-schema.json | 45 +++
3 files changed, 32 insertions(+), 71 deletions(-)
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 2a3bef7445..0dd8555867 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1280,7 +1280,7 @@ GList *ga_command_init_blockedrpcs(GList *blockedrpcs)
"guest-get-memory-blocks", "guest-set-memory-blocks",
"guest-get-memory-block-info",
NULL};
-char **p = (char **)list;
+const char **p = list;
while (*p) {
blockedrpcs = g_list_append(blockedrpcs, g_strdup(*p++));
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 9fe670d5b4..2533e4c748 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -1494,11 +1494,6 @@ out:
}
}
-void qmp_guest_suspend_hybrid(Error **errp)
-{
-error_setg(errp, QERR_UNSUPPORTED);
-}
-
static IP_ADAPTER_ADDRESSES *guest_get_adapters_addresses(Error **errp)
{
IP_ADAPTER_ADDRESSES *adptr_addrs = NULL;
@@ -1862,12 +1857,6 @@ GuestLogicalProcessorList *qmp_guest_get_vcpus(Error
**errp)
return NULL;
}
-int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
-{
-error_setg(errp, QERR_UNSUPPORTED);
-return -1;
-}
-
static gchar *
get_net_error_message(gint error)
{
@@ -1969,46 +1958,15 @@ done:
g_free(rawpasswddata);
}
-GuestMemoryBlockList *qmp_guest_get_memory_blocks(Error **errp)
-{
-error_setg(errp, QERR_UNSUPPORTED);
-return NULL;
-}
-
-GuestMemoryBlockResponseList *
-qmp_guest_set_memory_blocks(GuestMemoryBlockList *mem_blks, Error **errp)
-{
-error_setg(errp, QERR_UNSUPPORTED);
-return NULL;
-}
-
-GuestMemoryBlockInfo *qmp_guest_get_memory_block_info(Error **errp)
-{
-error_setg(errp, QERR_UNSUPPORTED);
-return NULL;
-}
-
/* add unsupported commands to the list of blocked RPCs */
GList *ga_command_init_blockedrpcs(GList *blockedrpcs)
{
-const char *list_unsupported[] = {
-"guest-suspend-hybrid",
-"guest-set-vcpus",
-"guest-get-memory-blocks", "guest-set-memory-blocks",
-"guest-get-memory-block-info",
-NULL};
-char **p = (char **)list_unsupported;
-
-while (*p) {
-blockedrpcs = g_list_append(blockedrpcs, g_strdup(*p++));
-}
-
if (!vss_init(true)) {
g_debug("vss_init failed, vss commands are going to be disabled");
const char *list[] = {
"guest-get-fsinfo", "guest-fsfreeze-status",
"guest-fsfreeze-freeze", "guest-fsfreeze-thaw", NULL};
-p = (char **)list;
+char **p = (char **)list;
while (*p) {
blockedrpcs = g_list_append(blockedrpcs, g_strdup(*p++));
@@ -2505,15 +2463,3 @@ char *qga_get_host_name(Error **errp)
return g_utf16_to_utf8(tmp, size, NULL, NULL, NULL);
}
-
-GuestDiskStatsInfoList *qmp_guest_get_diskstats(Error **errp)
-{
-error_setg(errp, QERR_UNSUPPORTED);
-return NULL;
-}
-
-GuestCpuStatsList *qmp_guest_get_cpustats(Error **errp)
-{
-error_setg(errp, QERR_UNSUPPORTED);
-return NULL;
-}
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index 1273d85bb5..2f0215afc7 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -637,7 +637,8 @@
#
# Since: 1.1
##
-{ 'command': 'guest-suspend-hybrid', 'success-response': false }
+{ 'command': 'guest-suspend-hybrid', 'success-response': false,
+ 'if': 'CONFIG_POSIX' }
[PATCH v3 21/22] qga: allow configuration file path via the cli
Allowing the user to set the QGA_CONF environment variable to change
the default configuration file path is very unusual practice, made
more obscure since this ability is not documented.
This introduces the more normal '-c PATH' / '--config=PATH' command
line argument approach. This requires that we parse the comamnd line
twice, since we want the command line arguments to take priority over
the configuration file settings in general.
Signed-off-by: Daniel P. Berrangé
---
docs/interop/qemu-ga.rst | 5 +
qga/main.c | 43 ++--
2 files changed, 38 insertions(+), 10 deletions(-)
diff --git a/docs/interop/qemu-ga.rst b/docs/interop/qemu-ga.rst
index 72fb75a6f5..e42b370319 100644
--- a/docs/interop/qemu-ga.rst
+++ b/docs/interop/qemu-ga.rst
@@ -33,6 +33,11 @@ Options
.. program:: qemu-ga
+.. option:: -c, --config=PATH
+
+ Configuration file path (the default is |CONFDIR|\ ``/qemu-ga.conf``,
+ unless overriden by the QGA_CONF environment variable)
+
.. option:: -m, --method=METHOD
Transport method: one of ``unix-listen``, ``virtio-serial``, or
diff --git a/qga/main.c b/qga/main.c
index 6ff022a85d..6ae911eb15 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -248,12 +248,16 @@ static void usage(const char *cmd)
#ifdef CONFIG_FSFREEZE
g_autofree char *fsfreeze_hook =
get_relocated_path(QGA_FSFREEZE_HOOK_DEFAULT);
#endif
+g_autofree char *conf_path = get_relocated_path(QGA_CONF_DEFAULT);
printf(
"Usage: %s [-m -p ] []\n"
"QEMU Guest Agent " QEMU_FULL_VERSION "\n"
QEMU_COPYRIGHT "\n"
"\n"
+" -c, --config=PATH configuration file path (default is\n"
+"%s/qemu-ga.conf\n"
+"unless overriden by the QGA_CONF environment variable)\n"
" -m, --method transport method: one of unix-listen, virtio-serial,\n"
"isa-serial, or vsock-listen (virtio-serial is the
default)\n"
" -p, --pathdevice/socket path (the default for virtio-serial is:\n"
@@ -294,8 +298,8 @@ QEMU_COPYRIGHT "\n"
"plug/unplug, etc.)\n"
" -h, --helpdisplay this help and exit\n"
"\n"
-QEMU_HELP_BOTTOM "\n"
-, cmd, QGA_VIRTIO_PATH_DEFAULT, QGA_SERIAL_PATH_DEFAULT,
+QEMU_HELP_BOTTOM "\n",
+cmd, conf_path, QGA_VIRTIO_PATH_DEFAULT, QGA_SERIAL_PATH_DEFAULT,
dfl_pathnames.pidfile,
#ifdef CONFIG_FSFREEZE
fsfreeze_hook,
@@ -1018,15 +1022,14 @@ static GList *split_list(const gchar *str, const gchar
*delim)
return list;
}
-static void config_load(GAConfig *config)
+static void config_load(GAConfig *config, const char *confpath, bool required)
{
GError *gerr = NULL;
GKeyFile *keyfile;
-g_autofree char *conf = g_strdup(g_getenv("QGA_CONF")) ?:
get_relocated_path(QGA_CONF_DEFAULT);
/* read system config */
keyfile = g_key_file_new();
-if (!g_key_file_load_from_file(keyfile, conf, 0, )) {
+if (!g_key_file_load_from_file(keyfile, confpath, 0, )) {
goto end;
}
if (g_key_file_has_key(keyfile, "general", "daemon", NULL)) {
@@ -1092,10 +1095,10 @@ static void config_load(GAConfig *config)
end:
g_key_file_free(keyfile);
-if (gerr &&
-!(gerr->domain == G_FILE_ERROR && gerr->code == G_FILE_ERROR_NOENT)) {
+if (gerr && (required ||
+ !(gerr->domain == G_FILE_ERROR && gerr->code ==
G_FILE_ERROR_NOENT))) {
g_critical("error loading configuration from path: %s, %s",
- conf, gerr->message);
+ confpath, gerr->message);
exit(EXIT_FAILURE);
}
g_clear_error();
@@ -1167,12 +1170,13 @@ static void config_dump(GAConfig *config)
static void config_parse(GAConfig *config, int argc, char **argv)
{
-const char *sopt = "hVvdm:p:l:f:F::b:a:s:t:Dr";
+const char *sopt = "hVvdc:m:p:l:f:F::b:a:s:t:Dr";
int opt_ind = 0, ch;
bool block_rpcs = false, allow_rpcs = false;
const struct option lopt[] = {
{ "help", 0, NULL, 'h' },
{ "version", 0, NULL, 'V' },
+{ "config", 1, NULL, 'c' },
{ "dump-conf", 0, NULL, 'D' },
{ "logfile", 1, NULL, 'l' },
{ "pidfile", 1, NULL, 'f' },
@@ -1192,6 +1196,26 @@ static void config_parse(GAConfig *config, int argc,
char **argv)
{ "retry-path", 0, NULL, 'r' },
{ NULL, 0, NULL, 0 }
};
+g_autofree char *confpath = g_strdup(g_getenv("QGA_CONF")) ?:
+get_relocated_path(QGA_CONF_DEFAULT);
+bool confrequired = false;
+
+while ((ch = getopt_long(argc, argv, sopt, lopt, NULL)) != -1) {
+switch (ch) {
+case
[PATCH v3 22/22] qga: centralize logic for disabling/enabling commands
It is confusing having many different pieces of code enabling and
disabling commands, and it is not clear that they all have the same
semantics, especially wrt prioritization of the block/allow lists.
The code attempted to prevent the user from setting both the block
and allow lists concurrently, however, the logic was flawed as it
checked settings in the configuration file separately from the
command line arguments. Thus it was possible to set a block list
in the config file and an allow list via a command line argument.
The --dump-conf option also creates a configuration file with both
keys present, even if unset, which means it is creating a config
that cannot actually be loaded again.
Centralizing the code in a single method "ga_apply_command_filters"
will provide a strong guarantee of consistency and clarify the
intended behaviour. With this there is no compelling technical
reason to prevent concurrent setting of both the allow and block
lists, so this flawed restriction is removed.
Signed-off-by: Daniel P. Berrangé
---
docs/interop/qemu-ga.rst | 14 +
qga/commands-posix.c | 6 --
qga/commands-win32.c | 6 --
qga/main.c | 128 +--
4 files changed, 70 insertions(+), 84 deletions(-)
diff --git a/docs/interop/qemu-ga.rst b/docs/interop/qemu-ga.rst
index e42b370319..fb75cfd8d4 100644
--- a/docs/interop/qemu-ga.rst
+++ b/docs/interop/qemu-ga.rst
@@ -28,6 +28,20 @@ configuration options on the command line. For the same key,
the last
option wins, but the lists accumulate (see below for configuration
file format).
+If an allowed RPCs list is defined in the configuration, then all
+RPCs will be blocked by default, except for the allowed list.
+
+If a blocked RPCs list is defined in the configuration, then all
+RPCs will be allowed by default, except for the blocked list.
+
+If both allowed and blocked RPCs lists are defined in the configuration,
+then all RPCs will be blocked by default, then the allowed list will
+be applied, followed by the blocked list.
+
+While filesystems are frozen, all except for a designated safe set
+of RPCs will blocked, regardless of what the general configuration
+declares.
+
Options
---
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index f4104f2760..578d29f228 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1136,12 +1136,6 @@ error:
#endif /* HAVE_GETIFADDRS */
-/* add unsupported commands to the list of blocked RPCs */
-GList *ga_command_init_blockedrpcs(GList *blockedrpcs)
-{
-return blockedrpcs;
-}
-
/* register init/cleanup routines for stateful command groups */
void ga_command_state_init(GAState *s, GACommandState *cs)
{
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 5866cc2e3c..61b36da469 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -1958,12 +1958,6 @@ done:
g_free(rawpasswddata);
}
-/* add unsupported commands to the list of blocked RPCs */
-GList *ga_command_init_blockedrpcs(GList *blockedrpcs)
-{
-return blockedrpcs;
-}
-
/* register init/cleanup routines for stateful command groups */
void ga_command_state_init(GAState *s, GACommandState *cs)
{
diff --git a/qga/main.c b/qga/main.c
index 6ae911eb15..b8f7b1e4a3 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -423,60 +423,79 @@ static gint ga_strcmp(gconstpointer str1, gconstpointer
str2)
return strcmp(str1, str2);
}
-/* disable commands that aren't safe for fsfreeze */
-static void ga_disable_not_allowed_freeze(const QmpCommand *cmd, void *opaque)
+static bool ga_command_is_allowed(const QmpCommand *cmd, GAState *state)
{
-bool allowed = false;
int i = 0;
+GAConfig *config = state->config;
const char *name = qmp_command_name(cmd);
+/* Fallback policy is allow everything */
+bool allowed = true;
-while (ga_freeze_allowlist[i] != NULL) {
-if (strcmp(name, ga_freeze_allowlist[i]) == 0) {
+if (config->allowedrpcs) {
+/*
+ * If an allow-list is given, this changes the fallback
+ * policy to deny everything
+ */
+allowed = false;
+
+if (g_list_find_custom(config->allowedrpcs, name, ga_strcmp) != NULL) {
allowed = true;
}
-i++;
}
-if (!allowed) {
-g_debug("disabling command: %s", name);
-qmp_disable_command(_commands, name, "the agent is in frozen
state");
-}
-}
-/* [re-]enable all commands, except those explicitly blocked by user */
-static void ga_enable_non_blocked(const QmpCommand *cmd, void *opaque)
-{
-GAState *s = opaque;
-GList *blockedrpcs = s->blockedrpcs;
-GList *allowedrpcs = s->allowedrpcs;
-const char *name = qmp_command_name(cmd);
-
-if (g_list_find_custom(blockedrpcs, name, ga_strcmp) == NULL) {
-if (qmp_command_is_enabled(cmd)) {
-return;
+/*
+ * If both allowedrpcs and blockedrpcs are set
[PATCH v3 15/22] qga: conditionalize schema for commands requiring libudev
Rather than creating stubs for every command that just return
QERR_UNSUPPORTED, use 'if' conditions in the schema to fully
exclude generation of the filesystem trimming commands on POSIX
platforms lacking required APIs.
The command will be rejected at QMP dispatch time instead,
avoiding reimplementing rejection by blocking the stub commands.
This changes the error message for affected commands from
{"class": "CommandNotFound", "desc": "Command FOO has been disabled"}
to
{"class": "CommandNotFound", "desc": "The command FOO has not been found"}
This has the additional benefit that the QGA protocol reference
now documents what conditions enable use of the command.
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Konstantin Kostiuk
Signed-off-by: Daniel P. Berrangé
---
qga/commands-linux.c | 8
qga/qapi-schema.json | 8
2 files changed, 4 insertions(+), 12 deletions(-)
diff --git a/qga/commands-linux.c b/qga/commands-linux.c
index 73b13fbaf6..89bdcded01 100644
--- a/qga/commands-linux.c
+++ b/qga/commands-linux.c
@@ -1049,14 +1049,6 @@ GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
return ret;
}
-#else
-
-GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
-{
-error_setg(errp, QERR_UNSUPPORTED);
-return NULL;
-}
-
#endif
/* Return a list of the disk device(s)' info which @mount lies on */
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index 21c65d1806..cf1ad42519 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -986,7 +986,7 @@
'media-errors-hi': 'uint64',
'number-of-error-log-entries-lo': 'uint64',
'number-of-error-log-entries-hi': 'uint64' },
- 'if': { 'any': [ 'CONFIG_WIN32', 'CONFIG_LINUX' ] } }
+ 'if': { 'any': [ 'CONFIG_WIN32', 'CONFIG_LIBUDEV' ] } }
##
# @GuestDiskSmart:
@@ -1001,7 +1001,7 @@
'base': { 'type': 'GuestDiskBusType' },
'discriminator': 'type',
'data': { 'nvme': 'GuestNVMeSmart' },
- 'if': { 'any': [ 'CONFIG_WIN32', 'CONFIG_LINUX' ] } }
+ 'if': { 'any': [ 'CONFIG_WIN32', 'CONFIG_LIBUDEV' ] } }
##
# @GuestDiskInfo:
@@ -1027,7 +1027,7 @@
'data': {'name': 'str', 'partition': 'bool', '*dependencies': ['str'],
'*address': 'GuestDiskAddress', '*alias': 'str',
'*smart': 'GuestDiskSmart'},
- 'if': { 'any': [ 'CONFIG_WIN32', 'CONFIG_LINUX' ] } }
+ 'if': { 'any': [ 'CONFIG_WIN32', 'CONFIG_LIBUDEV' ] } }
##
# @guest-get-disks:
@@ -1041,7 +1041,7 @@
##
{ 'command': 'guest-get-disks',
'returns': ['GuestDiskInfo'],
- 'if': { 'any': [ 'CONFIG_WIN32', 'CONFIG_LINUX' ] } }
+ 'if': { 'any': [ 'CONFIG_WIN32', 'CONFIG_LIBUDEV' ] } }
##
# @GuestFilesystemInfo:
--
2.45.1
[PATCH v3 18/22] qga: don't disable fsfreeze commands if vss_init fails
The fsfreeze commands are already written to report an error if
vss_init() fails. Reporting a more specific error message is more
helpful than a generic "command is disabled" message, which cannot
between an admin config decision and lack of platform support.
Reviewed-by: Manos Pitsidianakis
Signed-off-by: Daniel P. Berrangé
---
qga/commands-win32.c | 18 +++---
qga/main.c | 4
2 files changed, 7 insertions(+), 15 deletions(-)
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 2533e4c748..5866cc2e3c 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -1203,7 +1203,7 @@ GuestFilesystemInfoList *qmp_guest_get_fsinfo(Error
**errp)
GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **errp)
{
if (!vss_initialized()) {
-error_setg(errp, QERR_UNSUPPORTED);
+error_setg(errp, "fsfreeze not possible as VSS failed to initialize");
return 0;
}
@@ -1231,7 +1231,7 @@ int64_t qmp_guest_fsfreeze_freeze_list(bool
has_mountpoints,
Error *local_err = NULL;
if (!vss_initialized()) {
-error_setg(errp, QERR_UNSUPPORTED);
+error_setg(errp, "fsfreeze not possible as VSS failed to initialize");
return 0;
}
@@ -1266,7 +1266,7 @@ int64_t qmp_guest_fsfreeze_thaw(Error **errp)
int i;
if (!vss_initialized()) {
-error_setg(errp, QERR_UNSUPPORTED);
+error_setg(errp, "fsfreeze not possible as VSS failed to initialize");
return 0;
}
@@ -1961,18 +1961,6 @@ done:
/* add unsupported commands to the list of blocked RPCs */
GList *ga_command_init_blockedrpcs(GList *blockedrpcs)
{
-if (!vss_init(true)) {
-g_debug("vss_init failed, vss commands are going to be disabled");
-const char *list[] = {
-"guest-get-fsinfo", "guest-fsfreeze-status",
-"guest-fsfreeze-freeze", "guest-fsfreeze-thaw", NULL};
-char **p = (char **)list;
-
-while (*p) {
-blockedrpcs = g_list_append(blockedrpcs, g_strdup(*p++));
-}
-}
-
return blockedrpcs;
}
diff --git a/qga/main.c b/qga/main.c
index f4d5f15bb3..17b6ce18ac 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -1395,6 +1395,10 @@ static GAState *initialize_agent(GAConfig *config, int
socket_activation)
" '%s': %s", config->state_dir, strerror(errno));
return NULL;
}
+
+if (!vss_init(true)) {
+g_debug("vss_init failed, vss commands will not function");
+}
#endif
if (ga_is_frozen(s)) {
--
2.45.1
[PATCH v3 20/22] qga: remove pointless 'blockrpcs_key' variable
This variable was used to support back compat for the old config
file key name, and became redundant after the following change:
commit a7a2d636ae4549ef0551134d4bf8e084a14431c4
Author: Philippe Mathieu-Daudé
Date: Thu May 30 08:36:43 2024 +0200
qga: Remove deprecated 'blacklist' argument / config key
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Konstantin Kostiuk
Signed-off-by: Daniel P. Berrangé
---
qga/main.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/qga/main.c b/qga/main.c
index 647d27037c..6ff022a85d 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -1023,7 +1023,6 @@ static void config_load(GAConfig *config)
GError *gerr = NULL;
GKeyFile *keyfile;
g_autofree char *conf = g_strdup(g_getenv("QGA_CONF")) ?:
get_relocated_path(QGA_CONF_DEFAULT);
-const gchar *blockrpcs_key = "block-rpcs";
/* read system config */
keyfile = g_key_file_new();
@@ -1071,9 +1070,9 @@ static void config_load(GAConfig *config)
g_key_file_get_boolean(keyfile, "general", "retry-path", );
}
-if (g_key_file_has_key(keyfile, "general", blockrpcs_key, NULL)) {
+if (g_key_file_has_key(keyfile, "general", "block-rpcs", NULL)) {
config->bliststr =
-g_key_file_get_string(keyfile, "general", blockrpcs_key, );
+g_key_file_get_string(keyfile, "general", "block-rpcs", );
config->blockedrpcs = g_list_concat(config->blockedrpcs,
split_list(config->bliststr, ","));
}
@@ -1084,7 +1083,7 @@ static void config_load(GAConfig *config)
split_list(config->aliststr, ","));
}
-if (g_key_file_has_key(keyfile, "general", blockrpcs_key, NULL) &&
+if (g_key_file_has_key(keyfile, "general", "block-rpcs", NULL) &&
g_key_file_has_key(keyfile, "general", "allow-rpcs", NULL)) {
g_critical("wrong config, using 'block-rpcs' and 'allow-rpcs' keys at"
" the same time is not allowed");
--
2.45.1
[PATCH v3 11/22] qga: conditionalize schema for commands requiring linux/win32
Some commands were blocked based on CONFIG_FSFREEZE, but their
impl had nothing todo with CONFIG_FSFREEZE, and were instead
either Linux-only, or Win+Linux-only.
Rather than creating stubs for every command that just return
QERR_UNSUPPORTED, use 'if' conditions in the QAPI schema to
fully exclude generation of the stats and fsinfo commands on
platforms that can't support them.
The command will be rejected at QMP dispatch time instead,
avoiding reimplementing rejection by blocking the stub commands.
This changes the error message for affected commands from
{"class": "CommandNotFound", "desc": "Command FOO has been disabled"}
to
{"class": "CommandNotFound", "desc": "The command FOO has not been found"}
This has the additional benefit that the QGA protocol reference
now documents what conditions enable use of the command.
Signed-off-by: Daniel P. Berrangé
---
qga/commands-bsd.c | 24 ---
qga/commands-posix.c | 30 ++---
qga/qapi-schema.json | 45 +++-
3 files changed, 30 insertions(+), 69 deletions(-)
diff --git a/qga/commands-bsd.c b/qga/commands-bsd.c
index 17bddda1cf..9ce48af311 100644
--- a/qga/commands-bsd.c
+++ b/qga/commands-bsd.c
@@ -149,30 +149,6 @@ int qmp_guest_fsfreeze_do_thaw(Error **errp)
}
return ret;
}
-
-GuestFilesystemInfoList *qmp_guest_get_fsinfo(Error **errp)
-{
-error_setg(errp, QERR_UNSUPPORTED);
-return NULL;
-}
-
-GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
-{
-error_setg(errp, QERR_UNSUPPORTED);
-return NULL;
-}
-
-GuestDiskStatsInfoList *qmp_guest_get_diskstats(Error **errp)
-{
-error_setg(errp, QERR_UNSUPPORTED);
-return NULL;
-}
-
-GuestCpuStatsList *qmp_guest_get_cpustats(Error **errp)
-{
-error_setg(errp, QERR_UNSUPPORTED);
-return NULL;
-}
#endif /* CONFIG_FSFREEZE */
#ifdef HAVE_GETIFADDRS
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 09d08ee2ca..838dc3cf98 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1146,12 +1146,6 @@ error:
#if !defined(CONFIG_FSFREEZE)
-GuestFilesystemInfoList *qmp_guest_get_fsinfo(Error **errp)
-{
-error_setg(errp, QERR_UNSUPPORTED);
-return NULL;
-}
-
GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **errp)
{
error_setg(errp, QERR_UNSUPPORTED);
@@ -1181,25 +1175,6 @@ int64_t qmp_guest_fsfreeze_thaw(Error **errp)
return 0;
}
-
-GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
-{
-error_setg(errp, QERR_UNSUPPORTED);
-return NULL;
-}
-
-GuestDiskStatsInfoList *qmp_guest_get_diskstats(Error **errp)
-{
-error_setg(errp, QERR_UNSUPPORTED);
-return NULL;
-}
-
-GuestCpuStatsList *qmp_guest_get_cpustats(Error **errp)
-{
-error_setg(errp, QERR_UNSUPPORTED);
-return NULL;
-}
-
#endif /* CONFIG_FSFREEZE */
#if !defined(CONFIG_FSTRIM)
@@ -1217,10 +1192,9 @@ GList *ga_command_init_blockedrpcs(GList *blockedrpcs)
#if !defined(CONFIG_FSFREEZE)
{
const char *list[] = {
-"guest-get-fsinfo", "guest-fsfreeze-status",
+"guest-fsfreeze-status",
"guest-fsfreeze-freeze", "guest-fsfreeze-freeze-list",
-"guest-fsfreeze-thaw", "guest-get-fsinfo",
-"guest-get-disks", NULL};
+"guest-fsfreeze-thaw", NULL};
char **p = (char **)list;
while (*p) {
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index 79ed4f0e21..9bd5aa53bc 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -870,7 +870,8 @@
{ 'enum': 'GuestDiskBusType',
'data': [ 'ide', 'fdc', 'scsi', 'virtio', 'xen', 'usb', 'uml', 'sata',
'sd', 'unknown', 'ieee1394', 'ssa', 'fibre', 'raid', 'iscsi',
-'sas', 'mmc', 'virtual', 'file-backed-virtual', 'nvme' ] }
+'sas', 'mmc', 'virtual', 'file-backed-virtual', 'nvme' ],
+ 'if': { 'any': [ 'CONFIG_WIN32', 'CONFIG_LINUX' ] } }
##
@@ -888,7 +889,8 @@
##
{ 'struct': 'GuestPCIAddress',
'data': {'domain': 'int', 'bus': 'int',
- 'slot': 'int', 'function': 'int'} }
+ 'slot': 'int', 'function': 'int'},
+ 'if': { 'any': [ 'CONFIG_WIN32', 'CONFIG_LINUX' ] } }
##
# @GuestCCWAddress:
@@ -907,7 +909,8 @@
'data': {'cssid': 'int',
'ssid': 'int',
'subchno': 'int',
- 'devno': 'int'} }
+ 'devno': 'int'},
+ 'if': { 'any': [ 'CONFIG_WIN32', 'CONFIG_LINUX' ] } }
##
# @GuestDiskAddress:
@@ -936,7 +939,8 @@
'bus-type': 'GuestDiskBusType',
'bus': 'int', 'target': 'int', 'unit': 'int',
'*serial': 'str', '*dev': 'str',
- '*ccw-address': 'GuestCCWAddress'} }
+ '*ccw-address': 'GuestCCWAddress'},
+ 'if': { 'any': [ 'CONFIG_WIN32', 'CONFIG_LINUX' ] } }
##
# @GuestNVMeSmart:
@@ -973
[PATCH v3 17/22] qga: conditionalize schema for commands not supported on other UNIX
Rather than creating stubs for every command that just return
QERR_UNSUPPORTED, use 'if' conditions in the QAPI schema to
fully exclude generation of the commands on other UNIX.
The command will be rejected at QMP dispatch time instead,
avoiding reimplementing rejection by blocking the stub commands.
This changes the error message for affected commands from
{"class": "CommandNotFound", "desc": "Command FOO has been disabled"}
to
{"class": "CommandNotFound", "desc": "The command FOO has not been found"}
This has the additional benefit that the QGA protocol reference
now documents what conditions enable use of the command.
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Konstantin Kostiuk
Signed-off-by: Daniel P. Berrangé
---
meson.build | 1 +
qga/commands-posix.c | 8
qga/qapi-schema.json | 3 ++-
3 files changed, 3 insertions(+), 9 deletions(-)
diff --git a/meson.build b/meson.build
index 3f125d1b02..efc47612a3 100644
--- a/meson.build
+++ b/meson.build
@@ -2275,6 +2275,7 @@ config_host_data.set('CONFIG_ATTR', libattr.found())
config_host_data.set('CONFIG_BDRV_WHITELIST_TOOLS',
get_option('block_drv_whitelist_in_tools'))
config_host_data.set('CONFIG_BRLAPI', brlapi.found())
config_host_data.set('CONFIG_BSD', host_os in bsd_oses)
+config_host_data.set('CONFIG_FREEBSD', host_os == 'freebsd')
config_host_data.set('CONFIG_CAPSTONE', capstone.found())
config_host_data.set('CONFIG_COCOA', cocoa.found())
config_host_data.set('CONFIG_DARWIN', host_os == 'darwin')
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index a353f64ae6..f4104f2760 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -877,14 +877,6 @@ void qmp_guest_set_user_password(const char *username,
return;
}
}
-#else /* __linux__ || __FreeBSD__ */
-void qmp_guest_set_user_password(const char *username,
- const char *password,
- bool crypted,
- Error **errp)
-{
-error_setg(errp, QERR_UNSUPPORTED);
-}
#endif /* __linux__ || __FreeBSD__ */
#ifdef HAVE_GETIFADDRS
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index 0662a68c43..c763163fcd 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -1109,7 +1109,8 @@
# Since: 2.3
##
{ 'command': 'guest-set-user-password',
- 'data': { 'username': 'str', 'password': 'str', 'crypted': 'bool' } }
+ 'data': { 'username': 'str', 'password': 'str', 'crypted': 'bool' },
+ 'if': { 'any': [ 'CONFIG_WIN32', 'CONFIG_LINUX', 'CONFIG_FREEBSD'] } }
##
# @GuestMemoryBlock:
--
2.45.1
[PATCH v3 12/22] qga: conditionalize schema for commands only supported on Windows
Rather than creating stubs for every command that just return
QERR_UNSUPPORTED, use 'if' conditions in the QAPI schema to
fully exclude generation of the commands on non-Windows.
The command will be rejected at QMP dispatch time instead,
avoiding reimplementing rejection by blocking the stub commands.
This changes the error message for affected commands from
{"class": "CommandNotFound", "desc": "Command FOO has been disabled"}
to
{"class": "CommandNotFound", "desc": "The command FOO has not been found"}
This has the additional benefit that the QGA protocol reference
now documents what conditions enable use of the command.
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Konstantin Kostiuk
Signed-off-by: Daniel P. Berrangé
---
qga/commands-posix.c | 9 -
qga/qapi-schema.json | 15 ++-
2 files changed, 10 insertions(+), 14 deletions(-)
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 838dc3cf98..b7f96aa005 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1207,8 +1207,6 @@ GList *ga_command_init_blockedrpcs(GList *blockedrpcs)
blockedrpcs = g_list_append(blockedrpcs, g_strdup("guest-fstrim"));
#endif
-blockedrpcs = g_list_append(blockedrpcs, g_strdup("guest-get-devices"));
-
return blockedrpcs;
}
@@ -1419,13 +1417,6 @@ GuestOSInfo *qmp_guest_get_osinfo(Error **errp)
return info;
}
-GuestDeviceInfoList *qmp_guest_get_devices(Error **errp)
-{
-error_setg(errp, QERR_UNSUPPORTED);
-
-return NULL;
-}
-
#ifndef HOST_NAME_MAX
# ifdef _POSIX_HOST_NAME_MAX
# define HOST_NAME_MAX _POSIX_HOST_NAME_MAX
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index 9bd5aa53bc..de3fc46d2e 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -1527,7 +1527,8 @@
# @pci: PCI device
##
{ 'enum': 'GuestDeviceType',
- 'data': [ 'pci' ] }
+ 'data': [ 'pci' ],
+ 'if': 'CONFIG_WIN32' }
##
# @GuestDeviceIdPCI:
@@ -1539,7 +1540,8 @@
# Since: 5.2
##
{ 'struct': 'GuestDeviceIdPCI',
- 'data': { 'vendor-id': 'uint16', 'device-id': 'uint16' } }
+ 'data': { 'vendor-id': 'uint16', 'device-id': 'uint16' },
+ 'if': 'CONFIG_WIN32' }
##
# @GuestDeviceId:
@@ -1553,7 +1555,8 @@
{ 'union': 'GuestDeviceId',
'base': { 'type': 'GuestDeviceType' },
'discriminator': 'type',
- 'data': { 'pci': 'GuestDeviceIdPCI' } }
+ 'data': { 'pci': 'GuestDeviceIdPCI' },
+ 'if': 'CONFIG_WIN32' }
##
# @GuestDeviceInfo:
@@ -1574,7 +1577,8 @@
'*driver-date': 'int',
'*driver-version': 'str',
'*id': 'GuestDeviceId'
- } }
+ },
+ 'if': 'CONFIG_WIN32' }
##
# @guest-get-devices:
@@ -1586,7 +1590,8 @@
# Since: 5.2
##
{ 'command': 'guest-get-devices',
- 'returns': ['GuestDeviceInfo'] }
+ 'returns': ['GuestDeviceInfo'],
+ 'if': 'CONFIG_WIN32' }
##
# @GuestAuthorizedKeys:
--
2.45.1
[PATCH v3 19/22] qga: move declare of QGAConfig struct to top of file
It is referenced by QGAState already, and it is clearer to declare all
data types at the top of the file, rather than have them mixed with
code later.
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Konstantin Kostiuk
Signed-off-by: Daniel P. Berrangé
---
qga/main.c | 44 ++--
1 file changed, 22 insertions(+), 22 deletions(-)
diff --git a/qga/main.c b/qga/main.c
index 17b6ce18ac..647d27037c 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -70,6 +70,28 @@ typedef struct GAPersistentState {
typedef struct GAConfig GAConfig;
+struct GAConfig {
+char *channel_path;
+char *method;
+char *log_filepath;
+char *pid_filepath;
+#ifdef CONFIG_FSFREEZE
+char *fsfreeze_hook;
+#endif
+char *state_dir;
+#ifdef _WIN32
+const char *service;
+#endif
+gchar *bliststr; /* blockedrpcs may point to this string */
+gchar *aliststr; /* allowedrpcs may point to this string */
+GList *blockedrpcs;
+GList *allowedrpcs;
+int daemonize;
+GLogLevelFlags log_level;
+int dumpconf;
+bool retry_path;
+};
+
struct GAState {
JSONMessageParser parser;
GMainLoop *main_loop;
@@ -996,28 +1018,6 @@ static GList *split_list(const gchar *str, const gchar
*delim)
return list;
}
-struct GAConfig {
-char *channel_path;
-char *method;
-char *log_filepath;
-char *pid_filepath;
-#ifdef CONFIG_FSFREEZE
-char *fsfreeze_hook;
-#endif
-char *state_dir;
-#ifdef _WIN32
-const char *service;
-#endif
-gchar *bliststr; /* blockedrpcs may point to this string */
-gchar *aliststr; /* allowedrpcs may point to this string */
-GList *blockedrpcs;
-GList *allowedrpcs;
-int daemonize;
-GLogLevelFlags log_level;
-int dumpconf;
-bool retry_path;
-};
-
static void config_load(GAConfig *config)
{
GError *gerr = NULL;
--
2.45.1
[PATCH v3 09/22] qga: conditionalize schema for commands unsupported on non-Linux POSIX
Rather than creating stubs for every command that just return
QERR_UNSUPPORTED, use 'if' conditions in the QAPI schema to
fully exclude generation of the commands on non-Linux POSIX
platforms
The command will be rejected at QMP dispatch time instead,
avoiding reimplementing rejection by blocking the stub commands.
This changes the error message for affected commands from
{"class": "CommandNotFound", "desc": "Command FOO has been disabled"}
to
{"class": "CommandNotFound", "desc": "The command FOO has not been found"}
This has the additional benefit that the QGA protocol reference
now documents what conditions enable use of the command.
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Konstantin Kostiuk
Signed-off-by: Daniel P. Berrangé
---
qga/commands-posix.c | 66
qga/qapi-schema.json | 30 +++-
2 files changed, 17 insertions(+), 79 deletions(-)
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 0dd8555867..559d71ffae 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -887,56 +887,6 @@ void qmp_guest_set_user_password(const char *username,
}
#endif /* __linux__ || __FreeBSD__ */
-#ifndef __linux__
-
-void qmp_guest_suspend_disk(Error **errp)
-{
-error_setg(errp, QERR_UNSUPPORTED);
-}
-
-void qmp_guest_suspend_ram(Error **errp)
-{
-error_setg(errp, QERR_UNSUPPORTED);
-}
-
-void qmp_guest_suspend_hybrid(Error **errp)
-{
-error_setg(errp, QERR_UNSUPPORTED);
-}
-
-GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
-{
-error_setg(errp, QERR_UNSUPPORTED);
-return NULL;
-}
-
-int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
-{
-error_setg(errp, QERR_UNSUPPORTED);
-return -1;
-}
-
-GuestMemoryBlockList *qmp_guest_get_memory_blocks(Error **errp)
-{
-error_setg(errp, QERR_UNSUPPORTED);
-return NULL;
-}
-
-GuestMemoryBlockResponseList *
-qmp_guest_set_memory_blocks(GuestMemoryBlockList *mem_blks, Error **errp)
-{
-error_setg(errp, QERR_UNSUPPORTED);
-return NULL;
-}
-
-GuestMemoryBlockInfo *qmp_guest_get_memory_block_info(Error **errp)
-{
-error_setg(errp, QERR_UNSUPPORTED);
-return NULL;
-}
-
-#endif
-
#ifdef HAVE_GETIFADDRS
static GuestNetworkInterface *
guest_find_interface(GuestNetworkInterfaceList *head,
@@ -1272,22 +1222,6 @@ qmp_guest_fstrim(bool has_minimum, int64_t minimum,
Error **errp)
/* add unsupported commands to the list of blocked RPCs */
GList *ga_command_init_blockedrpcs(GList *blockedrpcs)
{
-#if !defined(__linux__)
-{
-const char *list[] = {
-"guest-suspend-disk", "guest-suspend-ram",
-"guest-suspend-hybrid", "guest-get-vcpus", "guest-set-vcpus",
-"guest-get-memory-blocks", "guest-set-memory-blocks",
-"guest-get-memory-block-info",
-NULL};
-const char **p = list;
-
-while (*p) {
-blockedrpcs = g_list_append(blockedrpcs, g_strdup(*p++));
-}
-}
-#endif
-
#if !defined(HAVE_GETIFADDRS)
blockedrpcs = g_list_append(blockedrpcs,
g_strdup("guest-network-get-interfaces"));
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index 2f0215afc7..38483652ac 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -566,7 +566,8 @@
#
# Since: 1.1
##
-{ 'command': 'guest-suspend-disk', 'success-response': false }
+{ 'command': 'guest-suspend-disk', 'success-response': false,
+ 'if': { 'any': ['CONFIG_LINUX', 'CONFIG_WIN32'] } }
##
# @guest-suspend-ram:
@@ -602,7 +603,8 @@
#
# Since: 1.1
##
-{ 'command': 'guest-suspend-ram', 'success-response': false }
+{ 'command': 'guest-suspend-ram', 'success-response': false,
+ 'if': { 'any': ['CONFIG_LINUX', 'CONFIG_WIN32'] } }
##
# @guest-suspend-hybrid:
@@ -638,7 +640,7 @@
# Since: 1.1
##
{ 'command': 'guest-suspend-hybrid', 'success-response': false,
- 'if': 'CONFIG_POSIX' }
+ 'if': 'CONFIG_LINUX' }
##
# @GuestIpAddressType:
@@ -751,7 +753,8 @@
{ 'struct': 'GuestLogicalProcessor',
'data': {'logical-id': 'int',
'online': 'bool',
- '*can-offline': 'bool'} }
+ '*can-offline': 'bool'},
+ 'if': { 'any': ['CONFIG_LINUX', 'CONFIG_WIN32'] } }
##
# @guest-get-vcpus:
@@ -766,7 +769,8 @@
# Since: 1.5
##
{ 'command': 'guest-get-vcpus',
- 'returns': ['GuestLogicalProcessor'] }
+ 'returns': ['GuestLogicalProcessor'],
+ 'if': { 'any': ['CONFIG_LINUX', 'CONFIG_WIN32'] } }
##
# @guest-set-vcpus:
@@ -809,7 +813,7 @@
{ 'command': 'guest-set-vcpus',
'data':{'vcpus': ['GuestLogicalProcessor'] },
'returns': 'int',
- 'if': 'CONFIG_POSIX' }
+ 'if': 'CONFIG_LINUX' }
##
# @GuestDiskBusType:
@@ -1103,7 +1107,7 @@
'data': {'phys-index': 'uint64',
'online': 'bool',
[PATCH v3 03/22] qga: move linux suspend command impls to commands-linux.c
The qmp_guest_suspend_{disk,ram,hybrid} command impls in
commands-posix.c are surrounded by '#ifdef __linux__' so should
instead live in commands-linux.c
Reviewed-by: Manos Pitsidianakis
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Konstantin Kostiuk
Signed-off-by: Daniel P. Berrangé
---
qga/commands-linux.c | 265 +++
qga/commands-posix.c | 265 ---
2 files changed, 265 insertions(+), 265 deletions(-)
diff --git a/qga/commands-linux.c b/qga/commands-linux.c
index 78580ac39d..3fabf54882 100644
--- a/qga/commands-linux.c
+++ b/qga/commands-linux.c
@@ -286,6 +286,271 @@ int qmp_guest_fsfreeze_do_thaw(Error **errp)
}
#endif /* CONFIG_FSFREEZE */
+
+#define LINUX_SYS_STATE_FILE "/sys/power/state"
+#define SUSPEND_SUPPORTED 0
+#define SUSPEND_NOT_SUPPORTED 1
+
+typedef enum {
+SUSPEND_MODE_DISK = 0,
+SUSPEND_MODE_RAM = 1,
+SUSPEND_MODE_HYBRID = 2,
+} SuspendMode;
+
+/*
+ * Executes a command in a child process using g_spawn_sync,
+ * returning an int >= 0 representing the exit status of the
+ * process.
+ *
+ * If the program wasn't found in path, returns -1.
+ *
+ * If a problem happened when creating the child process,
+ * returns -1 and errp is set.
+ */
+static int run_process_child(const char *command[], Error **errp)
+{
+int exit_status, spawn_flag;
+GError *g_err = NULL;
+bool success;
+
+spawn_flag = G_SPAWN_SEARCH_PATH | G_SPAWN_STDOUT_TO_DEV_NULL |
+ G_SPAWN_STDERR_TO_DEV_NULL;
+
+success = g_spawn_sync(NULL, (char **)command, NULL, spawn_flag,
+NULL, NULL, NULL, NULL,
+_status, _err);
+
+if (success) {
+return WEXITSTATUS(exit_status);
+}
+
+if (g_err && (g_err->code != G_SPAWN_ERROR_NOENT)) {
+error_setg(errp, "failed to create child process, error '%s'",
+ g_err->message);
+}
+
+g_error_free(g_err);
+return -1;
+}
+
+static bool systemd_supports_mode(SuspendMode mode, Error **errp)
+{
+const char *systemctl_args[3] = {"systemd-hibernate", "systemd-suspend",
+ "systemd-hybrid-sleep"};
+const char *cmd[4] = {"systemctl", "status", systemctl_args[mode], NULL};
+int status;
+
+status = run_process_child(cmd, errp);
+
+/*
+ * systemctl status uses LSB return codes so we can expect
+ * status > 0 and be ok. To assert if the guest has support
+ * for the selected suspend mode, status should be < 4. 4 is
+ * the code for unknown service status, the return value when
+ * the service does not exist. A common value is status = 3
+ * (program is not running).
+ */
+if (status > 0 && status < 4) {
+return true;
+}
+
+return false;
+}
+
+static void systemd_suspend(SuspendMode mode, Error **errp)
+{
+Error *local_err = NULL;
+const char *systemctl_args[3] = {"hibernate", "suspend", "hybrid-sleep"};
+const char *cmd[3] = {"systemctl", systemctl_args[mode], NULL};
+int status;
+
+status = run_process_child(cmd, _err);
+
+if (status == 0) {
+return;
+}
+
+if ((status == -1) && !local_err) {
+error_setg(errp, "the helper program 'systemctl %s' was not found",
+ systemctl_args[mode]);
+return;
+}
+
+if (local_err) {
+error_propagate(errp, local_err);
+} else {
+error_setg(errp, "the helper program 'systemctl %s' returned an "
+ "unexpected exit status code (%d)",
+ systemctl_args[mode], status);
+}
+}
+
+static bool pmutils_supports_mode(SuspendMode mode, Error **errp)
+{
+Error *local_err = NULL;
+const char *pmutils_args[3] = {"--hibernate", "--suspend",
+ "--suspend-hybrid"};
+const char *cmd[3] = {"pm-is-supported", pmutils_args[mode], NULL};
+int status;
+
+status = run_process_child(cmd, _err);
+
+if (status == SUSPEND_SUPPORTED) {
+return true;
+}
+
+if ((status == -1) && !local_err) {
+return false;
+}
+
+if (local_err) {
+error_propagate(errp, local_err);
+} else {
+error_setg(errp,
+ "the helper program '%s' returned an unexpected exit"
+ " status code (%d)", "pm-is-supported", status);
+}
+
+return false;
+}
+
+static void pmutils_suspend(SuspendMode mode, Error **errp)
+{
+Error *local_err = NULL;
+const char *pmutils_binaries[3] = {"pm-hibernate", "pm-suspend",
+ "pm-suspend-hybrid"};
+const char *cmd[2] = {pm
[PATCH v3 04/22] qga: move linux fs/disk command impls to commands-linux.c
The qmp_guest_{fstrim, get_fsinfo, get_disks} command impls in
commands-posix.c are surrounded by '#ifdef __linux__' so should
instead live in commands-linux.c
Reviewed-by: Manos Pitsidianakis
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Konstantin Kostiuk
Signed-off-by: Daniel P. Berrangé
---
qga/commands-linux.c | 904 ++
qga/commands-posix.c | 909 ---
2 files changed, 904 insertions(+), 909 deletions(-)
diff --git a/qga/commands-linux.c b/qga/commands-linux.c
index 3fabf54882..084e6c9e85 100644
--- a/qga/commands-linux.c
+++ b/qga/commands-linux.c
@@ -14,10 +14,21 @@
#include "qemu/osdep.h"
#include "qapi/error.h"
#include "qga-qapi-commands.h"
+#include "qapi/error.h"
+#include "qapi/qmp/qerror.h"
#include "commands-common.h"
#include "cutils.h"
#include
#include
+#include
+#include
+#include "block/nvme.h"
+
+#ifdef CONFIG_LIBUDEV
+#include
+#endif
+
+#include
#if defined(CONFIG_FSFREEZE) || defined(CONFIG_FSTRIM)
static int dev_major_minor(const char *devpath,
@@ -286,6 +297,899 @@ int qmp_guest_fsfreeze_do_thaw(Error **errp)
}
#endif /* CONFIG_FSFREEZE */
+#if defined(CONFIG_FSFREEZE)
+
+static char *get_pci_driver(char const *syspath, int pathlen, Error **errp)
+{
+char *path;
+char *dpath;
+char *driver = NULL;
+char buf[PATH_MAX];
+ssize_t len;
+
+path = g_strndup(syspath, pathlen);
+dpath = g_strdup_printf("%s/driver", path);
+len = readlink(dpath, buf, sizeof(buf) - 1);
+if (len != -1) {
+buf[len] = 0;
+driver = g_path_get_basename(buf);
+}
+g_free(dpath);
+g_free(path);
+return driver;
+}
+
+static int compare_uint(const void *_a, const void *_b)
+{
+unsigned int a = *(unsigned int *)_a;
+unsigned int b = *(unsigned int *)_b;
+
+return a < b ? -1 : a > b ? 1 : 0;
+}
+
+/* Walk the specified sysfs and build a sorted list of host or ata numbers */
+static int build_hosts(char const *syspath, char const *host, bool ata,
+ unsigned int *hosts, int hosts_max, Error **errp)
+{
+char *path;
+DIR *dir;
+struct dirent *entry;
+int i = 0;
+
+path = g_strndup(syspath, host - syspath);
+dir = opendir(path);
+if (!dir) {
+error_setg_errno(errp, errno, "opendir(\"%s\")", path);
+g_free(path);
+return -1;
+}
+
+while (i < hosts_max) {
+entry = readdir(dir);
+if (!entry) {
+break;
+}
+if (ata && sscanf(entry->d_name, "ata%d", hosts + i) == 1) {
+++i;
+} else if (!ata && sscanf(entry->d_name, "host%d", hosts + i) == 1) {
+++i;
+}
+}
+
+qsort(hosts, i, sizeof(hosts[0]), compare_uint);
+
+g_free(path);
+closedir(dir);
+return i;
+}
+
+/*
+ * Store disk device info for devices on the PCI bus.
+ * Returns true if information has been stored, or false for failure.
+ */
+static bool build_guest_fsinfo_for_pci_dev(char const *syspath,
+ GuestDiskAddress *disk,
+ Error **errp)
+{
+unsigned int pci[4], host, hosts[8], tgt[3];
+int i, nhosts = 0, pcilen;
+GuestPCIAddress *pciaddr = disk->pci_controller;
+bool has_ata = false, has_host = false, has_tgt = false;
+char *p, *q, *driver = NULL;
+bool ret = false;
+
+p = strstr(syspath, "/devices/pci");
+if (!p || sscanf(p + 12, "%*x:%*x/%x:%x:%x.%x%n",
+ pci, pci + 1, pci + 2, pci + 3, ) < 4) {
+g_debug("only pci device is supported: sysfs path '%s'", syspath);
+return false;
+}
+
+p += 12 + pcilen;
+while (true) {
+driver = get_pci_driver(syspath, p - syspath, errp);
+if (driver && (g_str_equal(driver, "ata_piix") ||
+ g_str_equal(driver, "sym53c8xx") ||
+ g_str_equal(driver, "virtio-pci") ||
+ g_str_equal(driver, "ahci") ||
+ g_str_equal(driver, "nvme") ||
+ g_str_equal(driver, "xhci_hcd") ||
+ g_str_equal(driver, "ehci-pci"))) {
+break;
+}
+
+g_free(driver);
+if (sscanf(p, "/%x:%x:%x.%x%n",
+ pci, pci + 1, pci + 2, pci + 3, ) == 4) {
+p += pcilen;
+continue;
+}
+
+g_debug("unsupported driver or sysfs path '%s'", syspath);
+return false;
+}
+
+p = strstr(syspath, "/target");
+if (p && sscanf(p + 7, "%*u:%*u:%*u/%*u:%u:%u:%u",
+tgt, tgt +
[PATCH v3 14/22] qga: conditionalize schema for commands requiring fstrim
Rather than creating stubs for every command that just return
QERR_UNSUPPORTED, use 'if' conditions in the QAPI schema to
fully exclude generation of the filesystem trimming commands
on POSIX platforms lacking required APIs.
The command will be rejected at QMP dispatch time instead,
avoiding reimplementing rejection by blocking the stub commands.
This changes the error message for affected commands from
{"class": "CommandNotFound", "desc": "Command FOO has been disabled"}
to
{"class": "CommandNotFound", "desc": "The command FOO has not been found"}
This has the additional benefit that the QGA protocol reference
now documents what conditions enable use of the command.
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Konstantin Kostiuk
Signed-off-by: Daniel P. Berrangé
---
qga/commands-posix.c | 13 -
qga/qapi-schema.json | 9 ++---
2 files changed, 6 insertions(+), 16 deletions(-)
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 9207cb7a8f..d92fa0ec87 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1144,22 +1144,9 @@ error:
#endif /* HAVE_GETIFADDRS */
-#if !defined(CONFIG_FSTRIM)
-GuestFilesystemTrimResponse *
-qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
-{
-error_setg(errp, QERR_UNSUPPORTED);
-return NULL;
-}
-#endif
-
/* add unsupported commands to the list of blocked RPCs */
GList *ga_command_init_blockedrpcs(GList *blockedrpcs)
{
-#if !defined(CONFIG_FSTRIM)
-blockedrpcs = g_list_append(blockedrpcs, g_strdup("guest-fstrim"));
-#endif
-
return blockedrpcs;
}
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index 62462f092c..21c65d1806 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -510,7 +510,8 @@
##
{ 'struct': 'GuestFilesystemTrimResult',
'data': {'path': 'str',
- '*trimmed': 'int', '*minimum': 'int', '*error': 'str'} }
+ '*trimmed': 'int', '*minimum': 'int', '*error': 'str'},
+ 'if': { 'any': ['CONFIG_WIN32', 'CONFIG_FSTRIM'] } }
##
# @GuestFilesystemTrimResponse:
@@ -520,7 +521,8 @@
# Since: 2.4
##
{ 'struct': 'GuestFilesystemTrimResponse',
- 'data': {'paths': ['GuestFilesystemTrimResult']} }
+ 'data': {'paths': ['GuestFilesystemTrimResult']},
+ 'if': { 'any': ['CONFIG_WIN32', 'CONFIG_FSTRIM'] } }
##
# @guest-fstrim:
@@ -542,7 +544,8 @@
##
{ 'command': 'guest-fstrim',
'data': { '*minimum': 'int' },
- 'returns': 'GuestFilesystemTrimResponse' }
+ 'returns': 'GuestFilesystemTrimResponse',
+ 'if': { 'any': ['CONFIG_WIN32', 'CONFIG_FSTRIM'] } }
##
# @guest-suspend-disk:
--
2.45.1
[PATCH v3 02/22] qga: move linux vcpu command impls to commands-linux.c
The qmp_guest_set_vcpus and qmp_guest_get_vcpus command impls in
commands-posix.c are surrounded by '#ifdef __linux__' so should
instead live in commands-linux.c
Reviewed-by: Manos Pitsidianakis
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Konstantin Kostiuk
Signed-off-by: Daniel P. Berrangé
---
qga/commands-linux.c | 141 +++
qga/commands-posix.c | 139 --
2 files changed, 141 insertions(+), 139 deletions(-)
diff --git a/qga/commands-linux.c b/qga/commands-linux.c
index 214e408fcd..78580ac39d 100644
--- a/qga/commands-linux.c
+++ b/qga/commands-linux.c
@@ -13,6 +13,7 @@
#include "qemu/osdep.h"
#include "qapi/error.h"
+#include "qga-qapi-commands.h"
#include "commands-common.h"
#include "cutils.h"
#include
@@ -284,3 +285,143 @@ int qmp_guest_fsfreeze_do_thaw(Error **errp)
return i;
}
#endif /* CONFIG_FSFREEZE */
+
+/* Transfer online/offline status between @vcpu and the guest system.
+ *
+ * On input either @errp or *@errp must be NULL.
+ *
+ * In system-to-@vcpu direction, the following @vcpu fields are accessed:
+ * - R: vcpu->logical_id
+ * - W: vcpu->online
+ * - W: vcpu->can_offline
+ *
+ * In @vcpu-to-system direction, the following @vcpu fields are accessed:
+ * - R: vcpu->logical_id
+ * - R: vcpu->online
+ *
+ * Written members remain unmodified on error.
+ */
+static void transfer_vcpu(GuestLogicalProcessor *vcpu, bool sys2vcpu,
+ char *dirpath, Error **errp)
+{
+int fd;
+int res;
+int dirfd;
+static const char fn[] = "online";
+
+dirfd = open(dirpath, O_RDONLY | O_DIRECTORY);
+if (dirfd == -1) {
+error_setg_errno(errp, errno, "open(\"%s\")", dirpath);
+return;
+}
+
+fd = openat(dirfd, fn, sys2vcpu ? O_RDONLY : O_RDWR);
+if (fd == -1) {
+if (errno != ENOENT) {
+error_setg_errno(errp, errno, "open(\"%s/%s\")", dirpath, fn);
+} else if (sys2vcpu) {
+vcpu->online = true;
+vcpu->can_offline = false;
+} else if (!vcpu->online) {
+error_setg(errp, "logical processor #%" PRId64 " can't be "
+ "offlined", vcpu->logical_id);
+} /* otherwise pretend successful re-onlining */
+} else {
+unsigned char status;
+
+res = pread(fd, , 1, 0);
+if (res == -1) {
+error_setg_errno(errp, errno, "pread(\"%s/%s\")", dirpath, fn);
+} else if (res == 0) {
+error_setg(errp, "pread(\"%s/%s\"): unexpected EOF", dirpath,
+ fn);
+} else if (sys2vcpu) {
+vcpu->online = (status != '0');
+vcpu->can_offline = true;
+} else if (vcpu->online != (status != '0')) {
+status = '0' + vcpu->online;
+if (pwrite(fd, , 1, 0) == -1) {
+error_setg_errno(errp, errno, "pwrite(\"%s/%s\")", dirpath,
+ fn);
+}
+} /* otherwise pretend successful re-(on|off)-lining */
+
+res = close(fd);
+g_assert(res == 0);
+}
+
+res = close(dirfd);
+g_assert(res == 0);
+}
+
+GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
+{
+GuestLogicalProcessorList *head, **tail;
+const char *cpu_dir = "/sys/devices/system/cpu";
+const gchar *line;
+g_autoptr(GDir) cpu_gdir = NULL;
+Error *local_err = NULL;
+
+head = NULL;
+tail =
+cpu_gdir = g_dir_open(cpu_dir, 0, NULL);
+
+if (cpu_gdir == NULL) {
+error_setg_errno(errp, errno, "failed to list entries: %s", cpu_dir);
+return NULL;
+}
+
+while (local_err == NULL && (line = g_dir_read_name(cpu_gdir)) != NULL) {
+GuestLogicalProcessor *vcpu;
+int64_t id;
+if (sscanf(line, "cpu%" PRId64, )) {
+g_autofree char *path = g_strdup_printf("/sys/devices/system/cpu/"
+"cpu%" PRId64 "/", id);
+vcpu = g_malloc0(sizeof *vcpu);
+vcpu->logical_id = id;
+vcpu->has_can_offline = true; /* lolspeak ftw */
+transfer_vcpu(vcpu, true, path, _err);
+QAPI_LIST_APPEND(tail, vcpu);
+}
+}
+
+if (local_err == NULL) {
+/* there's no guest with zero VCPUs */
+g_assert(head != NULL);
+return head;
+}
+
+qapi_free_GuestLogicalProcessorList(head);
+error_propagate(errp, local_err);
+return NULL;
+}
+
+int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
+{
+int64_t processed;
+Error *local_err = NULL;
+
+processed = 0;
+w
[PATCH v3 07/22] qga: move CONFIG_FSFREEZE/TRIM to be meson defined options
Defining these at the meson level allows them to be used a conditional
tests in the QAPI schemas.
Signed-off-by: Daniel P. Berrangé
---
meson.build | 15 +++
qga/commands-common.h | 9 -
2 files changed, 15 insertions(+), 9 deletions(-)
diff --git a/meson.build b/meson.build
index 6a93da48e1..3f125d1b02 100644
--- a/meson.build
+++ b/meson.build
@@ -2186,6 +2186,19 @@ have_virtfs_proxy_helper =
get_option('virtfs_proxy_helper') \
.require(libcap_ng.found(), error_message: 'the virtfs proxy helper
requires libcap-ng') \
.allowed()
+qga_fsfreeze = false
+qga_fstrim = false
+if host_os == 'linux'
+if cc.has_header_symbol('linux/fs.h', 'FIFREEZE')
+qga_fsfreeze = true
+endif
+if cc.has_header_symbol('linux/fs.h', 'FITRIM')
+qga_fstrim = true
+endif
+elif host_os == 'freebsd' and cc.has_header_symbol('ufs/ffs/fs.h',
'UFSSUSPEND')
+qga_fsfreeze = true
+endif
+
if get_option('block_drv_ro_whitelist') == ''
config_host_data.set('CONFIG_BDRV_RO_WHITELIST', '')
else
@@ -2422,6 +2435,8 @@ config_host_data.set('CONFIG_DEBUG_TCG',
get_option('debug_tcg'))
config_host_data.set('CONFIG_DEBUG_REMAP', get_option('debug_remap'))
config_host_data.set('CONFIG_QOM_CAST_DEBUG', get_option('qom_cast_debug'))
config_host_data.set('CONFIG_REPLICATION', get_option('replication').allowed())
+config_host_data.set('CONFIG_FSFREEZE', qga_fsfreeze)
+config_host_data.set('CONFIG_FSTRIM', qga_fstrim)
# has_header
config_host_data.set('CONFIG_EPOLL', cc.has_header('sys/epoll.h'))
diff --git a/qga/commands-common.h b/qga/commands-common.h
index 8c1c56aac9..263e7c0525 100644
--- a/qga/commands-common.h
+++ b/qga/commands-common.h
@@ -15,19 +15,10 @@
#if defined(__linux__)
#include
-#ifdef FIFREEZE
-#define CONFIG_FSFREEZE
-#endif
-#ifdef FITRIM
-#define CONFIG_FSTRIM
-#endif
#endif /* __linux__ */
#ifdef __FreeBSD__
#include
-#ifdef UFSSUSPEND
-#define CONFIG_FSFREEZE
-#endif
#endif /* __FreeBSD__ */
#if defined(CONFIG_FSFREEZE) || defined(CONFIG_FSTRIM)
--
2.45.1
[PATCH v3 01/22] qga: drop blocking of guest-get-memory-block-size command
This command has never existed in tree, since it was renamed to guest-get-memory-block-info before being merged. Reviewed-by: Manos Pitsidianakis Reviewed-by: Konstantin Kostiuk Signed-off-by: Daniel P. Berrangé --- qga/commands-posix.c | 2 +- qga/commands-win32.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/qga/commands-posix.c b/qga/commands-posix.c index 7f05996495..76af98ba32 100644 --- a/qga/commands-posix.c +++ b/qga/commands-posix.c @@ -3099,7 +3099,7 @@ GList *ga_command_init_blockedrpcs(GList *blockedrpcs) "guest-suspend-disk", "guest-suspend-ram", "guest-suspend-hybrid", "guest-get-vcpus", "guest-set-vcpus", "guest-get-memory-blocks", "guest-set-memory-blocks", -"guest-get-memory-block-size", "guest-get-memory-block-info", +"guest-get-memory-block-info", NULL}; char **p = (char **)list; diff --git a/qga/commands-win32.c b/qga/commands-win32.c index 0d1b836e87..9fe670d5b4 100644 --- a/qga/commands-win32.c +++ b/qga/commands-win32.c @@ -1995,7 +1995,7 @@ GList *ga_command_init_blockedrpcs(GList *blockedrpcs) "guest-suspend-hybrid", "guest-set-vcpus", "guest-get-memory-blocks", "guest-set-memory-blocks", -"guest-get-memory-block-size", "guest-get-memory-block-info", +"guest-get-memory-block-info", NULL}; char **p = (char **)list_unsupported; -- 2.45.1
[PATCH v3 00/22] qga: clean up command source locations and conditionals
This series is a side effect of other work I started, to attempt to make the QGA safe to use in confidential VMs by automatically restricting the permitted commands. Since this cleanup stands on its own, I'm sending it now. The QGA codebase has a very complicated maze of #ifdefs to create stubs for the various commands that cannot be implemented on certain platforms. It then has further logic to dynamically disable the stub commands at runtime, except this is not consistently applied, so some commands remain enabled despite being merely stubs. The resulting code is hard to follow, when trying to understand exactly what commands are available under what circumstances, and when changing impls it is easy to get the #ifdefs wrong, resulting in stubs getting missed on platforms without a real impl. In some cases, we have multiple stubs for the same command, due to the maze of #ifdefs. The QAPI schema language has support for many years for expressing conditions against commands when declaring them. This results in the QAPI code generator omitting their implementation entirely at build time. This has mutliple benefits * The unsupported commands are guaranteed to not exist at runtime * No stubs need ever be defined in the code * The generated QAPI reference manual documents the build conditions This series is broadly split into three parts * Moving tonnes of Linux only commands out of commands-posix.c into commands-linux.c to remove many #ifdefs. * Adding 'if' conditions in the QAPI schema to reflect the build conditions, removing many more #ifdefs * Sanitizing the logic for disabling/enabling commands at runtime to guarantee consistency Changed in v3: - Fix missing --help output for new -c / --config arg - Fix typos - Avoid repeated qmp_command_is_enabled call Changed in v2: - Make FSFreeze error reporting distinguish inability to enable VSS from user config choice - Fully remove ga_command_init_blockedrpcs() methods. No more special case disabling of commands. Either they're disabled at build time, or disabled by user config, or by well defined rule ie not permitted during FS freeze. - Apply rules later in startup to avoid crash from NULL config pointer - Document changed error messages in commit messages - Add -c / --config command line parameter - Fix mistaken enabling of fsfreeze hooks on win32 Daniel P. Berrangé (22): qga: drop blocking of guest-get-memory-block-size command qga: move linux vcpu command impls to commands-linux.c qga: move linux suspend command impls to commands-linux.c qga: move linux fs/disk command impls to commands-linux.c qga: move linux disk/cpu stats command impls to commands-linux.c qga: move linux memory block command impls to commands-linux.c qga: move CONFIG_FSFREEZE/TRIM to be meson defined options qga: conditionalize schema for commands unsupported on Windows qga: conditionalize schema for commands unsupported on non-Linux POSIX qga: conditionalize schema for commands requiring getifaddrs qga: conditionalize schema for commands requiring linux/win32 qga: conditionalize schema for commands only supported on Windows qga: conditionalize schema for commands requiring fsfreeze qga: conditionalize schema for commands requiring fstrim qga: conditionalize schema for commands requiring libudev qga: conditionalize schema for commands requiring utmpx qga: conditionalize schema for commands not supported on other UNIX qga: don't disable fsfreeze commands if vss_init fails qga: move declare of QGAConfig struct to top of file qga: remove pointless 'blockrpcs_key' variable qga: allow configuration file path via the cli qga: centralize logic for disabling/enabling commands docs/interop/qemu-ga.rst | 19 + meson.build | 16 + qga/commands-bsd.c | 24 - qga/commands-common.h|9 - qga/commands-linux.c | 1805 + qga/commands-posix.c | 2373 +++--- qga/commands-win32.c | 78 +- qga/main.c | 224 ++-- qga/qapi-schema.json | 153 ++- 9 files changed, 2240 insertions(+), 2461 deletions(-) -- 2.45.1
Re: [PATCH v2 22/22] qga: centralize logic for disabling/enabling commands
On Wed, Jul 03, 2024 at 01:01:11PM +0300, Manos Pitsidianakis wrote:
> Hello Daniel,
>
> This cleanup seems like a good idea,
>
> On Thu, 13 Jun 2024 18:44, "Daniel P. Berrangé" wrote:
> > It is confusing having many different pieces of code enabling and
> > disabling commands, and it is not clear that they all have the same
> > semantics, especially wrt prioritization of the block/allow lists.
> > The code attempted to prevent the user from setting both the block
> > and allow lists concurrently, however, the logic was flawed as it
> > checked settings in the configuration file separately from the
> > command line arguments. Thus it was possible to set a block list
> > in the config file and an allow list via a command line argument.
> > The --dump-conf option also creates a configuration file with both
> > keys present, even if unset, which means it is creating a config
> > that cannot actually be loaded again.
> >
> > Centralizing the code in a single method "ga_apply_command_filters"
> > will provide a strong guarantee of consistency and clarify the
> > intended behaviour. With this there is no compelling technical
> > reason to prevent concurrent setting of both the allow and block
> > lists, so this flawed restriction is removed.
> >
> > Signed-off-by: Daniel P. Berrangé
> > ---
> > docs/interop/qemu-ga.rst | 14 +
> > qga/commands-posix.c | 6 --
> > qga/commands-win32.c | 6 --
> > qga/main.c | 128 +--
> > 4 files changed, 70 insertions(+), 84 deletions(-)
> > diff --git a/qga/main.c b/qga/main.c
> > index f68a32bf7b..72c16fead8 100644
> > --- a/qga/main.c
> > +++ b/qga/main.c
> > @@ -419,60 +419,79 @@ static gint ga_strcmp(gconstpointer str1,
> > gconstpointer str2)
> > return strcmp(str1, str2);
> > }
> >
> > -/* disable commands that aren't safe for fsfreeze */
> > -static void ga_disable_not_allowed_freeze(const QmpCommand *cmd, void
> > *opaque)
> > +static bool ga_command_is_allowed(const QmpCommand *cmd, GAState *state)
> > {
> > -bool allowed = false;
> > int i = 0;
> > +GAConfig *config = state->config;
> > const char *name = qmp_command_name(cmd);
> > +/* Fallback policy is allow everything */
> > +bool allowed = true;
> >
> > -while (ga_freeze_allowlist[i] != NULL) {
> > -if (strcmp(name, ga_freeze_allowlist[i]) == 0) {
> > +if (config->allowedrpcs) {
> > +/*
> > + * If an allow-list is given, this changes the fallback
> > + * policy to deny everything
> > + */
> > +allowed = false;
> > +
> > +if (g_list_find_custom(config->allowedrpcs, name, ga_strcmp) !=
> > NULL) {
> > allowed = true;
> > }
> > -i++;
> > }
> > -if (!allowed) {
> > -g_debug("disabling command: %s", name);
> > -qmp_disable_command(_commands, name, "the agent is in frozen
> > state");
> > -}
> > -}
> >
> > -/* [re-]enable all commands, except those explicitly blocked by user */
> > -static void ga_enable_non_blocked(const QmpCommand *cmd, void *opaque)
> > -{
> > -GAState *s = opaque;
> > -GList *blockedrpcs = s->blockedrpcs;
> > -GList *allowedrpcs = s->allowedrpcs;
> > -const char *name = qmp_command_name(cmd);
> > -
> > -if (g_list_find_custom(blockedrpcs, name, ga_strcmp) == NULL) {
> > -if (qmp_command_is_enabled(cmd)) {
> > -return;
> > +/*
> > + * If both allowedrpcs and blockedrpcs are set, the blocked
> > + * list will take priority
> > + */
> > +if (config->blockedrpcs) {
> > +if (g_list_find_custom(config->blockedrpcs, name, ga_strcmp) !=
> > NULL) {
> > +allowed = false;
> > }
> > +}
> >
> > -if (allowedrpcs &&
> > -g_list_find_custom(allowedrpcs, name, ga_strcmp) == NULL) {
> > -return;
> > -}
> > +/*
> > + * If frozen, this filtering must take priority over
> > + * absolutely everything
> > + */
> > +if (state->frozen) {
> > +allowed = false;
> >
> > -g_debug("enabling command: %s", name);
> > -qmp_enable_command(_commands, name);
> > +while (ga_freeze_allowlist[i] != NULL) {
> > +
Re: [PATCH v2 0/9] RISC-V: ACPI: Namespace updates
On Fri, Jul 12, 2024 at 02:43:19PM +0200, Igor Mammedov wrote: > On Mon, 8 Jul 2024 17:17:32 +0530 > Sunil V L wrote: > > > This series adds few updates to RISC-V ACPI namespace for virt platform. > > Additionally, it has patches to enable ACPI table testing for RISC-V. > > > > 1) PCI Link devices need to be created outside the scope of the PCI root > > complex to ensure correct probe ordering by the OS. This matches the > > example given in ACPI spec as well. > > > > 2) Add PLIC and APLIC as platform devices as well to ensure probing > > order as per BRS spec [1] requirement. > > > > 3) BRS spec requires RISC-V to use new ACPI ID for the generic UART. So, > > update the HID of the UART. > > > > 4) Enabled ACPI tables tests for RISC-V which were originally part of > > [2] but couldn't get merged due to updates required in the expected AML > > files. I think combining those patches with this series makes it easier > > to merge since expected AML files are updated. > > > > [1] - https://github.com/riscv-non-isa/riscv-brs > > [2] - https://lists.gnu.org/archive/html/qemu-devel/2024-06/msg04734.html > > btw: CI is not happy about series, see: > https://gitlab.com/imammedo/qemu/-/pipelines/1371119552 > also 'cross-i686-tci' job routinely timeouts on bios-tables-test > but we still keep adding more tests to it. > We should either bump timeout to account for slowness or > disable bios-tables-test for that job. Asumming the test is functionally correct, and not hanging, then bumping the timeout is the right answer. You can do this in the meson.build file We should never disable tests only in CI, because non-CI users are just as likely to hit timeouts. 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 v2 18/22] qga: don't disable fsfreeze commands if vss_init fails
On Wed, Jul 03, 2024 at 01:21:29PM +0300, Manos Pitsidianakis wrote:
> On Thu, 13 Jun 2024 18:44, "Daniel P. Berrangé" wrote:
> > The fsfreeze commands are already written to report an error if
> > vss_init() fails. Reporting a more specific error message is more
> > helpful than a generic "command is disabled" message, which cannot
> > beteween an admin config decision and lack of platform support.
>
> s/beteween/between
>
> >
> > Signed-off-by: Daniel P. Berrangé
> > ---
> > qga/commands-win32.c | 18 +++---
> > qga/main.c | 4
> > 2 files changed, 7 insertions(+), 15 deletions(-)
> >
> > diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> > index 2533e4c748..5866cc2e3c 100644
> > --- a/qga/commands-win32.c
> > +++ b/qga/commands-win32.c
> > @@ -1203,7 +1203,7 @@ GuestFilesystemInfoList *qmp_guest_get_fsinfo(Error
> > **errp)
> > GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **errp)
> > {
> > if (!vss_initialized()) {
> > -error_setg(errp, QERR_UNSUPPORTED);
> > +error_setg(errp, "fsfreeze not possible as VSS failed to
> > initialize");
> > return 0;
>
> Should this be return -1 by the way?
This method has to return GuestFsfreezeStatus and
-1 isn't valid. Not a problem though, as the
QAPI code will check for *errp != NULL and not
consider the return value for error detection.
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: [RFC PATCH 7/8] tests/pytest: Add a function for extracting files from an archive
On Fri, Jul 12, 2024 at 01:52:03PM +0200, Thomas Huth wrote: > On 12/07/2024 11.14, Daniel P. Berrangé wrote: > > On Thu, Jul 11, 2024 at 01:55:45PM +0200, Thomas Huth wrote: > > > Some Avocado-based tests use the "archive" module from avocado.utils > > > to extract files from an archive. To be able to use these tests > > > without Avocado, we have to provide our own function for extracting > > > files. Fortunately, there is already the tarfile module that will > > > provide us with this functionality, so let's just add a nice wrapper > > > function around that. > > > > > > Signed-off-by: Thomas Huth > > > --- > > > tests/pytest/qemu_pytest/utils.py | 21 + > > > 1 file changed, 21 insertions(+) > > > create mode 100644 tests/pytest/qemu_pytest/utils.py > > > > > > diff --git a/tests/pytest/qemu_pytest/utils.py > > > b/tests/pytest/qemu_pytest/utils.py > > > new file mode 100644 > > > index 00..4eb5e5d5e5 > > > --- /dev/null > > > +++ b/tests/pytest/qemu_pytest/utils.py > > > @@ -0,0 +1,21 @@ > > > +# Utilities for python-based QEMU tests > > > +# > > > +# Copyright 2024 Red Hat, Inc. > > > +# > > > +# Authors: > > > +# Thomas Huth > > > +# > > > +# This work is licensed under the terms of the GNU GPL, version 2 or > > > +# later. See the COPYING file in the top-level directory. > > > + > > > +import tarfile > > > + > > > +def archive_extract(archive, dest_dir, member=None): > > > +with tarfile.open(archive) as tf: > > > +if hasattr(tarfile, 'data_filter'): > > > > Not convinced this is still needed. The python docs don't say anything > > about 'data_filter' being introduced after 3.0, so can likely > > assume it always exists. > > According to https://docs.python.org/3/library/tarfile.html : > > "Extraction filters were added to Python 3.12, but may be backported to > older versions as security updates. To check whether the feature is > available, use e.g. hasattr(tarfile, 'data_filter') rather than checking the > Python version." > > And it seems to be missing in Python 3.7, indeed: > > https://docs.python.org/3.7/library/tarfile.html > > So as long as we still support this old version, I think I've got to keep > this check. Opps, yes, I missed the docs. The version info is against the top heading, not repeated against each method. 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: [RFC PATCH 4/8] tests/pytest: add pytest to the meson build system
On Fri, Jul 12, 2024 at 12:14:45PM +0200, Thomas Huth wrote: > On 12/07/2024 11.01, Daniel P. Berrangé wrote: > > On Thu, Jul 11, 2024 at 01:55:42PM +0200, Thomas Huth wrote: > > > From: Ani Sinha > > > > > > Integrate the pytest framework with the meson build system. This > > > will make meson run all the pytests under the pytest directory. > > > > Lets add a note about the compelling benefit of this new approach > > > >With this change, each functional test becomes subject > >to an individual execution timeout, defaulting to 60 > >seconds, but overridable per-test. > > The avocado runner uses timeouts, too, so it's not really an additional > benefit that we get here. > > > For CI purposes we'll need to add 'python3-pytest' to > > tests/lcitool/projects/qemu.yml, and re-generate the > > the dockerfiles. Some of the other non-gitlab CI > > integrations probably need manual additions of pytest > > packages. > > I'm currently rather looking into getting rid of pytest and to use pycotap > instead: Using the TAP protocol for running the tests, you get a much nicer > output from the meson test runner, which can then count the subtests and > properly report SKIPs for tests that have not been run. I've just looked at pycotap and IIUC, there's no command line tool equivalent to '/usr/bin/pytest' at all. Each test case is expected to provide a stub for "__main__" to invoke the tests. As such each individual test is directly executable. This meshes nicely with what I'd suggested as changes in patch 1, and eliminating the intermediate runner process is a nice further simplification. So I'll be interested to see your next version using pycotap. 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: [RFC PATCH 4/8] tests/pytest: add pytest to the meson build system
On Fri, Jul 12, 2024 at 12:14:45PM +0200, Thomas Huth wrote: > On 12/07/2024 11.01, Daniel P. Berrangé wrote: > > On Thu, Jul 11, 2024 at 01:55:42PM +0200, Thomas Huth wrote: > > > From: Ani Sinha > > > > > > Integrate the pytest framework with the meson build system. This > > > will make meson run all the pytests under the pytest directory. > > > > Lets add a note about the compelling benefit of this new approach > > > >With this change, each functional test becomes subject > >to an individual execution timeout, defaulting to 60 > >seconds, but overridable per-test. > > The avocado runner uses timeouts, too, so it's not really an additional > benefit that we get here. At the meson level though, we can't put an overall cap on the execution time, as there's only 1 huge test visible, and thus the meson timeout multiplier also won't get honoured IIUC. > > > For CI purposes we'll need to add 'python3-pytest' to > > tests/lcitool/projects/qemu.yml, and re-generate the > > the dockerfiles. Some of the other non-gitlab CI > > integrations probably need manual additions of pytest > > packages. > > I'm currently rather looking into getting rid of pytest and to use pycotap > instead: Using the TAP protocol for running the tests, you get a much nicer > output from the meson test runner, which can then count the subtests and > properly report SKIPs for tests that have not been run. > > > > diff --git a/tests/Makefile.include b/tests/Makefile.include > > > index d39d5dd6a4..68151717d7 100644 > > > --- a/tests/Makefile.include > > > +++ b/tests/Makefile.include > > > @@ -3,12 +3,14 @@ > > > .PHONY: check-help > > > check-help: > > > @echo "Regression testing targets:" > > > - @echo " $(MAKE) check Run block, qapi-schema, unit, > > > softfloat, qtest and decodetree tests" > > > + @echo " $(MAKE) check Run block, qapi-schema, unit, > > > softfloat, qtest, pytest and decodetree tests" > > > @echo " $(MAKE) bench Run speed tests" > > > @echo > > > @echo "Individual test suites:" > > > @echo " $(MAKE) check-qtest-TARGET Run qtest tests for > > > given target" > > > @echo " $(MAKE) check-qtestRun qtest tests" > > > + @echo " $(MAKE) check-pytest Run pytest tests" > > > + @echo " $(MAKE) check-pytest-TARGETRun pytest for a given target" > > > > Or name it after the type of test rather than harness ? > > > > eg check-functional / check-functional-TARGET > > > > For that matter perhaps also for the dir name ? > > > > tests/functional/*.py > > I almost expected that discussion again ... (see > https://lists.gnu.org/archive/html/qemu-devel/2021-05/msg06553.html ) ... > last time we couldn't really agree on such a name and decided to go with the > name of the framework... > > I agree that "pytest" is likely not the best name here, especially if > switching to the pycotap test runner instead of using the "pytest" program, > but "functional" might trigger the same discussion again as last time ... > should it rather be "functional" or "validation" or "integration" etc.? IMHO you can just make an executive decision and pick one of those three. None of them are terrible, any would be a valid choice. > Maybe best if we come up with a new fictional name for the "new" test > framework... something like "pyqe" - PYthon-based Qemu test Environment"? > ... could be considered as a play on the word "pike", too, i.e. something > that makes sure that not everything gets in ... ? WDYT? A wierd acronym isn't really going to tell contributors anything about what this is, compared to calling it 'functional' or 'integration', etc. 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: [RFC PATCH] build deps: update lcitool to include rust bits
On Wed, Jul 10, 2024 at 04:43:35PM +0100, Alex Bennée wrote: > For rust development we need cargo, rustc and bindgen in our various > development environments. Update the libvirt-ci project to (!495) and > regenerate the containers and other dependency lists. > > Signed-off-by: Alex Bennée > > --- > NB: > - this is currently waiting on the upstream MR, but if you manually > add the remote > https://gitlab.com/stsquad/libvirt-ci/-/tree/more-rust-mappings the > submodule update will work. > --- > .gitlab-ci.d/cirrus/freebsd-13.vars | 2 +- > .gitlab-ci.d/cirrus/macos-13.vars | 2 +- > .gitlab-ci.d/cirrus/macos-14.vars | 2 +- > scripts/ci/setup/ubuntu/ubuntu-2204-aarch64.yaml | 3 +++ > scripts/ci/setup/ubuntu/ubuntu-2204-s390x.yaml| 3 +++ > tests/docker/dockerfiles/alpine.docker| 3 +++ > tests/docker/dockerfiles/centos9.docker | 3 +++ > tests/docker/dockerfiles/debian-amd64-cross.docker| 4 > tests/docker/dockerfiles/debian-arm64-cross.docker| 4 > tests/docker/dockerfiles/debian-armel-cross.docker| 4 > tests/docker/dockerfiles/debian-armhf-cross.docker| 4 > tests/docker/dockerfiles/debian-i686-cross.docker | 4 > tests/docker/dockerfiles/debian-mips64el-cross.docker | 4 > tests/docker/dockerfiles/debian-mipsel-cross.docker | 4 > tests/docker/dockerfiles/debian-ppc64el-cross.docker | 4 > tests/docker/dockerfiles/debian-s390x-cross.docker| 4 > tests/docker/dockerfiles/debian.docker| 3 +++ > tests/docker/dockerfiles/fedora-win64-cross.docker| 3 +++ > tests/docker/dockerfiles/fedora.docker| 3 +++ > tests/docker/dockerfiles/opensuse-leap.docker | 2 ++ > tests/docker/dockerfiles/ubuntu2204.docker| 3 +++ > tests/lcitool/libvirt-ci | 2 +- > tests/lcitool/projects/qemu.yml | 3 +++ > tests/vm/generated/freebsd.json | 2 ++ > 24 files changed, 71 insertions(+), 4 deletions(-) Reviewed-by: Daniel P. Berrangé 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 v2 21/22] qga: allow configuration file path via the cli
On Fri, Jul 12, 2024 at 12:05:23PM +0300, Konstantin Kostiuk wrote: > On Thu, Jun 13, 2024 at 6:45 PM Daniel P. Berrangé > wrote: > > > Allowing the user to set the QGA_CONF environment variable to change > > the default configuration file path is very unusual practice, made > > more obscure since this ability is not documented. > > > > This introduces the more normal '-c PATH' / '--config=PATH' command > > line argument approach. This requires that we parse the comamnd line > > twice, since we want the command line arguments to take priority over > > the configuration file settings in general. > > > > Signed-off-by: Daniel P. Berrangé > > --- > > docs/interop/qemu-ga.rst | 5 + > > qga/main.c | 35 +++ > > 2 files changed, 32 insertions(+), 8 deletions(-) > > > > diff --git a/docs/interop/qemu-ga.rst b/docs/interop/qemu-ga.rst > > index 72fb75a6f5..e42b370319 100644 > > --- a/docs/interop/qemu-ga.rst > > +++ b/docs/interop/qemu-ga.rst > > @@ -33,6 +33,11 @@ Options > > > > .. program:: qemu-ga > > > > +.. option:: -c, --config=PATH > > + > > + Configuration file path (the default is |CONFDIR|\ ``/qemu-ga.conf``, > > + unless overriden by the QGA_CONF environment variable) > > + > > .. option:: -m, --method=METHOD > > > > Please also update qga/main.c static void usage(const char *cmd) Opps, yes, will do. 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 04/14] qapi: add a 'command-features' pragma
On Fri, Jul 12, 2024 at 10:50:54AM +0200, Markus Armbruster wrote: > Daniel P. Berrangé writes: > > > On Fri, Jul 12, 2024 at 10:07:34AM +0200, Markus Armbruster wrote: > >> Daniel P. Berrangé writes: > >> > >> > The 'command-features' pragma allows for defining additional > >> > special features that are unique to a particular QAPI schema > >> > instance and its implementation. > >> > >> So far, we have special features (predefined, known to the generator and > >> treated specially), and normal features (user-defined, not known to the > >> generator). You create a new kind in between: user-defined, not known > >> to the generator, yet treated specially (I guess?). Hmm. > >> > >> Could you at least hint at indented use here? What special treatment do > >> you have in mind? > > > > Essentially, these features are a way to attach metadata to commands that > > the server side impl can later query. This eliminates the need to hardcode > > lists of commands, such as in QGA which hardcodes a list of commands which > > are safe to use when filesystems are frozen. This is illustrated later in > > this series. > > Please update docs/devel/qapi-code-gen.rst section "Pragma directives", > and maybe section "Features". > > I'm not sure conflating the new kind of feature with existing special > features is a good idea. I need to review more of the series before I > can make up my mind. I originally implemented a completely separate 'tags' concept in the QAPI parser, before deciding I was just re-inventing 'features' for no obvious benefit. The other nice thing about using features is that these are exposed in the schema and docs. With the 'fsfreeze' restriction in code, there's no formal docs of what commands are allowed when frozen, and this is also not exposed in QAPI schema to apps. Using 'features' we get all that as standard. 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: [RFC PATCH 7/8] tests/pytest: Add a function for extracting files from an archive
On Thu, Jul 11, 2024 at 01:55:45PM +0200, Thomas Huth wrote: > Some Avocado-based tests use the "archive" module from avocado.utils > to extract files from an archive. To be able to use these tests > without Avocado, we have to provide our own function for extracting > files. Fortunately, there is already the tarfile module that will > provide us with this functionality, so let's just add a nice wrapper > function around that. > > Signed-off-by: Thomas Huth > --- > tests/pytest/qemu_pytest/utils.py | 21 + > 1 file changed, 21 insertions(+) > create mode 100644 tests/pytest/qemu_pytest/utils.py > > diff --git a/tests/pytest/qemu_pytest/utils.py > b/tests/pytest/qemu_pytest/utils.py > new file mode 100644 > index 00..4eb5e5d5e5 > --- /dev/null > +++ b/tests/pytest/qemu_pytest/utils.py > @@ -0,0 +1,21 @@ > +# Utilities for python-based QEMU tests > +# > +# Copyright 2024 Red Hat, Inc. > +# > +# Authors: > +# Thomas Huth > +# > +# This work is licensed under the terms of the GNU GPL, version 2 or > +# later. See the COPYING file in the top-level directory. > + > +import tarfile > + > +def archive_extract(archive, dest_dir, member=None): > +with tarfile.open(archive) as tf: > +if hasattr(tarfile, 'data_filter'): Not convinced this is still needed. The python docs don't say anything about 'data_filter' being introduced after 3.0, so can likely assume it always exists. > +tf.extraction_filter = getattr(tarfile, 'data_filter', > + (lambda member, path: member)) > +if member: > +tf.extract(member=member, path=dest_dir) > +else: > +tf.extractall(path=dest_dir) > -- > 2.45.2 > 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: [RFC PATCH 6/8] tests/pytest: Convert some tests that download files via fetch_asset()
On Thu, Jul 11, 2024 at 01:55:44PM +0200, Thomas Huth wrote:
> Now that we've got a working fetch_asset() function, we can convert
> some Avocado tests that use this function for downloading their
> required files.
>
> Signed-off-by: Thomas Huth
> ---
> tests/pytest/meson.build | 16 +++
> .../test_machine_arm_n8x0.py} | 20 +++
> .../test_machine_avr6.py} | 7 ++-
> .../test_machine_loongarch.py}| 11 --
> .../test_machine_mips_loongson3v.py} | 19 ++
> 5 files changed, 35 insertions(+), 38 deletions(-)
> rename tests/{avocado/machine_arm_n8x0.py =>
> pytest/test_machine_arm_n8x0.py} (71%)
> rename tests/{avocado/machine_avr6.py => pytest/test_machine_avr6.py} (91%)
> rename tests/{avocado/machine_loongarch.py =>
> pytest/test_machine_loongarch.py} (89%)
> rename tests/{avocado/machine_mips_loongson3v.py =>
> pytest/test_machine_mips_loongson3v.py} (59%)
If you take my suggestion in the previous patch, the sha1 hashes
would need updating to sha256 hashes here. Aside from that
Reviewed-by: Daniel P. Berrangé
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: [RFC PATCH 5/8] tests_pytest: Implement fetch_asset() method for downloading assets
On Thu, Jul 11, 2024 at 01:55:43PM +0200, Thomas Huth wrote:
> In the pytests, we cannot use the fetch_asset() function from Avocado
> anymore, so we have to provide our own implementation now instead.
> Thus add such a function based on the _download_with_cache() function
> from tests/vm/basevm.py for this purpose.
>
> Signed-off-by: Thomas Huth
> ---
> tests/pytest/qemu_pytest/__init__.py | 40
> 1 file changed, 29 insertions(+), 11 deletions(-)
>
> diff --git a/tests/pytest/qemu_pytest/__init__.py
> b/tests/pytest/qemu_pytest/__init__.py
> index e3ed32e3de..73d80b3828 100644
> --- a/tests/pytest/qemu_pytest/__init__.py
> +++ b/tests/pytest/qemu_pytest/__init__.py
> @@ -11,6 +11,7 @@
> # This work is licensed under the terms of the GNU GPL, version 2 or
> # later. See the COPYING file in the top-level directory.
>
> +import hashlib
> import logging
> import os
> import shutil
> @@ -201,17 +202,34 @@ def setUp(self, bin_prefix):
> self.assertIsNotNone(SOURCE_DIR,'PYTEST_SOURCE_ROOT must be set')
> self.assertIsNotNone(self.qemu_bin, 'PYTEST_QEMU_BINARY must be set')
>
> -def fetch_asset(self, name,
> -asset_hash, algorithm=None,
> -locations=None, expire=None,
> -find_only=False, cancel_on_missing=True):
> -return super().fetch_asset(name,
> -asset_hash=asset_hash,
> -algorithm=algorithm,
> -locations=locations,
> -expire=expire,
> -find_only=find_only,
> -cancel_on_missing=cancel_on_missing)
> +def check_hash(self, file_name, expected_hash):
> +if not expected_hash:
> +return True
> +if len(expected_hash) == 32:
> +sum_prog = 'md5sum'
> +elif len(expected_hash) == 40:
> +sum_prog = 'sha1sum'
> +elif len(expected_hash) == 64:
> +sum_prog = 'sha256sum'
> +elif len(expected_hash) == 128:
> +sum_prog = 'sha512sum'
> +else:
> +raise Exception("unknown hash type")
Why shouldn't we just standardize on sha256 as we convert each test
to pytest ? sha512 is overkill, and md5/sha1 shouldn't really be used
anymore.
> +checksum = subprocess.check_output([sum_prog, file_name]).split()[0]
> +return expected_hash == checksum.decode("utf-8")
> +
> +def fetch_asset(self, url, asset_hash):
> +cache_dir = os.path.expanduser("~/.cache/qemu/download")
> +if not os.path.exists(cache_dir):
> +os.makedirs(cache_dir)
> +fname = os.path.join(cache_dir,
> + hashlib.sha1(url.encode("utf-8")).hexdigest())
> +if os.path.exists(fname) and self.check_hash(fname, asset_hash):
> +return fname
> +logging.debug("Downloading %s to %s...", url, fname)
> +subprocess.check_call(["wget", "-c", url, "-O", fname + ".download"])
> +os.rename(fname + ".download", fname)
> +return fname
To avoid a dep on an external command that may not be installed,
I think we could replace wget with native python code:
import urllib
from shutil import copyfileobj
with urllib.request.urlopen(url) as src:
with open(fname + ".download", "w+") as dst
copyfileobj(src, dst)
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: [RFC PATCH 4/8] tests/pytest: add pytest to the meson build system
On Thu, Jul 11, 2024 at 01:55:42PM +0200, Thomas Huth wrote:
> From: Ani Sinha
>
> Integrate the pytest framework with the meson build system. This
> will make meson run all the pytests under the pytest directory.
Lets add a note about the compelling benefit of this new approach
With this change, each functional test becomes subject
to an individual execution timeout, defaulting to 60
seconds, but overridable per-test.
>
> Signed-off-by: Ani Sinha
> [thuth: Removed the acpi-bits and adjusted for converted avocado tests
> instead]
> Signed-off-by: Thomas Huth
> ---
> tests/Makefile.include | 4 ++-
> tests/meson.build| 1 +
> tests/pytest/meson.build | 53
> 3 files changed, 57 insertions(+), 1 deletion(-)
> create mode 100644 tests/pytest/meson.build
For CI purposes we'll need to add 'python3-pytest' to
tests/lcitool/projects/qemu.yml, and re-generate the
the dockerfiles. Some of the other non-gitlab CI
integrations probably need manual additions of pytest
packages.
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index d39d5dd6a4..68151717d7 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -3,12 +3,14 @@
> .PHONY: check-help
> check-help:
> @echo "Regression testing targets:"
> - @echo " $(MAKE) check Run block, qapi-schema, unit,
> softfloat, qtest and decodetree tests"
> + @echo " $(MAKE) check Run block, qapi-schema, unit,
> softfloat, qtest, pytest and decodetree tests"
> @echo " $(MAKE) bench Run speed tests"
> @echo
> @echo "Individual test suites:"
> @echo " $(MAKE) check-qtest-TARGET Run qtest tests for given target"
> @echo " $(MAKE) check-qtestRun qtest tests"
> + @echo " $(MAKE) check-pytest Run pytest tests"
> + @echo " $(MAKE) check-pytest-TARGETRun pytest for a given target"
Or name it after the type of test rather than harness ?
eg check-functional / check-functional-TARGET
For that matter perhaps also for the dir name ?
tests/functional/*.py
> @echo " $(MAKE) check-unit Run qobject tests"
> @echo " $(MAKE) check-qapi-schema Run QAPI schema tests"
> @echo " $(MAKE) check-blockRun block tests"
> diff --git a/tests/meson.build b/tests/meson.build
> index acb6807094..17510a468e 100644
> --- a/tests/meson.build
> +++ b/tests/meson.build
> @@ -85,3 +85,4 @@ subdir('unit')
> subdir('qapi-schema')
> subdir('qtest')
> subdir('migration')
> +subdir('pytest')
> diff --git a/tests/pytest/meson.build b/tests/pytest/meson.build
> new file mode 100644
> index 00..1486628d45
> --- /dev/null
> +++ b/tests/pytest/meson.build
> @@ -0,0 +1,53 @@
> +slow_pytests = {
> + 'mem_addr_space' : 90,
> +}
> +
> +pytests_generic = [
> + 'empty_cpu_model',
> + 'info_usernet',
> + 'version',
> +]
> +
> +pytests_x86_64 = [
> + 'cpu_queries',
> + 'mem_addr_space',
> + 'virtio_version',
> +]
> +
> +pytest = find_program('pytest', required: false)
> +if not pytest.found()
> + message('pytest not available ==> Disabled the qemu-pytests.')
> + subdir_done()
> +endif
> +
> +foreach dir : target_dirs
> + if not dir.endswith('-softmmu')
> +continue
> + endif
> +
> + target_base = dir.split('-')[0]
> + pytest_emulator = emulators['qemu-system-' + target_base]
> + target_pytests = get_variable('pytests_' + target_base, []) +
> pytests_generic
> +
> + test_deps = roms
> + pytest_env = environment()
> + if have_tools
> +pytest_env.set('PYTEST_QEMU_IMG', './qemu-img')
> +test_deps += [qemu_img]
> + endif
> + pytest_env.set('PYTEST_QEMU_BINARY', meson.global_build_root() /
> 'qemu-system-' + target_base)
> + pytest_env.set('PYTEST_SOURCE_ROOT', meson.project_source_root())
> + pytest_env.set('PYTEST_BUILD_ROOT', meson.project_build_root())
> + pytest_env.set('PYTHONPATH', meson.project_source_root() / 'python')
> +
> + foreach test : target_pytests
> +test('pytest-@0@/@1@'.format(target_base, test),
> + pytest,
> + depends: [test_deps, pytest_emulator, emulator_modules],
> + env: pytest_env,
> + args: [meson.current_source_dir() / 'test_' + test + '.py'],
> + timeout: slow_pytests.get(test, 60),
> + priority: slow_pytests.get(test, 60),
> + suite: ['pytest', 'pytest-' + target_base])
> + endforeach
> +endforeach
> --
> 2.45.2
>
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: [RFC PATCH 3/8] tests/pytest: Convert info_usernet and version test with small adjustments
On Thu, Jul 11, 2024 at 01:55:41PM +0200, Thomas Huth wrote:
> These two simple tests can be converted to a pytest quite easily,
> we just have to set the machine to 'none' now manually since we
> don't support the avocado tags here yet.
>
> Signed-off-by: Thomas Huth
> ---
> .../info_usernet.py => pytest/test_info_usernet.py} | 6 ++
> tests/{avocado/version.py => pytest/test_version.py} | 8 +++-
> 2 files changed, 5 insertions(+), 9 deletions(-)
> rename tests/{avocado/info_usernet.py => pytest/test_info_usernet.py} (91%)
> rename tests/{avocado/version.py => pytest/test_version.py} (82%)
Reviewed-by: Daniel P. Berrangé
> diff --git a/tests/avocado/version.py b/tests/pytest/test_version.py
> similarity index 82%
> rename from tests/avocado/version.py
> rename to tests/pytest/test_version.py
> index c6139568a1..2d16b4075d 100644
> --- a/tests/avocado/version.py
> +++ b/tests/pytest/test_version.py
> @@ -9,15 +9,13 @@
> # later. See the COPYING file in the top-level directory.
>
>
> -from avocado_qemu import QemuSystemTest
> +from qemu_pytest import QemuSystemTest
>
>
> class Version(QemuSystemTest):
> -"""
> -:avocado: tags=quick
I was going to suggest we expose 'quick' in the meson.build a suite,
but it seems we've barely used this tag, only 2 other examples, so
its pointless.
> -:avocado: tags=machine:none
> -"""
> +
> def test_qmp_human_info_version(self):
> +self.machine = 'none'
> self.vm.add_args('-nodefaults')
> self.vm.launch()
> res = self.vm.cmd('human-monitor-command',
> --
> 2.45.2
>
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: [RFC PATCH 2/8] tests/pytest: Convert some simple avocado tests into pytests
On Thu, Jul 11, 2024 at 01:55:40PM +0200, Thomas Huth wrote:
> These test are rather simple and don't need any modifications apart
> from adjusting the "from avocado_qemu" line. These tests can now
> be run directly via "pytest" by setting the PYTHONPATH environment
> variable to the python folder of QEMU and by providing the QEMU
> binary via the PYTEST_QEMU_BINARY environment variable, and the source
> and build directories via the PYTEST_SOURCE_ROOTand PYTEST_BUILD_ROOT
> environment variables.
>
> Signed-off-by: Thomas Huth
> ---
> tests/{avocado/cpu_queries.py => pytest/test_cpu_queries.py} | 2 +-
> .../empty_cpu_model.py => pytest/test_empty_cpu_model.py} | 2 +-
> .../mem-addr-space-check.py => pytest/test_mem_addr_space.py} | 3 +--
> .../virtio_version.py => pytest/test_virtio_version.py}| 2 +-
> 4 files changed, 4 insertions(+), 5 deletions(-)
> rename tests/{avocado/cpu_queries.py => pytest/test_cpu_queries.py} (96%)
> rename tests/{avocado/empty_cpu_model.py => pytest/test_empty_cpu_model.py}
> (94%)
> rename tests/{avocado/mem-addr-space-check.py =>
> pytest/test_mem_addr_space.py} (99%)
> rename tests/{avocado/virtio_version.py => pytest/test_virtio_version.py}
> (99%)
Reviewed-by: Daniel P. Berrangé
though if you take my suggestion in the previous patch, then this
patch should 'chmod +x' all the test files, and add the __main__
magic to call 'unittest.main().
>
> diff --git a/tests/avocado/cpu_queries.py b/tests/pytest/test_cpu_queries.py
> similarity index 96%
> rename from tests/avocado/cpu_queries.py
> rename to tests/pytest/test_cpu_queries.py
> index d3faa14720..b300447121 100644
> --- a/tests/avocado/cpu_queries.py
> +++ b/tests/pytest/test_cpu_queries.py
> @@ -8,7 +8,7 @@
> # This work is licensed under the terms of the GNU GPL, version 2 or
> # later. See the COPYING file in the top-level directory.
>
> -from avocado_qemu import QemuSystemTest
> +from qemu_pytest import QemuSystemTest
>
> class QueryCPUModelExpansion(QemuSystemTest):
> """
> diff --git a/tests/avocado/empty_cpu_model.py
> b/tests/pytest/test_empty_cpu_model.py
> similarity index 94%
> rename from tests/avocado/empty_cpu_model.py
> rename to tests/pytest/test_empty_cpu_model.py
> index d906ef3d3c..113740bc82 100644
> --- a/tests/avocado/empty_cpu_model.py
> +++ b/tests/pytest/test_empty_cpu_model.py
> @@ -7,7 +7,7 @@
> #
> # This work is licensed under the terms of the GNU GPL, version 2 or
> # later. See the COPYING file in the top-level directory.
> -from avocado_qemu import QemuSystemTest
> +from qemu_pytest import QemuSystemTest
>
> class EmptyCPUModel(QemuSystemTest):
> def test(self):
> diff --git a/tests/avocado/mem-addr-space-check.py
> b/tests/pytest/test_mem_addr_space.py
> similarity index 99%
> rename from tests/avocado/mem-addr-space-check.py
> rename to tests/pytest/test_mem_addr_space.py
> index 85541ea051..6ae7ba5e6b 100644
> --- a/tests/avocado/mem-addr-space-check.py
> +++ b/tests/pytest/test_mem_addr_space.py
> @@ -8,8 +8,7 @@
> #
> # SPDX-License-Identifier: GPL-2.0-or-later
>
> -from avocado_qemu import QemuSystemTest
> -import signal
> +from qemu_pytest import QemuSystemTest
> import time
>
> class MemAddrCheck(QemuSystemTest):
> diff --git a/tests/avocado/virtio_version.py
> b/tests/pytest/test_virtio_version.py
> similarity index 99%
> rename from tests/avocado/virtio_version.py
> rename to tests/pytest/test_virtio_version.py
> index afe5e828b5..ca3aa806df 100644
> --- a/tests/avocado/virtio_version.py
> +++ b/tests/pytest/test_virtio_version.py
> @@ -12,7 +12,7 @@
> import os
>
> from qemu.machine import QEMUMachine
> -from avocado_qemu import QemuSystemTest
> +from qemu_pytest import QemuSystemTest
>
> # Virtio Device IDs:
> VIRTIO_NET = 1
> --
> 2.45.2
>
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: [RFC PATCH 1/8] tests/pytest: Add base classes for the upcoming pytest-based tests
On Thu, Jul 11, 2024 at 01:55:39PM +0200, Thomas Huth wrote:
> The file is a copy of the tests/avocado/avocado_qemu/__init__.py file
> with some adjustments to get rid of the Avocado dependencies (i.e.
> we also have to drop the LinuxSSHMixIn and LinuxTest for now).
>
> The emulator binary, source and build directory are now passed via
> environment variables that will be set via meson.build later.
>
> Signed-off-by: Thomas Huth
> ---
> tests/pytest/qemu_pytest/__init__.py | 344 +++
> 1 file changed, 344 insertions(+)
> create mode 100644 tests/pytest/qemu_pytest/__init__.py
>
> diff --git a/tests/pytest/qemu_pytest/__init__.py
> b/tests/pytest/qemu_pytest/__init__.py
> new file mode 100644
> index 00..e3ed32e3de
> --- /dev/null
> +++ b/tests/pytest/qemu_pytest/__init__.py
> @@ -0,0 +1,344 @@
> +# Test class and utilities for functional tests
> +#
> +# Copyright 2018, 2024 Red Hat, Inc.
> +#
> +# Original Author (Avocado-based tests):
> +# Cleber Rosa
> +#
> +# Adaption for pytest based version:
> +# Thomas Huth
> +#
> +# This work is licensed under the terms of the GNU GPL, version 2 or
> +# later. See the COPYING file in the top-level directory.
> +
> +import logging
> +import os
> +import shutil
> +import subprocess
> +import sys
> +import tempfile
> +import time
> +import uuid
> +import unittest
> +
> +from qemu.machine import QEMUMachine
> +from qemu.utils import (get_info_usernet_hostfwd_port, kvm_available,
> +tcg_available)
> +
> +BUILD_DIR = os.getenv('PYTEST_BUILD_ROOT')
> +SOURCE_DIR = os.getenv('PYTEST_SOURCE_ROOT')
We can make life slightly nicer for developers running tests directly
without meson, by figuring this out automatically if the env vars are
omitted. To enable devs to do
PYTEST_QEMU_BINARY=./build/qemu-system-aarch64 \
PYTHONPATH=./python \
pytest build/tests/pytest/test_info_usernet.py
I propose the following additional logic on top of your patch:
diff --git a/tests/pytest/qemu_pytest/__init__.py
b/tests/pytest/qemu_pytest/__init__.py
index 73d80b3828..711cb06012 100644
--- a/tests/pytest/qemu_pytest/__init__.py
+++ b/tests/pytest/qemu_pytest/__init__.py
@@ -21,13 +21,41 @@
import time
import uuid
import unittest
+from pathlib import Path
from qemu.machine import QEMUMachine
from qemu.utils import (get_info_usernet_hostfwd_port, kvm_available,
tcg_available)
-BUILD_DIR = os.getenv('PYTEST_BUILD_ROOT')
-SOURCE_DIR = os.getenv('PYTEST_SOURCE_ROOT')
+def _pytest_dir():
+if sys.argv[0].startswith("pytest") or "bin/pytest" in sys.argv[0]:
+if sys.argv[1].endswith(".py"):
+# Assume 'pytest ./build/tests/pytest/test_blah.py '
+return Path(sys.argv[1]).absolute().parent
+else:
+# Assume 'pytest ./build/tests/pytest'
+return Path(sys.argv[1]).absolute()
+
+# Assume './build/tests/pytest/test_NAME.py'
+if sys.argv[0].endswith(".py"):
+return Path(sys.argv[0]).absolute().parent
+
+raise Exception("Cannot identify pytest build dir, set PYTEST_BUILD_ROOT")
+
+def _build_dir():
+root = os.getenv('PYTEST_BUILD_ROOT')
+if root is not None:
+return Path(root)
+
+return _pytest_dir().parent.parent
+
+def _source_dir():
+root = os.getenv('PYTEST_SOURCE_ROOT')
+if root is not None:
+return Path(root)
+
+# Assume build/tests/pytest is a symlink to the source root
+return _pytest_dir().resolve().parent.parent
def has_cmd(name, args=None):
"""
@@ -189,8 +217,8 @@ class QemuBaseTest(unittest.TestCase):
qemu_bin = os.getenv('PYTEST_QEMU_BINARY')
-workdir = os.path.join(BUILD_DIR, 'tests/pytest')
-logdir = os.path.join(BUILD_DIR, 'tests/pytest')
+workdir = str(Path(_build_dir(), 'tests', 'pytest'))
+logdir = str(Path(_build_dir(), 'tests', 'pytest'))
cpu = None
machine = None
@@ -198,8 +226,6 @@ class QemuBaseTest(unittest.TestCase):
log = logging.getLogger('qemu-pytest')
def setUp(self, bin_prefix):
-self.assertIsNotNone(BUILD_DIR, 'PYTEST_BUILD_ROOT must be set')
-self.assertIsNotNone(SOURCE_DIR,'PYTEST_SOURCE_ROOT must be set')
self.assertIsNotNone(self.qemu_bin, 'PYTEST_QEMU_BINARY must be set')
def check_hash(self, file_name, expected_hash):
@@ -294,9 +320,11 @@ def get_qemu_img(self):
# If qemu-img has been built, use it, otherwise the system wide one
# will be used.
-qemu_img = os.path.join(BUILD_DIR, 'qemu-img')
-if not os.path.exists(qemu_img):
-qemu_img = find_command('qemu-img', False)
+qemu_img = Path(_build_dir(), 'qemu-img')
+if qemu_img.exists():
+return str(qemu_img)
+
+qemu_img = find_command('qemu-img', False)
if qemu_img is False:
self.cancel('Could not find "qemu-img"')
This also allows for executing the tests directly without even involving
Re: [PATCH 04/14] qapi: add a 'command-features' pragma
On Fri, Jul 12, 2024 at 10:07:34AM +0200, Markus Armbruster wrote: > Daniel P. Berrangé writes: > > > The 'command-features' pragma allows for defining additional > > special features that are unique to a particular QAPI schema > > instance and its implementation. > > So far, we have special features (predefined, known to the generator and > treated specially), and normal features (user-defined, not known to the > generator). You create a new kind in between: user-defined, not known > to the generator, yet treated specially (I guess?). Hmm. > > Could you at least hint at indented use here? What special treatment do > you have in mind? Essentially, these features are a way to attach metadata to commands that the server side impl can later query. This eliminates the need to hardcode lists of commands, such as in QGA which hardcodes a list of commands which are safe to use when filesystems are frozen. This is illustrated later in this series. > > > Signed-off-by: Daniel P. Berrangé > > --- > > scripts/qapi/parser.py | 2 ++ > > scripts/qapi/source.py | 2 ++ > > 2 files changed, 4 insertions(+) > > > > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py > > index 7b13a583ac..36a9046243 100644 > > --- a/scripts/qapi/parser.py > > +++ b/scripts/qapi/parser.py > > @@ -243,6 +243,8 @@ def check_list_str(name: str, value: object) -> > > List[str]: > > pragma.documentation_exceptions = check_list_str(name, value) > > elif name == 'member-name-exceptions': > > pragma.member_name_exceptions = check_list_str(name, value) > > +elif name == 'command-features': > > +pragma.command_features = check_list_str(name, value) > > else: > > raise QAPISemError(info, "unknown pragma '%s'" % name) > > > > diff --git a/scripts/qapi/source.py b/scripts/qapi/source.py > > index 7b379fdc92..07c2958ac4 100644 > > --- a/scripts/qapi/source.py > > +++ b/scripts/qapi/source.py > > @@ -28,6 +28,8 @@ def __init__(self) -> None: > > self.documentation_exceptions: List[str] = [] > > # Types whose member names may violate case conventions > > self.member_name_exceptions: List[str] = [] > > +# Arbitrary extra features recorded against commands > > +self.command_features: List[str] = [] > > > > > > class QAPISourceInfo: > 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: [RFC PATCH 0/8] Convert avocado tests to normal Python unittests
On Thu, Jul 11, 2024 at 07:44:39PM +0200, Thomas Huth wrote: > On 11/07/2024 16.39, Fabiano Rosas wrote: > > Thomas Huth writes: > ... > > > Things that need further attention though: > > > > > > - All tests that use the LinuxTest / LinuxDistro classes (e.g. based > > >on cloud-init images) really depend on the Avocado framework, > > >thus we'd need a solution for those if we want to continue with > > >this approach > > > > > > - Same for all tests that require the LinuxSSHMixIn class - we'd > > >need to provide a solution for ssh-based tests, too. > > > > These two seem to be dependent mostly avocado/utils only. Those could > > still be used without the whole framework, no? Say we keep importing > > avocado.utils, but run everything from meson, would that make sense? > > Yes ... maybe ... I can give it a try to see whether that works. We only import about 8 modules from avocado.utils. There are probably a few more indirectly used, but worst case we just clone the bits we need into the QEMU tree. 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] contrib/elf2dmp: a workaround for the buggy msvcrt.dll!fwrite
On Mon, Jul 08, 2024 at 07:25:20PM +0800, junjiehua wrote:
> when building elf2dump with x86_64-w64-mingw32-gcc, fwrite is imported from
> msvcrt.dll. However, the implementation of msvcrt.dll!fwrite is buggy:
> it enters an infinite loop when the size of a single write exceeds 4GB.
> This patch addresses the issue by splitting large physical memory
> blocks into smaller chunks.
>
> Signed-off-by: junjiehua
> ---
> contrib/elf2dmp/main.c | 19 +++
> 1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/contrib/elf2dmp/main.c b/contrib/elf2dmp/main.c
> index d046a72ae6..1994553d95 100644
> --- a/contrib/elf2dmp/main.c
> +++ b/contrib/elf2dmp/main.c
> @@ -23,6 +23,8 @@
> #define INITIAL_MXCSR 0x1f80
> #define MAX_NUMBER_OF_RUNS 42
>
> +#define MAX_CHUNK_SIZE (128 * 1024 * 1024)
> +
> typedef struct idt_desc {
> uint16_t offset1; /* offset bits 0..15 */
> uint16_t selector;
> @@ -434,13 +436,22 @@ static bool write_dump(struct pa_space *ps,
>
> for (i = 0; i < ps->block_nr; i++) {
> struct pa_block *b = >block[i];
> +size_t offset = 0;
> +size_t chunk_size;
>
> printf("Writing block #%zu/%zu of %"PRIu64" bytes to file...\n", i,
> ps->block_nr, b->size);
> -if (fwrite(b->addr, b->size, 1, dmp_file) != 1) {
> -eprintf("Failed to write block\n");
> -fclose(dmp_file);
> -return false;
> +
> +while (offset < b->size) {
> +chunk_size = (b->size - offset > MAX_CHUNK_SIZE)
> + ? MAX_CHUNK_SIZE
> + : (b->size - offset);
> +if (fwrite(b->addr + offset, chunk_size, 1, dmp_file) != 1) {
> +eprintf("Failed to write block\n");
> +fclose(dmp_file);
> +return false;
> +}
> +offset += chunk_size;
> }
> }
When reading the original ELF file, we don't actually fread() it,
instead we mmap it, using GMappedFile on Windows. Rather than
working around fwrite() bugs, we could do the same for writing
and create a mapped file and just memcpy the data across.
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] contrib/elf2dmp: a workaround for the buggy msvcrt.dll!fwrite
On Thu, Jul 11, 2024 at 04:53:50PM +0900, Akihiko Odaki wrote: > On 2024/07/10 17:02, hellord wrote: > > > > note: > > 1. The path of buggy msvcrt.dll is c:\windows\system32\msvcrt.dll( > > mingw64 links to it ); > > 2. fwrite implementation in another msvc library which is called > > ucrtbase.dll is correct(msvc links to it by default). > > I don't object to this change but you should use ucrt whenever possible. I'm > not confident that elf2dmp and other QEMU binaries would work well with > mvcrt. > > I even would like to force users to use ucrt and call setlocale(LC_ALL, > ".UTF8") to fix text encoding, but I haven't done that yet because Fedora, > which cross-compiles QEMU for CI, still uses msvcrt. Our native Windows builds are also validating with msvcrt, and Stefan's Windows packages for QEMU are also msvcrt. Users getting QEMU packages from msys can choose whether to pull the msvcrt build or the ucrt build, but forcing ucrt is a non-starter IMHO. 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 v1] target/i386: kvm: Block migration when enfore_cpuid is set to false
On Thu, Jul 11, 2024 at 03:09:28PM +, Wang, Wei W wrote: > On Thursday, July 11, 2024 10:14 PM, Daniel P. Berrangé wrote: > > On Thu, Jul 11, 2024 at 02:13:31PM +, Wang, Wei W wrote: > > > On Thursday, July 11, 2024 8:25 PM, Daniel P. Berrangé wrote: > > > > On Thu, Jul 11, 2024 at 12:10:34PM +, Wang, Wei W wrote: > > > > > On Thursday, July 11, 2024 7:48 PM, Daniel P. Berrangé wrote: > > > > > > On Wed, Jul 03, 2024 at 10:49:12PM +0800, Wei Wang wrote: > > > > > AFAIK, many users are not aware of this, and also we couldn't > > > > > assume everybody knows it. That's why we want to add the enforcement. > > > > > > > > Users who directly launch QEMU are expected to know about QEMU > > > > config details for migration. If they don't, then they ought to be > > > > using a higher level tool like libvirt, which ensures the configuration > > > > is > > migration compatible. > > > > > > Agree that libvirt has this advantage and is more user friendly. But > > > it doesn't seem to solve the issue mentioned by this patch - if users > > > don't > > explicitly set "enforce=true" > > > in libvirt configs for the guest, then migrating the guest across > > > hosts with different features could still be risky. Unless there is > > > similar enforcement in libvirt to require users to set "enforce=true" for > > > the > > guest to be migratable. > > > > Yes, libvirt takes steps to ensure CPU compatibility before migrating. > > Thanks for sharing, but curious about those steps. For example, with > "enforce=off" > (by default), features on the destination could be filtered by QEMU (kind of > silently, > just has warning logs). > How would the source side libvirt get informed about more features getting > filtered by > the destination side QEMU (as the destination host has less support for this > vcpu model)? > This causes inconsistencies. Libvirt queries QEMU to ask it what features were actually active in the guest. 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 v1] target/i386: kvm: Block migration when enfore_cpuid is set to false
On Thu, Jul 11, 2024 at 02:13:31PM +, Wang, Wei W wrote: > On Thursday, July 11, 2024 8:25 PM, Daniel P. Berrangé wrote: > > On Thu, Jul 11, 2024 at 12:10:34PM +, Wang, Wei W wrote: > > > On Thursday, July 11, 2024 7:48 PM, Daniel P. Berrangé wrote: > > > > On Wed, Jul 03, 2024 at 10:49:12PM +0800, Wei Wang wrote: > > > AFAIK, many users are not aware of this, and also we couldn't assume > > > everybody knows it. That's why we want to add the enforcement. > > > > Users who directly launch QEMU are expected to know about QEMU config > > details for migration. If they don't, then they ought to be using a higher > > level > > tool like libvirt, which ensures the configuration is migration compatible. > > Agree that libvirt has this advantage and is more user friendly. But it > doesn't seem to > solve the issue mentioned by this patch - if users don't explicitly set > "enforce=true" > in libvirt configs for the guest, then migrating the guest across hosts with > different > features could still be risky. Unless there is similar enforcement in libvirt > to require > users to set "enforce=true" for the guest to be migratable. Yes, libvirt takes steps to ensure CPU compatibility before migrating. 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 v1] target/i386: kvm: Block migration when enfore_cpuid is set to false
On Thu, Jul 11, 2024 at 01:48:59PM +, Wang, Wei W wrote: > On Thursday, July 11, 2024 8:25 PM, Daniel P. Berrangé wrote: > > On Thu, Jul 11, 2024 at 12:10:34PM +, Wang, Wei W wrote: > > > On Thursday, July 11, 2024 7:48 PM, Daniel P. Berrangé wrote: > > > > On Wed, Jul 03, 2024 at 10:49:12PM +0800, Wei Wang wrote: > > > > > When enforce_cpuid is set to false, the guest is launched with a > > > > > filtered set of features, meaning that unsupported features by the > > > > > host are removed from the guest's vCPU model. This could cause > > > > > issues for > > > > live migration. > > > > > For example, a guest on the source is running with features A and B. > > > > > If the destination host does not support feature B, the stub guest > > > > > can still be launched on the destination with feature A only if > > enforce_cpuid=false. > > > > > Live migration can start in this case, though it may fail later > > > > > when the states of feature B are put to the destination side. This > > > > > failure occurs in the late stage (i.e., stop phase) of the > > > > > migration flow, where the source guest has already been paused. > > > > > Tests show that in such cases the source guest does not recover, > > > > > and the destination is unable to resume to run. > > > > > > > > > > Make "enfore_cpuid=true" a hard requirement for a guest to be > > > > > migratable, and change the default value of "enforce_cpuid" to > > > > > true, making the guest vCPUs migratable by default. If the > > > > > destination stub guest has inconsistent CPUIDs (i.e., destination > > > > > host cannot support the features defined by the guest's vCPU > > > > > model), it fails to boot (with enfore_cpuid=true by default), > > > > > thereby preventing migration from occuring. If enfore_cpuid=false > > > > > is explicitly added for the guest, the guest is deemed as > > > > > non-migratable (via the migration blocker), so the above issue won't > > occur as the guest won't be migrated. > > > > > > > > Blocking migration when enforce=false is making an assumption that > > > > users of that setting are inherantly broken. This is NOT the case if > > > > the user/app has already validated compatibility in some manner > > > > outside QEMU. Blocking migration in this case will break valid working > > > > use > > cases. > > > > > > It's just an enforcement to ensure a safe migration. Without this > > > (i.e., the current QEMU code) is making an assumption that users > > > always have validated compatibility in a good manner outside QEMU, which > > is risky to some degree? > > > > QEMU configurations must never be assumed to be migratable by default. > > There is a huge set of things that a user must do with QEMU configuration to > > guarantee migratability beyond CPU features. All aspects of guest HW device > > topology must be set explicitly. > > What if the source and destination are required to use exactly the same QEMU > commands? Does this meet the feature and topology requirements as you > mentioned above? That is insufficient as it does not take account of device hotplug. > > > > IMHO this patch doesn't need to exist. If users of QEMU want strong > > > > protection they can already opt-in to that with enforce=true. > > > > > > AFAIK, many users are not aware of this, and also we couldn't assume > > > everybody knows it. That's why we want to add the enforcement. > > > > Users who directly launch QEMU are expected to know about QEMU config > > details for migration. If they don't, then they ought to be using a higher > > level > > tool like libvirt, which ensures the configuration is migration compatible. > > Could you explain how libvirt provides a more reliable assurance of migration > compatibility in its configuration compared to using raw QEMU commands? > Per my understanding, libvirt configs mostly map to the QEMU commands. Libvirt records the full details of the guest configuration required to reproduce the exact same guest ABI, even across device hotplug. This is why libvirt QEMU command lines are absolutely enourmous compared to minimalist command lines that users usuall run directly. 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: [RFC PATCH 0/8] Convert avocado tests to normal Python unittests
On Thu, Jul 11, 2024 at 01:55:38PM +0200, Thomas Huth wrote: > The Avocado v88 that we use in QEMU is already on a life support > system: It is not supported by upstream anymore, and with the latest > versions of Python, it won't work anymore since it depends on the > "imp" module that has been removed in Python 3.12. > > There have been several attempts to update the test suite in QEMU > to a newer version of Avocado, but so far no attempt has successfully > been merged yet. > > Additionally, the whole "make check" test suite in QEMU is using the > meson test runner nowadays, so running the python-based tests via the > Avocodo test runner looks and feels quite like an oddball, requiring > the users to deal with the knowledge of multiple test runners in > parallel. The fewer / simpler the layers we have in the execution path of tests the better our life will be in debugging IMHO. Having each individual test registered with meson has the particularly strong advantage that we can make use of meson's timeout feature to force individual tests to abort if they hang/run too slowly, as we did when converting the iotests to individual meson tests. > > So instead of trying to update the python-based test suite in QEMU > to a newer version of Avocado, we should maybe try to better integrate > it with the meson test runner instead. Indeed most tests work quite > nicely without the Avocado framework already, as you can see with > this patch series - it does not convert all tests, just a subset since > it is just an RFC so far, but as you can see, many tests only need > small modifications to work without Avocado. > > If you want to try it: Apply the patches, make sure that you have the > "pytest" program installed, then recompile and then run: > > make check-pytest > > Things that need further attention though: > > - All tests that use the LinuxTest / LinuxDistro classes (e.g. based > on cloud-init images) really depend on the Avocado framework, > thus we'd need a solution for those if we want to continue with > this approach Right, avocado is providing 2 distinct things, the test execution harness and the test framework APIs. It could be valid to remove use of the harness but keep using the framework APIs, especially if that's sufficient to unblock updating to new avocado versions too ? Over the longer term we can consider whether the framework APIs should remain or be replaced by something else. > - Same for all tests that require the LinuxSSHMixIn class - we'd > need to provide a solution for ssh-based tests, too. > > - We lose the way of running tests via the avocado tags this way... > single targets can still be tested by running "make check-pytest-arm" > for example, but running selected tests by other tags does not > work anymore. The meson "suites" concept is the logical equivalent of tags. You've wired up a suite for each architecture. We could define more suites if there are other useful criteria for filtering tests to be run. Perhaps machine type ? "make check-pytest-arm-" > - I haven't looked into logging yet ... this still needs some work > so that you could e.g. inspect the console output of the guests > somewhere Yep, debuggability is probably the single biggest problem we face with our tests. Simplifying the test execution harness will help in this respect, but yeah, we must have a way to capture logs of stuff executed. > - I did not work on documentation updates yet (will do that if we > agree to continue with this patch series) > > What's your thoughts? Is it worth to continue with this approach? > Or shall I rather forget about it and wait for the Avocado version > update? 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: Avocado 88.1 vs Python 3.12
On Thu, Jul 11, 2024 at 02:01:25PM +0200, Thomas Huth wrote:
> On 10/07/2024 01.45, Richard Henderson wrote:
> > On 7/9/24 09:26, Philippe Mathieu-Daudé wrote:
> > > On 9/7/24 17:41, Richard Henderson wrote:
> > > > Hi guys,
> > > >
> > > > I have reinstalled my development box to ubuntu 24 (because the
> > > > Rust support is better than my previous install; ho hum). I
> > > > thought I had tested everything in a VM before committing, but I
> > > > missed out on Avocado:
> > > >
> > > > > AVOCADO Downloading avocado tests VM image for aarch64
> > > > > Failed to load plugin from module "avocado.plugins.list":
> > > > > ModuleNotFoundError("No module named 'imp'") :
> > >
> > >
> > > > If I understand things correctly, the python "imp" package was
> > > > deprecated, and has been removed before v3.12. This is fixed in
> > > > upstream avocado as of v93. But we have a hard stop in
> > > > pythondeps.toml at v92.
> > > >
> > > > Remind me what the blocker is to upgrading?
> > >
> > > IIRC we're waiting for v2 of:
> > > https://lore.kernel.org/qemu-devel/20231208190911.102879-1-cr...@redhat.com/
> >
> > Yes indeed. There are two minor conflicts in rebasing this branch, but
> > otherwise it works. Cleber, do you have time to pick this up again?
>
> As an alternative, if nobody has time to work on that Avocado update, we
> could maybe also try to integrate the python-based tests directly with the
> meson test runner. A prototype can be found here:
>
> https://lore.kernel.org/qemu-devel/2024075546.40859-1-th...@redhat.com/
Ooh, that looks remarkably straightford. I'd love to see use using pytest as
the test harness, even if we keep using avocado framework in some of the
test case impls.
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 v1] target/i386: kvm: Block migration when enfore_cpuid is set to false
On Thu, Jul 11, 2024 at 12:10:34PM +, Wang, Wei W wrote: > On Thursday, July 11, 2024 7:48 PM, Daniel P. Berrangé wrote: > > On Wed, Jul 03, 2024 at 10:49:12PM +0800, Wei Wang wrote: > > > When enforce_cpuid is set to false, the guest is launched with a > > > filtered set of features, meaning that unsupported features by the > > > host are removed from the guest's vCPU model. This could cause issues for > > live migration. > > > For example, a guest on the source is running with features A and B. > > > If the destination host does not support feature B, the stub guest can > > > still be launched on the destination with feature A only if > > > enforce_cpuid=false. > > > Live migration can start in this case, though it may fail later when > > > the states of feature B are put to the destination side. This failure > > > occurs in the late stage (i.e., stop phase) of the migration > > > flow, where the source guest has already been paused. Tests show that > > > in such cases the source guest does not recover, and the destination > > > is unable to resume to run. > > > > > > Make "enfore_cpuid=true" a hard requirement for a guest to be > > > migratable, and change the default value of "enforce_cpuid" to true, > > > making the guest vCPUs migratable by default. If the destination stub > > > guest has inconsistent CPUIDs (i.e., destination host cannot support > > > the features defined by the guest's vCPU model), it fails to boot > > > (with enfore_cpuid=true by default), thereby preventing migration from > > > occuring. If enfore_cpuid=false is explicitly added for the guest, the > > > guest is deemed as non-migratable (via the migration blocker), so the > > > above issue won't occur as the guest won't be migrated. > > > > Blocking migration when enforce=false is making an assumption that users of > > that setting are inherantly broken. This is NOT the case if the user/app has > > already validated compatibility in some manner outside QEMU. Blocking > > migration in this case will break valid working use cases. > > It's just an enforcement to ensure a safe migration. Without this (i.e., the > current > QEMU code) is making an assumption that users always have validated > compatibility in a good manner outside QEMU, which is risky to some degree? QEMU configurations must never be assumed to be migratable by default. There is a huge set of things that a user must do with QEMU configuration to guarantee migratability beyond CPU features. All aspects of guest HW device topology must be set explicitly. > Do you see how this would break valid working use cases (any examples)? > This is actually what we are looking for. Please be aware that "enforce" is > changed to be true by default to make the guest to be migratable by default > under the enforcement. Setting "enforce" will break existing use of QEMU. It is valid to launch QEMU with a CPU model that is not fully supported by the host, allowing QEMU to disable unsupported features automatically. > > IMHO this patch doesn't need to exist. If users of QEMU want strong > > protection > > they can already opt-in to that with enforce=true. > > AFAIK, many users are not aware of this, and also we couldn't assume everybody > knows it. That's why we want to add the enforcement. Users who directly launch QEMU are expected to know about QEMU config details for migration. If they don't, then they ought to be using a higher level tool like libvirt, which ensures the configuration is migration compatible. 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 v1] target/i386: kvm: Block migration when enfore_cpuid is set to false
On Wed, Jul 03, 2024 at 10:49:12PM +0800, Wei Wang wrote: > When enforce_cpuid is set to false, the guest is launched with a filtered > set of features, meaning that unsupported features by the host are removed > from the guest's vCPU model. This could cause issues for live migration. > For example, a guest on the source is running with features A and B. If > the destination host does not support feature B, the stub guest can still > be launched on the destination with feature A only if enforce_cpuid=false. > Live migration can start in this case, though it may fail later when the > states of feature B are put to the destination side. This failure occurs > in the late stage (i.e., stop phase) of the migration flow, where the > source guest has already been paused. Tests show that in such cases the > source guest does not recover, and the destination is unable to resume to > run. > > Make "enfore_cpuid=true" a hard requirement for a guest to be migratable, > and change the default value of "enforce_cpuid" to true, making the guest > vCPUs migratable by default. If the destination stub guest has inconsistent > CPUIDs (i.e., destination host cannot support the features defined by the > guest's vCPU model), it fails to boot (with enfore_cpuid=true by default), > thereby preventing migration from occuring. If enfore_cpuid=false is > explicitly added for the guest, the guest is deemed as non-migratable > (via the migration blocker), so the above issue won't occur as the guest > won't be migrated. Blocking migration when enforce=false is making an assumption that users of that setting are inherantly broken. This is NOT the case if the user/app has already validated compatibility in some manner outside QEMU. Blocking migration in this case will break valid working use cases. IMHO this patch doesn't need to exist. If users of QEMU want strong protection they can already opt-in to that with enforce=true. 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] smbios: make memory device size configurable per Machine
On Thu, Jul 11, 2024 at 09:48:22AM +0200, Igor Mammedov wrote:
> Currently SMBIOS maximum memory device chunk is capped at 16Gb,
> which is fine for the most cases (QEMU uses it to describe initial
> RAM (type 17 SMBIOS table entries)).
> However when starting guest with terabytes of RAM this leads to
> too many memory device structures, which eventually upsets linux
> kernel as it reserves only 64K for these entries and when that
> border is crossed out it runs out of reserved memory.
>
> Instead of partitioning initial RAM on 16Gb chunks, use maximum
> possible chunk size that SMBIOS spec allows[1]. Which lets
> encode RAM in Mb units in uint32_t-1 field (upto 2047Tb).
> As result initial RAM will generate only one type 17 structure
> until host/guest reach ability to use more RAM in the future.
>
> Compat changes:
> We can't unconditionally change chunk size as it will break
> QEMU<->guest ABI (and migration). Thus introduce a new machine class
> field that would let older versioned machines to use 16Gb chunks
> while new machine type could use maximum possible chunk size.
>
> While it might seem to be risky to rise max entry size this much
> (much beyond of what current physical RAM modules support),
> I'd not expect it causing much issues, modulo uncovering bugs
> in software running within guest. And those should be fixed
> on guest side to handle SMBIOS spec properly, especially if
> guest is expected to support so huge RAM configs.
> In worst case, QEMU can reduce chunk size later if we would
> care enough about introducing a workaround for some 'unfixable'
> guest OS, either by fixing up the next machine type or
> giving users a CLI option to customize it.
I was wondering what real hardware does, since the best way to
avoid guest OS surprises is to align with real world behaviour.
IIUC, there is usually one Type 17 structure per physical
DIMM.
Most QEMU configs don't express DIMMs as a concept so in that
case, we can presume 1 virtual DIMM, and thus having one type
17 structure is a match for physical hw practices.
What about when the QEMU config has used nvdimm, pc-dimm,
or virtio-mem devices though ? It feels like the best practice
would be to have a type 17 structure for each instance of one
of those devices.
>
> 1) SMBIOS 3.1.0 7.18.5 Memory Device — Extended Size
>
> PS:
> * tested on 8Tb host with RHEL6 guest, which seems to parse
> type 17 SMBIOS table entries correctly (according to 'dmidecode').
>
> Signed-off-by: Igor Mammedov
> ---
> include/hw/boards.h | 4
> hw/arm/virt.c | 1 +
> hw/core/machine.c | 1 +
> hw/i386/pc_piix.c | 1 +
> hw/i386/pc_q35.c| 1 +
> hw/smbios/smbios.c | 11 ++-
> 6 files changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index ef6f18f2c1..48ff6d8b93 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -237,6 +237,9 @@ typedef struct {
> *purposes only.
> *Applies only to default memory backend, i.e., explicit memory backend
> *wasn't used.
> + * @smbios_memory_device_size:
> + *Default size of memory device,
> + *SMBIOS 3.1.0 "7.18 Memory Device (Type 17)"
> */
> struct MachineClass {
> /*< private >*/
> @@ -304,6 +307,7 @@ struct MachineClass {
> const CPUArchIdList *(*possible_cpu_arch_ids)(MachineState *machine);
> int64_t (*get_default_cpu_node_id)(const MachineState *ms, int idx);
> ram_addr_t (*fixup_ram_size)(ram_addr_t size);
> +uint64_t smbios_memory_device_size;
> };
>
> /**
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index b0c68d66a3..719e83e6a1 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -3308,6 +3308,7 @@ DEFINE_VIRT_MACHINE_AS_LATEST(9, 1)
> static void virt_machine_9_0_options(MachineClass *mc)
> {
> virt_machine_9_1_options(mc);
> +mc->smbios_memory_device_size = 16 * GiB;
> compat_props_add(mc->compat_props, hw_compat_9_0, hw_compat_9_0_len);
> }
> DEFINE_VIRT_MACHINE(9, 0)
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index bc38cad7f2..3cfdaec65d 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -1004,6 +1004,7 @@ static void machine_class_init(ObjectClass *oc, void
> *data)
> /* Default 128 MB as guest ram size */
> mc->default_ram_size = 128 * MiB;
> mc->rom_file_has_mr = true;
> +mc->smbios_memory_device_size = 2047 * TiB;
>
> /* numa node memory size aligned on 8MB by default.
> * On Linux, each node's border has to be 8MB aligned
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 9445b07b4f..d9e69243b4 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -495,6 +495,7 @@ static void pc_i440fx_machine_9_0_options(MachineClass *m)
> pc_i440fx_machine_9_1_options(m);
> m->alias = NULL;
> m->is_default = false;
> +m->smbios_memory_device_size = 16 * GiB;
>
> compat_props_add(m->compat_props, hw_compat_9_0, hw_compat_9_0_len);
>
Re: [PATCH v2 4/4] virtio-net: Remove fallback from ebpf-rss-fds
On Mon, Jul 08, 2024 at 04:38:09PM +0900, Akihiko Odaki wrote:
> If ebpf-rss-fds is specified but we fail to use, we should not fall
> back to loading eBPF programs by ourselves as such makes the situation
> complicated.
>
> Suggested-by: Daniel P. Berrangé
> Signed-off-by: Akihiko Odaki
> ---
> hw/net/virtio-net.c | 12 +++-
> 1 file changed, 3 insertions(+), 9 deletions(-)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index e779ba2df428..075c91f037d1 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -1396,15 +1396,9 @@ exit:
>
> static bool virtio_net_load_ebpf(VirtIONet *n)
> {
> -bool ret = false;
> -
> -if (virtio_net_attach_ebpf_to_backend(n->nic, -1)) {
> -if (!(n->ebpf_rss_fds && virtio_net_load_ebpf_fds(n))) {
> -ret = ebpf_rss_load(>ebpf_rss);
> -}
> -}
> -
> -return ret;
> +return virtio_net_attach_ebpf_to_backend(n->nic, -1) &&
> + (n->ebpf_rss_fds ? virtio_net_load_ebpf_fds(n) :
> + ebpf_rss_load(>ebpf_rss));
> }
>
> static void virtio_net_unload_ebpf(VirtIONet *n)
The eventual caller has an Error object that needs to be filled with
error details, not warn_report().
IMHO this patch needs to look like this:
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 9c7e85caea..c7fd52bbe9 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -1314,21 +1314,21 @@ static void virtio_net_disable_rss(VirtIONet *n)
virtio_net_commit_rss_config(n);
}
-static bool virtio_net_load_ebpf_fds(VirtIONet *n)
+static bool virtio_net_load_ebpf_fds(VirtIONet *n, Error **errp)
{
int fds[EBPF_RSS_MAX_FDS] = { [0 ... EBPF_RSS_MAX_FDS - 1] = -1};
int ret = true;
int i = 0;
if (n->nr_ebpf_rss_fds != EBPF_RSS_MAX_FDS) {
-warn_report("Expected %d file descriptors but got %d",
-EBPF_RSS_MAX_FDS, n->nr_ebpf_rss_fds);
- return false;
- }
+error_setg(errp, "Expected %d file descriptors but got %d",
+ EBPF_RSS_MAX_FDS, n->nr_ebpf_rss_fds);
+return false;
+}
for (i = 0; i < n->nr_ebpf_rss_fds; i++) {
fds[i] = monitor_fd_param(monitor_cur(), n->ebpf_rss_fds[i],
- _warn);
+ errp);
if (fds[i] < 0) {
ret = false;
goto exit;
@@ -1347,14 +1347,15 @@ exit:
return ret;
}
-static bool virtio_net_load_ebpf(VirtIONet *n)
+static bool virtio_net_load_ebpf(VirtIONet *n, Error **errp)
{
bool ret = false;
if (virtio_net_attach_ebpf_to_backend(n->nic, -1)) {
-if (!(n->ebpf_rss_fds && virtio_net_load_ebpf_fds(n))) {
+if (n->ebpf_rss_fds)
+ret = virtio_net_load_ebpf_fds(n, errp);
+else
ret = ebpf_rss_load(>ebpf_rss);
-}
}
return ret;
@@ -3750,7 +3751,7 @@ static void virtio_net_device_realize(DeviceState *dev,
Error **errp)
net_rx_pkt_init(>rx_pkt);
if (virtio_has_feature(n->host_features, VIRTIO_NET_F_RSS)) {
-virtio_net_load_ebpf(n);
+virtio_net_load_ebpf(n, errp);
}
}
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 v2 1/4] qdev-properties: Add DEFINE_PROP_ON_OFF_AUTO_BIT64()
On Mon, Jul 08, 2024 at 06:43:02AM -0400, Michael S. Tsirkin wrote:
> On Mon, Jul 08, 2024 at 04:38:06PM +0900, Akihiko Odaki wrote:
> > DEFINE_PROP_ON_OFF_AUTO_BIT64() corresponds to DEFINE_PROP_ON_OFF_AUTO()
> > as DEFINE_PROP_BIT64() corresponds to DEFINE_PROP_BOOL(). The difference
> > is that DEFINE_PROP_ON_OFF_AUTO_BIT64() exposes OnOffAuto instead of
> > bool.
> >
> > Signed-off-by: Akihiko Odaki
>
> There are a bunch of compatibility issues here.
> One is that PROP_BIT accepts different values:
>
>
> bool qapi_bool_parse(const char *name, const char *value, bool *obj, Error
> **errp)
> {
> if (g_str_equal(value, "on") ||
> g_str_equal(value, "yes") ||
> g_str_equal(value, "true") ||
> g_str_equal(value, "y")) {
> *obj = true;
> return true;
> }
> if (g_str_equal(value, "off") ||
> g_str_equal(value, "no") ||
> g_str_equal(value, "false") ||
> g_str_equal(value, "n")) {
> *obj = false;
> return true;
> }
>
> error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name,
>"'on' or 'off'");
> return false;
> }
That's just in relation to the CLI string parsing behaviour.
It is also broken at the JSON level, since
"rss": true
no longer works with device_add / -device JSON syntax.
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] i386/sev: Don't allow automatic fallback to legacy KVM_SEV*_INIT
On Tue, Jul 09, 2024 at 11:03:19PM -0500, Michael Roth wrote:
> On Thu, Jul 04, 2024 at 11:53:33AM +0200, Paolo Bonzini wrote:
> > On Thu, Jul 4, 2024 at 11:39 AM Daniel P. Berrangé
> > wrote:
> > > > The debug_swap parameter simply could not be enabled in the old API
> > > > without breaking measurements. The new API *is the fix* to allow using
> > > > it (though QEMU doesn't have the option plumbed in yet). There is no
> > > > extensibility.
> > > >
> > > > Enabling debug_swap by default is also a thorny problem; it cannot be
> > > > enabled by default because not all CPUs support it, and also we'd have
> > > > the same problem that we cannot enable debug_swap on new machine types
> > > > without requiring a new kernel. Tying the default to the -cpu model
> > > > would work but it is confusing.
> > >
> > > Presumably we can tie it to '-cpu host' without much problem, and
> > > then just leave it as an opt-in feature flag for named CPU models.
> >
> > '-cpu host' for SEV-SNP is also problematic, since CPUID is part of
> > the measurement. It's okay for starting guests in a quick and dirty
> > manner, but it cannot be used if measurement is in use.
> >
> > It's weird to have "-cpu" provide the default for "-object", since
> > -object is created much earlier than CPUs. But then "-cpu" itself is
> > weird because it's a kind of "factory" for future objects. Maybe we
> > should redo the same exercise I did to streamline machine
> > initialization, this time focusing on -cpu/-machine/-accel, to
> > understand the various phases and where sev-{,snp-}guest
> > initialization fits.
> >
> > > > I think it's reasonable if the fix is displayed right into the error
> > > > message. It's only needed for SEV-ES though, SEV can use the old and
> > > > new APIs interchangeably.
> > >
> > > FYI currently it is proposed to unconditionally force set
> > > legacy-vm-type=true
> > > in libvirt, so QEMU guests would *never* use the new ioctl, to fix what we
> > > consider to be a QEMU / kernel guest ABI regression.
> >
> > Ok, so it's probably best to apply both this and your patch for now.
> > Later debug-swap can be enabled and will automatically disable
> > legacy-vm-type if the user left it to the default.
>
> I think this seems like the ideal default behavior, where QEMU will
> continue to stick with legacy interface unless the user specifically
> enables a new option like debug-swap that relies on KVM_SEV_INIT2.
> So I reworked this patch to make legacy-vm-type an OnOffAuto field,
> where 'auto' implements the above semantics, and 'on'/'off' continue
> to behave as they do here in v1.
>
> At the moment, since QEMU doesn't actually expose anything that requires
> KVM_SEV_INIT2 for SEV-ES, setting legacy-vm-type to 'auto' or 'on' end
> up being equivalent, but by defaulting to 'auto' things will continue to
> "just work" on the libvirt side even when new features are enabled. And
> by adding a bit of that infrastructure now it's less likely that some
> option gets exposed in a way that doesn't abide by the above semantics.
>
> So as part of v2 I switched the default for 9.1+ to 'auto'. But if
> v1+Daniel's patch is still preferred that should be fine too.
I think your v2 patch is good on its own. It avoids the issues that
concerned me and has sensible future behaviour too.
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 v2] i386/sev: Don't allow automatic fallback to legacy KVM_SEV*_INIT
On Tue, Jul 09, 2024 at 11:10:05PM -0500, Michael Roth wrote: > Currently if the 'legacy-vm-type' property of the sev-guest object is > 'on', QEMU will attempt to use the newer KVM_SEV_INIT2 kernel > interface in conjunction with the newer KVM_X86_SEV_VM and > KVM_X86_SEV_ES_VM KVM VM types. > > This can lead to measurement changes if, for instance, an SEV guest was > created on a host that originally had an older kernel that didn't > support KVM_SEV_INIT2, but is booted on the same host later on after the > host kernel was upgraded. > > Instead, if legacy-vm-type is 'off', QEMU should fail if the > KVM_SEV_INIT2 interface is not provided by the current host kernel. > Modify the fallback handling accordingly. > > In the future, VMSA features and other flags might be added to QEMU > which will require legacy-vm-type to be 'off' because they will rely > on the newer KVM_SEV_INIT2 interface. It may be difficult to convey to > users what values of legacy-vm-type are compatible with which > features/options, so as part of this rework, switch legacy-vm-type to a > tri-state OnOffAuto option. 'auto' in this case will automatically > switch to using the newer KVM_SEV_INIT2, but only if it is required to > make use of new VMSA features or other options only available via > KVM_SEV_INIT2. > > Defining 'auto' in this way would avoid inadvertantly breaking > compatibility with older kernels since it would only be used in cases > where users opt into newer features that are only available via > KVM_SEV_INIT2 and newer kernels, and provide better default behavior > than the legacy-vm-type=off behavior that was previously in place, so > make it the default for 9.1+ machine types. > > Cc: Daniel P. Berrangé > Cc: Paolo Bonzini > cc: k...@vger.kernel.org > Signed-off-by: Michael Roth > --- > v2: > - switch to OnOffAuto for legacy-vm-type 'property' > - make 'auto' the default for 9.1+, which will automatically use > KVM_SEV_INIT2 when strictly required by a particular set of options, > but will otherwise keep using the legacy interface. > > hw/i386/pc.c | 2 +- > qapi/qom.json | 18 ++ > target/i386/sev.c | 85 +++ > 3 files changed, 83 insertions(+), 22 deletions(-) Reviewed-by: Daniel P. Berrangé 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] qdev-monitor: QAPIfy QMP device_add
On Mon, Jul 08, 2024 at 04:30:27PM +0200, Stefan Hajnoczi wrote:
> The QMP device_add monitor command converts the QDict arguments to
> QemuOpts and then back again to QDict. This process only supports scalar
> types. Device properties like virtio-blk-pci's iothread-vq-mapping (an
> array of objects) are silently dropped by qemu_opts_from_qdict() during
> the QemuOpts conversion even though QAPI is capable of validating them.
> As a result, hotplugging virtio-blk-pci devices with the
> iothread-vq-mapping property does not work as expected (the property is
> ignored). It's time to QAPIfy QMP device_add!
>
> Get rid of the QemuOpts conversion in qmp_device_add() and call
> qdev_device_add_from_qdict() with from_json=true. Using the QMP
> command's QDict arguments directly allows non-scalar properties.
>
> The HMP is also adjusted since qmp_device_add()'s now expects properly
> typed JSON arguments and cannot be used from HMP anymore. Move the code
> that was previously in qmp_device_add() (with QemuOpts conversion and
> from_json=false) into hmp_device_add() so that its behavior is
> unchanged.
>
> This patch changes the behavior of QMP device_add but not HMP
> device_add. QMP clients that sent incorrectly typed device_add QMP
> commands no longer work. This is a breaking change but clients should be
> using the correct types already. See the netdev_add QAPIfication in
> commit db2a380c8457 for similar reasoning.
>
> Markus helped me figure this out and even provided a draft patch. The
> code ended up very close to what he suggested.
>
> Suggested-by: Markus Armbruster
> Cc: Daniel P. Berrangé
> Signed-off-by: Stefan Hajnoczi
> ---
> system/qdev-monitor.c | 41 -
> 1 file changed, 28 insertions(+), 13 deletions(-)
I think we're justified in saying that applications should have been
using the correct types already.
On the libvirt side we already switched to using JSON for -device,
which has forced us to ensure we're using the correct types. So
the risk of converting device_add is minimal from libvirt's POV.
Other non-libvirt mgmt apps might get tripped up. Fixing those
should not be too difficult and fixed code would remain compatible
with older QEMU versions too.
Reviewed-by: Daniel P. Berrangé
>
> diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
> index 6af6ef7d66..1427aa173c 100644
> --- a/system/qdev-monitor.c
> +++ b/system/qdev-monitor.c
> @@ -849,18 +849,9 @@ void hmp_info_qdm(Monitor *mon, const QDict *qdict)
>
> void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp)
> {
> -QemuOpts *opts;
> DeviceState *dev;
>
> -opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict, errp);
> -if (!opts) {
> -return;
> -}
> -if (!monitor_cur_is_qmp() && qdev_device_help(opts)) {
> -qemu_opts_del(opts);
> -return;
> -}
> -dev = qdev_device_add(opts, errp);
> +dev = qdev_device_add_from_qdict(qdict, true, errp);
> if (!dev) {
> /*
> * Drain all pending RCU callbacks. This is done because
> @@ -872,8 +863,6 @@ void qmp_device_add(QDict *qdict, QObject **ret_data,
> Error **errp)
> * to the user
> */
> drain_call_rcu();
> -
> -qemu_opts_del(opts);
> return;
> }
> object_unref(OBJECT(dev));
> @@ -967,8 +956,34 @@ void qmp_device_del(const char *id, Error **errp)
> void hmp_device_add(Monitor *mon, const QDict *qdict)
> {
> Error *err = NULL;
> +QemuOpts *opts;
> +DeviceState *dev;
>
> -qmp_device_add((QDict *)qdict, NULL, );
> +opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict, );
> +if (!opts) {
> +goto out;
> +}
> +if (qdev_device_help(opts)) {
> +qemu_opts_del(opts);
> +return;
> +}
> +dev = qdev_device_add(opts, );
> +if (!dev) {
> +/*
> + * Drain all pending RCU callbacks. This is done because
> + * some bus related operations can delay a device removal
> + * (in this case this can happen if device is added and then
> + * removed due to a configuration error)
> + * to a RCU callback, but user might expect that this interface
> + * will finish its job completely once qmp command returns result
> + * to the user
> + */
> +drain_call_rcu();
> +
> +qemu_opts_del(opts);
> +}
> +object_unref(OBJECT(dev));
> +out:
> hmp_handle_error(mon, err);
> }
>
> --
> 2.45.2
>
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: [RFC PATCH v4 2/7] rust: add bindgen step as a meson dependency
On Tue, Jul 09, 2024 at 02:28:38PM +0200, Paolo Bonzini wrote: > On Tue, Jul 9, 2024 at 2:09 PM Peter Maydell wrote: > > * what is the actual baseline requirement? We definitely want > >to support "using rustup on an older system" (should be no > >problem) and "current distro building QEMU using the distro's > >rust", I assume. It would certainly be nice to have "building > >QEMU on the older-but-still-in-our-support-list distro releases > >with that distro's rust", but this probably implies not just > >a minimum rust version but also a limited set of crates. > > I don't think limiting ourselves to the set of crates in the distro is > feasible. It's not the way the language works, for example I tried > checking if the "cstr" crate exists and I didn't find it in Debian. Yep, Rust is new enough that it is highly likely that crates will be missing in multiple distros. For ordinary users, cargo will happily download the missing pieces so its not an issue. For distro packagers, they'll just have to either package up the crates, or bundle them in their QEMU build. Cargo makes the latter easy at least. If distros don't want bundling, they'll need to go the more involved route of packaging deps. IOW, from a distro POV, IMHO, we should focus on the Rust toolchain versions we need as the minimum bar. 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: [RFC PATCH v4 0/7] Add Rust support, implement ARM PL011
On Tue, Jul 09, 2024 at 09:54:43AM +0200, Paolo Bonzini wrote: > On Tue, Jul 9, 2024 at 9:38 AM Manos Pitsidianakis > wrote: > > Ah, alright. That wasn't obvious because that e-mail was not directed > > to me nor did it mention my name :) > > Oh, ok. Sorry about that. Generally when I say "we" I include as large > a part of the community as applicable. > > > I do not want to do that, in any case. I do not think it's the right > > approach. > > No problem with that (and in fact I agree, as I'd prefer a speedy > merge and doing the work on the QEMU master branch); however, we need > to reach an agreement on that and everybody (including Daniel) needs > to explain the reason for their position. > > Daniel's proposed criteria for merging include: > - CI integration > - CI passing for all supported targets (thus lowering the MSRV to 1.63.0) > - plus any the code changes that were or will be requested during review > > That seems to be a pretty high amount of work, and until it's done > everyone else is unable to contribute, not even in directions > orthogonal to the above (cross compilation support, less unsafe code, > porting more devices). My thought is that the initial merge focuses only on the build system integration. So that's basically patches 1 + 2 in this series. IMHO that is small enough that we should be able to demonstrate that we detect Rust, run bindgen & compile its result, on all our supported platforms without an unreasonable amount of effort. > So something has to give: either we decide for > an early merge, where the code is marked as experimental and disabled > by default. Personally I think it's fine, the contingency plan is > simply to "git rm -rf rust/". Or we can keep the above stringent > requirements for merging, but then I don't see it as a one-person job. Patch 3, the high level APIs is where I see most of the work and collaboration being needed, but that doesn't need to be rushed into the first merge. We would have a "rust" subsystem + maintainer who would presumably have a staging tree, etc in the normal way we work and collaborate 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: [RFC PATCH v4 0/7] Add Rust support, implement ARM PL011
On Mon, Jul 08, 2024 at 06:55:40PM +0200, Paolo Bonzini wrote: > Il lun 8 lug 2024, 18:33 Daniel P. Berrangé ha > scritto: > > > This series is still missing changes to enable build on all targets > > during CI, including cross-compiles, to prove that we're doing the > > correct thing on all our targetted platforms. That's a must have > > before considering it suitable for merge. > > > > But we're not—in particular it's still using several features not in all > supported distros. That's exactly why I suggest its a pre-requisite for merging this. Unless we're able to demonstrate that we can enable Rust on all our CI platforms, the benefits of Rust will not be realized in QEMU, and we'll have never ending debates about whether each given feature needs to be in C or Rust. > I also believe we should default to enabling rust toolchain by > > default in configure, and require and explicit --without-rust > > to disable it, *despite* it not technically being a mandatory > > featureyet. > > > > I guess the detection could be done, but actually enabling the build part > needs to wait until the minimum supported version is low 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 :|
