Re: [libvirt PATCH v3 0/2] Refuse PCI Address for ramfb device definition
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*'
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(&createdOnce, 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()
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
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*'
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(&createdOnce, 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*
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()
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
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
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()
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()
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()
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()
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
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
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
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()
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
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
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
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
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
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
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
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
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
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
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, &addr->uid, "uid"); > +if (addr->uid.isSet) > +virDomainZPCIAddressReleaseId(set, &addr->uid.value, "uid"); > } > > @@ -137,7 +138,8 @@ static void > virDomainZPCIAddressReleaseFid(virHashTablePtr set, > virZPCIDeviceAddressPtr addr) > { > -virDomainZPCIAddressReleaseId(set, &addr->fid, "fid"); > +if (addr->fid.isSet) > +virDomainZPCIAddressReleaseId(set, &addr->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, &id)) { +if (!id->isSet) { +virReportError(VIR_ERR_INTERNAL_ERROR, + _("No zPCI %s to reserve"), + name); +return -1; +} + +if (virHashLookup(set, &id->value)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("zPCI %s %o is already reserved"), - name, id); + name, id->value); return -1; } -if (virHashAddEntry(set, &id, (void*)1) < 0) { +if (virHashAddEntry(set, &id->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, &addr->uid, "uid"); } @@ -66,17 +73,20 @@ static int virDomainZPCIAddressReserveFid(virHashTablePtr set, virZPCIDeviceAddressPtr addr) { -return virDomainZPCIAddressReserveId(set, addr->fid.value, "fid"); +return virDomainZPCIAddressReserveId(set, &addr->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 (virH
Re: [PATCH libvirt v2 1/5] conf: use g_autofree to ensure automatic cleanup
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
On Thu, 2020-06-25 at 17:14 +0100, Daniel P. Berrangé wrote: > +.git_native_build_job_template: &git_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: &dist_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
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
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
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
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
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: &container_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: &script_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: &git_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: &dist_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: +-
[PATCH libvirt-dbus v3 1/3] src: fix double free of virDomain object
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)", &flags); @@ -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)", &flags); -- 2.26.2
[PATCH libvirt-dbus v3 0/3] Introduce GitLab CI and merge requests
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()
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(&network_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(&ptr, 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(&ptr, 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()
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(&network_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(&ptr, 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
Re: [PATCH] qemuxml2xmltest: Set dummy non-hypervisor drivers
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
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
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); 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
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
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
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
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
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
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
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 \ d
Re: [libvirt PATCH] qemu: ramfb video device doesn't support PCI address
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
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
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
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
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
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-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 equa
[libvirt-cim PATCH v2 1/2] gitlab: introduce CI jobs testing git master & distro libvirt
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: &container_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: &script_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: &git_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: &dist_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 a
[libvirt-cim PATCH v2 0/2] Introduce GitLab CI and merge requests
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
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
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
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
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'
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
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
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
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
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()
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(&network_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(&ptr, 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(&ptr, g_free) and I'm not sure it's worth it.
[PATCH] qemuDomainDeviceNetDefPostParse: Switch order of conditions
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