Re: [libvirt][RFC PATCH] add a new 'default' option for attribute mode in numatune

[Zhong, Luyao]

On 11/4/2020 9:02 PM, Martin Kletzander wrote:

On Fri, Oct 16, 2020 at 10:38:51PM +0800, Zhong, Luyao wrote:

On 10/16/2020 9:32 PM, Zang, Rui wrote:


How about if “migratable” is set, “mode” should be ignored/omitted? 
So any setting of “mode” will be rejected with an error indicating an 
invalid configuration.
We can say in the doc that “migratable” and “mode” shall not be set 
together. So even the default value of “mode” is not taken.



If "mode" is not set, it's the same as setting "strict" value ('strict'
is the default value). It involves some code detail, it will be
translated to enumerated type, the value is 0 when mode not set or set
to 'strict'. The code is in some fixed skeleton, so it's not easy to 
modify.




Well I see it as it is "strict". It does not mean "strict cgroup setting",
because cgroups are just one of the ways to enforce this.  Look at it 
this way:


mode can be:
  - strict: only these nodes can be used for the memory
  - preferred: there nodes should be preferred, but allocation should 
not fail

  - interleave: interleave the memory between these nodes

Due to the naming this maps to cgroup settings 1:1.

But now we have another way of enforcing this, using qemu cmdline 
option.  The

names actually map 1:1 to those as well:

   
https://gitlab.com/qemu-project/qemu/-/blob/master/qapi/machine.json#L901


So my idea was that we would add a movable/migratable/whatever attribute 
that
would tell us which way for enforcing we use because there does not seem 
to be
"one size fits all" solution.  Am I misunderstanding this discussion?  
Please

correct me if I am.  Thank you.

Actually I need a default memory policy(memory policy is 'hard coded' 
into the kernel) support, I thought "migratable" was enough to indicate 
that we rely on operating system to operate memory policy. So when 
"migratable" is set, "mode" should not be set. But when I was coding, I 
found "mode" default value is "strict", it is always "strict" even if 
"migratable" is yes, that means we configure two different memory 
policies at the same time. Then I still need a new option for "mode" to 
make it not conflicting with the "migratable", then if we have the new 
option("default") for "mode", it seems we can drop "migratable".


Besides, we can make "mode" being a "one size fits all" solution., just 
reject the different "mode" value config in memnode element when "mode" 
is "default" in memory element.


I summary it in the new email
https://www.redhat.com/archives/libvir-list/2020-November/msg00084.html

Sorry I didn't make it easy to understand.

Regards,
Luyao

So I need a option to indicate "I don't specify any mode.".


在 2020年10月16日，20:34，Zhong, Luyao  写道：

Hi Martin, Peter and other experts,

We got a consensus that we need introducing a new "migratable" 
attribute before. But in implementation, I found introducing a new 
'default' option for existing mode attribute is still neccessary.


I have a initial patch for 'migratable' and Peter gave some comments 
already.

https://www.redhat.com/archives/libvir-list/2020-October/msg00396.html

Current issue is, if I set 'migratable', any 'mode' should be 
ignored. Peter commented that I can't rely on docs to tell users 
some config is invalid, I need to reject the config in the code, I 
completely agree with that. But the 'mode' default value is 
'strict', it will always conflict with the 'migratable', at the end 
I still need introducing a new option for 'mode' which can be a 
legal config when 'migratable' is set.


If we have 'default' option, is 'migratable' still needed then?

FYI.
The 'mode' is corresponding to memory policy, there already a notion 
of default memory policy.

  quote:
    System Default Policy:  this policy is "hard coded" into the 
kernel.

(https://www.kernel.org/doc/Documentation/vm/numa_memory_policy.txt)
So it might be easier to understand if we introduce a 'default' 
option directly.


Regards,
Luyao


On 8/26/2020 6:20 AM, Martin Kletzander wrote:

On Tue, Aug 25, 2020 at 09:42:36PM +0800, Zhong, Luyao wrote:


On 8/19/2020 11:24 PM, Martin Kletzander wrote:

On Tue, Aug 18, 2020 at 07:49:30AM +, Zang, Rui wrote:




-Original Message-
From: Martin Kletzander 
Sent: Monday, August 17, 2020 4:58 PM
To: Zhong, Luyao 
Cc: libvir-list@redhat.com; Zang, Rui ; Michal
Privoznik

Subject: Re: [libvirt][RFC PATCH] add a new 'default' option for
attribute mode
in numatune

On Tue, Aug 11, 2020 at 04:39:42PM +0800, Zhong, Luyao wrote:



On 8/7/2020 4:24 PM, Martin Kletzander wrote:

On Fri, Aug 07, 2020 at 01:27:59PM +0800, Zhong, Luyao wrote:



On 8/3/2020 7:00 PM, Martin Kletzander wrote:

On Mon, Aug 03, 2020 at 05:31:56PM +0800, Luyao Zhong wrote:

Hi Libvirt experts,

I would like enhence the numatune snippet configuration. 
Given a

example snippet:


 ...
 
     ÂÂ
    
cellid="2" mode="preferred" nodeset="2"/>   
 ...



Currently, attribute mode is either 'interleave', 
'strict', 

Re: [PATCH 2/2] qemu: don't needlessly unset close callback during perform phase

[Nikolay Shirokovskiy]

On 06.11.2020 17:03, Jiri Denemark wrote:
> On Tue, Aug 18, 2020 at 13:26:36 +0300, Nikolay Shirokovskiy wrote:
>> During API call connection is referenced and close callback is called when
>> connection is disposed. Thus during API call close callback cannot be 
>> triggered
>> and we don't need to unset callback on API call duration.
>>
>> This code was added in [1] and commit does not explain this part of the
>> patch.
>>
>> [1] 1fdc53c3: qemu: Avoid dangling migration-out job when client dies
> 
> I suspect this was not the case 8 years ago when close callbacks were
> introduced. In any case, I don't think calling Unset in the begining of
> qemuMigrationSrcPerformPhase and Set at the end could cause any harm.
> The idea is to keep the callback set only when there's no running API.
> So while it may not be needed, I don't think there's a real need to
> change it.
> 
> Jirka
> 

Ok, thank you for review! First patch pushed with suggested changes.

Nikolay

Re: [PATCH] qemu: virtiofs: Add pool element to limit thread pool

[Masayoshi Mizuma]

On Mon, Oct 26, 2020 at 09:50:22AM +0100, Peter Krempa wrote:
> On Tue, Oct 20, 2020 at 13:54:33 -0400, Masayoshi Mizuma wrote:
> > From: Masayoshi Mizuma 
> > 
> > virtiofsd has --thread-pool-size=NUM option to limit the
> > thread pool size. It will be useful to tune the performance.
> 
> Usually it's great to document or at least link to a document which
> outlines how changing the parameter influences performance. In many
> cases theres just one good option and that should become default. In
> other cases it depends on deployment and in such case it's very good to
> prevent users from having to guess what it actually does.
> 
> In the end it serves as a justification that adding a tuning knob is
> worth it.
> 
> > Add pool element to use the option like as:
> > 
> >
> >
> >
> 
> I'd prefer if the attribute is named 'threads' as that seems to me to be
> more clear what it does without having to refer back to the
> documentation.

Got it, thanks. I'll change the parameter to 'threads'.

Thanks!
Masa

Re: [PATCH] lxc: Cleanup after failed startup

[Martin Kletzander]

On Fri, Nov 06, 2020 at 02:30:13PM +0100, Michal Privoznik wrote:

If starting an container fails, the virLXCProcessStop() is
called. But since vm->def->id is not set until libvirt_lxc is
spawned (the domain's ID is PID of that process),
virLXCProcessStop() returns early as virDomainObjIsActive()
returns false. But doing so leaves behind resources reserved for
the containers during the startup process. Most notably, hostdevs
are not re-attached to the host, the domain's transient XML is
not removed, etc.



I'm not sure the virLXCProcessCleanup() function is prepared to be called under
such circumstances as its qemu counterpart.  It looks virSecurity*RestoreLabel()
might be called when it ought not to be and that does not look safe to me.  But
I'd be glad to be proved wrong.


Signed-off-by: Michal Privoznik 
---
src/lxc/lxc_process.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
index c5a710fc3f..08c82b0e9a 100644
--- a/src/lxc/lxc_process.c
+++ b/src/lxc/lxc_process.c
@@ -855,7 +855,7 @@ int virLXCProcessStop(virLXCDriverPtr driver,
  vm->def->name, (int)vm->pid, (int)reason);
if (!virDomainObjIsActive(vm)) {
VIR_DEBUG("VM '%s' not active", vm->def->name);
-return 0;
+goto cleanup;
}

priv = vm->privateData;
--
2.26.2



signature.asc
Description: PGP signature

Re: [PATCH] news: Mention Cooperlake cpu model in v6.4.0

[Andrea Bolognani]

On Fri, 2020-11-06 at 15:10 +0100, Martin Kletzander wrote:
> On Wed, Oct 28, 2020 at 03:51:48PM +0800, Han Han wrote:
> > Signed-off-by: Han Han 
> > ---
> > NEWS.rst | 2 ++
> > 1 file changed, 2 insertions(+)
> 
> So yes, this was added in 6.4.0, but I don't know whether we have any policy
> regarding news for older releases as that would create inconsistency and there
> is always going to be something we missed.  Especially when lot of people do 
> not
> update the news file.  On the other hand it would appear in the news file from
> now on...  I really don't know.  Does anyone else have an opinion?  @Dan? 
> @Andrea?

I think it can be useful to add this kind of information even after
the fact, since we include the full release notes in release tarballs
as well as publishing them on the website. There is basically no
downside to this, even though of course it would be even better if we
would manage to add this information *before* the release is out :)

Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH] virGDBusBusInit: Properly check for error when looking up D-Bus address

[Pavel Hrdina]

On Fri, Nov 06, 2020 at 02:30:26PM +0100, Michal Privoznik wrote:
> The virGDBusBusInit is supposed to return a reference to
> requested bus type (system/session) or, if non-shared bus is
> requested then create a new bus of the type. As an argument, it
> gets a double pointer to GError which is passed to all g_dbus_*()
> calls which allocate it on failure. Pretty standard approach.
> However, since it is a double pointer we must dereference the
> first level to see if the value is NULL. IOW:
> 
>   if (*error)
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/util/virgdbus.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Pavel Hrdina 


signature.asc
Description: PGP signature

Re: [libvirt PATCH] Revert "Revert "spec: Simplify setting features off by default""

[Martin Kletzander]

On Thu, Oct 29, 2020 at 11:47:01AM +0100, Andrea Bolognani wrote:

As explained in the original commit (31d687a3218c), these values
are actually unaffected by the corresponding _without_* macros
and so we can leave out the additional processing / obfuscation.

This reverts commit ae23a87d85cfc2a964123d9bd44157a411428c0a.



Yeah, the previous revert was too ambitious.

Reviewed-by: Martin Kletzander 


Signed-off-by: Andrea Bolognani 
---
libvirt.spec.in | 16 
1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/libvirt.spec.in b/libvirt.spec.in
index 84515cc7de..2a4324b974 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -98,14 +98,14 @@
%define with_numactl  0%{!?_without_numactl:1}

# A few optional bits off by default, we enable later
-%define with_fuse 0%{!?_without_fuse:0}
-%define with_sanlock  0%{!?_without_sanlock:0}
-%define with_numad0%{!?_without_numad:0}
-%define with_firewalld_zone   0%{!?_without_firewalld:0}
-%define with_libssh2  0%{!?_without_libssh2:0}
-%define with_wireshark0%{!?_without_wireshark:0}
-%define with_libssh   0%{!?_without_libssh:0}
-%define with_dmidecode0%{!?_without_dmidecode:0}
+%define with_fuse 0
+%define with_sanlock  0
+%define with_numad0
+%define with_firewalld_zone   0
+%define with_libssh2  0
+%define with_wireshark0
+%define with_libssh   0
+%define with_dmidecode0

# Finally set the OS / architecture specific special cases

--
2.26.2



signature.asc
Description: PGP signature

Re: [PATCH] news: Mention Cooperlake cpu model in v6.4.0

[Martin Kletzander]

On Wed, Oct 28, 2020 at 03:51:48PM +0800, Han Han wrote:

Signed-off-by: Han Han 
---
NEWS.rst | 2 ++
1 file changed, 2 insertions(+)



So yes, this was added in 6.4.0, but I don't know whether we have any policy
regarding news for older releases as that would create inconsistency and there
is always going to be something we missed.  Especially when lot of people do not
update the news file.  On the other hand it would appear in the news file from
now on...  I really don't know.  Does anyone else have an opinion?  @Dan? 
@Andrea?


diff --git a/NEWS.rst b/NEWS.rst
index 2fef3f706c..5dac805390 100644
--- a/NEWS.rst
+++ b/NEWS.rst
@@ -463,6 +463,8 @@ v6.4.0 (2020-06-02)
already does in these cases. Users are encouraged to provide complete NUMA
topologies to avoid unexpected changes in the domain XML.

+  * Cooperlake x86 CPU model is added
+
* **Bug fixes**

  * qemu: fixed regression in network device hotplug with new qemu versions
--
2.28.0



signature.asc
Description: PGP signature

Re: [PATCH 2/2] qemu: don't needlessly unset close callback during perform phase

[Jiri Denemark]

On Tue, Aug 18, 2020 at 13:26:36 +0300, Nikolay Shirokovskiy wrote:
> During API call connection is referenced and close callback is called when
> connection is disposed. Thus during API call close callback cannot be 
> triggered
> and we don't need to unset callback on API call duration.
> 
> This code was added in [1] and commit does not explain this part of the
> patch.
> 
> [1] 1fdc53c3: qemu: Avoid dangling migration-out job when client dies

I suspect this was not the case 8 years ago when close callbacks were
introduced. In any case, I don't think calling Unset in the begining of
qemuMigrationSrcPerformPhase and Set at the end could cause any harm.
The idea is to keep the callback set only when there's no running API.
So while it may not be needed, I don't think there's a real need to
change it.

Jirka

Re: [PATCH v2] virsh: Allow listing just domain IDs

[Martin Kletzander]

On Thu, Oct 22, 2020 at 04:41:18PM +0200, Michal Privoznik wrote:

Some completers for libvirt related tools might want to list
domain IDs only. Just like the one I've implemented for
virt-viewer [1]. I've worked around it using some awk magic,
but if it was possible to just 'virsh list --id' then I could
drop awk.

1: https://www.redhat.com/archives/virt-tools-list/2019-May/msg00014.html

Signed-off-by: Michal Privoznik 
---

Diff to v1:
- Documented the new switch in the manpage
- Allowed --id to be used with --all

docs/manpages/virsh.rst  | 21 +-
tools/virsh-domain-monitor.c | 42 +---
2 files changed, 39 insertions(+), 24 deletions(-)

diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst
index d34a1c8684..848e1a6458 100644
--- a/docs/manpages/virsh.rst
+++ b/docs/manpages/virsh.rst
@@ -621,7 +621,7 @@ list

   list [--inactive | --all]
[--managed-save] [--title]
-{ [--table] | --name | --uuid }
+{ [--table] | --name | --uuid | --id }
[--persistent] [--transient]
[--with-managed-save] [--without-managed-save]
[--autostart] [--no-autostart]
@@ -758,16 +758,15 @@ If *--managed-save* is specified, then domains that have 
managed save state
in the listing. This flag is usable only with the default *--table* output.
Note that this flag does not filter the list of domains.

-If *--name* is specified, domain names are printed instead of the table
-formatted one per line. If *--uuid* is specified domain's UUID's are printed
-instead of names. Flag *--table* specifies that the legacy table-formatted
-output should be used. This is the default.
-
-If both *--name* and *--uuid* are specified, domain UUID's and names
-are printed side by side without any header. Flag *--table* specifies
-that the legacy table-formatted output should be used. This is the
-default if neither *--name* nor *--uuid* are specified. Option
-*--table* is mutually exclusive with options *--uuid* and *--name*.
+If *--name* is specified, domain names are printed instead of the
+table formatted one per line. If *--uuid* is specified domain's UUID's
+are printed instead of names. If *--id* is specified then domain's ID's
+are printed indead of names. However, it is possible to combine
+*--name*, *--uuid* and *--id* to select only desired fields for
+printing. Flag *--table* specifies that the legacy table-formatted
+output should be used, but it is mutually exclusive with *--name*,
+*--uuid* and *--id*. This is the default and will be used if neither of
+*--name*, *--uuid* or *--id* is specified.



Didn't you want to add something like the following?

  If neither *--name* nor *--uuid* is specified, but *--id* is, then only active
  domains are listed, even with the *--all* parameter as otherwise the output
  would just contain bunch of lines with just *-1*.

Otherwise the code is very unclear for no reason.

If that is the case, then

Reviewed-by: Martin Kletzander 


signature.asc
Description: PGP signature

[PATCH] virGDBusBusInit: Properly check for error when looking up D-Bus address

[Michal Privoznik]

The virGDBusBusInit is supposed to return a reference to
requested bus type (system/session) or, if non-shared bus is
requested then create a new bus of the type. As an argument, it
gets a double pointer to GError which is passed to all g_dbus_*()
calls which allocate it on failure. Pretty standard approach.
However, since it is a double pointer we must dereference the
first level to see if the value is NULL. IOW:

  if (*error)

Signed-off-by: Michal Privoznik 
---
 src/util/virgdbus.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/util/virgdbus.c b/src/util/virgdbus.c
index 4360a6acff..19fd7e2fe4 100644
--- a/src/util/virgdbus.c
+++ b/src/util/virgdbus.c
@@ -55,7 +55,7 @@ virGDBusBusInit(GBusType type, GError **error)
 return g_bus_get_sync(type, NULL, error);
 } else {
 address = g_dbus_address_get_for_bus_sync(type, NULL, error);
-if (error)
+if (*error)
 return NULL;
 return g_dbus_connection_new_for_address_sync(address,
   
G_DBUS_CONNECTION_FLAGS_NONE,
-- 
2.26.2

[PATCH] lxc: Cleanup after failed startup

[Michal Privoznik]

If starting an container fails, the virLXCProcessStop() is
called. But since vm->def->id is not set until libvirt_lxc is
spawned (the domain's ID is PID of that process),
virLXCProcessStop() returns early as virDomainObjIsActive()
returns false. But doing so leaves behind resources reserved for
the containers during the startup process. Most notably, hostdevs
are not re-attached to the host, the domain's transient XML is
not removed, etc.

Signed-off-by: Michal Privoznik 
---
 src/lxc/lxc_process.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
index c5a710fc3f..08c82b0e9a 100644
--- a/src/lxc/lxc_process.c
+++ b/src/lxc/lxc_process.c
@@ -855,7 +855,7 @@ int virLXCProcessStop(virLXCDriverPtr driver,
   vm->def->name, (int)vm->pid, (int)reason);
 if (!virDomainObjIsActive(vm)) {
 VIR_DEBUG("VM '%s' not active", vm->def->name);
-return 0;
+goto cleanup;
 }
 
 priv = vm->privateData;
-- 
2.26.2

Re: [PATCH 1/2] qemu: fix qemuMigrationSrcCleanup to use qemuMigrationJobFinish

[Jiri Denemark]

On Tue, Aug 18, 2020 at 13:26:35 +0300, Nikolay Shirokovskiy wrote:
> qemuMigrationSrcCleanup uses qemuDomainObjDiscardAsyncJob currently. But
> discard does not reduce jobs_queued counter so it leaks. Also discard does not
> notify other threads that job condition is available. Discard does reset 
> nested
> job but nested job is not possible in this conditions.
> 
> Signed-off-by: Nikolay Shirokovskiy 
> ---
>  src/qemu/qemu_migration.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 142faa2..301475a 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -2026,7 +2026,6 @@ qemuMigrationSrcCleanup(virDomainObjPtr vm,
>  switch ((qemuMigrationJobPhase) priv->job.phase) {
>  case QEMU_MIGRATION_PHASE_BEGIN3:
>  /* just forget we were about to migrate */
> -qemuDomainObjDiscardAsyncJob(driver, vm);
>  break;
>  
>  case QEMU_MIGRATION_PHASE_PERFORM3_DONE:
> @@ -2036,7 +2035,6 @@ qemuMigrationSrcCleanup(virDomainObjPtr vm,
>  qemuMigrationParamsReset(driver, vm, QEMU_ASYNC_JOB_MIGRATION_OUT,
>   jobPriv->migParams, priv->job.apiFlags);
>  /* clear the job and let higher levels decide what to do */
> -qemuDomainObjDiscardAsyncJob(driver, vm);
>  break;
>  
>  case QEMU_MIGRATION_PHASE_PERFORM3:
> @@ -2053,8 +2051,9 @@ qemuMigrationSrcCleanup(virDomainObjPtr vm,
>  case QEMU_MIGRATION_PHASE_NONE:
>  case QEMU_MIGRATION_PHASE_LAST:
>  /* unreachable */
> -;
> +return;
>  }
> +qemuMigrationJobFinish(driver, vm);
>  }

I was thinking whether we could somehow merge the two APIs, but
apparently we need both as some callers should not do the actions in
which these two APIs differ.

Anyway, this patch is good, but I'd prefer the qemuMigrationJobFinish
call to stay in each switch branch to make the code executed for each
phase immediately visible. And it's just one line so I don't see a
reason for moving it.

With the suggested change:

Reviewed-by: Jiri Denemark 

Re: [PATCH v2 00/12] Replace virHashTable by GHashTable

[Matt Coleman]

> On Nov 4, 2020, at 12:05 PM, Peter Krempa  wrote:
> 
> Our hash table API was surprisingly close to glibs.
> 
> v2:
> - pushed ACK'd patch
> - typo fix
> - rebased on current master
> - hashing function not replaced as our has random initialization
> 
> Peter Krempa (12):
>  virhashtest: testHashGetItems: Remove test case for sorting by value
>  util: hash: Rewrite sorting of elements in virHashGetItems
>  util: hash: Introduce virHashForEachSorted
>  Use virHashForEachSorted in tested code
>  tests: remove virdeterministichashmock.so
>  util: hash: Add delete-safe hash iterator
>  util: hash: Use virHashForEachSafe in places which might delete the
>element
>  util: hash: Don't use 'const' with virHashTablePtr
>  util: hash: Reimplement virHashTable using GHashTable
>  util: hash: Retire 'virHashTable' in favor of 'GHashTable'
>  util: hash: Add deprecation notices for functions which have
>g_hash_table replacements
>  tests: Remove 'virhashtest'

I built it locally and tested against a Hyper-V server.

Looks good to me.

Reviewed-by: Matt Coleman 

-- 
Matt

Re: [PATCH v2 00/12] Replace virHashTable by GHashTable

[Neal Gompa]

On Wed, Nov 4, 2020 at 12:06 PM Peter Krempa  wrote:
>
> Our hash table API was surprisingly close to glibs.
>
> v2:
> - pushed ACK'd patch
> - typo fix
> - rebased on current master
> - hashing function not replaced as our has random initialization
>
> Peter Krempa (12):
>   virhashtest: testHashGetItems: Remove test case for sorting by value
>   util: hash: Rewrite sorting of elements in virHashGetItems
>   util: hash: Introduce virHashForEachSorted
>   Use virHashForEachSorted in tested code
>   tests: remove virdeterministichashmock.so
>   util: hash: Add delete-safe hash iterator
>   util: hash: Use virHashForEachSafe in places which might delete the
> element
>   util: hash: Don't use 'const' with virHashTablePtr
>   util: hash: Reimplement virHashTable using GHashTable
>   util: hash: Retire 'virHashTable' in favor of 'GHashTable'
>   util: hash: Add deprecation notices for functions which have
> g_hash_table replacements
>   tests: Remove 'virhashtest'
>
>  src/conf/backup_conf.c|   2 +-
>  src/conf/domain_addr.h|   2 +-
>  src/conf/domain_conf.c|  12 +-
>  src/conf/domain_conf.h|   2 +-
>  src/conf/nwfilter_conf.h  |   2 +-
>  src/conf/nwfilter_ipaddrmap.c |   2 +-
>  src/conf/nwfilter_params.c|  34 +-
>  src/conf/nwfilter_params.h|  18 +-
>  src/conf/snapshot_conf.c  |   2 +-
>  src/conf/virchrdev.c  |   4 +-
>  src/conf/virdomainmomentobjlist.c |   4 +-
>  src/conf/virdomainobjlist.c   |   6 +-
>  src/conf/virinterfaceobj.c|   2 +-
>  src/conf/virnetworkobj.c  |   8 +-
>  src/conf/virnodedeviceobj.c   |   2 +-
>  src/conf/virnwfilterbindingdef.h  |   2 +-
>  src/conf/virnwfilterbindingobjlist.c  |   4 +-
>  src/conf/virsecretobj.c   |   2 +-
>  src/conf/virstorageobj.c  |  14 +-
>  src/hyperv/hyperv_driver.c|   4 +-
>  src/hyperv/hyperv_wmi.c   |  18 +-
>  src/hyperv/hyperv_wmi.h   |  12 +-
>  src/hypervisor/virclosecallbacks.c|   2 +-
>  src/libvirt_private.syms  |   2 +
>  src/libxl/libxl_logger.c  |   2 +-
>  src/locking/lock_daemon.c |   4 +-
>  src/nwfilter/nwfilter_dhcpsnoop.c |   6 +-
>  src/nwfilter/nwfilter_ebiptables_driver.c |  19 +-
>  src/nwfilter/nwfilter_gentech_driver.c|  36 +-
>  src/nwfilter/nwfilter_gentech_driver.h|   2 +-
>  src/nwfilter/nwfilter_learnipaddr.c   |   4 +-
>  src/nwfilter/nwfilter_tech_driver.h   |   2 +-
>  src/qemu/qemu_agent.c |   4 +-
>  src/qemu/qemu_backup.c|  14 +-
>  src/qemu/qemu_backup.h|   2 +-
>  src/qemu/qemu_block.c |  42 +-
>  src/qemu/qemu_block.h |  18 +-
>  src/qemu/qemu_blockjob.c  |   6 +-
>  src/qemu/qemu_capabilities.c  |   8 +-
>  src/qemu/qemu_checkpoint.c|   6 +-
>  src/qemu/qemu_checkpoint.h|   2 +-
>  src/qemu/qemu_conf.c  |   4 +-
>  src/qemu/qemu_conf.h  |   2 +-
>  src/qemu/qemu_domain.c|   8 +-
>  src/qemu/qemu_domain.h|   2 +-
>  src/qemu/qemu_driver.c|  24 +-
>  src/qemu/qemu_interop_config.c|  13 +-
>  src/qemu/qemu_migration.c |   2 +-
>  src/qemu/qemu_migration_cookie.c  |   2 +-
>  src/qemu/qemu_monitor.c   |  28 +-
>  src/qemu/qemu_monitor.h   |  20 +-
>  src/qemu/qemu_monitor_json.c  |  50 +-
>  src/qemu/qemu_monitor_json.h  |  22 +-
>  src/qemu/qemu_process.c   |  18 +-
>  src/qemu/qemu_qapi.c  |  14 +-
>  src/qemu/qemu_qapi.h  |   6 +-
>  src/qemu/qemu_snapshot.c  |  16 +-
>  src/rpc/virnetdaemon.c|  13 +-
>  src/security/security_selinux.c   |   2 +-
>  src/util/virfilecache.c   |   2 +-
>  src/util/virhash.c| 571 --
>  src/util/virhash.h|  47 +-
>  src/util/viriptables.c|   4 +-
>  src/util/virlockspace.c   |   4 +-
>  src/util/virmacmap.c  |   4 +-
>  src/util/virstoragefile.c |   8 +-
>  src/util/virsystemd.c |   2 +-
>  tests/meson.build |   2 -
>  tests/nwfilterxml2firewalltest.c  |  24 

Re: [PATCH 00/28] a bunch of domain_conf cleanup

[Neal Gompa]

On Thu, Nov 5, 2020 at 10:33 PM Matt Coleman  wrote:
>
> Most of this is making functions void that unnecessarily return an int.
> It also includes some conversion to GLib.
>
> Feel free to squash related commits, if you'd like. I left them separate
> to make it easier to review.
>
> Matt Coleman (28):
>   domain_conf: make virDomainDiskSetDriver() void
>   domain_conf: make virDomainPostParseCheckISCSIPath() void
>   domain_conf: use g_free() in virDomainPostParseCheckISCSIPath()
>   domain_conf: make virDomainHostdevAssignAddress() void
>   domain_conf: make virDomainChr/RNG/Video/VsockDefPostParse() and
> virDomainNVRAMDefFormat() void
>   domain_conf: make virDomainDeviceInfoFormat() void
>   domain_conf: make virDomainGraphicsDefParseXMLEGLHeadless() void
>   domain_conf: make virDomainLeaseDefFormat() void
>   domain_conf: make virDomainDiskSourceFormatNetwork() void
>   domain_conf: make virDomainDiskDefFormatIotune() void
>   domain_conf: make virDomainDiskDefFormatDriver() void
>   domain_conf: make virDomainControllerDriverFormat() void
>   domain_conf: make virDomainVirtioNetGuestOpts/HostOpts/DriverFormat()
> void
>   domain_conf: make virDomainRedirFilterDefFormat() void
>   domain_conf: make virDomainIOMMUDefFormat() void
>   domain_conf: make virDomainDefFormatBlkiotune() void
>   domain_conf: make virDomainChrSourceDefFormat() void
>   domain_conf: make virDomainDiskSetBlockIOTune() void
>   domain_conf: use g_free in virDomainDiskSetBlockIOTune()
>   domain_conf: use g_renew in virDomainDiskInsert() and
> virDomainControllerInsert()
>   domain_conf: make virDomainDiskInsert() void
>   domain_conf: make virDomainControllerInsert() void
>   domain_conf: use g_renew in virDomainLeaseInsertPreAlloc()
>   domain_conf: make virDomainLeaseInsertPreAlloc() void
>   domain_conf: make virDomainLeaseInsert() void
>   domain_conf: make virDomainPanicDefFormat() void
>   domain_conf: make virDomainShmemDefFormat() void
>   domain_conf: make virDomainVsockDefFormat() void
>
>  src/conf/domain_conf.c   | 349 ++-
>  src/conf/domain_conf.h   |  21 +--
>  src/libxl/libxl_conf.c   |   5 +-
>  src/libxl/libxl_domain.c |   5 +-
>  src/libxl/libxl_driver.c |   9 +-
>  src/libxl/xen_xl.c   |  12 +-
>  src/libxl/xen_xm.c   |  10 +-
>  src/lxc/lxc_driver.c |   3 +-
>  src/qemu/qemu_domain.c   |   5 +-
>  src/qemu/qemu_driver.c   |  15 +-
>  src/qemu/qemu_hotplug.c  |   3 +-
>  src/test/test_driver.c   |   3 +-
>  src/vz/vz_sdk.c  |   9 +-
>  tests/qemublocktest.c|   5 +-
>  14 files changed, 158 insertions(+), 296 deletions(-)
>
> --
> 2.27.0
>
>

Series LGTM.

Reviewed-by: Neal Gompa 


-- 
真実はいつも一つ！/ Always, there's only one truth!

Re: [PATCH] viridentitytest: Drop #if !WITH_SELINUX block

[Michal Prívozník]

On 11/6/20 11:17 AM, Ján Tomko wrote:

On a Friday in 2020, Michal Privoznik wrote:

The whole test is built only if SELinux secdriver is enabled and
thus the !WITH_SELINUX can't be satisfied really.


Which seems like a mistake. testIdentityAttrs does not depend on SELinux
and the virIdentity APIs it tests are called even from code that does
not require SELinux.



Well, it's like that for 6+ years :-) 
cdc5f3f1a3a960cbc988ad6a21078f135c936457


But okay, let me see if I can make the test run more often.

Michal

Re: [PATCH] util: xml: remove unused function virXMLChildElementCount

[Ján Tomko]

On a Friday in 2020, Yi Li wrote:

On 11/6/20, Ján Tomko  wrote:

On a Friday in 2020, Yi Li wrote:

---
src/libvirt_private.syms |  1 -
src/util/virxml.c| 21 -
src/util/virxml.h|  1 -
3 files changed, 23 deletions(-)



Looks good, but it's missing a sign-off.

Can you, please, include a Signed-off-by line to indicate you complied
with the Developer Certificate of Origin?



Signed-off-by: Yi Li 



Thanks, pushed now.

Jano


signature.asc
Description: PGP signature

Re: [PATCH] viridentitytest: Drop #if !WITH_SELINUX block

[Ján Tomko]

On a Friday in 2020, Michal Privoznik wrote:

The whole test is built only if SELinux secdriver is enabled and
thus the !WITH_SELINUX can't be satisfied really.


Which seems like a mistake. testIdentityAttrs does not depend on SELinux
and the virIdentity APIs it tests are called even from code that does
not require SELinux.

Jano


Also, the block
references @ret variable which exists no more after v5.9.0-rc1~269.

Signed-off-by: Michal Privoznik 
---
tests/viridentitytest.c | 8 
1 file changed, 8 deletions(-)

diff --git a/tests/viridentitytest.c b/tests/viridentitytest.c
index fd01eeaa80..7a7101ba2a 100644
--- a/tests/viridentitytest.c
+++ b/tests/viridentitytest.c
@@ -85,14 +85,6 @@ static int testIdentityGetSystem(const void *data)
const char *val;
int rc;

-#if !WITH_SELINUX
-if (context) {
-VIR_DEBUG("libvirt not compiled with SELinux, skipping this test");
-ret = EXIT_AM_SKIP;
-return -1;
-}
-#endif
-
if (!(ident = virIdentityGetSystem())) {
VIR_DEBUG("Unable to get system identity");
return -1;
--
2.26.2



signature.asc
Description: PGP signature

Re: [PATCH] util: xml: remove unused function virXMLChildElementCount

[Yi Li]

On 11/6/20, Ján Tomko  wrote:
> On a Friday in 2020, Yi Li wrote:
>>---
>> src/libvirt_private.syms |  1 -
>> src/util/virxml.c| 21 -
>> src/util/virxml.h|  1 -
>> 3 files changed, 23 deletions(-)
>>
>
> Looks good, but it's missing a sign-off.
>
> Can you, please, include a Signed-off-by line to indicate you complied
> with the Developer Certificate of Origin?
>

Signed-off-by: Yi Li 

> https://libvirt.org/hacking.html#developer-certificate-of-origin
>
> Replying to this e-mail with the line is enough, no need to resend the
> patch.
>

Thanks

> Jano
>

Re: [PATCH] util: xml: remove unused function virXMLChildElementCount

[Ján Tomko]

On a Friday in 2020, Yi Li wrote:

---
src/libvirt_private.syms |  1 -
src/util/virxml.c| 21 -
src/util/virxml.h|  1 -
3 files changed, 23 deletions(-)



Looks good, but it's missing a sign-off.

Can you, please, include a Signed-off-by line to indicate you complied
with the Developer Certificate of Origin?

https://libvirt.org/hacking.html#developer-certificate-of-origin

Replying to this e-mail with the line is enough, no need to resend the
patch.

Jano


signature.asc
Description: PGP signature

[PATCH] viridentitytest: Drop #if !WITH_SELINUX block

[Michal Privoznik]

The whole test is built only if SELinux secdriver is enabled and
thus the !WITH_SELINUX can't be satisfied really. Also, the block
references @ret variable which exists no more after v5.9.0-rc1~269.

Signed-off-by: Michal Privoznik 
---
 tests/viridentitytest.c | 8 
 1 file changed, 8 deletions(-)

diff --git a/tests/viridentitytest.c b/tests/viridentitytest.c
index fd01eeaa80..7a7101ba2a 100644
--- a/tests/viridentitytest.c
+++ b/tests/viridentitytest.c
@@ -85,14 +85,6 @@ static int testIdentityGetSystem(const void *data)
 const char *val;
 int rc;
 
-#if !WITH_SELINUX
-if (context) {
-VIR_DEBUG("libvirt not compiled with SELinux, skipping this test");
-ret = EXIT_AM_SKIP;
-return -1;
-}
-#endif
-
 if (!(ident = virIdentityGetSystem())) {
 VIR_DEBUG("Unable to get system identity");
 return -1;
-- 
2.26.2

Re: Libvirt Open Source Contribution

[Michal Prívozník]

On 11/5/20 10:04 PM, Barrett J Schonefeld wrote:

Hey folks,

We appreciate the feedback, and we've used this to complete some initial 
work on issue 11.


We started with small changes in src/util, and we submitted them via 
email (following these guidelines on submitting patches 
) to ensure we are on the 
right path.


We do not see the patch we submitted via email showing up in the 
threads, and we are wondering if we are submitting incorrectly or if the 
patch goes through some approval process before appearing in the threads.


Best regards,
Barrett Schonefeld


So I checked the moderator's queue and there are no patches stuck, so 
probably it is something else. Did you see any error when 'git 
send-email'-ing the patches? When I was setting up my machine I used to 
send patches just to me to verify send-email is working and only then I 
tried to send patches to the list.


Also, are you sure you (the sender) are subscribed to the list? 
Generally, if you aren't then list won't forward your e-mails/patches.


Michal