date:20200728

  Laine Stump wrote:

> On 7/28/20 7:44 PM, Ján Tomko wrote:
> > Ján Tomko (4):
> >tests: bhyve: split variable declarations
> >tests: bhyve: use g_autofree where possible
> >tests: bhyve: use g_autoptr where possible
> >tests: bhyve: remove unnecessary labels
> >
> >   tests/bhyveargv2xmltest.c | 47 ++-
> >   tests/bhyvexml2argvtest.c | 33 ++-
> >   tests/bhyvexml2xmltest.c  |  6 ++---
> >   3 files changed, 31 insertions(+), 55 deletions(-)
> >
> 
> Series
> 
> 
> Reviewed-by: Laine Stump 
> 

Reviewed-by: Roman Bogorodskiy 

Roman Bogorodskiy


signature.asc
Description: PGP signature

[PATCH v2] qemuDomainSaveInternal: fix memoryleak of virDomainDef

2020-07-28 Thread Chuan Zheng

From: Zheng Chuan 

virDomainDefPtr 'def' is forgot to free after qemuDomainDefFormatLive(), fix it.

Signed-off-by: Zheng Chuan 
---
 src/qemu/qemu_driver.c | 9 +++--
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 53980d4..2dafe7c 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -3356,18 +3356,15 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver,
  * is NULL or whether it was the live xml of the domain moments
  * before.  */
 if (xmlin) {
-virDomainDefPtr def = NULL;
+g_autoptr(virDomainDef) def = NULL;
 
 if (!(def = virDomainDefParseString(xmlin, driver->xmlopt,
 priv->qemuCaps,
 VIR_DOMAIN_DEF_PARSE_INACTIVE |
-
VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE))) {
+
VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE)))
 goto endjob;
-}
-if (!qemuDomainCheckABIStability(driver, vm, def)) {
-virDomainDefFree(def);
+if (!qemuDomainCheckABIStability(driver, vm, def))
 goto endjob;
-}
 xml = qemuDomainDefFormatLive(driver, priv->qemuCaps, def, NULL, true, 
true);
 } else {
 xml = qemuDomainDefFormatLive(driver, priv->qemuCaps, vm->def,
-- 
1.8.3.1

Re: [PATCH] qemuDomainSaveInternal: fix memoryleak of virDomainDef


On 7/28/20 9:56 PM, Chuan Zheng wrote:

From: Zheng Chuan 

virDomainDefPtr 'def' is forgot to free after qemuDomainDefFormatLive(), fix it.

Signed-off-by: Zheng Chuan 
---
  src/qemu/qemu_driver.c | 1 +
  1 file changed, 1 insertion(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 53980d4..b145318 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -3369,6 +3369,7 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver,
  goto endjob;
  }
  xml = qemuDomainDefFormatLive(driver, priv->qemuCaps, def, NULL, 
true, true);
+virDomainDefFree(def);
  } else {
  xml = qemuDomainDefFormatLive(driver, priv->qemuCaps, vm->def,
priv->origCPU, true, true);


Instead you could use g_autoptr when defining def:


  g_autoptr(virDomainDef) def = NULL;


(and then remove the existing call to virDomainDefFree() that's a few 
lines down)(and get rid of the {  } around the now single line "goto 
endjob;" that remains). That way def is just always freed when leaving 
the scope of the "if (xmlin) { }"

Re: [RFC] Dynamic creation of VFs in a network definition containing an SRIOV device


On 7/28/20 4:46 PM, Daniel Henrique Barboza wrote:



On 7/28/20 12:03 PM, Paulo de Rezende Pinatti wrote:

Context:

Libvirt can already detect the active VFs of an SRIOV PF device 
specified in a network definition and automatically assign these VFs 
to guests via an  entry referring to that network in the 
domain definition. This functionality, however, depends on the system 
administrator having activated in advance the desired number of VFs 
outside of libvirt (either manually or through system scripts).


It would be more convenient if the VFs activation could also be 
managed inside libvirt so that the whole management of the VF pool is 
done exclusively by libvirt and in only one place (the network 
definition) rather than spread in different components of the system.


Proposal:

We can extend the existing network definition by adding a new tag 
 as a child of the tag  in order to allow the user to specify 
how many VFs they wish to have activated for the corresponding SRIOV 
device when the network is started. That would look like the following:



    sriov-pool
    
  
 
  
    


At xml definition time nothing gets changed on the system, as it is 
today. When the network is started with 'virth net-start sriov-pool' 
then libvirt will activate the desired number of VFs as specified in 
the tag  of the network definition.


The operation might require resetting 'sriov_numvfs' to zero first in 
case the number of VFs currently active differs from the desired value.



You don't specifically say it here, but any time sriov_numvfs is changed 
(and it must be changed by first setting it to 0, then back to the new 
number), *all* existing VFs are destroyed, and then recreated. And when 
it is recreated, it is a completely new device, and any previous use of 
the device will be disrupted/forgotten/whatever - the exact behavior of 
any user of any of the previously existing devices is undefined, but it 
certainly will no longer work, and will be unrecoverable without 
starting over from scratch.



This means that any sort of API that can change sriov_numvfs has the 
potential to seriously mess up anything using the VFs, and so must take 
extra care to not do anything unless there's no possibility of that 
happening. Note that SR-IOV VFs aren't just used for assigning to guests 
with vfio. They can also be used for macvtap pass-through mode, and now 
for vdpa, and possibly/probably other things.



In order to avoid the situation where the user tries to start the 
network when a VF is already assigned to a running guest, the 
implementation will have to ensure all existing VFs of the target PF 
are not in use, otherwise VFs would be inadvertently hot-unplugged 
from guests upon network start. In such cases, trying to start the 
network will then result in an error.


I'm not sure about the "echo 0 > sriov_numvfs' part. It works like 
that for Mellanox
CX-4 and CX-5 cards but I can't say it works like that for every other 
SR-IOV card out

there.



It works that way for every SR-IOV card I've ever seen. If it isn't 
written in a standards document somewhere, it is at least a defacto 
standard.



Sooner enough, we'll have to handle specific behavior for the cards to 
create

the VFs.



If you're wondering if different cards create their VFs in different 
ways - at a lower level that is possibly the case. I know that in the 
past (before the sriov_totalvfs / sriov_numvfs sysfs interface existed) 
the way to create a certain number of VFs was to add options to the PF 
driver options, and the exact options were different for each vendor. 
The sysfs interface was at least partly intended to remedy that 
discrepancy between drivers.




Perhaps Laine can comment on this.

About the whole idea, it kind of changes the design of this network 
pool. As it is today,
at least from my reading of [1], Libvirt will use any available VF 
from the pool and allocate it
to the guest, coping with the existing host VF settings. Using this 
new option, Libvirt is now
setting the VFs to a specific number, which might as well be less than 
the actual setting,

disrupting the host for no apparent reason.

I would be on board with this idea if:

1 - The attribute is changed to "minimal VFs required for this pool" 
rather than "change the host
to match this VF number". This means that we wouldn't tamper with the 
created VFs if the host
already has more VFs that specified. In your example up there, setting 
10 VFs, what if the host
has 20 VFs? Why should Libvirt care about taking down 10 VFs that it 
wouldn't use in the

first place?

2 - we find a universal way (or as much closer as universal) to handle 
the creation of VFs.



Writing to sriov_numvfs is afaik, the universal interface to create VFs.




3 - we guarantee that the process of VF creation, which will take down 
all existing VFs in
case of CX-5 cards with echo 0 > numvfs for example, wouldn't disrupt 
the host in any

way.



Definitely this

Re: [libvirt PATCH 0/4] tests: bhyve: use more g_auto


On 7/28/20 7:44 PM, Ján Tomko wrote:

Ján Tomko (4):
   tests: bhyve: split variable declarations
   tests: bhyve: use g_autofree where possible
   tests: bhyve: use g_autoptr where possible
   tests: bhyve: remove unnecessary labels

  tests/bhyveargv2xmltest.c | 47 ++-
  tests/bhyvexml2argvtest.c | 33 ++-
  tests/bhyvexml2xmltest.c  |  6 ++---
  3 files changed, 31 insertions(+), 55 deletions(-)



Series


Reviewed-by: Laine Stump

Re: [libvirt PATCH 0/6] tests: qemu: use more g_auto


On 7/28/20 7:43 PM, Ján Tomko wrote:

Ján Tomko (6):
   tests: qemu: reduce scope of some variables
   tests: qemucapsxml2xmltest: split variable declaration
   tests: qemu: use g_autofree where possible
   tests: qemu: use g_autoptr where possible
   tests: qemu: use VIR_AUTOSTRINGLIST where possible
   tests: qemu: remove unnecessary labels

  tests/qemuagenttest.c | 16 +++
  tests/qemublocktest.c |  2 +-
  tests/qemucapabilitiestest.c  | 41 ++---
  tests/qemucaps2xmltest.c  | 39 ++--
  tests/qemudomainsnapshotxml2xmltest.c | 29 +---
  tests/qemuhotplugtest.c   | 52 +++---
  tests/qemumemlocktest.c   | 26 +++
  tests/qemumigparamstest.c | 62 +-
  tests/qemumonitortestutils.c  | 64 ++-
  tests/qemusecuritymock.c  | 12 ++---
  tests/qemuxml2argvtest.c  | 27 ---
  tests/qemuxml2xmltest.c   |  9 ++--
  tests/testutilsqemu.c | 11 ++---
  tests/utiltest.c  | 15 +--
  14 files changed, 133 insertions(+), 272 deletions(-)



Series


Reviewed-by: Laine Stump

Re: [libvirt PATCH 0/5] tests: commandtest: use g_auto more


On 7/28/20 7:40 PM, Ján Tomko wrote:

Ján Tomko (5):
   tests: commandtest: remove unused 'prefix' parameter
   tests: commandtest: use g_autofree
   tests: commandtest: use g_autoptr for virCommand
   tests: commandtest: use VIR_AUTOCLOSE
   tests: commandtest: drop unnecessary labels

  tests/commandtest.c | 327 ++--
  1 file changed, 105 insertions(+), 222 deletions(-)


For the series:


Reviewed-by: Laine Stump

[PATCH] qemuDomainSaveInternal: fix memoryleak of virDomainDef

2020-07-28 Thread Chuan Zheng

From: Zheng Chuan 

virDomainDefPtr 'def' is forgot to free after qemuDomainDefFormatLive(), fix it.

Signed-off-by: Zheng Chuan 
---
 src/qemu/qemu_driver.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 53980d4..b145318 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -3369,6 +3369,7 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver,
 goto endjob;
 }
 xml = qemuDomainDefFormatLive(driver, priv->qemuCaps, def, NULL, true, 
true);
+virDomainDefFree(def);
 } else {
 xml = qemuDomainDefFormatLive(driver, priv->qemuCaps, vm->def,
   priv->origCPU, true, true);
-- 
1.8.3.1

[libvirt PATCH 3/4] tests: bhyve: use g_autoptr where possible

Signed-off-by: Ján Tomko 
---
 tests/bhyveargv2xmltest.c |  3 +--
 tests/bhyvexml2argvtest.c | 12 
 2 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/tests/bhyveargv2xmltest.c b/tests/bhyveargv2xmltest.c
index e58a36d17c..e7ec3ae020 100644
--- a/tests/bhyveargv2xmltest.c
+++ b/tests/bhyveargv2xmltest.c
@@ -32,7 +32,7 @@ testCompareXMLToArgvFiles(const char *xmlfile,
 g_autofree char *cmd = NULL;
 g_autofree char *log = NULL;
 int ret = -1;
-virDomainDefPtr vmdef = NULL;
+g_autoptr(virDomainDef) vmdef = NULL;
 
 if (virTestLoadFile(cmdfile, ) < 0)
 goto fail;
@@ -86,7 +86,6 @@ testCompareXMLToArgvFiles(const char *xmlfile,
 ret = 0;
 
  fail:
-virDomainDefFree(vmdef);
 return ret;
 }
 
diff --git a/tests/bhyvexml2argvtest.c b/tests/bhyvexml2argvtest.c
index ce2af08d4c..33e7ac5772 100644
--- a/tests/bhyvexml2argvtest.c
+++ b/tests/bhyvexml2argvtest.c
@@ -29,10 +29,10 @@ static int testCompareXMLToArgvFiles(const char *xml,
 g_autofree char *actualargv = NULL;
 g_autofree char *actualld = NULL;
 g_autofree char *actualdm = NULL;
-virDomainDefPtr vmdef = NULL;
-virCommandPtr cmd = NULL;
-virCommandPtr ldcmd = NULL;
-virConnectPtr conn;
+g_autoptr(virDomainDef) vmdef = NULL;
+g_autoptr(virCommand) cmd = NULL;
+g_autoptr(virCommand) ldcmd = NULL;
+g_autoptr(virConnect) conn = NULL;
 int ret = -1;
 
 if (!(conn = virGetConnect()))
@@ -99,10 +99,6 @@ static int testCompareXMLToArgvFiles(const char *xml,
 vmdef->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC)
 virPortAllocatorRelease(vmdef->graphics[0]->data.vnc.port);
 
-virCommandFree(cmd);
-virCommandFree(ldcmd);
-virDomainDefFree(vmdef);
-virObjectUnref(conn);
 return ret;
 }
 
-- 
2.26.2

[libvirt PATCH 1/4] tests: bhyve: split variable declarations

One variable per line.

Signed-off-by: Ján Tomko 
---
 tests/bhyvexml2argvtest.c | 11 ---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/tests/bhyvexml2argvtest.c b/tests/bhyvexml2argvtest.c
index 9e7eb218b8..f7ae1db26d 100644
--- a/tests/bhyvexml2argvtest.c
+++ b/tests/bhyvexml2argvtest.c
@@ -26,9 +26,12 @@ static int testCompareXMLToArgvFiles(const char *xml,
  const char *dmcmdline,
  unsigned int flags)
 {
-char *actualargv = NULL, *actualld = NULL, *actualdm = NULL;
+char *actualargv = NULL;
+char *actualld = NULL;
+char *actualdm = NULL;
 virDomainDefPtr vmdef = NULL;
-virCommandPtr cmd = NULL, ldcmd = NULL;
+virCommandPtr cmd = NULL;
+virCommandPtr ldcmd = NULL;
 virConnectPtr conn;
 int ret = -1;
 
@@ -117,7 +120,9 @@ testCompareXMLToArgvHelper(const void *data)
 int ret = -1;
 const struct testInfo *info = data;
 char *xml = NULL;
-char *args = NULL, *ldargs = NULL, *dmargs = NULL;
+char *args = NULL;
+char *ldargs = NULL;
+char *dmargs = NULL;
 
 xml = g_strdup_printf("%s/bhyvexml2argvdata/bhyvexml2argv-%s.xml",
   abs_srcdir, info->name);
-- 
2.26.2

[libvirt PATCH 4/4] tests: bhyve: remove unnecessary labels

Signed-off-by: Ján Tomko 
---
 tests/bhyveargv2xmltest.c | 28 +++-
 tests/bhyvexml2argvtest.c |  5 +
 2 files changed, 12 insertions(+), 21 deletions(-)

diff --git a/tests/bhyveargv2xmltest.c b/tests/bhyveargv2xmltest.c
index e7ec3ae020..2a497f48e8 100644
--- a/tests/bhyveargv2xmltest.c
+++ b/tests/bhyveargv2xmltest.c
@@ -31,11 +31,10 @@ testCompareXMLToArgvFiles(const char *xmlfile,
 g_autofree char *actualxml = NULL;
 g_autofree char *cmd = NULL;
 g_autofree char *log = NULL;
-int ret = -1;
 g_autoptr(virDomainDef) vmdef = NULL;
 
 if (virTestLoadFile(cmdfile, ) < 0)
-goto fail;
+return -1;
 
 if (!(vmdef = bhyveParseCommandLineString(cmd, driver.bhyvecaps,
   driver.xmlopt))) {
@@ -43,16 +42,16 @@ testCompareXMLToArgvFiles(const char *xmlfile,
 VIR_TEST_DEBUG("Got expected failure from "
"bhyveParseCommandLineString.");
 } else {
-goto fail;
+return -1;
 }
 } else if ((flags & FLAG_EXPECT_FAILURE)) {
 VIR_TEST_DEBUG("Did not get expected failure from "
"bhyveParseCommandLineString.");
-goto fail;
+return -1;
 }
 
 if ((log = virTestLogContentAndReset()) == NULL)
-goto fail;
+return -1;
 if (flags & FLAG_EXPECT_WARNING) {
 if (*log) {
 VIR_TEST_DEBUG("Got expected warning from "
@@ -61,32 +60,29 @@ testCompareXMLToArgvFiles(const char *xmlfile,
 } else {
 VIR_TEST_DEBUG("bhyveParseCommandLineString "
"should have logged a warning");
-goto fail;
+return -1;
 }
 } else { /* didn't expect a warning */
 if (*log) {
 VIR_TEST_DEBUG("Got unexpected warning from "
"bhyveParseCommandLineString:\n%s",
log);
-goto fail;
+return -1;
 }
 }
 
 if (vmdef && !virDomainDefCheckABIStability(vmdef, vmdef, driver.xmlopt)) {
 VIR_TEST_DEBUG("ABI stability check failed on %s", xmlfile);
-goto fail;
+return -1;
 }
 
 if (vmdef && !(actualxml = virDomainDefFormat(vmdef, driver.xmlopt, 0)))
-goto fail;
+return -1;
 
 if (vmdef && virTestCompareToFile(actualxml, xmlfile) < 0)
-goto fail;
+return -1;
 
-ret = 0;
-
- fail:
-return ret;
+return 0;
 }
 
 struct testInfo {
@@ -97,7 +93,6 @@ struct testInfo {
 static int
 testCompareXMLToArgvHelper(const void *data)
 {
-int result = -1;
 const struct testInfo *info = data;
 g_autofree char *xml = NULL;
 g_autofree char *args = NULL;
@@ -107,8 +102,7 @@ testCompareXMLToArgvHelper(const void *data)
 args = g_strdup_printf("%s/bhyveargv2xmldata/bhyveargv2xml-%s.args",
abs_srcdir, info->name);
 
-result = testCompareXMLToArgvFiles(xml, args, info->flags);
-return result;
+return testCompareXMLToArgvFiles(xml, args, info->flags);
 }
 
 static int
diff --git a/tests/bhyvexml2argvtest.c b/tests/bhyvexml2argvtest.c
index 33e7ac5772..7ce4dc6e58 100644
--- a/tests/bhyvexml2argvtest.c
+++ b/tests/bhyvexml2argvtest.c
@@ -110,7 +110,6 @@ struct testInfo {
 static int
 testCompareXMLToArgvHelper(const void *data)
 {
-int ret = -1;
 const struct testInfo *info = data;
 g_autofree char *xml = NULL;
 g_autofree char *args = NULL;
@@ -126,9 +125,7 @@ testCompareXMLToArgvHelper(const void *data)
 dmargs = g_strdup_printf("%s/bhyvexml2argvdata/bhyvexml2argv-%s.devmap",
  abs_srcdir, info->name);
 
-ret = testCompareXMLToArgvFiles(xml, args, ldargs, dmargs, info->flags);
-
-return ret;
+return testCompareXMLToArgvFiles(xml, args, ldargs, dmargs, info->flags);
 }
 
 static int
-- 
2.26.2

[libvirt PATCH 0/4] tests: bhyve: use more g_auto

Ján Tomko (4):
  tests: bhyve: split variable declarations
  tests: bhyve: use g_autofree where possible
  tests: bhyve: use g_autoptr where possible
  tests: bhyve: remove unnecessary labels

 tests/bhyveargv2xmltest.c | 47 ++-
 tests/bhyvexml2argvtest.c | 33 ++-
 tests/bhyvexml2xmltest.c  |  6 ++---
 3 files changed, 31 insertions(+), 55 deletions(-)

-- 
2.26.2

[libvirt PATCH 2/4] tests: bhyve: use g_autofree where possible

Signed-off-by: Ján Tomko 
---
 tests/bhyveargv2xmltest.c | 16 +---
 tests/bhyvexml2argvtest.c | 21 +++--
 tests/bhyvexml2xmltest.c  |  6 ++
 3 files changed, 14 insertions(+), 29 deletions(-)

diff --git a/tests/bhyveargv2xmltest.c b/tests/bhyveargv2xmltest.c
index 735cc4b338..e58a36d17c 100644
--- a/tests/bhyveargv2xmltest.c
+++ b/tests/bhyveargv2xmltest.c
@@ -28,9 +28,9 @@ testCompareXMLToArgvFiles(const char *xmlfile,
   unsigned int flags)
 
 {
-char *actualxml = NULL;
-char *cmd = NULL;
-char *log = NULL;
+g_autofree char *actualxml = NULL;
+g_autofree char *cmd = NULL;
+g_autofree char *log = NULL;
 int ret = -1;
 virDomainDefPtr vmdef = NULL;
 
@@ -86,9 +86,6 @@ testCompareXMLToArgvFiles(const char *xmlfile,
 ret = 0;
 
  fail:
-VIR_FREE(actualxml);
-VIR_FREE(cmd);
-VIR_FREE(log);
 virDomainDefFree(vmdef);
 return ret;
 }
@@ -103,8 +100,8 @@ testCompareXMLToArgvHelper(const void *data)
 {
 int result = -1;
 const struct testInfo *info = data;
-char *xml = NULL;
-char *args = NULL;
+g_autofree char *xml = NULL;
+g_autofree char *args = NULL;
 
 xml = g_strdup_printf("%s/bhyveargv2xmldata/bhyveargv2xml-%s.xml",
   abs_srcdir, info->name);
@@ -112,9 +109,6 @@ testCompareXMLToArgvHelper(const void *data)
abs_srcdir, info->name);
 
 result = testCompareXMLToArgvFiles(xml, args, info->flags);
-
-VIR_FREE(xml);
-VIR_FREE(args);
 return result;
 }
 
diff --git a/tests/bhyvexml2argvtest.c b/tests/bhyvexml2argvtest.c
index f7ae1db26d..ce2af08d4c 100644
--- a/tests/bhyvexml2argvtest.c
+++ b/tests/bhyvexml2argvtest.c
@@ -26,9 +26,9 @@ static int testCompareXMLToArgvFiles(const char *xml,
  const char *dmcmdline,
  unsigned int flags)
 {
-char *actualargv = NULL;
-char *actualld = NULL;
-char *actualdm = NULL;
+g_autofree char *actualargv = NULL;
+g_autofree char *actualld = NULL;
+g_autofree char *actualdm = NULL;
 virDomainDefPtr vmdef = NULL;
 virCommandPtr cmd = NULL;
 virCommandPtr ldcmd = NULL;
@@ -99,9 +99,6 @@ static int testCompareXMLToArgvFiles(const char *xml,
 vmdef->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC)
 virPortAllocatorRelease(vmdef->graphics[0]->data.vnc.port);
 
-VIR_FREE(actualargv);
-VIR_FREE(actualld);
-VIR_FREE(actualdm);
 virCommandFree(cmd);
 virCommandFree(ldcmd);
 virDomainDefFree(vmdef);
@@ -119,10 +116,10 @@ testCompareXMLToArgvHelper(const void *data)
 {
 int ret = -1;
 const struct testInfo *info = data;
-char *xml = NULL;
-char *args = NULL;
-char *ldargs = NULL;
-char *dmargs = NULL;
+g_autofree char *xml = NULL;
+g_autofree char *args = NULL;
+g_autofree char *ldargs = NULL;
+g_autofree char *dmargs = NULL;
 
 xml = g_strdup_printf("%s/bhyvexml2argvdata/bhyvexml2argv-%s.xml",
   abs_srcdir, info->name);
@@ -135,10 +132,6 @@ testCompareXMLToArgvHelper(const void *data)
 
 ret = testCompareXMLToArgvFiles(xml, args, ldargs, dmargs, info->flags);
 
-VIR_FREE(xml);
-VIR_FREE(args);
-VIR_FREE(ldargs);
-VIR_FREE(dmargs);
 return ret;
 }
 
diff --git a/tests/bhyvexml2xmltest.c b/tests/bhyvexml2xmltest.c
index a0c20a14c1..2ac102b8e3 100644
--- a/tests/bhyvexml2xmltest.c
+++ b/tests/bhyvexml2xmltest.c
@@ -26,8 +26,8 @@ static int
 testCompareXMLToXMLHelper(const void *data)
 {
 const struct testInfo *info = data;
-char *xml_in = NULL;
-char *xml_out = NULL;
+g_autofree char *xml_in = NULL;
+g_autofree char *xml_out = NULL;
 bool is_different = info->flags & FLAG_IS_DIFFERENT;
 int ret = -1;
 
@@ -48,8 +48,6 @@ testCompareXMLToXMLHelper(const void *data)
 virResetLastError();
 }
 
-VIR_FREE(xml_in);
-VIR_FREE(xml_out);
 return ret;
 }
 
-- 
2.26.2

[libvirt PATCH 2/6] tests: qemucapsxml2xmltest: split variable declaration

One variable per line.

Signed-off-by: Ján Tomko 
---
 tests/qemucaps2xmltest.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/qemucaps2xmltest.c b/tests/qemucaps2xmltest.c
index 7a5125fea6..f087b037ce 100644
--- a/tests/qemucaps2xmltest.c
+++ b/tests/qemucaps2xmltest.c
@@ -137,7 +137,8 @@ testQemuCapsXML(const void *opaque)
 {
 int ret = -1;
 const testQemuData *data = opaque;
-char *capsFile = NULL, *xmlFile = NULL;
+char *capsFile = NULL
+char *xmlFile = NULL;
 char *capsData = NULL;
 char *capsXml = NULL;
 virCapsPtr capsProvided = NULL;
-- 
2.26.2

[libvirt PATCH 1/6] tests: qemu: reduce scope of some variables

Reduce the scope of some variables and mark them as
g_autofree.

Signed-off-by: Ján Tomko 
---
 tests/qemumonitortestutils.c |  6 ++
 tests/testutilsqemu.c|  4 +---
 tests/utiltest.c | 15 ++-
 3 files changed, 5 insertions(+), 20 deletions(-)

diff --git a/tests/qemumonitortestutils.c b/tests/qemumonitortestutils.c
index 180ac15a15..c7396e4361 100644
--- a/tests/qemumonitortestutils.c
+++ b/tests/qemumonitortestutils.c
@@ -787,7 +787,6 @@ qemuMonitorTestProcessCommandWithArgs(qemuMonitorTestPtr 
test,
 virJSONValuePtr val = NULL;
 virJSONValuePtr args;
 virJSONValuePtr argobj;
-char *argstr = NULL;
 const char *cmdname;
 size_t i;
 int ret = -1;
@@ -815,6 +814,8 @@ qemuMonitorTestProcessCommandWithArgs(qemuMonitorTestPtr 
test,
 /* validate the args */
 for (i = 0; i < data->nargs; i++) {
 qemuMonitorTestCommandArgsPtr arg = >args[i];
+g_autofree char *argstr = NULL;
+
 if (!(argobj = virJSONValueObjectGet(args, arg->argname))) {
 qemuMonitorTestError("Missing argument '%s' for command '%s'",
  arg->argname,
@@ -835,15 +836,12 @@ qemuMonitorTestProcessCommandWithArgs(qemuMonitorTestPtr 
test,
  arg->argval, argstr);
 goto cleanup;
 }
-
-VIR_FREE(argstr);
 }
 
 /* arguments checked out, return the response */
 ret = qemuMonitorTestAddResponse(test, data->response);
 
  cleanup:
-VIR_FREE(argstr);
 virJSONValueFree(val);
 return ret;
 }
diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c
index e3b1e2813b..855e51460b 100644
--- a/tests/testutilsqemu.c
+++ b/tests/testutilsqemu.c
@@ -497,7 +497,6 @@ testQemuGetLatestCapsForArch(const char *arch,
 DIR *dir = NULL;
 int rc;
 char *fullsuffix = NULL;
-char *tmp = NULL;
 unsigned long maxver = 0;
 unsigned long ver;
 g_autofree char *maxname = NULL;
@@ -509,7 +508,7 @@ testQemuGetLatestCapsForArch(const char *arch,
 goto cleanup;
 
 while ((rc = virDirRead(dir, , TEST_QEMU_CAPS_PATH)) > 0) {
-VIR_FREE(tmp);
+g_autofree char *tmp = NULL;
 
 tmp = g_strdup(STRSKIP(ent->d_name, "caps_"));
 
@@ -543,7 +542,6 @@ testQemuGetLatestCapsForArch(const char *arch,
 ret = g_strdup_printf("%s/%s", TEST_QEMU_CAPS_PATH, maxname);
 
  cleanup:
-VIR_FREE(tmp);
 VIR_FREE(fullsuffix);
 virDirClose();
 return ret;
diff --git a/tests/utiltest.c b/tests/utiltest.c
index 2bff7859dc..2921ae8d8c 100644
--- a/tests/utiltest.c
+++ b/tests/utiltest.c
@@ -42,23 +42,18 @@ static int
 testIndexToDiskName(const void *data G_GNUC_UNUSED)
 {
 size_t i;
-char *diskName = NULL;
 
 for (i = 0; i < G_N_ELEMENTS(diskNames); ++i) {
-VIR_FREE(diskName);
+g_autofree char *diskName = NULL;
 
 diskName = virIndexToDiskName(i, "sd");
 
 if (STRNEQ(diskNames[i], diskName)) {
 virTestDifference(stderr, diskNames[i], diskName);
-VIR_FREE(diskName);
-
 return -1;
 }
 }
 
-VIR_FREE(diskName);
-
 return 0;
 }
 
@@ -69,10 +64,9 @@ testDiskNameToIndex(const void *data G_GNUC_UNUSED)
 {
 size_t i;
 int idx;
-char *diskName = NULL;
 
 for (i = 0; i < 10; ++i) {
-VIR_FREE(diskName);
+g_autofree char *diskName = NULL;
 
 diskName = virIndexToDiskName(i, "sd");
 idx = virDiskNameToIndex(diskName);
@@ -80,15 +74,10 @@ testDiskNameToIndex(const void *data G_GNUC_UNUSED)
 if (idx < 0 || idx != i) {
 VIR_TEST_DEBUG("\nExpect [%zu]", i);
 VIR_TEST_DEBUG("Actual [%d]", idx);
-
-VIR_FREE(diskName);
-
 return -1;
 }
 }
 
-VIR_FREE(diskName);
-
 return 0;
 }
 
-- 
2.26.2

[libvirt PATCH 6/6] tests: qemu: remove unnecessary labels

Signed-off-by: Ján Tomko 
---
 tests/qemucapabilitiestest.c  | 14 ---
 tests/qemucaps2xmltest.c  | 13 --
 tests/qemudomainsnapshotxml2xmltest.c | 20 +++-
 tests/qemuhotplugtest.c   | 14 ---
 tests/qemumemlocktest.c   |  9 ++-
 tests/qemumigparamstest.c | 14 ---
 tests/qemumonitortestutils.c  | 34 ++-
 7 files changed, 43 insertions(+), 75 deletions(-)

diff --git a/tests/qemucapabilitiestest.c b/tests/qemucapabilitiestest.c
index 81f29c6493..82309b44bb 100644
--- a/tests/qemucapabilitiestest.c
+++ b/tests/qemucapabilitiestest.c
@@ -141,7 +141,6 @@ testQemuCaps(const void *opaque)
 static int
 testQemuCapsCopy(const void *opaque)
 {
-int ret = -1;
 const testQemuData *data = opaque;
 g_autofree char *capsFile = NULL;
 g_autoptr(virQEMUCaps) orig = NULL;
@@ -154,21 +153,18 @@ testQemuCapsCopy(const void *opaque)
 
 if (!(orig = qemuTestParseCapabilitiesArch(
   virArchFromString(data->archName), capsFile)))
-goto cleanup;
+return -1;
 
 if (!(copy = virQEMUCapsNewCopy(orig)))
-goto cleanup;
+return -1;
 
 if (!(actual = virQEMUCapsFormatCache(copy)))
-goto cleanup;
+return -1;
 
 if (virTestCompareToFile(actual, capsFile) < 0)
-goto cleanup;
+return -1;
 
-ret = 0;
-
- cleanup:
-return ret;
+return 0;
 }
 
 
diff --git a/tests/qemucaps2xmltest.c b/tests/qemucaps2xmltest.c
index 5316a221ff..af4c96cb69 100644
--- a/tests/qemucaps2xmltest.c
+++ b/tests/qemucaps2xmltest.c
@@ -129,7 +129,6 @@ testGetCaps(char *capsData, const testQemuData *data)
 static int
 testQemuCapsXML(const void *opaque)
 {
-int ret = -1;
 const testQemuData *data = opaque;
 g_autofree char *capsFile = NULL;
 g_autofree char *xmlFile = NULL;
@@ -144,21 +143,19 @@ testQemuCapsXML(const void *opaque)
data->archName, data->suffix);
 
 if (virTestLoadFile(capsFile, ) < 0)
-goto cleanup;
+return -1;
 
 if (!(capsProvided = testGetCaps(capsData, data)))
-goto cleanup;
+return -1;
 
 capsXml = virCapabilitiesFormatXML(capsProvided);
 if (!capsXml)
-goto cleanup;
+return -1;
 
 if (virTestCompareToFile(capsXml, xmlFile) < 0)
-goto cleanup;
+return -1;
 
-ret = 0;
- cleanup:
-return ret;
+return 0;
 }
 
 static int
diff --git a/tests/qemudomainsnapshotxml2xmltest.c 
b/tests/qemudomainsnapshotxml2xmltest.c
index 6c10f3e953..4b92967339 100644
--- a/tests/qemudomainsnapshotxml2xmltest.c
+++ b/tests/qemudomainsnapshotxml2xmltest.c
@@ -34,7 +34,6 @@ testCompareXMLToXMLFiles(const char *inxml,
 g_autofree char *inXmlData = NULL;
 g_autofree char *outXmlData = NULL;
 g_autofree char *actual = NULL;
-int ret = -1;
 unsigned int parseflags = VIR_DOMAIN_SNAPSHOT_PARSE_DISKS;
 unsigned int formatflags = VIR_DOMAIN_SNAPSHOT_FORMAT_SECURE;
 bool cur = false;
@@ -49,40 +48,37 @@ testCompareXMLToXMLFiles(const char *inxml,
 parseflags |= VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE;
 
 if (virTestLoadFile(inxml, ) < 0)
-goto cleanup;
+return -1;
 
 if (virTestLoadFile(outxml, ) < 0)
-goto cleanup;
+return -1;
 
 if (!(def = virDomainSnapshotDefParseString(inXmlData,
 driver.xmlopt, NULL, ,
 parseflags)))
-goto cleanup;
+return -1;
 if (cur) {
 if (!(flags & TEST_INTERNAL))
-goto cleanup;
+return -1;
 formatflags |= VIR_DOMAIN_SNAPSHOT_FORMAT_CURRENT;
 }
 if (flags & TEST_RUNNING) {
 if (def->state)
-goto cleanup;
+return -1;
 def->state = VIR_DOMAIN_RUNNING;
 }
 
 if (!(actual = virDomainSnapshotDefFormat(uuid, def,
   driver.xmlopt,
   formatflags)))
-goto cleanup;
+return -1;
 
 if (STRNEQ(outXmlData, actual)) {
 virTestDifferenceFull(stderr, outXmlData, outxml, actual, inxml);
-goto cleanup;
+return -1;
 }
 
-ret = 0;
-
- cleanup:
-return ret;
+return 0;
 }
 
 struct testInfo {
diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c
index ba644abe8a..1e18820a2b 100644
--- a/tests/qemuhotplugtest.c
+++ b/tests/qemuhotplugtest.c
@@ -472,33 +472,29 @@ testQemuHotplugCpuPrepare(const char *test,
 static int
 testQemuHotplugCpuFinalize(struct testQemuHotplugCpuData *data)
 {
-int ret = -1;
 g_autofree char *activeXML = NULL;
 g_autofree char *configXML = NULL;
 
 if (data->file_xml_res_live) {
 if (!(activeXML = virDomainDefFormat(data->vm->def, driver.xmlopt,

[libvirt PATCH 3/6] tests: qemu: use g_autofree where possible

Signed-off-by: Ján Tomko 
---
 tests/qemuagenttest.c |  9 +++-
 tests/qemucapabilitiestest.c  | 18 +--
 tests/qemucaps2xmltest.c  | 20 +
 tests/qemudomainsnapshotxml2xmltest.c |  9 +++-
 tests/qemuhotplugtest.c   | 32 +--
 tests/qemumemlocktest.c   | 10 +++--
 tests/qemumigparamstest.c | 24 +++-
 tests/qemumonitortestutils.c  | 15 +
 tests/qemusecuritymock.c  | 12 --
 tests/qemuxml2argvtest.c  | 15 +
 tests/qemuxml2xmltest.c   |  6 ++---
 tests/testutilsqemu.c |  7 ++
 12 files changed, 57 insertions(+), 120 deletions(-)

diff --git a/tests/qemuagenttest.c b/tests/qemuagenttest.c
index 2c3a1efb09..347438c14a 100644
--- a/tests/qemuagenttest.c
+++ b/tests/qemuagenttest.c
@@ -173,7 +173,7 @@ testQemuAgentGetFSInfoCommon(virDomainXMLOptionPtr xmlopt,
  virDomainDefPtr *def)
 {
 int ret = -1;
-char *domain_filename = NULL;
+g_autofree char *domain_filename = NULL;
 qemuMonitorTestPtr ret_test = NULL;
 virDomainDefPtr ret_def = NULL;
 
@@ -233,7 +233,6 @@ testQemuAgentGetFSInfoCommon(virDomainXMLOptionPtr xmlopt,
 ret = 0;
 
  cleanup:
-VIR_FREE(domain_filename);
 if (ret_test)
 qemuMonitorTestFree(ret_test);
 virDomainDefFree(ret_def);
@@ -664,7 +663,7 @@ testQemuAgentArbitraryCommand(const void *data)
 virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr)data;
 qemuMonitorTestPtr test = qemuMonitorTestNewAgent(xmlopt);
 int ret = -1;
-char *reply = NULL;
+g_autofree char *reply = NULL;
 
 if (!test)
 return -1;
@@ -693,7 +692,6 @@ testQemuAgentArbitraryCommand(const void *data)
 ret = 0;
 
  cleanup:
-VIR_FREE(reply);
 qemuMonitorTestFree(test);
 return ret;
 }
@@ -713,7 +711,7 @@ testQemuAgentTimeout(const void *data)
 {
 virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr)data;
 qemuMonitorTestPtr test = qemuMonitorTestNewAgent(xmlopt);
-char *reply = NULL;
+g_autofree char *reply = NULL;
 int ret = -1;
 
 if (!test)
@@ -757,7 +755,6 @@ testQemuAgentTimeout(const void *data)
 ret = 0;
 
  cleanup:
-VIR_FREE(reply);
 qemuMonitorTestFree(test);
 return ret;
 }
diff --git a/tests/qemucapabilitiestest.c b/tests/qemucapabilitiestest.c
index 5b32ac1451..be8741dd5f 100644
--- a/tests/qemucapabilitiestest.c
+++ b/tests/qemucapabilitiestest.c
@@ -71,12 +71,12 @@ testQemuCaps(const void *opaque)
 {
 int ret = -1;
 testQemuData *data = (void *) opaque;
-char *repliesFile = NULL;
-char *capsFile = NULL;
+g_autofree char *repliesFile = NULL;
+g_autofree char *capsFile = NULL;
 qemuMonitorTestPtr mon = NULL;
 virQEMUCapsPtr capsActual = NULL;
-char *binary = NULL;
-char *actual = NULL;
+g_autofree char *binary = NULL;
+g_autofree char *actual = NULL;
 unsigned int fakeMicrocodeVersion = 0;
 const char *p;
 
@@ -133,10 +133,6 @@ testQemuCaps(const void *opaque)
 
 ret = 0;
  cleanup:
-VIR_FREE(repliesFile);
-VIR_FREE(capsFile);
-VIR_FREE(actual);
-VIR_FREE(binary);
 qemuMonitorTestFree(mon);
 virObjectUnref(capsActual);
 return ret;
@@ -148,10 +144,10 @@ testQemuCapsCopy(const void *opaque)
 {
 int ret = -1;
 const testQemuData *data = opaque;
-char *capsFile = NULL;
+g_autofree char *capsFile = NULL;
 virQEMUCapsPtr orig = NULL;
 virQEMUCapsPtr copy = NULL;
-char *actual = NULL;
+g_autofree char *actual = NULL;
 
 capsFile = g_strdup_printf("%s/%s_%s.%s.xml",
data->outputDir, data->prefix, data->version,
@@ -173,10 +169,8 @@ testQemuCapsCopy(const void *opaque)
 ret = 0;
 
  cleanup:
-VIR_FREE(capsFile);
 virObjectUnref(orig);
 virObjectUnref(copy);
-VIR_FREE(actual);
 return ret;
 }
 
diff --git a/tests/qemucaps2xmltest.c b/tests/qemucaps2xmltest.c
index f087b037ce..5ff4374935 100644
--- a/tests/qemucaps2xmltest.c
+++ b/tests/qemucaps2xmltest.c
@@ -55,7 +55,7 @@ testQemuGetCaps(char *caps)
 xmlDocPtr xml;
 xmlXPathContextPtr ctxt = NULL;
 ssize_t i, n;
-xmlNodePtr *nodes = NULL;
+g_autofree xmlNodePtr *nodes = NULL;
 
 if (!(xml = virXMLParseStringCtxt(caps, "(test caps)", )))
 goto error;
@@ -69,26 +69,22 @@ testQemuGetCaps(char *caps)
 goto error;
 
 for (i = 0; i < n; i++) {
-char *str = virXMLPropString(nodes[i], "name");
+g_autofree char *str = virXMLPropString(nodes[i], "name");
 if (str) {
 int flag = virQEMUCapsTypeFromString(str);
 if (flag < 0) {
 fprintf(stderr, "Unknown qemu capabilities flag %s", str);
-VIR_FREE(str);
 goto error;
 }
-VIR_FREE(str);

[libvirt PATCH 0/6] tests: qemu: use more g_auto

Ján Tomko (6):
  tests: qemu: reduce scope of some variables
  tests: qemucapsxml2xmltest: split variable declaration
  tests: qemu: use g_autofree where possible
  tests: qemu: use g_autoptr where possible
  tests: qemu: use VIR_AUTOSTRINGLIST where possible
  tests: qemu: remove unnecessary labels

 tests/qemuagenttest.c | 16 +++
 tests/qemublocktest.c |  2 +-
 tests/qemucapabilitiestest.c  | 41 ++---
 tests/qemucaps2xmltest.c  | 39 ++--
 tests/qemudomainsnapshotxml2xmltest.c | 29 +---
 tests/qemuhotplugtest.c   | 52 +++---
 tests/qemumemlocktest.c   | 26 +++
 tests/qemumigparamstest.c | 62 +-
 tests/qemumonitortestutils.c  | 64 ++-
 tests/qemusecuritymock.c  | 12 ++---
 tests/qemuxml2argvtest.c  | 27 ---
 tests/qemuxml2xmltest.c   |  9 ++--
 tests/testutilsqemu.c | 11 ++---
 tests/utiltest.c  | 15 +--
 14 files changed, 133 insertions(+), 272 deletions(-)

-- 
2.26.2

[libvirt PATCH 5/6] tests: qemu: use VIR_AUTOSTRINGLIST where possible

Signed-off-by: Ján Tomko 
---
 tests/qemuxml2argvtest.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index f26de13df1..ecf6562e90 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -130,7 +130,7 @@ static virStorageVolPtr
 fakeStorageVolLookupByName(virStoragePoolPtr pool,
const char *name)
 {
-char **volinfo = NULL;
+VIR_AUTOSTRINGLIST volinfo = NULL;
 virStorageVolPtr ret = NULL;
 
 if (STREQ(pool->name, "inactive")) {
@@ -158,7 +158,6 @@ fakeStorageVolLookupByName(virStoragePoolPtr pool,
NULL, NULL);
 
  cleanup:
-virStringListFree(volinfo);
 return ret;
 
  fallback:
-- 
2.26.2

[libvirt PATCH 4/6] tests: qemu: use g_autoptr where possible

Signed-off-by: Ján Tomko 
---
 tests/qemuagenttest.c|  7 ++-
 tests/qemublocktest.c|  2 +-
 tests/qemucapabilitiestest.c |  9 +++--
 tests/qemucaps2xmltest.c |  7 ++-
 tests/qemuhotplugtest.c  |  6 ++
 tests/qemumemlocktest.c  |  7 ++-
 tests/qemumigparamstest.c| 24 
 tests/qemumonitortestutils.c |  9 +++--
 tests/qemuxml2argvtest.c |  9 +++--
 tests/qemuxml2xmltest.c  |  3 +--
 10 files changed, 27 insertions(+), 56 deletions(-)

diff --git a/tests/qemuagenttest.c b/tests/qemuagenttest.c
index 347438c14a..607bd97b5c 100644
--- a/tests/qemuagenttest.c
+++ b/tests/qemuagenttest.c
@@ -175,7 +175,7 @@ testQemuAgentGetFSInfoCommon(virDomainXMLOptionPtr xmlopt,
 int ret = -1;
 g_autofree char *domain_filename = NULL;
 qemuMonitorTestPtr ret_test = NULL;
-virDomainDefPtr ret_def = NULL;
+g_autoptr(virDomainDef) ret_def = NULL;
 
 if (!test || !def)
 return -1;
@@ -235,8 +235,6 @@ testQemuAgentGetFSInfoCommon(virDomainXMLOptionPtr xmlopt,
  cleanup:
 if (ret_test)
 qemuMonitorTestFree(ret_test);
-virDomainDefFree(ret_def);
-
 return ret;
 }
 
@@ -400,7 +398,7 @@ qemuAgentShutdownTestMonitorHandler(qemuMonitorTestPtr test,
 const char *cmdstr)
 {
 struct qemuAgentShutdownTestData *data;
-virJSONValuePtr val = NULL;
+g_autoptr(virJSONValue) val = NULL;
 virJSONValuePtr args;
 const char *cmdname;
 const char *mode;
@@ -447,7 +445,6 @@ qemuAgentShutdownTestMonitorHandler(qemuMonitorTestPtr test,
 ret = 0;
 
  cleanup:
-virJSONValueFree(val);
 return ret;
 
 }
diff --git a/tests/qemublocktest.c b/tests/qemublocktest.c
index 025e96ab4b..fb5319d7bd 100644
--- a/tests/qemublocktest.c
+++ b/tests/qemublocktest.c
@@ -987,7 +987,7 @@ mymain(void)
 struct testQemuBlockBitmapBlockcopyData blockbitmapblockcopydata;
 struct testQemuBlockBitmapBlockcommitData blockbitmapblockcommitdata;
 char *capslatest_x86_64 = NULL;
-virQEMUCapsPtr caps_x86_64 = NULL;
+g_autoptr(virQEMUCaps) caps_x86_64 = NULL;
 g_autoptr(virHashTable) qmp_schema_x86_64 = NULL;
 virJSONValuePtr qmp_schemaroot_x86_64_blockdev_add = NULL;
 g_autoptr(virStorageSource) bitmapSourceChain = NULL;
diff --git a/tests/qemucapabilitiestest.c b/tests/qemucapabilitiestest.c
index be8741dd5f..81f29c6493 100644
--- a/tests/qemucapabilitiestest.c
+++ b/tests/qemucapabilitiestest.c
@@ -74,7 +74,7 @@ testQemuCaps(const void *opaque)
 g_autofree char *repliesFile = NULL;
 g_autofree char *capsFile = NULL;
 qemuMonitorTestPtr mon = NULL;
-virQEMUCapsPtr capsActual = NULL;
+g_autoptr(virQEMUCaps) capsActual = NULL;
 g_autofree char *binary = NULL;
 g_autofree char *actual = NULL;
 unsigned int fakeMicrocodeVersion = 0;
@@ -134,7 +134,6 @@ testQemuCaps(const void *opaque)
 ret = 0;
  cleanup:
 qemuMonitorTestFree(mon);
-virObjectUnref(capsActual);
 return ret;
 }
 
@@ -145,8 +144,8 @@ testQemuCapsCopy(const void *opaque)
 int ret = -1;
 const testQemuData *data = opaque;
 g_autofree char *capsFile = NULL;
-virQEMUCapsPtr orig = NULL;
-virQEMUCapsPtr copy = NULL;
+g_autoptr(virQEMUCaps) orig = NULL;
+g_autoptr(virQEMUCaps) copy = NULL;
 g_autofree char *actual = NULL;
 
 capsFile = g_strdup_printf("%s/%s_%s.%s.xml",
@@ -169,8 +168,6 @@ testQemuCapsCopy(const void *opaque)
 ret = 0;
 
  cleanup:
-virObjectUnref(orig);
-virObjectUnref(copy);
 return ret;
 }
 
diff --git a/tests/qemucaps2xmltest.c b/tests/qemucaps2xmltest.c
index 5ff4374935..5316a221ff 100644
--- a/tests/qemucaps2xmltest.c
+++ b/tests/qemucaps2xmltest.c
@@ -94,7 +94,7 @@ testQemuGetCaps(char *caps)
 static virCapsPtr
 testGetCaps(char *capsData, const testQemuData *data)
 {
-virQEMUCapsPtr qemuCaps = NULL;
+g_autoptr(virQEMUCaps) qemuCaps = NULL;
 virCapsPtr caps = NULL;
 virArch arch = virArchFromString(data->archName);
 g_autofree char *binary = NULL;
@@ -119,11 +119,9 @@ testGetCaps(char *capsData, const testQemuData *data)
 goto error;
 }
 
-virObjectUnref(qemuCaps);
 return caps;
 
  error:
-virObjectUnref(qemuCaps);
 virObjectUnref(caps);
 return NULL;
 }
@@ -137,7 +135,7 @@ testQemuCapsXML(const void *opaque)
 g_autofree char *xmlFile = NULL;
 g_autofree char *capsData = NULL;
 g_autofree char *capsXml = NULL;
-virCapsPtr capsProvided = NULL;
+g_autoptr(virCaps) capsProvided = NULL;
 
 xmlFile = g_strdup_printf("%s/caps.%s.xml", data->outputDir, 
data->archName);
 
@@ -160,7 +158,6 @@ testQemuCapsXML(const void *opaque)
 
 ret = 0;
  cleanup:
-virObjectUnref(capsProvided);
 return ret;
 }
 
diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c
index 9d7d2e44e8..ba644abe8a 100644
--- a/tests/qemuhotplugtest.c
+++ b/tests/qemuhotplugtest.c
@@ -244,7 +244,7 @@

[libvirt PATCH 2/5] tests: commandtest: use g_autofree

Signed-off-by: Ján Tomko 
---
 tests/commandtest.c | 76 -
 1 file changed, 26 insertions(+), 50 deletions(-)

diff --git a/tests/commandtest.c b/tests/commandtest.c
index a18100a705..7c6c3ec75d 100644
--- a/tests/commandtest.c
+++ b/tests/commandtest.c
@@ -58,10 +58,10 @@ extern char **environ;
 static int checkoutput(const char *testname)
 {
 int ret = -1;
-char *expectname = NULL;
-char *expectlog = NULL;
-char *actualname = NULL;
-char *actuallog = NULL;
+g_autofree char *expectname = NULL;
+g_autofree char *expectlog = NULL;
+g_autofree char *actualname = NULL;
+g_autofree char *actuallog = NULL;
 
 expectname = g_strdup_printf("%s/commanddata/%s.log", abs_srcdir, 
testname);
 actualname = g_strdup_printf("%s/commandhelper.log", abs_builddir);
@@ -86,10 +86,6 @@ static int checkoutput(const char *testname)
  cleanup:
 if (actualname)
 unlink(actualname);
-VIR_FREE(actuallog);
-VIR_FREE(actualname);
-VIR_FREE(expectlog);
-VIR_FREE(expectname);
 return ret;
 }
 
@@ -247,7 +243,7 @@ static int test4(const void *unused G_GNUC_UNUSED)
 {
 virCommandPtr cmd = virCommandNewArgList(abs_builddir "/commandhelper",
  "--check-daemonize", NULL);
-char *pidfile = virPidFileBuildPath(abs_builddir, "commandhelper");
+g_autofree char *pidfile = virPidFileBuildPath(abs_builddir, 
"commandhelper");
 pid_t pid;
 int ret = -1;
 
@@ -275,7 +271,6 @@ static int test4(const void *unused G_GNUC_UNUSED)
 virCommandFree(cmd);
 if (pidfile)
 unlink(pidfile);
-VIR_FREE(pidfile);
 return ret;
 }
 
@@ -484,7 +479,7 @@ static int test12(const void *unused G_GNUC_UNUSED)
 static int test13(const void *unused G_GNUC_UNUSED)
 {
 virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper");
-char *outactual = NULL;
+g_autofree char *outactual = NULL;
 const char *outexpect = "BEGIN STDOUT\n"
 "Hello World\n"
 "END STDOUT\n";
@@ -512,7 +507,6 @@ static int test13(const void *unused G_GNUC_UNUSED)
 
  cleanup:
 virCommandFree(cmd);
-VIR_FREE(outactual);
 return ret;
 }
 
@@ -523,16 +517,16 @@ static int test13(const void *unused G_GNUC_UNUSED)
 static int test14(const void *unused G_GNUC_UNUSED)
 {
 virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper");
-char *outactual = NULL;
+g_autofree char *outactual = NULL;
 const char *outexpect = "BEGIN STDOUT\n"
 "Hello World\n"
 "END STDOUT\n";
-char *erractual = NULL;
+g_autofree char *erractual = NULL;
 const char *errexpect = "BEGIN STDERR\n"
 "Hello World\n"
 "END STDERR\n";
 
-char *jointactual = NULL;
+g_autofree char *jointactual = NULL;
 const char *jointexpect = "BEGIN STDOUT\n"
 "BEGIN STDERR\n"
 "Hello World\n"
@@ -582,9 +576,6 @@ static int test14(const void *unused G_GNUC_UNUSED)
 
  cleanup:
 virCommandFree(cmd);
-VIR_FREE(outactual);
-VIR_FREE(erractual);
-VIR_FREE(jointactual);
 return ret;
 }
 
@@ -596,7 +587,7 @@ static int test14(const void *unused G_GNUC_UNUSED)
 static int test15(const void *unused G_GNUC_UNUSED)
 {
 virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper");
-char *cwd = NULL;
+g_autofree char *cwd = NULL;
 int ret = -1;
 
 cwd = g_strdup_printf("%s/commanddata", abs_srcdir);
@@ -611,7 +602,6 @@ static int test15(const void *unused G_GNUC_UNUSED)
 ret = checkoutput("test15");
 
  cleanup:
-VIR_FREE(cwd);
 virCommandFree(cmd);
 
 return ret;
@@ -623,7 +613,7 @@ static int test15(const void *unused G_GNUC_UNUSED)
 static int test16(const void *unused G_GNUC_UNUSED)
 {
 virCommandPtr cmd = virCommandNew("true");
-char *outactual = NULL;
+g_autofree char *outactual = NULL;
 const char *outexpect = "A=B C='D  E' true F 'G  H'";
 int ret = -1;
 int fd = -1;
@@ -658,7 +648,6 @@ static int test16(const void *unused G_GNUC_UNUSED)
  cleanup:
 virCommandFree(cmd);
 VIR_FORCE_CLOSE(fd);
-VIR_FREE(outactual);
 return ret;
 }
 
@@ -669,8 +658,8 @@ static int test17(const void *unused G_GNUC_UNUSED)
 {
 virCommandPtr cmd = virCommandNew("true");
 int ret = -1;
-char *outbuf;
-char *errbuf = NULL;
+char *outbuf = NULL;
+g_autofree char *errbuf = NULL;
 
 virCommandSetOutputBuffer(cmd, );
 if (outbuf != NULL) {
@@ -711,7 +700,6 @@ static int test17(const void *unused G_GNUC_UNUSED)
  cleanup:
 virCommandFree(cmd);
 VIR_FREE(outbuf);
-VIR_FREE(errbuf);
 return ret;
 }
 
@@ -721,7 +709,7 @@ static int test17(const void *unused G_GNUC_UNUSED)
 static int test18(const void *unused G_GNUC_UNUSED)
 {
 virCommandPtr cmd = virCommandNewArgList("sleep", "100", NULL);
-char *pidfile = virPidFileBuildPath(abs_builddir, "commandhelper");
+g_autofree char *pidfile =

[libvirt PATCH 5/5] tests: commandtest: drop unnecessary labels

Signed-off-by: Ján Tomko 
---
 tests/commandtest.c | 106 +++-
 1 file changed, 36 insertions(+), 70 deletions(-)

diff --git a/tests/commandtest.c b/tests/commandtest.c
index b0bf6f379a..42225a8ef2 100644
--- a/tests/commandtest.c
+++ b/tests/commandtest.c
@@ -97,20 +97,16 @@ static int checkoutput(const char *testname)
 static int test0(const void *unused G_GNUC_UNUSED)
 {
 g_autoptr(virCommand) cmd = NULL;
-int ret = -1;
 
 cmd = virCommandNew(abs_builddir "/commandhelper-doesnotexist");
 if (virCommandRun(cmd, NULL) == 0)
-goto cleanup;
+return -1;
 
 if (virGetLastErrorCode() == VIR_ERR_OK)
-goto cleanup;
+return -1;
 
 virResetLastError();
-ret = 0;
-
- cleanup:
-return ret;
+return 0;
 }
 
 /*
@@ -121,24 +117,21 @@ static int test0(const void *unused G_GNUC_UNUSED)
 static int test1(const void *unused G_GNUC_UNUSED)
 {
 g_autoptr(virCommand) cmd = NULL;
-int ret = -1;
 int status;
 
 cmd = virCommandNew(abs_builddir "/commandhelper-doesnotexist");
 if (virCommandRun(cmd, ) < 0)
-goto cleanup;
+return -1;
 if (status != EXIT_ENOENT)
-goto cleanup;
+return -1;
 
 virCommandRawStatus(cmd);
 if (virCommandRun(cmd, ) < 0)
-goto cleanup;
+return -1;
 if (!WIFEXITED(status) || WEXITSTATUS(status) != EXIT_ENOENT)
-goto cleanup;
-ret = 0;
+return -1;
 
- cleanup:
-return ret;
+return 0;
 }
 
 /*
@@ -177,11 +170,10 @@ static int test3(const void *unused G_GNUC_UNUSED)
 VIR_AUTOCLOSE newfd2 = dup(STDERR_FILENO);
 int newfd3 = dup(STDERR_FILENO);
 struct stat before, after;
-int ret = -1;
 
 if (fstat(newfd3, ) < 0) {
 perror("fstat");
-goto cleanup;
+return -1;
 }
 virCommandPassFD(cmd, newfd1, 0);
 virCommandPassFD(cmd, newfd3,
@@ -189,13 +181,13 @@ static int test3(const void *unused G_GNUC_UNUSED)
 
 if (virCommandRun(cmd, NULL) < 0) {
 printf("Cannot run child %s\n", virGetLastErrorMessage());
-goto cleanup;
+return -1;
 }
 
 if (fcntl(newfd1, F_GETFL) < 0 ||
 fcntl(newfd2, F_GETFL) < 0) {
 puts("fds 1/2 were not open");
-goto cleanup;
+return -1;
 }
 
 /* We expect newfd3 to be closed, but the
@@ -211,14 +203,11 @@ static int test3(const void *unused G_GNUC_UNUSED)
 before.st_dev == after.st_dev &&
 before.st_mode == after.st_mode) {
 puts("fd 3 should not be open");
-goto cleanup;
+return -1;
 }
 }
 
-ret = checkoutput("test3");
-
- cleanup:
-return ret;
+return checkoutput("test3");
 }
 
 
@@ -550,7 +539,6 @@ static int test15(const void *unused G_GNUC_UNUSED)
 {
 g_autoptr(virCommand) cmd = virCommandNew(abs_builddir "/commandhelper");
 g_autofree char *cwd = NULL;
-int ret = -1;
 
 cwd = g_strdup_printf("%s/commanddata", abs_srcdir);
 virCommandSetWorkingDirectory(cmd, cwd);
@@ -558,14 +546,10 @@ static int test15(const void *unused G_GNUC_UNUSED)
 
 if (virCommandRun(cmd, NULL) < 0) {
 printf("Cannot run child %s\n", virGetLastErrorMessage());
-goto cleanup;
+return -1;
 }
 
-ret = checkoutput("test15");
-
- cleanup:
-
-return ret;
+return checkoutput("test15");
 }
 
 /*
@@ -576,7 +560,6 @@ static int test16(const void *unused G_GNUC_UNUSED)
 g_autoptr(virCommand) cmd = virCommandNew("true");
 g_autofree char *outactual = NULL;
 const char *outexpect = "A=B C='D  E' true F 'G  H'";
-int ret = -1;
 VIR_AUTOCLOSE fd = -1;
 
 virCommandAddEnvPair(cmd, "A", "B");
@@ -586,28 +569,25 @@ static int test16(const void *unused G_GNUC_UNUSED)
 
 if ((outactual = virCommandToString(cmd, false)) == NULL) {
 printf("Cannot convert to string: %s\n", virGetLastErrorMessage());
-goto cleanup;
+return -1;
 }
 if ((fd = open(abs_builddir "/commandhelper.log",
O_CREAT | O_TRUNC | O_WRONLY, 0600)) < 0) {
 printf("Cannot open log file: %s\n", g_strerror(errno));
-goto cleanup;
+return -1;
 }
 virCommandWriteArgLog(cmd, fd);
 if (VIR_CLOSE(fd) < 0) {
 printf("Cannot close log file: %s\n", g_strerror(errno));
-goto cleanup;
+return -1;
 }
 
 if (STRNEQ(outactual, outexpect)) {
 virTestDifference(stderr, outexpect, outactual);
-goto cleanup;
+return -1;
 }
 
-ret = checkoutput("test16");
-
- cleanup:
-return ret;
+return checkoutput("test16");
 }
 
 /*
@@ -715,32 +695,28 @@ static int test19(const void *unused G_GNUC_UNUSED)
 {
 g_autoptr(virCommand) cmd = virCommandNewArgList("sleep", "100", NULL);
 pid_t pid;
-int ret = -1;
 
 alarm(5);
 if (virCommandRunAsync(cmd, ) < 0) {
 printf("Cannot run child %s\n",

[libvirt PATCH 4/5] tests: commandtest: use VIR_AUTOCLOSE

Signed-off-by: Ján Tomko 
---
 tests/commandtest.c | 13 -
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/tests/commandtest.c b/tests/commandtest.c
index a54e467b2b..b0bf6f379a 100644
--- a/tests/commandtest.c
+++ b/tests/commandtest.c
@@ -173,8 +173,8 @@ static int test2(const void *unused G_GNUC_UNUSED)
 static int test3(const void *unused G_GNUC_UNUSED)
 {
 g_autoptr(virCommand) cmd = virCommandNew(abs_builddir "/commandhelper");
-int newfd1 = dup(STDERR_FILENO);
-int newfd2 = dup(STDERR_FILENO);
+VIR_AUTOCLOSE newfd1 = dup(STDERR_FILENO);
+VIR_AUTOCLOSE newfd2 = dup(STDERR_FILENO);
 int newfd3 = dup(STDERR_FILENO);
 struct stat before, after;
 int ret = -1;
@@ -218,9 +218,6 @@ static int test3(const void *unused G_GNUC_UNUSED)
 ret = checkoutput("test3");
 
  cleanup:
-/* coverity[double_close] */
-VIR_FORCE_CLOSE(newfd1);
-VIR_FORCE_CLOSE(newfd2);
 return ret;
 }
 
@@ -580,7 +577,7 @@ static int test16(const void *unused G_GNUC_UNUSED)
 g_autofree char *outactual = NULL;
 const char *outexpect = "A=B C='D  E' true F 'G  H'";
 int ret = -1;
-int fd = -1;
+VIR_AUTOCLOSE fd = -1;
 
 virCommandAddEnvPair(cmd, "A", "B");
 virCommandAddEnvPair(cmd, "C", "D  E");
@@ -610,7 +607,6 @@ static int test16(const void *unused G_GNUC_UNUSED)
 ret = checkoutput("test16");
 
  cleanup:
-VIR_FORCE_CLOSE(fd);
 return ret;
 }
 
@@ -1039,7 +1035,7 @@ static int test26(const void *unused G_GNUC_UNUSED)
 "wallop";
 
 int ret = -1;
-int fd = -1;
+VIR_AUTOCLOSE fd = -1;
 
 virCommandAddEnvPair(cmd, "A", "B");
 virCommandAddEnvPair(cmd, "C", "D  E");
@@ -1071,7 +1067,6 @@ static int test26(const void *unused G_GNUC_UNUSED)
 ret = checkoutput("test26");
 
  cleanup:
-VIR_FORCE_CLOSE(fd);
 return ret;
 }
 
-- 
2.26.2

[libvirt PATCH 1/5] tests: commandtest: remove unused 'prefix' parameter

The 'checkoutput' function does have a parameter for a possible
prefix, but it is now unused.

Introduced-by: 241ac07124a3172dc38198d777eb3f04846f6c52
Used-by: 62f263a73e8202f2af7309d1a3b9a66e6b57
Unused-since: 2dfacbffea47743fff942b6401f5f36b3ae4655a

Signed-off-by: Ján Tomko 
---
 tests/commandtest.c | 52 ++---
 1 file changed, 21 insertions(+), 31 deletions(-)

diff --git a/tests/commandtest.c b/tests/commandtest.c
index 9bef825239..a18100a705 100644
--- a/tests/commandtest.c
+++ b/tests/commandtest.c
@@ -55,8 +55,7 @@ main(void)
 /* Some UNIX lack it in headers & it doesn't hurt to redeclare */
 extern char **environ;
 
-static int checkoutput(const char *testname,
-   char *prefix)
+static int checkoutput(const char *testname)
 {
 int ret = -1;
 char *expectname = NULL;
@@ -77,15 +76,6 @@ static int checkoutput(const char *testname,
 goto cleanup;
 }
 
-if (prefix) {
-char *tmp = NULL;
-
-tmp = g_strdup_printf("%s%s", prefix, expectlog);
-
-VIR_FREE(expectlog);
-expectlog = tmp;
-}
-
 if (STRNEQ(expectlog, actuallog)) {
 virTestDifference(stderr, expectlog, actuallog);
 goto cleanup;
@@ -172,7 +162,7 @@ static int test2(const void *unused G_GNUC_UNUSED)
 return -1;
 }
 
-if ((ret = checkoutput("test2", NULL)) != 0) {
+if ((ret = checkoutput("test2")) != 0) {
 virCommandFree(cmd);
 return ret;
 }
@@ -185,7 +175,7 @@ static int test2(const void *unused G_GNUC_UNUSED)
 
 virCommandFree(cmd);
 
-return checkoutput("test2", NULL);
+return checkoutput("test2");
 }
 
 /*
@@ -237,7 +227,7 @@ static int test3(const void *unused G_GNUC_UNUSED)
 }
 }
 
-ret = checkoutput("test3", NULL);
+ret = checkoutput("test3");
 
  cleanup:
 virCommandFree(cmd);
@@ -279,7 +269,7 @@ static int test4(const void *unused G_GNUC_UNUSED)
 while (kill(pid, 0) != -1)
 g_usleep(100*1000);
 
-ret = checkoutput("test4", NULL);
+ret = checkoutput("test4");
 
  cleanup:
 virCommandFree(cmd);
@@ -308,7 +298,7 @@ static int test5(const void *unused G_GNUC_UNUSED)
 
 virCommandFree(cmd);
 
-return checkoutput("test5", NULL);
+return checkoutput("test5");
 }
 
 
@@ -331,7 +321,7 @@ static int test6(const void *unused G_GNUC_UNUSED)
 
 virCommandFree(cmd);
 
-return checkoutput("test6", NULL);
+return checkoutput("test6");
 }
 
 
@@ -355,7 +345,7 @@ static int test7(const void *unused G_GNUC_UNUSED)
 
 virCommandFree(cmd);
 
-return checkoutput("test7", NULL);
+return checkoutput("test7");
 }
 
 /*
@@ -379,7 +369,7 @@ static int test8(const void *unused G_GNUC_UNUSED)
 
 virCommandFree(cmd);
 
-return checkoutput("test8", NULL);
+return checkoutput("test8");
 }
 
 
@@ -415,7 +405,7 @@ static int test9(const void *unused G_GNUC_UNUSED)
 
 virCommandFree(cmd);
 
-return checkoutput("test9", NULL);
+return checkoutput("test9");
 }
 
 
@@ -440,7 +430,7 @@ static int test10(const void *unused G_GNUC_UNUSED)
 
 virCommandFree(cmd);
 
-return checkoutput("test10", NULL);
+return checkoutput("test10");
 }
 
 /*
@@ -463,7 +453,7 @@ static int test11(const void *unused G_GNUC_UNUSED)
 
 virCommandFree(cmd);
 
-return checkoutput("test11", NULL);
+return checkoutput("test11");
 }
 
 /*
@@ -484,7 +474,7 @@ static int test12(const void *unused G_GNUC_UNUSED)
 
 virCommandFree(cmd);
 
-return checkoutput("test12", NULL);
+return checkoutput("test12");
 }
 
 /*
@@ -518,7 +508,7 @@ static int test13(const void *unused G_GNUC_UNUSED)
 goto cleanup;
 }
 
-ret = checkoutput("test13", NULL);
+ret = checkoutput("test13");
 
  cleanup:
 virCommandFree(cmd);
@@ -588,7 +578,7 @@ static int test14(const void *unused G_GNUC_UNUSED)
 goto cleanup;
 }
 
-ret = checkoutput("test14", NULL);
+ret = checkoutput("test14");
 
  cleanup:
 virCommandFree(cmd);
@@ -618,7 +608,7 @@ static int test15(const void *unused G_GNUC_UNUSED)
 goto cleanup;
 }
 
-ret = checkoutput("test15", NULL);
+ret = checkoutput("test15");
 
  cleanup:
 VIR_FREE(cwd);
@@ -663,7 +653,7 @@ static int test16(const void *unused G_GNUC_UNUSED)
 goto cleanup;
 }
 
-ret = checkoutput("test16", NULL);
+ret = checkoutput("test16");
 
  cleanup:
 virCommandFree(cmd);
@@ -836,7 +826,7 @@ static int test20(const void *unused G_GNUC_UNUSED)
 goto cleanup;
 }
 
-ret = checkoutput("test20", NULL);
+ret = checkoutput("test20");
  cleanup:
 virCommandFree(cmd);
 VIR_FREE(buf);
@@ -894,7 +884,7 @@ static int test21(const void *unused G_GNUC_UNUSED)
 goto cleanup;
 }
 
-ret = checkoutput("test21", NULL);
+ret = checkoutput("test21");
  cleanup:
 VIR_FREE(outbuf);
 VIR_FREE(errbuf);
@@ -1136,7 +1126,7 @@ static int test26(const void

[libvirt PATCH 0/5] tests: commandtest: use g_auto more

Ján Tomko (5):
  tests: commandtest: remove unused 'prefix' parameter
  tests: commandtest: use g_autofree
  tests: commandtest: use g_autoptr for virCommand
  tests: commandtest: use VIR_AUTOCLOSE
  tests: commandtest: drop unnecessary labels

 tests/commandtest.c | 327 ++--
 1 file changed, 105 insertions(+), 222 deletions(-)

-- 
2.26.2

[libvirt PATCH 3/5] tests: commandtest: use g_autoptr for virCommand

Except for a few cases where freeing it explicitly
seems to be done on purpose.

Signed-off-by: Ján Tomko 
---
 tests/commandtest.c | 94 -
 1 file changed, 25 insertions(+), 69 deletions(-)

diff --git a/tests/commandtest.c b/tests/commandtest.c
index 7c6c3ec75d..a54e467b2b 100644
--- a/tests/commandtest.c
+++ b/tests/commandtest.c
@@ -96,7 +96,7 @@ static int checkoutput(const char *testname)
  */
 static int test0(const void *unused G_GNUC_UNUSED)
 {
-virCommandPtr cmd;
+g_autoptr(virCommand) cmd = NULL;
 int ret = -1;
 
 cmd = virCommandNew(abs_builddir "/commandhelper-doesnotexist");
@@ -110,7 +110,6 @@ static int test0(const void *unused G_GNUC_UNUSED)
 ret = 0;
 
  cleanup:
-virCommandFree(cmd);
 return ret;
 }
 
@@ -121,7 +120,7 @@ static int test0(const void *unused G_GNUC_UNUSED)
  */
 static int test1(const void *unused G_GNUC_UNUSED)
 {
-virCommandPtr cmd;
+g_autoptr(virCommand) cmd = NULL;
 int ret = -1;
 int status;
 
@@ -139,7 +138,6 @@ static int test1(const void *unused G_GNUC_UNUSED)
 ret = 0;
 
  cleanup:
-virCommandFree(cmd);
 return ret;
 }
 
@@ -149,28 +147,22 @@ static int test1(const void *unused G_GNUC_UNUSED)
  */
 static int test2(const void *unused G_GNUC_UNUSED)
 {
-virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper");
+g_autoptr(virCommand) cmd = virCommandNew(abs_builddir "/commandhelper");
 int ret;
 
 if (virCommandRun(cmd, NULL) < 0) {
 printf("Cannot run child %s\n", virGetLastErrorMessage());
-virCommandFree(cmd);
 return -1;
 }
 
-if ((ret = checkoutput("test2")) != 0) {
-virCommandFree(cmd);
+if ((ret = checkoutput("test2")) != 0)
 return ret;
-}
 
 if (virCommandRun(cmd, NULL) < 0) {
 printf("Cannot run child %s\n", virGetLastErrorMessage());
-virCommandFree(cmd);
 return -1;
 }
 
-virCommandFree(cmd);
-
 return checkoutput("test2");
 }
 
@@ -180,7 +172,7 @@ static int test2(const void *unused G_GNUC_UNUSED)
  */
 static int test3(const void *unused G_GNUC_UNUSED)
 {
-virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper");
+g_autoptr(virCommand) cmd = virCommandNew(abs_builddir "/commandhelper");
 int newfd1 = dup(STDERR_FILENO);
 int newfd2 = dup(STDERR_FILENO);
 int newfd3 = dup(STDERR_FILENO);
@@ -226,7 +218,6 @@ static int test3(const void *unused G_GNUC_UNUSED)
 ret = checkoutput("test3");
 
  cleanup:
-virCommandFree(cmd);
 /* coverity[double_close] */
 VIR_FORCE_CLOSE(newfd1);
 VIR_FORCE_CLOSE(newfd2);
@@ -241,8 +232,8 @@ static int test3(const void *unused G_GNUC_UNUSED)
  */
 static int test4(const void *unused G_GNUC_UNUSED)
 {
-virCommandPtr cmd = virCommandNewArgList(abs_builddir "/commandhelper",
- "--check-daemonize", NULL);
+g_autoptr(virCommand) cmd = virCommandNewArgList(abs_builddir 
"/commandhelper",
+ "--check-daemonize", 
NULL);
 g_autofree char *pidfile = virPidFileBuildPath(abs_builddir, 
"commandhelper");
 pid_t pid;
 int ret = -1;
@@ -268,7 +259,6 @@ static int test4(const void *unused G_GNUC_UNUSED)
 ret = checkoutput("test4");
 
  cleanup:
-virCommandFree(cmd);
 if (pidfile)
 unlink(pidfile);
 return ret;
@@ -281,18 +271,15 @@ static int test4(const void *unused G_GNUC_UNUSED)
  */
 static int test5(const void *unused G_GNUC_UNUSED)
 {
-virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper");
+g_autoptr(virCommand) cmd = virCommandNew(abs_builddir "/commandhelper");
 
 virCommandAddEnvPassCommon(cmd);
 
 if (virCommandRun(cmd, NULL) < 0) {
 printf("Cannot run child %s\n", virGetLastErrorMessage());
-virCommandFree(cmd);
 return -1;
 }
 
-virCommandFree(cmd);
-
 return checkoutput("test5");
 }
 
@@ -303,19 +290,16 @@ static int test5(const void *unused G_GNUC_UNUSED)
  */
 static int test6(const void *unused G_GNUC_UNUSED)
 {
-virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper");
+g_autoptr(virCommand) cmd = virCommandNew(abs_builddir "/commandhelper");
 
 virCommandAddEnvPass(cmd, "DISPLAY");
 virCommandAddEnvPass(cmd, "DOESNOTEXIST");
 
 if (virCommandRun(cmd, NULL) < 0) {
 printf("Cannot run child %s\n", virGetLastErrorMessage());
-virCommandFree(cmd);
 return -1;
 }
 
-virCommandFree(cmd);
-
 return checkoutput("test6");
 }
 
@@ -326,7 +310,7 @@ static int test6(const void *unused G_GNUC_UNUSED)
  */
 static int test7(const void *unused G_GNUC_UNUSED)
 {
-virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper");
+g_autoptr(virCommand) cmd = virCommandNew(abs_builddir "/commandhelper");
 
 virCommandAddEnvPassCommon(cmd);
 virCommandAddEnvPass(cmd, "DISPLAY");
@@ -334,12 +318,9 @@

[libvirt PATCH 0/4] ci: run clang on Linux and parallelize more

I found out we do not run tests compiled with my preferred compiler [0]:
https://www.redhat.com/archives/libvir-list/2020-July/msg01280.html

Run it on Fedora 31 and Rawhide, to get some variety in the coverage.

Corresponding libvirt-ci change:
https://gitlab.com/libvirt/libvirt-ci/-/merge_requests/38

[0] although I haven't benchmarked the build time recently

https://gitlab.com/jano.tomko/libvirt/-/pipelines/171846516

Ján Tomko (4):
  ci: refresh Dockerfiles
  ci: add a job for clang
  ci: run Cirrus-based builds sooner
  ci: use 'needs' more often

 .gitlab-ci.yml| 29 ++-
 ci/containers/libvirt-centos-7.Dockerfile |  1 -
 ci/containers/libvirt-centos-8.Dockerfile |  1 -
 .../libvirt-centos-stream.Dockerfile  |  1 -
 ...libvirt-debian-10-cross-aarch64.Dockerfile |  1 -
 .../libvirt-debian-10-cross-armv6l.Dockerfile |  2 +-
 .../libvirt-debian-10-cross-armv7l.Dockerfile |  2 +-
 .../libvirt-debian-10-cross-i686.Dockerfile   |  1 -
 .../libvirt-debian-10-cross-mips.Dockerfile   |  1 -
 ...ibvirt-debian-10-cross-mips64el.Dockerfile |  1 -
 .../libvirt-debian-10-cross-mipsel.Dockerfile |  1 -
 ...libvirt-debian-10-cross-ppc64le.Dockerfile |  1 -
 .../libvirt-debian-10-cross-s390x.Dockerfile  |  1 -
 ci/containers/libvirt-debian-10.Dockerfile|  1 -
 ...ibvirt-debian-sid-cross-aarch64.Dockerfile |  1 -
 ...libvirt-debian-sid-cross-armv6l.Dockerfile |  2 +-
 ...libvirt-debian-sid-cross-armv7l.Dockerfile |  2 +-
 .../libvirt-debian-sid-cross-i686.Dockerfile  |  1 -
 ...bvirt-debian-sid-cross-mips64el.Dockerfile |  1 -
 ...libvirt-debian-sid-cross-mipsel.Dockerfile |  2 +-
 ...ibvirt-debian-sid-cross-ppc64le.Dockerfile |  1 -
 .../libvirt-debian-sid-cross-s390x.Dockerfile |  1 -
 ci/containers/libvirt-debian-sid.Dockerfile   |  1 -
 ci/containers/libvirt-fedora-31.Dockerfile|  2 +-
 ci/containers/libvirt-fedora-32.Dockerfile|  2 +-
 .../libvirt-fedora-rawhide.Dockerfile |  2 +-
 ci/containers/libvirt-opensuse-151.Dockerfile |  1 -
 ci/containers/libvirt-ubuntu-1804.Dockerfile  |  1 -
 ci/containers/libvirt-ubuntu-2004.Dockerfile  |  1 -
 29 files changed, 36 insertions(+), 29 deletions(-)

-- 
2.26.2

[libvirt PATCH 1/4] ci: refresh Dockerfiles

We dropped ncurses and added libnuma-dev on all debian archs
as of libvirt-ci commit:

commit 4717287565d81b747d3176332108fe0302de2669
projects: libvirt doesn't depend on ncurses

Signed-off-by: Ján Tomko 
---
 ci/containers/libvirt-centos-7.Dockerfile  | 1 -
 ci/containers/libvirt-centos-8.Dockerfile  | 1 -
 ci/containers/libvirt-centos-stream.Dockerfile | 1 -
 ci/containers/libvirt-debian-10-cross-aarch64.Dockerfile   | 1 -
 ci/containers/libvirt-debian-10-cross-armv6l.Dockerfile| 2 +-
 ci/containers/libvirt-debian-10-cross-armv7l.Dockerfile| 2 +-
 ci/containers/libvirt-debian-10-cross-i686.Dockerfile  | 1 -
 ci/containers/libvirt-debian-10-cross-mips.Dockerfile  | 1 -
 ci/containers/libvirt-debian-10-cross-mips64el.Dockerfile  | 1 -
 ci/containers/libvirt-debian-10-cross-mipsel.Dockerfile| 1 -
 ci/containers/libvirt-debian-10-cross-ppc64le.Dockerfile   | 1 -
 ci/containers/libvirt-debian-10-cross-s390x.Dockerfile | 1 -
 ci/containers/libvirt-debian-10.Dockerfile | 1 -
 ci/containers/libvirt-debian-sid-cross-aarch64.Dockerfile  | 1 -
 ci/containers/libvirt-debian-sid-cross-armv6l.Dockerfile   | 2 +-
 ci/containers/libvirt-debian-sid-cross-armv7l.Dockerfile   | 2 +-
 ci/containers/libvirt-debian-sid-cross-i686.Dockerfile | 1 -
 ci/containers/libvirt-debian-sid-cross-mips64el.Dockerfile | 1 -
 ci/containers/libvirt-debian-sid-cross-mipsel.Dockerfile   | 2 +-
 ci/containers/libvirt-debian-sid-cross-ppc64le.Dockerfile  | 1 -
 ci/containers/libvirt-debian-sid-cross-s390x.Dockerfile| 1 -
 ci/containers/libvirt-debian-sid.Dockerfile| 1 -
 ci/containers/libvirt-fedora-31.Dockerfile | 1 -
 ci/containers/libvirt-fedora-32.Dockerfile | 1 -
 ci/containers/libvirt-fedora-rawhide.Dockerfile| 1 -
 ci/containers/libvirt-opensuse-151.Dockerfile  | 1 -
 ci/containers/libvirt-ubuntu-1804.Dockerfile   | 1 -
 ci/containers/libvirt-ubuntu-2004.Dockerfile   | 1 -
 28 files changed, 5 insertions(+), 28 deletions(-)

diff --git a/ci/containers/libvirt-centos-7.Dockerfile 
b/ci/containers/libvirt-centos-7.Dockerfile
index 08d9386644..0054587542 100644
--- a/ci/containers/libvirt-centos-7.Dockerfile
+++ b/ci/containers/libvirt-centos-7.Dockerfile
@@ -87,7 +87,6 @@ WEiJKtQrZDJloqtyi/mmRa1VsV7RYR0VPJjhK/R8EQ7Ysshy\n\
 lsof \
 lvm2 \
 make \
-ncurses-devel \
 net-tools \
 netcf-devel \
 nfs-utils \
diff --git a/ci/containers/libvirt-centos-8.Dockerfile 
b/ci/containers/libvirt-centos-8.Dockerfile
index 2ac825fc80..b5ce6fa2db 100644
--- a/ci/containers/libvirt-centos-8.Dockerfile
+++ b/ci/containers/libvirt-centos-8.Dockerfile
@@ -60,7 +60,6 @@ RUN dnf install 'dnf-command(config-manager)' -y && \
 lvm2 \
 make \
 meson \
-ncurses-devel \
 net-tools \
 netcf-devel \
 nfs-utils \
diff --git a/ci/containers/libvirt-centos-stream.Dockerfile 
b/ci/containers/libvirt-centos-stream.Dockerfile
index e0025e2acb..8f8e089a25 100644
--- a/ci/containers/libvirt-centos-stream.Dockerfile
+++ b/ci/containers/libvirt-centos-stream.Dockerfile
@@ -61,7 +61,6 @@ RUN dnf install -y centos-release-stream && \
 lvm2 \
 make \
 meson \
-ncurses-devel \
 net-tools \
 netcf-devel \
 nfs-utils \
diff --git a/ci/containers/libvirt-debian-10-cross-aarch64.Dockerfile 
b/ci/containers/libvirt-debian-10-cross-aarch64.Dockerfile
index 2d49f5e6e7..3013c4316d 100644
--- a/ci/containers/libvirt-debian-10-cross-aarch64.Dockerfile
+++ b/ci/containers/libvirt-debian-10-cross-aarch64.Dockerfile
@@ -90,7 +90,6 @@ RUN export DEBIAN_FRONTEND=noninteractive && \
 libglusterfs-dev:arm64 \
 libgnutls28-dev:arm64 \
 libiscsi-dev:arm64 \
-libncurses-dev:arm64 \
 libnl-3-dev:arm64 \
 libnl-route-3-dev:arm64 \
 libnuma-dev:arm64 \
diff --git a/ci/containers/libvirt-debian-10-cross-armv6l.Dockerfile 
b/ci/containers/libvirt-debian-10-cross-armv6l.Dockerfile
index f9d6ee4c9f..adc3ff345c 100644
--- a/ci/containers/libvirt-debian-10-cross-armv6l.Dockerfile
+++ b/ci/containers/libvirt-debian-10-cross-armv6l.Dockerfile
@@ -90,9 +90,9 @@ RUN export DEBIAN_FRONTEND=noninteractive && \
 libglusterfs-dev:armel \
 libgnutls28-dev:armel \
 libiscsi-dev:armel \
-libncurses-dev:armel \
 libnl-3-dev:armel \
 libnl-route-3-dev:armel \
+libnuma-dev:armel \
 libparted-dev:armel \
 libpcap0.8-dev:armel \
 libpciaccess-dev:armel \
diff --git a/ci/containers/libvirt-debian-10-cross-armv7l.Dockerfile 
b/ci/containers/libvirt-debian-10-cross-armv7l.Dockerfile
index f26e059535..f1fa703fef 100644
---

[libvirt PATCH 2/4] ci: add a job for clang

Out of the Linux distros we build on in the CI, clang
is only available on Fedora.

Add a job to Fedora 31 and Rawhide, to have coverage
for clang on Linux as well.

Includes a refresh of the Dockerfiles to commit TBD:
https://gitlab.com/libvirt/libvirt-ci/-/merge_requests/38

Signed-off-by: Ján Tomko 
---
 .gitlab-ci.yml  | 16 +++-
 ci/containers/libvirt-fedora-31.Dockerfile  |  1 +
 ci/containers/libvirt-fedora-32.Dockerfile  |  1 +
 ci/containers/libvirt-fedora-rawhide.Dockerfile |  1 +
 4 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 702198ec8e..f61ad7151c 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -52,7 +52,7 @@ stages:
   script:
 - mkdir build
 - cd build
-- ../autogen.sh || (cat config.log && exit 1)
+- ../autogen.sh CC=$CC || (cat config.log && exit 1)
 - $MAKE distcheck
 
 # Jobs that we delegate to Cirrus CI because they require an operating
@@ -306,6 +306,20 @@ x64-fedora-rawhide:
   variables:
 NAME: fedora-rawhide
 
+x64-fedora-31-clang:
+  <<: *native_build_job_definition
+  needs: ["x64-fedora-31-container"]
+  variables:
+CC: clang
+NAME: fedora-31
+
+x64-fedora-rawhide-clang:
+  <<: *native_build_job_definition
+  needs: ["x64-fedora-rawhide-container"]
+  variables:
+CC: clang
+NAME: fedora-rawhide
+
 x64-opensuse-151:
   <<: *native_build_job_definition
   variables:
diff --git a/ci/containers/libvirt-fedora-31.Dockerfile 
b/ci/containers/libvirt-fedora-31.Dockerfile
index 72e6ae3b69..84b19e7330 100644
--- a/ci/containers/libvirt-fedora-31.Dockerfile
+++ b/ci/containers/libvirt-fedora-31.Dockerfile
@@ -12,6 +12,7 @@ RUN dnf update -y && \
 ca-certificates \
 ccache \
 chrony \
+clang \
 cppi \
 cyrus-sasl-devel \
 dbus-devel \
diff --git a/ci/containers/libvirt-fedora-32.Dockerfile 
b/ci/containers/libvirt-fedora-32.Dockerfile
index ea38a7f084..bbd7ff87a4 100644
--- a/ci/containers/libvirt-fedora-32.Dockerfile
+++ b/ci/containers/libvirt-fedora-32.Dockerfile
@@ -12,6 +12,7 @@ RUN dnf update -y && \
 ca-certificates \
 ccache \
 chrony \
+clang \
 cppi \
 cyrus-sasl-devel \
 dbus-devel \
diff --git a/ci/containers/libvirt-fedora-rawhide.Dockerfile 
b/ci/containers/libvirt-fedora-rawhide.Dockerfile
index 6da9bc8f69..d362662f35 100644
--- a/ci/containers/libvirt-fedora-rawhide.Dockerfile
+++ b/ci/containers/libvirt-fedora-rawhide.Dockerfile
@@ -13,6 +13,7 @@ RUN dnf update -y --nogpgcheck fedora-gpg-keys && \
 ca-certificates \
 ccache \
 chrony \
+clang \
 cppi \
 cyrus-sasl-devel \
 dbus-devel \
-- 
2.26.2

[libvirt PATCH 3/4] ci: run Cirrus-based builds sooner

Jobs using cirrus-run don't need to wait for any jobs from
the container stage. Run them as soon as possible.

Signed-off-by: Ján Tomko 
---
 .gitlab-ci.yml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index f61ad7151c..077465e436 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -337,6 +337,7 @@ x64-ubuntu-2004:
 
 x64-freebsd-12-build:
   <<: *cirrus_build_job_definition
+  needs: []
   variables:
 NAME: freebsd-12
 CIRRUS_VM_INSTANCE_TYPE: freebsd_instance
@@ -346,6 +347,7 @@ x64-freebsd-12-build:
 
 x64-macos-1015-build:
   <<: *cirrus_build_job_definition
+  needs: []
   variables:
 NAME: macos-1015
 CIRRUS_VM_INSTANCE_TYPE: osx_instance
-- 
2.26.2

[libvirt PATCH 4/4] ci: use 'needs' more often

Make the pipeline chart more interesting.

Signed-off-by: Ján Tomko 
---
 .gitlab-ci.yml | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 077465e436..7d23ddfdf9 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -268,41 +268,49 @@ mingw64-fedora-rawhide-container:
 
 x64-debian-10:
   <<: *native_build_job_definition
+  needs: ["x64-debian-10-container"]
   variables:
 NAME: debian-10
 
 x64-debian-sid:
   <<: *native_build_job_definition
+  needs: ["x64-debian-sid-container"]
   variables:
 NAME: debian-sid
 
 x64-centos-7:
   <<: *native_build_job_definition
+  needs: ["x64-centos-7-container"]
   variables:
 NAME: centos-7
 
 x64-centos-8:
   <<: *native_build_job_definition
+  needs: ["x64-centos-8-container"]
   variables:
 NAME: centos-8
 
 x64-centos-stream:
   <<: *native_build_job_definition
+  needs: ["x64-centos-stream-container"]
   variables:
 NAME: centos-stream
 
 x64-fedora-31:
   <<: *native_build_job_definition
+  needs: ["x64-fedora-31-container"]
   variables:
 NAME: fedora-31
 
 x64-fedora-32:
   <<: *native_build_job_definition
+  needs: ["x64-fedora-32-container"]
   variables:
 NAME: fedora-32
 
 x64-fedora-rawhide:
   <<: *native_build_job_definition
+  needs: ["x64-fedora-rawhide-container"]
   variables:
 NAME: fedora-rawhide
 
@@ -322,6 +330,7 @@ x64-fedora-rawhide-clang:
 
 x64-opensuse-151:
   <<: *native_build_job_definition
+  needs: ["x64-opensuse-151-container"]
   variables:
 NAME: opensuse-151
 
@@ -433,6 +442,7 @@ mingw64-fedora-rawhide:
 website:
   stage: builds
   image: $CI_REGISTRY_IMAGE/ci-centos-8:latest
+  needs: ["x64-centos-8-container"]
   before_script:
 - *script_variables
   script:
@@ -455,6 +465,7 @@ website:
 codestyle:
   stage: builds
   image: $CI_REGISTRY_IMAGE/ci-opensuse-151:latest
+  needs: ["x64-opensuse-151-container"]
   before_script:
 - *script_variables
   script:
-- 
2.26.2

Re: [RFC] Dynamic creation of VFs in a network definition containing an SRIOV device

2020-07-28 Thread Daniel Henrique Barboza

On 7/28/20 12:03 PM, Paulo de Rezende Pinatti wrote:

Context:

Libvirt can already detect the active VFs of an SRIOV PF device specified in a
network definition and automatically assign these VFs to guests via an
entry referring to that network in the domain definition. This
functionality, however, depends on the system administrator having activated in
advance the desired number of VFs outside of libvirt (either manually or through
system scripts).

It would be more convenient if the VFs activation could also be managed inside
libvirt so that the whole management of the VF pool is done exclusively by
libvirt and in only one place (the network definition) rather than spread in
different components of the system.

Proposal:

We can extend the existing network definition by adding a new tag as a child of
the tag in order to allow the user to specify how many VFs they wish to have
activated for the corresponding SRIOV device when the network is started. That would look
like the following:

sriov-pool

At xml definition time nothing gets changed on the system, as it is today. When the
network is started with 'virth net-start sriov-pool' then libvirt will activate the
desired number of VFs as specified in the tag of the network definition.

The operation might require resetting 'sriov_numvfs' to zero first in case the
number of VFs currently active differs from the desired value. In order to
avoid the situation where the user tries to start the network when a VF is
already assigned to a running guest, the implementation will have to ensure all
existing VFs of the target PF are not in use, otherwise VFs would be
inadvertently hot-unplugged from guests upon network start. In such cases,
trying to start the network will then result in an error.

I'm not sure about the "echo 0 > sriov_numvfs' part. It works like that for
Mellanox
CX-4 and CX-5 cards but I can't say it works like that for every other SR-IOV
card out
there. Sooner enough, we'll have to handle specific behavior for the cards to
create
the VFs. Perhaps Laine can comment on this.

About the whole idea, it kind of changes the design of this network pool. As it
is today,
at least from my reading of [1], Libvirt will use any available VF from the
pool and allocate it
to the guest, coping with the existing host VF settings. Using this new option,
Libvirt is now
setting the VFs to a specific number, which might as well be less than the
actual setting,
disrupting the host for no apparent reason.

I would be on board with this idea if:

1 - The attribute is changed to "minimal VFs required for this pool" rather than
"change the host
to match this VF number". This means that we wouldn't tamper with the created
VFs if the host
already has more VFs that specified. In your example up there, setting 10 VFs,
what if the host
has 20 VFs? Why should Libvirt care about taking down 10 VFs that it wouldn't
use in the
first place?

2 - we find a universal way (or as much closer as universal) to handle the
creation of VFs.

3 - we guarantee that the process of VF creation, which will take down all
existing VFs in
case of CX-5 cards with echo 0 > numvfs for example, wouldn't disrupt the host
in any
way.

(1) is an easier sell. Rename the attribute to "vf minimalNum" or something
like that, then
refuse to net-start if the host has less than the set amount of VFs checking
sriov_numvfs.
Start the network if sriov_numvfs >= minimal. This would bring immediate value
to the existing
design, allowing the user to specify the minimal amount of VFs the user intends
to
consume from the pool.

(2) and (3) are more complicated. Specially (2).

Thanks,

DHB

[1]
https://wiki.libvirt.org/page/Networking#Assignment_from_a_pool_of_SRIOV_VFs_in_a_libvirt_.3Cnetwork.3E_definition

Stopping the network with 'virsh net-destroy' will cause all VFs to be removed.
Similarly to when starting the network, the implementation will also need to
verify for running guests in order to prevent inadvertent hot-unplugging.

Is the functionality proposed above desirable?

Re: [libvirt PATCH 312/351] meson: tests: add file access test setup

On Tue, Jul 28, 2020 at 09:32:57PM +0200, Pavel Hrdina wrote:
> On Tue, Jul 28, 2020 at 09:13:41PM +0200, Peter Krempa wrote:
> > On Tue, Jul 28, 2020 at 18:48:18 +0200, Pavel Hrdina wrote:
> > > On Tue, Jul 28, 2020 at 03:39:41PM +0200, Pavel Hrdina wrote:
> > > > On Tue, Jul 28, 2020 at 03:19:25PM +0200, Pino Toscano wrote:
> > > > > On Tuesday, 28 July 2020 14:56:45 CEST Peter Krempa wrote:
> > > > > > On Thu, Jul 16, 2020 at 11:59:08 +0200, Pavel Hrdina wrote:
> > > > > > > We need to modify check-file-access.py to be usable as wrapper for
> > > > > > > libvirt tests. This way we can run the tests using this command:
> > > > > > > 
> > > > > > > meson test --setup access
> > > > > > > 
> > > > > > > which will run all tests using check-file-access.py as a wrapper.
> > > > > > > 
> > > > > > > With autotools all file access are written into single file for 
> > > > > > > all
> > > > > > > tests and compared once the whole test suite is done.
> > > > > > > 
> > > > > > > With Meson we will compare the file access after every single test
> > > > > > > because it is used as wrapper now. That requires writing the file
> > > > > > > access into separate files for every single test as they are 
> > > > > > > executed
> > > > > > > in parallel.
> > > > > > > 
> > > > > > > Since the wrapper is used for all tests in Meson including tests 
> > > > > > > outside
> > > > > > > of tests directory we have to check for presence of the output 
> > > > > > > file.
> > > > > > > We should also cleanup after ourselves.
> > > > > > > 
> > > > > > > Signed-off-by: Pavel Hrdina 
> > > > > > > ---
> > > > > > >  Makefile.am  |  3 ---
> > > > > > >  scripts/check-file-access.py | 24 +++-
> > > > > > >  tests/Makefile.am| 12 
> > > > > > >  tests/meson.build|  8 
> > > > > > >  tests/virtestmock.c  |  2 +-
> > > > > > >  5 files changed, 28 insertions(+), 21 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/Makefile.am b/Makefile.am
> > > > > > > index 363c5cf66fd..d05a0c1a85a 100644
> > > > > > > --- a/Makefile.am
> > > > > > > +++ b/Makefile.am
> > > > > > > @@ -37,9 +37,6 @@ srpm: clean
> > > > > > >  
> > > > > > >  check-local: all tests
> > > > > > >  
> > > > > > > -check-access: all
> > > > > > > - @($(MAKE) $(AM_MAKEFLAGS) -C tests check-access)
> > > > > > > -
> > > > > > >  dist-hook: gen-AUTHORS
> > > > > > >  
> > > > > > >  .PHONY: gen-AUTHORS
> > > > > > > diff --git a/scripts/check-file-access.py 
> > > > > > > b/scripts/check-file-access.py
> > > > > > > index aa120cafacf..f0e98f4b652 100755
> > > > > > > --- a/scripts/check-file-access.py
> > > > > > > +++ b/scripts/check-file-access.py
> > > > > > > @@ -21,15 +21,27 @@
> > > > > > >  #
> > > > > > >  #
> > > > > > >  
> > > > > > > +import os
> > > > > > > +import random
> > > > > > >  import re
> > > > > > > +import string
> > > > > > >  import sys
> > > > > > >  
> > > > > > > -if len(sys.argv) != 3:
> > > > > > > -print("syntax: %s ACCESS-FILE PERMITTED-ACCESS-FILE")
> > > > > > > -sys.exit(1)
> > > > > > > +abs_builddir = os.environ.get('abs_builddir', '')
> > > > > > > +abs_srcdir = os.environ.get('abs_srcdir', '')
> > > > > > >  
> > > > > > > -access_file = sys.argv[1]
> > > > > > > -permitted_file = sys.argv[2]
> > > > > > > +filename = ''.join(random.choice(string.ascii_letters) for _ in 
> > > > > > > range(16))
> > > > > > 
> > > > > > Umm, python doesn't have a 'mkostemp' equivalent? This seems a bit
> > > > > > fishy.
> > > > > 
> > > > > Sure, it is the tempfile module:
> > > > > https://docs.python.org/3/library/tempfile.html
> > > > > 
> > > > >   filename = tempfile.mkstemp(dir=abs_builddir, 
> > > > > prefix='file-access-', suffix='.txt')
> > > > 
> > > > I'll look into it. When porting it I just looked for any solution as
> > > > this can be changed any time after the series is pushed.
> > > 
> > > So I looked into it and the python script doesn't create the temporary
> > > file. The file is created by virtestmock.c if we run our test suite with
> > > file access check. The existence of the file is also used to check if
> > > there is something to be compared as not all of the tests needs to be
> > > check if they access some system files, for example all the check-* test
> > > cases.
> > > 
> > > The tempfile is a nice module but useless in this case where we care
> > > about only getting a temporary file name. In order to use the module
> > > we would have to do something like this:
> > > 
> > > 
> > > fd, filename = tempfile.mkstemp(dir=abs_builddir, prefix='file-access-', 
> > > suffix='.txt')
> > > os.close(fd)
> > > os.unlink()
> > > 
> > > 
> > > which looks ugly. So I would rather keep it as it is.
> > 
> > Your algorithm is not robust enough so it could end up causing random
> > test failures. Unlikely but possible.
> > 
> > You can pass the filename to the test and let the mock append to the
> > existing file and then read

Re: [libvirt PATCH 312/351] meson: tests: add file access test setup

On Tue, Jul 28, 2020 at 09:13:41PM +0200, Peter Krempa wrote:
> On Tue, Jul 28, 2020 at 18:48:18 +0200, Pavel Hrdina wrote:
> > On Tue, Jul 28, 2020 at 03:39:41PM +0200, Pavel Hrdina wrote:
> > > On Tue, Jul 28, 2020 at 03:19:25PM +0200, Pino Toscano wrote:
> > > > On Tuesday, 28 July 2020 14:56:45 CEST Peter Krempa wrote:
> > > > > On Thu, Jul 16, 2020 at 11:59:08 +0200, Pavel Hrdina wrote:
> > > > > > We need to modify check-file-access.py to be usable as wrapper for
> > > > > > libvirt tests. This way we can run the tests using this command:
> > > > > > 
> > > > > > meson test --setup access
> > > > > > 
> > > > > > which will run all tests using check-file-access.py as a wrapper.
> > > > > > 
> > > > > > With autotools all file access are written into single file for all
> > > > > > tests and compared once the whole test suite is done.
> > > > > > 
> > > > > > With Meson we will compare the file access after every single test
> > > > > > because it is used as wrapper now. That requires writing the file
> > > > > > access into separate files for every single test as they are 
> > > > > > executed
> > > > > > in parallel.
> > > > > > 
> > > > > > Since the wrapper is used for all tests in Meson including tests 
> > > > > > outside
> > > > > > of tests directory we have to check for presence of the output file.
> > > > > > We should also cleanup after ourselves.
> > > > > > 
> > > > > > Signed-off-by: Pavel Hrdina 
> > > > > > ---
> > > > > >  Makefile.am  |  3 ---
> > > > > >  scripts/check-file-access.py | 24 +++-
> > > > > >  tests/Makefile.am| 12 
> > > > > >  tests/meson.build|  8 
> > > > > >  tests/virtestmock.c  |  2 +-
> > > > > >  5 files changed, 28 insertions(+), 21 deletions(-)
> > > > > > 
> > > > > > diff --git a/Makefile.am b/Makefile.am
> > > > > > index 363c5cf66fd..d05a0c1a85a 100644
> > > > > > --- a/Makefile.am
> > > > > > +++ b/Makefile.am
> > > > > > @@ -37,9 +37,6 @@ srpm: clean
> > > > > >  
> > > > > >  check-local: all tests
> > > > > >  
> > > > > > -check-access: all
> > > > > > -   @($(MAKE) $(AM_MAKEFLAGS) -C tests check-access)
> > > > > > -
> > > > > >  dist-hook: gen-AUTHORS
> > > > > >  
> > > > > >  .PHONY: gen-AUTHORS
> > > > > > diff --git a/scripts/check-file-access.py 
> > > > > > b/scripts/check-file-access.py
> > > > > > index aa120cafacf..f0e98f4b652 100755
> > > > > > --- a/scripts/check-file-access.py
> > > > > > +++ b/scripts/check-file-access.py
> > > > > > @@ -21,15 +21,27 @@
> > > > > >  #
> > > > > >  #
> > > > > >  
> > > > > > +import os
> > > > > > +import random
> > > > > >  import re
> > > > > > +import string
> > > > > >  import sys
> > > > > >  
> > > > > > -if len(sys.argv) != 3:
> > > > > > -print("syntax: %s ACCESS-FILE PERMITTED-ACCESS-FILE")
> > > > > > -sys.exit(1)
> > > > > > +abs_builddir = os.environ.get('abs_builddir', '')
> > > > > > +abs_srcdir = os.environ.get('abs_srcdir', '')
> > > > > >  
> > > > > > -access_file = sys.argv[1]
> > > > > > -permitted_file = sys.argv[2]
> > > > > > +filename = ''.join(random.choice(string.ascii_letters) for _ in 
> > > > > > range(16))
> > > > > 
> > > > > Umm, python doesn't have a 'mkostemp' equivalent? This seems a bit
> > > > > fishy.
> > > > 
> > > > Sure, it is the tempfile module:
> > > > https://docs.python.org/3/library/tempfile.html
> > > > 
> > > >   filename = tempfile.mkstemp(dir=abs_builddir, prefix='file-access-', 
> > > > suffix='.txt')
> > > 
> > > I'll look into it. When porting it I just looked for any solution as
> > > this can be changed any time after the series is pushed.
> > 
> > So I looked into it and the python script doesn't create the temporary
> > file. The file is created by virtestmock.c if we run our test suite with
> > file access check. The existence of the file is also used to check if
> > there is something to be compared as not all of the tests needs to be
> > check if they access some system files, for example all the check-* test
> > cases.
> > 
> > The tempfile is a nice module but useless in this case where we care
> > about only getting a temporary file name. In order to use the module
> > we would have to do something like this:
> > 
> > 
> > fd, filename = tempfile.mkstemp(dir=abs_builddir, prefix='file-access-', 
> > suffix='.txt')
> > os.close(fd)
> > os.unlink()
> > 
> > 
> > which looks ugly. So I would rather keep it as it is.
> 
> Your algorithm is not robust enough so it could end up causing random
> test failures. Unlikely but possible.
> 
> You can pass the filename to the test and let the mock append to the
> existing file and then read it.

You are missing the point that the presence of the file is used later in
the script:

if ret != 0 or not os.path.exists(access_file):
sys.exit(ret)

the whole point is that only if the file exists the script will do the
actual check and compare the file-access-***.txt file.

If the file doesn't exist the

Re: [libvirt PATCH 312/351] meson: tests: add file access test setup

On Tue, Jul 28, 2020 at 18:48:18 +0200, Pavel Hrdina wrote:
> On Tue, Jul 28, 2020 at 03:39:41PM +0200, Pavel Hrdina wrote:
> > On Tue, Jul 28, 2020 at 03:19:25PM +0200, Pino Toscano wrote:
> > > On Tuesday, 28 July 2020 14:56:45 CEST Peter Krempa wrote:
> > > > On Thu, Jul 16, 2020 at 11:59:08 +0200, Pavel Hrdina wrote:
> > > > > We need to modify check-file-access.py to be usable as wrapper for
> > > > > libvirt tests. This way we can run the tests using this command:
> > > > > 
> > > > > meson test --setup access
> > > > > 
> > > > > which will run all tests using check-file-access.py as a wrapper.
> > > > > 
> > > > > With autotools all file access are written into single file for all
> > > > > tests and compared once the whole test suite is done.
> > > > > 
> > > > > With Meson we will compare the file access after every single test
> > > > > because it is used as wrapper now. That requires writing the file
> > > > > access into separate files for every single test as they are executed
> > > > > in parallel.
> > > > > 
> > > > > Since the wrapper is used for all tests in Meson including tests 
> > > > > outside
> > > > > of tests directory we have to check for presence of the output file.
> > > > > We should also cleanup after ourselves.
> > > > > 
> > > > > Signed-off-by: Pavel Hrdina 
> > > > > ---
> > > > >  Makefile.am  |  3 ---
> > > > >  scripts/check-file-access.py | 24 +++-
> > > > >  tests/Makefile.am| 12 
> > > > >  tests/meson.build|  8 
> > > > >  tests/virtestmock.c  |  2 +-
> > > > >  5 files changed, 28 insertions(+), 21 deletions(-)
> > > > > 
> > > > > diff --git a/Makefile.am b/Makefile.am
> > > > > index 363c5cf66fd..d05a0c1a85a 100644
> > > > > --- a/Makefile.am
> > > > > +++ b/Makefile.am
> > > > > @@ -37,9 +37,6 @@ srpm: clean
> > > > >  
> > > > >  check-local: all tests
> > > > >  
> > > > > -check-access: all
> > > > > - @($(MAKE) $(AM_MAKEFLAGS) -C tests check-access)
> > > > > -
> > > > >  dist-hook: gen-AUTHORS
> > > > >  
> > > > >  .PHONY: gen-AUTHORS
> > > > > diff --git a/scripts/check-file-access.py 
> > > > > b/scripts/check-file-access.py
> > > > > index aa120cafacf..f0e98f4b652 100755
> > > > > --- a/scripts/check-file-access.py
> > > > > +++ b/scripts/check-file-access.py
> > > > > @@ -21,15 +21,27 @@
> > > > >  #
> > > > >  #
> > > > >  
> > > > > +import os
> > > > > +import random
> > > > >  import re
> > > > > +import string
> > > > >  import sys
> > > > >  
> > > > > -if len(sys.argv) != 3:
> > > > > -print("syntax: %s ACCESS-FILE PERMITTED-ACCESS-FILE")
> > > > > -sys.exit(1)
> > > > > +abs_builddir = os.environ.get('abs_builddir', '')
> > > > > +abs_srcdir = os.environ.get('abs_srcdir', '')
> > > > >  
> > > > > -access_file = sys.argv[1]
> > > > > -permitted_file = sys.argv[2]
> > > > > +filename = ''.join(random.choice(string.ascii_letters) for _ in 
> > > > > range(16))
> > > > 
> > > > Umm, python doesn't have a 'mkostemp' equivalent? This seems a bit
> > > > fishy.
> > > 
> > > Sure, it is the tempfile module:
> > > https://docs.python.org/3/library/tempfile.html
> > > 
> > >   filename = tempfile.mkstemp(dir=abs_builddir, prefix='file-access-', 
> > > suffix='.txt')
> > 
> > I'll look into it. When porting it I just looked for any solution as
> > this can be changed any time after the series is pushed.
> 
> So I looked into it and the python script doesn't create the temporary
> file. The file is created by virtestmock.c if we run our test suite with
> file access check. The existence of the file is also used to check if
> there is something to be compared as not all of the tests needs to be
> check if they access some system files, for example all the check-* test
> cases.
> 
> The tempfile is a nice module but useless in this case where we care
> about only getting a temporary file name. In order to use the module
> we would have to do something like this:
> 
> 
> fd, filename = tempfile.mkstemp(dir=abs_builddir, prefix='file-access-', 
> suffix='.txt')
> os.close(fd)
> os.unlink()
> 
> 
> which looks ugly. So I would rather keep it as it is.

Your algorithm is not robust enough so it could end up causing random
test failures. Unlikely but possible.

You can pass the filename to the test and let the mock append to the
existing file and then read it.

Re: [PATCH] doc: fix name of file containing max number of VFs


On 7/28/20 7:32 AM, Daniel Henrique Barboza wrote:



On 7/28/20 6:16 AM, Paulo de Rezende Pinatti wrote:

Signed-off-by: Paulo de Rezende Pinatti 
---
  docs/formatnode.html.in | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/formatnode.html.in b/docs/formatnode.html.in
index e4328fedbe..8a51c4da80 100644
--- a/docs/formatnode.html.in
+++ b/docs/formatnode.html.in
@@ -141,7 +141,7 @@
  In this case this device is an SRIOV PF, and 
the capability

  element will have a list of address
  subelements, one for each VF on this PF. If the 
host system
-    supports reporting it (via the "sriov_maxvfs" 
file in the
+    supports reporting it (via the "sriov_totalvfs" 
file in the


I checked in the Linux kernel and there is no reference to 
'sriov_maxvfs'.

This means that it's not likely to be something that it's one way in
s390x/powerpc but another way in x86.

Also, googling 'sriov_maxvfs' gives 3 results only. First result isn't
related to a sriov_maxvfs file, the remaining results are links to this
Libvirt documentation. So yeah.



Late to the party, but as the person who put this typo into the tree, I 
can verify that it is indeed a typo, and never had any basis in reality. :-P






Reviewed-by: Daniel Henrique Barboza 



  device's sysfs directory) the capability 
element will also
  have an attribute named maxCount 
which is the
  maximum number of SRIOV VFs supported by this 
device, which

Re: [libvirt PATCH 034/351] meson: add AppArmor build dependency

On Tue, Jul 28, 2020 at 07:03:56PM +0200, Michal Privoznik wrote:
> On 7/16/20 11:54 AM, Pavel Hrdina wrote:
> > APPARMOR_MOUNT is not used so dropping it in meson.
> > 
> > Signed-off-by: Pavel Hrdina 
> > ---
> >   configure.ac|  3 ---
> >   m4/virt-apparmor.m4 | 41 -
> >   meson.build |  8 
> >   meson_options.txt   |  4 
> >   4 files changed, 12 insertions(+), 44 deletions(-)
> >   delete mode 100644 m4/virt-apparmor.m4
> > 
> > diff --git a/configure.ac b/configure.ac
> > index 53c87b4e293..5bf4727faa7 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -114,7 +114,6 @@ fi
> >   # Check for compiler and library settings.
> > -LIBVIRT_ARG_APPARMOR
> >   LIBVIRT_ARG_ATTR
> >   LIBVIRT_ARG_AUDIT
> >   LIBVIRT_ARG_BASH_COMPLETION
> > @@ -150,7 +149,6 @@ LIBVIRT_ARG_VIRTUALPORT
> >   LIBVIRT_ARG_WIRESHARK
> >   LIBVIRT_ARG_YAJL
> > -LIBVIRT_CHECK_APPARMOR
> >   LIBVIRT_CHECK_ATTR
> >   LIBVIRT_CHECK_AUDIT
> >   LIBVIRT_CHECK_BASH_COMPLETION
> > @@ -479,7 +477,6 @@ LIBVIRT_RESULT_DRIVER_MODULES
> >   AC_MSG_NOTICE([])
> >   AC_MSG_NOTICE([Libraries])
> >   AC_MSG_NOTICE([])
> > -LIBVIRT_RESULT_APPARMOR
> >   LIBVIRT_RESULT_ATTR
> >   LIBVIRT_RESULT_AUDIT
> >   LIBVIRT_RESULT_BASH_COMPLETION
> > diff --git a/m4/virt-apparmor.m4 b/m4/virt-apparmor.m4
> > deleted file mode 100644
> > index ebddfce2015..000
> > --- a/m4/virt-apparmor.m4
> > +++ /dev/null
> > @@ -1,41 +0,0 @@
> > -dnl The libapparmor.so library
> > -dnl
> > -dnl Copyright (C) 2012-2013 Red Hat, Inc.
> > -dnl
> > -dnl This library is free software; you can redistribute it and/or
> > -dnl modify it under the terms of the GNU Lesser General Public
> > -dnl License as published by the Free Software Foundation; either
> > -dnl version 2.1 of the License, or (at your option) any later version.
> > -dnl
> > -dnl This library is distributed in the hope that it will be useful,
> > -dnl but WITHOUT ANY WARRANTY; without even the implied warranty of
> > -dnl MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > -dnl Lesser General Public License for more details.
> > -dnl
> > -dnl You should have received a copy of the GNU Lesser General Public
> > -dnl License along with this library.  If not, see
> > -dnl .
> > -dnl
> > -
> > -AC_DEFUN([LIBVIRT_ARG_APPARMOR],[
> > -  LIBVIRT_ARG_WITH_FEATURE([APPARMOR], [AppArmor], [check])
> > -  LIBVIRT_ARG_WITH([APPARMOR_MOUNT], [set AppArmor mount point], [check])
> > -])
> > -
> > -AC_DEFUN([LIBVIRT_CHECK_APPARMOR],[
> > -  LIBVIRT_CHECK_LIB([APPARMOR], [apparmor],
> > -[aa_change_profile], [sys/apparmor.h])
> > -
> > -  if test "$with_apparmor" = "yes"; then
> > -AC_DEFINE_UNQUOTED([APPARMOR_DIR],
> > -   "/etc/apparmor.d",
> > -   [path to apparmor directory])
> > -AC_DEFINE_UNQUOTED([APPARMOR_PROFILES_PATH],
> > -   "/sys/kernel/security/apparmor/profiles",
> > -   [path to kernel profiles])
> > -  fi
> > -])
> > -
> > -AC_DEFUN([LIBVIRT_RESULT_APPARMOR],[
> > -  LIBVIRT_RESULT_LIB([APPARMOR])
> > -])
> > diff --git a/meson.build b/meson.build
> > index d8ff8e0d658..be0bc975116 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -951,6 +951,13 @@ else
> > acl_dep = dependency('', required: false)
> >   endif
> > +apparmor_dep = cc.find_library('AppArmor', required: 
> > get_option('apparmor'))
> 
> IIUC this tries to find -lAppArmor, but the library is called
> /usr/lib64/libapparmor.so.1.6.2. And also, it provides pkgconfig file:
> /usr/lib64/pkgconfig/libapparmor.pc
> 
> I don't know how to fix it, so I'm just reporting it.

Thanks for the report, the -lAppArmor was my mistake, not using
pkgconfig is pre-existing with autotools but in this case I can convert
it directly to pkgconfig in meson. The libapparmor.pc file exits on all
supported distributions so we should be safe.

No worries :) I'll fix it.

Pavel


signature.asc
Description: PGP signature

Re: [PATCH] doc: fix name of file containing max number of VFs


s/doc/docs/ in the summary to match the directory prefix

On a Tuesday in 2020, Paulo de Rezende Pinatti wrote:

Signed-off-by: Paulo de Rezende Pinatti 
---
docs/formatnode.html.in | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/formatnode.html.in b/docs/formatnode.html.in
index e4328fedbe..8a51c4da80 100644
--- a/docs/formatnode.html.in
+++ b/docs/formatnode.html.in
@@ -141,7 +141,7 @@
In this case this device is an SRIOV PF, and the capability
element will have a list of address
subelements, one for each VF on this PF. If the host system
-supports reporting it (via the "sriov_maxvfs" file in the
+supports reporting it (via the "sriov_totalvfs" file in the
device's sysfs directory) the capability element will also
have an attribute named maxCount which is the
maximum number of SRIOV VFs supported by this device, which


Reviewed-by: Ján Tomko 

and pushed.

Jano


signature.asc
Description: PGP signature

Re: [libvirt PATCH 034/351] meson: add AppArmor build dependency


On 7/16/20 11:54 AM, Pavel Hrdina wrote:

APPARMOR_MOUNT is not used so dropping it in meson.

Signed-off-by: Pavel Hrdina 
---
  configure.ac|  3 ---
  m4/virt-apparmor.m4 | 41 -
  meson.build |  8 
  meson_options.txt   |  4 
  4 files changed, 12 insertions(+), 44 deletions(-)
  delete mode 100644 m4/virt-apparmor.m4

diff --git a/configure.ac b/configure.ac
index 53c87b4e293..5bf4727faa7 100644
--- a/configure.ac
+++ b/configure.ac
@@ -114,7 +114,6 @@ fi
  
  # Check for compiler and library settings.
  
-LIBVIRT_ARG_APPARMOR

  LIBVIRT_ARG_ATTR
  LIBVIRT_ARG_AUDIT
  LIBVIRT_ARG_BASH_COMPLETION
@@ -150,7 +149,6 @@ LIBVIRT_ARG_VIRTUALPORT
  LIBVIRT_ARG_WIRESHARK
  LIBVIRT_ARG_YAJL
  
-LIBVIRT_CHECK_APPARMOR

  LIBVIRT_CHECK_ATTR
  LIBVIRT_CHECK_AUDIT
  LIBVIRT_CHECK_BASH_COMPLETION
@@ -479,7 +477,6 @@ LIBVIRT_RESULT_DRIVER_MODULES
  AC_MSG_NOTICE([])
  AC_MSG_NOTICE([Libraries])
  AC_MSG_NOTICE([])
-LIBVIRT_RESULT_APPARMOR
  LIBVIRT_RESULT_ATTR
  LIBVIRT_RESULT_AUDIT
  LIBVIRT_RESULT_BASH_COMPLETION
diff --git a/m4/virt-apparmor.m4 b/m4/virt-apparmor.m4
deleted file mode 100644
index ebddfce2015..000
--- a/m4/virt-apparmor.m4
+++ /dev/null
@@ -1,41 +0,0 @@
-dnl The libapparmor.so library
-dnl
-dnl Copyright (C) 2012-2013 Red Hat, Inc.
-dnl
-dnl This library is free software; you can redistribute it and/or
-dnl modify it under the terms of the GNU Lesser General Public
-dnl License as published by the Free Software Foundation; either
-dnl version 2.1 of the License, or (at your option) any later version.
-dnl
-dnl This library is distributed in the hope that it will be useful,
-dnl but WITHOUT ANY WARRANTY; without even the implied warranty of
-dnl MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
-dnl Lesser General Public License for more details.
-dnl
-dnl You should have received a copy of the GNU Lesser General Public
-dnl License along with this library.  If not, see
-dnl .
-dnl
-
-AC_DEFUN([LIBVIRT_ARG_APPARMOR],[
-  LIBVIRT_ARG_WITH_FEATURE([APPARMOR], [AppArmor], [check])
-  LIBVIRT_ARG_WITH([APPARMOR_MOUNT], [set AppArmor mount point], [check])
-])
-
-AC_DEFUN([LIBVIRT_CHECK_APPARMOR],[
-  LIBVIRT_CHECK_LIB([APPARMOR], [apparmor],
-[aa_change_profile], [sys/apparmor.h])
-
-  if test "$with_apparmor" = "yes"; then
-AC_DEFINE_UNQUOTED([APPARMOR_DIR],
-   "/etc/apparmor.d",
-   [path to apparmor directory])
-AC_DEFINE_UNQUOTED([APPARMOR_PROFILES_PATH],
-   "/sys/kernel/security/apparmor/profiles",
-   [path to kernel profiles])
-  fi
-])
-
-AC_DEFUN([LIBVIRT_RESULT_APPARMOR],[
-  LIBVIRT_RESULT_LIB([APPARMOR])
-])
diff --git a/meson.build b/meson.build
index d8ff8e0d658..be0bc975116 100644
--- a/meson.build
+++ b/meson.build
@@ -951,6 +951,13 @@ else
acl_dep = dependency('', required: false)
  endif
  
+apparmor_dep = cc.find_library('AppArmor', required: get_option('apparmor'))


IIUC this tries to find -lAppArmor, but the library is called 
/usr/lib64/libapparmor.so.1.6.2. And also, it provides pkgconfig file: 
/usr/lib64/pkgconfig/libapparmor.pc


I don't know how to fix it, so I'm just reporting it.

Sorry,
Michal

Re: [libvirt PATCH 312/351] meson: tests: add file access test setup

On Tue, Jul 28, 2020 at 03:39:41PM +0200, Pavel Hrdina wrote:
> On Tue, Jul 28, 2020 at 03:19:25PM +0200, Pino Toscano wrote:
> > On Tuesday, 28 July 2020 14:56:45 CEST Peter Krempa wrote:
> > > On Thu, Jul 16, 2020 at 11:59:08 +0200, Pavel Hrdina wrote:
> > > > We need to modify check-file-access.py to be usable as wrapper for
> > > > libvirt tests. This way we can run the tests using this command:
> > > > 
> > > > meson test --setup access
> > > > 
> > > > which will run all tests using check-file-access.py as a wrapper.
> > > > 
> > > > With autotools all file access are written into single file for all
> > > > tests and compared once the whole test suite is done.
> > > > 
> > > > With Meson we will compare the file access after every single test
> > > > because it is used as wrapper now. That requires writing the file
> > > > access into separate files for every single test as they are executed
> > > > in parallel.
> > > > 
> > > > Since the wrapper is used for all tests in Meson including tests outside
> > > > of tests directory we have to check for presence of the output file.
> > > > We should also cleanup after ourselves.
> > > > 
> > > > Signed-off-by: Pavel Hrdina 
> > > > ---
> > > >  Makefile.am  |  3 ---
> > > >  scripts/check-file-access.py | 24 +++-
> > > >  tests/Makefile.am| 12 
> > > >  tests/meson.build|  8 
> > > >  tests/virtestmock.c  |  2 +-
> > > >  5 files changed, 28 insertions(+), 21 deletions(-)
> > > > 
> > > > diff --git a/Makefile.am b/Makefile.am
> > > > index 363c5cf66fd..d05a0c1a85a 100644
> > > > --- a/Makefile.am
> > > > +++ b/Makefile.am
> > > > @@ -37,9 +37,6 @@ srpm: clean
> > > >  
> > > >  check-local: all tests
> > > >  
> > > > -check-access: all
> > > > -   @($(MAKE) $(AM_MAKEFLAGS) -C tests check-access)
> > > > -
> > > >  dist-hook: gen-AUTHORS
> > > >  
> > > >  .PHONY: gen-AUTHORS
> > > > diff --git a/scripts/check-file-access.py b/scripts/check-file-access.py
> > > > index aa120cafacf..f0e98f4b652 100755
> > > > --- a/scripts/check-file-access.py
> > > > +++ b/scripts/check-file-access.py
> > > > @@ -21,15 +21,27 @@
> > > >  #
> > > >  #
> > > >  
> > > > +import os
> > > > +import random
> > > >  import re
> > > > +import string
> > > >  import sys
> > > >  
> > > > -if len(sys.argv) != 3:
> > > > -print("syntax: %s ACCESS-FILE PERMITTED-ACCESS-FILE")
> > > > -sys.exit(1)
> > > > +abs_builddir = os.environ.get('abs_builddir', '')
> > > > +abs_srcdir = os.environ.get('abs_srcdir', '')
> > > >  
> > > > -access_file = sys.argv[1]
> > > > -permitted_file = sys.argv[2]
> > > > +filename = ''.join(random.choice(string.ascii_letters) for _ in 
> > > > range(16))
> > > 
> > > Umm, python doesn't have a 'mkostemp' equivalent? This seems a bit
> > > fishy.
> > 
> > Sure, it is the tempfile module:
> > https://docs.python.org/3/library/tempfile.html
> > 
> >   filename = tempfile.mkstemp(dir=abs_builddir, prefix='file-access-', 
> > suffix='.txt')
> 
> I'll look into it. When porting it I just looked for any solution as
> this can be changed any time after the series is pushed.

So I looked into it and the python script doesn't create the temporary
file. The file is created by virtestmock.c if we run our test suite with
file access check. The existence of the file is also used to check if
there is something to be compared as not all of the tests needs to be
check if they access some system files, for example all the check-* test
cases.

The tempfile is a nice module but useless in this case where we care
about only getting a temporary file name. In order to use the module
we would have to do something like this:


fd, filename = tempfile.mkstemp(dir=abs_builddir, prefix='file-access-', 
suffix='.txt')
os.close(fd)
os.unlink()


which looks ugly. So I would rather keep it as it is.

Pavel


signature.asc
Description: PGP signature

Re: [libvirt PATCH 281/351] meson: tests: build commandhelper binary

On Tue, Jul 28, 2020 at 01:28:20PM +0200, Pavel Hrdina wrote:
> On Tue, Jul 28, 2020 at 12:49:03PM +0200, Peter Krempa wrote:
> > On Thu, Jul 16, 2020 at 11:58:37 +0200, Pavel Hrdina wrote:
> > > Signed-off-by: Pavel Hrdina 
> > > ---
> > >  tests/Makefile.am | 14 +-
> > >  tests/meson.build | 17 +
> > >  2 files changed, 18 insertions(+), 13 deletions(-)
> > 
> > [...]
> > 
> > > diff --git a/tests/meson.build b/tests/meson.build
> > > index 5cbd3cd2077..fa116a0e249 100644
> > > --- a/tests/meson.build
> > > +++ b/tests/meson.build
> > > @@ -170,3 +170,20 @@ test_file_wrapper_lib = static_library(
> > >[ 'virfilewrapper.c' ],
> > >dependencies: [ tests_dep ],
> > >  )
> > > +
> > > +
> > > +# build helpers used by tests
> > > +
> > > +# Must not link to any libvirt modules - libc only otherwise external
> > > +# libraries might unexpectedly leak file descriptors into commandhelper
> > > +# invalidating the test logic assumptions.
> > 
> > This didn't work out:
> > 
> > $ ldd commandtest
> > linux-vdso.so.1 (0x7ffd24939000)
> > libvirt.so.0 => 
> > /home/pipo/build/libvirt/gcc/tests/./../src/libvirt.so.0 
> > (0x7feeabbf3000)
> > libglib-2.0.so.0 => /lib64/libglib-2.0.so.0 (0x7feeabaa8000)
> > libgcc_s.so.1 => /lib64/libgcc_s.so.1 (0x7feeaba8d000)
> > libc.so.6 => /lib64/libc.so.6 (0x7feeab8c3000)
> > libutil.so.1 => /lib64/libutil.so.1 (0x7feeab8be000)
> > libgobject-2.0.so.0 => /lib64/libgobject-2.0.so.0 (0x7feeab865000)
> > libgio-2.0.so.0 => /lib64/libgio-2.0.so.0 (0x7feeab67f000)
> > libxml2.so.2 => /lib64/libxml2.so.2 (0x7feeab50f000)
> > libacl.so.1 => /lib64/libacl.so.1 (0x7feeab504000)
> > libaudit.so.1 => /lib64/libaudit.so.1 (0x7feeab4d8000)
> > libcap-ng.so.0 => /lib64/libcap-ng.so.0 (0x7feeab4d)
> > libdbus-1.so.3 => /lib64/libdbus-1.so.3 (0x7feeab479000)
> > libgnutls.so.30 => /lib64/libgnutls.so.30 (0x7feeab28b000)
> > libnl-3.so.200 => /lib64/libnl-3.so.200 (0x7feeab267000)
> > libnuma.so.1 => /lib64/libnuma.so.1 (0x7feeab259000)
> > libselinux.so.1 => /lib64/libselinux.so.1 (0x7feeab22c000)
> > libyajl.so.2 => /lib64/libyajl.so.2 (0x7feeab22)
> > libssh2.so.1 => /lib64/libssh2.so.1 (0x7feeab1de000)
> > libssh.so.4 => /lib64/libssh.so.4 (0x7feeab16a000)
> > libsasl2.so.3 => /lib64/libsasl2.so.3 (0x7feeab14a000)
> > libtirpc.so.3 => /lib64/libtirpc.so.3 (0x7feeab119000)
> > libcurl.so.4 => /lib64/libcurl.so.4 (0x7feeab085000)
> > libwsman.so.1 => /lib64/libwsman.so.1 (0x7feeab04b000)
> > libwsman_client.so.4 => /lib64/libwsman_client.so.4 (0x7feeab03d000)
> > libwsman_curl_client_transport.so.1 => 
> > /lib64/libwsman_curl_client_transport.so.1 (0x7feeab032000)
> > libdl.so.2 => /lib64/libdl.so.2 (0x7feeab02b000)
> > libpthread.so.0 => /lib64/libpthread.so.0 (0x7feeab009000)
> > /lib64/ld-linux-x86-64.so.2 (0x7feeac0b2000)
> > libpcre.so.1 => /lib64/libpcre.so.1 (0x7feeaaf9)
> > libffi.so.6 => /lib64/libffi.so.6 (0x7feeaaf85000)
> > libgmodule-2.0.so.0 => /lib64/libgmodule-2.0.so.0 (0x7feeaaf7d000)
> > libz.so.1 => /lib64/libz.so.1 (0x7feeaaf63000)
> > libmount.so.1 => /lib64/libmount.so.1 (0x7feeaaf03000)
> > libresolv.so.2 => /lib64/libresolv.so.2 (0x7feeaaee9000)
> > liblzma.so.5 => /lib64/liblzma.so.5 (0x7feeaaebf000)
> > libm.so.6 => /lib64/libm.so.6 (0x7feeaad79000)
> > libattr.so.1 => /lib64/libattr.so.1 (0x7feeaad6f000)
> > libsystemd.so.0 => /lib64/libsystemd.so.0 (0x7feeaacb8000)
> > libp11-kit.so.0 => /lib64/libp11-kit.so.0 (0x7feeaab88000)
> > libidn2.so.0 => /lib64/libidn2.so.0 (0x7feeaab66000)
> > libunistring.so.2 => /lib64/libunistring.so.2 (0x7feeaa9e1000)
> > libtasn1.so.6 => /lib64/libtasn1.so.6 (0x7feeaa9cb000)
> > libnettle.so.7 => /lib64/libnettle.so.7 (0x7feeaa98c000)
> > libhogweed.so.5 => /lib64/libhogweed.so.5 (0x7feeaa95a000)
> > libgmp.so.10 => /lib64/libgmp.so.10 (0x7feeaa8c3000)
> > libpcre2-8.so.0 => /lib64/libpcre2-8.so.0 (0x7feeaa82a000)
> > libssl.so.1.1 => /lib64/libssl.so.1.1 (0x7feeaa793000)
> > libcrypto.so.1.1 => /lib64/libcrypto.so.1.1 (0x7feeaa4a6000)
> > libgssapi_krb5.so.2 => /lib64/libgssapi_krb5.so.2 (0x7feeaa44d000)
> > libcrypt.so.2 => /lib64/libcrypt.so.2 (0x7feeaa412000)
> > libkrb5.so.3 => /lib64/libkrb5.so.3 (0x7feeaa327000)
> > libk5crypto.so.3 => /lib64/libk5crypto.so.3 (0x7feeaa30e000)
> > libcom_err.so.2 => /lib64/libcom_err.so.2 (0x7feeaa307000)
> > libnghttp2.so.14 => /lib64/libnghttp2.so.14 (0x7feeaa2db000)
> > libpsl.so.5 => /lib64/libpsl.so.5 (0x7feeaa2c6000)
> > libldap-2.4.so.2 => /lib64/libldap-2.4.so.2 (0x7feeaa274000)
> >

Re: [PATCH 5/5] src: add missing balloon stats docs


On 7/21/20 10:07 AM, Nikolay Shirokovskiy wrote:

Signed-off-by: Nikolay Shirokovskiy 
---
  src/libvirt-domain.c | 28 
  1 file changed, 28 insertions(+)

diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index fe4dab7..36fa8a2 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -11455,6 +11455,34 @@ virConnectGetDomainCapabilities(virConnectPtr conn,
   * as unsigned long long.
   * "balloon.maximum" - the maximum memory in kiB allowed
   * as unsigned long long.
+ * "balloon.swap_in" - the amount of data read from swap space (in KiB)
+ * as unsigned long long
+ * "balloon.swap_out" - the amount of memory written out to swap space
+ *  (in KiB) as unsigned long long
+ * "balloon.major_fault" - the number of page faults when disk IO was
+ * required as unsigned long long
+ * "balloon.minor_fault" - the number of other page faults
+ * as unsigned long long
+ * "balloon.unused" - the amount of memory left unused by the system
+ *(in KiB) as unsigned long long
+ * "balloon.available" - the amount of usable memory as seen by the domain
+ *   (in KiB) as unsigned long long
+ * "balloon.rss" - Resident Set Size of running domain's process
+ * (in KiB) as unsigned long long
+ * "balloon.usable" - the amount of memory which can be reclaimed by 
balloon
+ *without causing host swapping (in KiB)
+ *as unsigned long long
+ * "balloon.last-update" - timestamp of the last update of statistics
+ * (in seconds) as unsigned long long
+ * "balloon.disk_caches" - the amount of memory that can be reclaimed
+ * without additional I/O, typically disk (in KiB)
+ * as unsigned long long
+ * "hugetlb_pgalloc" - the number of successful huge page allocations
+ * from inside the domain via virtio balloon
+ * as unsigned long long
+ * "hugetlb_pgfail" - the number of failed huge page allocations
+ *from inside the domain via virtio balloon
+ *as unsigned long long


These last two ^^ are also missing the balloon prefix.

Michal

Re: [PATCH 0/5] docs: misc docs enhancements for statistic API


On 7/21/20 10:07 AM, Nikolay Shirokovskiy wrote:

Nikolay Shirokovskiy (5):
   docs: fix typo in virsh.rst for balloon.major_fault
   include: clarify docs for hugetlb in virDomainMemoryStatTags
   docs: add missing balloon stats docs in domstats
   docs: add missing iothread stats docs in domstats
   src: add missing balloon stats docs

  docs/manpages/virsh.rst  | 11 ++-
  include/libvirt/libvirt-domain.h |  4 ++--
  src/libvirt-domain.c | 28 
  3 files changed, 40 insertions(+), 3 deletions(-)



Reviewed-by: Michal Privoznik 

and pushed. Docs changes are safe for freeze.

Michal

Re: [PATCH 2/5] include: clarify docs for hugetlb in virDomainMemoryStatTags


On 7/21/20 10:07 AM, Nikolay Shirokovskiy wrote:

The term number is used for other stats and even for hugetlb
stats in virsh man page. The term number is also more clear.

Signed-off-by: Nikolay Shirokovskiy 
---
  include/libvirt/libvirt-domain.h | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)



Missed the src/libvirt-domain.c docs.

Michal

Re: [PATCH 0/5] be consistent about error checking xmlNodeGetContent() return


On 7/20/20 10:48 PM, Laine Stump wrote:

Awhile back I noticed that calls to xmlNodeGetContent() from libvirt
code were inconsistent in their handling of the returned
pointer. Sometimes we would assume the return was always non-NULL
(dereferencing with wild abandon without concern for the
consequences), sometimes we would interpret NULL as "", and sometimes
as OOM. I sent mail about this to the list last week, wondering (and
doubting) if we could assume that a NULL return would always mean OOM:

https://www.redhat.com/archives/libvir-list/2020-July/msg00333.html

After looking at the libxml code, danpb's determination was that a
NULL return from xmlNodeGetContent *might* mean OOM, but it might also
mean some odd XML that we weren't expecting, so we can't just always
exit on a NULL return. He also agreed with (and expanded on) my
suspicion that there really is no reliable way to tell the reason for
a NULL return from xmlNodeGetContent, and suggested that possibly we
could just examing the xmlNode directly to learn the content instead
of calling xmlNodeGetContent().

This series is a followup to that discussion. The first 4 patches
clean up the code with the result that:

1) a libvirt wrapper function is always called instead of calling
xmlNodeGetContent() directly.

2) that wrapper function logs an error when it gets back NULL from
xmlNodeGetContent().

3) All the callers check for a NULL return, and do a "silent error
return" themselves when there is a NULL.

In the final patch, I try out Dan's idea of looking at the xmlNode
object directly to get the content. It turns out it's not as
straightforward as you would think from just looking at the layout of
the object - a full explanation is in patch 5. I'm mainly sending that
patch as an "FYI" (one step back from an "RFC"), since really all it
changes is that libvirt will exit on OOM, and log (different, but not
any more informative) error messages when the problem isn't
OOM. Unless someone has a strong opinion otherwise, I think just the
first 4 patches should be applied, and users can just "deal" with the
ambiguity in case of error.


Laine Stump (5):
   conf: refactor virDomainBlkioDeviceParseXML to reduce calls to
 xmlNodeGetContent
   util: replace all calls to xmlNodeGetContent with
 virXMLNodeContentString
   util: log an error if virXMLNodeContentString will return NULL
   treat all NULL returns from virXMLNodeContentString() as an error
   util: open code virXMLNodeContentString to access the node object
 directly

  src/conf/domain_conf.c  | 194 
  src/conf/network_conf.c |   7 +-
  src/conf/node_device_conf.c |   6 +-
  src/util/virxml.c   |  53 +-
  4 files changed, 169 insertions(+), 91 deletions(-)



Reviewed-by: Michal Privoznik 

Michal

Re: [FYI PATCH 5/5] util: open code virXMLNodeContentString to access the node object directly


On 7/20/20 10:48 PM, Laine Stump wrote:

(I am *NOT* advocating that we apply this patch. Just providing it for
informational purposes, since we had previously discussed this
possibility on the list)

Since it's impossible to determine whether xmlNodeContent has returned
a NULL due to OOM, or due to badly formed / evil XML, this patch open
codes virXMLNodeContentString to get the content string directly from
the node.

This turns out to not be so easy as it seemed at first glance when it
was suggested - the "content" member of the element node itself does
not contain the content string for the node. The content string that
we want can be found (at least for our uses of libxml) by looking for
a child node of the element node - if that child node is of type
XML_TEXT_NODE, then the content member of *that* node is the string
we're looking for. If there is no child node, then the element has no
content, so we return "". Likewise, if the child node is type
XML_TEXT_NODE but has no content, we also return "". In all other
cases, we log an error and return because this is some case that
hasn't been encountered in our test cases, so either someone is
sending bad XML, or our assumptions about the layout of the XML node
object list are incorrect.

Note that while calling virXMLNodeContentString() would return NULL
from an OOM situation, this new code will exit the process on OOM
(since it is calling glib for memory allocation).

Signed-off-by: Laine Stump 
---
  src/util/virxml.c | 43 ++-
  1 file changed, 38 insertions(+), 5 deletions(-)

diff --git a/src/util/virxml.c b/src/util/virxml.c
index 5315d4ff6f..b2298d74c8 100644
--- a/src/util/virxml.c
+++ b/src/util/virxml.c
@@ -538,7 +538,17 @@ virXMLPropStringLimit(xmlNodePtr node,
  char *
  virXMLNodeContentString(xmlNodePtr node)
  {
-char *ret = (char *)xmlNodeGetContent(node);
+/* We specifically avoid using virXMLNodeContentString() here, because
+ * when NULL is returned, it is difficult/impossible to
+ * distinguish between 1) OOM, 2) NULL content, 3) some other error.
+ */


s/virXMLNodeContentString/xmlNodeGetContent/

This patch makes sense to me. I'll leave it up to you whether you merge 
it or not.


Michal

Re: [PATCH] qemu: Do not silently allow non-available timers on non-x86 systems

2020-07-28 Thread Boris Fiuczynski


On 7/22/20 1:21 PM, Thomas Huth wrote:

libvirt currently silently allows  and some
other timer tags in the guest XML definition for timers that do not
exist on non-x86 systems. We should not silently ignore these tags
since the users might not get what they expected otherwise.

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1754887
Signed-off-by: Thomas Huth 
---
  src/qemu/qemu_validate.c | 12 
  1 file changed, 12 insertions(+)

diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index 488f258d00..667ac5cc23 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -371,6 +371,18 @@ qemuValidateDomainDefClockTimers(const virDomainDef *def,
  case VIR_DOMAIN_TIMER_NAME_TSC:
  case VIR_DOMAIN_TIMER_NAME_KVMCLOCK:
  case VIR_DOMAIN_TIMER_NAME_HYPERVCLOCK:
+if (!ARCH_IS_X86(def->os.arch)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("Configuring the '%s' timer is not supported "
+ "for virtType=%s arch=%s machine=%s guests"),
+   virDomainTimerNameTypeToString(timer->name),
+   virDomainVirtTypeToString(def->virtType),
+   virArchToString(def->os.arch),
+   def->os.machine);
+return -1;
+}
+break;
+
  case VIR_DOMAIN_TIMER_NAME_LAST:
  break;
  



This would render previously as valid accepted domains invalid, e.g. on 
s390x using kvmclock: As long as the user does not specify the "present" 
attribute the domain starts without error since qemus cpu parameter is 
not extended.


The feedback of all other archs would be good to have.

@Daniel: What's your opinion?

--
Mit freundlichen Grüßen/Kind regards
   Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [PATCH] doc: fix name of file containing max number of VFs

2020-07-28 Thread Boris Fiuczynski


On 7/28/20 11:16 AM, Paulo de Rezende Pinatti wrote:

Signed-off-by: Paulo de Rezende Pinatti 
---
  docs/formatnode.html.in | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/formatnode.html.in b/docs/formatnode.html.in
index e4328fedbe..8a51c4da80 100644
--- a/docs/formatnode.html.in
+++ b/docs/formatnode.html.in
@@ -141,7 +141,7 @@
  In this case this device is an SRIOV PF, and the 
capability
  element will have a list of address
  subelements, one for each VF on this PF. If the host 
system
-supports reporting it (via the "sriov_maxvfs" file in the
+supports reporting it (via the "sriov_totalvfs" file in the
  device's sysfs directory) the capability element will also
  have an attribute named maxCount which is the
  maximum number of SRIOV VFs supported by this device, 
which



Looking into RHEL docs:
https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/8/html/configuring_and_managing_virtualization/managing-virtual-devices_configuring-and-managing-virtualization

Reviewed-by: Boris Fiuczynski 

--
Mit freundlichen Grüßen/Kind regards
   Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

[RFC] Dynamic creation of VFs in a network definition containing an SRIOV device

2020-07-28 Thread Paulo de Rezende Pinatti


Context:

Libvirt can already detect the active VFs of an SRIOV PF device 
specified in a network definition and automatically assign these VFs to 
guests via an  entry referring to that network in the domain 
definition. This functionality, however, depends on the system 
administrator having activated in advance the desired number of VFs 
outside of libvirt (either manually or through system scripts).


It would be more convenient if the VFs activation could also be managed 
inside libvirt so that the whole management of the VF pool is done 
exclusively by libvirt and in only one place (the network definition) 
rather than spread in different components of the system.


Proposal:

We can extend the existing network definition by adding a new tag  
as a child of the tag  in order to allow the user to specify how 
many VFs they wish to have activated for the corresponding SRIOV device 
when the network is started. That would look like the following:



   sriov-pool
   
 

 
   


At xml definition time nothing gets changed on the system, as it is 
today. When the network is started with 'virth net-start sriov-pool' 
then libvirt will activate the desired number of VFs as specified in the 
tag  of the network definition.


The operation might require resetting 'sriov_numvfs' to zero first in 
case the number of VFs currently active differs from the desired value. 
In order to avoid the situation where the user tries to start the 
network when a VF is already assigned to a running guest, the 
implementation will have to ensure all existing VFs of the target PF are 
not in use, otherwise VFs would be inadvertently hot-unplugged from 
guests upon network start. In such cases, trying to start the network 
will then result in an error.


Stopping the network with 'virsh net-destroy' will cause all VFs to be 
removed. Similarly to when starting the network, the implementation will 
also need to verify for running guests in order to prevent inadvertent 
hot-unplugging.


Is the functionality proposed above desirable?


--
Thanks and best regards,

Paulo de Rezende Pinatti

Re: [libvirt PATCH 321/351] meson: docs: introduce meson-html-gen.py helper

On Tue, Jul 28, 2020 at 04:52:18PM +0200, Peter Krempa wrote:
> On Tue, Jul 28, 2020 at 16:46:55 +0200, Pavel Hrdina wrote:
> > On Tue, Jul 28, 2020 at 04:23:15PM +0200, Peter Krempa wrote:
> > > On Tue, Jul 28, 2020 at 16:14:03 +0200, Pavel Hrdina wrote:
> 
> [...]
> 
> > If there majority agrees on removing the script I would rather do it as
> > a followup series.
> 
> I don't care that much about repetition of build target declarations
> accross various subdirectories. In this case I care that stuff is put
> into a script which doesn't need to be there and meson can handle it
> just fine. In the end many of the subdirectories do it directly.

The issue with repetition is a possibility to forget to change all
places in the future if we need to modify the code.

> Fair enough, but you should compile a list of all that stuff you want to
> postpone as part of the final submission so that it won't be forgotten.

I have that list together with all the fixes done based on review and
other fixes that where needed because of changes in master.

I'll include all of the notes with all the changes when sending a mail
with v2.

Pavel


signature.asc
Description: PGP signature

[PATCH v2 4/4] tests: schema: test bhyvexml2xmloutdata schemas

Signed-off-by: Roman Bogorodskiy 
---
 tests/virschematest.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/virschematest.c b/tests/virschematest.c
index 8720031375..17eb2a4b34 100644
--- a/tests/virschematest.c
+++ b/tests/virschematest.c
@@ -201,7 +201,8 @@ mymain(void)
 DO_TEST_DIR("domain.rng", "domainschemadata",
 "qemuxml2argvdata", "xmconfigdata",
 "qemuxml2xmloutdata", "lxcxml2xmldata",
-"lxcxml2xmloutdata", "bhyvexml2argvdata", 
"genericxml2xmlindata",
+"lxcxml2xmloutdata", "bhyvexml2argvdata",
+"bhyvexml2xmloutdata", "genericxml2xmlindata",
 "genericxml2xmloutdata", "xlconfigdata", 
"libxlxml2domconfigdata",
 "qemuhotplugtestdomains");
 DO_TEST_DIR("domaincaps.rng", "domaincapsdata");
-- 
2.27.0

[PATCH v2 1/4] bhyve: implement sound device support

bhyve supports intel hda sound devices that could be specified
on the command like using "-1:0,hda,play=$play_dev,rec=$rec_dev",
where "1:0" is a PCI address, and "$play_dev" and "$rec_dev"
point to the playback and recording device on the host respectively.
Currently, schema of the 'sound' element doesn't allow specifying
neither playback nor recording devices, so for now hardcode
/dev/dsp0, which is the first audio device on the host.

One more simplification is to stick to "ich6" model, however
the device shows as "ich7" in a guest.

Signed-off-by: Roman Bogorodskiy 
---
 src/bhyve/bhyve_capabilities.c| 14 
 src/bhyve/bhyve_capabilities.h|  1 +
 src/bhyve/bhyve_command.c | 33 +
 src/bhyve/bhyve_device.c  |  9 +
 .../bhyvexml2argv-sound.args  | 10 ++
 .../bhyvexml2argv-sound.ldargs|  3 ++
 .../bhyvexml2argvdata/bhyvexml2argv-sound.xml | 24 +
 tests/bhyvexml2argvtest.c |  6 +++-
 .../bhyvexml2xmlout-sound.xml | 36 +++
 tests/bhyvexml2xmltest.c  |  1 +
 10 files changed, 136 insertions(+), 1 deletion(-)
 create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-sound.args
 create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-sound.ldargs
 create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-sound.xml
 create mode 100644 tests/bhyvexml2xmloutdata/bhyvexml2xmlout-sound.xml

diff --git a/src/bhyve/bhyve_capabilities.c b/src/bhyve/bhyve_capabilities.c
index fb8829d571..36f3985335 100644
--- a/src/bhyve/bhyve_capabilities.c
+++ b/src/bhyve/bhyve_capabilities.c
@@ -323,6 +323,17 @@ bhyveProbeCapsXHCIController(unsigned int *caps, char 
*binary)
 }
 
 
+static int
+bhyveProbeCapsSoundHda(unsigned int *caps, char *binary)
+{
+return bhyveProbeCapsDeviceHelper(caps, binary,
+  "-s",
+  "0,hda",
+  "pci slot 0:0: unknown device \"hda\"",
+  BHYVE_CAP_SOUND_HDA);
+}
+
+
 int
 virBhyveProbeCaps(unsigned int *caps)
 {
@@ -351,6 +362,9 @@ virBhyveProbeCaps(unsigned int *caps)
 if ((ret = bhyveProbeCapsXHCIController(caps, binary)))
 goto out;
 
+if ((ret = bhyveProbeCapsSoundHda(caps, binary)))
+goto out;
+
  out:
 VIR_FREE(binary);
 return ret;
diff --git a/src/bhyve/bhyve_capabilities.h b/src/bhyve/bhyve_capabilities.h
index 12926cf423..1ac9ff4283 100644
--- a/src/bhyve/bhyve_capabilities.h
+++ b/src/bhyve/bhyve_capabilities.h
@@ -49,6 +49,7 @@ typedef enum {
 BHYVE_CAP_FBUF = 1 << 4,
 BHYVE_CAP_XHCI = 1 << 5,
 BHYVE_CAP_CPUTOPOLOGY = 1 << 6,
+BHYVE_CAP_SOUND_HDA = 1 << 7,
 } virBhyveCapsFlags;
 
 int virBhyveProbeGrubCaps(virBhyveGrubCapsFlags *caps);
diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c
index 22d0b24ec4..64af0b3598 100644
--- a/src/bhyve/bhyve_command.c
+++ b/src/bhyve/bhyve_command.c
@@ -475,6 +475,34 @@ bhyveBuildGraphicsArgStr(const virDomainDef *def,
 
 }
 
+static int
+bhyveBuildSoundArgStr(const virDomainDef *def G_GNUC_UNUSED,
+  virDomainSoundDefPtr sound,
+  bhyveConnPtr driver,
+  virCommandPtr cmd)
+{
+if (!(bhyveDriverGetBhyveCaps(driver) & BHYVE_CAP_SOUND_HDA)) {
+/* Currently, bhyve only supports "hda" sound devices, so
+   if it's not supported, sound devices are not supported at all */
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("Sound devices emulation is not supported "
+ "by given bhyve binary"));
+return -1;
+}
+
+if (sound->model != VIR_DOMAIN_SOUND_MODEL_ICH6) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("Sound device model is not supported"));
+return -1;
+}
+
+virCommandAddArg(cmd, "-s");
+virCommandAddArgFormat(cmd, "%d:%d,hda,play=/dev/dsp0",
+   sound->info.addr.pci.slot,
+   sound->info.addr.pci.function);
+return 0;
+}
+
 virCommandPtr
 virBhyveProcessBuildBhyveCmd(bhyveConnPtr driver, virDomainDefPtr def,
  bool dryRun)
@@ -619,6 +647,11 @@ virBhyveProcessBuildBhyveCmd(bhyveConnPtr driver, 
virDomainDefPtr def,
 }
 }
 
+for (i = 0; i < def->nsounds; i++) {
+if (bhyveBuildSoundArgStr(def, def->sounds[i], driver, cmd) < 0)
+goto error;
+}
+
 if (bhyveDomainDefNeedsISAController(def))
 bhyveBuildLPCArgStr(def, cmd);
 
diff --git a/src/bhyve/bhyve_device.c b/src/bhyve/bhyve_device.c
index fc52280361..3c8ff587e0 100644
--- a/src/bhyve/bhyve_device.c
+++ b/src/bhyve/bhyve_device.c
@@ -154,6 +154,15 @@ bhyveAssignDevicePCISlots(virDomainDefPtr def,
 return -1;
 }
 
+for (i = 0; i <

[PATCH v2 3/4] bhyve: allow to specify host sound device

Allow to map sound playback and recording devices to host devices
using "" OSS audio backend.

Signed-off-by: Roman Bogorodskiy 
---
 src/bhyve/bhyve_command.c | 37 +--
 .../bhyvexml2argv-sound.args  |  2 +-
 .../bhyvexml2argvdata/bhyvexml2argv-sound.xml |  8 +++-
 .../bhyvexml2xmlout-sound.xml |  5 +++
 4 files changed, 47 insertions(+), 5 deletions(-)

diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c
index 64af0b3598..9c48c31066 100644
--- a/src/bhyve/bhyve_command.c
+++ b/src/bhyve/bhyve_command.c
@@ -478,9 +478,13 @@ bhyveBuildGraphicsArgStr(const virDomainDef *def,
 static int
 bhyveBuildSoundArgStr(const virDomainDef *def G_GNUC_UNUSED,
   virDomainSoundDefPtr sound,
+  virHashTablePtr audios,
   bhyveConnPtr driver,
   virCommandPtr cmd)
 {
+g_auto(virBuffer) params = VIR_BUFFER_INITIALIZER;
+virDomainAudioDefPtr audio;
+
 if (!(bhyveDriverGetBhyveCaps(driver) & BHYVE_CAP_SOUND_HDA)) {
 /* Currently, bhyve only supports "hda" sound devices, so
if it's not supported, sound devices are not supported at all */
@@ -497,9 +501,25 @@ bhyveBuildSoundArgStr(const virDomainDef *def 
G_GNUC_UNUSED,
 }
 
 virCommandAddArg(cmd, "-s");
-virCommandAddArgFormat(cmd, "%d:%d,hda,play=/dev/dsp0",
+
+if (audios) {
+audio = virHashLookup(audios, sound->audioId);
+
+if (audio) {
+if (audio->type == VIR_DOMAIN_AUDIO_TYPE_OSS) {
+if (audio->inputDev)
+virBufferAsprintf(, ",play=%s", audio->inputDev);
+
+if (audio->outputDev)
+virBufferAsprintf(, ",rec=%s", audio->outputDev);
+}
+}
+}
+virCommandAddArgFormat(cmd, "%d:%d,hda%s",
sound->info.addr.pci.slot,
-   sound->info.addr.pci.function);
+   sound->info.addr.pci.function,
+   virBufferCurrentContent());
+
 return 0;
 }
 
@@ -519,6 +539,7 @@ virBhyveProcessBuildBhyveCmd(bhyveConnPtr driver, 
virDomainDefPtr def,
 size_t i;
 unsigned nusbcontrollers = 0;
 unsigned nvcpus = virDomainDefGetVcpus(def);
+virHashTablePtr audios = NULL;
 
 /* CPUs */
 virCommandAddArg(cmd, "-c");
@@ -647,11 +668,20 @@ virBhyveProcessBuildBhyveCmd(bhyveConnPtr driver, 
virDomainDefPtr def,
 }
 }
 
+if (def->naudios > 0) {
+audios = virHashCreate(def->naudios, NULL);
+
+for (i = 0; i < def->naudios; i++)
+virHashAddEntry(audios, def->audios[i]->id, def->audios[i]);
+}
+
 for (i = 0; i < def->nsounds; i++) {
-if (bhyveBuildSoundArgStr(def, def->sounds[i], driver, cmd) < 0)
+if (bhyveBuildSoundArgStr(def, def->sounds[i], audios, driver, cmd) < 
0)
 goto error;
 }
 
+virHashFree(audios);
+
 if (bhyveDomainDefNeedsISAController(def))
 bhyveBuildLPCArgStr(def, cmd);
 
@@ -675,6 +705,7 @@ virBhyveProcessBuildBhyveCmd(bhyveConnPtr driver, 
virDomainDefPtr def,
 return cmd;
 
  error:
+virHashFree(audios);
 virCommandFree(cmd);
 return NULL;
 }
diff --git a/tests/bhyvexml2argvdata/bhyvexml2argv-sound.args 
b/tests/bhyvexml2argvdata/bhyvexml2argv-sound.args
index c242708ff1..05ff4965dd 100644
--- a/tests/bhyvexml2argvdata/bhyvexml2argv-sound.args
+++ b/tests/bhyvexml2argvdata/bhyvexml2argv-sound.args
@@ -7,4 +7,4 @@
 -s 0:0,hostbridge \
 -s 2:0,ahci,hd:/tmp/freebsd.img \
 -s 3:0,virtio-net,faketapdev,mac=52:54:00:b9:94:02 \
--s 4:0,hda,play=/dev/dsp0 bhyve
+-s 4:0,hda,play=/dev/dsp0,rec=/dev/dsp0 bhyve
diff --git a/tests/bhyvexml2argvdata/bhyvexml2argv-sound.xml 
b/tests/bhyvexml2argvdata/bhyvexml2argv-sound.xml
index dd4f0491a2..f622208b79 100644
--- a/tests/bhyvexml2argvdata/bhyvexml2argv-sound.xml
+++ b/tests/bhyvexml2argvdata/bhyvexml2argv-sound.xml
@@ -19,6 +19,12 @@
   
   
 
-
+
+  
+
+
+  
+  
+
   
 
diff --git a/tests/bhyvexml2xmloutdata/bhyvexml2xmlout-sound.xml 
b/tests/bhyvexml2xmloutdata/bhyvexml2xmlout-sound.xml
index f5da7c23c8..529629f165 100644
--- a/tests/bhyvexml2xmloutdata/bhyvexml2xmlout-sound.xml
+++ b/tests/bhyvexml2xmloutdata/bhyvexml2xmlout-sound.xml
@@ -30,7 +30,12 @@
   
 
 
+  
   
 
+
+  
+  
+
   
 
-- 
2.27.0

[PATCH v2 2/4] conf: allow to map sound device to host device

Introduce a new device element "" which allows
to map guest sound device specified using the ""
element to specific audio backend.

Example:

  
 
  
  
 
 
  

This block maps to OSS audio backend on the host using
/dev/dsp0 device for both input (recording)
and output (playback).

OSS is the only backend supported so far.

Signed-off-by: Roman Bogorodskiy 
---
 docs/schemas/domaincommon.rng  |  36 
 src/conf/domain_capabilities.c |   4 +
 src/conf/domain_conf.c | 156 -
 src/conf/domain_conf.h |  24 +
 src/conf/virconftypes.h|   3 +
 src/libvirt_private.syms   |   2 +
 src/qemu/qemu_command.c|   1 +
 src/qemu/qemu_domain.c |   1 +
 src/qemu/qemu_domain_address.c |   2 +
 src/qemu/qemu_driver.c |   5 ++
 src/qemu/qemu_hotplug.c|   3 +
 src/qemu/qemu_validate.c   |   1 +
 12 files changed, 236 insertions(+), 2 deletions(-)

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index a810f569c6..b0a5e08ba0 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -4366,12 +4366,47 @@
 
   
 
+
+  
+
+  
+
+  
+
 
   
 
   
 
   
+  
+
+  
+
+  
+  
+
+  oss
+
+  
+  
+
+  
+
+  
+
+  
+
+
+  
+
+  
+
+  
+
+  
+
+  
   
 
   
@@ -5286,6 +5321,7 @@
 
 
 
+
 
 
 
diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c
index 837b004334..165a792cdf 100644
--- a/src/conf/domain_capabilities.c
+++ b/src/conf/domain_capabilities.c
@@ -692,6 +692,10 @@ virDomainCapsDeviceDefValidate(const virDomainCaps *caps,
 ret = virDomainCapsDeviceVideoDefValidate(caps, dev->data.video);
 break;
 
+case VIR_DOMAIN_DEVICE_AUDIO:
+/* TODO: add validation */
+break;
+
 case VIR_DOMAIN_DEVICE_DISK:
 case VIR_DOMAIN_DEVICE_REDIRDEV:
 case VIR_DOMAIN_DEVICE_NET:
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 7ecd2818b9..50a5c3387d 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -323,6 +323,7 @@ VIR_ENUM_IMPL(virDomainDevice,
   "memory",
   "iommu",
   "vsock",
+  "audio",
 );
 
 VIR_ENUM_IMPL(virDomainDiskDevice,
@@ -728,6 +729,11 @@ VIR_ENUM_IMPL(virDomainSoundModel,
   "usb",
 );
 
+VIR_ENUM_IMPL(virDomainAudioType,
+  VIR_DOMAIN_AUDIO_TYPE_LAST,
+  "oss",
+);
+
 VIR_ENUM_IMPL(virDomainKeyWrapCipherName,
   VIR_DOMAIN_KEY_WRAP_CIPHER_NAME_LAST,
   "aes",
@@ -2850,6 +2856,21 @@ void virDomainSoundDefFree(virDomainSoundDefPtr def)
 virDomainSoundCodecDefFree(def->codecs[i]);
 VIR_FREE(def->codecs);
 
+VIR_FREE(def->audioId);
+
+VIR_FREE(def);
+}
+
+void virDomainAudioDefFree(virDomainAudioDefPtr def)
+{
+if (!def)
+return;
+
+VIR_FREE(def->id);
+
+VIR_FREE(def->inputDev);
+VIR_FREE(def->outputDev);
+
 VIR_FREE(def);
 }
 
@@ -3206,6 +3227,9 @@ void virDomainDeviceDefFree(virDomainDeviceDefPtr def)
 case VIR_DOMAIN_DEVICE_VSOCK:
 virDomainVsockDefFree(def->data.vsock);
 break;
+case VIR_DOMAIN_DEVICE_AUDIO:
+virDomainAudioDefFree(def->data.audio);
+break;
 case VIR_DOMAIN_DEVICE_LAST:
 case VIR_DOMAIN_DEVICE_NONE:
 break;
@@ -3466,6 +3490,10 @@ void virDomainDefFree(virDomainDefPtr def)
 virDomainSoundDefFree(def->sounds[i]);
 VIR_FREE(def->sounds);
 
+for (i = 0; i < def->naudios; i++)
+virDomainAudioDefFree(def->audios[i]);
+VIR_FREE(def->audios);
+
 for (i = 0; i < def->nvideos; i++)
 virDomainVideoDefFree(def->videos[i]);
 VIR_FREE(def->videos);
@@ -4054,6 +4082,7 @@ virDomainDeviceGetInfo(virDomainDeviceDefPtr device)
 case VIR_DOMAIN_DEVICE_LEASE:
 case VIR_DOMAIN_DEVICE_GRAPHICS:
 case VIR_DOMAIN_DEVICE_IOMMU:
+case VIR_DOMAIN_DEVICE_AUDIO:
 case VIR_DOMAIN_DEVICE_LAST:
 case VIR_DOMAIN_DEVICE_NONE:
 break;
@@ -4148,6 +4177,9 @@ virDomainDeviceSetData(virDomainDeviceDefPtr device,
 case VIR_DOMAIN_DEVICE_LEASE:
 device->data.lease = devicedata;
 break;
+case VIR_DOMAIN_DEVICE_AUDIO:
+device->data.audio = devicedata;
+break;
 case VIR_DOMAIN_DEVICE_NONE:
 case VIR_DOMAIN_DEVICE_LAST:
 break;
@@ -4414,6 +4446,7 @@ virDomainDeviceInfoIterateInternal(virDomainDefPtr def,
 case VIR_DOMAIN_DEVICE_MEMORY:
 case VIR_DOMAIN_DEVICE_IOMMU:
 case VIR_DOMAIN_DEVICE_VSOCK:
+case VIR_DOMAIN_DEVICE_AUDIO:

[PATCH v2 0/4] bhyve: implement sound device support

Changes from v1:

Main change is the addition of the "" element that
allows to map the "" device to the host audio backend.
Would appreciate initial feedback on this one, and then I'll proceed
with adding more validation.

Roman Bogorodskiy (4):
  bhyve: implement sound device support
  conf: allow to map sound device to host device
  bhyve: allow to specify host sound device
  tests: schema: test bhyvexml2xmloutdata schemas

 docs/schemas/domaincommon.rng |  36 
 src/bhyve/bhyve_capabilities.c|  14 ++
 src/bhyve/bhyve_capabilities.h|   1 +
 src/bhyve/bhyve_command.c |  64 +++
 src/bhyve/bhyve_device.c  |   9 +
 src/conf/domain_capabilities.c|   4 +
 src/conf/domain_conf.c| 156 +-
 src/conf/domain_conf.h|  24 +++
 src/conf/virconftypes.h   |   3 +
 src/libvirt_private.syms  |   2 +
 src/qemu/qemu_command.c   |   1 +
 src/qemu/qemu_domain.c|   1 +
 src/qemu/qemu_domain_address.c|   2 +
 src/qemu/qemu_driver.c|   5 +
 src/qemu/qemu_hotplug.c   |   3 +
 src/qemu/qemu_validate.c  |   1 +
 .../bhyvexml2argv-sound.args  |  10 ++
 .../bhyvexml2argv-sound.ldargs|   3 +
 .../bhyvexml2argvdata/bhyvexml2argv-sound.xml |  30 
 tests/bhyvexml2argvtest.c |   6 +-
 .../bhyvexml2xmlout-sound.xml |  41 +
 tests/bhyvexml2xmltest.c  |   1 +
 tests/virschematest.c |   3 +-
 23 files changed, 416 insertions(+), 4 deletions(-)
 create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-sound.args
 create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-sound.ldargs
 create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-sound.xml
 create mode 100644 tests/bhyvexml2xmloutdata/bhyvexml2xmlout-sound.xml

-- 
2.27.0

Re: [libvirt PATCH 321/351] meson: docs: introduce meson-html-gen.py helper

On Tue, Jul 28, 2020 at 16:46:55 +0200, Pavel Hrdina wrote:
> On Tue, Jul 28, 2020 at 04:23:15PM +0200, Peter Krempa wrote:
> > On Tue, Jul 28, 2020 at 16:14:03 +0200, Pavel Hrdina wrote:

[...]

> If there majority agrees on removing the script I would rather do it as
> a followup series.

I don't care that much about repetition of build target declarations
accross various subdirectories. In this case I care that stuff is put
into a script which doesn't need to be there and meson can handle it
just fine. In the end many of the subdirectories do it directly.

Fair enough, but you should compile a list of all that stuff you want to
postpone as part of the final submission so that it won't be forgotten.

Re: [libvirt PATCH 321/351] meson: docs: introduce meson-html-gen.py helper

On Tue, Jul 28, 2020 at 04:23:15PM +0200, Peter Krempa wrote:
> On Tue, Jul 28, 2020 at 16:14:03 +0200, Pavel Hrdina wrote:
> > On Tue, Jul 28, 2020 at 03:59:58PM +0200, Peter Krempa wrote:
> > > On Tue, Jul 28, 2020 at 15:57:16 +0200, Pavel Hrdina wrote:
> > > > On Tue, Jul 28, 2020 at 03:18:23PM +0200, Peter Krempa wrote:
> > > > > On Thu, Jul 16, 2020 at 11:59:17 +0200, Pavel Hrdina wrote:
> > > > > > Signed-off-by: Pavel Hrdina 
> > > > > > ---
> > > > > >  docs/Makefile.am  | 26 -
> > > > > >  scripts/meson-html-gen.py | 49 
> > > > > > +++
> > > > > >  scripts/meson.build   |  1 +
> > > > > >  3 files changed, 50 insertions(+), 26 deletions(-)
> > > > > >  create mode 100755 scripts/meson-html-gen.py
> > > 
> > > [...]
> > > 
> > > > > > +)
> > > > > > +
> > > > > > +html = subprocess.run(
> > > > > > +[args.xmllint, '--nonet', '--format', '-'],
> > > > > > +input=html_tmp.stdout,
> > > > > > +stdout=subprocess.PIPE,
> > > > > > +stderr=subprocess.PIPE,
> > > > > 
> > > > > We can then do this as separate stage.
> > > > 
> > > > What do you mean by separate stage? Not following here.
> > > 
> > > I was arguing that the reformatting done by xmllint can be done as a
> > > separate build target thus eliminating the need to have this script
> > > completely.
> > 
> > Right, I guess now it can be done like that since there is not the part
> > that modified the string directly in python. However, I would like to
> > keep it like this.
> 
> Well, and I don't like the extra python wrapper which obscures what is
> happening. Arguably we could write everything ourselves and just invoke
> a massive script from make/meson/whatever.

Thanks for invalid argument.

> > We will not have temporary files in the build directory. With two
> > targets the temporary file should not be removed otherwise ninja would
> > rebuild it every single time and that would cause to rebuild the
> > resulting HTML as well.
> 
> Umm there's plenty of temporary stuff in the build directory. I don't
> think we care. 

Not in the docs directory. But I take it as it was not a strong argument
from my side. However, there is another argument.

It is called from docs/meson.build, docs/internals/meson.build,
docs/kbase/meson.build and docs/manpages/meson.build. In order to avoid
repetition in meson it is recommended to use scripts as meson doesn't
support functions.

To avoid the repetition we could do the same trick as in src/ directory
by using list of dictionaries which would be processed only in
docs/meson.build.

If there majority agrees on removing the script I would rather do it as
a followup series.

Pavel


signature.asc
Description: PGP signature

Re: [libvirt PATCH 342/351] meson: add syntax-check

On Tue, Jul 28, 2020 at 03:08:11PM +0100, Daniel P. Berrangé wrote:
> On Tue, Jul 28, 2020 at 04:04:39PM +0200, Pavel Hrdina wrote:
> > On Tue, Jul 28, 2020 at 03:48:29PM +0200, Peter Krempa wrote:
> > > On Thu, Jul 16, 2020 at 11:59:38 +0200, Pavel Hrdina wrote:
> > > > Signed-off-by: Pavel Hrdina 
> > > > ---
> > > >  build-aux/Makefile.in |  9 +++
> > > >  .../Makefile.nonreentrant |  0
> > > >  build-aux/meson.build | 30 +
> > > >  build-aux/syntax-check.mk | 62 +--
> > > >  meson.build   |  2 +
> > > >  5 files changed, 72 insertions(+), 31 deletions(-)
> > > >  create mode 100644 build-aux/Makefile.in
> > > >  rename Makefile.nonreentrant => build-aux/Makefile.nonreentrant (100%)
> > > >  create mode 100644 build-aux/meson.build
> > > 
> > > [...]
> > > 
> > > > +make_prog = find_program('make')
> > > > +
> > > > +# There is no way how to pass value to -j using run_target so let's use
> > > > +# it without any value to run all tests in parallel.
> > > > +run_target(
> > > > +  'syntax-check',
> > > > +  command: [
> > > > +make_prog, '-C', meson.current_build_dir(), '-j', 'syntax-check',
> > > > +  ],
> > > 
> > > While I do run syntax check with unlimited '-j'. I don't think it's
> > > entirely cool to impose that on everybody. Specifically overcommiting
> > > the system is not cool. Since meson is automagically scaling can't we
> > > use the meson-detected cpu number here?
> > 
> > Unfortunately no, that was the first thing I was trying to figure out
> > by going through meson code as well. It's not ideal I know.
> > 
> > Other options are to not use -j at all which is no-go or we can add some
> > code to detect the available number of CPUs. But again it would not
> > reflect the fact if user runs 'ninja -j N'.
> 
> In libosinfo we put "syntax-check" as part of the unit tests, rather
> than as a separate meson target. With that you don't need to pass
> -j to syntax-check, because other unit tests are running in parallel
> already, and chances are syntax-check will finish first even when
> serialized.

That was my idea as well, but I know that we don't want to run
syntax-check in downstream packaging as it usually fails because of
outdated tools or GCC or other bit's involved in syntax check.

On the other hand we would have syntax-check part of the meson dist
command which is equivalent to make distcheck. Not sure if desired.

To solve the issue with downstream packaging meson supports labels by
using 'suite' argument when calling test() function. We can use
suite: 'syntax-check' for this purpose and in spec file we can have this
line:

VIR_TEST_DEBUG=1 %meson_test --no-suite syntax-check

With all that in mind, running 'meson test' or 'ninja test' will show
that our syntax-check running using single thread takes really long.

On my box all the whole test suite without syntax-check takes 5.793s
so let's say 6s but the syntax-check part takes 24.716s.

I don't think we want to do that.

Pavel


signature.asc
Description: PGP signature

Re: [libvirt PATCH 0/9] Remove some unused macros

2020-07-28 Thread Jonathon Jongsma

Reviewed-by: Jonathon Jongsma 


On Tue, 2020-07-28 at 13:54 +0200, Ján Tomko wrote:
> I built with -Wunused-macros again after five years [0]
> the good news is we don't really have that many -
> most of them are VIR_FROM_THIS or macros we have included
> for completeness.
> 
> https://gitlab.com/jano.tomko/libvirt/-/pipelines/171383854
> 
> [0] a0482396d7e0b164e71b2bdd881d9db5c6521303
> 
> Ján Tomko (9):
>   locking: sanlock: drop unused LOCKSPACE_SLEEP
>   storage: logical: drop unused PV_BLANK_SECTOR_SIZE
>   storage: scsi: remove unused LINUX_SYSFS_SCSI_HOST_POSTFIX
>   test: remove unused NUM_CELLS
>   util: remove unused VIR_MCAST_ADDR_LEN
>   virsh: remove unused FILTER macro
>   virt-admin: remove unused VIRT_ADMIN_TIME_BUFLEN
>   util: vportprofile: remove unused constants
>   util: netdevip: remove unused VIR_NETDEV_FAMILY
> 
>  src/locking/lock_driver_sanlock.c | 2 --
>  src/storage/storage_backend_logical.c | 2 --
>  src/storage/storage_backend_scsi.c| 1 -
>  src/test/test_driver.c| 2 --
>  src/util/virnetdev.c  | 1 -
>  src/util/virnetdevip.c| 5 -
>  src/util/virnetdevvportprofile.c  | 4 
>  tools/virsh-network.c | 4 
>  tools/virt-admin.c| 3 ---
>  9 files changed, 24 deletions(-)
>

Re: [libvirt PATCH 321/351] meson: docs: introduce meson-html-gen.py helper

On Tue, Jul 28, 2020 at 16:14:03 +0200, Pavel Hrdina wrote:
> On Tue, Jul 28, 2020 at 03:59:58PM +0200, Peter Krempa wrote:
> > On Tue, Jul 28, 2020 at 15:57:16 +0200, Pavel Hrdina wrote:
> > > On Tue, Jul 28, 2020 at 03:18:23PM +0200, Peter Krempa wrote:
> > > > On Thu, Jul 16, 2020 at 11:59:17 +0200, Pavel Hrdina wrote:
> > > > > Signed-off-by: Pavel Hrdina 
> > > > > ---
> > > > >  docs/Makefile.am  | 26 -
> > > > >  scripts/meson-html-gen.py | 49 
> > > > > +++
> > > > >  scripts/meson.build   |  1 +
> > > > >  3 files changed, 50 insertions(+), 26 deletions(-)
> > > > >  create mode 100755 scripts/meson-html-gen.py
> > 
> > [...]
> > 
> > > > > +)
> > > > > +
> > > > > +html = subprocess.run(
> > > > > +[args.xmllint, '--nonet', '--format', '-'],
> > > > > +input=html_tmp.stdout,
> > > > > +stdout=subprocess.PIPE,
> > > > > +stderr=subprocess.PIPE,
> > > > 
> > > > We can then do this as separate stage.
> > > 
> > > What do you mean by separate stage? Not following here.
> > 
> > I was arguing that the reformatting done by xmllint can be done as a
> > separate build target thus eliminating the need to have this script
> > completely.
> 
> Right, I guess now it can be done like that since there is not the part
> that modified the string directly in python. However, I would like to
> keep it like this.

Well, and I don't like the extra python wrapper which obscures what is
happening. Arguably we could write everything ourselves and just invoke
a massive script from make/meson/whatever.

> We will not have temporary files in the build directory. With two
> targets the temporary file should not be removed otherwise ninja would
> rebuild it every single time and that would cause to rebuild the
> resulting HTML as well.

Umm there's plenty of temporary stuff in the build directory. I don't
think we care.

Re: [libvirt PATCH 342/351] meson: add syntax-check

On Tue, Jul 28, 2020 at 15:08:11 +0100, Daniel Berrange wrote:
> On Tue, Jul 28, 2020 at 04:04:39PM +0200, Pavel Hrdina wrote:
> > On Tue, Jul 28, 2020 at 03:48:29PM +0200, Peter Krempa wrote:
> > > On Thu, Jul 16, 2020 at 11:59:38 +0200, Pavel Hrdina wrote:
> > > > Signed-off-by: Pavel Hrdina 
> > > > ---
> > > >  build-aux/Makefile.in |  9 +++
> > > >  .../Makefile.nonreentrant |  0
> > > >  build-aux/meson.build | 30 +
> > > >  build-aux/syntax-check.mk | 62 +--
> > > >  meson.build   |  2 +
> > > >  5 files changed, 72 insertions(+), 31 deletions(-)
> > > >  create mode 100644 build-aux/Makefile.in
> > > >  rename Makefile.nonreentrant => build-aux/Makefile.nonreentrant (100%)
> > > >  create mode 100644 build-aux/meson.build
> > > 
> > > [...]
> > > 
> > > > +make_prog = find_program('make')
> > > > +
> > > > +# There is no way how to pass value to -j using run_target so let's use
> > > > +# it without any value to run all tests in parallel.
> > > > +run_target(
> > > > +  'syntax-check',
> > > > +  command: [
> > > > +make_prog, '-C', meson.current_build_dir(), '-j', 'syntax-check',
> > > > +  ],
> > > 
> > > While I do run syntax check with unlimited '-j'. I don't think it's
> > > entirely cool to impose that on everybody. Specifically overcommiting
> > > the system is not cool. Since meson is automagically scaling can't we
> > > use the meson-detected cpu number here?
> > 
> > Unfortunately no, that was the first thing I was trying to figure out
> > by going through meson code as well. It's not ideal I know.
> > 
> > Other options are to not use -j at all which is no-go or we can add some
> > code to detect the available number of CPUs. But again it would not
> > reflect the fact if user runs 'ninja -j N'.
> 
> In libosinfo we put "syntax-check" as part of the unit tests, rather
> than as a separate meson target. With that you don't need to pass
> -j to syntax-check, because other unit tests are running in parallel
> already, and chances are syntax-check will finish first even when
> serialized.

Unfortunately it's not even close.

Serialized syntax-check:
real0m22.139s
user0m24.209s
sys 0m6.788s

test suite:
real0m4.833s
user0m12.408s
sys 0m3.918s

syntax-check with -j == ncpus: (24 thread box)

real0m2.099s
user0m28.558s
sys 0m7.739s


As said, I'm a big fan of -jncpus or -j. so I really want to keep it
especially given the data above, but on the other hand I don't want to
set the CI boxes on fire.

Re: [libvirt PATCH 321/351] meson: docs: introduce meson-html-gen.py helper

On Tue, Jul 28, 2020 at 03:59:58PM +0200, Peter Krempa wrote:
> On Tue, Jul 28, 2020 at 15:57:16 +0200, Pavel Hrdina wrote:
> > On Tue, Jul 28, 2020 at 03:18:23PM +0200, Peter Krempa wrote:
> > > On Thu, Jul 16, 2020 at 11:59:17 +0200, Pavel Hrdina wrote:
> > > > Signed-off-by: Pavel Hrdina 
> > > > ---
> > > >  docs/Makefile.am  | 26 -
> > > >  scripts/meson-html-gen.py | 49 +++
> > > >  scripts/meson.build   |  1 +
> > > >  3 files changed, 50 insertions(+), 26 deletions(-)
> > > >  create mode 100755 scripts/meson-html-gen.py
> 
> [...]
> 
> > > > +)
> > > > +
> > > > +html = subprocess.run(
> > > > +[args.xmllint, '--nonet', '--format', '-'],
> > > > +input=html_tmp.stdout,
> > > > +stdout=subprocess.PIPE,
> > > > +stderr=subprocess.PIPE,
> > > 
> > > We can then do this as separate stage.
> > 
> > What do you mean by separate stage? Not following here.
> 
> I was arguing that the reformatting done by xmllint can be done as a
> separate build target thus eliminating the need to have this script
> completely.

Right, I guess now it can be done like that since there is not the part
that modified the string directly in python. However, I would like to
keep it like this.

We will not have temporary files in the build directory. With two
targets the temporary file should not be removed otherwise ninja would
rebuild it every single time and that would cause to rebuild the
resulting HTML as well.

Pavel


signature.asc
Description: PGP signature

Re: [libvirt PATCH 342/351] meson: add syntax-check

2020-07-28 Thread Daniel P . Berrangé

On Tue, Jul 28, 2020 at 04:04:39PM +0200, Pavel Hrdina wrote:
> On Tue, Jul 28, 2020 at 03:48:29PM +0200, Peter Krempa wrote:
> > On Thu, Jul 16, 2020 at 11:59:38 +0200, Pavel Hrdina wrote:
> > > Signed-off-by: Pavel Hrdina 
> > > ---
> > >  build-aux/Makefile.in |  9 +++
> > >  .../Makefile.nonreentrant |  0
> > >  build-aux/meson.build | 30 +
> > >  build-aux/syntax-check.mk | 62 +--
> > >  meson.build   |  2 +
> > >  5 files changed, 72 insertions(+), 31 deletions(-)
> > >  create mode 100644 build-aux/Makefile.in
> > >  rename Makefile.nonreentrant => build-aux/Makefile.nonreentrant (100%)
> > >  create mode 100644 build-aux/meson.build
> > 
> > [...]
> > 
> > > +make_prog = find_program('make')
> > > +
> > > +# There is no way how to pass value to -j using run_target so let's use
> > > +# it without any value to run all tests in parallel.
> > > +run_target(
> > > +  'syntax-check',
> > > +  command: [
> > > +make_prog, '-C', meson.current_build_dir(), '-j', 'syntax-check',
> > > +  ],
> > 
> > While I do run syntax check with unlimited '-j'. I don't think it's
> > entirely cool to impose that on everybody. Specifically overcommiting
> > the system is not cool. Since meson is automagically scaling can't we
> > use the meson-detected cpu number here?
> 
> Unfortunately no, that was the first thing I was trying to figure out
> by going through meson code as well. It's not ideal I know.
> 
> Other options are to not use -j at all which is no-go or we can add some
> code to detect the available number of CPUs. But again it would not
> reflect the fact if user runs 'ninja -j N'.

In libosinfo we put "syntax-check" as part of the unit tests, rather
than as a separate meson target. With that you don't need to pass
-j to syntax-check, because other unit tests are running in parallel
already, and chances are syntax-check will finish first even when
serialized.


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: [libvirt PATCH 343/351] meson: update spec file to use meson

On Tue, Jul 28, 2020 at 03:51:43PM +0200, Peter Krempa wrote:
> On Thu, Jul 16, 2020 at 11:59:39 +0200, Pavel Hrdina wrote:
> > Signed-off-by: Pavel Hrdina 
> > ---
> >  libvirt.spec.in | 203 +---
> >  meson.build |   1 -
> >  2 files changed, 89 insertions(+), 115 deletions(-)
> > 
> > diff --git a/libvirt.spec.in b/libvirt.spec.in
> > index 522e36b6a51..94c8715faa1 100644
> > --- a/libvirt.spec.in
> > +++ b/libvirt.spec.in
> 
> [...]
> 
> > @@ -257,12 +252,7 @@ Requires: libvirt-libs = %{version}-%{release}
> >  
> >  # All build-time requirements. Run-time requirements are
> >  # listed against each sub-RPM
> > -%if 0%{?enable_autotools}
> > -BuildRequires: autoconf
> > -BuildRequires: automake
> >  BuildRequires: gettext-devel
> > -BuildRequires: libtool
> > -%endif
> 
> Shouldn't actually 'meson' and 'ninja' be added to BuildRequires?

Right, I wanted to wait on how we solve the downstream issue with
missing correct meson version. But I guess it will be up to downstream
to modify the spec file so I'll add it.

Pavel


signature.asc
Description: PGP signature

Re: [libvirt PATCH 342/351] meson: add syntax-check

On Tue, Jul 28, 2020 at 03:48:29PM +0200, Peter Krempa wrote:
> On Thu, Jul 16, 2020 at 11:59:38 +0200, Pavel Hrdina wrote:
> > Signed-off-by: Pavel Hrdina 
> > ---
> >  build-aux/Makefile.in |  9 +++
> >  .../Makefile.nonreentrant |  0
> >  build-aux/meson.build | 30 +
> >  build-aux/syntax-check.mk | 62 +--
> >  meson.build   |  2 +
> >  5 files changed, 72 insertions(+), 31 deletions(-)
> >  create mode 100644 build-aux/Makefile.in
> >  rename Makefile.nonreentrant => build-aux/Makefile.nonreentrant (100%)
> >  create mode 100644 build-aux/meson.build
> 
> [...]
> 
> > +make_prog = find_program('make')
> > +
> > +# There is no way how to pass value to -j using run_target so let's use
> > +# it without any value to run all tests in parallel.
> > +run_target(
> > +  'syntax-check',
> > +  command: [
> > +make_prog, '-C', meson.current_build_dir(), '-j', 'syntax-check',
> > +  ],
> 
> While I do run syntax check with unlimited '-j'. I don't think it's
> entirely cool to impose that on everybody. Specifically overcommiting
> the system is not cool. Since meson is automagically scaling can't we
> use the meson-detected cpu number here?

Unfortunately no, that was the first thing I was trying to figure out
by going through meson code as well. It's not ideal I know.

Other options are to not use -j at all which is no-go or we can add some
code to detect the available number of CPUs. But again it would not
reflect the fact if user runs 'ninja -j N'.

Pavel


signature.asc
Description: PGP signature

Re: [libvirt PATCH 321/351] meson: docs: introduce meson-html-gen.py helper

On Tue, Jul 28, 2020 at 15:57:16 +0200, Pavel Hrdina wrote:
> On Tue, Jul 28, 2020 at 03:18:23PM +0200, Peter Krempa wrote:
> > On Thu, Jul 16, 2020 at 11:59:17 +0200, Pavel Hrdina wrote:
> > > Signed-off-by: Pavel Hrdina 
> > > ---
> > >  docs/Makefile.am  | 26 -
> > >  scripts/meson-html-gen.py | 49 +++
> > >  scripts/meson.build   |  1 +
> > >  3 files changed, 50 insertions(+), 26 deletions(-)
> > >  create mode 100755 scripts/meson-html-gen.py

[...]

> > > +)
> > > +
> > > +html = subprocess.run(
> > > +[args.xmllint, '--nonet', '--format', '-'],
> > > +input=html_tmp.stdout,
> > > +stdout=subprocess.PIPE,
> > > +stderr=subprocess.PIPE,
> > 
> > We can then do this as separate stage.
> 
> What do you mean by separate stage? Not following here.

I was arguing that the reformatting done by xmllint can be done as a
separate build target thus eliminating the need to have this script
completely.

Re: [libvirt PATCH 321/351] meson: docs: introduce meson-html-gen.py helper

On Tue, Jul 28, 2020 at 03:18:23PM +0200, Peter Krempa wrote:
> On Thu, Jul 16, 2020 at 11:59:17 +0200, Pavel Hrdina wrote:
> > Signed-off-by: Pavel Hrdina 
> > ---
> >  docs/Makefile.am  | 26 -
> >  scripts/meson-html-gen.py | 49 +++
> >  scripts/meson.build   |  1 +
> >  3 files changed, 50 insertions(+), 26 deletions(-)
> >  create mode 100755 scripts/meson-html-gen.py
> > 
> > diff --git a/docs/Makefile.am b/docs/Makefile.am
> > index a2fe2fbdc75..0c42db2badb 100644
> > --- a/docs/Makefile.am
> > +++ b/docs/Makefile.am
> > @@ -320,32 +320,6 @@ news.html.in: $(top_srcdir)/NEWS.rst
> > $(AM_V_GEN)$(MKDIR_P) `dirname $@` && \
> >   $(RST2HTML) --strict $< > $@ || { rm $@ && exit 1; }
> >  
> > -%.html.tmp: %.html.in site.xsl subsite.xsl page.xsl \
> > -   $(acl_generated)
> > -   $(AM_V_GEN)name=`echo $@ | sed -e 's/.tmp//'`; \
> > - genhtmlin=`echo $@ | sed -e 's/.tmp/.in/'`; \
> > - rst=`echo $@ | sed -e 's/.html.tmp/.rst/'`; \
> > - src="$$genhtmlin"; \
> > - test -f "$$genhtmlin" && src="$$rst"; \
> > - dir=`dirname $@` ; \
> > - if test "$$dir" = "."; \
> > - then \
> > -   style=site.xsl; \
> > - else \
> > -   $(MKDIR_P) $$dir; \
> > -   style=subsite.xsl; \
> > - fi; \
> > - $(XSLTPROC) --stringparam pagename $$name \
> > -   --stringparam pagesrc $$src \
> > -   --stringparam builddir '$(abs_top_builddir)' \
> > -   --stringparam timestamp $(timestamp) --nonet \
> > -   $(top_srcdir)/docs/$$style $< > $@ \
> > -   || { rm $@ && exit 1; }
> > -
> > -%.html: %.html.tmp
> > -   $(AM_V_GEN)$(XMLLINT) --nonet --format $< > $@ \
> > - || { rm $@ && exit 1; }
> > -
> >  $(apihtml_generated): html/index.html
> >  $(apiadminhtml_generated): html/index-admin.html
> >  $(apiqemuhtml_generated): html/index-qemu.html
> > diff --git a/scripts/meson-html-gen.py b/scripts/meson-html-gen.py
> > new file mode 100755
> > index 000..9ac649a9ef7
> > --- /dev/null
> > +++ b/scripts/meson-html-gen.py
> > @@ -0,0 +1,49 @@
> > +#!/usr/bin/env python3
> > +
> > +import argparse
> > +import os
> > +import subprocess
> > +
> > +parser = argparse.ArgumentParser()
> > +parser.add_argument("xsltproc", type=str, help="path to xsltproc bin")
> > +parser.add_argument("xmllint", type=str, help="path to xmllint bin")
> > +parser.add_argument("builddir", type=str, help="build root dir path")
> > +parser.add_argument("timestamp", type=str, help="docs timestamp")
> > +parser.add_argument("style", type=str, help="XSL stile file")
> > +parser.add_argument("infile", type=str, help="path to source HTML file")
> > +parser.add_argument("htmlfile", type=str, help="path to generated HTML 
> > file")
> > +args = parser.parse_args()
> > +
> > +name = os.path.basename(args.htmlfile).replace('.html', '')
> > +
> > +pagesrc = args.infile
> > +rstfile = pagesrc.replace('.html.in', '.rst')
> > +if os.path.exists(rstfile):
> > +pagesrc = rstfile
> > +
> > +with open(args.infile, 'rb') as infile:
> > +html_in_data = infile.read()
> > +
> > +html_tmp = subprocess.run(
> > +[
> > +args.xsltproc,
> > +'--stringparam', 'pagename', name,
> > +'--stringparam', 'pagesrc', pagesrc,
> > +'--stringparam', 'builddir', args.builddir,
> > +'--stringparam', 'timestamp', args.timestamp,
> > +'--nonet', args.style, '-',
> > +],
> > +input=html_in_data,
> > +stdout=subprocess.PIPE,
> > +stderr=subprocess.PIPE,
> 
> xsltproc can take an input file as argument so if all of this is just
> for feeding it the input, it's not necessary to do it so.

Nice catch, leftover from one of the previous versions where I was
modifying the infile data directly in python, hence the open() call
and passing it to stdin. I'll remove it.

> > +)
> > +
> > +html = subprocess.run(
> > +[args.xmllint, '--nonet', '--format', '-'],
> > +input=html_tmp.stdout,
> > +stdout=subprocess.PIPE,
> > +stderr=subprocess.PIPE,
> 
> We can then do this as separate stage.

What do you mean by separate stage? Not following here.

Pavel


signature.asc
Description: PGP signature

Re: [libvirt PATCH 343/351] meson: update spec file to use meson

On Thu, Jul 16, 2020 at 11:59:39 +0200, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina 
> ---
>  libvirt.spec.in | 203 +---
>  meson.build |   1 -
>  2 files changed, 89 insertions(+), 115 deletions(-)
> 
> diff --git a/libvirt.spec.in b/libvirt.spec.in
> index 522e36b6a51..94c8715faa1 100644
> --- a/libvirt.spec.in
> +++ b/libvirt.spec.in

[...]

> @@ -257,12 +252,7 @@ Requires: libvirt-libs = %{version}-%{release}
>  
>  # All build-time requirements. Run-time requirements are
>  # listed against each sub-RPM
> -%if 0%{?enable_autotools}
> -BuildRequires: autoconf
> -BuildRequires: automake
>  BuildRequires: gettext-devel
> -BuildRequires: libtool
> -%endif

Shouldn't actually 'meson' and 'ninja' be added to BuildRequires?

Re: [libvirt PATCH 342/351] meson: add syntax-check

On Thu, Jul 16, 2020 at 11:59:38 +0200, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina 
> ---
>  build-aux/Makefile.in |  9 +++
>  .../Makefile.nonreentrant |  0
>  build-aux/meson.build | 30 +
>  build-aux/syntax-check.mk | 62 +--
>  meson.build   |  2 +
>  5 files changed, 72 insertions(+), 31 deletions(-)
>  create mode 100644 build-aux/Makefile.in
>  rename Makefile.nonreentrant => build-aux/Makefile.nonreentrant (100%)
>  create mode 100644 build-aux/meson.build

[...]

> +make_prog = find_program('make')
> +
> +# There is no way how to pass value to -j using run_target so let's use
> +# it without any value to run all tests in parallel.
> +run_target(
> +  'syntax-check',
> +  command: [
> +make_prog, '-C', meson.current_build_dir(), '-j', 'syntax-check',
> +  ],

While I do run syntax check with unlimited '-j'. I don't think it's
entirely cool to impose that on everybody. Specifically overcommiting
the system is not cool. Since meson is automagically scaling can't we
use the meson-detected cpu number here?

Re: [libvirt PATCH 312/351] meson: tests: add file access test setup

On Tue, Jul 28, 2020 at 03:19:25PM +0200, Pino Toscano wrote:
> On Tuesday, 28 July 2020 14:56:45 CEST Peter Krempa wrote:
> > On Thu, Jul 16, 2020 at 11:59:08 +0200, Pavel Hrdina wrote:
> > > We need to modify check-file-access.py to be usable as wrapper for
> > > libvirt tests. This way we can run the tests using this command:
> > > 
> > > meson test --setup access
> > > 
> > > which will run all tests using check-file-access.py as a wrapper.
> > > 
> > > With autotools all file access are written into single file for all
> > > tests and compared once the whole test suite is done.
> > > 
> > > With Meson we will compare the file access after every single test
> > > because it is used as wrapper now. That requires writing the file
> > > access into separate files for every single test as they are executed
> > > in parallel.
> > > 
> > > Since the wrapper is used for all tests in Meson including tests outside
> > > of tests directory we have to check for presence of the output file.
> > > We should also cleanup after ourselves.
> > > 
> > > Signed-off-by: Pavel Hrdina 
> > > ---
> > >  Makefile.am  |  3 ---
> > >  scripts/check-file-access.py | 24 +++-
> > >  tests/Makefile.am| 12 
> > >  tests/meson.build|  8 
> > >  tests/virtestmock.c  |  2 +-
> > >  5 files changed, 28 insertions(+), 21 deletions(-)
> > > 
> > > diff --git a/Makefile.am b/Makefile.am
> > > index 363c5cf66fd..d05a0c1a85a 100644
> > > --- a/Makefile.am
> > > +++ b/Makefile.am
> > > @@ -37,9 +37,6 @@ srpm: clean
> > >  
> > >  check-local: all tests
> > >  
> > > -check-access: all
> > > - @($(MAKE) $(AM_MAKEFLAGS) -C tests check-access)
> > > -
> > >  dist-hook: gen-AUTHORS
> > >  
> > >  .PHONY: gen-AUTHORS
> > > diff --git a/scripts/check-file-access.py b/scripts/check-file-access.py
> > > index aa120cafacf..f0e98f4b652 100755
> > > --- a/scripts/check-file-access.py
> > > +++ b/scripts/check-file-access.py
> > > @@ -21,15 +21,27 @@
> > >  #
> > >  #
> > >  
> > > +import os
> > > +import random
> > >  import re
> > > +import string
> > >  import sys
> > >  
> > > -if len(sys.argv) != 3:
> > > -print("syntax: %s ACCESS-FILE PERMITTED-ACCESS-FILE")
> > > -sys.exit(1)
> > > +abs_builddir = os.environ.get('abs_builddir', '')
> > > +abs_srcdir = os.environ.get('abs_srcdir', '')
> > >  
> > > -access_file = sys.argv[1]
> > > -permitted_file = sys.argv[2]
> > > +filename = ''.join(random.choice(string.ascii_letters) for _ in 
> > > range(16))
> > 
> > Umm, python doesn't have a 'mkostemp' equivalent? This seems a bit
> > fishy.
> 
> Sure, it is the tempfile module:
> https://docs.python.org/3/library/tempfile.html
> 
>   filename = tempfile.mkstemp(dir=abs_builddir, prefix='file-access-', 
> suffix='.txt')

I'll look into it. When porting it I just looked for any solution as
this can be changed any time after the series is pushed.

Thanks

Pavel


signature.asc
Description: PGP signature

Re: [libvirt PATCH 326/351] meson: docs: build news.html from news.xml

On Thu, Jul 16, 2020 at 11:59:22 +0200, Pavel Hrdina wrote:

Wrong subject. 'from NEWS.rst'.


> Signed-off-by: Pavel Hrdina 
> ---

Re: [libvirt PATCH 324/351] meson: docs: build .html files from .rst files

On Thu, Jul 16, 2020 at 11:59:20 +0200, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina 
> ---
>  docs/Makefile.am |  3 ---
>  docs/meson.build | 61 
>  2 files changed, 61 insertions(+), 3 deletions(-)

This and the previous commit can actually be optimized:

https://www.redhat.com/archives/libvir-list/2020-July/msg01627.html

Obviously it can be done later with my patch.

Re: [libvirt PATCH 312/351] meson: tests: add file access test setup

2020-07-28 Thread Pino Toscano

On Tuesday, 28 July 2020 14:56:45 CEST Peter Krempa wrote:
> On Thu, Jul 16, 2020 at 11:59:08 +0200, Pavel Hrdina wrote:
> > We need to modify check-file-access.py to be usable as wrapper for
> > libvirt tests. This way we can run the tests using this command:
> > 
> > meson test --setup access
> > 
> > which will run all tests using check-file-access.py as a wrapper.
> > 
> > With autotools all file access are written into single file for all
> > tests and compared once the whole test suite is done.
> > 
> > With Meson we will compare the file access after every single test
> > because it is used as wrapper now. That requires writing the file
> > access into separate files for every single test as they are executed
> > in parallel.
> > 
> > Since the wrapper is used for all tests in Meson including tests outside
> > of tests directory we have to check for presence of the output file.
> > We should also cleanup after ourselves.
> > 
> > Signed-off-by: Pavel Hrdina 
> > ---
> >  Makefile.am  |  3 ---
> >  scripts/check-file-access.py | 24 +++-
> >  tests/Makefile.am| 12 
> >  tests/meson.build|  8 
> >  tests/virtestmock.c  |  2 +-
> >  5 files changed, 28 insertions(+), 21 deletions(-)
> > 
> > diff --git a/Makefile.am b/Makefile.am
> > index 363c5cf66fd..d05a0c1a85a 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -37,9 +37,6 @@ srpm: clean
> >  
> >  check-local: all tests
> >  
> > -check-access: all
> > -   @($(MAKE) $(AM_MAKEFLAGS) -C tests check-access)
> > -
> >  dist-hook: gen-AUTHORS
> >  
> >  .PHONY: gen-AUTHORS
> > diff --git a/scripts/check-file-access.py b/scripts/check-file-access.py
> > index aa120cafacf..f0e98f4b652 100755
> > --- a/scripts/check-file-access.py
> > +++ b/scripts/check-file-access.py
> > @@ -21,15 +21,27 @@
> >  #
> >  #
> >  
> > +import os
> > +import random
> >  import re
> > +import string
> >  import sys
> >  
> > -if len(sys.argv) != 3:
> > -print("syntax: %s ACCESS-FILE PERMITTED-ACCESS-FILE")
> > -sys.exit(1)
> > +abs_builddir = os.environ.get('abs_builddir', '')
> > +abs_srcdir = os.environ.get('abs_srcdir', '')
> >  
> > -access_file = sys.argv[1]
> > -permitted_file = sys.argv[2]
> > +filename = ''.join(random.choice(string.ascii_letters) for _ in range(16))
> 
> Umm, python doesn't have a 'mkostemp' equivalent? This seems a bit
> fishy.

Sure, it is the tempfile module:
https://docs.python.org/3/library/tempfile.html

  filename = tempfile.mkstemp(dir=abs_builddir, prefix='file-access-', 
suffix='.txt')

-- 
Pino Toscano

signature.asc
Description: This is a digitally signed message part.

Re: [libvirt PATCH 321/351] meson: docs: introduce meson-html-gen.py helper

On Thu, Jul 16, 2020 at 11:59:17 +0200, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina 
> ---
>  docs/Makefile.am  | 26 -
>  scripts/meson-html-gen.py | 49 +++
>  scripts/meson.build   |  1 +
>  3 files changed, 50 insertions(+), 26 deletions(-)
>  create mode 100755 scripts/meson-html-gen.py
> 
> diff --git a/docs/Makefile.am b/docs/Makefile.am
> index a2fe2fbdc75..0c42db2badb 100644
> --- a/docs/Makefile.am
> +++ b/docs/Makefile.am
> @@ -320,32 +320,6 @@ news.html.in: $(top_srcdir)/NEWS.rst
>   $(AM_V_GEN)$(MKDIR_P) `dirname $@` && \
> $(RST2HTML) --strict $< > $@ || { rm $@ && exit 1; }
>  
> -%.html.tmp: %.html.in site.xsl subsite.xsl page.xsl \
> - $(acl_generated)
> - $(AM_V_GEN)name=`echo $@ | sed -e 's/.tmp//'`; \
> -   genhtmlin=`echo $@ | sed -e 's/.tmp/.in/'`; \
> -   rst=`echo $@ | sed -e 's/.html.tmp/.rst/'`; \
> -   src="$$genhtmlin"; \
> -   test -f "$$genhtmlin" && src="$$rst"; \
> -   dir=`dirname $@` ; \
> -   if test "$$dir" = "."; \
> -   then \
> - style=site.xsl; \
> -   else \
> - $(MKDIR_P) $$dir; \
> - style=subsite.xsl; \
> -   fi; \
> -   $(XSLTPROC) --stringparam pagename $$name \
> - --stringparam pagesrc $$src \
> - --stringparam builddir '$(abs_top_builddir)' \
> - --stringparam timestamp $(timestamp) --nonet \
> - $(top_srcdir)/docs/$$style $< > $@ \
> - || { rm $@ && exit 1; }
> -
> -%.html: %.html.tmp
> - $(AM_V_GEN)$(XMLLINT) --nonet --format $< > $@ \
> -   || { rm $@ && exit 1; }
> -
>  $(apihtml_generated): html/index.html
>  $(apiadminhtml_generated): html/index-admin.html
>  $(apiqemuhtml_generated): html/index-qemu.html
> diff --git a/scripts/meson-html-gen.py b/scripts/meson-html-gen.py
> new file mode 100755
> index 000..9ac649a9ef7
> --- /dev/null
> +++ b/scripts/meson-html-gen.py
> @@ -0,0 +1,49 @@
> +#!/usr/bin/env python3
> +
> +import argparse
> +import os
> +import subprocess
> +
> +parser = argparse.ArgumentParser()
> +parser.add_argument("xsltproc", type=str, help="path to xsltproc bin")
> +parser.add_argument("xmllint", type=str, help="path to xmllint bin")
> +parser.add_argument("builddir", type=str, help="build root dir path")
> +parser.add_argument("timestamp", type=str, help="docs timestamp")
> +parser.add_argument("style", type=str, help="XSL stile file")
> +parser.add_argument("infile", type=str, help="path to source HTML file")
> +parser.add_argument("htmlfile", type=str, help="path to generated HTML file")
> +args = parser.parse_args()
> +
> +name = os.path.basename(args.htmlfile).replace('.html', '')
> +
> +pagesrc = args.infile
> +rstfile = pagesrc.replace('.html.in', '.rst')
> +if os.path.exists(rstfile):
> +pagesrc = rstfile
> +
> +with open(args.infile, 'rb') as infile:
> +html_in_data = infile.read()
> +
> +html_tmp = subprocess.run(
> +[
> +args.xsltproc,
> +'--stringparam', 'pagename', name,
> +'--stringparam', 'pagesrc', pagesrc,
> +'--stringparam', 'builddir', args.builddir,
> +'--stringparam', 'timestamp', args.timestamp,
> +'--nonet', args.style, '-',
> +],
> +input=html_in_data,
> +stdout=subprocess.PIPE,
> +stderr=subprocess.PIPE,

xsltproc can take an input file as argument so if all of this is just
for feeding it the input, it's not necessary to do it so.


> +)
> +
> +html = subprocess.run(
> +[args.xmllint, '--nonet', '--format', '-'],
> +input=html_tmp.stdout,
> +stdout=subprocess.PIPE,
> +stderr=subprocess.PIPE,

We can then do this as separate stage.

Re: [libvirt PATCH 313/351] meson: tests: add valgrind test setup

On Thu, Jul 16, 2020 at 11:59:09 +0200, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina 
> ---
>  Makefile.am   |  4 +---
>  configure.ac  |  3 +--
>  tests/Makefile.am | 23 ---
>  tests/meson.build | 11 +++
>  4 files changed, 13 insertions(+), 28 deletions(-)
>  delete mode 100644 tests/Makefile.am
> 
> diff --git a/Makefile.am b/Makefile.am
> index d05a0c1a85a..549ade3db20 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -21,7 +21,7 @@
>  DISTCHECK_CONFIGURE_FLAGS = --enable-werror
>  
>  SUBDIRS = . docs \
> -  tests po examples
> +  po examples
>  
>  XZ_OPT ?= -v -T0
>  export XZ_OPT
> @@ -35,8 +35,6 @@ rpm: clean
>  srpm: clean
>   @(unset CDPATH ; $(MAKE) dist && rpmbuild -ts $(distdir).tar.xz)
>  
> -check-local: all tests
> -
>  dist-hook: gen-AUTHORS
>  
>  .PHONY: gen-AUTHORS
> diff --git a/configure.ac b/configure.ac
> index 20926ee9f19..78676d73d2b 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -115,6 +115,5 @@ AC_CONFIG_FILES([\
>  libvirt-admin.pc \
>  libvirt.spec mingw-libvirt.spec \
>  po/Makefile \
> -examples/Makefile \
> -tests/Makefile])
> +examples/Makefile])
>  AC_OUTPUT
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> deleted file mode 100644
> index 04c37ccda2e..000
> --- a/tests/Makefile.am
> +++ /dev/null
> @@ -1,23 +0,0 @@
> -## Process this file with automake to produce Makefile.in
> -
> -## Copyright (C) 2005-2019 Red Hat, Inc.
> -##
> -## This library is free software; you can redistribute it and/or
> -## modify it under the terms of the GNU Lesser General Public
> -## License as published by the Free Software Foundation; either
> -## version 2.1 of the License, or (at your option) any later version.
> -##
> -## This library is distributed in the hope that it will be useful,
> -## but WITHOUT ANY WARRANTY; without even the implied warranty of
> -## MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> -## Lesser General Public License for more details.
> -##
> -## You should have received a copy of the GNU Lesser General Public
> -## License along with this library.  If not, see
> -## .
> -
> -VALGRIND = valgrind --quiet --leak-check=full --trace-children=yes \
> - 
> --trace-children-skip="*/tools/virsh","*/tests/commandhelper","/usr/bin/*" \
> - --suppressions=$(abs_srcdir)/.valgrind.supp
> -valgrind:
> - $(MAKE) check VG="$(LIBTOOL) --mode=execute $(VALGRIND)"
> diff --git a/tests/meson.build b/tests/meson.build
> index cf848678505..1510d6ef3f3 100644
> --- a/tests/meson.build
> +++ b/tests/meson.build
> @@ -673,3 +673,14 @@ add_test_setup(
>],
>exe_wrapper: [ meson_python_prog, check_file_access_prog ],
>  )
> +
> +add_test_setup(
> +  'valgrind',
> +  exe_wrapper: [
> +'valgrind', '--quiet', '--leak-check=full', '--trace-children=yes',
> +'--trace-children-skip="*/tools/virsh,*/tests/commandhelper,/usr/bin/*"',
> +'--suppressions=@0@'.format(meson.current_source_dir() / 
> '.valgrind.supp'),
> +'--error-exitcode=1',
> +  ],
> +  timeout_multiplier: 4,

Please add a note that the default timeout for meson tests is 30 seconds
and that this is used to increase it for potentially long-running tests.

Re: [libvirt PATCH 312/351] meson: tests: add file access test setup

On Thu, Jul 16, 2020 at 11:59:08 +0200, Pavel Hrdina wrote:
> We need to modify check-file-access.py to be usable as wrapper for
> libvirt tests. This way we can run the tests using this command:
> 
> meson test --setup access
> 
> which will run all tests using check-file-access.py as a wrapper.
> 
> With autotools all file access are written into single file for all
> tests and compared once the whole test suite is done.
> 
> With Meson we will compare the file access after every single test
> because it is used as wrapper now. That requires writing the file
> access into separate files for every single test as they are executed
> in parallel.
> 
> Since the wrapper is used for all tests in Meson including tests outside
> of tests directory we have to check for presence of the output file.
> We should also cleanup after ourselves.
> 
> Signed-off-by: Pavel Hrdina 
> ---
>  Makefile.am  |  3 ---
>  scripts/check-file-access.py | 24 +++-
>  tests/Makefile.am| 12 
>  tests/meson.build|  8 
>  tests/virtestmock.c  |  2 +-
>  5 files changed, 28 insertions(+), 21 deletions(-)
> 
> diff --git a/Makefile.am b/Makefile.am
> index 363c5cf66fd..d05a0c1a85a 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -37,9 +37,6 @@ srpm: clean
>  
>  check-local: all tests
>  
> -check-access: all
> - @($(MAKE) $(AM_MAKEFLAGS) -C tests check-access)
> -
>  dist-hook: gen-AUTHORS
>  
>  .PHONY: gen-AUTHORS
> diff --git a/scripts/check-file-access.py b/scripts/check-file-access.py
> index aa120cafacf..f0e98f4b652 100755
> --- a/scripts/check-file-access.py
> +++ b/scripts/check-file-access.py
> @@ -21,15 +21,27 @@
>  #
>  #
>  
> +import os
> +import random
>  import re
> +import string
>  import sys
>  
> -if len(sys.argv) != 3:
> -print("syntax: %s ACCESS-FILE PERMITTED-ACCESS-FILE")
> -sys.exit(1)
> +abs_builddir = os.environ.get('abs_builddir', '')
> +abs_srcdir = os.environ.get('abs_srcdir', '')
>  
> -access_file = sys.argv[1]
> -permitted_file = sys.argv[2]
> +filename = ''.join(random.choice(string.ascii_letters) for _ in range(16))

Umm, python doesn't have a 'mkostemp' equivalent? This seems a bit
fishy.


> +access_file = os.path.join(abs_builddir, 
> 'file-access-{0}.txt'.format(filename))
> +permitted_file = os.path.join(abs_srcdir, 'permitted_file_access.txt')
> +
> +os.environ['VIR_TEST_FILE_ACCESS_OUTPUT'] = access_file
> +
> +test = ' '.join(sys.argv[1:])
> +
> +ret = os.system(test)
> +
> +if ret != 0 or not os.is_file(access_file):
> +sys.exit(ret)
>  
>  known_actions = ["open", "fopen", "access", "stat", "lstat", "connect"]
>  
> @@ -120,6 +132,8 @@ for file in files:
>  print(": %s" % file["testname"], end="")
>  print("")
>  
> +os.remove(access_file)

Wouldn't it make sense to keep this file on failure?

> +
>  if err:
>  sys.exit(1)
>  sys.exit(0)

Re: [libvirt PATCH 290/351] meson: tests: add ESX specific tests

On Tue, Jul 28, 2020 at 02:06:08PM +0200, Peter Krempa wrote:
> On Thu, Jul 16, 2020 at 11:58:46 +0200, Pavel Hrdina wrote:
> > Signed-off-by: Pavel Hrdina 
> > ---
> >  src/esx/meson.build |  2 ++
> >  tests/Makefile.am   | 14 --
> >  tests/meson.build   |  6 ++
> >  3 files changed, 8 insertions(+), 14 deletions(-)
> > 
> > diff --git a/src/esx/meson.build b/src/esx/meson.build
> > index 1718411d759..469c2044c63 100644
> > --- a/src/esx/meson.build
> > +++ b/src/esx/meson.build
> > @@ -63,3 +63,5 @@ if conf.has('WITH_ESX')
> >  else
> >sym_files += 'libvirt_esx.syms'
> >  endif
> > +
> > +esx_inc_dir = include_directories('.')
> > diff --git a/tests/Makefile.am b/tests/Makefile.am
> > index b721b135cfd..55369717cd9 100644
> > --- a/tests/Makefile.am
> > +++ b/tests/Makefile.am
> > @@ -70,10 +70,6 @@ if WITH_OPENVZ
> >  test_programs += openvzutilstest
> >  endif WITH_OPENVZ
> >  
> > -if WITH_ESX
> > -test_programs += esxutilstest
> > -endif WITH_ESX
> > -
> >  if WITH_VBOX
> >  test_programs += vboxsnapshotxmltest
> >  endif WITH_VBOX
> > @@ -375,16 +371,6 @@ openvzutilstest_LDADD = $(LDADDS) \
> > ../src/libvirt_driver_openvz.la
> >  endif ! WITH_OPENVZ
> >  
> > -if WITH_ESX
> > -esxutilstest_SOURCES = \
> > -   esxutilstest.c \
> > -   testutils.c testutils.h
> > -esxutilstest_LDADD = $(LDADDS)
> > -esxutilstest_CFLAGS = \
> > -   -I$(top_builddir)/src/esx \
> > -   $(AM_CFLAGS)
> > -endif ! WITH_ESX
> > -
> >  if WITH_VBOX
> >  vboxsnapshotxmltest_SOURCES = \
> > vboxsnapshotxmltest.c \
> > diff --git a/tests/meson.build b/tests/meson.build
> > index 86f12a58f7b..1eded8454f1 100644
> > --- a/tests/meson.build
> > +++ b/tests/meson.build
> > @@ -356,6 +356,12 @@ if conf.has('WITH_DBUS')
> >endif
> >  endif
> >  
> > +if conf.has('WITH_ESX')
> > +  tests += [
> > +{ 'name': 'esxutilstest', 'include': [ esx_inc_dir ], 'deps': [ 
> > dbus_dep ] },
> 
> dbus? that doesn't seem right.

No idea how it got here, it's not correct. I'll fix it.

Thanks

Pavel


signature.asc
Description: PGP signature

Re: [libvirt PATCH 290/351] meson: tests: add ESX specific tests

On Thu, Jul 16, 2020 at 11:58:46 +0200, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina 
> ---
>  src/esx/meson.build |  2 ++
>  tests/Makefile.am   | 14 --
>  tests/meson.build   |  6 ++
>  3 files changed, 8 insertions(+), 14 deletions(-)
> 
> diff --git a/src/esx/meson.build b/src/esx/meson.build
> index 1718411d759..469c2044c63 100644
> --- a/src/esx/meson.build
> +++ b/src/esx/meson.build
> @@ -63,3 +63,5 @@ if conf.has('WITH_ESX')
>  else
>sym_files += 'libvirt_esx.syms'
>  endif
> +
> +esx_inc_dir = include_directories('.')
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index b721b135cfd..55369717cd9 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -70,10 +70,6 @@ if WITH_OPENVZ
>  test_programs += openvzutilstest
>  endif WITH_OPENVZ
>  
> -if WITH_ESX
> -test_programs += esxutilstest
> -endif WITH_ESX
> -
>  if WITH_VBOX
>  test_programs += vboxsnapshotxmltest
>  endif WITH_VBOX
> @@ -375,16 +371,6 @@ openvzutilstest_LDADD = $(LDADDS) \
>   ../src/libvirt_driver_openvz.la
>  endif ! WITH_OPENVZ
>  
> -if WITH_ESX
> -esxutilstest_SOURCES = \
> - esxutilstest.c \
> - testutils.c testutils.h
> -esxutilstest_LDADD = $(LDADDS)
> -esxutilstest_CFLAGS = \
> - -I$(top_builddir)/src/esx \
> - $(AM_CFLAGS)
> -endif ! WITH_ESX
> -
>  if WITH_VBOX
>  vboxsnapshotxmltest_SOURCES = \
>   vboxsnapshotxmltest.c \
> diff --git a/tests/meson.build b/tests/meson.build
> index 86f12a58f7b..1eded8454f1 100644
> --- a/tests/meson.build
> +++ b/tests/meson.build
> @@ -356,6 +356,12 @@ if conf.has('WITH_DBUS')
>endif
>  endif
>  
> +if conf.has('WITH_ESX')
> +  tests += [
> +{ 'name': 'esxutilstest', 'include': [ esx_inc_dir ], 'deps': [ dbus_dep 
> ] },

dbus? that doesn't seem right.

> +  ]

Re: [PATCH] Qemu: migration: Not bind RAM info with active migration status

2020-07-28 Thread zhukeqian




On 2020/7/28 19:44, Daniel P. Berrangé wrote:
> On Tue, Jul 28, 2020 at 07:35:11PM +0800, zhukeqian wrote:
>>
>>
>> On 2020/7/28 18:32, Daniel P. Berrangé wrote:
>>> On Wed, Jul 15, 2020 at 02:18:01PM +0800, Keqian Zhu wrote:
 For that Qemu supports returning incoming migration info since its commit
 65ace0604551 (migration: add postcopy total blocktime into query-migrate),
 which may contains active status, but without RAM info. Drop this binding
 relationship check in libvirt.
>>>
>>> I'm not clear from this description what the actual bug in libvirt
>>> is ?   What currently fails in libvirt that this patch fixes ?
>>>
>> When query migration status, libvirt assumes that when migration status is 
>> active,
>> then RAM info must be set. However qemu has changed this assumption, so 
>> libvirt will
>> report "migration was active, but no RAM info was set" when query migration 
>> info.
> 
> So you're trying to make it possible to query migration stats on the
> destination host ?  IOW, this is a new feature, rather than a bug
> fix ?
Yes, actually I do not obviously mention it as bug fix in patch message.

Thanks,
Keqian
> 
> Regards,
> Daniel
>

[libvirt PATCH 9/9] util: netdevip: remove unused VIR_NETDEV_FAMILY

Signed-off-by: Ján Tomko 
Fixes: cf0568b0af4ef4f903da1b184fc9b232c6f29457
---
 src/util/virnetdevip.c | 5 -
 1 file changed, 5 deletions(-)

diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c
index 8b85c7beca..409f062c5c 100644
--- a/src/util/virnetdevip.c
+++ b/src/util/virnetdevip.c
@@ -43,11 +43,6 @@
 #ifdef __linux__
 # include 
 # include 
-# define VIR_NETDEV_FAMILY AF_UNIX
-#elif defined(HAVE_STRUCT_IFREQ) && defined(AF_LOCAL)
-# define VIR_NETDEV_FAMILY AF_LOCAL
-#else
-# undef HAVE_STRUCT_IFREQ
 #endif
 
 #define VIR_DAD_WAIT_TIMEOUT 20 /* seconds */
-- 
2.26.2

[libvirt PATCH 7/9] virt-admin: remove unused VIRT_ADMIN_TIME_BUFLEN

The switch to GDateTime removed the last use.

Signed-off-by: Ján Tomko 
Fixes: 3caa28dc50df7ec215713075d669b20bef6473a2
---
 tools/virt-admin.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/tools/virt-admin.c b/tools/virt-admin.c
index fef0332a0d..cc136397bd 100644
--- a/tools/virt-admin.c
+++ b/tools/virt-admin.c
@@ -42,9 +42,6 @@
 
 #define VIRT_ADMIN_PROMPT "virt-admin # "
 
-/* we don't need precision to milliseconds in this module */
-#define VIRT_ADMIN_TIME_BUFLEN VIR_TIME_STRING_BUFLEN - 3
-
 static char *progname;
 
 static const vshCmdGrp cmdGroups[];
-- 
2.26.2

[libvirt PATCH 6/9] virsh: remove unused FILTER macro

Signed-off-by: Ján Tomko 
Fixes: c2dd9ddf7b18ebfb108623d4711c80b716a261d7
---
 tools/virsh-network.c | 4 
 1 file changed, 4 deletions(-)

diff --git a/tools/virsh-network.c b/tools/virsh-network.c
index ed90736345..b10cd03bcc 100644
--- a/tools/virsh-network.c
+++ b/tools/virsh-network.c
@@ -1744,9 +1744,6 @@ static const vshCmdOptDef opts_network_port_list[] = {
 {.name = NULL}
 };
 
-#define FILTER(NAME, FLAG) \
-if (vshCommandOptBool(cmd, NAME)) \
-flags |= (FLAG)
 static bool
 cmdNetworkPortList(vshControl *ctl, const vshCmd *cmd)
 {
@@ -1802,7 +1799,6 @@ cmdNetworkPortList(vshControl *ctl, const vshCmd *cmd)
 virshNetworkPortListFree(list);
 return ret;
 }
-#undef FILTER
 
 
 const vshCmdDef networkCmds[] = {
-- 
2.26.2

[libvirt PATCH 3/9] storage: scsi: remove unused LINUX_SYSFS_SCSI_HOST_POSTFIX

Unused since its introduction.

Signed-off-by: Ján Tomko 
Fixes: 81d0ffbc3baee53467f664ea03b1349e8b6dba5a
---
 src/storage/storage_backend_scsi.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/storage/storage_backend_scsi.c 
b/src/storage/storage_backend_scsi.c
index 37ae4325ca..e528d7622c 100644
--- a/src/storage/storage_backend_scsi.c
+++ b/src/storage/storage_backend_scsi.c
@@ -41,7 +41,6 @@
 VIR_LOG_INIT("storage.storage_backend_scsi");
 
 #define LINUX_SYSFS_SCSI_HOST_PREFIX "/sys/class/scsi_host"
-#define LINUX_SYSFS_SCSI_HOST_POSTFIX "device"
 #define LINUX_SYSFS_SCSI_HOST_SCAN_STRING "- - -"
 
 typedef struct _virStoragePoolFCRefreshInfo virStoragePoolFCRefreshInfo;
-- 
2.26.2

[libvirt PATCH 0/9] Remove some unused macros

I built with -Wunused-macros again after five years [0]
the good news is we don't really have that many -
most of them are VIR_FROM_THIS or macros we have included
for completeness.

https://gitlab.com/jano.tomko/libvirt/-/pipelines/171383854

[0] a0482396d7e0b164e71b2bdd881d9db5c6521303

Ján Tomko (9):
  locking: sanlock: drop unused LOCKSPACE_SLEEP
  storage: logical: drop unused PV_BLANK_SECTOR_SIZE
  storage: scsi: remove unused LINUX_SYSFS_SCSI_HOST_POSTFIX
  test: remove unused NUM_CELLS
  util: remove unused VIR_MCAST_ADDR_LEN
  virsh: remove unused FILTER macro
  virt-admin: remove unused VIRT_ADMIN_TIME_BUFLEN
  util: vportprofile: remove unused constants
  util: netdevip: remove unused VIR_NETDEV_FAMILY

 src/locking/lock_driver_sanlock.c | 2 --
 src/storage/storage_backend_logical.c | 2 --
 src/storage/storage_backend_scsi.c| 1 -
 src/test/test_driver.c| 2 --
 src/util/virnetdev.c  | 1 -
 src/util/virnetdevip.c| 5 -
 src/util/virnetdevvportprofile.c  | 4 
 tools/virsh-network.c | 4 
 tools/virt-admin.c| 3 ---
 9 files changed, 24 deletions(-)

-- 
2.26.2

[libvirt PATCH 4/9] test: remove unused NUM_CELLS

Signed-off-by: Ján Tomko 
Fixes: 2bd7ed78a7cd68775de8cb912c7a57db7721a10b
---
 src/test/test_driver.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 993f405f3c..01c4675c30 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -83,8 +83,6 @@ struct _testCell {
 typedef struct _testCell testCell;
 typedef struct _testCell *testCellPtr;
 
-#define MAX_CELLS 128
-
 struct _testAuth {
 char *username;
 char *password;
-- 
2.26.2

[libvirt PATCH 1/9] locking: sanlock: drop unused LOCKSPACE_SLEEP

After dropping support for sanlock < 2.4,
this constant is no longer used.

Signed-off-by: Ján Tomko 
Fixes: c4951694786ecd45424769979762c17e4c8e56d0
---
 src/locking/lock_driver_sanlock.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/src/locking/lock_driver_sanlock.c 
b/src/locking/lock_driver_sanlock.c
index f414e3ec07..d4816030cf 100644
--- a/src/locking/lock_driver_sanlock.c
+++ b/src/locking/lock_driver_sanlock.c
@@ -185,8 +185,6 @@ 
virLockManagerSanlockInitLockspace(virLockManagerSanlockDriverPtr driver,
 return ret;
 }
 
-/* How much ms sleep before retrying to add a lockspace? */
-#define LOCKSPACE_SLEEP 100
 /* How many times try adding a lockspace? */
 #define LOCKSPACE_RETRIES 10
 
-- 
2.26.2

[libvirt PATCH 8/9] util: vportprofile: remove unused constants

After the switch to libnl these are no longer used.

Signed-off-by: Ján Tomko 
Fixes: 77e7c13b2ef95724c39395d92725b1a892ff84de
---
 src/util/virnetdevvportprofile.c | 4 
 1 file changed, 4 deletions(-)

diff --git a/src/util/virnetdevvportprofile.c b/src/util/virnetdevvportprofile.c
index ef372aca22..020683fa04 100644
--- a/src/util/virnetdevvportprofile.c
+++ b/src/util/virnetdevvportprofile.c
@@ -63,10 +63,6 @@ VIR_LOG_INIT("util.netdevvportprofile");
 
 # define MICROSEC_PER_SEC   (1000 * 1000)
 
-# define NLMSGBUF_SIZE  256
-# define RATTBUF_SIZE   64
-
-
 # define STATUS_POLL_TIMEOUT_USEC (10 * MICROSEC_PER_SEC)
 # define STATUS_POLL_INTERVL_USEC (MICROSEC_PER_SEC / 8)
 
-- 
2.26.2

[libvirt PATCH 2/9] storage: logical: drop unused PV_BLANK_SECTOR_SIZE

Signed-off-by: Ján Tomko 
Fixes: d942bf6e9e8e3991808ca5185098257e84acab5d
---
 src/storage/storage_backend_logical.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/src/storage/storage_backend_logical.c 
b/src/storage/storage_backend_logical.c
index 60958d3d2c..43cf3a1598 100644
--- a/src/storage/storage_backend_logical.c
+++ b/src/storage/storage_backend_logical.c
@@ -40,8 +40,6 @@
 
 VIR_LOG_INIT("storage.storage_backend_logical");
 
-#define PV_BLANK_SECTOR_SIZE 512
-
 
 static int
 virStorageBackendLogicalSetActive(virStoragePoolObjPtr pool,
-- 
2.26.2

[libvirt PATCH 5/9] util: remove unused VIR_MCAST_ADDR_LEN

Unused since its introduction.

Signed-off-by: Ján Tomko 
Fixes: cc0e8c244d080f56392278e836cc378ba848e7aa
---
 src/util/virnetdev.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 41dc5bd4c4..b42fa86672 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -72,7 +72,6 @@ VIR_LOG_INIT("util.netdev");
 #define MAX_MCAST_SIZE 50*14336
 #define VIR_MCAST_NAME_LEN (IFNAMSIZ + 1)
 #define VIR_MCAST_TOKEN_DELIMS " \n"
-#define VIR_MCAST_ADDR_LEN (VIR_MAC_HEXLEN + 1)
 
 #if defined(SIOCSIFFLAGS) && defined(HAVE_STRUCT_IFREQ)
 # define VIR_IFF_UP IFF_UP
-- 
2.26.2

Re: [libvirt PATCH 000/351] port libvirt to Meson build system


On a Tuesday in 2020, Daniel P. Berrangé wrote:

On Tue, Jul 28, 2020 at 12:24:56PM +0200, Ján Tomko wrote:

For unattended bisects, it would be nice to return 125 which is the
magic value meaning 'skip' to 'git bisect run'.


I don't think its possible to force the ninja/meson exit status
upon failure. I think at best uou could have a wrapper script
"mymeson" that looks for this error message and returns 125.



  meson test

returns 125 on a build failure, that might be helpful.

Jano


signature.asc
Description: PGP signature

Re: [libvirt PATCH] ci: move sanity_checks after other stages

2020-07-28 Thread Daniel P . Berrangé

On Tue, Jul 28, 2020 at 01:42:02PM +0200, Ján Tomko wrote:
> Be less hostile to developers and run the build jobs even
> if the dco-check failed. That way they can test their own
> "private" branches without a sign-off.
> 
> Also specify empty `needs` for it, since it does not depend
> on any of the jobs in the container phase and can be run
> right away.
> 
> Signed-off-by: Ján Tomko 
> ---
> A failed build still runs DCO:
> https://gitlab.com/jano.tomko/libvirt/-/pipelines/171565689
> and vice-versa:
> https://gitlab.com/jano.tomko/libvirt/-/pipelines/171582293
> 
>  .gitlab-ci.yml | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé 


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] Qemu: migration: Not bind RAM info with active migration status

2020-07-28 Thread Daniel P . Berrangé

On Tue, Jul 28, 2020 at 07:35:11PM +0800, zhukeqian wrote:
> 
> 
> On 2020/7/28 18:32, Daniel P. Berrangé wrote:
> > On Wed, Jul 15, 2020 at 02:18:01PM +0800, Keqian Zhu wrote:
> >> For that Qemu supports returning incoming migration info since its commit
> >> 65ace0604551 (migration: add postcopy total blocktime into query-migrate),
> >> which may contains active status, but without RAM info. Drop this binding
> >> relationship check in libvirt.
> > 
> > I'm not clear from this description what the actual bug in libvirt
> > is ?   What currently fails in libvirt that this patch fixes ?
> > 
> When query migration status, libvirt assumes that when migration status is 
> active,
> then RAM info must be set. However qemu has changed this assumption, so 
> libvirt will
> report "migration was active, but no RAM info was set" when query migration 
> info.

So you're trying to make it possible to query migration stats on the
destination host ?  IOW, this is a new feature, rather than a bug
fix ?

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 :|

[libvirt PATCH] ci: move sanity_checks after other stages

Be less hostile to developers and run the build jobs even
if the dco-check failed. That way they can test their own
"private" branches without a sign-off.

Also specify empty `needs` for it, since it does not depend
on any of the jobs in the container phase and can be run
right away.

Signed-off-by: Ján Tomko 
---
A failed build still runs DCO:
https://gitlab.com/jano.tomko/libvirt/-/pipelines/171565689
and vice-versa:
https://gitlab.com/jano.tomko/libvirt/-/pipelines/171582293

 .gitlab-ci.yml | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 702198ec8e..f97c96dd21 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -2,9 +2,9 @@ variables:
   GIT_DEPTH: 100
 
 stages:
-  - sanity_checks
   - containers
   - builds
+  - sanity_checks
 
 .script_variables: _variables |
   export MAKEFLAGS="-j$(getconf _NPROCESSORS_ONLN)"
@@ -481,6 +481,7 @@ potfile:
 # merge requests are submitted
 check-dco:
   stage: sanity_checks
+  needs: []
   image: registry.gitlab.com/libvirt/libvirt-ci/check-dco:master
   script:
 - /check-dco
-- 
2.26.2

Re: [PATCH] Qemu: migration: Not bind RAM info with active migration status

2020-07-28 Thread zhukeqian




On 2020/7/28 18:32, Daniel P. Berrangé wrote:
> On Wed, Jul 15, 2020 at 02:18:01PM +0800, Keqian Zhu wrote:
>> For that Qemu supports returning incoming migration info since its commit
>> 65ace0604551 (migration: add postcopy total blocktime into query-migrate),
>> which may contains active status, but without RAM info. Drop this binding
>> relationship check in libvirt.
> 
> I'm not clear from this description what the actual bug in libvirt
> is ?   What currently fails in libvirt that this patch fixes ?
> 
When query migration status, libvirt assumes that when migration status is 
active,
then RAM info must be set. However qemu has changed this assumption, so libvirt 
will
report "migration was active, but no RAM info was set" when query migration 
info.

Thanks,
Keqian
>>
>> Signed-off-by: Keqian Zhu 
>> ---
>>  src/qemu/qemu_monitor_json.c | 88 +---
>>  1 file changed, 42 insertions(+), 46 deletions(-)
>>
>> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
>> index d808c4b55b..ba8e340742 100644
>> --- a/src/qemu/qemu_monitor_json.c
>> +++ b/src/qemu/qemu_monitor_json.c
>> @@ -3547,56 +3547,52 @@ 
>> qemuMonitorJSONGetMigrationStatsReply(virJSONValuePtr reply,
>>  case QEMU_MONITOR_MIGRATION_STATUS_PRE_SWITCHOVER:
>>  case QEMU_MONITOR_MIGRATION_STATUS_DEVICE:
>>  ram = virJSONValueObjectGetObject(ret, "ram");
>> -if (!ram) {
>> -virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> -   _("migration was active, but no RAM info was 
>> set"));
>> -return -1;
>> -}
>> +if (ram) {
>> +if (virJSONValueObjectGetNumberUlong(ram, "transferred",
>> + >ram_transferred) < 
>> 0) {
>> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> +   _("migration was active, but RAM 
>> 'transferred' "
>> + "data was missing"));
>> +return -1;
>> +}
>> +if (virJSONValueObjectGetNumberUlong(ram, "remaining",
>> + >ram_remaining) < 
>> 0) {
>> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> +   _("migration was active, but RAM 'remaining' 
>> "
>> + "data was missing"));
>> +return -1;
>> +}
>> +if (virJSONValueObjectGetNumberUlong(ram, "total",
>> + >ram_total) < 0) {
>> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> +   _("migration was active, but RAM 'total' "
>> + "data was missing"));
>> +return -1;
>> +}
>>  
>> -if (virJSONValueObjectGetNumberUlong(ram, "transferred",
>> - >ram_transferred) < 0) {
>> -virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> -   _("migration was active, but RAM 'transferred' "
>> - "data was missing"));
>> -return -1;
>> -}
>> -if (virJSONValueObjectGetNumberUlong(ram, "remaining",
>> - >ram_remaining) < 0) {
>> -virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> -   _("migration was active, but RAM 'remaining' "
>> - "data was missing"));
>> -return -1;
>> -}
>> -if (virJSONValueObjectGetNumberUlong(ram, "total",
>> - >ram_total) < 0) {
>> -virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> -   _("migration was active, but RAM 'total' "
>> - "data was missing"));
>> -return -1;
>> -}
>> +if (virJSONValueObjectGetNumberDouble(ram, "mbps", ) == 0 
>> &&
>> +mbps > 0) {
>> +/* mpbs from QEMU reports Mbits/s (M as in 10^6 not Mi as 
>> 2^20) */
>> +stats->ram_bps = mbps * (1000 * 1000 / 8);
>> +}
>>  
>> -if (virJSONValueObjectGetNumberDouble(ram, "mbps", ) == 0 &&
>> -mbps > 0) {
>> -/* mpbs from QEMU reports Mbits/s (M as in 10^6 not Mi as 2^20) 
>> */
>> -stats->ram_bps = mbps * (1000 * 1000 / 8);
>> +if (virJSONValueObjectGetNumberUlong(ram, "duplicate",
>> + >ram_duplicate) == 
>> 0)
>> +stats->ram_duplicate_set = true;
>> +ignore_value(virJSONValueObjectGetNumberUlong(ram, "normal",
>> +  
>> >ram_normal));
>> +ignore_value(virJSONValueObjectGetNumberUlong(ram, 
>> "normal-bytes",
>> +

Re: [libvirt PATCH 000/351] port libvirt to Meson build system

On Tue, Jul 28, 2020 at 12:15:25PM +0200, Peter Krempa wrote:
> On Tue, Jul 28, 2020 at 10:33:52 +0200, Pavel Hrdina wrote:
> > On Tue, Jul 28, 2020 at 10:09:42AM +0200, Peter Krempa wrote:
> 
> [...]
> 
> > 
> > Here I disagree, it also means that compilation should not fail. We do
> > comments all the time to patch series that every single commit should
> > compile correctly on it's own within the series.
> 
> Doing a 'return 0' is not compiling code correctly. The idea of
> requirements to bulild cleanly is that you can test the code.
> 
> In this case we need to observe it from a different angle though.
> 
> In reality an incomplete build is a failed build regardless of what the
> return value of the build system is. And that is important here. The
> main reason for the build system is to build everything so the only
> success is when everything is built.
> 
> If you don't have the resulting binary it's impossible to test the code
> or do anything else.
> 
> What would be even worse is to get a compiled binary that e.g. doesn't
> have the dependencies installed (such as RNG schemas, cpu XML docs and
> such) and fails in magic ways. At that point you can't be sure whether
> it's the bug you are trying to locate or just plainly broken build
> which the build system lied to you that it's complete.

Obviously this would be a big issue if it would randomly fail because of
some missing files that are not yet compiled. I have no objection here.

If your only use-case is to run binaries during git bisect then yes
having not-complete build when build system returns 0 is not correct.

I'm just saying there might be some other use-cases for git bisect
where having incomplete build is not an issue and we should not dismiss
these use-case.

Pavel


signature.asc
Description: PGP signature

Re: [PATCH] doc: fix name of file containing max number of VFs

2020-07-28 Thread Daniel Henrique Barboza





On 7/28/20 6:16 AM, Paulo de Rezende Pinatti wrote:

Signed-off-by: Paulo de Rezende Pinatti 
---
  docs/formatnode.html.in | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/formatnode.html.in b/docs/formatnode.html.in
index e4328fedbe..8a51c4da80 100644
--- a/docs/formatnode.html.in
+++ b/docs/formatnode.html.in
@@ -141,7 +141,7 @@
  In this case this device is an SRIOV PF, and the 
capability
  element will have a list of address
  subelements, one for each VF on this PF. If the host 
system
-supports reporting it (via the "sriov_maxvfs" file in the
+supports reporting it (via the "sriov_totalvfs" file in the


I checked in the Linux kernel and there is no reference to 'sriov_maxvfs'.
This means that it's not likely to be something that it's one way in
s390x/powerpc but another way in x86.

Also, googling 'sriov_maxvfs' gives 3 results only. First result isn't
related to a sriov_maxvfs file, the remaining results are links to this
Libvirt documentation. So yeah.


Reviewed-by: Daniel Henrique Barboza 




  device's sysfs directory) the capability element will also
  have an attribute named maxCount which is the
  maximum number of SRIOV VFs supported by this device, 
which

Re: [PATCH] Qemu: migration: Not bind RAM info with active migration status

2020-07-28 Thread zhukeqian

Hi Daniel,

On 2020/7/28 18:21, Daniel Henrique Barboza wrote:
> 
> 
> On 7/28/20 5:46 AM, zhukeqian wrote:
>> Hi Daniel,
>>
>> On 2020/7/17 22:33, Daniel Henrique Barboza wrote:
>>>
>>>
>>> On 7/15/20 3:18 AM, Keqian Zhu wrote:
 For that Qemu supports returning incoming migration info since its commit
 65ace0604551 (migration: add postcopy total blocktime into query-migrate),
>>>
>>>
>>> It is worth saying that this QEMU commit that decoupled the RAM
>>> status from the active migration status is live since early 2018:
>>>
>>> $ git show 65ace0604551
>>> commit 65ace060455122a461cdc9302238b914084bcd42
>>> Author: Alexey Perevalov 
>>> Date:   Thu Mar 22 21:17:27 2018 +0300
>>>
>>>  migration: add postcopy total blocktime into query-migrate
>>>
>>> $ git describe 65ace0604551
>>> v2.12.0-6-g65ace06045
>>>
>>>
>>> I am not sure if we care about removing a migration failure check for
>>> QEMU 2.12 when we're waiting for 5.1 to come out. My guess is that we
>>> do care, but  not enough to demand a "if (QEMU <= 2.12)" in this logic.
>>> I'll also assume that the existing failure check is doing more harm than
>>> good nowadays, so:
>>>
>>>
>>> Reviewed-by: Daniel Henrique Barboza 
>>>
>> Do you have authority to merge this patch?
> 
> I don't have the authority to merge this in.
> 
> We're on code freeze for release 6.6.0 at this moment, meaning that only
> urgent fixes will make it upstream during this time frame. I believe a
> commiter/maintainer will be able to review/merge this patch shortly
> after that.
OK, I see. Thanks :-)
> 
> 
> Thanks,
> 
> 
> DHB
> 
>>
>> Thanks,
>> Keqian
>>
>

Re: [libvirt PATCH 281/351] meson: tests: build commandhelper binary