- Devel - libvirt List Archives

[PATCH] util: remove ATTRIBUTE_NONNULL from virDirClose declaration
by Laine Stump 13 Nov '20

13 Nov '20

Before commit 24d8968c, virDirClose took a DIR**, and that was never NULL, so its declaration included ATTRIBUTE_NONNULL(1). Since that commit, virDirClose takes a DIR*, and it may be NULL (e.g. if the DIR* is initialized to NULL and was never closed). Even though virDirClose() is currently only called implicitly (as the cleanup for a g_autoptr(DIR)), and (as I've just newly learned) the autocleanup function g_autoptr will only be called if the pointer in question is non-null (see the definition of _GLIB_AUTOPTR_CLEAR_FUNC_NAME in /usr/include/glib-2.0/glib/gmacros.h), it does still cause Coverity to complain that it *could* be called with a NULL, and it's also possible that in the future someone might add code that explicitly calls virDirClose. To eliminate the Coverity complaints, and protect against the hypothetical future where someone both explicitly calls virDirClose() with a potentially NULL value, *and* re-enables the nonnull directive when not building with Coverity (disabled by commit eefb881) this patch removes the ATTRIBUTE_NONNULL(1) from the declaration of virDirClose(). Fixes: 24d8968cd0a718af4badbbc858b1b449fea7205a Details-Research-by: Dan Berrange <berrange(a)redhat.com> Signed-off-by: Laine Stump <laine(a)redhat.com> --- src/util/virfile.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/util/virfile.h b/src/util/virfile.h index ba31a78e58..32505826ee 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -269,8 +269,7 @@ int virDirOpenQuiet(DIR **dirp, const char *dirname) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT; int virDirRead(DIR *dirp, struct dirent **ent, const char *dirname) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT; -void virDirClose(DIR *dirp) - ATTRIBUTE_NONNULL(1); +void virDirClose(DIR *dirp); G_DEFINE_AUTOPTR_CLEANUP_FUNC(DIR, virDirClose); int virFileMakePath(const char *path) G_GNUC_WARN_UNUSED_RESULT; -- 2.28.0

1 1

[PATCH 1/1] qemu_driver.c: do not redefine 'event' in qemuDomainDefineXMLFlags()
by Daniel Henrique Barboza 13 Nov '20

13 Nov '20

A bad merge while rebasing 74b2834333a caused the @event variable to be defined twice, inside the 'cleanup' label, causing coverity errors. This code was originally moved outside of the label by commit 773c7c43611a. Delete the unintended code in the 'cleanup' label. Fixes: 74b2834333ab3bf500f870e0a6d4e8309379d96a Reported-by: John Ferlan <jferlan(a)redhat.com> Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com> --- src/qemu/qemu_driver.c | 9 --------- 1 file changed, 9 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 78ccf96df5..ac38edf009 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6748,15 +6748,6 @@ qemuDomainDefineXMLFlags(virConnectPtr conn, } } - event = virDomainEventLifecycleNewFromObj(vm, - VIR_DOMAIN_EVENT_DEFINED, - !oldDef ? - VIR_DOMAIN_EVENT_DEFINED_ADDED : - VIR_DOMAIN_EVENT_DEFINED_UPDATED); - - VIR_INFO("Creating domain '%s'", vm->def->name); - dom = virGetDomain(conn, vm->def->name, vm->def->uuid, vm->def->id); - virDomainObjEndAPI(&vm); virObjectEventStateQueue(driver->domainEventState, event); return dom; -- 2.26.2

2 1

[PATCH v1 00/10] more simple g_autoptr() cleanups
by Daniel Henrique Barboza 13 Nov '20

13 Nov '20

I intended to clean up the virDomainDefFree() calls that I kept seeing in qemu_driver.c, ended up doing a bit more than that. Daniel Henrique Barboza (10): qemu_driver.c: use g_autoptr() with virDomainDef pointers qemu_driver.c: use g_autoptr() with qemuMigrationParams pointers qemu_driver.c: use g_autoptr() with virDomainDeviceDefPtr qemu_driver.c: remove unneeded 'cleanup' labels qemu_snapshot.c: remove uneeded 'cleanup' label in qemuSnapshotDelete() qemu_domain.c: use g_autoptr() with virDomainDef pointers qemu_domain.c: remove unneeded cleanup labels qemu_domain.c: modernize qemuDomainFixupCPUs() qemu_domain.c: modernize qemuDomainWriteMasterKeyFile() qemu_domain.c: modernize qemuMonitorGetCpuHalted() src/qemu/qemu_domain.c | 100 ++++++------------ src/qemu/qemu_driver.c | 219 +++++++++++++++------------------------ src/qemu/qemu_snapshot.c | 3 +- 3 files changed, 119 insertions(+), 203 deletions(-) -- 2.26.2

3 14

[PATCH] virnetdevopenvswitch: Fix ATTRIBUTE_NONNULL() tag for virNetDevOpenvswitchGetVhostuserIfname()
by Michal Privoznik 13 Nov '20

13 Nov '20

After e4c29e2904 the function has one argument more and the argument that can't be NULL moved from second to third position. Reported-by: John Ferlan <jferlan(a)redhat.com> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com> --- Pushed as trivial. src/util/virnetdevopenvswitch.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virnetdevopenvswitch.h b/src/util/virnetdevopenvswitch.h index 5cd1d22ae3..8409aa92ac 100644 --- a/src/util/virnetdevopenvswitch.h +++ b/src/util/virnetdevopenvswitch.h @@ -63,7 +63,7 @@ int virNetDevOpenvswitchInterfaceGetMaster(const char *ifname, char **master) int virNetDevOpenvswitchGetVhostuserIfname(const char *path, bool server, char **ifname) - ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT G_GNUC_NO_INLINE; + ATTRIBUTE_NONNULL(3) G_GNUC_WARN_UNUSED_RESULT G_GNUC_NO_INLINE; int virNetDevOpenvswitchUpdateVlan(const char *ifname, const virNetDevVlan *virtVlan) -- 2.26.2

1 0

[libvirt PATCH] kbase: Shorten "less verbose QEMU logging" example
by Andrea Bolognani 13 Nov '20

13 Nov '20

Rationale for the changes: * util.dbus produces very little logging, so it doesn't really make sense to filter it out; * util.udev doesn't exist; * access can be filtered out entirely, as nothing very interesting is produced by the only other component in the same package (access.accessdriverpolkit). Signed-off-by: Andrea Bolognani <abologna(a)redhat.com> --- docs/kbase/debuglogs.rst | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/docs/kbase/debuglogs.rst b/docs/kbase/debuglogs.rst index 7945cf8bed..fed998efd8 100644 --- a/docs/kbase/debuglogs.rst +++ b/docs/kbase/debuglogs.rst @@ -178,13 +178,12 @@ Less verbose logging for QEMU VMs Some subsystems are very noisy and usually not the culprit of the problems. They can be silenced individually for a less verbose log while still logging -everything else. Usual suspects are the JSON code, udev, authentication and such. +everything else. Usual suspects are the JSON code, RPC, authentication and such. A permissive filter is good for development use cases. :: - 3:remote 4:event 3:util.json 3:util.object 3:util.dbus 3:util.udev 3:node_device 3:rpc 3:access.accessmanager 3:util.netlink 1:* - + 3:remote 4:event 3:util.json 3:util.object 3:util.netlink 3:node_device 3:rpc 3:access 1:* Minimalistic QEMU QMP monitor logging ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -- 2.26.2

3 3

[PATCH 0/2] virnetdevopenvswitch: Get names for dpdkvhostuserclient too
by Michal Privoznik 13 Nov '20

13 Nov '20

The first patch fixes the actual problem, the second is just a cleanup I've came up with whilst looking at the code. Michal Prívozník (2): virnetdevopenvswitch: Get names for dpdkvhostuserclient too virnetdevopenvswitch: Simplify OVS_VSCTL cmd creation src/qemu/qemu_command.c | 1 + src/qemu/qemu_hotplug.c | 1 + src/util/virnetdevopenvswitch.c | 92 +++++++++++++++++++-------------- src/util/virnetdevopenvswitch.h | 1 + tests/qemuxml2argvmock.c | 1 + 5 files changed, 56 insertions(+), 40 deletions(-) -- 2.26.2

3 5

[libvirt PATCH] docs: compiling.html: pass -d to xz to decompress
by Daniel P. Berrangé 13 Nov '20

13 Nov '20

From: Jonathan Watt <jwatt(a)jwatt.org> tar on macOS recognizes XZ compression automatically, but that is not the case for GNU tar (1.32 at least). On Fedora 33 the current instructions result in the following error: $ xz -c libvirt-6.9.0.tar.xz | tar xvf - tar: Archive is compressed. Use -J option tar: Error is not recoverable: exiting now Reviewed-by: Daniel P. Berrangé <berrange(a)redhat.com> Signed-off-by: Jonathan Watt <jwatt(a)jwatt.org> --- Pushed as a trivial patch from gitlab docs/compiling.html.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/compiling.html.in b/docs/compiling.html.in index c898d16313..c2c9d9f0ed 100644 --- a/docs/compiling.html.in +++ b/docs/compiling.html.in @@ -14,7 +14,7 @@ </p> <pre> -$ xz -c libvirt-x.x.x.tar.xz | tar xvf - +$ xz -dc libvirt-x.x.x.tar.xz | tar xvf - $ cd libvirt-x.x.x $ meson build</pre> -- 2.28.0

1 0

[PATCH 0/6] qemu: Enable client TLS certificate validation by default for chardev, migration, and backup servers
by Peter Krempa 13 Nov '20

13 Nov '20

See patches 6 for a explanation. Peter Krempa (6): qemu: conf: Allow individual control of default value for *_tls_x509_verify qemu: conf: Clarify default of "vnc_tls_x509_verify" qemu: conf: Enable 'chardev_tls_x509_verify' by default qemu: conf: Enable 'migrate_tls_x509_verify' by default qemu: conf: Enable 'backup_tls_x509_verify' by default NEWS: Mention change of default for TLS certificate verification NEWS.rst | 11 +++++++++++ src/qemu/qemu.conf | 18 ++++++++++++++---- src/qemu/qemu_conf.c | 22 ++++++++++++++-------- src/qemu/qemu_conf.h | 1 + 4 files changed, 40 insertions(+), 12 deletions(-) -- 2.28.0

5 14

Re: [PATCH] Added attach-disk parameters for network disk support
by Peter Krempa 13 Nov '20

13 Nov '20

On Thu, Nov 12, 2020 at 15:15:27 -0600, Ryan Gahagan wrote: > Hey Peter, Hi Ryan, firstly I'd like to ask you to keep the conversation on-list. (use reply-all all the time). This ensures that the response is public and also other people can jump in and respont. At the very least it can be used for future reference. I'll be re-adding the mailing list in this response. Also please avoid top-posting in techincal lists. It's always best to respond inline at the proper context that is being discussed. > Sorry about all the issues with our commit, we're new to this and trying to > make our first contribution to this repo. We had a couple questions over > your feedback and wanted to run them by you before making another request. Sure thing, you can always ask on the mailing list even without patches. > In the original code, the --source parameter is marked as required but okay > to be empty. If the string is empty, then we think the parameter read in by > vshCommandOptStringReq would be NULL, in which case no <source> tag would > be generated at all. If this is the case, then why is --source required in > the first place? We see that --source-name is mutually exclusive witha That's a quirk of it being a positional argument (VSH_OT_DATA). A positional argument is mandatory. > --source, but is it also okay to be empty? For example, in the issue > <https://gitlab.com/libvirt/libvirt/-/issues/16> this patch was based on, > there's an example XML which has a <source> with neither a file nor a name. > Is this example incorrect, or is it fine to have neither and only have a > protocol? That is correct for example with the NBD protocol where a missing name actually means the default unnamed export. The thing is that we can't really change --source to be something else than VSH_OT_DATA since it would change the semantics of the virsh command. You probably will need to use it also for the 'name' attribute and switch the name according to the presence of the 'protocol' attribute/option. > When it comes to making these mutually exclusive, we also were confused on > what flags to give them. Would it be proper behavior to give them both no > flags or only the VSH_OFLAG_EMPTY_OK flag and then check that one is > defined in the cmdAttachDisk method (using VSH_EXCLUSIVE_OPTIONS_VAR) and > error there instead? Those are two separate things. If it makes sense for a flag to be specified but empty and it impacts behaviour then you need to use VSH_OFLAG_EMPTY_OK. Any non-positional arguments (those which need --) can be made optional. On the other hand that does not say anything about how they can or can't be combined with other options and thus you need to declare that in the code. > If the user provides an empty source without any protocol, but provides > --source-host-* parameters, what should be generated instead? Would this This should be an error. --source-host must require the use of --source-protocol > cause an error, would it generate an in-line <host> tag, or would it > generate a completely empty <source> tag followed by the host information? The protocol option flag should be mandatory if you want to use the host/socket. > > -Ryan Gahagan > > On Thu, Nov 12, 2020 at 2:57 AM Peter Krempa <pkrempa(a)redhat.com> wrote: > > > On Wed, Nov 11, 2020 at 17:00:51 -0600, Ryan Gahagan wrote: > > > Related issue: https://gitlab.com/libvirt/libvirt/-/issues/16 > > > Added in support for the following parameters in attach-disk: > > > --source-name > > > --source-protocol > > > --source-host-name > > > --source-host-socket > > > --source-host-transport Please trim parts of the message that are no longer relevant. [...]

1 0

[PATCH] viridentitytest: Run more frequently
by Michal Privoznik 13 Nov '20

13 Nov '20

The viridentitytest tests our viridentity module which is compiled on all platforms and OSes. There is no need to have SELinux secdriver as individual test cases are skipped if SELinux is missing. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com> --- Successfully build here: https://gitlab.com/MichalPrivoznik/libvirt/-/pipelines/213586713 While technically this is a v2 of: https://www.redhat.com/archives/libvir-list/2020-November/msg00308.html it's implementing completely different approach and thus I did not mark it as v2. tests/meson.build | 2 +- tests/viridentitytest.c | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/tests/meson.build b/tests/meson.build index 8decdfa20c..9f0051303d 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -312,6 +312,7 @@ tests += [ { 'name': 'virfirewalltest' }, { 'name': 'virhostcputest', 'link_whole': [ test_file_wrapper_lib ] }, { 'name': 'virhostdevtest' }, + { 'name': 'viridentitytest' }, { 'name': 'viriscsitest' }, { 'name': 'virkeycodetest' }, { 'name': 'virkmodtest' }, @@ -480,7 +481,6 @@ if conf.has('WITH_SECDRIVER_SELINUX') if conf.has('WITH_LIBATTR') tests += [ { 'name': 'securityselinuxtest' }, - { 'name': 'viridentitytest' }, ] if conf.has('WITH_QEMU') diff --git a/tests/viridentitytest.c b/tests/viridentitytest.c index fd01eeaa80..afb9fdaec4 100644 --- a/tests/viridentitytest.c +++ b/tests/viridentitytest.c @@ -88,8 +88,7 @@ static int testIdentityGetSystem(const void *data) #if !WITH_SELINUX if (context) { VIR_DEBUG("libvirt not compiled with SELinux, skipping this test"); - ret = EXIT_AM_SKIP; - return -1; + return EXIT_AM_SKIP; } #endif -- 2.26.2

3 2