Re: [libvirt PATCH v3 0/2] Refuse PCI Address for ramfb device definition

[Laine Stump]

On 6/25/20 4:18 PM, Jonathon Jongsma wrote:

Changes in this version:
  - Add the test case input file
  - modify the test itself to properly fail when an input file is missing.

Jonathon Jongsma (2):
   qemu: ramfb video device doesn't support PCI address
   tests: ensure failure if input file doesn't exist

  src/qemu/qemu_validate.c  |  8 +
  ...video-ramfb-display-device-pci-address.xml | 30 +++
  tests/qemuxml2argvtest.c  |  7 +
  3 files changed, 45 insertions(+)
  create mode 100644 
tests/qemuxml2argvdata/video-ramfb-display-device-pci-address.xml



For both:

Reviewed-by: Laine Stump 

and pushed (...nd I just realized I forgot to add the Reviewed-by: 
tag before pushing :-/. Ah well, it shows me as the committer, so you 
won't have to *totally* take the fall if something blows up :-))

Re: [PATCH 16/25] squash into 'network: convert local pointers to g_auto*'

[Laine Stump]

On 6/25/20 7:34 PM, Ján Tomko wrote:

On a Wednesday in 2020, Laine Stump wrote:

OOPS!!

I meant to squash this into patch 10 before posting. If you want to 
just review it separately I can squash it in before push. Or if you 
want to be pedantic I can squash it in and resend :-)




To me, these seem like changes unrelated to patch 10 and deserve their
own commit.



Well, they're all related to the cleanup that naturally follows from 
making a pointer g_autofree - 1) you need to initialize it when you 
define it (and so you might as well initialize it with the first real 
value it's going to get, as long as that value would end up *always* 
being assigned anyway, and 2) code reduction related to removing 
now-empty cleanup: label. But I see you just responded to patch 10 
saying you thought the patch was too long and should be split, so I 
suppose I could make this the 2nd of the 2 that you suggest.





Jano


On 6/24/20 11:34 PM, Laine Stump wrote:

Signed-off-by: Laine Stump 
---
 src/network/bridge_driver_linux.c | 9 ++---
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/src/network/bridge_driver_linux.c 
b/src/network/bridge_driver_linux.c

index 0d0ac730f2..7f765bcf99 100644
--- a/src/network/bridge_driver_linux.c
+++ b/src/network/bridge_driver_linux.c
@@ -834,7 +834,7 @@ int networkAddFirewallRules(virNetworkDefPtr def)
 {
 size_t i;
 virNetworkIPDefPtr ipdef;
-    g_autoptr(virFirewall) fw = NULL;
+    g_autoptr(virFirewall) fw = virFirewallNew();
 if (virOnce(, networkSetupPrivateChains) < 0)
 return -1;
@@ -920,8 +920,6 @@ int networkAddFirewallRules(virNetworkDefPtr def)
 }
 }
-    fw = virFirewallNew();
-
 virFirewallStartTransaction(fw, 0);
 networkAddGeneralFirewallRules(fw, def);
@@ -946,10 +944,7 @@ int networkAddFirewallRules(virNetworkDefPtr def)
 virFirewallStartTransaction(fw, 
VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS);

 networkAddChecksumFirewallRules(fw, def);
-    if (virFirewallApply(fw) < 0)
-    return -1;
-
-    return 0;
+    return virFirewallApply(fw);
 }
 /* Remove all rules for all ip addresses (and general rules) on a 
network */

Re: [PATCH 07/25] util: eliminate error label in virDomainDefFormatInternalSetRootName()

[Laine Stump]

On 6/25/20 7:08 PM, Ján Tomko wrote:

On a Wednesday in 2020, Laine Stump wrote:

The only reason for the error label in this function is to call
virBufferFreeAndReset(). It's actually more common for a failed format
function to just leave the virBuffer alone and let the caller free it
when there is a failure, and in fact the only caller of this function
that *wasn't* already calling virBufferFreeAndReset() on failure was
virDomainDefFormat() (via virDomainDefFormatInternal()).



qemuDomainDefFormatXMLInternal does not call it either.



Dang! I thought I had followed every call chain with cscope, but maybe I 
just searched in this one file? Anyway, it's especially embarrassing 
because not only did I miss qemuDomainFormatXMLInternal(), I also missed 
virDomainSnapshotDefFormat (which called 
virDomainSnapshotDefFormatInternal(), which calls 
virDomainDefFormatInternal()) :-(



I think as a followup patch, I should convert every occurrence of 
"virBuffer blah = VIR_BUFFER_INITIALIZER" to "g_auto(virBuffer) blah = 
VIR_BUFFER_INITIALIZER" - in a quick search just now I already found a 
couple more (totally unrelated to virDomainDefFormat) that aren't 
properly cleared out on error.



Thanks for taking the time to actually fact check my claims.


#FakeCommitLogs





That is easily solved by modifying virDomainDefFormat() to declare its
virBuffer buf with g_auto(), so that virBufferFreeAndReset() is
automatically called.

Signed-off-by: Laine Stump 
---
src/conf/domain_conf.c | 88 --
1 file changed, 42 insertions(+), 46 deletions(-)


With that fixed:
Reviewed-by: Ján Tomko 

Jano

Re: [PATCH 09/25] util: add g_autoptr cleanup function for virFirewall objects

[Laine Stump]

On 6/25/20 7:17 PM, Ján Tomko wrote:

The cleanup function was already added by:
commit 2ad0284627ea3d6c123e0a266b9c7bb00aea4576
CommitDate: 2018-07-27 17:21:04 +0200

    util: firewall: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC

On a Wednesday in 2020, Laine Stump wrote:

Put in a separate patch so that two future patches can be re-ordered /
selectively backported independent of each other.

Signed-off-by: Laine Stump 
---
src/util/virfirewall.h | 1 +
1 file changed, 1 insertion(+)

diff --git a/src/util/virfirewall.h b/src/util/virfirewall.h
index 6148f46827..ff690b36f0 100644
--- a/src/util/virfirewall.h
+++ b/src/util/virfirewall.h
@@ -40,6 +40,7 @@ virFirewallPtr virFirewallNew(void);

void virFirewallFree(virFirewallPtr firewall);

+


This is just a whitespace change that contradicts the prevailing style
in this file.



Right. Another patch that I intended to remove before I posted, but 
forgot because it was late and I wanted to end the day with a clean slate.



Derp.

Re: [PATCH 16/25] squash into 'network: convert local pointers to g_auto*'

[Ján Tomko]

On a Wednesday in 2020, Laine Stump wrote:

OOPS!!

I meant to squash this into patch 10 before posting. If you want to 
just review it separately I can squash it in before push. Or if you 
want to be pedantic I can squash it in and resend :-)




To me, these seem like changes unrelated to patch 10 and deserve their
own commit.

Jano


On 6/24/20 11:34 PM, Laine Stump wrote:

Signed-off-by: Laine Stump 
---
 src/network/bridge_driver_linux.c | 9 ++---
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/src/network/bridge_driver_linux.c 
b/src/network/bridge_driver_linux.c
index 0d0ac730f2..7f765bcf99 100644
--- a/src/network/bridge_driver_linux.c
+++ b/src/network/bridge_driver_linux.c
@@ -834,7 +834,7 @@ int networkAddFirewallRules(virNetworkDefPtr def)
 {
 size_t i;
 virNetworkIPDefPtr ipdef;
-g_autoptr(virFirewall) fw = NULL;
+g_autoptr(virFirewall) fw = virFirewallNew();
 if (virOnce(, networkSetupPrivateChains) < 0)
 return -1;
@@ -920,8 +920,6 @@ int networkAddFirewallRules(virNetworkDefPtr def)
 }
 }
-fw = virFirewallNew();
-
 virFirewallStartTransaction(fw, 0);
 networkAddGeneralFirewallRules(fw, def);
@@ -946,10 +944,7 @@ int networkAddFirewallRules(virNetworkDefPtr def)
 virFirewallStartTransaction(fw, VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS);
 networkAddChecksumFirewallRules(fw, def);
-if (virFirewallApply(fw) < 0)
-return -1;
-
-return 0;
+return virFirewallApply(fw);
 }
 /* Remove all rules for all ip addresses (and general rules) on a network */





signature.asc
Description: PGP signature

Re: [PATCH 10/25] network: convert local pointers to g_auto*

[Ján Tomko]

On a Wednesday in 2020, Laine Stump wrote:

This includes those that use plain VIR_FREE() as well as those that
have a cleanup function defined for use via g_auto/g_autoptr
(virCommand, virFirewall, virBuffer, virJSONValue etc).

Signed-off-by: Laine Stump 
---
src/network/bridge_driver.c   | 477 +++---
src/network/bridge_driver_linux.c |  55 ++--
src/network/leaseshelper.c|  16 +-
src/util/virdnsmasq.h |   4 +
4 files changed, 209 insertions(+), 343 deletions(-)



Since this patch touches way too many functions, it would be much easier
to read in two separate commits:
1) add g_auto markers and remove the corresponding VIR_FREE's, possibly
   leaving empty cleanup sections
2) removing all the now unnecessary labels and gotos

Jano


signature.asc
Description: PGP signature

Re: [PATCH 15/25] network: use proper arg type when calling virNetDevSetOnline()

[Ján Tomko]

On a Wednesday in 2020, Laine Stump wrote:

The 2nd arg to this function is a bool, not an int.

Signed-off-by: Laine Stump 
---
src/network/bridge_driver.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH 12/25] network: make networkDnsmasqXmlNsDef private to bridge_driver.c

[Ján Tomko]

On a Wednesday in 2020, Laine Stump wrote:

This struct isn't used anywhere else.

Signed-off-by: Laine Stump 
---
src/network/bridge_driver.c | 10 ++
src/network/bridge_driver.h |  9 -
2 files changed, 10 insertions(+), 9 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH 09/25] util: add g_autoptr cleanup function for virFirewall objects

[Ján Tomko]

The cleanup function was already added by:
commit 2ad0284627ea3d6c123e0a266b9c7bb00aea4576
CommitDate: 2018-07-27 17:21:04 +0200

util: firewall: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC

On a Wednesday in 2020, Laine Stump wrote:

Put in a separate patch so that two future patches can be re-ordered /
selectively backported independent of each other.

Signed-off-by: Laine Stump 
---
src/util/virfirewall.h | 1 +
1 file changed, 1 insertion(+)

diff --git a/src/util/virfirewall.h b/src/util/virfirewall.h
index 6148f46827..ff690b36f0 100644
--- a/src/util/virfirewall.h
+++ b/src/util/virfirewall.h
@@ -40,6 +40,7 @@ virFirewallPtr virFirewallNew(void);

void virFirewallFree(virFirewallPtr firewall);

+


This is just a whitespace change that contradicts the prevailing style
in this file.

Jano


/**
 * virFirewallAddRule:
 * @firewall: firewall ruleset to add to
--
2.25.4



signature.asc
Description: PGP signature

Re: [PATCH 08/25] network: fix memory leak in networkBuildDhcpDaemonCommandLine()

[Ján Tomko]

On a Wednesday in 2020, Laine Stump wrote:

hostsfilestr was not being freed. This will be turned into g_autofree
in an upcoming patch converting a lot more of the same file to using
g_auto*, but I wanted to make a separate patch for this first so the
other patch is simpler to review.



It also makes it easier to backport just the fix without the refactor.

Fixes: 97a0aa246799c97d0a9ca9ecd6b4fd932ae4756c


Signed-off-by: Laine Stump 
---
src/network/bridge_driver.c | 1 +
1 file changed, 1 insertion(+)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH 07/25] util: eliminate error label in virDomainDefFormatInternalSetRootName()

[Ján Tomko]

On a Wednesday in 2020, Laine Stump wrote:

The only reason for the error label in this function is to call
virBufferFreeAndReset(). It's actually more common for a failed format
function to just leave the virBuffer alone and let the caller free it
when there is a failure, and in fact the only caller of this function
that *wasn't* already calling virBufferFreeAndReset() on failure was
virDomainDefFormat() (via virDomainDefFormatInternal()).



qemuDomainDefFormatXMLInternal does not call it either.


That is easily solved by modifying virDomainDefFormat() to declare its
virBuffer buf with g_auto(), so that virBufferFreeAndReset() is
automatically called.

Signed-off-by: Laine Stump 
---
src/conf/domain_conf.c | 88 --
1 file changed, 42 insertions(+), 46 deletions(-)


With that fixed:
Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH 06/25] conf: eliminate useless error label in virDomainFeaturesDefParse()

[Ján Tomko]

On a Wednesday in 2020, Laine Stump wrote:

The error: label in this function just does "return -1", so replace
all the "goto error" in the function with "return -1".



I split this out from virDomainDefParse quickly as a build breaker fix
and forgot to follow up with this cleanup.


Signed-off-by: Laine Stump 
---
src/conf/domain_conf.c | 91 --
1 file changed, 44 insertions(+), 47 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH 05/25] util: remove OOM error log from virGetHostnameImpl()

[Ján Tomko]

On a Wednesday in 2020, Laine Stump wrote:

The strings allocated in virGetHostnameImpl() are all allocated via
g_strdup(), which will exit on OOM anyway, so the call to
virReportOOMError() is redundant, and removing it allows slight
modification to the code, in particular the cleanup label can be
eliminated.

Signed-off-by: Laine Stump 
---
src/util/virutil.c | 10 ++
1 file changed, 2 insertions(+), 8 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH 04/25] util: validate return from xmlNodeGetContent before use

[Ján Tomko]

On a Wednesday in 2020, Laine Stump wrote:

There were a few uses of xmlNodeGetContent() that didn't check for
NULL before using the result.

A NULL return from xmlNodeGetContent() *could* (probably does) mean
that there was an Out of Memory condition, but it is unclear from the
documentation if that is always the case, or if it could just indicate
a missing value in the document, so we don't report an OOM error, but
just don't try to use it for, e.g., conversion to an integer.


Is it possible to have an element with "no value"?

Even  gives me an empty string instead of NULL.

Jano



Signed-off-by: Laine Stump 
---
src/conf/domain_conf.c | 28 ++--
1 file changed, 14 insertions(+), 14 deletions(-)


signature.asc
Description: PGP signature

Re: [PATCH 03/25] conf: refactor virDomainBlkioDeviceParseXML to remove possible NULL dereference

[Ján Tomko]

On a Wednesday in 2020, Laine Stump wrote:

virDomainBlkioDeviceParseXML() has multiple cases of sending the
return from xmlNodeGetContent() directly to virStrToLong_xx() without
checking for NULL. Although it is *very* rare for xmlNodeGetContent()
to return NULL (possibly it only happens in an OOM condition? The
documentation is unclear), it could happen, and the refactor in this
patch manages to eliminate several lines of repeated code while adding
in a (single) check for NULL.

Signed-off-by: Laine Stump 
---
src/conf/domain_conf.c | 39 +++
1 file changed, 15 insertions(+), 24 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 1916b51d38..8cde1cd0e8 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1628,73 +1628,64 @@ virDomainBlkioDeviceParseXML(xmlNodePtr root,
 virBlkioDevicePtr dev)
{
xmlNodePtr node;
-g_autofree char *c = NULL;
+g_autofree char *path = NULL;

node = root->children;
while (node) {
-if (node->type == XML_ELEMENT_NODE) {
-if (virXMLNodeNameEqual(node, "path") && !dev->path) {
-dev->path = (char *)xmlNodeGetContent(node);
+g_autofree char *c = NULL;
+
+if (node->type == XML_ELEMENT_NODE &&
+(c = (char *)xmlNodeGetContent(node))) {


Missing ErrorReport if xmlNodeGetContent fails.

Converting this open-coded for loop to an actual for loop would
grant us 'continue' privileges, which would make the checks
nicer and give a possibility of assigning the path directly
to 'path', without the extra steal_pointer.

Alternatively, the minimum diff where I'd consider this patch to be
a strict improvement is:

} else if (node->type == XML_ELEMENT_NODE && !c) {
virReportOOMError();
return -1;
}

With that: Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH 02/25] use g_autoptr for all xmlBuffers

[Ján Tomko]

On a Wednesday in 2020, Laine Stump wrote:

AUTOPTR_CLEANUP_FUNC is set to xmlBufferFree() in util/virxml.h (This
is actually new - added accidentally (but fortunately harmlessly!) in
commit 257aba2dafe. I had added it along with the hunks in this patch,
then decided to remove it and submit separately, but missed taking out
the hunk in virxml.h)

Signed-off-by: Laine Stump 
---
src/conf/domain_conf.c  |  4 +---
src/conf/network_conf.c |  4 +---
src/util/virxml.c   | 12 +++-
src/vmx/vmx.c   | 10 +++---
4 files changed, 8 insertions(+), 22 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH 01/25] conf, vmx: check for OOM after calling xmlBufferCreate()

[Ján Tomko]

On a Wednesday in 2020, Laine Stump wrote:

Although libvirt itself uses g_malloc0() and friends, which exit when
there isn't enouogh memory, libxml2 uses standard malloc(), which just
returns NULL on OOM - this means we must check for NULL on return from
any libxml2 functions that allocate memory.

xmlBufferCreate(), for example, might return NULL, and we don't always
check for it. This patch adds checks where it isn't already done.

(NB: Although libxml2 has a provision for changing behavior on OOM (by
calling xmlMemSetup() to change what functions are used to
allocating/freeing memory), we can't use that, since parts of libvirt
code end up in libvirt.so, which is linked and called directly by
applications that may themselves use libxml2 (and may have already set
their own alternate malloc()), e.g. drivers like esx which live totally
in the library rather than a separate process.)

Signed-off-by: Laine Stump 
---
src/conf/domain_conf.c  | 6 +-
src/conf/network_conf.c | 6 +-
src/vmx/vmx.c   | 7 +--
3 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
index f2248cef53..fa9766995c 100644
--- a/src/vmx/vmx.c
+++ b/src/vmx/vmx.c
@@ -708,8 +708,11 @@ virVMXConvertToUTF8(const char *encoding, const char 
*string)
return NULL;
}

-input = xmlBufferCreateStatic((char *)string, strlen(string));
-utf8 = xmlBufferCreate();
+if (!(input = xmlBufferCreateStatic((char *)string, strlen(string))) ||
+!(utf8 = xmlBufferCreate())) {


My Clang complains that 'utf8' might be used uninitialized if the first
part of the condition is true.


+virReportOOMError();
+goto cleanup;
+}

if (xmlCharEncInFunc(handler, utf8, input) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,


With 'utf8' initialized:
Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

[PATCH v1] qemu: monitor: substitute missing model name for host-passthrough

[Collin Walling]

When executing the hypervisor-cpu-compare/baseline commands and
the XML file contains a CPU definition using host-passthrough
and no model name, the commands will fail and return an error
message from the QMP response.

Let's fix this by checking for host-passthrough and a missing
model name when converting a CPU definition to a JSON object.
If these conditions are matched, then the JSON object will be
constructed using "host" as the model name.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1850680

Signed-off-by: Collin Walling 
---
 src/qemu/qemu_monitor_json.c | 13 -
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 3070c1e6b3..448a3a9356 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -5804,9 +5804,20 @@ qemuMonitorJSONMakeCPUModel(virCPUDefPtr cpu,
 {
 virJSONValuePtr model = virJSONValueNewObject();
 virJSONValuePtr props = NULL;
+const char *model_name = cpu->model;
 size_t i;
 
-if (virJSONValueObjectAppendString(model, "name", cpu->model) < 0)
+if (!model_name) {
+if (cpu->mode == VIR_CPU_MODE_HOST_PASSTHROUGH) {
+model_name = "host";
+} else {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("cpu parameter is missing a model name"));
+goto error;
+}
+}
+
+if (virJSONValueObjectAppendString(model, "name", model_name) < 0)
 goto error;
 
 if (cpu->nfeatures || !migratable) {
-- 
2.26.2

Re: [libvirt PATCH v2] ci: Use openSUSE image for code style checking

[Jonathon Jongsma]

On Mon, 2020-06-22 at 18:18 +0200, Andrea Bolognani wrote:
> On Mon, 2020-06-22 at 09:43 -0500, Jonathon Jongsma wrote:
> > CentOS does not have the cppi package, so some code style checks
> > are
> > skipped. Switch to a openSUSE image to do the code style checks.
> > 
> > Signed-off-by: Jonathon Jongsma 
> > ---
> >  .gitlab-ci.yml | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Reviewed-by: Andrea Bolognani 
> 

Not that it's a particularly important patch, but I just thought I'd
mention that if we do want it upstream, I'll need somebody to push it
for me ;)


Jonathon

[libvirt PATCH v3 0/2] Refuse PCI Address for ramfb device definition

[Jonathon Jongsma]

Changes in this version:
 - Add the test case input file
 - modify the test itself to properly fail when an input file is missing.

Jonathon Jongsma (2):
  qemu: ramfb video device doesn't support PCI address
  tests: ensure failure if input file doesn't exist

 src/qemu/qemu_validate.c  |  8 +
 ...video-ramfb-display-device-pci-address.xml | 30 +++
 tests/qemuxml2argvtest.c  |  7 +
 3 files changed, 45 insertions(+)
 create mode 100644 
tests/qemuxml2argvdata/video-ramfb-display-device-pci-address.xml

-- 
2.21.3

[libvirt PATCH v3 1/2] qemu: ramfb video device doesn't support PCI address

[Jonathon Jongsma]

Although a ramfb video device is not a PCI device, we don't currently
report an error for ramfb device definitions containing a PCI address.
However, a guest configured with such a device will fail to start:

# virsh start test1
error: Failed to start domain test1
error: internal error: qemu unexpectedly closed the monitor: 
2020-06-16T05:23:02.759221Z qemu-kvm: -device 
ramfb,id=video0,bus=pcie.0,addr=0x1: Device 'ramfb' can't go on PCIE bus

A better approach is to reject any device definitions that contain PCI
addresses.  While this is a change in behavior, any existing
configurations were non-functional.

https://bugzilla.redhat.com/show_bug.cgi?id=1847259

Signed-off-by: Jonathon Jongsma 
---
 src/qemu/qemu_validate.c  |  8 +
 ...video-ramfb-display-device-pci-address.xml | 30 +++
 tests/qemuxml2argvtest.c  |  1 +
 3 files changed, 39 insertions(+)
 create mode 100644 
tests/qemuxml2argvdata/video-ramfb-display-device-pci-address.xml

diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index 5082a29dc7..b13c03759e 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -1925,6 +1925,14 @@ qemuValidateDomainDeviceDefVideo(const virDomainVideoDef 
*video,
 if (qemuValidateDomainVirtioOptions(video->virtio, qemuCaps) < 0)
 return -1;
 
+if (video->type == VIR_DOMAIN_VIDEO_TYPE_RAMFB &&
+video->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("'address' is not supported for 'ramfb' video 
devices"));
+return -1;
+}
+
+
 return 0;
 }
 
diff --git a/tests/qemuxml2argvdata/video-ramfb-display-device-pci-address.xml 
b/tests/qemuxml2argvdata/video-ramfb-display-device-pci-address.xml
new file mode 100644
index 00..db110560ac
--- /dev/null
+++ b/tests/qemuxml2argvdata/video-ramfb-display-device-pci-address.xml
@@ -0,0 +1,30 @@
+
+  QEMUGuest1
+  c7a5fdbd-edaf-9455-926a-d65c16db1809
+  1048576
+  1048576
+  1
+  
+hvm
+
+  
+  
+  destroy
+  restart
+  destroy
+  
+/usr/bin/qemu-system-i386
+
+  
+  
+  
+  
+
+
+
+  
+  
+
+
+  
+
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 1195f9c982..f2522fa530 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -2276,6 +2276,7 @@ mymain(void)
 QEMU_CAPS_VIRTIO_GPU_MAX_OUTPUTS);
 DO_TEST_CAPS_LATEST("video-bochs-display-device");
 DO_TEST_CAPS_LATEST("video-ramfb-display-device");
+DO_TEST_CAPS_LATEST_PARSE_ERROR("video-ramfb-display-device-pci-address");
 DO_TEST("video-none-device",
 QEMU_CAPS_VNC);
 DO_TEST_PARSE_ERROR("video-invalid-multiple-devices", NONE);
-- 
2.21.3

[libvirt PATCH v3 2/2] tests: ensure failure if input file doesn't exist

[Jonathon Jongsma]

When using the DO_TEST_PARSE_ERROR() macro, a failure to parse the input
file is considered a successful test. However, if the input file is
totally missing, that should be distinguished from a parsing error and
not be treated as a test success.

The function virDomainDefParseFile() simply returns NULL for any parse
failure, including a missing file. So we need to explicitly check
whether the file exists first, and fail the test if it is missing.

Signed-off-by: Jonathon Jongsma 
---
 tests/qemuxml2argvtest.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index f2522fa530..248ce07811 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -608,6 +608,12 @@ testCompareXMLToArgv(const void *data)
 if (!(vm = virDomainObjNew(driver.xmlopt)))
 goto cleanup;
 
+if (!virFileExists(info->infile)) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   "Input file '%s' not found", info->infile);
+goto cleanup;
+}
+
 parseFlags |= VIR_DOMAIN_DEF_PARSE_INACTIVE;
 if (!(vm->def = virDomainDefParseFile(info->infile,
   driver.xmlopt,
-- 
2.21.3

Re: [PATCH libvirt v2 0/5] Fix zPCI address auto-generation on s390

[Andrea Bolognani]

On Thu, 2020-06-18 at 10:25 +0200, Shalini Chellathurai Saroja wrote:
> Shalini Chellathurai Saroja (5):
>   conf: use g_autofree to ensure automatic cleanup
>   conf: fix zPCI address auto-generation on s390
>   qemu: move ZPCI uid validation into device validation
>   tests: qemu: add more tests for ZPCI on S390
>   tests: add test with PCI and CCW device

Aside from the comments in patch 2/5, everything looks good to me.

The series does, however, not update the release notes (NEWS.rst):
can you please post a 6/5 patch that takes care of that, assuming we
decide not to go with a respin?

Thanks!

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH libvirt v2 5/5] tests: add test with PCI and CCW device

[Andrea Bolognani]

On Thu, 2020-06-18 at 10:25 +0200, Shalini Chellathurai Saroja wrote:
> Add test with a ZPCI host device and a CCW memballoon device to ensure
> that CCW address remains the default address assigned.
> 
> Signed-off-by: Boris Fiuczynski 
> Signed-off-by: Shalini Chellathurai Saroja 
> Reviewed-by: Bjoern Walk 
> Reviewed-by: Boris Fiuczynski 
> ---
>  .../hostdev-vfio-zpci-ccw-memballoon.args | 28 
>  .../hostdev-vfio-zpci-ccw-memballoon.xml  | 17 ++
>  tests/qemuxml2argvtest.c  |  4 +++
>  .../hostdev-vfio-zpci-ccw-memballoon.xml  | 32 +++
>  tests/qemuxml2xmltest.c   |  4 +++
>  5 files changed, 85 insertions(+)

Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH libvirt v2 4/5] tests: qemu: add more tests for ZPCI on S390

[Andrea Bolognani]

On Thu, 2020-06-18 at 10:25 +0200, Shalini Chellathurai Saroja wrote:
> 1. Test for auto-generating uids while specifying valid fids
> 2. Test for auto-generating fids while specifying valid uids
> 3. Test for parse error while specifying a valid fid and an invalid
>uid
> 4. Test for parse error while specifying two ZPCI devices with same
>uid and fid addresses
> 5. Test for parse error when both uid and fid are set to zero
> 6. Test for error while specifying uid and not providing ZPCI
>capability.
> 
> Signed-off-by: Boris Fiuczynski 
> Signed-off-by: Bjoern Walk 
> Signed-off-by: Shalini Chellathurai Saroja 
> Reviewed-by: Bjoern Walk 
> Reviewed-by: Boris Fiuczynski 
> ---
>  .../hostdev-vfio-zpci-autogenerate-fids.args  | 31 +
>  .../hostdev-vfio-zpci-autogenerate-fids.xml   | 29 +
>  .../hostdev-vfio-zpci-autogenerate-uids.args  | 31 +
>  .../hostdev-vfio-zpci-autogenerate-uids.xml   | 29 +
>  .../hostdev-vfio-zpci-duplicate.xml   | 30 +
>  ...ostdev-vfio-zpci-invalid-uid-valid-fid.xml | 21 +
>  .../hostdev-vfio-zpci-set-zero.xml| 21 +
>  tests/qemuxml2argvtest.c  | 18 
>  .../hostdev-vfio-zpci-autogenerate-fids.xml   | 43 +++
>  .../hostdev-vfio-zpci-autogenerate-uids.xml   | 43 +++
>  tests/qemuxml2xmltest.c   |  6 +++
>  11 files changed, 302 insertions(+)

Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH libvirt v2 3/5] qemu: move ZPCI uid validation into device validation

[Andrea Bolognani]

On Thu, 2020-06-18 at 10:25 +0200, Shalini Chellathurai Saroja wrote:
> qemu: move ZPCI uid validation into device validation
> 
> The ZPCI device validation is specific to qemu. So, let us move the
> ZPCI uid validation out of domain xml parsing into qemu domain device
> validation.

We can just talk about "zPCI validation" instead of singling out
the uid attribute specifically.

Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH libvirt v2 2/5] conf: fix zPCI address auto-generation on s390

[Andrea Bolognani]

On Thu, 2020-06-18 at 10:25 +0200, Shalini Chellathurai Saroja wrote:
> @@ -129,7 +129,8 @@ static void
>  virDomainZPCIAddressReleaseUid(virHashTablePtr set,
> virZPCIDeviceAddressPtr addr)
>  {
> -virDomainZPCIAddressReleaseId(set, >uid, "uid");
> +if (addr->uid.isSet)
> +virDomainZPCIAddressReleaseId(set, >uid.value, "uid");
>  }
>  
> @@ -137,7 +138,8 @@ static void
>  virDomainZPCIAddressReleaseFid(virHashTablePtr set,
> virZPCIDeviceAddressPtr addr)
>  {
> -virDomainZPCIAddressReleaseId(set, >fid, "fid");
> +if (addr->fid.isSet)
> +virDomainZPCIAddressReleaseId(set, >fid.value, "fid");
>  }
> 
> -static int
> -virDomainZPCIAddressReserveNextAddr(virDomainZPCIAddressIdsPtr zpciIds,
> -virZPCIDeviceAddressPtr addr)
> -{
> -if (virDomainZPCIAddressReserveNextUid(zpciIds->uids, addr) < 0)
> -return -1;
> -
> -if (virDomainZPCIAddressReserveNextFid(zpciIds->fids, addr) < 0) {
> -virDomainZPCIAddressReleaseUid(zpciIds->uids, addr);
> -return -1;
> -}
> +addr->uid.isSet = true;
> +addr->fid.isSet = true;

First of all, this is much closer to what I had in mind. Good job!

We're not quite there yet, though: as you can see from the hunks
above, there are still many scenarios in which we're either
manipulating id->value and id->isSet separately, or we needlessly
duplicate checks and don't take advantage of the fact that we now
have the virZPCIDeviceAddressID struct.

I have attached a patch that fixes these issues: as you can see,
it's pretty simple, because you've laid all the groundwork :) so it
just needs to polish things up a bit. It also solves the situation
where calling virDomainZPCIAddressRelease{U,F}id() would not set
id->isSet back to false.

Speaking of polish, while functionally this doesn't make any
difference it would be IMHO better to rename the current
virDomainZPCIAddressReserveAddr() to
virDomainZPCIAddressEnsureAddr(), since in terms of semantics it
more closely matches those of the PCI address function of the same
name. This will hopefully help reduce confusion. I've attached a
patch that performs this change as well.

Everything else looks good. Please let me know what you think of
the changes in the attached patches!

-- 
Andrea Bolognani / Red Hat / Virtualization
From 82197ea6d794144e51b72f97df2315a65134b385 Mon Sep 17 00:00:00 2001
From: Andrea Bolognani 
Date: Thu, 25 Jun 2020 18:37:27 +0200
Subject: [libvirt PATCH 1/2] conf: Move id->{value,isSet} handling further
 down the stack

Signed-off-by: Andrea Bolognani 
---
 src/conf/domain_addr.c | 61 --
 1 file changed, 35 insertions(+), 26 deletions(-)

diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
index 90ed31ef4e..493b155129 100644
--- a/src/conf/domain_addr.c
+++ b/src/conf/domain_addr.c
@@ -33,20 +33,27 @@ VIR_LOG_INIT("conf.domain_addr");
 
 static int
 virDomainZPCIAddressReserveId(virHashTablePtr set,
-  unsigned int id,
+  virZPCIDeviceAddressID *id,
   const char *name)
 {
-if (virHashLookup(set, )) {
+if (!id->isSet) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("No zPCI %s to reserve"),
+   name);
+return -1;
+}
+
+if (virHashLookup(set, >value)) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("zPCI %s %o is already reserved"),
-   name, id);
+   name, id->value);
 return -1;
 }
 
-if (virHashAddEntry(set, , (void*)1) < 0) {
+if (virHashAddEntry(set, >value, (void*)1) < 0) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to reserve %s %o"),
-   name, id);
+   name, id->value);
 return -1;
 }
 
@@ -58,7 +65,7 @@ static int
 virDomainZPCIAddressReserveUid(virHashTablePtr set,
virZPCIDeviceAddressPtr addr)
 {
-return virDomainZPCIAddressReserveId(set, addr->uid.value, "uid");
+return virDomainZPCIAddressReserveId(set, >uid, "uid");
 }
 
 
@@ -66,17 +73,20 @@ static int
 virDomainZPCIAddressReserveFid(virHashTablePtr set,
virZPCIDeviceAddressPtr addr)
 {
-return virDomainZPCIAddressReserveId(set, addr->fid.value, "fid");
+return virDomainZPCIAddressReserveId(set, >fid, "fid");
 }
 
 
 static int
 virDomainZPCIAddressAssignId(virHashTablePtr set,
- unsigned int *id,
+ virZPCIDeviceAddressID *id,
  unsigned int min,
  unsigned int max,
  const char *name)
 {
+if (id->isSet)
+return 0;
+
 while (virHashLookup(set, )) {
 if (min == max) {
   

Re: [PATCH libvirt v2 1/5] conf: use g_autofree to ensure automatic cleanup

[Andrea Bolognani]

On Thu, 2020-06-18 at 10:25 +0200, Shalini Chellathurai Saroja wrote:
> Signed-off-by: Bjoern Walk 
> Signed-off-by: Shalini Chellathurai Saroja 
> Reviewed-by: Boris Fiuczynski 
> ---
>  src/conf/device_conf.c | 17 ++---
>  1 file changed, 6 insertions(+), 11 deletions(-)

Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH libvirt-dbus v3 2/3] gitlab: introduce CI jobs testing git master & distro libvirt

[Andrea Bolognani]

On Thu, 2020-06-25 at 17:14 +0100, Daniel P. Berrangé wrote:
> +.git_native_build_job_template: _native_build_job_definition
> +  image: $CI_REGISTRY_IMAGE/ci-$NAME:latest
> +  stage: builds
> +  cache:
> +paths:
> +  - ccache/
> +key: "$CI_JOB_NAME"
> +  before_script:
> +- *script_variables
> +  script:
[...]
> +- meson build --prefix="$VROOT"
> +- $NINJA -C build
> +- if test "$TESTS" != "skip";
> +  then
> +$NINJA -C build test;
> +$NINJA -C build install;
> +$NINJA -C build dist;

We can still run 'ninja install' even when we're told to skip tests,
as the two are not related.

> +.dist_native_build_job_template: _native_build_job_definition
> +  image: $CI_REGISTRY_IMAGE/ci-$NAME:latest
> +  stage: builds
> +  cache:
> +paths:
> +  - ccache/
> +key: "$CI_JOB_NAME"
> +  before_script:
> +- *script_variables
> +  script:
> +- meson build --prefix="$VROOT"
> +- $NINJA -C build
> +- if test "$TESTS" != "skip";
> +  then
> +$NINJA -C build test;
> +$NINJA -C build install;
> +$NINJA -C build dist;

Same here.

With that changed,

  Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt PATCH v2] qemu: ramfb video device doesn't support PCI address

[Laine Stump]

On 6/25/20 12:30 PM, Jonathon Jongsma wrote:

On Thu, 2020-06-25 at 12:20 -0400, Laine Stump wrote:

On 6/25/20 10:34 AM, Jonathon Jongsma wrote:

Although a ramfb video device is not a PCI device, we don't
currently
report an error for ramfb device definitions containing a PCI
address.
However, a guest configured with such a device will fail to start:

  # virsh start test1
  error: Failed to start domain test1
  error: internal error: qemu unexpectedly closed the monitor:
2020-06-16T05:23:02.759221Z qemu-kvm: -device
ramfb,id=video0,bus=pcie.0,addr=0x1: Device 'ramfb' can't go on
PCIE bus

A better approach is to reject any device definitions that contain
PCI
addresses.  While this is a change in behavior, any existing
configurations were non-functional.

https://bugzilla.redhat.com/show_bug.cgi?id=1847259

Signed-off-by: Jonathon Jongsma 
---
changes in v2:
   - move validation to qemu-specific validation function as
suggested by Laine

   src/qemu/qemu_validate.c | 8 
   tests/qemuxml2argvtest.c | 1 +
   2 files changed, 9 insertions(+)

diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index 5082a29dc7..b13c03759e 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -1925,6 +1925,14 @@ qemuValidateDomainDeviceDefVideo(const
virDomainVideoDef *video,
   if (qemuValidateDomainVirtioOptions(video->virtio, qemuCaps)
< 0)
   return -1;
   
+if (video->type == VIR_DOMAIN_VIDEO_TYPE_RAMFB &&

+video->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("'address' is not supported for 'ramfb'
video devices"));
+return -1;
+}
+
+
   return 0;
   }
   
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c

index 1195f9c982..f2522fa530 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -2276,6 +2276,7 @@ mymain(void)
   QEMU_CAPS_VIRTIO_GPU_MAX_OUTPUTS);
   DO_TEST_CAPS_LATEST("video-bochs-display-device");
   DO_TEST_CAPS_LATEST("video-ramfb-display-device");
+DO_TEST_CAPS_LATEST_PARSE_ERROR("video-ramfb-display-device-
pci-address");

Did you forget to git-add the test case data?

OK, well that points out an interesting property of the
DO_TEST_CAPS_LATEST_PARSE_ERROR() macro. It passed the 'make check'
because it failed to parse the xml file as expected. But the reason it
failed was obviously not the reason I expected it to fail... I want it
to fail due to the specifying a PCI address for the ramfb device. But
it actually fails because the xml file didn't exist... :/



That gave me the best laugh I'd had all day!! :-)


I just realized you don't have push privileges on gitlab. If you re-send 
with test case data, I'll push it.



(I guess we should also fix the test harness to "fail to fail" if the 
test case data is missing, but that's a separate issue)

Re: [libvirt PATCH v2] qemu: ramfb video device doesn't support PCI address

[Jonathon Jongsma]

On Thu, 2020-06-25 at 12:20 -0400, Laine Stump wrote:
> On 6/25/20 10:34 AM, Jonathon Jongsma wrote:
> > Although a ramfb video device is not a PCI device, we don't
> > currently
> > report an error for ramfb device definitions containing a PCI
> > address.
> > However, a guest configured with such a device will fail to start:
> > 
> >  # virsh start test1
> >  error: Failed to start domain test1
> >  error: internal error: qemu unexpectedly closed the monitor:
> > 2020-06-16T05:23:02.759221Z qemu-kvm: -device
> > ramfb,id=video0,bus=pcie.0,addr=0x1: Device 'ramfb' can't go on
> > PCIE bus
> > 
> > A better approach is to reject any device definitions that contain
> > PCI
> > addresses.  While this is a change in behavior, any existing
> > configurations were non-functional.
> > 
> > https://bugzilla.redhat.com/show_bug.cgi?id=1847259
> > 
> > Signed-off-by: Jonathon Jongsma 
> > ---
> > changes in v2:
> >   - move validation to qemu-specific validation function as
> > suggested by Laine
> > 
> >   src/qemu/qemu_validate.c | 8 
> >   tests/qemuxml2argvtest.c | 1 +
> >   2 files changed, 9 insertions(+)
> > 
> > diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
> > index 5082a29dc7..b13c03759e 100644
> > --- a/src/qemu/qemu_validate.c
> > +++ b/src/qemu/qemu_validate.c
> > @@ -1925,6 +1925,14 @@ qemuValidateDomainDeviceDefVideo(const
> > virDomainVideoDef *video,
> >   if (qemuValidateDomainVirtioOptions(video->virtio, qemuCaps)
> > < 0)
> >   return -1;
> >   
> > +if (video->type == VIR_DOMAIN_VIDEO_TYPE_RAMFB &&
> > +video->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
> > +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > +   _("'address' is not supported for 'ramfb'
> > video devices"));
> > +return -1;
> > +}
> > +
> > +
> >   return 0;
> >   }
> >   
> > diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> > index 1195f9c982..f2522fa530 100644
> > --- a/tests/qemuxml2argvtest.c
> > +++ b/tests/qemuxml2argvtest.c
> > @@ -2276,6 +2276,7 @@ mymain(void)
> >   QEMU_CAPS_VIRTIO_GPU_MAX_OUTPUTS);
> >   DO_TEST_CAPS_LATEST("video-bochs-display-device");
> >   DO_TEST_CAPS_LATEST("video-ramfb-display-device");
> > +DO_TEST_CAPS_LATEST_PARSE_ERROR("video-ramfb-display-device-
> > pci-address");
> 
> Did you forget to git-add the test case data?

OK, well that points out an interesting property of the
DO_TEST_CAPS_LATEST_PARSE_ERROR() macro. It passed the 'make check'
because it failed to parse the xml file as expected. But the reason it
failed was obviously not the reason I expected it to fail... I want it
to fail due to the specifying a PCI address for the ramfb device. But
it actually fails because the xml file didn't exist... :/


> 
> 
> >   DO_TEST("video-none-device",
> >   QEMU_CAPS_VNC);
> >   DO_TEST_PARSE_ERROR("video-invalid-multiple-devices", NONE);
> > 
> 
> With the test case data added
> 
> Reviewed-by: Laine Stump 

Re: [libvirt PATCH v2] qemu: ramfb video device doesn't support PCI address

[Laine Stump]

On 6/25/20 10:34 AM, Jonathon Jongsma wrote:

Although a ramfb video device is not a PCI device, we don't currently
report an error for ramfb device definitions containing a PCI address.
However, a guest configured with such a device will fail to start:

 # virsh start test1
 error: Failed to start domain test1
 error: internal error: qemu unexpectedly closed the monitor: 
2020-06-16T05:23:02.759221Z qemu-kvm: -device 
ramfb,id=video0,bus=pcie.0,addr=0x1: Device 'ramfb' can't go on PCIE bus

A better approach is to reject any device definitions that contain PCI
addresses.  While this is a change in behavior, any existing
configurations were non-functional.

https://bugzilla.redhat.com/show_bug.cgi?id=1847259

Signed-off-by: Jonathon Jongsma 
---
changes in v2:
  - move validation to qemu-specific validation function as suggested by Laine

  src/qemu/qemu_validate.c | 8 
  tests/qemuxml2argvtest.c | 1 +
  2 files changed, 9 insertions(+)

diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index 5082a29dc7..b13c03759e 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -1925,6 +1925,14 @@ qemuValidateDomainDeviceDefVideo(const virDomainVideoDef 
*video,
  if (qemuValidateDomainVirtioOptions(video->virtio, qemuCaps) < 0)
  return -1;
  
+if (video->type == VIR_DOMAIN_VIDEO_TYPE_RAMFB &&

+video->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("'address' is not supported for 'ramfb' video 
devices"));
+return -1;
+}
+
+
  return 0;
  }
  
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c

index 1195f9c982..f2522fa530 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -2276,6 +2276,7 @@ mymain(void)
  QEMU_CAPS_VIRTIO_GPU_MAX_OUTPUTS);
  DO_TEST_CAPS_LATEST("video-bochs-display-device");
  DO_TEST_CAPS_LATEST("video-ramfb-display-device");
+DO_TEST_CAPS_LATEST_PARSE_ERROR("video-ramfb-display-device-pci-address");



Did you forget to git-add the test case data?



  DO_TEST("video-none-device",
  QEMU_CAPS_VNC);
  DO_TEST_PARSE_ERROR("video-invalid-multiple-devices", NONE);



With the test case data added

Reviewed-by: Laine Stump 

[PATCH libvirt-dbus v3 3/3] gitlab: add CONTRIBUTING.rst file to indicate use of merge requests

[Daniel P . Berrangé]

With the introduction of automated CI pipelines, we are now ready to switch
to using merge requests for the project. With this switch we longer wish
to have patches sent to the mailing list, and thus the git-publish
config is removed.

Reviewed-by: Andrea Bolognani 
Signed-off-by: Daniel P. Berrangé 
---
 .gitpublish  |  4 
 CONTRIBUTING.rst | 28 
 2 files changed, 28 insertions(+), 4 deletions(-)
 delete mode 100644 .gitpublish
 create mode 100644 CONTRIBUTING.rst

diff --git a/.gitpublish b/.gitpublish
deleted file mode 100644
index d21df70..000
--- a/.gitpublish
+++ /dev/null
@@ -1,4 +0,0 @@
-[gitpublishprofile "default"]
-base = master
-to = libvir-list@redhat.com
-prefix = libvirt-dbus PATCH
diff --git a/CONTRIBUTING.rst b/CONTRIBUTING.rst
new file mode 100644
index 000..8391896
--- /dev/null
+++ b/CONTRIBUTING.rst
@@ -0,0 +1,28 @@
+
+Contributing to libvirt-dbus
+
+
+The libvirt DBus API binding accepts code contributions via merge requests
+on the GitLab project:
+
+https://gitlab.com/libvirt/libvirt-dbus/-/merge_requests
+
+It is required that automated CI pipelines succeed before a merge request
+will be accepted. The global pipeline status for the ``master`` branch is
+visible at:
+
+https://gitlab.com/libvirt/libvirt-dbus/pipelines
+
+CI pipeline results for merge requests will be visible via the contributors'
+own private repository fork:
+
+https://gitlab.com/yourusername/libvirt-dbus/pipelines
+
+Contributions submitted to the project must be in compliance with the
+Developer Certificate of Origin Version 1.1. This is documented at:
+
+https://developercertificate.org/
+
+To indicate compliance, each commit in a series must have a "Signed-off-by"
+tag with the submitter's name and email address. This can be added by passing
+the ``-s`` flag to ``git commit`` when creating the patches.
-- 
2.26.2

[PATCH libvirt-dbus v3 2/3] gitlab: introduce CI jobs testing git master & distro libvirt

[Daniel P . Berrangé]

The dbus build needs to validate one axis

  - A variety of libvirt versions

We test a variety of libvirt versions by running a build against the
distro provided libvirt packages. All that is then missing is a build
against the latest libvirt git master, which only needs to be run on
a single distro, for which CentOS 8 & Stream are picked as a stable
long life base.

RPM build and tests are skipped on CentOS 7 since it lacks the required
min version of some packages.

Signed-off-by: Daniel P. Berrangé 
---
 .gitlab-ci.yml| 218 ++
 ci/containers/README.rst  |  14 ++
 ci/containers/libvirt-centos-7.Dockerfile |  90 
 ci/containers/libvirt-centos-8.Dockerfile |  68 ++
 .../libvirt-centos-stream.Dockerfile  |  69 ++
 ci/containers/libvirt-debian-10.Dockerfile|  61 +
 ci/containers/libvirt-debian-9.Dockerfile |  64 +
 ci/containers/libvirt-debian-sid.Dockerfile   |  61 +
 ci/containers/libvirt-fedora-31.Dockerfile|  58 +
 ci/containers/libvirt-fedora-32.Dockerfile|  58 +
 .../libvirt-fedora-rawhide.Dockerfile |  59 +
 ci/containers/libvirt-opensuse-151.Dockerfile |  60 +
 ci/containers/libvirt-ubuntu-1804.Dockerfile  |  64 +
 ci/containers/libvirt-ubuntu-2004.Dockerfile  |  61 +
 ci/containers/refresh |  37 +++
 15 files changed, 1042 insertions(+)
 create mode 100644 ci/containers/README.rst
 create mode 100644 ci/containers/libvirt-centos-7.Dockerfile
 create mode 100644 ci/containers/libvirt-centos-8.Dockerfile
 create mode 100644 ci/containers/libvirt-centos-stream.Dockerfile
 create mode 100644 ci/containers/libvirt-debian-10.Dockerfile
 create mode 100644 ci/containers/libvirt-debian-9.Dockerfile
 create mode 100644 ci/containers/libvirt-debian-sid.Dockerfile
 create mode 100644 ci/containers/libvirt-fedora-31.Dockerfile
 create mode 100644 ci/containers/libvirt-fedora-32.Dockerfile
 create mode 100644 ci/containers/libvirt-fedora-rawhide.Dockerfile
 create mode 100644 ci/containers/libvirt-opensuse-151.Dockerfile
 create mode 100644 ci/containers/libvirt-ubuntu-1804.Dockerfile
 create mode 100644 ci/containers/libvirt-ubuntu-2004.Dockerfile
 create mode 100755 ci/containers/refresh

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 50dae92..9b61c70 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -1,6 +1,99 @@
 
 stages:
   - prebuild
+  - containers
+  - builds
+
+.container_job_template: _job_definition
+  image: docker:stable
+  stage: containers
+  services:
+- docker:dind
+  before_script:
+- export TAG="$CI_REGISTRY_IMAGE/ci-$NAME:latest"
+- export COMMON_TAG="$CI_REGISTRY/libvirt/libvirt-dbus/ci-$NAME:latest"
+- docker info
+- docker login registry.gitlab.com -u "$CI_REGISTRY_USER" -p 
"$CI_REGISTRY_PASSWORD"
+  script:
+- docker pull "$TAG" || docker pull "$COMMON_TAG" || true
+- docker build --cache-from "$TAG" --cache-from "$COMMON_TAG" --tag "$TAG" 
-f "ci/containers/libvirt-$NAME.Dockerfile" ci
+- docker push "$TAG"
+  after_script:
+- docker logout
+
+.script_variables: _variables |
+  export MAKEFLAGS="-j$(getconf _NPROCESSORS_ONLN)"
+  export SCRATCH_DIR="/tmp/scratch"
+  export VROOT="$SCRATCH_DIR/vroot"
+  export CCACHE_DIR="$PWD/ccache"
+  export CCACHE_MAXSIZE="500M"
+  export PATH="$CCACHE_WRAPPERSDIR:$VROOT/bin:$PATH"
+  export PKG_CONFIG_PATH="$VROOT/lib/pkgconfig"
+  export XDG_DATA_DIRS="$VROOT/share:/usr/share"
+  export GI_TYPELIB_PATH="$VROOT/lib/girepository-1.0"
+  export LD_LIBRARY_PATH="$VROOT/lib"
+
+.git_native_build_job_template: _native_build_job_definition
+  image: $CI_REGISTRY_IMAGE/ci-$NAME:latest
+  stage: builds
+  cache:
+paths:
+  - ccache/
+key: "$CI_JOB_NAME"
+  before_script:
+- *script_variables
+  script:
+- pushd "$PWD"
+- mkdir -p "$SCRATCH_DIR"
+- cd "$SCRATCH_DIR"
+- git clone --depth 1 https://gitlab.com/libvirt/libvirt.git
+- git clone --depth 1 https://gitlab.com/libvirt/libvirt-glib.git
+- mkdir libvirt/build
+- cd libvirt/build
+- ../autogen.sh --prefix="$VROOT" --without-libvirtd
+- $MAKE install
+- cd ../..
+- mkdir libvirt-glib/build
+- cd libvirt-glib/build
+- ../autogen.sh --prefix="$VROOT"
+- $MAKE install
+- popd
+- meson build --prefix="$VROOT"
+- $NINJA -C build
+- if test "$TESTS" != "skip";
+  then
+$NINJA -C build test;
+$NINJA -C build install;
+$NINJA -C build dist;
+  fi
+- if test -x /usr/bin/rpmbuild && test "$RPM" != "skip";
+  then
+rpmbuild --nodeps -ta build/meson-dist/libvirt-dbus*.tar.xz;
+  fi
+
+.dist_native_build_job_template: _native_build_job_definition
+  image: $CI_REGISTRY_IMAGE/ci-$NAME:latest
+  stage: builds
+  cache:
+paths:
+  - ccache/
+key: "$CI_JOB_NAME"
+  before_script:
+- *script_variables
+  script:
+- meson build 

[PATCH libvirt-dbus v3 1/3] src: fix double free of virDomain object

[Daniel P . Berrangé]

The virDomainSnapshotGetDomain() method does NOT increment the
refcount on the returned virDomain, so it must not be unrefed.

This double free is responsible for random failures of the
test_snapshot.py tests.

Reviewed-by: Andrea Bolognani 
Signed-off-by: Daniel P. Berrangé 
---
 src/domainsnapshot.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/domainsnapshot.c b/src/domainsnapshot.c
index f3571a1..78a9f9a 100644
--- a/src/domainsnapshot.c
+++ b/src/domainsnapshot.c
@@ -63,7 +63,7 @@ virtDBusDomainSnapshotGetParent(GVariant *inArgs,
 g_autoptr(virDomainSnapshot) domainSnapshot = NULL;
 g_autoptr(virDomainSnapshot) parent = NULL;
 guint flags;
-g_autoptr(virDomain) domain = NULL;
+virDomainPtr domain = NULL;
 g_autofree gchar *parentPath = NULL;
 
 g_variant_get(inArgs, "(u)", );
@@ -159,7 +159,7 @@ virtDBusDomainSnapshotListAllChildren(GVariant *inArgs,
 guint flags;
 GVariantBuilder builder;
 GVariant *gdomainSnapshots;
-g_autoptr(virDomain) domain = NULL;
+virDomainPtr domain = NULL;
 
 g_variant_get(inArgs, "(u)", );
 
-- 
2.26.2

[PATCH libvirt-dbus v3 0/3] Introduce GitLab CI and merge requests

[Daniel P . Berrangé]

This introduces CI build coverage and then documents the switch to
use merge requests.

Daniel P. Berrangé (3):
  src: fix double free of virDomain object
  gitlab: introduce CI jobs testing git master & distro libvirt
  gitlab: add CONTRIBUTING.rst file to indicate use of merge requests

 .gitlab-ci.yml| 218 ++
 .gitpublish   |   4 -
 CONTRIBUTING.rst  |  28 +++
 ci/containers/README.rst  |  14 ++
 ci/containers/libvirt-centos-7.Dockerfile |  90 
 ci/containers/libvirt-centos-8.Dockerfile |  68 ++
 .../libvirt-centos-stream.Dockerfile  |  69 ++
 ci/containers/libvirt-debian-10.Dockerfile|  61 +
 ci/containers/libvirt-debian-9.Dockerfile |  64 +
 ci/containers/libvirt-debian-sid.Dockerfile   |  61 +
 ci/containers/libvirt-fedora-31.Dockerfile|  58 +
 ci/containers/libvirt-fedora-32.Dockerfile|  58 +
 .../libvirt-fedora-rawhide.Dockerfile |  59 +
 ci/containers/libvirt-opensuse-151.Dockerfile |  60 +
 ci/containers/libvirt-ubuntu-1804.Dockerfile  |  64 +
 ci/containers/libvirt-ubuntu-2004.Dockerfile  |  61 +
 ci/containers/refresh |  37 +++
 src/domainsnapshot.c  |   4 +-
 18 files changed, 1072 insertions(+), 6 deletions(-)
 delete mode 100644 .gitpublish
 create mode 100644 CONTRIBUTING.rst
 create mode 100644 ci/containers/README.rst
 create mode 100644 ci/containers/libvirt-centos-7.Dockerfile
 create mode 100644 ci/containers/libvirt-centos-8.Dockerfile
 create mode 100644 ci/containers/libvirt-centos-stream.Dockerfile
 create mode 100644 ci/containers/libvirt-debian-10.Dockerfile
 create mode 100644 ci/containers/libvirt-debian-9.Dockerfile
 create mode 100644 ci/containers/libvirt-debian-sid.Dockerfile
 create mode 100644 ci/containers/libvirt-fedora-31.Dockerfile
 create mode 100644 ci/containers/libvirt-fedora-32.Dockerfile
 create mode 100644 ci/containers/libvirt-fedora-rawhide.Dockerfile
 create mode 100644 ci/containers/libvirt-opensuse-151.Dockerfile
 create mode 100644 ci/containers/libvirt-ubuntu-1804.Dockerfile
 create mode 100644 ci/containers/libvirt-ubuntu-2004.Dockerfile
 create mode 100755 ci/containers/refresh

-- 
2.26.2

Re: [PATCH 11/25] network: use g_free() in place of remaining VIR_FREE()

[Daniel P . Berrangé]

On Thu, Jun 25, 2020 at 11:01:48AM -0400, Laine Stump wrote:
> On 6/25/20 3:55 AM, Peter Krempa wrote:
> > On Wed, Jun 24, 2020 at 23:34:00 -0400, Laine Stump wrote:
> > > Signed-off-by: Laine Stump 
> > > ---
> > >   src/network/bridge_driver.c | 59 +
> > >   1 file changed, 33 insertions(+), 26 deletions(-)
> > > 
> > > diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> > > index 668aa9ca88..a1b2f5b6c7 100644
> > > --- a/src/network/bridge_driver.c
> > > +++ b/src/network/bridge_driver.c
> > [...]
> > 
> > > @@ -706,7 +706,8 @@ networkStateInitialize(bool privileged,
> > >   network_driver->lockFD = -1;
> > >   if (virMutexInit(_driver->lock) < 0) {
> > > -VIR_FREE(network_driver);
> > > +g_free(network_driver);
> > > +network_driver = NULL;
> > >   goto error;
> > In general I'm agains senseless replacement of VIR_FREE for g_free.
> > There is IMO no value to do so. VIR_FREE is now implemented via
> > g_clear_pointer(, g_free) so g_free is actually used.
> > 
> > Mass replacements are also substrate for adding bugs and need to be
> > approached carefully, so doing this en-mass might lead to others
> > attempting the same with possibly less care.


> > In general, mass replacements should be done only to
> > 
> > g_clear_pointer(, g_free)
> > 
> > and I'm not sure it's worth it.
> > 
> 
> There's no getting around it - that looks ugly. And who wants to replace
> 5506 occurences of one simple-looking thing with something else that's
> functionally equivalent but more painful to look at?
> 
> 
> I would vote for just documenting that, for safety and consistency reasons,
> VIR_FREE() should always be used instead of g_free(), and eliminating all
> direct use of g_free() (along with the aforementioned syntax check). (BTW, I
> had assumed there had been more changes to g_free(), but when I looked at my
> current tree just now, there were only 228 occurences, including the changes
> in this patch)

The point in getting rid of VIR_FREE is so that we reduce the libvirt
specific wrappers in favour of standard APIs.

A large portion of the VIR_FREE's will be eliminated by g_autoptr.

Another large set of them are used in the virFooStructFree() methods.
Those can all be converted to g_free safely, as all the methods do
is free stuff.

Most VIR_FREEs that occur at the exit of functions can also be
safely converted to g_free, if g_autoptr  isnt applicable. Sometimes
needs a little care if you have multiple goto jumps between labels.

The big danger cases are the VIR_FREE()s that occur in the middle
of methods, especially in loop bodies. Those the ones that must
use the g_clear_pointer, and that's not very many of those, so the
ugly syntax isn't an issue.

So I see no reason to keep VIR_FREE around long term.

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 11/25] network: use g_free() in place of remaining VIR_FREE()

[Laine Stump]

On 6/25/20 3:55 AM, Peter Krempa wrote:

On Wed, Jun 24, 2020 at 23:34:00 -0400, Laine Stump wrote:

Signed-off-by: Laine Stump 
---
  src/network/bridge_driver.c | 59 +
  1 file changed, 33 insertions(+), 26 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 668aa9ca88..a1b2f5b6c7 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c

[...]


@@ -706,7 +706,8 @@ networkStateInitialize(bool privileged,
  
  network_driver->lockFD = -1;

  if (virMutexInit(_driver->lock) < 0) {
-VIR_FREE(network_driver);
+g_free(network_driver);
+network_driver = NULL;
  goto error;

In general I'm agains senseless replacement of VIR_FREE for g_free.
There is IMO no value to do so. VIR_FREE is now implemented via
g_clear_pointer(, g_free) so g_free is actually used.

Mass replacements are also substrate for adding bugs and need to be
approached carefully, so doing this en-mass might lead to others
attempting the same with possibly less care.



Actually I agree with you :-)


When we started into all this glib stuff, I thought that it was kind of 
unproductive to churn the code around in cases where it was just 
renaming one thing to something else - aside from (as you point out) 
being a siren call for regressions, this also makes backports to old 
branches more annoying, obscures *actual* functional history, and 
besides, what happens the *next* time we want to change how we do 
[whatever thing we're changing]? Do we do yet another global replacement 
for the "new new hotness"? But this was one of those things that didn't 
seem worth getting in the way of (and in balance it was definitely a net 
win), so I mostly ignored it (including not going out of my way to 
convert any code over just for the sake of converting).



In the meantime, lots and lots of patches have come in converting this 
stuff piecemeal over the codebase, and it's all becoming more and more 
g_*-centric. I still didn't really bother with it much.



Then I saw a memory leak in a patch a couple weeks ago that wouldn't 
have occurred if the existing function had used g_autofree (and thus 
reminded the author to use g_autofree for their additions to this 
existing function). This led me to make a patch to convert that file to 
use g_autofree and g_autoptr wherever possible, which in turn got me to 
look at xmlBuffer allocation/free and notice a couple bugs, which led to 
noticing something else inconsistent with current style, which led to 
noticing some other existing bug, and from there to something else ad 
infinitum.



So this one recognition of a single memory leak organically led to a 
bunch of drive-by patches, but the drive-by patches left everything in 
an in-between limbo state - half of things were the "old way" and half 
were the "new way". Somewhere in the middle of all this, I looked back 
at a recent set of patches from danpb for reference, and saw that along 
with making locals g_auto*, and changing VIR_ALLOC to g_new0, he had 
also replaced VIR_FREE with g_free, so I figured I should probably do 
that too while I was already churning things. The semantic change (no 
longer setting the pointer to the freed memory to NULL) was bothered me, 
but since it was already being used, I assumed there must have been 
discussion about it among all the glib conversion mails I skipped over, 
and decided to make my patches consistent with "current convention", and 
just carefully examine each usage to assure that either the pointer 
wasn't referenced after free, or that it was explicitly set to NULL.



I do recognize your concern that "some other people" (thanks for 
explicitly, though incorrectly, excluding me! :-)) may not be as 
diligent when doing a similar replacement though, and even after doing 
it myself I have concern that I may have missed something.



And now you point out the new implementation to VIR_FREE() (*yet 
another* change missed by me, as with so many other things that whiz by 
on the mailing list) that uses g_clear_pointer (which, having not read 
through the glib reference manual nor worked on other projects using 
glib, I didn't know about until today)! This validates my original 
apprehension (in the before before time) about replacing VIR_* with g_* 
macros - when we use our own macros it may be slightly more difficult 
for first-time readers of the code who *might* have already been 
familiar with glib (or maybe not), but it allows us to easily change the 
underlying implementation in the future without yet again churning 
through all the code.



This convinces me that VIR_FREE shouldn't be replaced with g_free in 
*any* circumstance. As a matter of fact, I would even go so far as to 
say that use of g_free() should be .. er "prohibited" with a syntax 
check (or would that be limiting free speech?).



(BTW, in case someone might bring up the argument of "g_free() should be 
used in 

Re: [PATCH] qemuxml2xmltest: Set dummy non-hypervisor drivers

[Daniel P . Berrangé]

On Thu, Jun 25, 2020 at 04:37:59PM +0200, Michal Privoznik wrote:
> When parsing domain XML post parse callbacks are run and one of
> them might try and call API from a non-hypervisor driver (e.g.
> just like qemuDomainDeviceNetDefPostParse() is doing - it calls a
> network API). To avoid this in the test suite, set dummy drivers,
> which renders all non-hypervisor APIs return error.
> 
> This mimics what qemuxml2argvtest does.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  tests/qemuxml2xmltest.c | 12 
>  1 file changed, 12 insertions(+)

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 libvirt-dbus v2 2/3] gitlab: introduce CI jobs testing git master & distro libvirt

[Daniel P . Berrangé]

On Thu, Jun 25, 2020 at 04:34:51PM +0200, Andrea Bolognani wrote:
> On Thu, 2020-06-25 at 15:20 +0100, Daniel P. Berrangé wrote:
> > On Thu, Jun 25, 2020 at 04:16:35PM +0200, Andrea Bolognani wrote:
> > > We should really try to figure out why the packages are absent from
> > > the repos, however. Wasn't the PowerTools repo supposed to take care
> > > of that? I'm pretty sure I've seen this situation for at least one
> > > package myself.
> > 
> > It isn't in the repos that I've looked at
> > 
> >   http://linux-mirrors.fnal.gov/linux/centos/8/BaseOS/x86_64/os/Packages/
> >   http://linux-mirrors.fnal.gov/linux/centos/8/AppStream/x86_64/os/Packages/
> >   
> > http://linux-mirrors.fnal.gov/linux/centos/8/PowerTools/x86_64/os/Packages/
> > 
> > and I don't really want to spend time debugging modularity in centos
> 
> What about
> 
>   
> http://linux-mirrors.fnal.gov/linux/centos/8/virt/x86_64/advanced-virtualization/Packages/l/
> 
> ? I can see libvirt-gobject-devel in there.

I dont see any advanced-virt for centos stream though


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

[PATCH] qemuxml2xmltest: Set dummy non-hypervisor drivers

[Michal Privoznik]

When parsing domain XML post parse callbacks are run and one of
them might try and call API from a non-hypervisor driver (e.g.
just like qemuDomainDeviceNetDefPostParse() is doing - it calls a
network API). To avoid this in the test suite, set dummy drivers,
which renders all non-hypervisor APIs return error.

This mimics what qemuxml2argvtest does.

Signed-off-by: Michal Privoznik 
---
 tests/qemuxml2xmltest.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index 5a124853b4..11ff17d83c 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -135,6 +135,7 @@ mymain(void)
 char *fakerootdir;
 virQEMUDriverConfigPtr cfg = NULL;
 virHashTablePtr capslatest = NULL;
+g_autoptr(virConnect) conn = NULL;
 
 capslatest = testQemuGetLatestCaps();
 if (!capslatest)
@@ -164,6 +165,16 @@ mymain(void)
 cfg = virQEMUDriverGetConfig();
 driver.privileged = true;
 
+if (!(conn = virGetConnect()))
+goto cleanup;
+
+virSetConnectInterface(conn);
+virSetConnectNetwork(conn);
+virSetConnectNWFilter(conn);
+virSetConnectNodeDev(conn);
+virSetConnectSecret(conn);
+virSetConnectStorage(conn);
+
 # define DO_TEST_INTERNAL(_name, suffix, when, ...) \
 do { \
 static struct testQemuInfo info = { \
@@ -1471,6 +1482,7 @@ mymain(void)
 DO_TEST_CAPS_LATEST("virtio-9p-multidevs");
 DO_TEST("downscript", NONE);
 
+ cleanup:
 if (getenv("LIBVIRT_SKIP_CLEANUP") == NULL)
 virFileDeleteTree(fakerootdir);
 
-- 
2.26.2

Re: [PATCH libvirt-dbus v2 2/3] gitlab: introduce CI jobs testing git master & distro libvirt

[Andrea Bolognani]

On Thu, 2020-06-25 at 15:20 +0100, Daniel P. Berrangé wrote:
> On Thu, Jun 25, 2020 at 04:16:35PM +0200, Andrea Bolognani wrote:
> > We should really try to figure out why the packages are absent from
> > the repos, however. Wasn't the PowerTools repo supposed to take care
> > of that? I'm pretty sure I've seen this situation for at least one
> > package myself.
> 
> It isn't in the repos that I've looked at
> 
>   http://linux-mirrors.fnal.gov/linux/centos/8/BaseOS/x86_64/os/Packages/
>   http://linux-mirrors.fnal.gov/linux/centos/8/AppStream/x86_64/os/Packages/
>   http://linux-mirrors.fnal.gov/linux/centos/8/PowerTools/x86_64/os/Packages/
> 
> and I don't really want to spend time debugging modularity in centos

What about

  
http://linux-mirrors.fnal.gov/linux/centos/8/virt/x86_64/advanced-virtualization/Packages/l/

? I can see libvirt-gobject-devel in there.

-- 
Andrea Bolognani / Red Hat / Virtualization

[libvirt PATCH v2] qemu: ramfb video device doesn't support PCI address

[Jonathon Jongsma]

Although a ramfb video device is not a PCI device, we don't currently
report an error for ramfb device definitions containing a PCI address.
However, a guest configured with such a device will fail to start:

# virsh start test1
error: Failed to start domain test1
error: internal error: qemu unexpectedly closed the monitor: 
2020-06-16T05:23:02.759221Z qemu-kvm: -device 
ramfb,id=video0,bus=pcie.0,addr=0x1: Device 'ramfb' can't go on PCIE bus

A better approach is to reject any device definitions that contain PCI
addresses.  While this is a change in behavior, any existing
configurations were non-functional.

https://bugzilla.redhat.com/show_bug.cgi?id=1847259

Signed-off-by: Jonathon Jongsma 
---
changes in v2:
 - move validation to qemu-specific validation function as suggested by Laine

 src/qemu/qemu_validate.c | 8 
 tests/qemuxml2argvtest.c | 1 +
 2 files changed, 9 insertions(+)

diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index 5082a29dc7..b13c03759e 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -1925,6 +1925,14 @@ qemuValidateDomainDeviceDefVideo(const virDomainVideoDef 
*video,
 if (qemuValidateDomainVirtioOptions(video->virtio, qemuCaps) < 0)
 return -1;
 
+if (video->type == VIR_DOMAIN_VIDEO_TYPE_RAMFB &&
+video->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("'address' is not supported for 'ramfb' video 
devices"));
+return -1;
+}
+
+
 return 0;
 }
 
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 1195f9c982..f2522fa530 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -2276,6 +2276,7 @@ mymain(void)
 QEMU_CAPS_VIRTIO_GPU_MAX_OUTPUTS);
 DO_TEST_CAPS_LATEST("video-bochs-display-device");
 DO_TEST_CAPS_LATEST("video-ramfb-display-device");
+DO_TEST_CAPS_LATEST_PARSE_ERROR("video-ramfb-display-device-pci-address");
 DO_TEST("video-none-device",
 QEMU_CAPS_VNC);
 DO_TEST_PARSE_ERROR("video-invalid-multiple-devices", NONE);
-- 
2.21.3

Re: [libvirt PATCH] ci: Refresh Dockerfiles

[Andrea Bolognani]

On Thu, 2020-06-25 at 15:25 +0100, Daniel P. Berrangé wrote:
> On Thu, Jun 25, 2020 at 04:19:16PM +0200, Andrea Bolognani wrote:
> > All targets get pip3, which is now part of the base system, and
> > the number of native packages included in the MinGW containers is
> > significantly reduced.
> 
> FWIW, I thinkit would be benefit to reduce and even eliminate
> the base set in the futre. It didn't have any downsides in
> CentOS CI, since we had VMs with all projects installed.
> 
> With containers though, it feels silly to install a pile of
> python stuff in containers for golang projects. Or install
> autotools in meson projects and vica-verca.

Yeah, we can certainly do better. In due time :)

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt PATCH] ci: Refresh Dockerfiles

[Daniel P . Berrangé]

On Thu, Jun 25, 2020 at 04:19:16PM +0200, Andrea Bolognani wrote:
> All targets get pip3, which is now part of the base system, and
> the number of native packages included in the MinGW containers is
> significantly reduced.

FWIW, I thinkit would be benefit to reduce and even eliminate
the base set in the futre. It didn't have any downsides in
CentOS CI, since we had VMs with all projects installed.

With containers though, it feels silly to install a pile of
python stuff in containers for golang projects. Or install
autotools in meson projects and vica-verca.

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 libvirt-dbus v2 2/3] gitlab: introduce CI jobs testing git master & distro libvirt

[Daniel P . Berrangé]

On Thu, Jun 25, 2020 at 04:16:35PM +0200, Andrea Bolognani wrote:
> On Thu, 2020-06-25 at 13:32 +0100, Daniel P. Berrangé wrote:
> > On Wed, Jun 24, 2020 at 07:16:16PM +0200, Andrea Bolognani wrote:
> > > What's the rationale for building libvirt and libvirt-glib from git
> > > on CentOS Stream in addition to CentOS 8?
> > 
> > There's something odd with the repos, probably due to modularity. The
> > end result is that the libvirt-gobject-devel packages are missing, and
> > I've not found where they might live. This affects 8 & 8-Stream the
> > same.
> 
> Oh, so you're building libvirt-glib from git in order to work around
> this limitation. Sound reasonable, if hacky :)
> 
> We should really try to figure out why the packages are absent from
> the repos, however. Wasn't the PowerTools repo supposed to take care
> of that? I'm pretty sure I've seen this situation for at least one
> package myself.

It isn't in the repos that I've looked at

  http://linux-mirrors.fnal.gov/linux/centos/8/BaseOS/x86_64/os/Packages/
  http://linux-mirrors.fnal.gov/linux/centos/8/AppStream/x86_64/os/Packages/
  http://linux-mirrors.fnal.gov/linux/centos/8/PowerTools/x86_64/os/Packages/

and I don't really want to spend time debugging modularity in centos

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 libvirt-dbus v2 2/3] gitlab: introduce CI jobs testing git master & distro libvirt

[Andrea Bolognani]

On Thu, 2020-06-25 at 13:32 +0100, Daniel P. Berrangé wrote:
> On Wed, Jun 24, 2020 at 07:16:16PM +0200, Andrea Bolognani wrote:
> > What's the rationale for building libvirt and libvirt-glib from git
> > on CentOS Stream in addition to CentOS 8?
> 
> There's something odd with the repos, probably due to modularity. The
> end result is that the libvirt-gobject-devel packages are missing, and
> I've not found where they might live. This affects 8 & 8-Stream the
> same.

Oh, so you're building libvirt-glib from git in order to work around
this limitation. Sound reasonable, if hacky :)

We should really try to figure out why the packages are absent from
the repos, however. Wasn't the PowerTools repo supposed to take care
of that? I'm pretty sure I've seen this situation for at least one
package myself.

-- 
Andrea Bolognani / Red Hat / Virtualization

[libvirt PATCH] ci: Refresh Dockerfiles

[Andrea Bolognani]

All targets get pip3, which is now part of the base system, and
the number of native packages included in the MinGW containers is
significantly reduced.

The corresponding libvirt-ci commit is 4ff697ba0b5d.

Signed-off-by: Andrea Bolognani 
---
Pushed under the Dockerfile refresh rule.

 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 |  1 +
 .../libvirt-debian-10-cross-armv7l.Dockerfile |  1 +
 .../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 |  1 +
 ...libvirt-debian-sid-cross-armv7l.Dockerfile |  1 +
 .../libvirt-debian-sid-cross-i686.Dockerfile  |  1 +
 ...bvirt-debian-sid-cross-mips64el.Dockerfile |  1 +
 ...libvirt-debian-sid-cross-mipsel.Dockerfile |  1 +
 ...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|  1 +
 ci/containers/libvirt-fedora-32.Dockerfile|  1 +
 ...rt-fedora-rawhide-cross-mingw32.Dockerfile | 42 +--
 ...rt-fedora-rawhide-cross-mingw64.Dockerfile | 42 +--
 .../libvirt-fedora-rawhide.Dockerfile |  1 +
 ci/containers/libvirt-ubuntu-2004.Dockerfile  |  1 +
 27 files changed, 29 insertions(+), 80 deletions(-)

diff --git a/ci/containers/libvirt-centos-8.Dockerfile 
b/ci/containers/libvirt-centos-8.Dockerfile
index b6510834de..f376126d93 100644
--- a/ci/containers/libvirt-centos-8.Dockerfile
+++ b/ci/containers/libvirt-centos-8.Dockerfile
@@ -76,6 +76,7 @@ RUN dnf install 'dnf-command(config-manager)' -y && \
 python3 \
 python3-docutils \
 python3-flake8 \
+python3-pip \
 python3-setuptools \
 python3-wheel \
 qemu-img \
diff --git a/ci/containers/libvirt-centos-stream.Dockerfile 
b/ci/containers/libvirt-centos-stream.Dockerfile
index f65580b67d..bc75c95193 100644
--- a/ci/containers/libvirt-centos-stream.Dockerfile
+++ b/ci/containers/libvirt-centos-stream.Dockerfile
@@ -77,6 +77,7 @@ RUN dnf install -y centos-release-stream && \
 python3 \
 python3-docutils \
 python3-flake8 \
+python3-pip \
 python3-setuptools \
 python3-wheel \
 qemu-img \
diff --git a/ci/containers/libvirt-debian-10-cross-aarch64.Dockerfile 
b/ci/containers/libvirt-debian-10-cross-aarch64.Dockerfile
index 3620bbdf7f..a03a7fd26a 100644
--- a/ci/containers/libvirt-debian-10-cross-aarch64.Dockerfile
+++ b/ci/containers/libvirt-debian-10-cross-aarch64.Dockerfile
@@ -45,6 +45,7 @@ RUN export DEBIAN_FRONTEND=noninteractive && \
 policykit-1 \
 python3 \
 python3-docutils \
+python3-pip \
 python3-setuptools \
 python3-wheel \
 qemu-utils \
diff --git a/ci/containers/libvirt-debian-10-cross-armv6l.Dockerfile 
b/ci/containers/libvirt-debian-10-cross-armv6l.Dockerfile
index 5d390d0160..4f673cb3e2 100644
--- a/ci/containers/libvirt-debian-10-cross-armv6l.Dockerfile
+++ b/ci/containers/libvirt-debian-10-cross-armv6l.Dockerfile
@@ -45,6 +45,7 @@ RUN export DEBIAN_FRONTEND=noninteractive && \
 policykit-1 \
 python3 \
 python3-docutils \
+python3-pip \
 python3-setuptools \
 python3-wheel \
 qemu-utils \
diff --git a/ci/containers/libvirt-debian-10-cross-armv7l.Dockerfile 
b/ci/containers/libvirt-debian-10-cross-armv7l.Dockerfile
index 2fdbe99bff..fa5fae4399 100644
--- a/ci/containers/libvirt-debian-10-cross-armv7l.Dockerfile
+++ b/ci/containers/libvirt-debian-10-cross-armv7l.Dockerfile
@@ -45,6 +45,7 @@ RUN export DEBIAN_FRONTEND=noninteractive && \
 policykit-1 \
 python3 \
 python3-docutils \
+python3-pip \
 python3-setuptools \
 python3-wheel \
 qemu-utils \
diff --git a/ci/containers/libvirt-debian-10-cross-i686.Dockerfile 
b/ci/containers/libvirt-debian-10-cross-i686.Dockerfile
index 6517a4cb2e..a3bc96ac7a 100644
--- a/ci/containers/libvirt-debian-10-cross-i686.Dockerfile
+++ b/ci/containers/libvirt-debian-10-cross-i686.Dockerfile
@@ -45,6 +45,7 @@ RUN export DEBIAN_FRONTEND=noninteractive && \
 policykit-1 \
 python3 \
 python3-docutils \
+python3-pip \
 python3-setuptools \
 python3-wheel \
 qemu-utils \

Re: [libvirt PATCH] qemu: ramfb video device doesn't support PCI address

[Jonathon Jongsma]

On Wed, 2020-06-24 at 23:21 -0400, Laine Stump wrote:
> On 6/24/20 6:06 PM, Jonathon Jongsma wrote:
> > Although a ramfb video device is not a PCI device, we don't
> > currently
> > report an error for ramfb device definitions containing a PCI
> > address.
> > However, a guest configured with such a device will fail to start:
> > 
> >  # virsh start test1
> >  error: Failed to start domain test1
> >  error: internal error: qemu unexpectedly closed the monitor:
> > 2020-06-16T05:23:02.759221Z qemu-kvm: -device
> > ramfb,id=video0,bus=pcie.0,addr=0x1: Device 'ramfb' can't go on
> > PCIE bus
> > 
> > A better approach is to reject any device definitions that contain
> > PCI
> > addresses.  While this is a change in behavior, any existing
> > configurations were non-functional.
> > 
> > https://bugzilla.redhat.com/show_bug.cgi?id=1847259
> > 
> > Signed-off-by: Jonathon Jongsma 
> > ---
> >   src/conf/domain_conf.c   | 7 +++
> >   tests/qemuxml2argvtest.c | 1 +
> >   2 files changed, 8 insertions(+)
> > 
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index fc7fcfb0c6..1a06cb3f4b 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -6608,6 +6608,13 @@ virDomainVideoDefValidate(const
> > virDomainVideoDef *video,
> >   return -1;
> >   }
> >   
> > +if (video->type == VIR_DOMAIN_VIDEO_TYPE_RAMFB &&
> > +video->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
> > +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > +   _("'address' is not supported for 'ramfb'
> > video devices"));
> > +return -1;
> > +}
> > +
> 
> There may be disagreement about this, and in practical terms it makes
> no 
> difference (because no domain type other than QEMU is ever going to
> have 
> one of these devices anyway), but since ramfb is a qemu-specific
> device 
> (isn't it?), I think this check would be better put in 
> qemu_validate.c:qemuValidateDomainDeviceDevVideo(), which is the 
> qemu-specific validation function for video devices.
> 
> 
> (I  see there is already validation in virDomainVideoDefValidate()
> for a 
> qemu-specific (afaik) video type - i is even checking for one
> backend 
> that is named VIR_DOMAIN_VIDEO_BACKEND_TYPE_QEMU)
> 
> 
> I don't really have a strong opinion though, since the other
> hypervisor 
> drivers don't seem all that concerned about fleshing out their 
> validation functions, and what you have works properly for qemu :-P

For what it's worth, I agree with you. I'll re-spin the patch.

Jonathon

Re: [PATCH] qemuDomainDeviceNetDefPostParse: Switch order of conditions

[Daniel P . Berrangé]

On Thu, Jun 25, 2020 at 03:54:18PM +0200, Michal Privoznik wrote:
> On 6/25/20 10:38 AM, Daniel P. Berrangé wrote:
> > On Thu, Jun 25, 2020 at 09:48:56AM +0200, Michal Privoznik wrote:
> > > A few commits back (in v6.4.0-131-gbdb8f2e418) the post parse
> > > function for domain interface was changed so that it doesn't fill
> > > in model for hostdev types of interfaces (including network type
> > > interfaces which would end up hostdevs).
> > > 
> > > While the idea is sound, the execution can be a bit better:
> > > virDomainNetResolveActualType() which is used to determine
> > > runtime type of given interface is heavy gun - it connects to
> > > network driver, fetches network XML, parses it. This all is
> > > followed by check whether the interface doesn't already have
> > > model set (from domain XML).
> > > 
> > > If we switch the order of these two checks then the short circuit
> > > evaluation will ensure the expensive check is done only if really
> > > needed.
> > > 
> > > This commit in fact fixes qemuxml2xmltest which due to lacking
> > > fake network driver tries to connect to network:///session and
> > > start the virtnetworkd. Fortunately, because of
> > > v6.3.0-25-gf28fbb05d3 it fails to do so and
> > > virDomainNetResolveActualType() returns -1. The only reason we
> > > don't see the test failing is because our input XMLs have model
> > > and thus we are saved by the latter (now former) check.
> > > 
> > > Signed-off-by: Michal Privoznik 
> > > ---
> > > 
> > > While this patch is technically correct (the best way to be correct), it
> > > can also be viewed as papering over the real issue. Question is, how to
> > > address that. Nor xml2xml test nor xml2argv test are creating fake
> > > network driver. Is mocking virDomainNetResolveActualType() the way to go
> > > then? Or should we create the fake network driver with networks and
> > > everything?
> > 
> > The qemuxml2argtest calls virSetConnectNetwork() to provide a stub
> > network driver, likewise for other secondary drivers. This prevents
> > any risk of spawning daemons. We should probably do that for the
> > other tests too.
> 
> What do you mean by "stub driver"? I can see conn->networkDriver = NULL. I
> mean, it works, but it's not a stub driver :-) Basically any virNetwork*
> call fails and we merely just let the code deal with it.

Yeah, by stub i mean something that prevents any network APIs from
working.

xs

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] qemuDomainDeviceNetDefPostParse: Switch order of conditions

[Michal Privoznik]

On 6/25/20 10:38 AM, Daniel P. Berrangé wrote:

On Thu, Jun 25, 2020 at 09:48:56AM +0200, Michal Privoznik wrote:

A few commits back (in v6.4.0-131-gbdb8f2e418) the post parse
function for domain interface was changed so that it doesn't fill
in model for hostdev types of interfaces (including network type
interfaces which would end up hostdevs).

While the idea is sound, the execution can be a bit better:
virDomainNetResolveActualType() which is used to determine
runtime type of given interface is heavy gun - it connects to
network driver, fetches network XML, parses it. This all is
followed by check whether the interface doesn't already have
model set (from domain XML).

If we switch the order of these two checks then the short circuit
evaluation will ensure the expensive check is done only if really
needed.

This commit in fact fixes qemuxml2xmltest which due to lacking
fake network driver tries to connect to network:///session and
start the virtnetworkd. Fortunately, because of
v6.3.0-25-gf28fbb05d3 it fails to do so and
virDomainNetResolveActualType() returns -1. The only reason we
don't see the test failing is because our input XMLs have model
and thus we are saved by the latter (now former) check.

Signed-off-by: Michal Privoznik 
---

While this patch is technically correct (the best way to be correct), it
can also be viewed as papering over the real issue. Question is, how to
address that. Nor xml2xml test nor xml2argv test are creating fake
network driver. Is mocking virDomainNetResolveActualType() the way to go
then? Or should we create the fake network driver with networks and
everything?


The qemuxml2argtest calls virSetConnectNetwork() to provide a stub
network driver, likewise for other secondary drivers. This prevents
any risk of spawning daemons. We should probably do that for the
other tests too.


What do you mean by "stub driver"? I can see conn->networkDriver = NULL. 
I mean, it works, but it's not a stub driver :-) Basically any 
virNetwork* call fails and we merely just let the code deal with it.


Anyway, I will post that as a follow up patch, but this opens an 
interesting question - every test that parses a domain XML will run post 
parse callbacks and thus potentially suffers from the same problem.
But that shouldn't block this patch because as I say, this can be viewed 
as performance improvement.


Michal

Re: [libvirt-cim PATCH v2 0/2] Introduce GitLab CI and merge requests

[Andrea Bolognani]

On Thu, 2020-06-25 at 13:42 +0100, Daniel P. Berrangé wrote:
> Daniel P. Berrangé (2):
>   gitlab: introduce CI jobs testing git master & distro libvirt
>   gitlab: add CONTRIBUTING.rst file to indicate use of merge requests
> 
>  .gitlab-ci.yml| 114 ++
>  .gitpublish   |   4 -
>  CONTRIBUTING.rst  |  28 +
>  ci/containers/README.rst  |  14 +++
>  ci/containers/libvirt-centos-7.Dockerfile |  96 +++
>  ci/containers/libvirt-fedora-31.Dockerfile|  56 +
>  ci/containers/libvirt-fedora-32.Dockerfile|  56 +
>  .../libvirt-fedora-rawhide.Dockerfile |  57 +
>  ci/containers/refresh |  27 +
>  9 files changed, 448 insertions(+), 4 deletions(-)

Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH] qemuDomainDeviceNetDefPostParse: Switch order of conditions

[Laine Stump]

On 6/25/20 3:48 AM, Michal Privoznik wrote:

A few commits back (in v6.4.0-131-gbdb8f2e418) the post parse
function for domain interface was changed so that it doesn't fill
in model for hostdev types of interfaces (including network type
interfaces which would end up hostdevs).

While the idea is sound, the execution can be a bit better:
virDomainNetResolveActualType() which is used to determine
runtime type of given interface is heavy gun - it connects to
network driver, fetches network XML, parses it. This all is
followed by check whether the interface doesn't already have
model set (from domain XML).

If we switch the order of these two checks then the short circuit
evaluation will ensure the expensive check is done only if really
needed.



Oops! I should have caught that when I reviewed the earlier commit.


Reviewed-by: Laine Stump 

[RFC PATCH] docs: backup: Convert XML documentation to RST

[Peter Krempa]

Switch to the new format for easier extension.

Signed-off-by: Peter Krempa 
---

This was a surprisingly easy conversion done using:

 pandoc --from html --to rst --toc --columns 80 --standalone 
formatbackup.html.in -o formatbackup.rst

The file generated by pandoc required following manual tweaks:
1) table of contents command needed to be moved after the first heading
2) definition list entries were not separated by an empty line
3) XML examples had one extra line with spaces only (Faithfully ported
 from html)

Here both versions can be compared:
https://pipo.sk.gitlab.io/-/libvirt/-/jobs/611434730/artifacts/website/formatbackup.html
https://libvirt.org/formatbackup.html


 docs/formatbackup.html.in | 191 --
 docs/formatbackup.rst | 149 +
 2 files changed, 149 insertions(+), 191 deletions(-)
 delete mode 100644 docs/formatbackup.html.in
 create mode 100644 docs/formatbackup.rst

diff --git a/docs/formatbackup.html.in b/docs/formatbackup.html.in
deleted file mode 100644
index 9e69d8f7d3..00
--- a/docs/formatbackup.html.in
+++ /dev/null
@@ -1,191 +0,0 @@
-
-
-http://www.w3.org/1999/xhtml;>
-  
-Backup XML format
-
-
-
-Backup XML
-
-
-  Creating a backup, whether full or incremental, is done
-  via virDomainBackupBegin(), which takes an XML
-  description of the actions to perform, as well as an optional
-  second XML document describing a
-  checkpoint to create at the same point in time. See
-  also a comparison between
-  the various state capture APIs.
-
-
-  There are two general modes for backups: a push mode (where the
-  hypervisor writes out the data to the destination file, which
-  may be local or remote), and a pull mode (where the hypervisor
-  creates an NBD server that a third-party client can then read as
-  needed, and which requires the use of temporary storage,
-  typically local, until the backup is complete).
-
-
-  The instructions for beginning a backup job are provided as
-  attributes and elements of the
-  top-level domainbackup element. This element
-  includes an optional attribute mode which can be
-  either "push" or "pull" (default
-  push). virDomainBackupGetXMLDesc() can be used to
-  see the actual values selected for elements omitted during
-  creation (for example, learning which port the NBD server is
-  using in the pull model or what file names libvirt generated
-  when none were supplied). The following child elements and attributes
-  are supported:
-
-
-  incremental
-  An optional element giving the name of an existing
-checkpoint of the domain, which will be used to make this
-backup an incremental one. In the push model, only changes
-since the named checkpoint are written to the destination. In
-the pull model, the NBD server uses the
-NBD_OPT_SET_META_CONTEXT extension to advertise to the client
-which portions of the export contain changes since the named
-checkpoint. If omitted, a full backup is performed.
-  
-  server
-  Present only for a pull mode backup. Contains the same
-attributes as
-the protocol
-element of a disk attached via NBD in the domain (such as
-transport, socket, name, port, or tls), necessary to set up an
-NBD server that exposes the content of each disk at the time
-the backup is started.
-  
-  disks
-  An optional listing of instructions for disks participating
-in the backup (if omitted, all disks participate and libvirt
-attempts to generate filenames by appending the current
-timestamp as a suffix). If the entire element was omitted on
-input, then all disks participate in the backup, otherwise,
-only the disks explicitly listed which do not also
-use backup='no' will participate. On output, this
-is the state of each of the domain's disk in relation to the
-backup operation.
-
-  disk
-  This sub-element describes the backup properties of a
-specific disk, with the following attributes and child
-elements:
-
-  name
-  A mandatory attribute which must match
-the target dev='name'/
-of one of
-the disk
-devices specified for the domain at the time of
-the checkpoint.
-  backup
-  Setting this attribute to yes(default) specifies
-that the disk should take part in the backup and using
-no excludes the disk from the backup.
-  exportname
-  Allows modification of the NBD export name for the given 
disk.
-By default equal 

[libvirt-cim PATCH v2 1/2] gitlab: introduce CI jobs testing git master & distro libvirt

[Daniel P . Berrangé]

The sandbox build needs to validate two axis

  - A variety of distro versions
  - A variety of libvirt versions

We test a variety of libvirt versions by running a build against the
distro provided libvirt packages. All that is then missing is a build
against the latest libvirt git master, which only needs to be run on
a single distro, for which CentOS 7 is picked as a stable long life
base.

Signed-off-by: Daniel P. Berrangé 
---
 .gitlab-ci.yml| 114 ++
 ci/containers/README.rst  |  14 +++
 ci/containers/libvirt-centos-7.Dockerfile |  96 +++
 ci/containers/libvirt-fedora-31.Dockerfile|  56 +
 ci/containers/libvirt-fedora-32.Dockerfile|  56 +
 .../libvirt-fedora-rawhide.Dockerfile |  57 +
 ci/containers/refresh |  27 +
 7 files changed, 420 insertions(+)
 create mode 100644 ci/containers/README.rst
 create mode 100644 ci/containers/libvirt-centos-7.Dockerfile
 create mode 100644 ci/containers/libvirt-fedora-31.Dockerfile
 create mode 100644 ci/containers/libvirt-fedora-32.Dockerfile
 create mode 100644 ci/containers/libvirt-fedora-rawhide.Dockerfile
 create mode 100755 ci/containers/refresh

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 50dae92..9d052a4 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -1,6 +1,78 @@
 
 stages:
   - prebuild
+  - containers
+  - builds
+
+.container_job_template: _job_definition
+  image: docker:stable
+  stage: containers
+  services:
+- docker:dind
+  before_script:
+- export TAG="$CI_REGISTRY_IMAGE/ci-$NAME:latest"
+- export COMMON_TAG="$CI_REGISTRY/libvirt/libvirt-cim/ci-$NAME:latest"
+- docker info
+- docker login registry.gitlab.com -u "$CI_REGISTRY_USER" -p 
"$CI_REGISTRY_PASSWORD"
+  script:
+- docker pull "$TAG" || docker pull "$COMMON_TAG" || true
+- docker build --cache-from "$TAG" --cache-from "$COMMON_TAG" --tag "$TAG" 
-f "ci/containers/libvirt-$NAME.Dockerfile" ci
+- docker push "$TAG"
+  after_script:
+- docker logout
+
+.script_variables: _variables |
+  export MAKEFLAGS="-j$(getconf _NPROCESSORS_ONLN)"
+  export SCRATCH_DIR="/tmp/scratch"
+  export VROOT="$SCRATCH_DIR/vroot"
+  export CCACHE_DIR="$PWD/ccache"
+  export CCACHE_MAXSIZE="500M"
+  export PATH="$CCACHE_WRAPPERSDIR:$VROOT/bin:$PATH"
+  export PKG_CONFIG_PATH="$VROOT/lib/pkgconfig"
+  export LD_LIBRARY_PATH="$VROOT/lib"
+
+.git_native_build_job_template: _native_build_job_definition
+  image: $CI_REGISTRY_IMAGE/ci-$NAME:latest
+  stage: builds
+  cache:
+paths:
+  - ccache/
+key: "$CI_JOB_NAME"
+  before_script:
+- *script_variables
+  script:
+- pushd "$PWD"
+- mkdir -p "$SCRATCH_DIR"
+- cd "$SCRATCH_DIR"
+- git clone --depth 1 https://gitlab.com/libvirt/libvirt.git
+- mkdir libvirt/build
+- cd libvirt/build
+- ../autogen.sh --prefix="$VROOT" --without-libvirtd
+- $MAKE install
+- popd
+- mkdir build
+- cd build
+- ../autogen.sh --prefix="$VROOT"
+- $MAKE install
+- $MAKE dist
+- if test -x /usr/bin/rpmbuild && test "$RPM" != "skip" ; then rpmbuild 
--nodeps -ta libvirt-cim*.tar.gz ; fi
+
+.dist_native_build_job_template: _native_build_job_definition
+  image: $CI_REGISTRY_IMAGE/ci-$NAME:latest
+  stage: builds
+  cache:
+paths:
+  - ccache/
+key: "$CI_JOB_NAME"
+  before_script:
+- *script_variables
+  script:
+- mkdir build
+- cd build
+- ../autogen.sh --prefix="$VROOT"
+- $MAKE install
+- $MAKE dist
+- if test -x /usr/bin/rpmbuild && test "$RPM" != "skip" ; then rpmbuild 
-ta libvirt-cim*.tar.gz ; fi
 
 # Check that all commits are signed-off for the DCO.
 # Skip on "libvirt" namespace, since we only need to run
@@ -14,3 +86,45 @@ check-dco:
   except:
 variables:
   - $CI_PROJECT_NAMESPACE == 'libvirt'
+
+x64-centos-7-container:
+  <<: *container_job_definition
+  variables:
+NAME: centos-7
+
+x64-fedora-31-container:
+  <<: *container_job_definition
+  variables:
+NAME: fedora-31
+
+x64-fedora-32-container:
+  <<: *container_job_definition
+  variables:
+NAME: fedora-32
+
+x64-fedora-rawhide-container:
+  <<: *container_job_definition
+  variables:
+NAME: fedora-rawhide
+
+
+
+x64-centos-7-git-build:
+  <<: *git_native_build_job_definition
+  variables:
+NAME: centos-7
+
+x64-fedora-31-dist-build:
+  <<: *dist_native_build_job_definition
+  variables:
+NAME: fedora-31
+
+x64-fedora-32-dist-build:
+  <<: *dist_native_build_job_definition
+  variables:
+NAME: fedora-32
+
+x64-fedora-rawhide-dist-build:
+  <<: *dist_native_build_job_definition
+  variables:
+NAME: fedora-rawhide
diff --git a/ci/containers/README.rst b/ci/containers/README.rst
new file mode 100644
index 000..530897e
--- /dev/null
+++ b/ci/containers/README.rst
@@ -0,0 +1,14 @@
+CI job assets
+=
+
+This directory contains assets used in the automated CI jobs, most

[libvirt-cim PATCH v2 0/2] Introduce GitLab CI and merge requests

[Daniel P . Berrangé]

Daniel P. Berrangé (2):
  gitlab: introduce CI jobs testing git master & distro libvirt
  gitlab: add CONTRIBUTING.rst file to indicate use of merge requests

 .gitlab-ci.yml| 114 ++
 .gitpublish   |   4 -
 CONTRIBUTING.rst  |  28 +
 ci/containers/README.rst  |  14 +++
 ci/containers/libvirt-centos-7.Dockerfile |  96 +++
 ci/containers/libvirt-fedora-31.Dockerfile|  56 +
 ci/containers/libvirt-fedora-32.Dockerfile|  56 +
 .../libvirt-fedora-rawhide.Dockerfile |  57 +
 ci/containers/refresh |  27 +
 9 files changed, 448 insertions(+), 4 deletions(-)
 delete mode 100644 .gitpublish
 create mode 100644 CONTRIBUTING.rst
 create mode 100644 ci/containers/README.rst
 create mode 100644 ci/containers/libvirt-centos-7.Dockerfile
 create mode 100644 ci/containers/libvirt-fedora-31.Dockerfile
 create mode 100644 ci/containers/libvirt-fedora-32.Dockerfile
 create mode 100644 ci/containers/libvirt-fedora-rawhide.Dockerfile
 create mode 100755 ci/containers/refresh

-- 
2.26.2

[libvirt-cim PATCH v2 2/2] gitlab: add CONTRIBUTING.rst file to indicate use of merge requests

[Daniel P . Berrangé]

With the introduction of automated CI pipelines, we are now ready to switch
to using merge requests for the project. With this switch we longer wish
to have patches sent to the mailing list, and thus the git-publish
config is removed.

Reviewed-by: Andrea Bolognani 
Signed-off-by: Daniel P. Berrangé 
---
 .gitpublish  |  4 
 CONTRIBUTING.rst | 28 
 2 files changed, 28 insertions(+), 4 deletions(-)
 delete mode 100644 .gitpublish
 create mode 100644 CONTRIBUTING.rst

diff --git a/.gitpublish b/.gitpublish
deleted file mode 100644
index 2a466ac..000
--- a/.gitpublish
+++ /dev/null
@@ -1,4 +0,0 @@
-[gitpublishprofile "default"]
-base = master
-to = libvir-list@redhat.com
-prefix = libvirt-cim PATCH
diff --git a/CONTRIBUTING.rst b/CONTRIBUTING.rst
new file mode 100644
index 000..57a7951
--- /dev/null
+++ b/CONTRIBUTING.rst
@@ -0,0 +1,28 @@
+===
+Contributing to libvirt-cim
+===
+
+The libvirt CIM API binding accepts code contributions via merge requests
+on the GitLab project:
+
+https://gitlab.com/libvirt/libvirt-cim/-/merge_requests
+
+It is required that automated CI pipelines succeed before a merge request
+will be accepted. The global pipeline status for the ``master`` branch is
+visible at:
+
+https://gitlab.com/libvirt/libvirt-cim/pipelines
+
+CI pipeline results for merge requests will be visible via the contributors'
+own private repository fork:
+
+https://gitlab.com/yourusername/libvirt-cim/pipelines
+
+Contributions submitted to the project must be in compliance with the
+Developer Certificate of Origin Version 1.1. This is documented at:
+
+https://developercertificate.org/
+
+To indicate compliance, each commit in a series must have a "Signed-off-by"
+tag with the submitter's name and email address. This can be added by passing
+the ``-s`` flag to ``git commit`` when creating the patches.
-- 
2.26.2

Re: [PATCH libvirt-dbus v2 2/3] gitlab: introduce CI jobs testing git master & distro libvirt

[Daniel P . Berrangé]

On Wed, Jun 24, 2020 at 07:16:16PM +0200, Andrea Bolognani wrote:
> On Wed, 2020-06-24 at 17:26 +0100, Daniel P. Berrangé wrote:
> > The dbus build needs to validate one axis
> > 
> >   - A variety of libvirt versions
> > 
> > We test a variety of libvirt versions by running a build against the
> > distro provided libvirt packages. All that is then missing is a build
> > against the latest libvirt git master, which only needs to be run on
> > a single distro, for which CentOS 8 is picked as a stable long life
> > base.
> 
> This...
> 
> > +++ b/.gitlab-ci.yml
> > +x64-centos-8-git-build:
> > +  <<: *git_native_build_job_definition
> > +  variables:
> > +NAME: centos-8
> > +
> > +x64-centos-stream-git-build:
> > +  <<: *git_native_build_job_definition
> > +  variables:
> > +NAME: centos-stream
> 
> ... contradicts this...
> 
> > +++ b/ci/containers/refresh
> > +for host in $HOSTS
> > +do
> > +if test "$host" = "libvirt-centos-8" || test "$host" = 
> > "libvirt-centos-stream"
> > +then
> > +$LCITOOL dockerfile $host 
> > libvirt+minimal,libvirt-glib,libvirt-dbus > $host.Dockerfile
> 
> ... and this.
> 
> What's the rationale for building libvirt and libvirt-glib from git
> on CentOS Stream in addition to CentOS 8?

There's something odd with the repos, probably due to modularity. The
end result is that the libvirt-gobject-devel packages are missing, and
I've not found where they might live. This affects 8 & 8-Stream the
same.

> 
> > +if test "$host" = "libvirt-debian-9" || test "$host" = 
> > "libvirt-ubuntu-1804"
> > +then
> > +   sed -i -e 's/libvirt-dev/libvirt-dev libvirt-daemon/' $host.Dockerfile
> 
> This line is not indented correctly.
> 
> Additionally, please add a comment explaining why this hack is needed
> in the first place, something along the lines of
> 
>   Before Debian version 4.10.0-2, some of the runtime files needed by
>   libvirt were mistakenly shipped in the libvirt-daemon package
> 
> One more nitpick: the conditional would look nicer and be easier to
> tweak later as
> 
>   if test "$host" = "libvirt-debian-9" ||
>  test "$host" = "libvirt-ubuntu-1804"
>   then
> 
> Same applies above.

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 libvirt-cim 1/2] gitlab: introduce CI jobs testing git master & distro libvirt

[Daniel P . Berrangé]

On Wed, Jun 24, 2020 at 08:06:08PM +0200, Andrea Bolognani wrote:
> On Wed, 2020-06-24 at 17:20 +0100, Daniel P. Berrangé wrote:
> > diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> > new file mode 100644
> > index 000..75d34c3
> > --- /dev/null
> > +++ b/.gitlab-ci.yml
> 
> Oh and this doesn't apply cleanly on top of master, because you're
> trying to create a new file while .gitlab-ci.yml already exists. I
> think you made this commit on top of master^ instead of master.

Yeah, didn't realize my fork was outdated

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 libvirt-cim 1/2] gitlab: introduce CI jobs testing git master & distro libvirt

[Daniel P . Berrangé]

On Wed, Jun 24, 2020 at 07:39:21PM +0200, Andrea Bolognani wrote:
> On Wed, 2020-06-24 at 17:20 +0100, Daniel P. Berrangé wrote:
> > The sandbox build needs to validate two axis
> > 
> >   - A variety of distro versions
> >   - A variety of libvirt versions
> > 
> > We test a variety of libvirt versions by running a build against the
> > distro provided libvirt packages. All that is then missing is a build
> > against the latest libvirt git master, which only needs to be run on
> > a single distro, for which CentOS 7 is picked as a stable long life
> > base.
> 
> This...
> 
> > +x64-fedora-rawhide-git-build:
> > +  <<: *git_native_build_job_definition
> > +  variables:
> > +NAME: fedora-rawhide
> 
> ... disagrees with this...
> 
> > +++ b/ci/containers/refresh
> > +for host in $HOSTS
> > +do
> > +if test "$host" = "libvirt-fedora-rawhide"
> > +then
> > +$LCITOOL dockerfile $host libvirt+minimal,libvirt+dist,libvirt-cim 
> > > $host.Dockerfile
> 
> ... and this.
> 
> I think the commit message is correct here: we want git builds to
> happen on long-term supported distro, which Rawhide couldn't be
> further from, but perhaps I'm missing something?

It was a mistake

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 6/6] kbase: incrementalbackupinternals: Describe 'block commit'

[Peter Krempa]

On Wed, Jun 24, 2020 at 09:35:22 -0500, Eric Blake wrote:
> On 6/24/20 9:07 AM, Peter Krempa wrote:
> > oVirt does merge images out of libvirt in some cases. Add docs outlining
> > how it's done from a high level.
> > 
> > Signed-off-by: Peter Krempa 
> > ---
> >   docs/kbase/incrementalbackupinternals.rst | 37 +++
> >   1 file changed, 37 insertions(+)
> > 
> > diff --git a/docs/kbase/incrementalbackupinternals.rst 
> > b/docs/kbase/incrementalbackupinternals.rst
> > index 9a96ef6df3..e50bf52e89 100644
> > --- a/docs/kbase/incrementalbackupinternals.rst
> > +++ b/docs/kbase/incrementalbackupinternals.rst
> > @@ -245,3 +245,40 @@ the snapshot.
> >   continue
> > 
> >   create RECORDING bitmap named BITMAP in OVERLAY with GRANULARITY
> > +
> > +Commiting external snapshots manually
> 
> Committing
> 
> > +-
> > +
> > +``block commit`` refers to an operation where data from a subchain of the
> > +backing chain is merged down into the backing image of the subchain 
> > removing all
> > +images in the subchain .
> 
> Drop space before .
> 
> Does oVirt care about block pull, or only block commit?  But block pull can
> be a separate patch if we need more information; this one is useful as-is
> (well, with typos fixed).

oVirt doesn't use block pull for now and even libvirt doesn't implement
bitmap handling for the pull job for now. I'll add information here once
I do the implementation.

Re: [PATCH 1/6] kbase: incrementalbackupinternals: Add snapshot terminology

[Peter Krempa]

On Wed, Jun 24, 2020 at 09:23:00 -0500, Eric Blake wrote:
> On 6/24/20 9:07 AM, Peter Krempa wrote:
> > Make it obvious what's meant by 'overlay' and 'backing image' for sake
> > of extension of the document.
> > 
> > Signed-off-by: Peter Krempa 
> > ---
> >   docs/kbase/incrementalbackupinternals.rst | 15 +++
> >   1 file changed, 15 insertions(+)
> 
> Reviewed-by: Eric Blake 
> 
> > 
> > diff --git a/docs/kbase/incrementalbackupinternals.rst 
> > b/docs/kbase/incrementalbackupinternals.rst
> > index 0c4b4f7486..eada0d2935 100644
> > --- a/docs/kbase/incrementalbackupinternals.rst
> > +++ b/docs/kbase/incrementalbackupinternals.rst
> > @@ -94,6 +94,21 @@ so, even if we obviously can't guarantee that.
> >   Integration with external snapshots
> >   ===
> > 
> > +External snapshot terminology
> > +-
> > +
> > +External snapshots on a disk level consist of layered chains of disk 
> > images. An
> > +image in the chain can have a ``backing image`` placed below. Any chunk in 
> > the
> > +current image which was not written explicitly is transparent and if read 
> > the
> > +data from the backing image is passed through. An image placed on top of 
> > the
> > +current image is called ``overlay``.
> > +
> > +The bottommost backing image at the end of the chain is also usually 
> > described
> > +as ``base image``.
> > +
> > +The topmost overlay is the image which is being written to by the VM and 
> > is also
> > +described as the ``active`` layer or image.
> 
> Maybe it's also worth a paragraph mentioning how diagramming the chains we
> typically use <- for 'Backed by', as in:
> 
> Base <- Top

Good idea! I'll follow up with a patch to add the paragraph.

Re: [PATCH] NEWS: mention fix for crash in qemuDomainBlockCommit

[Andrea Bolognani]

On Thu, 2020-06-25 at 10:25 +0200, Peter Krempa wrote:
> There were two upstream issues filed for the problem so it's worth
> mentioning.
> 
> Signed-off-by: Peter Krempa 
> ---
>  NEWS.rst | 5 +
>  1 file changed, 5 insertions(+)

Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH] qemuDomainDeviceNetDefPostParse: Switch order of conditions

[Daniel P . Berrangé]

On Thu, Jun 25, 2020 at 09:48:56AM +0200, Michal Privoznik wrote:
> A few commits back (in v6.4.0-131-gbdb8f2e418) the post parse
> function for domain interface was changed so that it doesn't fill
> in model for hostdev types of interfaces (including network type
> interfaces which would end up hostdevs).
> 
> While the idea is sound, the execution can be a bit better:
> virDomainNetResolveActualType() which is used to determine
> runtime type of given interface is heavy gun - it connects to
> network driver, fetches network XML, parses it. This all is
> followed by check whether the interface doesn't already have
> model set (from domain XML).
> 
> If we switch the order of these two checks then the short circuit
> evaluation will ensure the expensive check is done only if really
> needed.
> 
> This commit in fact fixes qemuxml2xmltest which due to lacking
> fake network driver tries to connect to network:///session and
> start the virtnetworkd. Fortunately, because of
> v6.3.0-25-gf28fbb05d3 it fails to do so and
> virDomainNetResolveActualType() returns -1. The only reason we
> don't see the test failing is because our input XMLs have model
> and thus we are saved by the latter (now former) check.
> 
> Signed-off-by: Michal Privoznik 
> ---
> 
> While this patch is technically correct (the best way to be correct), it
> can also be viewed as papering over the real issue. Question is, how to
> address that. Nor xml2xml test nor xml2argv test are creating fake
> network driver. Is mocking virDomainNetResolveActualType() the way to go
> then? Or should we create the fake network driver with networks and
> everything?

The qemuxml2argtest calls virSetConnectNetwork() to provide a stub
network driver, likewise for other secondary drivers. This prevents
any risk of spawning daemons. We should probably do that for the
other tests too.

> 
>  src/qemu/qemu_domain.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 2efbf1a4b3..c5b8d91f9a 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -5230,8 +5230,8 @@ qemuDomainDeviceNetDefPostParse(virDomainNetDefPtr net,
>  virQEMUCapsPtr qemuCaps)
>  {
>  if (net->type != VIR_DOMAIN_NET_TYPE_HOSTDEV &&
> -virDomainNetResolveActualType(net) != VIR_DOMAIN_NET_TYPE_HOSTDEV &&
> -!virDomainNetGetModelString(net))
> +!virDomainNetGetModelString(net) &&
> +virDomainNetResolveActualType(net) != VIR_DOMAIN_NET_TYPE_HOSTDEV)
>  net->model = qemuDomainDefaultNetModel(def, qemuCaps);
>  
>  return 0;
> -- 
> 2.26.2
> 

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

[PATCH] NEWS: mention fix for crash in qemuDomainBlockCommit

[Peter Krempa]

There were two upstream issues filed for the problem so it's worth
mentioning.

Signed-off-by: Peter Krempa 
---
 NEWS.rst | 5 +
 1 file changed, 5 insertions(+)

diff --git a/NEWS.rst b/NEWS.rst
index 2fc43c31b9..54b1e4a992 100644
--- a/NEWS.rst
+++ b/NEWS.rst
@@ -51,6 +51,11 @@ v6.5.0 (unreleased)

 * **Bug fixes**

+  * qemu: fixed crash in ``qemuDomainBlockCommit``
+
+This release fixes a regression which was introduced in libvirt v6.4.0
+where libvirtd always crashes when a block commit of a disk is requested.
+

 v6.4.0 (2020-06-02)
 ===
-- 
2.26.2

Re: [PATCH 11/25] network: use g_free() in place of remaining VIR_FREE()

[Peter Krempa]

On Wed, Jun 24, 2020 at 23:34:00 -0400, Laine Stump wrote:
> Signed-off-by: Laine Stump 
> ---
>  src/network/bridge_driver.c | 59 +
>  1 file changed, 33 insertions(+), 26 deletions(-)
> 
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 668aa9ca88..a1b2f5b6c7 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c

[...]

> @@ -706,7 +706,8 @@ networkStateInitialize(bool privileged,
>  
>  network_driver->lockFD = -1;
>  if (virMutexInit(_driver->lock) < 0) {
> -VIR_FREE(network_driver);
> +g_free(network_driver);
> +network_driver = NULL;
>  goto error;

In general I'm agains senseless replacement of VIR_FREE for g_free.
There is IMO no value to do so. VIR_FREE is now implemented via
g_clear_pointer(, g_free) so g_free is actually used.

Mass replacements are also substrate for adding bugs and need to be
approached carefully, so doing this en-mass might lead to others
attempting the same with possibly less care.

In general, mass replacements should be done only to

g_clear_pointer(, g_free)

and I'm not sure it's worth it.

[PATCH] qemuDomainDeviceNetDefPostParse: Switch order of conditions

[Michal Privoznik]

A few commits back (in v6.4.0-131-gbdb8f2e418) the post parse
function for domain interface was changed so that it doesn't fill
in model for hostdev types of interfaces (including network type
interfaces which would end up hostdevs).

While the idea is sound, the execution can be a bit better:
virDomainNetResolveActualType() which is used to determine
runtime type of given interface is heavy gun - it connects to
network driver, fetches network XML, parses it. This all is
followed by check whether the interface doesn't already have
model set (from domain XML).

If we switch the order of these two checks then the short circuit
evaluation will ensure the expensive check is done only if really
needed.

This commit in fact fixes qemuxml2xmltest which due to lacking
fake network driver tries to connect to network:///session and
start the virtnetworkd. Fortunately, because of
v6.3.0-25-gf28fbb05d3 it fails to do so and
virDomainNetResolveActualType() returns -1. The only reason we
don't see the test failing is because our input XMLs have model
and thus we are saved by the latter (now former) check.

Signed-off-by: Michal Privoznik 
---

While this patch is technically correct (the best way to be correct), it
can also be viewed as papering over the real issue. Question is, how to
address that. Nor xml2xml test nor xml2argv test are creating fake
network driver. Is mocking virDomainNetResolveActualType() the way to go
then? Or should we create the fake network driver with networks and
everything?

 src/qemu/qemu_domain.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 2efbf1a4b3..c5b8d91f9a 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -5230,8 +5230,8 @@ qemuDomainDeviceNetDefPostParse(virDomainNetDefPtr net,
 virQEMUCapsPtr qemuCaps)
 {
 if (net->type != VIR_DOMAIN_NET_TYPE_HOSTDEV &&
-virDomainNetResolveActualType(net) != VIR_DOMAIN_NET_TYPE_HOSTDEV &&
-!virDomainNetGetModelString(net))
+!virDomainNetGetModelString(net) &&
+virDomainNetResolveActualType(net) != VIR_DOMAIN_NET_TYPE_HOSTDEV)
 net->model = qemuDomainDefaultNetModel(def, qemuCaps);
 
 return 0;
-- 
2.26.2