The 'black' tool is intended to be an opinionated formatting
tool for python code. It is complementary to flake8 which
validates coding bad practices, but (mostly) ignores code
layout issues.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The flake8 check W503 does not want a line break before
binary operator. This is contrary to the style that the
'black' formatting tool wants to use. Defer to 'black'
as it is intended to be an opinionated formatting tool
standardizing python code style, and thus not to be
customized per project.
The flake8 check E203 does not want whitespace before
a ':'. This is, however, desirable when indexing array
slices eg
self.lookahead[skip : skip + 1]
which is a format that 'black' produces.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The current RPC code is post-processed to introduce an
intermediate variable, rather than casting directly
to char ** at time of use. This is said to be a workaround
for type-puning warnings that the compiler emitted.
Neither GCC or CLang emit any warnings for the code in
question today, across any of the architectures we
test in CI. Thus it is presumed that somewhere in the
15 years since the workaround was done, the compilers
have got smarter and do the right thing.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
This environment variable is supposedly set according to the contents
of ~/.CFUserTextEncoding, and certainly on MacOS 14 (Sonoma) it is set
in the environment of child processes created by execve() (used by
virCommand()), causing commandtest to fail. (However, the value that is
shown in $__CF_USER_TEXT_ENCODING during the test 1) is not in the
environment of the shell the test is run from, and 2) doesn't match
the contents of ~/.CFUserTextEncoding.)
It is true, though, that filtering out this environment setting from
the test results permits commandtest to pass on macOS 14 (Sonoma).
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Commit changing the code to allow passing NULL as @data into
qemuSaveImageDecompressionStart() was not correct as it left the
original call into the function as well.
Introduced-by: 2f3e582a1a
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2247754
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
This is the default for the version of rpcgen shipped with
Linux distributions, but the one in macOS and possibly others
default to K&R C, which modern compilers don't appreciate.
Luckily, all versions of rpcgen shipped with our target
platforms seem to support the -C option.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_SKIP_UNMAP is no longer
referenced inside the code.
QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_AUTO_READONLY is passed from
various code paths to the qemuBlockStorageSourceGetBackendProps helper,
but it's no longer used.
Both thus can be removed.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The code was refactored to format the 'read-only' and 'auto-read-only'
flags via the common helper, so the logic determining their values can
be removed.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Use the qemuBlockStorageSourceAddBlockdevCommonProps helper when
formatting protocol layer both when it's used as backing for a format
node and when it's used as the effective node.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The resulting properties are identical, as the hostdev backend code
doesn't set any of the extra properties.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Introduce a mode where the protocol layer -blockdev will be formatted
so that it can be used as the effective node (used to access data from
the device). For this new mode we'll use
qemuBlockStorageSourceAddBlockdevCommonProps.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Use the new helper in qemuBlockStorageSourceGetBlockdevStorageSliceProps
to format the common bits.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The new helper replaces qemuBlockStorageSourceGetBlockdevFormatCommonProps
and the two inline instances generating the common properties for a
blockdev layer.
The new helper is to be used for both the format layer and the storage
backing layer, thus a new parameter 'effective' switches between the
modes.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Use the same ordering of the relevant fields as we do for the format
layer -blockdev so that later they can be refactored without test
fallout.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Change the return value type to 'virDomainDiskGetDetectZeroes'.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
At this point only a single code path (for formatting -drive for legacy
SD cards) uses the 'legacy' output and that code path doesn't populate
the node name. Thus we can unify the code block and simplify the JSON
formatters.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The 'systemd-analyze security' command looks at the unit file
configuration and reports on any settings which increase the
attack surface for the daemon. Since most systemd units are
fairly minimalist, this is generally informing us about settings
that we never put any thought into using before.
In its current configuration it reports
# systemd-analyze security virtlogd.service
...snip...
→ Overall exposure level for virtlogd.service: 9.6 UNSAFE 😨
which is pretty terrible as a score.
If we apply all of the recommendations that appear possible
without (knowingly) breaking functionality it reports:
# systemd-analyze security virtlogd.service
...snip...
→ Overall exposure level for virtlogd.service: 2.2 OK 🙂
which is a pretty decent improvement.
Some of the settings we would like to enable require a systemd
version that is newer than that available in our oldest distro
target - RHEL-8 at v239.
NB, RestrictSUIDSGID is technically newer than 239, but RHEL-8
backported it, and other distros we target have it by default.
Remaining recommendations are
✗ CapabilityBoundingSet=~CAP_(DAC_*|FOWNER|IPC_OWNER)
We block FOWNER/IPC_OWNER, but can't block the two DAC
capabilities. Historically apps/users might point QEMU
to log files in $HOME, pre-created with their own user
ID.
✗ IPAddressDeny=
Not required since RestrictAddressFamilies blocks IP
usage. Ignoring this avoids the overhead of creating
a traffic filter than will never be used.
✗ NoNewPrivileges=
Highly desirable, but cannot enable it yet, because it
will block the ability to transition to the virtlogd_t
SELinux domain during execve. The SELinux policy needs
fixing to permit this transition under NNP first.
✗ PrivateTmp=
There is a decent chance people have VMs configured
with a serial port logfile pointing at /tmp. We would
cause a regression to use private /tmp for logging
✗ PrivateUsers=
This would put virtlogd inside a user namespace where
its root is in fact unprivileged. Same problem as the
User= setting below
✗ ProcSubset=
Libraries we link to might read certain non-PID related
files from /proc
✗ ProtectClock=
Requires v245
✗ ProtectHome=
Same problem as PrivateTmp=. There's a decent chance
that someone has a VM configured to write a logfile
to /home
✗ ProtectHostname=
Requires v241
✗ ProtectKernelLogs
Requires v244
✗ ProtectProc
Requires v247
✗ ProtectSystem=
We only set it to 'full', as 'strict' is not viable for
our required usage
✗ RootDirectory=/RootImage=
We are not capable of running inside a custom chroot
given needs to write log files to arbitrary places
✗ RestrictAddressFamilies=~AF_UNIX
We need AF_UNIX to communicate with other libvirt daemons
✗ SystemCallFilter=~@resources
We link to libvirt.so which links to libnuma.so which has
a constructor that calls set_mempolicy. This is highly
undesirable todo during a constructor.
✗ User=/DynamicUser=
This is highly desirable, but we currently read/write
logs as root, and directories we're told to write into
could be anywhere. So using a non-root user would have
a major risk of regressions for applications and also
have upgrade implications
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The hyperkitty search facility does a massively better job
than google docs for mailing lists.
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Setup the VDPA bits of the appropriate part of the image chain for block
copy.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The code which opens the VDPA device and prepares it for FD passing was
not called in the hotplug code path, preventing hotplug of VDPA disks
with:
error: internal error: argument key 'path' must not have null value
Use the new helper qemuProcessPrepareHostStorageDisk to setup the VDPA
definition.
Closes: https://gitlab.com/libvirt/libvirt/-/issues/539
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Currently the code sets up only VDPA backends but will be used later in
hotplug code too.
This patch also uses normal forward iteration in the loop in
qemuProcessPrepareHostStorage as we don't need to remove disks from the
disk list at that point.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Move the docs for the <acpi><table> element under a common section as
it's not specific for direct kernel boot. In fact the original use was
for Windows activation.
Fixes: 72f652da63
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Legacy mode used to be needed for use with -drive, which was almost
completely deleted. We now have qemuxml2argvtest test cases checking a
few cases and the rest uses the modern mode only. Thus we don't need to
test the legacy mode any more.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Condition handling failure of the first virTestRun was lacking the 'ret
= -1' line thus the subsequent line was taken as it's body rendering the
first invocation useless.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
QEMU deprecated and removed support for those protocols, but due to a
logic bug in the tests it was not caught. Remove the test cases first.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Update to v8.1.0-2161-ga95260486a
Notable changes:
- new CPU topology parameters: 'books', 'drawers'
- new migration parameter 'avail-switchover-bandwidth'
- new s390x CPU properties: 'dedicated', 'entitlement'
- new 'hostmem' parameter for 'virtio-gpu'
- new device 'piix4-isa'
- new property 'dynamic-memslots' of 'virtio-mem' device
- new 'gsi*' parameters of 'ICH9-LPC'
- new experimental 'x-south-bridge' machine option
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Given that this variable now controls not just whether C tests
are built, but also whether any test at all is executed, the new
name is more appropriate.
Update the description for the corresponding meson option
accordingly.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
Currently, passing -Dtests=disabled only disables a subset of
tests: those that are written in C and thus require compilation.
Other tests, such as the syntax-check ones and those that are
implemented as scripts, are always enabled.
There's a potentially dangerous consequence of this behavior:
when tests are disabled, 'meson test' will succeed as if they
had been enabled. No indication of this will be shown, so the
user will likely make the reasonable assumption that everything
is fine when in fact the significantly reduced coverage might
be hiding failures.
To solve this issues, disable *all* tests when asked to do so,
and inject an intentionally failing test to ensure that 'meson
test' doesn't succeed.
Best viewed with 'git show -w'.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
It only makes sense to enable expensive tests when tests are
enabled. Disallow invalid configurations.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
There are some cases in which we automatically disable tests when
using Clang as the compiler. If the user has explicitly asked for
tests to be enabled, however, we should error out instead of
silently disabling things.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
This will make future patches nicer.
Note that we need to handle these somewhat late because of the
dependency on information about the compiler and the flags it
supports.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
As explained in the comment, the syntax-check machinery uses git
to figure out the list of files it should operate on, so we can
only enable it when building from git.
Despite only registering the various tests with meson in that
case, however, we unconditionally perform a bunch of preparation
that is only useful for the purpose of registering and running
the tests. If we're not going to do that, we can skip a few steps
and save a bit of time.
Best viewed with 'git show -w'.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
Clang can be too aggressive at optimizations, which can end up
breaking our test suite. See f9f5ab5718 for details.
As a result of this, since 7944700b40 we are automatically
disabling tests when Clang is used unless it supports the
-fsemantic-interposition compiler flag.
Since the version of Clang included in macOS doesn't support that
compiler flag, we end up always disabling the test suite on that
platform.
This is already far from ideal, considering that it was just last
year when we finally managed to get the test suite to successfully
pass on macOS, and it would be a real shame if the situation
regressed again.
With the upcoming changes, which will turn running 'meson test'
into a hard failure if tests are disabled, this behavior will
result in every single pipeline failing.
Work around the problem the only way we can: disabling
optimizations entirely for the macOS CI jobs.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
Test cases that depend on duplicating fds are using fairly big
values as targets.
This works fine on Linux, where RLIMIT_NOFILE is 1024 by
default, but fails on macOS which uses 256 as the default.
Decrease the values so that they're valid across all platforms.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
Both tests pass a disk source definition which didn't go through the
preparation steps and thus contains only the target information that
were originally present, thus we should be using the
QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_TARGET_ONLY flag.
For the same reason QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_AUTO_READONLY
used in 'testJSONtoJSON' doesn't make sense.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Return whether a relevant cachemode was presented rather than returning
an error, so that callers can be simplified. Use the proper enum type as
argument rather than typecasting in the switch statement.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The aforementioned fields in virStorageSource struct are copies of the
disk properties, but were not converted to the proper type yet.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Certain disk config fields are mirrored between the disk and storage
source definitions, but the proper types are not available for use in
the virStorageSource definition. Move them so they can be used properly.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Formatting of the 'nbdkit' driven backend breaks out of the switch
statement so we don't need to have an unnecessary block and indentation
level for the case when nbdkit is not in use.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
'qemuBuildDriveSourceStr' used to build the legacy -drive commandline
for SD cards is the only user of qemuDiskSourceGetProps. Move the helper
directly inline.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The 'auto-read-only' blockdev option is available in all supported qemu
versions so we can remove the migration hack which disabled it.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Add a few examples of SD cards backed with network storage to capture
the current state as the formatter code is about to be refactored.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Daniel P. Berrangé