Re: RFC: do we want/need the "Ptr" typedefs for internal code ?

2021-03-09 Thread Peter Krempa
On Tue, Mar 09, 2021 at 17:44:16 +, Daniel Berrange wrote: > One of the conventions we have had since the early days of libvirt is > that every struct typedef, has a corresponding "Ptr" typedef too. > > For example > > typedef struct _virDomainDef virDomainDef; > typedef virDomainDef

[PATCH] virQEMUCapsInitQMPArch: Refactor cleanup

2021-03-09 Thread Yi Li
Switch to using the 'g_auto*' helpers. Signed-off-by: Yi Li --- src/qemu/qemu_capabilities.c | 13 - 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 790375ac38..447cf77875 100644 ---

Re: RFC: do we want/need the "Ptr" typedefs for internal code ?

2021-03-09 Thread Jiri Denemark
On Tue, Mar 09, 2021 at 17:44:16 +, Daniel P. Berrangé wrote: ... > We can't do anything about the use "Ptr" in the include/ files because > that is public ABI. We can potentially eliminate "Ptr" types everywhere > else in the codebase, even the src/libvirt*.c files corresponding to > the

Re: RFC: do we want/need the "Ptr" typedefs for internal code ?

2021-03-09 Thread Laine Stump
On 3/9/21 1:23 PM, Andrea Bolognani wrote: On Tue, 2021-03-09 at 17:44 +, Daniel P. Berrangé wrote: One of the conventions we have had since the early days of libvirt is that every struct typedef, has a corresponding "Ptr" typedef too. For example typedef struct _virDomainDef

Re: [PATCH 08/16] xenParseVif: Refactor parser

2021-03-09 Thread Ján Tomko
On a Tuesday in 2021, Peter Krempa wrote: Use g_strsplit to split the string and avoid use of stack'd strings. Signed-off-by: Peter Krempa --- src/libxl/xen_common.c | 136 ++--- 1 file changed, 46 insertions(+), 90 deletions(-) diff --git

Re: RFC: do we want/need the "Ptr" typedefs for internal code ?

2021-03-09 Thread Ján Tomko
On a Tuesday in 2021, Daniel P. Berrangé wrote: One of the conventions we have had since the early days of libvirt is that every struct typedef, has a corresponding "Ptr" typedef too. For example typedef struct _virDomainDef virDomainDef; typedef virDomainDef *virDomainDefPtr;

Re: RFC: do we want/need the "Ptr" typedefs for internal code ?

2021-03-09 Thread Daniel P . Berrangé
On Tue, Mar 09, 2021 at 07:23:15PM +0100, Andrea Bolognani wrote: > On Tue, 2021-03-09 at 17:44 +, Daniel P. Berrangé wrote: > > One of the conventions we have had since the early days of libvirt is > > that every struct typedef, has a corresponding "Ptr" typedef too. > > > > For example > >

Re: [PATCH v3 3/4] fdc: Inline fdctrl_connect_drives() into fdctrl_realize_common()

2021-03-09 Thread Richard Henderson
On 3/9/21 8:12 AM, Markus Armbruster wrote: @@ -2565,6 +2551,7 @@ static void fdctrl_realize_common(DeviceState *dev, FDCtrl *fdctrl, Error **errp) { int i, j; +FDrive *drive; static int command_tables_inited = 0; if

Re: RFC: do we want/need the "Ptr" typedefs for internal code ?

2021-03-09 Thread Andrea Bolognani
On Tue, 2021-03-09 at 17:44 +, Daniel P. Berrangé wrote: > One of the conventions we have had since the early days of libvirt is > that every struct typedef, has a corresponding "Ptr" typedef too. > > For example > > typedef struct _virDomainDef virDomainDef; > typedef virDomainDef

Re: [libvirt PATCH 14/18] qemu: add support for generating -audiodev arguments

2021-03-09 Thread Daniel P . Berrangé
On Tue, Mar 09, 2021 at 04:30:15PM +0100, Michal Privoznik wrote: > On 3/3/21 7:18 PM, Daniel P. Berrangé wrote: > > The -audiodev argument is replacing the QEMU_AUDIO_DRV env variable (and > > its relations). > > > > Sadly we still have to use the SDL_AUDIODRIVER env variable because that > >

Re: [libvirt PATCH 06/18] conf: refactor OSS audio backend specific options

2021-03-09 Thread Daniel P . Berrangé
On Tue, Mar 09, 2021 at 04:30:18PM +0100, Michal Privoznik wrote: > On 3/3/21 7:18 PM, Daniel P. Berrangé wrote: > > To prepare for the introduction for more backend specific audio options, > > move the OSS options into a dedicated struct and introduce separate > > helper methods for

RFC: do we want/need the "Ptr" typedefs for internal code ?

2021-03-09 Thread Daniel P . Berrangé
One of the conventions we have had since the early days of libvirt is that every struct typedef, has a corresponding "Ptr" typedef too. For example typedef struct _virDomainDef virDomainDef; typedef virDomainDef *virDomainDefPtr; Periodically someone has questioned what the purpose of

Re: [PATCH v3 4/4] blockdev: Drop deprecated bogus -drive interface type

2021-03-09 Thread Daniel P . Berrangé
On Tue, Mar 09, 2021 at 05:12:13PM +0100, Markus Armbruster wrote: > Drop the crap deprecated in commit a1b40bda08 "blockdev: Deprecate > -drive with bogus interface type" (v5.1.0). > > Signed-off-by: Markus Armbruster > --- > docs/system/deprecated.rst | 7 -- >

Re: [PATCH v3 3/4] fdc: Inline fdctrl_connect_drives() into fdctrl_realize_common()

2021-03-09 Thread Daniel P . Berrangé
On Tue, Mar 09, 2021 at 05:12:12PM +0100, Markus Armbruster wrote: > The previous commit rendered the name fdctrl_connect_drives() somewhat > misleading. Get rid of it by inlining the (now pretty simple) > function into its only caller. > > Signed-off-by: Markus Armbruster > --- >

Re: [PATCH v3 2/4] fdc: Drop deprecated floppy configuration

2021-03-09 Thread Daniel P . Berrangé
On Tue, Mar 09, 2021 at 05:12:11PM +0100, Markus Armbruster wrote: > Drop the crap deprecated in commit 4a27a638e7 "fdc: Deprecate > configuring floppies with -global isa-fdc" (v5.1.0). > > Signed-off-by: Markus Armbruster > --- > docs/system/deprecated.rst | 49 --- >

Re: [PATCH v3 1/4] docs/system/deprecated: Fix note on fdc drive properties

2021-03-09 Thread Daniel P . Berrangé
On Tue, Mar 09, 2021 at 05:12:10PM +0100, Markus Armbruster wrote: > Commit 4a27a638e7 "fdc: Deprecate configuring floppies with -global > isa-fdc" actually deprecated any use of floppy controller driver > properties, not just with -global. Correct the deprecation note > accordingly. > > Fixes:

[PATCH v3 1/4] docs/system/deprecated: Fix note on fdc drive properties

2021-03-09 Thread Markus Armbruster
Commit 4a27a638e7 "fdc: Deprecate configuring floppies with -global isa-fdc" actually deprecated any use of floppy controller driver properties, not just with -global. Correct the deprecation note accordingly. Fixes: 4a27a638e718b445648de6b27c709353551d9b44 Signed-off-by: Markus Armbruster ---

[PATCH v3 2/4] fdc: Drop deprecated floppy configuration

2021-03-09 Thread Markus Armbruster
Drop the crap deprecated in commit 4a27a638e7 "fdc: Deprecate configuring floppies with -global isa-fdc" (v5.1.0). Signed-off-by: Markus Armbruster --- docs/system/deprecated.rst | 49 --- docs/system/removed-features.rst | 49 +++ hw/block/fdc.c | 54 +--

[PATCH v3 3/4] fdc: Inline fdctrl_connect_drives() into fdctrl_realize_common()

2021-03-09 Thread Markus Armbruster
The previous commit rendered the name fdctrl_connect_drives() somewhat misleading. Get rid of it by inlining the (now pretty simple) function into its only caller. Signed-off-by: Markus Armbruster --- hw/block/fdc.c | 23 --- 1 file changed, 8 insertions(+), 15 deletions(-)

[PATCH v3 4/4] blockdev: Drop deprecated bogus -drive interface type

2021-03-09 Thread Markus Armbruster
Drop the crap deprecated in commit a1b40bda08 "blockdev: Deprecate -drive with bogus interface type" (v5.1.0). Signed-off-by: Markus Armbruster --- docs/system/deprecated.rst | 7 -- docs/system/removed-features.rst | 7 ++ include/sysemu/blockdev.h| 1 - blockdev.c

[PATCH v3 0/4] Drop deprecated floppy config & bogus -drive if=T

2021-03-09 Thread Markus Armbruster
v3: * PATCH 1: New [Daniel] v2: * Rebased, straightforward conflict with commit f5d33dd51f "hw/block/fdc: Remove the check_media_rate property" resolved * PATCH 2: Commit message fixed [Kevin] Markus Armbruster (3): fdc: Drop deprecated floppy configuration fdc: Inline

[PULL 05/17] utils: Deprecate hex-with-suffix sizes

2021-03-09 Thread Eric Blake
Supporting '0x20M' looks odd, particularly since we have a 'B' suffix that is ambiguous for bytes, as well as a less-frequently-used 'E' suffix for extremely large exibytes. In practice, people using hex inputs are specifying values in bytes (and would have written 0x200, or possibly relied

Re: [PATCH 1/4] qemu_shim: Replace g_file_get_contents() with virFileReadAll()

2021-03-09 Thread Michal Privoznik
On 3/9/21 3:56 PM, Daniel P. Berrangé wrote: On Tue, Mar 09, 2021 at 03:26:22PM +0100, Michal Privoznik wrote: The qemu_shim (compiled into virt-qemu-run-binary) reads several files provided by user (XML definition of secret, value of the secret, XML definition of domain) and it does so using

Re: [libvirt PATCH 00/18] qemu: add support for audio backend configuration

2021-03-09 Thread Michal Privoznik
On 3/3/21 7:18 PM, Daniel P. Berrangé wrote: Historically we've done almost nothing with audio backend configuration. In QEMU we merely set QEMU_AUDIO_DRV to one of sdl, spice, none depending on . We also have the somewhat crazy ability to let QEMU inherit the QEMU_AUDIO_DRV env variable from

Re: [libvirt PATCH 14/18] qemu: add support for generating -audiodev arguments

2021-03-09 Thread Michal Privoznik
On 3/3/21 7:18 PM, Daniel P. Berrangé wrote: The -audiodev argument is replacing the QEMU_AUDIO_DRV env variable (and its relations). Sadly we still have to use the SDL_AUDIODRIVER env variable because that wasn't mapped into QAPI schema. Signed-off-by: Daniel P. Berrangé --- Don't forget

Re: [libvirt PATCH 06/18] conf: refactor OSS audio backend specific options

2021-03-09 Thread Michal Privoznik
On 3/3/21 7:18 PM, Daniel P. Berrangé wrote: To prepare for the introduction for more backend specific audio options, move the OSS options into a dedicated struct and introduce separate helper methods for parse/format/free. Signed-off-by: Daniel P. Berrangé --- docs/schemas/domaincommon.rng

Re: [PATCH 1/4] qemu_shim: Replace g_file_get_contents() with virFileReadAll()

2021-03-09 Thread Daniel P . Berrangé
On Tue, Mar 09, 2021 at 03:26:22PM +0100, Michal Privoznik wrote: > The qemu_shim (compiled into virt-qemu-run-binary) reads several > files provided by user (XML definition of secret, value of the > secret, XML definition of domain) and it does so using > g_file_get_contents(). This is

[PATCH 4/4] docs: Discourage use of some glib functions

2021-03-09 Thread Michal Privoznik
Unfortunately, not all GLib functions provide the level of security we want, or behave how we want. So far, g_file_get_contents() and g_get_host_name() have been identified. Signed-off-by: Michal Privoznik --- docs/glib-adoption.rst | 17 + 1 file changed, 17 insertions(+) diff

[PATCH 3/4] syntax-check: Update list of gethostname exceptions

2021-03-09 Thread Michal Privoznik
The only place where gethostname() is acceptable is in virGetHostnameImpl() which lives in src/util/virutil.c. Reflect this in the list of exceptions for the syntax-check rule. Signed-off-by: Michal Privoznik --- build-aux/syntax-check.mk | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

[PATCH 0/4] Discourage use of some GLib functions

2021-03-09 Thread Michal Privoznik
See 4/4 for the list and explanation. Michal Prívozník (4): qemu_shim: Replace g_file_get_contents() with virFileReadAll() virutil: Do not use g_get_host_name() to obtain hostname syntax-check: Update list of gethostname exceptions docs: Discourage use of some glib functions

[PATCH 1/4] qemu_shim: Replace g_file_get_contents() with virFileReadAll()

2021-03-09 Thread Michal Privoznik
The qemu_shim (compiled into virt-qemu-run-binary) reads several files provided by user (XML definition of secret, value of the secret, XML definition of domain) and it does so using g_file_get_contents(). This is potentially dangerous, because there is no limit on the size of files/buffers.

[PATCH 2/4] virutil: Do not use g_get_host_name() to obtain hostname

2021-03-09 Thread Michal Privoznik
The problem is that g_get_host_name() caches the hostname in a thread local variable. Therefore, it doesn't reflect any subsequent hostname changes. While this might be acceptable for logs where the hostname is printed exactly once when the libvirtd starts up, it is not optimal for

[libvirt PATCH v2 1/4] util: Try to get limits from /proc

2021-03-09 Thread Andrea Bolognani
Calling prlimit() requires elevated privileges, specifically CAP_SYS_RESOURCE, and getrlimit() only works for the current process which is too limiting for our needs; /proc/$pid/limits, on the other hand, can be read by any process, so implement parsing that file as a fallback for when prlimit()

[libvirt PATCH v2 3/4] qemu: Refactor qemuDomainAdjustMaxMemLock()

2021-03-09 Thread Andrea Bolognani
Store the current memory locking limit and the desired one separately, which will help with later changes. Signed-off-by: Andrea Bolognani Reviewed-by: Michal Privoznik --- src/qemu/qemu_domain.c | 20 +++- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git

[libvirt PATCH v2 4/4] qemu: Only raise memlock limit if necessary

2021-03-09 Thread Andrea Bolognani
Attempting to set the memlock limit might fail if we're running in a containerized environment where CAP_SYS_RESOURCE is not available, and if the limit is already high enough there's no point in trying to raise anyway. https://bugzilla.redhat.com/show_bug.cgi?id=1916346 Signed-off-by: Andrea

[libvirt PATCH v2 2/4] qemu: Don't ignore virProcessGetMaxMemLock() errors

2021-03-09 Thread Andrea Bolognani
Now that we've implemented a fallback for the function that obtains the information from /proc, there is no reason we would get a failure unless there's something seriously wrong with the environment we're running in, in which case we're better off reporting the issue to the user rather than

[libvirt PATCH v2 0/4] qemu: Only raise memlock limit if necessary

2021-03-09 Thread Andrea Bolognani
This feature has been requested by KubeVirt developers and will make it possible for them to make some VFIO-related features, such as migration and hotplug, work correctly. https://bugzilla.redhat.com/show_bug.cgi?id=1916346 Changes from [v1]: * prep patches have been pushed; * parsing

Re: [PATCH v4 1/1] qemu: add per-vcpu delay stats

2021-03-09 Thread Aleksei Zakharov
вт, 9 мар. 2021 г. в 15:35, Michal Privoznik : > On 2/19/21 9:08 PM, Aleksei Zakharov wrote: > > This patch adds delay time (steal time inside guest) to libvirt > > domain per-vcpu stats. Delay time is an important performance metric. > > It is a consequence of the overloaded CPU. Knowledge of

Re: [libvirt PATCH 12/17] util: Try to get limits from /proc

2021-03-09 Thread Andrea Bolognani
On Tue, 2021-03-09 at 12:43 +0100, Michal Privoznik wrote: > On 3/5/21 8:13 PM, Andrea Bolognani wrote: > > +if (!g_file_get_contents(procfile, , , NULL)) > > +return -1; > > I did not spot this yesterday, but now I'm working on a something else > and have to read a contents of a

Re: [PATCH v4 1/1] qemu: add per-vcpu delay stats

2021-03-09 Thread Michal Privoznik
On 2/19/21 9:08 PM, Aleksei Zakharov wrote: This patch adds delay time (steal time inside guest) to libvirt domain per-vcpu stats. Delay time is an important performance metric. It is a consequence of the overloaded CPU. Knowledge of the delay time of a virtual machine helps to understand if it

Re: [PATCH] domaincapstest: Return EXIT_SUCCESS / EXIT_FAILURE instead of -1

2021-03-09 Thread Jiri Denemark
On Tue, Mar 09, 2021 at 12:54:55 +0100, Peter Krempa wrote: > The value is used as return value for the process itself. > > Signed-off-by: Peter Krempa > --- > tests/domaincapstest.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tests/domaincapstest.c

Re: [PATCH] qemuMigrationSrcRun: Don't jump to 'exit_monitor' from outside of the monitor

2021-03-09 Thread Jiri Denemark
On Tue, Mar 09, 2021 at 12:51:04 +0100, Peter Krempa wrote: > Failure of 'qemuMigrationSetDBusVMState' would jump to 'exit_monitor' > but the function isn't called inside of the monitor context. > > Signed-off-by: Peter Krempa > --- > src/qemu/qemu_migration.c | 2 +- > 1 file changed, 1

[PATCH] domaincapstest: Return EXIT_SUCCESS / EXIT_FAILURE instead of -1

2021-03-09 Thread Peter Krempa
The value is used as return value for the process itself. Signed-off-by: Peter Krempa --- tests/domaincapstest.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/domaincapstest.c b/tests/domaincapstest.c index 7a082705c6..65d9f4c635 100644 --- a/tests/domaincapstest.c

[PATCH] qemuMigrationSrcRun: Don't jump to 'exit_monitor' from outside of the monitor

2021-03-09 Thread Peter Krempa
Failure of 'qemuMigrationSetDBusVMState' would jump to 'exit_monitor' but the function isn't called inside of the monitor context. Signed-off-by: Peter Krempa --- src/qemu/qemu_migration.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_migration.c

Re: [libvirt PATCH 12/17] util: Try to get limits from /proc

2021-03-09 Thread Michal Privoznik
On 3/5/21 8:13 PM, Andrea Bolognani wrote: Calling prlimit() requires elevated privileges, specifically CAP_SYS_RESOURCE, and getrlimit() only works for the current process which is too limiting for our needs; /proc/$pid/limits, on the other hand, can be read by any process, so implement parsing

Re: [PATCH 16/23] conf: get rid of macros in virDomainDiskDefIotuneParse

2021-03-09 Thread Nikolay Shirokovskiy
ср, 3 мар. 2021 г. в 17:06, Peter Krempa : > On Mon, Jan 11, 2021 at 12:50:09 +0300, Nikolay Shirokovskiy wrote: > > At first glance we don't get much win because of introduction of > > virDomainBlockIoTuneFieldNames and virDomainBlockIoTuneFields. But we > are going > > to use these two in other

Re: [PATCH v2 00/12] qemu: Prepare for QAPIfied object-add

2021-03-09 Thread Peter Krempa
On Mon, Mar 08, 2021 at 14:40:42 +0100, Peter Krempa wrote: > On Wed, Feb 24, 2021 at 16:57:54 +0100, Peter Krempa wrote: > > QEMU plans to QAPIfy object add. This series prepares for the API change > > (drop of 'props' wrapper for the object) and adds testing based on our > > qemuxml2argv test

Re: [PATCH] meson: Add documentation installation directory option

2021-03-09 Thread Michal Privoznik
On 3/4/21 12:33 PM, Daniel Henrique Barboza wrote: On 2/26/21 4:11 PM, Chris Mayo wrote: Allow the directory to be chosen at installation time, to support local conventions e.g. versioning. Signed-off-by: Chris Mayo --- Reviewed-by: Daniel Henrique Barboza Pushed. Congratulations on

Re: [libvirt PATCH] util: Fix error reporting in virnetlink

2021-03-09 Thread Michal Privoznik
On 3/9/21 11:22 AM, Andrea Bolognani wrote: The preprocessor macro we use to check whether we're on Linux has not been spelled properly, and so we will always report the error message intended for other platforms. Fixes: 879bcee08ce0f91f556fddfe452c3fbed5318468 Signed-off-by: Andrea Bolognani

Re: [libvirt PATCH] util: don't log error if SRIOV PF has no associated netdev

2021-03-09 Thread Michal Privoznik
On 2/24/21 4:35 AM, Laine Stump wrote: Some SRIOV PFs don't have a netdev associated with them (the spec apparently doesn't require it). In most cases when libvirt is dealing with an SRIOV VF, that VF must have a PF, and the PF *must* have an associated netdev (the only way to set the MAC

Re: [PATCH 16/23] conf: get rid of macros in virDomainDiskDefIotuneParse

2021-03-09 Thread Peter Krempa
On Tue, Mar 09, 2021 at 10:38:36 +0300, Nikolay Shirokovskiy wrote: > ср, 3 мар. 2021 г. в 17:06, Peter Krempa : > > > On Mon, Jan 11, 2021 at 12:50:09 +0300, Nikolay Shirokovskiy wrote: > > > At first glance we don't get much win because of introduction of > > > virDomainBlockIoTuneFieldNames

Re: [PATCH 16/23] conf: get rid of macros in virDomainDiskDefIotuneParse

2021-03-09 Thread Peter Krempa
On Tue, Mar 09, 2021 at 10:38:36 +0300, Nikolay Shirokovskiy wrote: > ср, 3 мар. 2021 г. в 17:06, Peter Krempa : > > > On Mon, Jan 11, 2021 at 12:50:09 +0300, Nikolay Shirokovskiy wrote: > > > At first glance we don't get much win because of introduction of > > > virDomainBlockIoTuneFieldNames

[libvirt PATCH] util: Fix error reporting in virnetlink

2021-03-09 Thread Andrea Bolognani
The preprocessor macro we use to check whether we're on Linux has not been spelled properly, and so we will always report the error message intended for other platforms. Fixes: 879bcee08ce0f91f556fddfe452c3fbed5318468 Signed-off-by: Andrea Bolognani --- src/util/virnetlink.c | 2 +- 1 file

Re: [PATCH v3 10/30] qapi/qom: Add ObjectOptions for tls-*, deprecate 'loaded'

2021-03-09 Thread Daniel P . Berrangé
On Mon, Mar 08, 2021 at 05:54:20PM +0100, Kevin Wolf wrote: > This adds a QAPI schema for the properties of the tls-* objects. > > The 'loaded' property doesn't seem to make sense as an external > interface: It is automatically set to true in ucc->complete, and > explicitly setting it to true

Re: [PATCH v3 09/30] qapi/qom: Add ObjectOptions for secret*, deprecate 'loaded'

2021-03-09 Thread Daniel P . Berrangé
On Mon, Mar 08, 2021 at 05:54:19PM +0100, Kevin Wolf wrote: > This adds a QAPI schema for the properties of the secret* objects. > > The 'loaded' property doesn't seem to make sense as an external > interface: It is automatically set to true in ucc->complete, and > explicitly setting it to true

Re: [PATCH v3 03/30] qapi/qom: Add ObjectOptions for authz-*

2021-03-09 Thread Daniel P . Berrangé
On Mon, Mar 08, 2021 at 05:54:13PM +0100, Kevin Wolf wrote: > This adds a QAPI schema for the properties of the authz-* objects. > > Signed-off-by: Kevin Wolf > Acked-by: Peter Krempa > Reviewed-by: Eric Blake > --- > qapi/authz.json | 61 +--- >