Re: recommendations for handling Hyper-V version number
On Wed, Sep 23, 2020 at 8:36 PM Matt Coleman wrote: By the way, due to your domain's DMARC policy, gmail puts your emails into spam. Therefore it's possible quite a lot of list subscribers aren't seeing them. ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of libvir-list-boun...@redhat.com designates 216.205.24.124 as permitted sender) smtp.mailfrom=libvir-list-boun...@redhat.com; dmarc=fail (p=REJECT sp=REJECT dis=QUARANTINE) header.from=datto.com -- Chris Murphy
Re: [PATCH] libxl: use b_info->{acpi,acpi} when available
On 9/23/20 1:54 AM, Marek Marczykowski-Górecki wrote:
On Tue, Sep 22, 2020 at 09:41:00PM -0600, Jim Fehlig wrote:
On 9/18/20 9:51 AM, Daniel P. Berrangé wrote:
On Fri, Sep 18, 2020 at 05:41:21PM +0200, Marek Marczykowski-Górecki wrote:
On Fri, Sep 18, 2020 at 05:12:52PM +0200, Michal Prívozník wrote:
On 9/18/20 1:31 PM, Daniel P. Berrangé wrote:
On Wed, Sep 16, 2020 at 11:09:31AM +0200, Michal Privoznik wrote:
On 9/10/20 6:18 AM, Marek Marczykowski-Górecki wrote:
b_info->u.hvm.{acpi,apic} are deprecated. But also, on recent libxl
version (4.14) the old one seems to be broken. While libxl part should
be fixed too, update the usage here and at some point drop support for
the old version.
b_info->acpi was added in Xen 4.8
b_info->apic was added in Xen 4.10
Xen 4.10 is the oldest version that still has security support (until
December 2020).
Signed-off-by: Marek Marczykowski-Górecki
---
src/libxl/libxl_conf.c | 13 +
tests/libxlxml2domconfigdata/basic-hvm.json | 4 ++--
tests/libxlxml2domconfigdata/cpu-shares-hvm.json| 4 ++--
.../libxlxml2domconfigdata/fullvirt-acpi-slic.json | 4 ++--
.../fullvirt-cpuid-legacy-nest.json | 4 ++--
tests/libxlxml2domconfigdata/fullvirt-cpuid.json| 4 ++--
.../max-eventchannels-hvm.json | 4 ++--
tests/libxlxml2domconfigdata/max-gntframes-hvm.json | 4 ++--
tests/libxlxml2domconfigdata/moredevs-hvm.json | 4 ++--
.../libxlxml2domconfigdata/variable-clock-hvm.json | 4 ++--
.../vnuma-hvm-legacy-nest.json | 4 ++--
tests/libxlxml2domconfigdata/vnuma-hvm.json | 4 ++--
12 files changed, 35 insertions(+), 22 deletions(-)
This looks good to me.
Reviewed-by: Michal Privoznik
I'll wait a bit with pushing it though in case Jim wants to chime in.
This broke the build on Ubuntu 1804 due to tests failing
TEST: libxlxml2domconfigtest
.. 10 FAIL
Oh, Ubuntu 18.04 has libxen-dev-4.9.2 and I'm not sure about FreeBSD, but
probably something old too. So we can't use xen 4.10 APIs even though it was
released 3 years ago.
Unfortunately, we will have to support Ubuntu 18.04 for quite some time
because 20.04 was released only a while ago and we have this two year
transition period:
https://libvirt.org/platforms.html
Marek, are you okay with me reverting the patch?
Technically, the driver code itself should work on both versions (there
is an #ifdef for that). It's only an issue with tests, where we don't have
#ifdef inside json files...
One idea would be to duplicate those affected json files and pick the
right one based on the libxenlight version, but it doesn't sound nice...
Any alternative ideas?
I think just duplicating the JSON files is the best solution because it
is simple, low overhead and keeps test coverage in both versions.
I'm catching up on libvirt mail and could have missed it, but I didn't see a
follow-up patch to fix the test.
Marek, do you have time for it? If not I'll get to it tomorrow.
Sadly I'm pretty busy this week, I may find some time tomorrow but can't
promise it.
I cooked up a slightly more evil hack to avoid duplicate test data files
https://www.redhat.com/archives/libvir-list/2020-September/msg01311.html
Regards,
Jim
[PATCH] tests: Adjust libxlxml2domconfigtest to work with Xen < 4.10
Commit f253dc90f5 introduced a test regression in environments with
Xen < 4.10. The logic in libxl_conf.c correctly maps ACPI and APIC
from virDomainObj to libxl_domain_conf based on
LIBXL_HAVE_BUILDINFO_APIC, but the tests did not account for the
different libxl_domain_conf JSON representations.
One approach to fixing the test regression is to duplicate JSON test
data files, having one set for Xen <= 4.9 and another for Xen 4.10
and greater. To avoid duplicate data files, this patch takes the
approach of modifying the libxl_domain_conf object based on
LIBXL_HAVE_BUILDINFO_APIC, before retrieving the JSON representation.
It allows using the same test data files for all supported versions
of Xen by adjusting the intermediate form of libxl_domain_conf object
as needed.
Signed-off-by: Jim Fehlig
---
tests/libxlxml2domconfigtest.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/tests/libxlxml2domconfigtest.c b/tests/libxlxml2domconfigtest.c
index c4d5db9a7b..d58be1211b 100644
--- a/tests/libxlxml2domconfigtest.c
+++ b/tests/libxlxml2domconfigtest.c
@@ -97,6 +97,20 @@ testCompareXMLToDomConfig(const char *xmlfile,
"Failed to create libxl_domain_config from JSON doc");
goto cleanup;
}
+
+/*
+ * In order to have common test files between Xen 4.9 and newer Xen
versions,
+ * tweak the expected libxl_domain_config object before getting a json
+ * representation.
+ */
+# ifndef LIBXL_HAVE_BUILDINFO_APIC
+if (expectconfig.c_info.type == LIBXL_DOMAIN_TYPE_HVM) {
+libxl_defbool_unset(_info.acpi);
+libxl_defbool_set(_info.u.hvm.apic, true);
+libxl_defbool_set(_info.u.hvm.acpi, true);
+}
+# endif
+
if (!(expectjson = libxl_domain_config_to_json(cfg->ctx, ))) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
"Failed to retrieve JSON doc for libxl_domain_config");
--
2.28.0
handling Hyper-V automatic startup values
Hello, I’m implementing some new functionality in the Hyper-V driver and could use some input on how I should handle automatic startup values. Microsoft’s Msvm_VirtualSystemSettingData class stores this setting in a property named AutomaticStartupAction: https://docs.microsoft.com/en-us/windows/win32/hyperv_v2/msvm-virtualsystemsettingdata This property has several possible values with different meanings: * 2 means “None.”, which represents that the VM will not automatically start at boot. This corresponds to libvirt’s domainGetAutostart outputting boolean false. * 3 means “Restart if previously active.” This will start the machine at boot if it was running when the host was shut down or unexpectedly powered off. It appears libvirt does not have a way to represent this. * 4 means “Always start.” This corresponds to libvirt’s domainGetAutostart outputting boolean true. * 5 through 32768 are reserved. I’m unsure how to handle the value 3, since libvirt treats this setting as a boolean... The domainGetAutostart function places the value in an int: * https://gitlab.com/libvirt/libvirt/-/blob/master/src/driver-hypervisor.h#L1498 * https://gitlab.com/libvirt/libvirt/-/blob/master/src/driver-hypervisor.h#L461-463 However, virDomainGetAutostart’s docblock says that it will be treated as a boolean: https://gitlab.com/libvirt/libvirt/-/blob/master/src/libvirt-domain.c#L6690-6729 Its usage in `virsh dominfo` confirms that: https://gitlab.com/libvirt/libvirt/-/blob/master/tools/virsh-domain-monitor.c#L1354-1355 I haven’t investigated how other languages' bindings treat the field. Currently, my code treats anything over 2 as autostart being enabled (although, perhaps I should ignore 5+). I feel like that pretty closely represents the VM’s configuration, since it will autostart in certain cases and it definitely isn’t configured to never autostart. For 3 (“Restart if previously active.”), it could be argued that libvirt should only say that autostart is enabled when the VM is running. This would more closely represent what the VM’s runtime behavior will be, since `virsh list —autostart` would show you which VMs would boot if the hypervisor rebooted at that point in time. However, it could be considered confusing because the VM’s configuration would appear to change depending on whether or not it was running. Ultimately, it seems like libvirt’s concept of autostart functionality needs to be extended. Along those lines, it was pointed out on IRC that libvirt lacks the ability to represent hypervisors’ host shutdown activities: > danpb: we should introduce a .. lifecycle action > in the XML to express what actions to take on host shutdown and one of those > actions can be “stop-and-restart-on-next-boot” Hyper-V has a separate setting, AutomaticShutdownAction, which governs the actions taken when the host stops: * 2 means “Turn off.” This abruptly powers off the VM. * 3 means “Save state.” This saves the running VM’s state to disk. * 4 means “Shutdown.” The performs a clean shutdown of the VM. * 5 through 32768 are reserved. It seems to me that the two settings complement each other but cover different functionality. I feel like 3 (“Restart if previously active.”) is more closely related to startup activities than the host stopping. Since it seems like more pervasive changes are needed, I’d prefer to commit something “good enough” for the Hyper-V driver and then refine it along with the rest of the drivers. Should I make domainGetAutostart only output true for Hyper-V VMs configured with AutomaticStartupAction=3 when they are running? Or, should I leave it as I currently have it (2 = autostart is disabled; 3+ = autostart is enabled)? Thanks for your time and input, and apologies for how long this got. Matt
[PATCH v1 1/2] qemu: driver: use model name "host" for mode "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 after the CPU definition has been converted from
XML. If these conditions are matched, then the CPU definition's
model name will be set to "host".
Signed-off-by: Collin Walling
---
src/qemu/qemu_driver.c | 41 +++--
1 file changed, 31 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 1cecef01f7..427d2419f3 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -12281,6 +12281,26 @@ qemuConnectCPUModelComparison(virQEMUCapsPtr qemuCaps,
}
+static int
+qemuConnectCheckCPUModel(virCPUDefPtr cpu)
+{
+if (!cpu->model) {
+/*
+ * On some architectures a model name is never present
+ * for the host-passthrough mode, so default it to "host"
+ */
+if (cpu->mode == VIR_CPU_MODE_HOST_PASSTHROUGH) {
+cpu->model = g_strdup("host");
+} else {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("cpu parameter is missing a model name"));
+return -1;
+}
+}
+return 0;
+}
+
+
static int
qemuConnectCompareHypervisorCPU(virConnectPtr conn,
const char *emulator,
@@ -12336,15 +12356,9 @@ qemuConnectCompareHypervisorCPU(virConnectPtr conn,
if (virCPUDefParseXMLString(xmlCPU, VIR_CPU_TYPE_AUTO, ) < 0)
goto cleanup;
-if (!cpu->model) {
-if (cpu->mode == VIR_CPU_MODE_HOST_PASSTHROUGH) {
-cpu->model = g_strdup("host");
-} else {
-virReportError(VIR_ERR_INVALID_ARG, "%s",
- _("cpu parameter is missing a model name"));
-goto cleanup;
-}
-}
+if (qemuConnectCheckCPUModel(cpu) < 0)
+goto cleanup;
+
ret = qemuConnectCPUModelComparison(qemuCaps, cfg->libDir,
cfg->user, cfg->group,
hvCPU, cpu, failIncompatible);
@@ -12470,10 +12484,17 @@ qemuConnectCPUModelBaseline(virQEMUCapsPtr qemuCaps,
if (VIR_ALLOC(baseline) < 0)
return NULL;
-if (virCPUDefCopyModel(baseline, cpus[0], false))
+if (qemuConnectCheckCPUModel(cpus[0]) < 0)
+return NULL;
+
+if (virCPUDefCopyModel(baseline, cpus[0], false) < 0)
return NULL;
for (i = 1; i < ncpus; i++) {
+
+if (qemuConnectCheckCPUModel(cpus[i]) < 0)
+return NULL;
+
if (qemuMonitorGetCPUModelBaseline(proc->mon, baseline,
cpus[i], ) < 0)
return NULL;
--
2.26.2
[PATCH v1 2/2] qemu: driver: perform CPU model expansion on single CPU during baseline
When executing the hypervisor-cpu-baseline command and if there is only
a single CPU definition present in the XML file, then libvirt will
print an unhelpful message:
"error: An error occurred, but the cause is unknown"
This is due to no CPU definition ever being "baselined", since the
API expects at least two CPU models.
Let's fix this by performing a CPU model expansion on the single CPU
definition and returning the result to the caller.
Signed-off-by: Collin Walling
---
src/qemu/qemu_driver.c | 17 +
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 427d2419f3..97a960a769 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -12472,6 +12472,7 @@ qemuConnectCPUModelBaseline(virQEMUCapsPtr qemuCaps,
g_autoptr(qemuProcessQMP) proc = NULL;
g_autoptr(virCPUDef) baseline = NULL;
qemuMonitorCPUModelInfoPtr result = NULL;
+qemuMonitorCPUModelExpansionType expansion_type;
size_t i;
if (!(proc = qemuProcessQMPNew(virQEMUCapsGetBinary(qemuCaps),
@@ -12503,15 +12504,15 @@ qemuConnectCPUModelBaseline(virQEMUCapsPtr qemuCaps,
return NULL;
}
-if (expand_features) {
-if (qemuMonitorGetCPUModelExpansion(proc->mon,
-
QEMU_MONITOR_CPU_MODEL_EXPANSION_FULL,
-baseline, true, false, ) <
0)
-return NULL;
+expansion_type = expand_features ? QEMU_MONITOR_CPU_MODEL_EXPANSION_FULL
+ : QEMU_MONITOR_CPU_MODEL_EXPANSION_STATIC;
-if (qemuConnectStealCPUModelFromInfo(baseline, ) < 0)
-return NULL;
-}
+if (qemuMonitorGetCPUModelExpansion(proc->mon, expansion_type,
+baseline, true, false, ) < 0)
+return NULL;
+
+if (qemuConnectStealCPUModelFromInfo(baseline, ) < 0)
+return NULL;
return g_steal_pointer();
}
--
2.26.2
Re: [libvirt PATCH v2] qemu: substitute missing model name for host-passthrough
On 9/23/20 5:01 PM, Jiri Denemark wrote:
> On Wed, Sep 23, 2020 at 16:17:04 -0400, Collin Walling wrote:
>> On 9/23/20 2:52 PM, Collin Walling wrote:
>>> On 9/23/20 10:18 AM, Jiri Denemark wrote:
On Wed, Sep 23, 2020 at 09:26:58 +0200, Tim Wiederhake wrote:
> From: Collin Walling
>
> Before:
> $ uname -m
> s390x
> $ cat passthrough-cpu.xml
>
> $ virsh hypervisor-cpu-compare passthrough-cpu.xml
> error: Failed to compare hypervisor CPU with passthrough-cpu.xml
> error: internal error: unable to execute QEMU command
> 'query-cpu-model-comp
> arison': Invalid parameter type for 'modelb.name', expected: string
>
> After:
> $ virsh hypervisor-cpu-compare passthrough-cpu.xml
> CPU described in passthrough-cpu.xml is identical to the CPU provided
> by hy
> pervisor on the host
>
> Signed-off-by: Tim Wiederhake
> ---
> src/qemu/qemu_driver.c | 9 +
> 1 file changed, 9 insertions(+)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index ae715c01d7..1cecef01f7 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -12336,6 +12336,15 @@ qemuConnectCompareHypervisorCPU(virConnectPtr
> conn,
> if (virCPUDefParseXMLString(xmlCPU, VIR_CPU_TYPE_AUTO, ) < 0)
> goto cleanup;
>
> +if (!cpu->model) {
> +if (cpu->mode == VIR_CPU_MODE_HOST_PASSTHROUGH) {
> +cpu->model = g_strdup("host");
> +} else {
> +virReportError(VIR_ERR_INVALID_ARG, "%s",
> + _("cpu parameter is missing a model
> name"));
> +goto cleanup;
> +}
> +}
> ret = qemuConnectCPUModelComparison(qemuCaps, cfg->libDir,
> cfg->user, cfg->group,
> hvCPU, cpu,
> failIncompatible);
Reviewed-by: Jiri Denemark
I'll wait some time for Collin to add Signed-of-by tag before pushing
this.
>>>
>>> Signed-off-by: Collin Walling
>>>
>>> Thanks!
>>>
>>
>> Actually, it might help to extend this functionality for baseline as
>> well. If anything to at least catch the case when a CPU definition in
>> the XML file is missing a tag. Right now, virsh will either
>> report "an unknown error occurred" when the XML file contains a _single_
>> element without a tag, or it will report the same "Invalid
>> parameter type for 'modela.name', expected: string" mentioned above when
>> there are multiple definitions in the file, and at least one of them is
>> missing a tag.
>
> Hmm, I would expect libvirt to complain about missing CPU model. If
> that's not the case, we need to fix it. But we should not fix it the
> same way. This change in the Compare API is hidden inside libvirt, we
> just tell QEMU we're comparing "host" when the cpu->mode is
> host-passthrough and get the result, which is basically yes/no.
>
> But with baseline we get the result and make a guest CPU definition out
> of it. By setting cpu->model to "host" we would could end up with
> host in the result or even random result depending on the
> host performing the baseline API. This API should just reject any CPU
> definition without as invalid argument.
>
> Jirka
>
On s390x, QMP will internally convert the model "host" to a proper CPU
model.
How about when baselining and if there is only a single CPU definition
and the model is "host" (either provided verbatim by the file, or
converted when the mode is host-passthrough), then perform a CPU model
expansion on the single CPU definition? No baselining would technically
be performed in this case.
I think this would support the behavior reported by the virsh man page:
"""
When FILE contains only a single CPU definition, the command will print
the same CPU with restrictions imposed by the capabilities of
the hypervisor. Specifically, running th virsh
hypervisor-cpu-baseline command with no additional options on the
result of virsh domcapabili‐ ties will transform the host CPU
model from domain capabilities XML to a form directly usable in
domain XML.
"""
Essentially 2 patches:
- set model name to "host" when mode is "host-passthrough" for baseline
- use CPU model expansion when baseline is executed with only a single CPU
I can propose the patches tonight and we can look them over whenever.
--
Regards,
Collin
Stay safe and stay healthy
Re: [libvirt PATCH v2] qemu: substitute missing model name for host-passthrough
On Wed, Sep 23, 2020 at 16:17:04 -0400, Collin Walling wrote:
> On 9/23/20 2:52 PM, Collin Walling wrote:
> > On 9/23/20 10:18 AM, Jiri Denemark wrote:
> >> On Wed, Sep 23, 2020 at 09:26:58 +0200, Tim Wiederhake wrote:
> >>> From: Collin Walling
> >>>
> >>> Before:
> >>> $ uname -m
> >>> s390x
> >>> $ cat passthrough-cpu.xml
> >>>
> >>> $ virsh hypervisor-cpu-compare passthrough-cpu.xml
> >>> error: Failed to compare hypervisor CPU with passthrough-cpu.xml
> >>> error: internal error: unable to execute QEMU command
> >>> 'query-cpu-model-comp
> >>> arison': Invalid parameter type for 'modelb.name', expected: string
> >>>
> >>> After:
> >>> $ virsh hypervisor-cpu-compare passthrough-cpu.xml
> >>> CPU described in passthrough-cpu.xml is identical to the CPU provided
> >>> by hy
> >>> pervisor on the host
> >>>
> >>> Signed-off-by: Tim Wiederhake
> >>> ---
> >>> src/qemu/qemu_driver.c | 9 +
> >>> 1 file changed, 9 insertions(+)
> >>>
> >>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> >>> index ae715c01d7..1cecef01f7 100644
> >>> --- a/src/qemu/qemu_driver.c
> >>> +++ b/src/qemu/qemu_driver.c
> >>> @@ -12336,6 +12336,15 @@ qemuConnectCompareHypervisorCPU(virConnectPtr
> >>> conn,
> >>> if (virCPUDefParseXMLString(xmlCPU, VIR_CPU_TYPE_AUTO, ) < 0)
> >>> goto cleanup;
> >>>
> >>> +if (!cpu->model) {
> >>> +if (cpu->mode == VIR_CPU_MODE_HOST_PASSTHROUGH) {
> >>> +cpu->model = g_strdup("host");
> >>> +} else {
> >>> +virReportError(VIR_ERR_INVALID_ARG, "%s",
> >>> + _("cpu parameter is missing a model
> >>> name"));
> >>> +goto cleanup;
> >>> +}
> >>> +}
> >>> ret = qemuConnectCPUModelComparison(qemuCaps, cfg->libDir,
> >>> cfg->user, cfg->group,
> >>> hvCPU, cpu,
> >>> failIncompatible);
> >>
> >> Reviewed-by: Jiri Denemark
> >>
> >> I'll wait some time for Collin to add Signed-of-by tag before pushing
> >> this.
> >>
> >
> > Signed-off-by: Collin Walling
> >
> > Thanks!
> >
>
> Actually, it might help to extend this functionality for baseline as
> well. If anything to at least catch the case when a CPU definition in
> the XML file is missing a tag. Right now, virsh will either
> report "an unknown error occurred" when the XML file contains a _single_
> element without a tag, or it will report the same "Invalid
> parameter type for 'modela.name', expected: string" mentioned above when
> there are multiple definitions in the file, and at least one of them is
> missing a tag.
Hmm, I would expect libvirt to complain about missing CPU model. If
that's not the case, we need to fix it. But we should not fix it the
same way. This change in the Compare API is hidden inside libvirt, we
just tell QEMU we're comparing "host" when the cpu->mode is
host-passthrough and get the result, which is basically yes/no.
But with baseline we get the result and make a guest CPU definition out
of it. By setting cpu->model to "host" we would could end up with
host in the result or even random result depending on the
host performing the baseline API. This API should just reject any CPU
definition without as invalid argument.
Jirka
Re: [PATCH v2 0/6] qemu: Use memory-backend-* for regular guest memory
On 9/17/20 6:35 AM, Michal Privoznik wrote: v2 of: https://www.redhat.com/archives/libvir-list/2020-September/msg00023.html diff to v1: - I've dropped the capability (patch 5/7 in the original set) and am relying on qemu providing default-ram-id. I've tested migration from a daemon with these patches to a daemon without and also vice versa and it worked. Reviewed-by: Daniel Henrique Barboza FYI, patch 01 doesn't apply in master after 63af8fdeb2f6. It's a trivial conflict to solve though (just switch places between mem-path and prealloc). Thanks, DHB Michal Prívozník (6): qemuBuildMemoryBackendProps: Move @prealloc setting to backend agnostic part qemuBuildMemoryBackendProps: Respect //memoryBacking/allocation/@mode=immediate qemuBuildMemoryBackendProps: Prealloc mem for memfd backend qemuBuildMemoryBackendProps: Fix const correctness qemu: Track default-ram-id machine attribute qemu: Use memory-backend-* for regular guest memory src/qemu/qemu_capabilities.c | 35 +++- src/qemu/qemu_capabilities.h | 3 + src/qemu/qemu_capspriv.h | 3 +- src/qemu/qemu_command.c | 96 +++-- src/qemu/qemu_command.h | 4 +- src/qemu/qemu_monitor.c | 1 + src/qemu/qemu_monitor.h | 1 + src/qemu/qemu_monitor_json.c | 11 + .../caps_5.2.0.x86_64.xml | 196 +- .../blkdeviotune-group-num.x86_64-latest.args | 3 +- ...blkdeviotune-max-length.x86_64-latest.args | 3 +- .../blkdeviotune-max.x86_64-latest.args | 3 +- .../channel-unix-guestfwd.x86_64-latest.args | 3 +- .../console-virtio-unix.x86_64-latest.args| 3 +- .../controller-virtio-scsi.x86_64-latest.args | 3 +- ...-Icelake-Server-pconfig.x86_64-latest.args | 3 +- .../cpu-translation.x86_64-latest.args| 3 +- .../cputune-cpuset-big-id.x86_64-latest.args | 3 +- .../disk-aio-io_uring.x86_64-latest.args | 3 +- .../disk-aio.x86_64-latest.args | 3 +- ...-backing-chains-noindex.x86_64-latest.args | 3 +- .../disk-cache.x86_64-latest.args | 4 +- .../disk-cdrom-bus-other.x86_64-latest.args | 3 +- ...m-empty-network-invalid.x86_64-latest.args | 3 +- .../disk-cdrom-network.x86_64-latest.args | 3 +- .../disk-cdrom-tray.x86_64-latest.args| 3 +- .../disk-cdrom.x86_64-latest.args | 3 +- .../disk-copy_on_read.x86_64-latest.args | 3 +- .../disk-detect-zeroes.x86_64-latest.args | 3 +- .../disk-discard.x86_64-latest.args | 3 +- .../disk-error-policy.x86_64-latest.args | 3 +- .../disk-floppy-q35-2_11.x86_64-latest.args | 4 +- .../disk-floppy-q35-2_9.x86_64-latest.args| 4 +- .../disk-floppy.x86_64-latest.args| 3 +- .../disk-network-gluster.x86_64-latest.args | 3 +- .../disk-network-http.x86_64-latest.args | 3 +- .../disk-network-iscsi.x86_64-latest.args | 3 +- .../disk-network-nbd.x86_64-latest.args | 3 +- .../disk-network-rbd.x86_64-latest.args | 3 +- .../disk-network-sheepdog.x86_64-latest.args | 3 +- ...isk-network-source-auth.x86_64-latest.args | 3 +- ...isk-network-tlsx509-nbd.x86_64-latest.args | 3 +- .../disk-nvme.x86_64-latest.args | 3 +- .../disk-readonly-disk.x86_64-latest.args | 3 +- .../disk-scsi-device-auto.x86_64-latest.args | 3 +- .../disk-scsi.x86_64-latest.args | 3 +- .../disk-shared.x86_64-latest.args| 3 +- .../disk-slices.x86_64-latest.args| 3 +- ...irtio-scsi-reservations.x86_64-latest.args | 3 +- .../eoi-disabled.x86_64-latest.args | 3 +- .../eoi-enabled.x86_64-latest.args| 3 +- .../floppy-drive-fat.x86_64-latest.args | 3 +- .../qemuxml2argvdata/fs9p.x86_64-latest.args | 3 +- .../genid-auto.x86_64-latest.args | 3 +- .../qemuxml2argvdata/genid.x86_64-latest.args | 3 +- ...egl-headless-rendernode.x86_64-latest.args | 3 +- .../graphics-egl-headless.x86_64-latest.args | 3 +- ...pice-gl-auto-rendernode.x86_64-latest.args | 3 +- ...graphics-vnc-tls-secret.x86_64-latest.args | 3 +- .../graphics-vnc-tls.x86_64-latest.args | 3 +- ...tdev-mdev-display-ramfb.x86_64-latest.args | 3 +- ...play-spice-egl-headless.x86_64-latest.args | 3 +- ...ev-display-spice-opengl.x86_64-latest.args | 3 +- ...isplay-vnc-egl-headless.x86_64-latest.args | 3 +- ...ostdev-mdev-display-vnc.x86_64-latest.args | 3 +- .../hostdev-scsi-lsi.x86_64-latest.args | 3 +- ...ostdev-scsi-virtio-scsi.x86_64-latest.args | 3 +- .../qemuxml2argvdata/hugepages-memaccess.args | 30 +-- .../hugepages-memaccess2.args | 12 +- .../hugepages-numa-nodeset-part.args
Re: [libvirt PATCH v2] qemu: substitute missing model name for host-passthrough
On 9/23/20 2:52 PM, Collin Walling wrote:
> On 9/23/20 10:18 AM, Jiri Denemark wrote:
>> On Wed, Sep 23, 2020 at 09:26:58 +0200, Tim Wiederhake wrote:
>>> From: Collin Walling
>>>
>>> Before:
>>> $ uname -m
>>> s390x
>>> $ cat passthrough-cpu.xml
>>>
>>> $ virsh hypervisor-cpu-compare passthrough-cpu.xml
>>> error: Failed to compare hypervisor CPU with passthrough-cpu.xml
>>> error: internal error: unable to execute QEMU command
>>> 'query-cpu-model-comp
>>> arison': Invalid parameter type for 'modelb.name', expected: string
>>>
>>> After:
>>> $ virsh hypervisor-cpu-compare passthrough-cpu.xml
>>> CPU described in passthrough-cpu.xml is identical to the CPU provided by
>>> hy
>>> pervisor on the host
>>>
>>> Signed-off-by: Tim Wiederhake
>>> ---
>>> src/qemu/qemu_driver.c | 9 +
>>> 1 file changed, 9 insertions(+)
>>>
>>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>>> index ae715c01d7..1cecef01f7 100644
>>> --- a/src/qemu/qemu_driver.c
>>> +++ b/src/qemu/qemu_driver.c
>>> @@ -12336,6 +12336,15 @@ qemuConnectCompareHypervisorCPU(virConnectPtr conn,
>>> if (virCPUDefParseXMLString(xmlCPU, VIR_CPU_TYPE_AUTO, ) < 0)
>>> goto cleanup;
>>>
>>> +if (!cpu->model) {
>>> +if (cpu->mode == VIR_CPU_MODE_HOST_PASSTHROUGH) {
>>> +cpu->model = g_strdup("host");
>>> +} else {
>>> +virReportError(VIR_ERR_INVALID_ARG, "%s",
>>> + _("cpu parameter is missing a model name"));
>>> +goto cleanup;
>>> +}
>>> +}
>>> ret = qemuConnectCPUModelComparison(qemuCaps, cfg->libDir,
>>> cfg->user, cfg->group,
>>> hvCPU, cpu, failIncompatible);
>>
>> Reviewed-by: Jiri Denemark
>>
>> I'll wait some time for Collin to add Signed-of-by tag before pushing
>> this.
>>
>
> Signed-off-by: Collin Walling
>
> Thanks!
>
Actually, it might help to extend this functionality for baseline as
well. If anything to at least catch the case when a CPU definition in
the XML file is missing a tag. Right now, virsh will either
report "an unknown error occurred" when the XML file contains a _single_
element without a tag, or it will report the same "Invalid
parameter type for 'modela.name', expected: string" mentioned above when
there are multiple definitions in the file, and at least one of them is
missing a tag.
I can propose something to wrap the above in a helper function and
extend its use for baseline as well.
--
Regards,
Collin
Stay safe and stay healthy
[libvirt PATCH 5/5] tools: use g_new0 instead of VIR_ALLOC*
With the exception of vsh*alloc.
Signed-off-by: Ján Tomko
---
tools/virsh-domain-monitor.c| 11 +++
tools/virsh-domain.c| 26 --
tools/virt-admin-completer.c| 12 +---
tools/virt-login-shell-helper.c | 13 -
tools/vsh-table.c | 21 +++--
tools/vsh.c | 5 +
6 files changed, 24 insertions(+), 64 deletions(-)
diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
index 848efc8aa3..c8a7c0f1b7 100644
--- a/tools/virsh-domain-monitor.c
+++ b/tools/virsh-domain-monitor.c
@@ -1234,8 +1234,7 @@ cmdDomBlkError(vshControl *ctl, const vshCmd *cmd)
ndisks = count;
if (ndisks) {
-if (VIR_ALLOC_N(disks, ndisks) < 0)
-goto cleanup;
+disks = g_new0(virDomainDiskError, ndisks);
if ((count = virDomainGetDiskErrors(dom, disks, ndisks, 0)) == -1)
goto cleanup;
@@ -1378,10 +1377,7 @@ cmdDominfo(vshControl *ctl, const vshCmd *cmd)
vshPrint(ctl, "%-15s %s\n", _("Security DOI:"), secmodel.doi);
/* Security labels are only valid for active domains */
-if (VIR_ALLOC(seclabel) < 0) {
-virshDomainFree(dom);
-return false;
-}
+seclabel = g_new0(virSecurityLabel, 1);
if (virDomainGetSecurityLabel(dom, seclabel) == -1) {
virshDomainFree(dom);
@@ -2280,8 +2276,7 @@ cmdDomstats(vshControl *ctl, const vshCmd *cmd)
flags |= VIR_CONNECT_GET_ALL_DOMAINS_STATS_NOWAIT;
if (vshCommandOptBool(cmd, "domain")) {
-if (VIR_ALLOC_N(domlist, 1) < 0)
-goto cleanup;
+domlist = g_new0(virDomainPtr, 1);
ndoms = 1;
while ((opt = vshCommandOptArgv(ctl, cmd, opt))) {
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 32edfc0398..dfcba04682 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -1762,8 +1762,7 @@ virshBlockJobWaitInit(vshControl *ctl,
virshBlockJobWaitDataPtr ret;
virshControlPtr priv = ctl->privData;
-if (VIR_ALLOC(ret) < 0)
-return NULL;
+ret = g_new0(virshBlockJobWaitData, 1);
ret->ctl = ctl;
ret->dom = dom;
@@ -8177,8 +8176,7 @@ cmdCPUStats(vshControl *ctl, const vshCmd *cmd)
goto cleanup;
}
-if (VIR_ALLOC_N(params, nparams * MIN(show_count, 128)) < 0)
-goto cleanup;
+params = g_new0(virTypedParameter, nparams * MIN(show_count, 128));
while (show_count) {
int ncpus = MIN(show_count, 128);
@@ -8215,8 +8213,7 @@ cmdCPUStats(vshControl *ctl, const vshCmd *cmd)
goto cleanup;
}
-if (VIR_ALLOC_N(params, nparams) < 0)
-goto cleanup;
+params = g_new0(virTypedParameter, nparams);
/* passing start_cpu == -1 gives us domain's total status */
if ((stats_per_cpu = virDomainGetCPUStats(dom, params, nparams,
@@ -10086,14 +10083,9 @@ cmdLxcEnterNamespace(vshControl *ctl, const vshCmd
*cmd)
goto cleanup;
if (setlabel) {
-if (VIR_ALLOC(secmodel) < 0) {
-vshError(ctl, "%s", _("Failed to allocate security model"));
-goto cleanup;
-}
-if (VIR_ALLOC(seclabel) < 0) {
-vshError(ctl, "%s", _("Failed to allocate security label"));
-goto cleanup;
-}
+secmodel = g_new0(virSecurityModel, 1);
+seclabel = g_new0(virSecurityLabel, 1);
+
if (virNodeGetSecurityModel(priv->conn, secmodel) < 0)
goto cleanup;
if (virDomainGetSecurityLabel(dom, seclabel) < 0)
@@ -13737,8 +13729,7 @@ cmdEvent(vshControl *ctl, const vshCmd *cmd)
}
if (all) {
-if (VIR_ALLOC_N(data, VIR_DOMAIN_EVENT_ID_LAST) < 0)
-goto cleanup;
+data = g_new0(virshDomEventData, VIR_DOMAIN_EVENT_ID_LAST);
for (i = 0; i < VIR_DOMAIN_EVENT_ID_LAST; i++) {
data[i].ctl = ctl;
data[i].loop = loop;
@@ -13748,8 +13739,7 @@ cmdEvent(vshControl *ctl, const vshCmd *cmd)
data[i].id = -1;
}
} else {
-if (VIR_ALLOC_N(data, 1) < 0)
-goto cleanup;
+data = g_new0(virshDomEventData, 1);
data[0].ctl = ctl;
data[0].loop = vshCommandOptBool(cmd, "loop");
data[0].count =
diff --git a/tools/virt-admin-completer.c b/tools/virt-admin-completer.c
index 7201bb71eb..61004beeb5 100644
--- a/tools/virt-admin-completer.c
+++ b/tools/virt-admin-completer.c
@@ -46,8 +46,7 @@ vshAdmServerCompleter(vshControl *ctl,
if ((nsrvs = virAdmConnectListServers(priv->conn, , 0)) < 0)
return NULL;
-if (VIR_ALLOC_N(ret, nsrvs + 1) < 0)
-goto error;
+ret = g_new0(char *, nsrvs + 1);
for (i = 0; i < nsrvs; i++) {
const char *name = virAdmServerGetName(srvs[i]);
@@ -59,13 +58,4 @@ vshAdmServerCompleter(vshControl *ctl,
VIR_FREE(srvs);
return ret;
-
- error:
-
[libvirt PATCH 3/5] secret: use g_new0 instead of VIR_ALLOC*
Signed-off-by: Ján Tomko
---
src/secret/secret_driver.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c
index 1cb342878f..45da09322b 100644
--- a/src/secret/secret_driver.c
+++ b/src/secret/secret_driver.c
@@ -459,8 +459,7 @@ secretStateInitialize(bool privileged,
virStateInhibitCallback callback G_GNUC_UNUSED,
void *opaque G_GNUC_UNUSED)
{
-if (VIR_ALLOC(driver) < 0)
-return VIR_DRV_STATE_INIT_ERROR;
+driver = g_new0(virSecretDriverState, 1);
driver->lockFD = -1;
if (virMutexInit(>lock) < 0) {
--
2.26.2
[libvirt PATCH 1/5] node_device: use g_new0 instead of VIR_ALLOC*
Signed-off-by: Ján Tomko
---
src/node_device/node_device_udev.c | 27 +--
1 file changed, 9 insertions(+), 18 deletions(-)
diff --git a/src/node_device/node_device_udev.c
b/src/node_device/node_device_udev.c
index 12e3f30bad..2d0ca27fc6 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -413,13 +413,11 @@ udevProcessPCI(struct udev_device *device,
goto cleanup;
if (virPCIDeviceIsPCIExpress(pciDev) > 0) {
-if (VIR_ALLOC(pci_express) < 0)
-goto cleanup;
+pci_express = g_new0(virPCIEDeviceInfo, 1);
if (virPCIDeviceHasPCIExpressLink(pciDev) > 0) {
-if (VIR_ALLOC(pci_express->link_cap) < 0 ||
-VIR_ALLOC(pci_express->link_sta) < 0)
-goto cleanup;
+pci_express->link_cap = g_new0(virPCIELink, 1);
+pci_express->link_sta = g_new0(virPCIELink, 1);
if (virPCIDeviceGetLinkCapSta(pciDev,
_express->link_cap->port,
@@ -1159,8 +1157,7 @@ udevGetDeviceNodes(struct udev_device *device,
udev_list_entry_foreach(list_entry,
udev_device_get_devlinks_list_entry(device))
n++;
-if (VIR_ALLOC_N(def->devlinks, n + 1) < 0)
-return -1;
+def->devlinks = g_new0(char *, n + 1);
n = 0;
udev_list_entry_foreach(list_entry,
udev_device_get_devlinks_list_entry(device)) {
@@ -1371,16 +1368,14 @@ udevAddOneDevice(struct udev_device *device)
bool new_device = true;
int ret = -1;
-if (VIR_ALLOC(def) != 0)
-goto cleanup;
+def = g_new0(virNodeDeviceDef, 1);
def->sysfs_path = g_strdup(udev_device_get_syspath(device));
if (udevGetStringProperty(device, "DRIVER", >driver) < 0)
goto cleanup;
-if (VIR_ALLOC(def->caps) != 0)
-goto cleanup;
+def->caps = g_new0(virNodeDevCapsDef, 1);
if (udevGetDeviceType(device, >caps->data.type) != 0)
goto cleanup;
@@ -1788,13 +1783,10 @@ udevSetupSystemDev(void)
virNodeDeviceObjPtr obj = NULL;
int ret = -1;
-if (VIR_ALLOC(def) < 0)
-return -1;
+def = g_new0(virNodeDeviceDef, 1);
def->name = g_strdup("computer");
-
-if (VIR_ALLOC(def->caps) != 0)
-goto cleanup;
+def->caps = g_new0(virNodeDevCapsDef, 1);
#if defined(__x86_64__) || defined(__i386__) || defined(__amd64__)
udevGetDMIData(>caps->data.system);
@@ -1882,8 +1874,7 @@ nodeStateInitialize(bool privileged,
return -1;
}
-if (VIR_ALLOC(driver) < 0)
-return VIR_DRV_STATE_INIT_ERROR;
+driver = g_new0(virNodeDeviceDriverState, 1);
driver->lockFD = -1;
if (virMutexInit(>lock) < 0) {
--
2.26.2
[libvirt PATCH 2/5] openvz: use g_new0 instead of VIR_ALLOC*
Signed-off-by: Ján Tomko
---
src/openvz/openvz_conf.c | 12
src/openvz/openvz_driver.c | 3 +--
2 files changed, 5 insertions(+), 10 deletions(-)
diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c
index 6d54123a35..8f1740863c 100644
--- a/src/openvz/openvz_conf.c
+++ b/src/openvz/openvz_conf.c
@@ -205,8 +205,7 @@ openvzReadNetworkConf(virDomainDefPtr def,
} else if (ret > 0) {
token = strtok_r(temp, " ", );
while (token != NULL) {
-if (VIR_ALLOC(net) < 0)
-goto error;
+net = g_new0(virDomainNetDef, 1);
net->type = VIR_DOMAIN_NET_TYPE_ETHERNET;
if (virDomainNetAppendIPAddress(net, token, AF_UNSPEC, 0) < 0)
@@ -238,8 +237,7 @@ openvzReadNetworkConf(virDomainDefPtr def,
int len;
/* add new device to list */
-if (VIR_ALLOC(net) < 0)
-goto error;
+net = g_new0(virDomainNetDef, 1);
net->type = VIR_DOMAIN_NET_TYPE_BRIDGE;
@@ -259,8 +257,7 @@ openvzReadNetworkConf(virDomainDefPtr def,
goto error;
}
-if (VIR_ALLOC_N(net->ifname, len+1) < 0)
-goto error;
+net->ifname = g_new0(char, len+1);
if (virStrncpy(net->ifname, p, len, len+1) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -276,8 +273,7 @@ openvzReadNetworkConf(virDomainDefPtr def,
goto error;
}
-if (VIR_ALLOC_N(net->data.bridge.brname, len+1) < 0)
-goto error;
+net->data.bridge.brname = g_new0(char, len+1);
if (virStrncpy(net->data.bridge.brname, p, len, len+1) <
0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c
index 71e270ea09..f90ae4fe69 100644
--- a/src/openvz/openvz_driver.c
+++ b/src/openvz/openvz_driver.c
@@ -1287,8 +1287,7 @@ static virDrvOpenStatus openvzConnectOpen(virConnectPtr
conn,
/* We now know the URI is definitely for this driver, so beyond
* here, don't return DECLINED, always use ERROR */
-if (VIR_ALLOC(driver) < 0)
-return VIR_DRV_OPEN_ERROR;
+driver = g_new0(struct openvz_driver, 1);
if (!(driver->domains = virDomainObjListNew()))
goto cleanup;
--
2.26.2
Re: [PATCH v1] qemu: monitor: substitute missing model name for host-passthrough
On 9/23/20 9:50 AM, Jiri Denemark wrote: > Collin, I apologize for not getting to you earlier. > > On Wed, Sep 16, 2020 at 12:11:08 -0400, Collin Walling wrote: >> On 9/16/20 3:03 AM, Michal Privoznik wrote: >>> On 9/15/20 10:25 PM, Collin Walling wrote: One more ping in attempt to get this in the right direction. Otherwise I'll post my next idea and we can go from there :) >>> >>> I agree with Peter that while the idea might look correct it's too deep. >>> Thinking about this issue, should a host-passthough CPU definition be permitted for the baseline & comparison commands? The model represented under this mode is not considered migration safe and it may make sense to simply fail early since these commands aim to construct/determine a migratable CPU model, respectively. >>> >>> Honestly, I don't know much about this CPU models area, but is that true >>> even for two identical hosts? Say I have two desktops next to each >>> other, with the same CPU and I want to migrate. I could use host model, >>> couldn't I? >>> >> >> "Host-model" is an alias for a CPU model that closely represents the >> capabilities of the host machine (on s390, because this model is defined >> by the hypervisor, it can also be called the "hypervisor CPU model" -- >> not an important detail). >> >> However, a guest running with the host-passthrough mode is not >> considered migration safe as that guest may covertly run with >> features/capabilities that are not directly exposed to the hypervisor. > > Right, but migration may still be possible and working fine if both host > are identical. > >> From what I understand regarding the hypervisor-cpu-compare and >> hypervisor-cpu-baseline commands is that they aim to assist with >> determining the migratability of guests based on their CPU model and >> feature set (usually along with a host CPU in the equation as well). > > Baseline with a host-passthrough CPU is not indeed very useful, but Agreed, but I think baseline would still benefit from the error catching that is proposed in the CPU comparison patch (I continue the conversation over on that thread). > compare could still be used and its usage is not limited to migration. > For example, you can use it to check whether a domain with a guest CPU > configuration can be started on a specific host before you actually try > to start it. And reporting host-passthrough as incompatible would be > wrong. > > Anyway, thanks for your patch, it was mostly correct, it just needed to > be done a bit higher in the call graph. Incidentally, Tim Wiederhake [1] > took this original patch and moved the change to the right place. The > authorship is still yours, so if you want to append you signed-off-by > tag there, I'll wait a bit before pushing Tim's patch. Thanks. Gave my sign-off. > > Jirka > > [1] https://www.redhat.com/archives/libvir-list/2020-September/msg01177.html > -- Regards, Collin Stay safe and stay healthy
[libvirt PATCH 4/5] security: use g_new0 instead of VIR_ALLOC*
Signed-off-by: Ján Tomko
---
src/security/security_apparmor.c | 3 +--
src/security/security_dac.c | 9 +++--
src/security/security_manager.c | 14 +-
src/security/security_selinux.c | 9 +++--
src/security/security_stack.c| 6 ++
5 files changed, 14 insertions(+), 27 deletions(-)
diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
index eea37dca83..c2d86c6940 100644
--- a/src/security/security_apparmor.c
+++ b/src/security/security_apparmor.c
@@ -867,8 +867,7 @@ AppArmorSetSecurityHostdevLabel(virSecurityManagerPtr mgr,
if (profile_loaded(secdef->imagelabel) < 0)
return 0;
-if (VIR_ALLOC(ptr) < 0)
-return -1;
+ptr = g_new0(struct SDPDOP, 1);
ptr->mgr = mgr;
ptr->def = def;
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index d9d4cda159..258d246659 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -108,8 +108,7 @@ virSecurityDACChownListAppend(virSecurityDACChownListPtr
list,
char *tmp = NULL;
virSecurityDACChownItemPtr item = NULL;
-if (VIR_ALLOC(item) < 0)
-return -1;
+item = g_new0(virSecurityDACChownItem, 1);
tmp = g_strdup(path);
@@ -227,8 +226,7 @@ virSecurityDACTransactionRun(pid_t pid G_GNUC_UNUSED,
int ret = -1;
if (list->lock) {
-if (VIR_ALLOC_N(paths, list->nItems) < 0)
-return -1;
+paths = g_new0(const char *, list->nItems);
for (i = 0; i < list->nItems; i++) {
virSecurityDACChownItemPtr item = list->items[i];
@@ -580,8 +578,7 @@ virSecurityDACTransactionStart(virSecurityManagerPtr mgr)
return -1;
}
-if (VIR_ALLOC(list) < 0)
-return -1;
+list = g_new0(virSecurityDACChownList, 1);
list->manager = virObjectRef(mgr);
diff --git a/src/security/security_manager.c b/src/security/security_manager.c
index 17b565cc12..be81ee5e44 100644
--- a/src/security/security_manager.c
+++ b/src/security/security_manager.c
@@ -87,8 +87,7 @@ virSecurityManagerNewDriver(virSecurityDriverPtr drv,
virCheckFlags(VIR_SECURITY_MANAGER_NEW_MASK, NULL);
-if (VIR_ALLOC_N(privateData, drv->privateDataLen) < 0)
-return NULL;
+privateData = g_new0(char, drv->privateDataLen);
if (!(mgr = virObjectLockableNew(virSecurityManagerClass)))
goto error;
@@ -1034,8 +1033,7 @@ virSecurityManagerGetNested(virSecurityManagerPtr mgr)
if (STREQ("stack", mgr->drv->name))
return virSecurityStackGetNested(mgr);
-if (VIR_ALLOC_N(list, 2) < 0)
-return NULL;
+list = g_new0(virSecurityManagerPtr, 2);
list[0] = mgr;
list[1] = NULL;
@@ -1346,9 +1344,8 @@ virSecurityManagerMetadataLock(virSecurityManagerPtr mgr
G_GNUC_UNUSED,
const char **locked_paths = NULL;
virSecurityManagerMetadataLockStatePtr ret = NULL;
-if (VIR_ALLOC_N(fds, npaths) < 0 ||
-VIR_ALLOC_N(locked_paths, npaths) < 0)
-return NULL;
+fds = g_new0(int, npaths);
+locked_paths = g_new0(const char *, npaths);
/* Sort paths to lock in order to avoid deadlocks with other
* processes. For instance, if one process wants to lock
@@ -1441,8 +1438,7 @@ virSecurityManagerMetadataLock(virSecurityManagerPtr mgr
G_GNUC_UNUSED,
VIR_APPEND_ELEMENT_COPY_INPLACE(fds, nfds, fd);
}
-if (VIR_ALLOC(ret) < 0)
-goto cleanup;
+ret = g_new0(virSecurityManagerMetadataLockState, 1);
ret->paths = g_steal_pointer(_paths);
ret->fds = g_steal_pointer();
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index 87741d6dad..e40d670e97 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -123,8 +123,7 @@
virSecuritySELinuxContextListAppend(virSecuritySELinuxContextListPtr list,
int ret = -1;
virSecuritySELinuxContextItemPtr item = NULL;
-if (VIR_ALLOC(item) < 0)
-return -1;
+item = g_new0(virSecuritySELinuxContextItem, 1);
item->path = g_strdup(path);
item->tcon = g_strdup(tcon);
@@ -258,8 +257,7 @@ virSecuritySELinuxTransactionRun(pid_t pid G_GNUC_UNUSED,
int ret = -1;
if (list->lock) {
-if (VIR_ALLOC_N(paths, list->nItems) < 0)
-return -1;
+paths = g_new0(const char *, list->nItems);
for (i = 0; i < list->nItems; i++) {
virSecuritySELinuxContextItemPtr item = list->items[i];
@@ -1088,8 +1086,7 @@ virSecuritySELinuxTransactionStart(virSecurityManagerPtr
mgr)
return -1;
}
-if (VIR_ALLOC(list) < 0)
-return -1;
+list = g_new0(virSecuritySELinuxContextList, 1);
list->manager = virObjectRef(mgr);
diff --git a/src/security/security_stack.c b/src/security/security_stack.c
index 2480c47f70..3bfcb1e2f7 100644
--- a/src/security/security_stack.c
+++ b/src/security/security_stack.c
@@ -56,8 +56,7 @@
[libvirt PATCH 0/5] Use g_new0 more (glib chronicles)
Remove VIR_ALLOC* from tools/ (with the exception of vsh*alloc, which will be cleaned up later) and some random subdirs from src/ Ján Tomko (5): node_device: use g_new0 instead of VIR_ALLOC* openvz: use g_new0 instead of VIR_ALLOC* secret: use g_new0 instead of VIR_ALLOC* security: use g_new0 instead of VIR_ALLOC* tools: use g_new0 instead of VIR_ALLOC* src/node_device/node_device_udev.c | 27 +-- src/openvz/openvz_conf.c | 12 src/openvz/openvz_driver.c | 3 +-- src/secret/secret_driver.c | 3 +-- src/security/security_apparmor.c | 3 +-- src/security/security_dac.c| 9 +++-- src/security/security_manager.c| 14 +- src/security/security_selinux.c| 9 +++-- src/security/security_stack.c | 6 ++ tools/virsh-domain-monitor.c | 11 +++ tools/virsh-domain.c | 26 -- tools/virt-admin-completer.c | 12 +--- tools/virt-login-shell-helper.c| 13 - tools/vsh-table.c | 21 +++-- tools/vsh.c| 5 + 15 files changed, 53 insertions(+), 121 deletions(-) -- 2.26.2
Re: [PATCH] RFC: fix error message in virMigrate3 if connection is broken
On Mon, Sep 21, 2020 at 10:51:16 +0300, Nikolay Shirokovskiy wrote:
> Currently virDomainMigrate3 reports "this function is not supported by the
> connection driver: virDomainMigrate3" if connection to destination for example
> is broken. This is a bit misleading.
>
> This is a result of we treat errors as unsupported feature in
> VIR_DRV_SUPPORTS_FEATURE macro. So let's handle errors instead. In order to
> keep logic clean as before there are new helper functions
> virDrvSupportsFeature/virDrvNSupportsFeature. They allow to keep if/else if
> structure of feature tests.
>
> I guess all the other occurences of VIR_DRV_SUPPORTS_FEATURE need to be
> replaced with one these new helper functions so that we detect error early and
> not have issues similar to virDomainMigrate3. I'm going to fix the other
> places if this RFC is approved.
To be honest, I think this is quite ugly.
>
> ---
> src/datatypes.c | 70 +++
> src/datatypes.h | 7 +
> src/libvirt-domain.c | 76
> +++-
> 3 files changed, 123 insertions(+), 30 deletions(-)
>
> diff --git a/src/datatypes.c b/src/datatypes.c
> index 1db38c5..3fb71f4 100644
> --- a/src/datatypes.c
> +++ b/src/datatypes.c
> @@ -1257,3 +1257,73 @@ virAdmClientDispose(void *obj)
>
> virObjectUnref(clt->srv);
> }
> +
> +
> +/*
> + * Tests if feature is supported by connection. If testing failed
> + * due to error then function returns true as well and set @err flag
> + * to true. Thus positive result should be checked for an error.
> + * If @err is already set to true then no checking is done and
> + * the function returns true immediately so that previous error
> + * is not overwritten.
> + *
> + * Returns:
> + * truefeature is supported or testing hit error
> + * false feature is not supported
> + */
> +bool
> +virDrvSupportsFeature(virConnectPtr conn,
> + virDrvFeature feature,
> + bool *err)
> +{
> +int rc;
> +
> +if (*err)
> +return true;
> +
> +if (!conn->driver->connectSupportsFeature)
> +return false;
> +
> +if ((rc = conn->driver->connectSupportsFeature(conn, feature)) < 0) {
> +*err = true;
> +return true;
> +}
> +
> +return rc > 0 ? true : false;
> +}
I would just make virDrvSupportsFeature a tiny wrapper around
conn->driver->connectSupportsFeature checking
conn->driver->connectSupportsFeature != NULL and that's it. It could
break the if/elseif flow, but with much cleaner semantics and reduced
black magic.
Jirka
Re: [libvirt PATCH v2] qemu: substitute missing model name for host-passthrough
On 9/23/20 10:18 AM, Jiri Denemark wrote:
> On Wed, Sep 23, 2020 at 09:26:58 +0200, Tim Wiederhake wrote:
>> From: Collin Walling
>>
>> Before:
>> $ uname -m
>> s390x
>> $ cat passthrough-cpu.xml
>>
>> $ virsh hypervisor-cpu-compare passthrough-cpu.xml
>> error: Failed to compare hypervisor CPU with passthrough-cpu.xml
>> error: internal error: unable to execute QEMU command 'query-cpu-model-comp
>> arison': Invalid parameter type for 'modelb.name', expected: string
>>
>> After:
>> $ virsh hypervisor-cpu-compare passthrough-cpu.xml
>> CPU described in passthrough-cpu.xml is identical to the CPU provided by hy
>> pervisor on the host
>>
>> Signed-off-by: Tim Wiederhake
>> ---
>> src/qemu/qemu_driver.c | 9 +
>> 1 file changed, 9 insertions(+)
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index ae715c01d7..1cecef01f7 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -12336,6 +12336,15 @@ qemuConnectCompareHypervisorCPU(virConnectPtr conn,
>> if (virCPUDefParseXMLString(xmlCPU, VIR_CPU_TYPE_AUTO, ) < 0)
>> goto cleanup;
>>
>> +if (!cpu->model) {
>> +if (cpu->mode == VIR_CPU_MODE_HOST_PASSTHROUGH) {
>> +cpu->model = g_strdup("host");
>> +} else {
>> +virReportError(VIR_ERR_INVALID_ARG, "%s",
>> + _("cpu parameter is missing a model name"));
>> +goto cleanup;
>> +}
>> +}
>> ret = qemuConnectCPUModelComparison(qemuCaps, cfg->libDir,
>> cfg->user, cfg->group,
>> hvCPU, cpu, failIncompatible);
>
> Reviewed-by: Jiri Denemark
>
> I'll wait some time for Collin to add Signed-of-by tag before pushing
> this.
>
Signed-off-by: Collin Walling
Thanks!
--
Regards,
Collin
Stay safe and stay healthy
Re: [libvirt PATCH 11/14] storage: createFileDir: use less ternary operators
On a Wednesday in 2020, Ján Tomko wrote:
Introduce separate variables and if conditions
with spaces around them to make the function call
easier to read.
Signed-off-by: Ján Tomko
---
src/storage/storage_util.c | 14 +-
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c
index 93c24ab6bc..49ecbc5344 100644
--- a/src/storage/storage_util.c
+++ b/src/storage/storage_util.c
@@ -1997,6 +1997,8 @@ createFileDir(virStoragePoolObjPtr pool,
unsigned int flags)
This function already has flags ^
{
virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
+mode_t permmode = VIR_STORAGE_DEFAULT_VOL_PERM_MODE;
+unsigned int flags = 0;
Consider
s/flags/createflags/
squashed in from here on
Jano
signature.asc
Description: PGP signature
[libvirt PATCH 2/6] conf: split out virDomainDefParseMemory
Signed-off-by: Ján Tomko
---
src/conf/domain_conf.c | 209 ++---
1 file changed, 114 insertions(+), 95 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index c208cb91a6..2d420458c6 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -21160,6 +21160,119 @@ virDomainDefParseCaps(virDomainDefPtr def,
}
+static int
+virDomainDefParseMemory(virDomainDefPtr def,
+xmlXPathContextPtr ctxt)
+{
+g_autofree xmlNodePtr *nodes = NULL;
+g_autofree char *tmp = NULL;
+xmlNodePtr node = NULL;
+size_t i;
+int n;
+
+/* Extract domain memory */
+if (virDomainParseMemory("./memory[1]", NULL, ctxt,
+ >mem.total_memory, false, true) < 0)
+goto error;
+
+if (virDomainParseMemory("./currentMemory[1]", NULL, ctxt,
+ >mem.cur_balloon, false, true) < 0)
+goto error;
+
+if (virDomainParseMemory("./maxMemory[1]", NULL, ctxt,
+ >mem.max_memory, false, false) < 0)
+goto error;
+
+if (virXPathUInt("string(./maxMemory[1]/@slots)", ctxt,
>mem.memory_slots) == -2) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("Failed to parse memory slot count"));
+goto error;
+}
+
+/* and info about it */
+if ((tmp = virXPathString("string(./memory[1]/@dumpCore)", ctxt)) &&
+(def->mem.dump_core = virTristateSwitchTypeFromString(tmp)) <= 0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Invalid memory core dump attribute value '%s'"),
tmp);
+goto error;
+}
+VIR_FREE(tmp);
+
+tmp = virXPathString("string(./memoryBacking/source/@type)", ctxt);
+if (tmp) {
+if ((def->mem.source = virDomainMemorySourceTypeFromString(tmp)) <= 0)
{
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unknown memoryBacking/source/type '%s'"), tmp);
+goto error;
+}
+VIR_FREE(tmp);
+}
+
+tmp = virXPathString("string(./memoryBacking/access/@mode)", ctxt);
+if (tmp) {
+if ((def->mem.access = virDomainMemoryAccessTypeFromString(tmp)) <= 0)
{
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unknown memoryBacking/access/mode '%s'"), tmp);
+goto error;
+}
+VIR_FREE(tmp);
+}
+
+tmp = virXPathString("string(./memoryBacking/allocation/@mode)", ctxt);
+if (tmp) {
+if ((def->mem.allocation =
virDomainMemoryAllocationTypeFromString(tmp)) <= 0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unknown memoryBacking/allocation/mode '%s'"),
tmp);
+goto error;
+}
+VIR_FREE(tmp);
+}
+
+if (virXPathNode("./memoryBacking/hugepages", ctxt)) {
+/* hugepages will be used */
+if ((n = virXPathNodeSet("./memoryBacking/hugepages/page", ctxt,
)) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("cannot extract hugepages nodes"));
+goto error;
+}
+
+if (n) {
+if (VIR_ALLOC_N(def->mem.hugepages, n) < 0)
+goto error;
+
+for (i = 0; i < n; i++) {
+if (virDomainHugepagesParseXML(nodes[i], ctxt,
+ >mem.hugepages[i]) < 0)
+goto error;
+def->mem.nhugepages++;
+}
+
+VIR_FREE(nodes);
+} else {
+/* no hugepage pages */
+if (VIR_ALLOC(def->mem.hugepages) < 0)
+goto error;
+
+def->mem.nhugepages = 1;
+}
+}
+
+if ((node = virXPathNode("./memoryBacking/nosharepages", ctxt)))
+def->mem.nosharepages = true;
+
+if (virXPathBoolean("boolean(./memoryBacking/locked)", ctxt))
+def->mem.locked = true;
+
+if (virXPathBoolean("boolean(./memoryBacking/discard)", ctxt))
+def->mem.discard = VIR_TRISTATE_BOOL_YES;
+
+return 0;
+
+ error:
+return -1;
+}
+
+
static int
virDomainMemorytuneDefParseMemory(xmlXPathContextPtr ctxt,
xmlNodePtr node,
@@ -21344,102 +21457,8 @@ virDomainDefParseXML(xmlDocPtr xml,
goto error;
}
-/* Extract domain memory */
-if (virDomainParseMemory("./memory[1]", NULL, ctxt,
- >mem.total_memory, false, true) < 0)
+if (virDomainDefParseMemory(def, ctxt) < 0)
goto error;
-
-if (virDomainParseMemory("./currentMemory[1]", NULL, ctxt,
- >mem.cur_balloon, false, true) < 0)
-goto error;
-
-if (virDomainParseMemory("./maxMemory[1]", NULL, ctxt,
- >mem.max_memory, false, false) < 0)
-goto error;
-
-if (virXPathUInt("string(./maxMemory[1]/@slots)", ctxt,
[libvirt PATCH 0/6] conf: split some funcs out of virDomainDefParseXML
Ján Tomko (6): conf: split out virDomainDefParseIDs conf: split out virDomainDefParseMemory conf: introduce virDomainDefTunablesParse conf: introduce virDomainDefLifecycleParse conf: introduce virDomainDefClockParse conf: introduce virDomainDefControllersParse src/conf/domain_conf.c | 728 - 1 file changed, 419 insertions(+), 309 deletions(-) -- 2.26.2
[libvirt PATCH 6/6] conf: introduce virDomainDefControllersParse
Signed-off-by: Ján Tomko
---
src/conf/domain_conf.c | 122 +++--
1 file changed, 70 insertions(+), 52 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index ccfe32685c..680524ce08 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -21795,6 +21795,75 @@ virDomainDefClockParse(virDomainDefPtr def,
return -1;
}
+static int
+virDomainDefControllersParse(virDomainDefPtr def,
+ xmlXPathContextPtr ctxt,
+ virDomainXMLOptionPtr xmlopt,
+ unsigned int flags,
+ bool *usb_none)
+{
+g_autofree xmlNodePtr *nodes = NULL;
+bool usb_other = false;
+bool usb_master = false;
+size_t i;
+int n;
+
+if ((n = virXPathNodeSet("./devices/controller", ctxt, )) < 0)
+goto error;
+
+if (n && VIR_ALLOC_N(def->controllers, n) < 0)
+goto error;
+
+for (i = 0; i < n; i++) {
+virDomainControllerDefPtr controller =
virDomainControllerDefParseXML(xmlopt,
+
nodes[i],
+
ctxt,
+
flags);
+
+if (!controller)
+goto error;
+
+/* sanitize handling of "none" usb controller */
+if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_USB) {
+if (controller->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE) {
+if (usb_other || *usb_none) {
+virDomainControllerDefFree(controller);
+virReportError(VIR_ERR_XML_DETAIL, "%s",
+ _("Can't add another USB controller: "
+ "USB is disabled for this domain"));
+goto error;
+}
+*usb_none = true;
+} else {
+if (*usb_none) {
+virDomainControllerDefFree(controller);
+virReportError(VIR_ERR_XML_DETAIL, "%s",
+ _("Can't add another USB controller: "
+ "USB is disabled for this domain"));
+goto error;
+}
+usb_other = true;
+}
+
+if (controller->info.mastertype ==
VIR_DOMAIN_CONTROLLER_MASTER_NONE)
+usb_master = true;
+}
+
+virDomainControllerInsertPreAlloced(def, controller);
+}
+VIR_FREE(nodes);
+
+if (usb_other && !usb_master) {
+virReportError(VIR_ERR_XML_DETAIL, "%s",
+ _("No master USB controller specified"));
+goto error;
+}
+
+return 0;
+
+ error:
+return -1;
+}
static virDomainDefPtr
virDomainDefParseXML(xmlDocPtr xml,
@@ -21808,8 +21877,6 @@ virDomainDefParseXML(xmlDocPtr xml,
virDomainDefPtr def;
bool uuid_generated = false;
bool usb_none = false;
-bool usb_other = false;
-bool usb_master = false;
g_autofree xmlNodePtr *nodes = NULL;
g_autofree char *tmp = NULL;
@@ -21940,58 +22007,9 @@ virDomainDefParseXML(xmlDocPtr xml,
}
VIR_FREE(nodes);
-/* analysis of the controller devices */
-if ((n = virXPathNodeSet("./devices/controller", ctxt, )) < 0)
+if (virDomainDefControllersParse(def, ctxt, xmlopt, flags, _none) < 0)
goto error;
-if (n && VIR_ALLOC_N(def->controllers, n) < 0)
-goto error;
-
-for (i = 0; i < n; i++) {
-virDomainControllerDefPtr controller =
virDomainControllerDefParseXML(xmlopt,
-
nodes[i],
-
ctxt,
-
flags);
-
-if (!controller)
-goto error;
-
-/* sanitize handling of "none" usb controller */
-if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_USB) {
-if (controller->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE) {
-if (usb_other || usb_none) {
-virDomainControllerDefFree(controller);
-virReportError(VIR_ERR_XML_DETAIL, "%s",
- _("Can't add another USB controller: "
- "USB is disabled for this domain"));
-goto error;
-}
-usb_none = true;
-} else {
-if (usb_none) {
-virDomainControllerDefFree(controller);
-virReportError(VIR_ERR_XML_DETAIL, "%s",
- _("Can't add another USB controller: "
- "USB is disabled for this
[libvirt PATCH 1/6] conf: split out virDomainDefParseIDs
Signed-off-by: Ján Tomko
---
src/conf/domain_conf.c | 139 +++--
1 file changed, 79 insertions(+), 60 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 22c6ba3b0d..c208cb91a6 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -21011,6 +21011,83 @@ virDomainCachetuneDefParse(virDomainDefPtr def,
}
+static int
+virDomainDefParseIDs(virDomainDefPtr def,
+ xmlXPathContextPtr ctxt,
+ unsigned int flags,
+ bool *uuid_generated)
+{
+g_autofree xmlNodePtr *nodes = NULL;
+g_autofree char *tmp = NULL;
+long id = -1;
+int n;
+
+if (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE))
+if (virXPathLong("string(./@id)", ctxt, ) < 0)
+id = -1;
+def->id = (int)id;
+
+/* Extract domain name */
+if (!(def->name = virXPathString("string(./name[1])", ctxt))) {
+virReportError(VIR_ERR_NO_NAME, NULL);
+goto error;
+}
+
+/* Extract domain uuid. If both uuid and sysinfo/system/entry/uuid
+ * exist, they must match; and if only the latter exists, it can
+ * also serve as the uuid. */
+tmp = virXPathString("string(./uuid[1])", ctxt);
+if (!tmp) {
+if (virUUIDGenerate(def->uuid) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ "%s", _("Failed to generate UUID"));
+goto error;
+}
+*uuid_generated = true;
+} else {
+if (virUUIDParse(tmp, def->uuid) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ "%s", _("malformed uuid element"));
+goto error;
+}
+VIR_FREE(tmp);
+}
+
+/* Extract domain genid - a genid can either be provided or generated */
+if ((n = virXPathNodeSet("./genid", ctxt, )) < 0)
+goto error;
+
+if (n > 0) {
+if (n != 1) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("element 'genid' can only appear once"));
+goto error;
+}
+def->genidRequested = true;
+if (!(tmp = virXPathString("string(./genid)", ctxt))) {
+if (virUUIDGenerate(def->genid) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ "%s", _("Failed to generate genid"));
+goto error;
+}
+def->genidGenerated = true;
+} else {
+if (virUUIDParse(tmp, def->genid) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ "%s", _("malformed genid element"));
+goto error;
+}
+VIR_FREE(tmp);
+}
+}
+VIR_FREE(nodes);
+return 0;
+
+ error:
+return -1;
+}
+
+
static int
virDomainDefParseCaps(virDomainDefPtr def,
xmlXPathContextPtr ctxt,
@@ -21220,7 +21297,6 @@ virDomainDefParseXML(xmlDocPtr xml,
xmlNodePtr node = NULL;
size_t i, j;
int n;
-long id = -1;
virDomainDefPtr def;
bool uuid_generated = false;
bool usb_none = false;
@@ -21244,69 +21320,12 @@ virDomainDefParseXML(xmlDocPtr xml,
if (!(def = virDomainDefNew()))
return NULL;
-if (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE))
-if (virXPathLong("string(./@id)", ctxt, ) < 0)
-id = -1;
-def->id = (int)id;
+if (virDomainDefParseIDs(def, ctxt, flags, _generated) < 0)
+goto error;
if (virDomainDefParseCaps(def, ctxt, xmlopt) < 0)
goto error;
-/* Extract domain name */
-if (!(def->name = virXPathString("string(./name[1])", ctxt))) {
-virReportError(VIR_ERR_NO_NAME, NULL);
-goto error;
-}
-
-/* Extract domain uuid. If both uuid and sysinfo/system/entry/uuid
- * exist, they must match; and if only the latter exists, it can
- * also serve as the uuid. */
-tmp = virXPathString("string(./uuid[1])", ctxt);
-if (!tmp) {
-if (virUUIDGenerate(def->uuid) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
- "%s", _("Failed to generate UUID"));
-goto error;
-}
-uuid_generated = true;
-} else {
-if (virUUIDParse(tmp, def->uuid) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
- "%s", _("malformed uuid element"));
-goto error;
-}
-VIR_FREE(tmp);
-}
-
-/* Extract domain genid - a genid can either be provided or generated */
-if ((n = virXPathNodeSet("./genid", ctxt, )) < 0)
-goto error;
-
-if (n > 0) {
-if (n != 1) {
-virReportError(VIR_ERR_XML_ERROR, "%s",
- _("element 'genid' can only appear once"));
-goto error;
-}
-def->genidRequested = true;
-if (!(tmp = virXPathString("string(./genid)", ctxt))) {
-if
[libvirt PATCH 5/6] conf: introduce virDomainDefClockParse
Signed-off-by: Ján Tomko
---
src/conf/domain_conf.c | 185 +++--
1 file changed, 102 insertions(+), 83 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 41dba831ce..ccfe32685c 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -21695,6 +21695,107 @@ virDomainDefLifecycleParse(virDomainDefPtr def,
}
+static int
+virDomainDefClockParse(virDomainDefPtr def,
+ xmlXPathContextPtr ctxt)
+{
+size_t i;
+int n;
+g_autofree xmlNodePtr *nodes = NULL;
+g_autofree char *tmp = NULL;
+
+if ((tmp = virXPathString("string(./clock/@offset)", ctxt)) &&
+(def->clock.offset = virDomainClockOffsetTypeFromString(tmp)) < 0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unknown clock offset '%s'"), tmp);
+goto error;
+}
+VIR_FREE(tmp);
+
+switch (def->clock.offset) {
+case VIR_DOMAIN_CLOCK_OFFSET_LOCALTIME:
+case VIR_DOMAIN_CLOCK_OFFSET_UTC:
+tmp = virXPathString("string(./clock/@adjustment)", ctxt);
+if (tmp) {
+if (STREQ(tmp, "reset")) {
+def->clock.data.utc_reset = true;
+} else {
+if (virStrToLong_ll(tmp, NULL, 10,
+>clock.data.variable.adjustment) < 0)
{
+virReportError(VIR_ERR_XML_ERROR,
+ _("unknown clock adjustment '%s'"),
+ tmp);
+goto error;
+}
+switch (def->clock.offset) {
+case VIR_DOMAIN_CLOCK_OFFSET_LOCALTIME:
+def->clock.data.variable.basis =
VIR_DOMAIN_CLOCK_BASIS_LOCALTIME;
+break;
+case VIR_DOMAIN_CLOCK_OFFSET_UTC:
+def->clock.data.variable.basis =
VIR_DOMAIN_CLOCK_BASIS_UTC;
+break;
+}
+def->clock.offset = VIR_DOMAIN_CLOCK_OFFSET_VARIABLE;
+}
+VIR_FREE(tmp);
+} else {
+def->clock.data.utc_reset = false;
+}
+break;
+
+case VIR_DOMAIN_CLOCK_OFFSET_VARIABLE:
+if (virXPathLongLong("number(./clock/@adjustment)", ctxt,
+ >clock.data.variable.adjustment) < 0)
+def->clock.data.variable.adjustment = 0;
+if (virXPathLongLong("number(./clock/@adjustment0)", ctxt,
+ >clock.data.variable.adjustment0) < 0)
+def->clock.data.variable.adjustment0 = 0;
+tmp = virXPathString("string(./clock/@basis)", ctxt);
+if (tmp) {
+if ((def->clock.data.variable.basis =
virDomainClockBasisTypeFromString(tmp)) < 0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unknown clock basis '%s'"), tmp);
+goto error;
+}
+VIR_FREE(tmp);
+} else {
+def->clock.data.variable.basis = VIR_DOMAIN_CLOCK_BASIS_UTC;
+}
+break;
+
+case VIR_DOMAIN_CLOCK_OFFSET_TIMEZONE:
+def->clock.data.timezone = virXPathString("string(./clock/@timezone)",
ctxt);
+if (!def->clock.data.timezone) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("missing 'timezone' attribute for clock with
offset='timezone'"));
+goto error;
+}
+break;
+}
+
+if ((n = virXPathNodeSet("./clock/timer", ctxt, )) < 0)
+goto error;
+
+if (n && VIR_ALLOC_N(def->clock.timers, n) < 0)
+goto error;
+
+for (i = 0; i < n; i++) {
+virDomainTimerDefPtr timer = virDomainTimerDefParseXML(nodes[i],
+ ctxt);
+if (!timer)
+goto error;
+
+def->clock.timers[def->clock.ntimers++] = timer;
+}
+VIR_FREE(nodes);
+
+return 0;
+
+ error:
+return -1;
+}
+
+
static virDomainDefPtr
virDomainDefParseXML(xmlDocPtr xml,
xmlXPathContextPtr ctxt,
@@ -21814,90 +21915,8 @@ virDomainDefParseXML(xmlDocPtr xml,
if (virDomainPerfDefParseXML(def, ctxt) < 0)
goto error;
-if ((tmp = virXPathString("string(./clock/@offset)", ctxt)) &&
-(def->clock.offset = virDomainClockOffsetTypeFromString(tmp)) < 0) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("unknown clock offset '%s'"), tmp);
+if (virDomainDefClockParse(def, ctxt) < 0)
goto error;
-}
-VIR_FREE(tmp);
-
-switch (def->clock.offset) {
-case VIR_DOMAIN_CLOCK_OFFSET_LOCALTIME:
-case VIR_DOMAIN_CLOCK_OFFSET_UTC:
-tmp = virXPathString("string(./clock/@adjustment)", ctxt);
-if (tmp) {
-if (STREQ(tmp, "reset")) {
-def->clock.data.utc_reset = true;
-} else {
-if (virStrToLong_ll(tmp,
[libvirt PATCH 3/6] conf: introduce virDomainDefTunablesParse
Signed-off-by: Ján Tomko
---
src/conf/domain_conf.c | 129 -
1 file changed, 75 insertions(+), 54 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 2d420458c6..c12cc1f216 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -21401,64 +21401,16 @@ virDomainMemorytuneDefParse(virDomainDefPtr def,
}
-static virDomainDefPtr
-virDomainDefParseXML(xmlDocPtr xml,
- xmlXPathContextPtr ctxt,
- virDomainXMLOptionPtr xmlopt,
- unsigned int flags)
+static int
+virDomainDefTunablesParse(virDomainDefPtr def,
+ xmlXPathContextPtr ctxt,
+ virDomainXMLOptionPtr xmlopt,
+ unsigned int flags)
{
-xmlNodePtr node = NULL;
+g_autofree xmlNodePtr *nodes = NULL;
size_t i, j;
int n;
-virDomainDefPtr def;
-bool uuid_generated = false;
-bool usb_none = false;
-bool usb_other = false;
-bool usb_master = false;
-g_autofree xmlNodePtr *nodes = NULL;
-g_autofree char *tmp = NULL;
-if (flags & VIR_DOMAIN_DEF_PARSE_VALIDATE_SCHEMA) {
-g_autofree char *schema = NULL;
-
-schema = virFileFindResource("domain.rng",
- abs_top_srcdir "/docs/schemas",
- PKGDATADIR "/schemas");
-if (!schema)
-return NULL;
-if (virXMLValidateAgainstSchema(schema, xml) < 0)
-return NULL;
-}
-
-if (!(def = virDomainDefNew()))
-return NULL;
-
-if (virDomainDefParseIDs(def, ctxt, flags, _generated) < 0)
-goto error;
-
-if (virDomainDefParseCaps(def, ctxt, xmlopt) < 0)
-goto error;
-
-/* Extract short description of domain (title) */
-def->title = virXPathString("string(./title[1])", ctxt);
-if (def->title && strchr(def->title, '\n')) {
-virReportError(VIR_ERR_XML_ERROR, "%s",
- _("Domain title can't contain newlines"));
-goto error;
-}
-
-/* Extract documentation if present */
-def->description = virXPathString("string(./description[1])", ctxt);
-
-/* analysis of security label, done early even though we format it
- * late, so devices can refer to this for defaults */
-if (!(flags & VIR_DOMAIN_DEF_PARSE_SKIP_SECLABEL)) {
-if (virSecurityLabelDefsParseXML(def, ctxt, xmlopt, flags) == -1)
-goto error;
-}
-
-if (virDomainDefParseMemory(def, ctxt) < 0)
-goto error;
/* Extract blkio cgroup tunables */
if (virXPathUInt("string(./blkiotune/weight)", ctxt,
>blkio.weight) < 0)
@@ -21687,6 +21639,75 @@ virDomainDefParseXML(xmlDocPtr xml,
}
VIR_FREE(nodes);
+return 0;
+
+ error:
+return -1;
+}
+
+
+static virDomainDefPtr
+virDomainDefParseXML(xmlDocPtr xml,
+ xmlXPathContextPtr ctxt,
+ virDomainXMLOptionPtr xmlopt,
+ unsigned int flags)
+{
+xmlNodePtr node = NULL;
+size_t i, j;
+int n;
+virDomainDefPtr def;
+bool uuid_generated = false;
+bool usb_none = false;
+bool usb_other = false;
+bool usb_master = false;
+g_autofree xmlNodePtr *nodes = NULL;
+g_autofree char *tmp = NULL;
+
+if (flags & VIR_DOMAIN_DEF_PARSE_VALIDATE_SCHEMA) {
+g_autofree char *schema = NULL;
+
+schema = virFileFindResource("domain.rng",
+ abs_top_srcdir "/docs/schemas",
+ PKGDATADIR "/schemas");
+if (!schema)
+return NULL;
+if (virXMLValidateAgainstSchema(schema, xml) < 0)
+return NULL;
+}
+
+if (!(def = virDomainDefNew()))
+return NULL;
+
+if (virDomainDefParseIDs(def, ctxt, flags, _generated) < 0)
+goto error;
+
+if (virDomainDefParseCaps(def, ctxt, xmlopt) < 0)
+goto error;
+
+/* Extract short description of domain (title) */
+def->title = virXPathString("string(./title[1])", ctxt);
+if (def->title && strchr(def->title, '\n')) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("Domain title can't contain newlines"));
+goto error;
+}
+
+/* Extract documentation if present */
+def->description = virXPathString("string(./description[1])", ctxt);
+
+/* analysis of security label, done early even though we format it
+ * late, so devices can refer to this for defaults */
+if (!(flags & VIR_DOMAIN_DEF_PARSE_SKIP_SECLABEL)) {
+if (virSecurityLabelDefsParseXML(def, ctxt, xmlopt, flags) == -1)
+goto error;
+}
+
+if (virDomainDefParseMemory(def, ctxt) < 0)
+goto error;
+
+if (virDomainDefTunablesParse(def, ctxt, xmlopt, flags) < 0)
+goto error;
+
if (virCPUDefParseXML(ctxt, "./cpu[1]", VIR_CPU_TYPE_GUEST, >cpu) < 0)
[libvirt PATCH 4/6] conf: introduce virDomainDefLifecycleParse
Signed-off-by: Ján Tomko
---
src/conf/domain_conf.c | 86 --
1 file changed, 50 insertions(+), 36 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index c12cc1f216..41dba831ce 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -21646,6 +21646,55 @@ virDomainDefTunablesParse(virDomainDefPtr def,
}
+static int
+virDomainDefLifecycleParse(virDomainDefPtr def,
+ xmlXPathContextPtr ctxt)
+{
+if (virDomainEventActionParseXML(ctxt, "on_reboot",
+ "string(./on_reboot[1])",
+ >onReboot,
+ VIR_DOMAIN_LIFECYCLE_ACTION_RESTART,
+ virDomainLifecycleActionTypeFromString) <
0)
+goto error;
+
+if (virDomainEventActionParseXML(ctxt, "on_poweroff",
+ "string(./on_poweroff[1])",
+ >onPoweroff,
+ VIR_DOMAIN_LIFECYCLE_ACTION_DESTROY,
+ virDomainLifecycleActionTypeFromString) <
0)
+goto error;
+
+if (virDomainEventActionParseXML(ctxt, "on_crash",
+ "string(./on_crash[1])",
+ >onCrash,
+ VIR_DOMAIN_LIFECYCLE_ACTION_DESTROY,
+ virDomainLifecycleActionTypeFromString) <
0)
+goto error;
+
+if (virDomainEventActionParseXML(ctxt, "on_lockfailure",
+ "string(./on_lockfailure[1])",
+ >onLockFailure,
+ VIR_DOMAIN_LOCK_FAILURE_DEFAULT,
+ virDomainLockFailureTypeFromString) < 0)
+goto error;
+
+if (virDomainPMStateParseXML(ctxt,
+ "string(./pm/suspend-to-mem/@enabled)",
+ >pm.s3) < 0)
+goto error;
+
+if (virDomainPMStateParseXML(ctxt,
+ "string(./pm/suspend-to-disk/@enabled)",
+ >pm.s4) < 0)
+goto error;
+
+return 0;
+
+ error:
+return -1;
+}
+
+
static virDomainDefPtr
virDomainDefParseXML(xmlDocPtr xml,
xmlXPathContextPtr ctxt,
@@ -21759,42 +21808,7 @@ virDomainDefParseXML(xmlDocPtr xml,
if (virDomainFeaturesDefParse(def, ctxt) < 0)
goto error;
-if (virDomainEventActionParseXML(ctxt, "on_reboot",
- "string(./on_reboot[1])",
- >onReboot,
- VIR_DOMAIN_LIFECYCLE_ACTION_RESTART,
- virDomainLifecycleActionTypeFromString) <
0)
-goto error;
-
-if (virDomainEventActionParseXML(ctxt, "on_poweroff",
- "string(./on_poweroff[1])",
- >onPoweroff,
- VIR_DOMAIN_LIFECYCLE_ACTION_DESTROY,
- virDomainLifecycleActionTypeFromString) <
0)
-goto error;
-
-if (virDomainEventActionParseXML(ctxt, "on_crash",
- "string(./on_crash[1])",
- >onCrash,
- VIR_DOMAIN_LIFECYCLE_ACTION_DESTROY,
- virDomainLifecycleActionTypeFromString) <
0)
-goto error;
-
-if (virDomainEventActionParseXML(ctxt, "on_lockfailure",
- "string(./on_lockfailure[1])",
- >onLockFailure,
- VIR_DOMAIN_LOCK_FAILURE_DEFAULT,
- virDomainLockFailureTypeFromString) < 0)
-goto error;
-
-if (virDomainPMStateParseXML(ctxt,
- "string(./pm/suspend-to-mem/@enabled)",
- >pm.s3) < 0)
-goto error;
-
-if (virDomainPMStateParseXML(ctxt,
- "string(./pm/suspend-to-disk/@enabled)",
- >pm.s4) < 0)
+if (virDomainDefLifecycleParse(def, ctxt) < 0)
goto error;
if (virDomainPerfDefParseXML(def, ctxt) < 0)
--
2.26.2
[libvirt PATCH 11/14] storage: createFileDir: use less ternary operators
Introduce separate variables and if conditions
with spaces around them to make the function call
easier to read.
Signed-off-by: Ján Tomko
---
src/storage/storage_util.c | 14 +-
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c
index 93c24ab6bc..49ecbc5344 100644
--- a/src/storage/storage_util.c
+++ b/src/storage/storage_util.c
@@ -1997,6 +1997,8 @@ createFileDir(virStoragePoolObjPtr pool,
unsigned int flags)
{
virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
+mode_t permmode = VIR_STORAGE_DEFAULT_VOL_PERM_MODE;
+unsigned int flags = 0;
virCheckFlags(0, -1);
@@ -2013,15 +2015,17 @@ createFileDir(virStoragePoolObjPtr pool,
return -1;
}
+if (vol->target.perms->mode != (mode_t)-1)
+permmode = vol->target.perms->mode;
+
+if (def->type == VIR_STORAGE_POOL_NETFS)
+flags |= VIR_DIR_CREATE_AS_UID;
if (virDirCreate(vol->target.path,
- (vol->target.perms->mode == (mode_t)-1 ?
- VIR_STORAGE_DEFAULT_VOL_PERM_MODE :
- vol->target.perms->mode),
+ permmode,
vol->target.perms->uid,
vol->target.perms->gid,
- (def->type == VIR_STORAGE_POOL_NETFS
- ? VIR_DIR_CREATE_AS_UID : 0)) < 0) {
+ flags) < 0) {
return -1;
}
--
2.26.2
[libvirt PATCH 14/14] Reduce scope of some variables
Signed-off-by: Ján Tomko
---
src/libvirt-host.c | 4 +---
src/node_device/node_device_driver.c | 3 ++-
src/storage/storage_backend_mpath.c | 3 +--
src/storage/storage_util.c | 6 ++
tools/virsh-pool.c | 3 ++-
5 files changed, 8 insertions(+), 11 deletions(-)
diff --git a/src/libvirt-host.c b/src/libvirt-host.c
index 07d13585f4..58622d415d 100644
--- a/src/libvirt-host.c
+++ b/src/libvirt-host.c
@@ -1352,8 +1352,6 @@ virConnectSetKeepAlive(virConnectPtr conn,
int interval,
unsigned int count)
{
-int ret = -1;
-
VIR_DEBUG("conn=%p, interval=%d, count=%u", conn, interval, count);
virResetLastError();
@@ -1361,7 +1359,7 @@ virConnectSetKeepAlive(virConnectPtr conn,
virCheckConnectReturn(conn, -1);
if (conn->driver->connectSetKeepAlive) {
-ret = conn->driver->connectSetKeepAlive(conn, interval, count);
+int ret = conn->driver->connectSetKeepAlive(conn, interval, count);
if (ret < 0)
goto error;
return ret;
diff --git a/src/node_device/node_device_driver.c
b/src/node_device/node_device_driver.c
index f4b140bef4..f5ea973c7a 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -747,7 +747,6 @@ nodeDeviceCreateXML(virConnectPtr conn,
g_autoptr(virNodeDeviceDef) def = NULL;
g_autofree char *wwnn = NULL;
g_autofree char *wwpn = NULL;
-int parent_host = -1;
virNodeDevicePtr device = NULL;
const char *virt_type = NULL;
@@ -765,6 +764,8 @@ nodeDeviceCreateXML(virConnectPtr conn,
return NULL;
if (nodeDeviceHasCapability(def, VIR_NODE_DEV_CAP_SCSI_HOST)) {
+int parent_host;
+
if (virNodeDeviceGetWWNs(def, , ) == -1)
return NULL;
diff --git a/src/storage/storage_backend_mpath.c
b/src/storage/storage_backend_mpath.c
index fffc0f86b7..f474ab32a9 100644
--- a/src/storage/storage_backend_mpath.c
+++ b/src/storage/storage_backend_mpath.c
@@ -151,13 +151,12 @@ static int
virStorageBackendCreateVols(virStoragePoolObjPtr pool,
struct dm_names *names)
{
-int is_mpath = 0;
uint32_t minor = -1;
uint32_t next;
g_autofree char *map_device = NULL;
do {
-is_mpath = virStorageBackendIsMultipath(names->name);
+int is_mpath = virStorageBackendIsMultipath(names->name);
if (is_mpath < 0)
return -1;
diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c
index 23632fc884..7bfcf53570 100644
--- a/src/storage/storage_util.c
+++ b/src/storage/storage_util.c
@@ -2416,7 +2416,6 @@ virStorageBackendVolUploadLocal(virStoragePoolObjPtr pool
G_GNUC_UNUSED,
unsigned int flags)
{
char *target_path = vol->target.path;
-int has_snap = 0;
bool sparse = flags & VIR_STORAGE_VOL_UPLOAD_SPARSE_STREAM;
g_autofree char *path = NULL;
@@ -2427,7 +2426,7 @@ virStorageBackendVolUploadLocal(virStoragePoolObjPtr pool
G_GNUC_UNUSED,
* when volUpload is fully finished. */
if (vol->target.format == VIR_STORAGE_FILE_PLOOP) {
/* Fail if the volume contains snapshots or we failed to check it.*/
-has_snap = storageBackendPloopHasSnapshots(vol->target.path);
+int has_snap = storageBackendPloopHasSnapshots(vol->target.path);
if (has_snap < 0) {
return -1;
} else if (!has_snap) {
@@ -2456,13 +2455,12 @@ virStorageBackendVolDownloadLocal(virStoragePoolObjPtr
pool G_GNUC_UNUSED,
unsigned int flags)
{
char *target_path = vol->target.path;
-int has_snap = 0;
bool sparse = flags & VIR_STORAGE_VOL_DOWNLOAD_SPARSE_STREAM;
g_autofree char *path = NULL;
virCheckFlags(VIR_STORAGE_VOL_DOWNLOAD_SPARSE_STREAM, -1);
if (vol->target.format == VIR_STORAGE_FILE_PLOOP) {
-has_snap = storageBackendPloopHasSnapshots(vol->target.path);
+int has_snap = storageBackendPloopHasSnapshots(vol->target.path);
if (has_snap < 0) {
return -1;
} else if (!has_snap) {
diff --git a/tools/virsh-pool.c b/tools/virsh-pool.c
index 634e9ac8cd..497b409989 100644
--- a/tools/virsh-pool.c
+++ b/tools/virsh-pool.c
@@ -1583,7 +1583,6 @@ cmdPoolInfo(vshControl *ctl, const vshCmd *cmd)
virStoragePoolInfo info;
virStoragePoolPtr pool;
int autostart = 0;
-int persistent = 0;
bool ret = true;
bool bytes = false;
char uuid[VIR_UUID_STRING_BUFLEN];
@@ -1601,6 +1600,8 @@ cmdPoolInfo(vshControl *ctl, const vshCmd *cmd)
if (virStoragePoolGetInfo(pool, ) == 0) {
double val;
const char *unit;
+int persistent;
+
vshPrint(ctl, "%-15s %s\n", _("State:"),
virshStoragePoolStateToString(info.state));
--
2.26.2
[libvirt PATCH 10/14] storage: createFileDir: remove useless 'err' variable
Signed-off-by: Ján Tomko
---
src/storage/storage_util.c | 17 -
1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c
index 9171cb084f..93c24ab6bc 100644
--- a/src/storage/storage_util.c
+++ b/src/storage/storage_util.c
@@ -1997,7 +1997,6 @@ createFileDir(virStoragePoolObjPtr pool,
unsigned int flags)
{
virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
-int err;
virCheckFlags(0, -1);
@@ -2015,14 +2014,14 @@ createFileDir(virStoragePoolObjPtr pool,
}
-if ((err = virDirCreate(vol->target.path,
-(vol->target.perms->mode == (mode_t)-1 ?
- VIR_STORAGE_DEFAULT_VOL_PERM_MODE :
- vol->target.perms->mode),
-vol->target.perms->uid,
-vol->target.perms->gid,
-(def->type == VIR_STORAGE_POOL_NETFS
- ? VIR_DIR_CREATE_AS_UID : 0))) < 0) {
+if (virDirCreate(vol->target.path,
+ (vol->target.perms->mode == (mode_t)-1 ?
+ VIR_STORAGE_DEFAULT_VOL_PERM_MODE :
+ vol->target.perms->mode),
+ vol->target.perms->uid,
+ vol->target.perms->gid,
+ (def->type == VIR_STORAGE_POOL_NETFS
+ ? VIR_DIR_CREATE_AS_UID : 0)) < 0) {
return -1;
}
--
2.26.2
[libvirt PATCH 00/14] Various cleanups
Some reported by cppcheck, maybe even Coverity, the rest I noticed while looking at the code. Ján Tomko (14): tools: virshCheckpointListCollect: remove unused names vbox: remove VBoxCGlueTerm vbox: StartMachine: overwrite ret less often Remove pointless assignments Remove pointless initializations virsh: virshStreamSourceSkip: remove unused 'off' Do not check whether unsigned variables are negative libxl: remove unused 'bits' from struct guest_arch api: virDomainMemoryStats: use 'ret' variable storage: createFileDir: remove useless 'err' variable storage: createFileDir: use less ternary operators vbox: reduce variable scope in vboxDumpStorageControllers storage: storageBackendWipeLocal: reduce variable scope Reduce scope of some variables examples/c/admin/logging.c | 2 +- src/libvirt-domain.c | 8 ++--- src/libvirt-host.c | 4 +-- src/libxl/libxl_capabilities.c | 1 - src/libxl/libxl_driver.c | 2 +- src/node_device/node_device_driver.c | 3 +- src/openvz/openvz_driver.c | 2 +- src/storage/storage_backend_mpath.c | 3 +- src/storage/storage_util.c | 33 ++-- src/test/test_driver.c | 2 +- src/vbox/vbox_XPCOMCGlue.c | 19 src/vbox/vbox_XPCOMCGlue.h | 1 - src/vbox/vbox_common.c | 46 +--- tests/virnumamock.c | 2 +- tests/virrandommock.c| 2 +- tools/virsh-checkpoint.c | 5 --- tools/virsh-pool.c | 3 +- tools/virsh-util.c | 3 +- 18 files changed, 53 insertions(+), 88 deletions(-) -- 2.26.2
[libvirt PATCH 04/14] Remove pointless assignments
There's no need to initialize every variable,
especially not if it's overwritten two lines later.
Signed-off-by: Ján Tomko
---
examples/c/admin/logging.c | 2 +-
src/libxl/libxl_driver.c | 2 +-
src/vbox/vbox_common.c | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/examples/c/admin/logging.c b/examples/c/admin/logging.c
index 730ae40d9d..c183c4867d 100644
--- a/examples/c/admin/logging.c
+++ b/examples/c/admin/logging.c
@@ -29,7 +29,7 @@ int main(int argc, char **argv)
const char *set_outputs = NULL;
const char *set_filters = NULL;
-ret = c = -1;
+ret = -1;
opterr = 0;
while ((c = getopt(argc, argv, ":hpo:f:")) > 0) {
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 161a6882f3..ea7d08cd11 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -5423,7 +5423,7 @@ libxlDiskSectorSize(int domid, int devno)
return ret;
}
-path = val = NULL;
+val = NULL;
path = g_strdup_printf("/local/domain/%d/device/vbd/%d/backend", domid,
devno);
if ((val = xs_read(handle, XBT_NULL, path, )) == NULL)
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index 99c7fbd117..2783827012 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -4618,7 +4618,7 @@ vboxSnapshotRedefine(virDomainPtr dom,
if (openSessionForMachine(data, dom->uuid, , ) < 0)
goto cleanup;
-rc = gVBoxAPI.UIMachine.SaveSettings(machine);
+gVBoxAPI.UIMachine.SaveSettings(machine);
/* It may failed when the machine is not mutable. */
rc = gVBoxAPI.UIMachine.GetSettingsFilePath(machine, );
if (NS_FAILED(rc)) {
--
2.26.2
[libvirt PATCH 07/14] Do not check whether unsigned variables are negative
Signed-off-by: Ján Tomko
---
src/openvz/openvz_driver.c | 2 +-
src/vbox/vbox_common.c | 9 -
2 files changed, 5 insertions(+), 6 deletions(-)
diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c
index 71e270ea09..74d87b1085 100644
--- a/src/openvz/openvz_driver.c
+++ b/src/openvz/openvz_driver.c
@@ -1215,7 +1215,7 @@ static int openvzDomainSetVcpusFlags(virDomainPtr dom,
unsigned int nvcpus,
if (!(vm = openvzDomObjFromDomain(driver, dom->uuid)))
return -1;
-if (nvcpus <= 0) {
+if (nvcpus == 0) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Number of vCPUs should be >= 1"));
goto cleanup;
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index 2783827012..b9b72e2e02 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -2111,7 +2111,7 @@ vboxStartMachine(virDomainPtr dom, int maxDomID, IMachine
*machine, vboxIID *iid
VBOX_UTF16_TO_UTF8(valueDisplayUtf16, );
VBOX_UTF16_FREE(valueDisplayUtf16);
-if (strlen(valueDisplayUtf8) <= 0)
+if (strlen(valueDisplayUtf8) == 0)
VBOX_UTF8_FREE(valueDisplayUtf8);
}
@@ -2990,7 +2990,7 @@ vboxHostDeviceGetXMLDesc(vboxDriverPtr data,
virDomainDefPtr def, IMachine *mach
gVBoxAPI.UArray.vboxArrayGet(, USBCommon,
gVBoxAPI.UArray.handleUSBGetDeviceFilters(USBCommon));
-if (deviceFilters.count <= 0)
+if (deviceFilters.count == 0)
goto release_filters;
/* check if the filters are active and then only
@@ -3612,9 +3612,8 @@ vboxDumpSharedFolders(virDomainDefPtr def, vboxDriverPtr
data, IMachine *machine
gVBoxAPI.UArray.vboxArrayGet(, machine,
gVBoxAPI.UArray.handleMachineGetSharedFolders(machine));
-if (sharedFolders.count <= 0) {
-if (sharedFolders.count == 0)
-ret = 0;
+if (sharedFolders.count == 0) {
+ret = 0;
goto cleanup;
}
--
2.26.2
[libvirt PATCH 09/14] api: virDomainMemoryStats: use 'ret' variable
Instead of 'nr_stats_ret'. Also reduce its scope.
Signed-off-by: Ján Tomko
---
src/libvirt-domain.c | 8 +++-
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index cde86c77e8..415482a526 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -5741,7 +5741,6 @@ virDomainMemoryStats(virDomainPtr dom,
virDomainMemoryStatPtr stats,
unsigned int nr_stats, unsigned int flags)
{
virConnectPtr conn;
-unsigned long nr_stats_ret = 0;
VIR_DOMAIN_DEBUG(dom, "stats=%p, nr_stats=%u, flags=0x%x",
stats, nr_stats, flags);
@@ -5758,11 +5757,10 @@ virDomainMemoryStats(virDomainPtr dom,
virDomainMemoryStatPtr stats,
conn = dom->conn;
if (conn->driver->domainMemoryStats) {
-nr_stats_ret = conn->driver->domainMemoryStats(dom, stats, nr_stats,
- flags);
-if (nr_stats_ret == -1)
+int ret = conn->driver->domainMemoryStats(dom, stats, nr_stats, flags);
+if (ret == -1)
goto error;
-return nr_stats_ret;
+return ret;
}
virReportUnsupportedError();
--
2.26.2
[libvirt PATCH 08/14] libxl: remove unused 'bits' from struct guest_arch
It was made pointless by:
commit c25c18f71bdc43a1305be4ad1a2ca91b25cf13f3
Convert capabilities / domain_conf to use virArch
and unused by:
commit 8db1f2d228bb2f27a729a873dcdb81ce3c7c38fd
Fix libxl driver for virArch changes
Signed-off-by: Ján Tomko
---
src/libxl/libxl_capabilities.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/src/libxl/libxl_capabilities.c b/src/libxl/libxl_capabilities.c
index 8284ea3d53..e5b144ea1d 100644
--- a/src/libxl/libxl_capabilities.c
+++ b/src/libxl/libxl_capabilities.c
@@ -52,7 +52,6 @@ enum libxlHwcapVersion {
struct guest_arch {
virArch arch;
-int bits;
int hvm;
int pvh;
int pae;
--
2.26.2
[libvirt PATCH 03/14] vbox: StartMachine: overwrite ret less often
Use goto to jump over the ret = 0 assignment
as is usual in rest of the code.
Signed-off-by: Ján Tomko
---
src/vbox/vbox_common.c | 12 +++-
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index 9978741a64..99c7fbd117 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -2090,7 +2090,7 @@ vboxStartMachine(virDomainPtr dom, int maxDomID, IMachine
*machine, vboxIID *iid
int ret = -1;
if (!data->vboxObj)
-return ret;
+return -1;
VBOX_UTF8_TO_UTF16("FRONTEND/Type", );
gVBoxAPI.UIMachine.GetExtraData(machine, keyTypeUtf16, );
@@ -2177,7 +2177,7 @@ vboxStartMachine(virDomainPtr dom, int maxDomID, IMachine
*machine, vboxIID *iid
if (NS_FAILED(rc)) {
virReportError(VIR_ERR_OPERATION_FAILED, "%s",
_("OpenRemoteSession/LaunchVMProcess failed, domain
can't be started"));
-ret = -1;
+goto cleanup;
} else {
PRBool completed = 0;
resultCodeUnion resultCode;
@@ -2186,19 +2186,21 @@ vboxStartMachine(virDomainPtr dom, int maxDomID,
IMachine *machine, vboxIID *iid
rc = gVBoxAPI.UIProgress.GetCompleted(progress, );
if (NS_FAILED(rc)) {
/* error */
-ret = -1;
+goto cleanup;
}
gVBoxAPI.UIProgress.GetResultCode(progress, );
if (RC_FAILED(resultCode)) {
/* error */
-ret = -1;
+goto cleanup;
} else {
/* all ok set the domid */
dom->id = maxDomID + 1;
-ret = 0;
}
}
+ret = 0;
+
+ cleanup:
VBOX_RELEASE(progress);
gVBoxAPI.UISession.Close(data->vboxSession);
--
2.26.2
[libvirt PATCH 12/14] vbox: reduce variable scope in vboxDumpStorageControllers
Most of the variables were reinitialized on every iteration.
Also remove the pointless initialization of 'i'.
Signed-off-by: Ján Tomko
---
src/vbox/vbox_common.c | 23 +--
1 file changed, 9 insertions(+), 14 deletions(-)
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index b9b72e2e02..6a517ff96c 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -3079,23 +3079,18 @@ static int
vboxDumpStorageControllers(virDomainDefPtr def, IMachine *machine)
{
vboxArray storageControllers = VBOX_ARRAY_INITIALIZER;
-IStorageController *controller = NULL;
-PRUint32 storageBus = StorageBus_Null;
-PRUint32 controllerType = StorageControllerType_Null;
-virDomainControllerDefPtr cont = NULL;
-size_t i = 0;
-int model = -1, ret = -1;
-virDomainControllerType type = VIR_DOMAIN_CONTROLLER_TYPE_LAST;
+int ret = -1;
+size_t i;
gVBoxAPI.UArray.vboxArrayGet(, machine,
gVBoxAPI.UArray.handleMachineGetStorageControllers(machine));
for (i = 0; i < storageControllers.count; i++) {
-controller = storageControllers.items[i];
-storageBus = StorageBus_Null;
-controllerType = StorageControllerType_Null;
-type = VIR_DOMAIN_CONTROLLER_TYPE_LAST;
-model = -1;
+IStorageController *controller = storageControllers.items[i];
+PRUint32 storageBus = StorageBus_Null;
+PRUint32 controllerType = StorageControllerType_Null;
+virDomainControllerType type = VIR_DOMAIN_CONTROLLER_TYPE_LAST;
+int model = -1;
if (!controller)
continue;
@@ -3133,8 +3128,6 @@ vboxDumpStorageControllers(virDomainDefPtr def, IMachine
*machine)
case StorageControllerType_IntelAhci:
case StorageControllerType_I82078:
case StorageControllerType_Null:
-model = -1;
-
break;
}
@@ -3165,6 +3158,8 @@ vboxDumpStorageControllers(virDomainDefPtr def, IMachine
*machine)
}
if (type != VIR_DOMAIN_CONTROLLER_TYPE_LAST) {
+virDomainControllerDefPtr cont;
+
cont = virDomainDefAddController(def, type, -1, model);
if (!cont) {
virReportError(VIR_ERR_INTERNAL_ERROR,
--
2.26.2
[libvirt PATCH 06/14] virsh: virshStreamSourceSkip: remove unused 'off'
Signed-off-by: Ján Tomko
---
tools/virsh-util.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/tools/virsh-util.c b/tools/virsh-util.c
index b8b0f98066..af7ed55348 100644
--- a/tools/virsh-util.c
+++ b/tools/virsh-util.c
@@ -173,9 +173,8 @@ virshStreamSourceSkip(virStreamPtr st G_GNUC_UNUSED,
{
virshStreamCallbackDataPtr cbData = opaque;
int fd = cbData->fd;
-off_t cur;
-if ((cur = lseek(fd, offset, SEEK_CUR)) == (off_t) -1)
+if (lseek(fd, offset, SEEK_CUR) == (off_t) -1)
return -1;
return 0;
--
2.26.2
[libvirt PATCH 01/14] tools: virshCheckpointListCollect: remove unused names
Introduced by:
commit 689beaa47c51fb49fafa992dd468116b8f6b0782
and unused since.
Signed-off-by: Ján Tomko
---
tools/virsh-checkpoint.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/tools/virsh-checkpoint.c b/tools/virsh-checkpoint.c
index 33e70515ad..f3c4fe90ba 100644
--- a/tools/virsh-checkpoint.c
+++ b/tools/virsh-checkpoint.c
@@ -581,7 +581,6 @@ virshCheckpointListCollect(vshControl *ctl,
bool tree)
{
size_t i;
-char **names = NULL;
int count = -1;
virDomainCheckpointPtr *chks;
virshCheckpointListPtr checkpointlist = vshMalloc(ctl,
@@ -628,10 +627,6 @@ virshCheckpointListCollect(vshControl *ctl,
cleanup:
virshCheckpointListFree(checkpointlist);
-if (names && count > 0)
-for (i = 0; i < count; i++)
-VIR_FREE(names[i]);
-VIR_FREE(names);
return ret;
}
--
2.26.2
[libvirt PATCH 13/14] storage: storageBackendWipeLocal: reduce variable scope
Also use MIN instead of open-coding it.
Signed-off-by: Ján Tomko
---
src/storage/storage_util.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c
index 49ecbc5344..23632fc884 100644
--- a/src/storage/storage_util.c
+++ b/src/storage/storage_util.c
@@ -2526,10 +2526,8 @@ storageBackendWipeLocal(const char *path,
size_t writebuf_length,
bool zero_end)
{
-int written = 0;
unsigned long long remaining = 0;
off_t size;
-size_t write_size = 0;
g_autofree char *writebuf = NULL;
if (VIR_ALLOC_N(writebuf, writebuf_length) < 0)
@@ -2557,9 +2555,9 @@ storageBackendWipeLocal(const char *path,
remaining = wipe_len;
while (remaining > 0) {
+size_t write_size = MIN(writebuf_length, remaining);
+int written = safewrite(fd, writebuf, write_size);
-write_size = (writebuf_length < remaining) ? writebuf_length :
remaining;
-written = safewrite(fd, writebuf, write_size);
if (written < 0) {
virReportSystemError(errno,
_("Failed to write %zu bytes to "
--
2.26.2
[libvirt PATCH 05/14] Remove pointless initializations
Signed-off-by: Ján Tomko
---
src/test/test_driver.c | 2 +-
tests/virnumamock.c| 2 +-
tests/virrandommock.c | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index d582c207f4..cbbfea6665 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -4354,7 +4354,7 @@ testNodeGetFreePages(virConnectPtr conn G_GNUC_UNUSED,
unsigned long long *counts,
unsigned int flags)
{
-size_t i = 0, j = 0;
+size_t i, j;
int x = 6;
virCheckFlags(0, -1);
diff --git a/tests/virnumamock.c b/tests/virnumamock.c
index 40e18e646e..d39c264a3f 100644
--- a/tests/virnumamock.c
+++ b/tests/virnumamock.c
@@ -130,7 +130,7 @@ virNumaGetPages(int node,
{
const int pages_def[] = { 4, 2 * 1024, 1 * 1024 * 1024};
const int npages_def = G_N_ELEMENTS(pages_def);
-size_t i = 0;
+size_t i;
if (pages_size)
*pages_size = g_new0(unsigned int, npages_def);
diff --git a/tests/virrandommock.c b/tests/virrandommock.c
index 6dd15213e3..ca0520a5a3 100644
--- a/tests/virrandommock.c
+++ b/tests/virrandommock.c
@@ -69,7 +69,7 @@ int
gnutls_dh_params_generate2(gnutls_dh_params_t dparams,
unsigned int bits)
{
-int rc = 0;
+int rc;
VIR_MOCK_REAL_INIT(gnutls_dh_params_generate2);
--
2.26.2
[libvirt PATCH 02/14] vbox: remove VBoxCGlueTerm
cppcheck reports:
src/vbox/vbox_XPCOMCGlue.c:226:21: style:
The statement 'if (hVBoxXPCOMC!=NULL) hVBoxXPCOMC=NULL' is
logically equivalent to 'hVBoxXPCOMC=NULL'.
[duplicateConditionalAssign]
It does not matter anyway because this function
is never called.
Fixes: e1506cb4eb7eab96e7ded27a23f0d8ac9697ac2a
Signed-off-by: Ján Tomko
---
src/vbox/vbox_XPCOMCGlue.c | 19 ---
src/vbox/vbox_XPCOMCGlue.h | 1 -
2 files changed, 20 deletions(-)
diff --git a/src/vbox/vbox_XPCOMCGlue.c b/src/vbox/vbox_XPCOMCGlue.c
index 3cbb679110..2936ff0edb 100644
--- a/src/vbox/vbox_XPCOMCGlue.c
+++ b/src/vbox/vbox_XPCOMCGlue.c
@@ -217,25 +217,6 @@ VBoxCGlueInit(unsigned int *version)
}
-/**
- * Terminate the C glue library.
- */
-void
-VBoxCGlueTerm(void)
-{
-if (hVBoxXPCOMC != NULL) {
-#if 0 /* VBoxRT.so doesn't like being reloaded. See @bugref{3725}. */
-dlclose(g_hVBoxXPCOMC);
-#endif
-hVBoxXPCOMC = NULL;
-}
-
-pVBoxFuncs_v2_2 = NULL;
-g_pfnGetFunctions = NULL;
-}
-
-
-
/*
* In XPCOM an array is represented by 1) a pointer to an array of pointers
* that point to the items and 2) an unsigned int representing the number of
diff --git a/src/vbox/vbox_XPCOMCGlue.h b/src/vbox/vbox_XPCOMCGlue.h
index 517b02393f..d0e579482e 100644
--- a/src/vbox/vbox_XPCOMCGlue.h
+++ b/src/vbox/vbox_XPCOMCGlue.h
@@ -36,7 +36,6 @@
extern PFNVBOXGETXPCOMCFUNCTIONS g_pfnGetFunctions;
int VBoxCGlueInit(unsigned int *version);
-void VBoxCGlueTerm(void);
typedef struct _vboxArray vboxArray;
--
2.26.2
Re: [PATCH] apparmor: Allow /usr/libexec for libxl-save-helper and pygrub
On Tue, Sep 22, 2020 at 6:35 PM Jim Fehlig wrote:
>
> Like other distros, openSUSE Tumbleweed recently changed libexecdir from
> /usr/lib to /usr/libexec. Add it as an allowed path for libxl-save-helper
> and pygrub.
>
> Signed-off-by: Jim Fehlig
> ---
>
> I considered including /usr/lib64, but I don't think any distros are
> installing xen libexecdir targets to /usr/lib64. Happy to include it
> if I'm wrong :-).
>
> src/security/apparmor/usr.sbin.libvirtd.in | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/security/apparmor/usr.sbin.libvirtd.in
> b/src/security/apparmor/usr.sbin.libvirtd.in
> index f2030764cd..bf4563e1e8 100644
> --- a/src/security/apparmor/usr.sbin.libvirtd.in
> +++ b/src/security/apparmor/usr.sbin.libvirtd.in
> @@ -86,8 +86,8 @@ profile libvirtd @sbindir@/libvirtd
> flags=(attach_disconnected) {
>/{usr/,}lib/udev/scsi_id PUx,
>/usr/{lib,lib64}/xen-common/bin/xen-toolstack PUx,
>/usr/{lib,lib64}/xen/bin/* Ux,
> - /usr/lib/xen-*/bin/libxl-save-helper PUx,
> - /usr/lib/xen-*/bin/pygrub PUx,
> + /usr/{lib,libexec}/xen-*/bin/libxl-save-helper PUx,
> + /usr/{lib,libexec}/xen-*/bin/pygrub PUx,
>/usr/{lib,lib64,lib/qemu,libexec}/vhost-user-gpu PUx,
>/usr/{lib,lib64,lib/qemu,libexec}/virtiofsd PUx,
>
> --
> 2.28.0
>
Yay! Looks great to me!
Reviewed-by: Neal Gompa
--
真実はいつも一つ！/ Always, there's only one truth!
Re: [PATCH] apparmor: Allow /usr/libexec for libxl-save-helper and pygrub
On Wed, Sep 23, 2020 at 12:46 PM Jim Fehlig wrote: > > On 9/23/20 7:51 AM, Jim Fehlig wrote: > > On 9/23/20 7:26 AM, Christian Ehrhardt wrote: > >> On Wed, Sep 23, 2020 at 12:35 AM Jim Fehlig wrote: > >>> > >>> Like other distros, openSUSE Tumbleweed recently changed libexecdir from > >>> /usr/lib to /usr/libexec. Add it as an allowed path for libxl-save-helper > >>> and pygrub. > >> > >> Hi Jim, > >> ack to the intention, but I think since this should use @libexecdir@ I > >> think. > >> Or did anything change that this doesn't apply anymore ... in that > >> case I beg your pardon. > >> > >> [1]: > >> https://libvirt.org/git/?p=libvirt.git;a=commit;h=5c8bd31c881e99261ac098e867a79b300440731a > >> > > > > Heh, I see that skipped over the xen stuff :-). I'll send a V2 later. > > Thinking about it more, perhaps it is best to go with this V1 patch since > these > are not files provided by libvirt but xen, where conceivably libvirt and xen > could be built with different libexecdir? IMO it would be best to explicitly > list the known paths distros have used for libxl-save-helper and pygrub. > It is entirely possible that one has not been updated yet, or someone is mixing packages, so this patch makes sense over having it assume a specific path. -- 真実はいつも一つ！/ Always, there's only one truth!
Re: [PATCH V3] Modify virCPUarmCompare to perform compare actions
On Wed, Sep 16, 2020 at 16:58:03 +0800, Zhenyu Zheng wrote:
> Modify virCPUarmCompare in cpu_arm.c to perform compare action.
> This patch only adds host to host CPU compare, the rest cases
> remains the same. This is useful for source and destination host
> compare during migrations to avoid migration between different
> CPU models that have different CPU freatures.
>
> Signed-off-by: Zhenyu Zheng
> ---
> src/cpu/cpu_arm.c | 43 +++
> 1 file changed, 39 insertions(+), 4 deletions(-)
>
> diff --git a/src/cpu/cpu_arm.c b/src/cpu/cpu_arm.c
> index 939a3b8390..b420b14e86 100644
> --- a/src/cpu/cpu_arm.c
> +++ b/src/cpu/cpu_arm.c
> @@ -463,11 +463,46 @@ virCPUarmBaseline(virCPUDefPtr *cpus,
> }
>
> static virCPUCompareResult
> -virCPUarmCompare(virCPUDefPtr host G_GNUC_UNUSED,
> - virCPUDefPtr cpu G_GNUC_UNUSED,
> - bool failMessages G_GNUC_UNUSED)
> +virCPUarmCompare(virCPUDefPtr host,
> + virCPUDefPtr cpu,
> + bool failIncompatible
> +)
> {
> -return VIR_CPU_COMPARE_IDENTICAL;
> +virCPUCompareResult ret = VIR_CPU_COMPARE_IDENTICAL;
This looks a bit too complicated as there's no common code to be
executed for several ret values. Just drop this variable and return
directly wherever you set it.
> +
> +/* Only support host to host CPU compare for ARM*/
> +if (cpu->type != VIR_CPU_TYPE_HOST)
> +return ret;
> +
> +if (!host || !host->model) {
> +if (failIncompatible) {
> +virReportError(VIR_ERR_CPU_INCOMPATIBLE, "%s",
> + _("unknown host CPU"));
> +ret = VIR_CPU_COMPARE_ERROR;
> +} else {
> +VIR_WARN("unknown host CPU");
> +ret = VIR_CPU_COMPARE_INCOMPATIBLE;
> +}
> +return ret;
> +}
> +
> +/* Compare vendor and model to check if CPUs are identical */
> +if (STRNEQ(host->vendor, cpu->vendor) ||
> +STRNEQ(host->model, cpu->model)) {
Use STRNEQ_NULLABLE instead.
> +VIR_DEBUG("Host CPU model does not match required CPU model %s",
> + cpu->model);
NULLSTR(cpu->model)
and I would also add NULLSTR(cpu->vendor) in the message just in case
the CPUs differ only in vendor.
> +
> +if (failIncompatible) {
> +ret = VIR_CPU_COMPARE_ERROR;
> +virReportError(VIR_ERR_CPU_INCOMPATIBLE,
> + _("Host CPU model does not match required CPU
> model %s"),
> + cpu->model);
> +} else {
> +ret = VIR_CPU_COMPARE_INCOMPATIBLE;
> +}
> +}
> +
> +return ret;
> }
>
> static int
Jirka
Re: [PATCH] xen: Don't add dom0 twice on driver reload
On a Friday in 2020, Jim Fehlig wrote: When the xen driver loads, it probes libxl for some info about dom0 and adds it to the virDomainObjList. The driver then looks for any domains in stateDir and if they are still alive adds them to the list as well. This logic is a bit flawed wrt handling driver reload and causes the following error internal error: unexpected domain Domain-0 already exists A simple fix is to load all domains from stateDir first and then only add dom0 if needed. Signed-off-by: Jim Fehlig --- src/libxl/libxl_driver.c | 46 +++- 1 file changed, 26 insertions(+), 20 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH] apparmor: Allow /usr/libexec for libxl-save-helper and pygrub
On 9/23/20 7:51 AM, Jim Fehlig wrote: On 9/23/20 7:26 AM, Christian Ehrhardt wrote: On Wed, Sep 23, 2020 at 12:35 AM Jim Fehlig wrote: Like other distros, openSUSE Tumbleweed recently changed libexecdir from /usr/lib to /usr/libexec. Add it as an allowed path for libxl-save-helper and pygrub. Hi Jim, ack to the intention, but I think since this should use @libexecdir@ I think. Or did anything change that this doesn't apply anymore ... in that case I beg your pardon. [1]: https://libvirt.org/git/?p=libvirt.git;a=commit;h=5c8bd31c881e99261ac098e867a79b300440731a Heh, I see that skipped over the xen stuff :-). I'll send a V2 later. Thinking about it more, perhaps it is best to go with this V1 patch since these are not files provided by libvirt but xen, where conceivably libvirt and xen could be built with different libexecdir? IMO it would be best to explicitly list the known paths distros have used for libxl-save-helper and pygrub. Regards, Jim
Re: [PATCH 13/13] virDomainSnapshotDiskDef: Remove 'idx' field
On a Wednesday in 2020, Peter Krempa wrote: It's no longer needed and is valid only after virDomainSnapshotAlignDisks is called while holding the lock. Signed-off-by: Peter Krempa --- src/conf/snapshot_conf.h | 1 - 1 file changed, 1 deletion(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH 12/13] virDomainSnapshotAlignDisks: refactor extension to all disks
On a Wednesday in 2020, Peter Krempa wrote: Last step of the algorithm in virDomainSnapshotAlignDisks is to extend the array of disks to all VM's disk and provide defaults. This was done by extending the array, adding defaults at the end and then sorting it. This requires the 'idx' variable and also a separate sorting function. If we store the pointer to existing snapshot disk definitions in a hash table and create a new array of snapshot disk definitions, we can fill the new array directly by either copying the definition from the old array or adding the default. This avoids the sorting step and thus even the need to store the index of the domain disk altogether. Signed-off-by: Peter Krempa --- src/conf/snapshot_conf.c | 48 +++- 1 file changed, 18 insertions(+), 30 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH 11/13] virDomainSnapshotAlignDisks: clarify handing of snapshot location
On a Wednesday in 2020, Peter Krempa wrote: Remove the use of the 'disk_snapshot' temporary variable since accessing the disk definition now isn't that much longer to write and use expicit explicit value checks instead of the (non-)zero check to make it more obvious what the code is doing. Signed-off-by: Peter Krempa --- src/conf/snapshot_conf.c | 16 1 file changed, 8 insertions(+), 8 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH 10/13] virDomainSnapshotAlignDisks: remove unnecessary 'tmp' variable
On a Wednesday in 2020, Peter Krempa wrote: The converted string is used exactly once so we can call the conversion without storing the result in a variable. I presume this was done to reduce line length, not to reuse it. Signed-off-by: Peter Krempa --- src/conf/snapshot_conf.c | 6 ++ 1 file changed, 2 insertions(+), 4 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH 09/13] virDomainSnapshotAlignDisks: Extract domain disk definition to a local variable
On a Wednesday in 2020, Peter Krempa wrote: Extract the disk def to a local variable so that it's more obvious what's happening and it will also allow further simplification. Signed-off-by: Peter Krempa --- src/conf/snapshot_conf.c | 9 ++--- 1 file changed, 6 insertions(+), 3 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH 12/13] virDomainSnapshotAlignDisks: refactor extension to all disks
On 9/23/20 8:33 AM, Peter Krempa wrote: Last step of the algorithm in virDomainSnapshotAlignDisks is to extend the array of disks to all VM's disk and provide defaults. This was done by extending the array, adding defaults at the end and then sorting it. This requires the 'idx' variable and also a separate sorting function. If we store the pointer to existing snapshot disk definitions in a hash table and create a new array of snapshot disk definitions, we can fill the new array directly by either copying the definition from the old array or adding the default. This avoids the sorting step and thus even the need to store the index of the domain disk altogether. Signed-off-by: Peter Krempa --- src/conf/snapshot_conf.c | 48 +++- 1 file changed, 18 insertions(+), 30 deletions(-) Nice cleanup. Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH 08/13] virDomainSnapshotAlignDisks: Add 'domdef' local variable
On a Wednesday in 2020, Peter Krempa wrote: There are multiple places accessing the domain definition. Extract it to a local variable so that it's more clear what's happening. Signed-off-by: Peter Krempa --- src/conf/snapshot_conf.c | 27 ++- 1 file changed, 14 insertions(+), 13 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH 07/13] virDomainSnapshotAlignDisks: Rename 'disk' -> 'snapdisk'
On a Wednesday in 2020, Peter Krempa wrote: The 'disk' variable usually refers to a definition of a disk from the domain definition. Rename it to 'snapdisk' to be clear that we are Sounds catchy. Have you bought the domain already? talking about the snapshot disk definition especially since this function also accesses the domain disk definition. Signed-off-by: Peter Krempa --- src/conf/snapshot_conf.c | 54 1 file changed, 27 insertions(+), 27 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH 11/13] virDomainSnapshotAlignDisks: clarify handing of snapshot location
On 9/23/20 8:33 AM, Peter Krempa wrote: Remove the use of the 'disk_snapshot' temporary variable since accessing the disk definition now isn't that much longer to write and use expicit explicit value checks instead of the (non-)zero check to make it more obvious what the code is doing. Signed-off-by: Peter Krempa --- src/conf/snapshot_conf.c | 16 1 file changed, 8 insertions(+), 8 deletions(-) Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH 06/13] virDomainSnapshotAlignDisks: Rename 'def' -> 'snapdef'
On a Wednesday in 2020, Peter Krempa wrote: While this function resides in the snapshot config module the 'def' , the variable is referencing the VM definition in most places. Change the name to 'snapdef' to avoid ambiguity especially since we are also dealing with the domain definition in this function. Signed-off-by: Peter Krempa --- src/conf/snapshot_conf.c | 42 1 file changed, 21 insertions(+), 21 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH 05/13] virDomainSnapshotAlignDisks: Refactor cleanup
On a Wednesday in 2020, Peter Krempa wrote: Use automatic pointer for the bitmap and get rid of the 'cleanup' label and 'ret' variable. Signed-off-by: Peter Krempa --- src/conf/snapshot_conf.c | 33 + 1 file changed, 13 insertions(+), 20 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH 04/13] qemuSnapshotCreateInactiveExternal: Don't access 'idx' of snapshot
On a Wednesday in 2020, Peter Krempa wrote: After virDomainSnapshotAlignDisks is called the definitions of disks in the snapshot definition and in the domain definition are in the same order so they can be addressed using the same index. This frees up 'idx' to be removed later. Signed-off-by: Peter Krempa --- src/qemu/qemu_snapshot.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH 03/13] qemuDomainBlockRebase: Replace ternary operator with if/else
On a Wednesday in 2020, Peter Krempa wrote: Signed-off-by: Peter Krempa --- src/qemu/qemu_driver.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH 01/13] virStorageSourceNew: Abort on failure
On a Wednesday in 2020, Peter Krempa wrote: Add an abort() on the class/object allocation failures so that virStorageSourceNew() always returns a virStorageSource and remove checks from all callers. The allocation APIs in GLib and libvirt already do abort(). All that's left are usage errors. Signed-off-by: Peter Krempa --- src/conf/backup_conf.c| 7 ++ src/conf/domain_conf.c| 21 ++ src/conf/snapshot_conf.c | 7 ++ src/conf/storage_conf.c | 4 +--- src/qemu/qemu_domain.c| 21 ++ src/qemu/qemu_driver.c| 12 +++--- src/qemu/qemu_migration.c | 7 ++ src/qemu/qemu_snapshot.c | 3 +-- src/storage/storage_backend_gluster.c | 5 + src/storage/storage_backend_logical.c | 4 +--- src/storage/storage_util.c| 10 +++-- src/util/virstoragefile.c | 32 ++- tests/qemublocktest.c | 16 +++--- tests/virstoragetest.c| 5 + 14 files changed, 46 insertions(+), 108 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH 02/13] virStorageVolDefParseXML: Use g_steal_pointer
On a Wednesday in 2020, Peter Krempa wrote: Signed-off-by: Peter Krempa --- src/conf/storage_conf.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [libvirt PATCHv3] tests: build SELinux tests (glib chronicles?)
I presume you added the part in the parentheses just for mailing list submission. On Wed, Sep 23, 2020 at 17:05:05 +0200, Ján Tomko wrote: > We set WITH_LIBATTR in meson.build, not WITH_ATTR. Linking fix not mentioned. > Signed-off-by: Ján Tomko > Fixes: 3ace72965c3b11fc763f781ae7ce3ca29dd36507 > --- > tests/meson.build | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) Reviewed-by: Peter Krempa
[libvirt PATCHv3] tests: build SELinux tests (glib chronicles?)
We set WITH_LIBATTR in meson.build, not WITH_ATTR.
Signed-off-by: Ján Tomko
Fixes: 3ace72965c3b11fc763f781ae7ce3ca29dd36507
---
tests/meson.build | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tests/meson.build b/tests/meson.build
index 31e8d66c7a..cb720f3afe 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -477,7 +477,7 @@ if conf.has('WITH_REMOTE')
endif
if conf.has('WITH_SECDRIVER_SELINUX')
- if conf.has('WITH_ATTR')
+ if conf.has('WITH_LIBATTR')
tests += [
{ 'name': 'securityselinuxtest' },
{ 'name': 'viridentitytest' },
@@ -485,7 +485,7 @@ if conf.has('WITH_SECDRIVER_SELINUX')
if conf.has('WITH_QEMU')
tests += [
-{ 'name': 'securityselinuxlabeltest', 'link_whole': [
test_utils_qemu_lib ] },
+{ 'name': 'securityselinuxlabeltest', 'link_with': [
test_qemu_driver_lib ], 'link_whole': [ test_utils_qemu_lib ] },
]
endif
endif
--
2.26.2
Re: [libvirt PATCH] util: do not unref event thread after joining it
On Wed, Sep 23, 2020 at 16:39:13 +0200, Ján Tomko wrote: > ==295055== Invalid read of size 4 ... g_thread_join() eats a reference Reviewed-by: Peter Krempa
Re: [libvirt PATCH] virnetdaemon: fix memory leak in virNetDaemonCallInhibit
On Wed, Sep 23, 2020 at 16:37:48 +0200, Pavel Hrdina wrote: > g_variant_new() returns a weak reference which can be consumed by passing > to other g_variant* functions or to g_dbus_connection_call* functions. > > This make it possible to call g_variant_new() directly as argument to > the functions above. Because this might be confusing I explicitly call > g_variant_ref_sink() to make it normal reference in both > virGDBusCallMethod() and virGDBusCallMethodWithFD() so the caller is > always responsible for the data. > > Reported-by: Peter Krempa > Signed-off-by: Pavel Hrdina > --- Reviewed-by: Peter Krempa
[libvirt PATCH] util: do not unref event thread after joining it
==295055== Invalid read of size 4
==295055==at 0x4DA4AE4: g_thread_unref (in
/usr/lib64/libglib-2.0.so.0.6400.5)
==295055==by 0x491D5FA: vir_event_thread_finalize (vireventthread.c:47)
==295055==by 0x4E6BCFF: g_object_unref (in
/usr/lib64/libgobject-2.0.so.0.6400.5)
==295055==by 0x22F35CF4: qemuProcessQMPFree (qemu_process.c:8525)
==295055==by 0x22E71B58: glib_autoptr_clear_qemuProcessQMP
(qemu_process.h:237)
...
==295055==by 0x22E98A29: qemuDomainPostParseDataAlloc (qemu_domain.c:5476)
==295055==by 0x49ABF83: virDomainDefPostParse (domain_conf.c:6023)
==295055== Address 0x2acb1c68 is 24 bytes inside a block of size 88 free'd
==295055==at 0x483B9F5: free (vg_replace_malloc.c:538)
==295055==by 0x4D80A4C: g_free (in /usr/lib64/libglib-2.0.so.0.6400.5)
...
==295055==by 0x491D5F1: vir_event_thread_finalize (vireventthread.c:46)
==295055==by 0x4E6BCFF: g_object_unref (in
/usr/lib64/libgobject-2.0.so.0.6400.5)
==295055==by 0x22F35CF4: qemuProcessQMPFree (qemu_process.c:8525)
==295055==by 0x22E71B58: glib_autoptr_clear_qemuProcessQMP
(qemu_process.h:237)
...
==295055== Block was alloc'd at
==295055==at 0x483A809: malloc (vg_replace_malloc.c:307)
==295055==by 0x4D80958: g_malloc (in /usr/lib64/libglib-2.0.so.0.6400.5)
...
==295055==by 0x4DA4C32: g_thread_try_new (in
/usr/lib64/libglib-2.0.so.0.6400.5)
==295055==by 0x491D3BC: virEventThreadStart (vireventthread.c:159)
==295055==by 0x491D3BC: virEventThreadNew (vireventthread.c:185)
...
Signed-off-by: Ján Tomko
Fixes: f4fc3db9204407874181117085756c9ced78adad
---
src/util/vireventthread.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/src/util/vireventthread.c b/src/util/vireventthread.c
index 1bca2aa57a..8342f420f6 100644
--- a/src/util/vireventthread.c
+++ b/src/util/vireventthread.c
@@ -44,7 +44,6 @@ vir_event_thread_finalize(GObject *object)
if (evt->thread) {
g_main_loop_quit(evt->loop);
g_thread_join(evt->thread);
-g_thread_unref(evt->thread);
}
g_main_loop_unref(evt->loop);
--
2.26.2
Re: [libvirt PATCH] virnetdaemon: fix memory leak in virNetDaemonCallInhibit
On a Wednesday in 2020, Pavel Hrdina wrote: g_variant_new() returns a weak reference which can be consumed by passing to other g_variant* functions or to g_dbus_connection_call* functions. This make it possible to call g_variant_new() directly as argument to the functions above. Because this might be confusing I explicitly call g_variant_ref_sink() to make it normal reference in both virGDBusCallMethod() and virGDBusCallMethodWithFD() so the caller is always responsible for the data. Reported-by: Peter Krempa Signed-off-by: Pavel Hrdina --- src/rpc/virnetdaemon.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
[libvirt PATCH] virnetdaemon: fix memory leak in virNetDaemonCallInhibit
g_variant_new() returns a weak reference which can be consumed by passing
to other g_variant* functions or to g_dbus_connection_call* functions.
This make it possible to call g_variant_new() directly as argument to
the functions above. Because this might be confusing I explicitly call
g_variant_ref_sink() to make it normal reference in both
virGDBusCallMethod() and virGDBusCallMethodWithFD() so the caller is
always responsible for the data.
Reported-by: Peter Krempa
Signed-off-by: Pavel Hrdina
---
src/rpc/virnetdaemon.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/rpc/virnetdaemon.c b/src/rpc/virnetdaemon.c
index f3a5e9f75c..2e01244f74 100644
--- a/src/rpc/virnetdaemon.c
+++ b/src/rpc/virnetdaemon.c
@@ -469,7 +469,7 @@ virNetDaemonCallInhibit(virNetDaemonPtr dmn,
{
g_autoptr(GVariant) reply = NULL;
g_autoptr(GUnixFDList) replyFD = NULL;
-GVariant *message = NULL;
+g_autoptr(GVariant) message = NULL;
GDBusConnection *systemBus;
int fd;
int rc;
--
2.26.2
Re: [libvirt PATCH] util/virgdbus: fix memory leak in virGDBusIsServiceInList
On Wed, Sep 23, 2020 at 15:57:00 +0200, Pavel Hrdina wrote: > g_variant_iter_loop() handles freeing all arguments unless we break out > of the loop, in that case we have to free them manually. > > Reported-by: Peter Krempa > Signed-off-by: Pavel Hrdina > --- > src/util/virgdbus.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) Reviewed-by: Peter Krempa
Re: [libvirt PATCH v2] qemu: substitute missing model name for host-passthrough
On Wed, Sep 23, 2020 at 09:26:58 +0200, Tim Wiederhake wrote:
> From: Collin Walling
>
> Before:
> $ uname -m
> s390x
> $ cat passthrough-cpu.xml
>
> $ virsh hypervisor-cpu-compare passthrough-cpu.xml
> error: Failed to compare hypervisor CPU with passthrough-cpu.xml
> error: internal error: unable to execute QEMU command 'query-cpu-model-comp
> arison': Invalid parameter type for 'modelb.name', expected: string
>
> After:
> $ virsh hypervisor-cpu-compare passthrough-cpu.xml
> CPU described in passthrough-cpu.xml is identical to the CPU provided by hy
> pervisor on the host
>
> Signed-off-by: Tim Wiederhake
> ---
> src/qemu/qemu_driver.c | 9 +
> 1 file changed, 9 insertions(+)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index ae715c01d7..1cecef01f7 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -12336,6 +12336,15 @@ qemuConnectCompareHypervisorCPU(virConnectPtr conn,
> if (virCPUDefParseXMLString(xmlCPU, VIR_CPU_TYPE_AUTO, ) < 0)
> goto cleanup;
>
> +if (!cpu->model) {
> +if (cpu->mode == VIR_CPU_MODE_HOST_PASSTHROUGH) {
> +cpu->model = g_strdup("host");
> +} else {
> +virReportError(VIR_ERR_INVALID_ARG, "%s",
> + _("cpu parameter is missing a model name"));
> +goto cleanup;
> +}
> +}
> ret = qemuConnectCPUModelComparison(qemuCaps, cfg->libDir,
> cfg->user, cfg->group,
> hvCPU, cpu, failIncompatible);
Reviewed-by: Jiri Denemark
I'll wait some time for Collin to add Signed-of-by tag before pushing
this.
Re: [libvirt PATCHv2 2/2] tests: use g_new0 instead of VIR_ALLOC_N
On Wed, Sep 23, 2020 at 15:39:50 +0200, Ján Tomko wrote: > Signed-off-by: Ján Tomko > --- Reviewed-by: Peter Krempa
Re: [libvirt PATCHv2 1/2] tests: build SELinux tests
On Wed, Sep 23, 2020 at 15:39:49 +0200, Ján Tomko wrote: > We set WITH_LIBATTR in meson.build, not WITH_ATTR. > > Signed-off-by: Ján Tomko > Fixes: 3ace72965c3b11fc763f781ae7ce3ca29dd36507 > --- > tests/meson.build | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) I got: [1010/1047] Linking target tests/securityselinuxlabeltest FAILED: tests/securityselinuxlabeltest cc -o tests/securityselinuxlabeltest src/libvirt_probes.o tests/securityselinuxlabeltest.p/securityselinuxlabeltest.c.o -Wl,--as-needed -Wl,--no-undefined -Wl,-export-dynamic -pie -Wl,--whole-archive -Wl,--start-group tests/libtest_utils.a tests/libtest_utils_qemu.a -Wl,--no-whole-archive src/libvirt.so.0.6008.0 -Wl,--no-copy-dt-needed-entries -Wl,-export-dynamic -ldl /usr/lib64/libglib-2.0.so /usr/lib64/libgobject-2.0.so /usr/lib64/libgio-2.0.so /usr/lib64/libgnutls.so /usr/lib64/libnl-3.so /usr/lib64/libnl-route-3.so /usr/lib64/libxml2.so /usr/lib64/libsasl2.so -lselinux /usr/lib64/libtirpc.so /usr/lib64/libyajl.so -Wl,-export-dynamic -lselinux -Wl,-export-dynamic -lselinux -Wl,--end-group '-Wl,-rpath,$ORIGIN/../src' -Wl,-rpath-link,/home/pipo/build/libvirt/gcc/src /bin/ld: tests/securityselinuxlabeltest.p/securityselinuxlabeltest.c.o: in function `mymain': /home/pipo/build/libvirt/gcc/../../../libvirt/tests/securityselinuxlabeltest.c:349: undefined reference to `virQEMUCapsNew' /bin/ld: /home/pipo/build/libvirt/gcc/../../../libvirt/tests/securityselinuxlabeltest.c:352: undefined reference to `virQEMUCapsSet' /bin/ld: /home/pipo/build/libvirt/gcc/../../../libvirt/tests/securityselinuxlabeltest.c:353: undefined reference to `virQEMUCapsSet' /bin/ld: tests/libtest_utils_qemu.a(testutilsqemu.c.o): in function `qemuTestParseCapabilitiesArch': /home/pipo/build/libvirt/gcc/../../../libvirt/tests/testutilsqemu.c:292: undefined reference to `virQEMUCapsNewBinary' /bin/ld: /home/pipo/build/libvirt/gcc/../../../libvirt/tests/testutilsqemu.c:293: undefined reference to `virQEMUCapsLoadCache' /bin/ld: tests/libtest_utils_qemu.a(testutilsqemu.c.o): in function `qemuTestCapsCacheInsert': /home/pipo/build/libvirt/gcc/../../../libvirt/tests/testutilsqemu.c:328: undefined reference to `virQEMUCapsNewCopy' /bin/ld: /home/pipo/build/libvirt/gcc/../../../libvirt/tests/testutilsqemu.c:333: undefined reference to `virQEMUCapsHasMachines' /bin/ld: /home/pipo/build/libvirt/gcc/../../../libvirt/tests/testutilsqemu.c:330: undefined reference to `virQEMUCapsNew' after applying this patch.
[libvirt PATCH] util/virgdbus: fix memory leak in virGDBusIsServiceInList
g_variant_iter_loop() handles freeing all arguments unless we break out
of the loop, in that case we have to free them manually.
Reported-by: Peter Krempa
Signed-off-by: Pavel Hrdina
---
src/util/virgdbus.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/util/virgdbus.c b/src/util/virgdbus.c
index cd9ca8d5d6..4360a6acff 100644
--- a/src/util/virgdbus.c
+++ b/src/util/virgdbus.c
@@ -359,8 +359,10 @@ virGDBusIsServiceInList(const char *listMethod,
g_variant_get(reply, "(as)", );
while (g_variant_iter_loop(iter, "s", )) {
-if (STREQ(str, name))
+if (STREQ(str, name)) {
+g_free(str);
return 0;
+}
}
return -2;
--
2.26.2
Re: [PATCH v2 2/2] block: deprecate the sheepdog block driver
Daniel P. Berrangé writes:
> This thread from a little over a year ago:
>
> http://lists.wpkg.org/pipermail/sheepdog/2019-March/thread.html
>
> states that sheepdog is no longer actively developed. The only mentioned
> users are some companies who are said to have it for legacy reasons with
> plans to replace it by Ceph. There is talk about cutting out existing
> features to turn it into a simple demo of how to write a distributed
> block service. There is no evidence of anyone working on that idea:
>
> https://github.com/sheepdog/sheepdog/commits/master
>
> No real commits to git since Jan 2018, and before then just some minor
> technical debt cleanup..
Drop the extra period.
>
> There is essentially no activity on the mailing list aside from
> patches to QEMU that get CC'd due to our MAINTAINERS entry.
>
> Fedora packages for sheepdog failed to build from upstream source
> because of the more strict linker that no longer merges duplicate
> global symbols. Fedora patches it to add the missing "extern"
> annotations and presumably other distros do to, but upstream source
> remains broken.
>
> There is only basic compile testing, no functional testing of the
> driver.
>
> Since there are no build pre-requisites the sheepdog driver is currently
> enabled unconditionally. This would result in configure issuing a
> deprecation warning by default for all users. Thus the configure default
> is changed to disable it, requiring users to pass --enable-sheepdog to
> build the driver.
>
> Signed-off-by: Daniel P. Berrangé
> ---
> block/sheepdog.c | 15 +++
> configure | 5 +++--
> docs/system/deprecated.rst | 9 +
> 3 files changed, 27 insertions(+), 2 deletions(-)
>
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index cbbebc1aaf..7f68bd6a1a 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -242,6 +242,17 @@ typedef struct SheepdogInode {
> */
> #define FNV1A_64_INIT ((uint64_t)0xcbf29ce484222325ULL)
>
> +static void deprecation_warning(void)
> +{
> +static bool warned = false;
Obey checkpatch :)
> +
> +if (!warned) {
> +warn_report("the sheepdog block driver is deprecated and will be "
> +"removed in a future release");
Similar warnings elsewhere don't say "will be removed".
Some of them are nice enough to advise what to use instead, but that may
not be practical here.
> +warned = true;
> +}
> +}
> +
> /*
> * 64 bit Fowler/Noll/Vo FNV-1a hash code
> */
> @@ -1548,6 +1559,8 @@ static int sd_open(BlockDriverState *bs, QDict
> *options, int flags,
> char *buf = NULL;
> QemuOpts *opts;
>
> +deprecation_warning();
> +
> s->bs = bs;
> s->aio_context = bdrv_get_aio_context(bs);
>
> @@ -2007,6 +2020,8 @@ static int sd_co_create(BlockdevCreateOptions *options,
> Error **errp)
>
> assert(options->driver == BLOCKDEV_DRIVER_SHEEPDOG);
>
> +deprecation_warning();
> +
> s = g_new0(BDRVSheepdogState, 1);
>
> /* Steal SocketAddress from QAPI, set NULL to prevent double free */
> diff --git a/configure b/configure
> index 7564479008..c6af83f2e6 100755
> --- a/configure
> +++ b/configure
> @@ -533,7 +533,7 @@ vdi="yes"
> vvfat="yes"
> qed="yes"
> parallels="yes"
> -sheepdog="yes"
> +sheepdog="no"
> libxml2=""
> debug_mutex="no"
> libpmem=""
> @@ -1941,7 +1941,7 @@ disabled with --disable-FEATURE, default is enabled if
> available:
>vvfat vvfat image format support
>qed qed image format support
>parallels parallels image format support
> - sheepdogsheepdog block driver support
> + sheepdogsheepdog block driver support (deprecated)
>crypto-afalgLinux AF_ALG crypto backend driver
>capstonecapstone disassembler support
>debug-mutex mutex debugging support
> @@ -7350,6 +7350,7 @@ if test "$parallels" = "yes" ; then
>echo "CONFIG_PARALLELS=y" >> $config_host_mak
> fi
> if test "$sheepdog" = "yes" ; then
> + add_to deprecated_features "sheepdog"
>echo "CONFIG_SHEEPDOG=y" >> $config_host_mak
> fi
> if test "$pty_h" = "yes" ; then
> diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
> index 0cb8b01424..49b9f4b02e 100644
> --- a/docs/system/deprecated.rst
> +++ b/docs/system/deprecated.rst
> @@ -405,6 +405,15 @@ The above, converted to the current supported format::
>
>json:{"file.driver":"rbd", "file.pool":"rbd", "file.image":"name"}
>
> +``sheepdog`` driver (since 5.2.0)
> +^
> +
> +The ``sheepdog`` block device driver is deprecated. The corresponding
> upstream
> +server project is no longer actively maintained. Users are recommended to
> switch
> +to an alternative distributed block device driver such as RBD. The
> +``qemu-img convert`` command can be used to liberate existing data by moving
> +it out of sheepdog volumes into an alternative storage backend.
> +
> linux-user mode CPUs
Re: [PATCH] apparmor: Allow /usr/libexec for libxl-save-helper and pygrub
On 9/23/20 7:26 AM, Christian Ehrhardt wrote: On Wed, Sep 23, 2020 at 12:35 AM Jim Fehlig wrote: Like other distros, openSUSE Tumbleweed recently changed libexecdir from /usr/lib to /usr/libexec. Add it as an allowed path for libxl-save-helper and pygrub. Hi Jim, ack to the intention, but I think since this should use @libexecdir@ I think. Or did anything change that this doesn't apply anymore ... in that case I beg your pardon. [1]: https://libvirt.org/git/?p=libvirt.git;a=commit;h=5c8bd31c881e99261ac098e867a79b300440731a Heh, I see that skipped over the xen stuff :-). I'll send a V2 later. Regards, Jim
Re: [PATCH v1] qemu: monitor: substitute missing model name for host-passthrough
Collin, I apologize for not getting to you earlier. On Wed, Sep 16, 2020 at 12:11:08 -0400, Collin Walling wrote: > On 9/16/20 3:03 AM, Michal Privoznik wrote: > > On 9/15/20 10:25 PM, Collin Walling wrote: > >> One more ping in attempt to get this in the right direction. Otherwise > >> I'll post my next idea and we can go from there :) > > > > I agree with Peter that while the idea might look correct it's too deep. > > > >> > >> Thinking about this issue, should a host-passthough CPU definition be > >> permitted for the baseline & comparison commands? The model represented > >> under this mode is not considered migration safe and it may make sense > >> to simply fail early since these commands aim to construct/determine a > >> migratable CPU model, respectively. > > > > Honestly, I don't know much about this CPU models area, but is that true > > even for two identical hosts? Say I have two desktops next to each > > other, with the same CPU and I want to migrate. I could use host model, > > couldn't I? > > > > "Host-model" is an alias for a CPU model that closely represents the > capabilities of the host machine (on s390, because this model is defined > by the hypervisor, it can also be called the "hypervisor CPU model" -- > not an important detail). > > However, a guest running with the host-passthrough mode is not > considered migration safe as that guest may covertly run with > features/capabilities that are not directly exposed to the hypervisor. Right, but migration may still be possible and working fine if both host are identical. > From what I understand regarding the hypervisor-cpu-compare and > hypervisor-cpu-baseline commands is that they aim to assist with > determining the migratability of guests based on their CPU model and > feature set (usually along with a host CPU in the equation as well). Baseline with a host-passthrough CPU is not indeed very useful, but compare could still be used and its usage is not limited to migration. For example, you can use it to check whether a domain with a guest CPU configuration can be started on a specific host before you actually try to start it. And reporting host-passthrough as incompatible would be wrong. Anyway, thanks for your patch, it was mostly correct, it just needed to be done a bit higher in the call graph. Incidentally, Tim Wiederhake [1] took this original patch and moved the change to the right place. The authorship is still yours, so if you want to append you signed-off-by tag there, I'll wait a bit before pushing Tim's patch. Jirka [1] https://www.redhat.com/archives/libvir-list/2020-September/msg01177.html
[libvirt PATCHv2 1/2] tests: build SELinux tests
We set WITH_LIBATTR in meson.build, not WITH_ATTR.
Signed-off-by: Ján Tomko
Fixes: 3ace72965c3b11fc763f781ae7ce3ca29dd36507
---
tests/meson.build | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/meson.build b/tests/meson.build
index 31e8d66c7a..eaf6628fb9 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -477,7 +477,7 @@ if conf.has('WITH_REMOTE')
endif
if conf.has('WITH_SECDRIVER_SELINUX')
- if conf.has('WITH_ATTR')
+ if conf.has('WITH_LIBATTR')
tests += [
{ 'name': 'securityselinuxtest' },
{ 'name': 'viridentitytest' },
--
2.26.2
[libvirt PATCHv2 0/2] tests: switch to g_new0 (glib chronicles)
This time actually compile-tested. Ján Tomko (2): tests: build SELinux tests tests: use g_new0 instead of VIR_ALLOC_N tests/commandtest.c | 7 +++ tests/domaincapstest.c | 3 +-- tests/fdstreamtest.c | 10 -- tests/meson.build| 2 +- tests/qemuxml2argvtest.c | 3 +-- tests/securityselinuxlabeltest.c | 3 +-- tests/securityselinuxtest.c | 3 +-- tests/testutils.c| 8 ++-- tests/testutilsqemu.c| 6 ++ tests/vircgrouptest.c| 3 +-- tests/vircryptotest.c| 5 ++--- tests/virhostcputest.c | 3 +-- tests/virnetmessagetest.c| 6 ++ tests/virnetsockettest.c | 6 +- tests/virnettlshelpers.c | 3 +-- tests/virstringtest.c| 3 +-- tests/xlconfigtest.c | 3 +-- tests/xmconfigtest.c | 3 +-- 18 files changed, 27 insertions(+), 53 deletions(-) -- 2.26.2
[libvirt PATCHv2 2/2] tests: use g_new0 instead of VIR_ALLOC_N
Signed-off-by: Ján Tomko
---
tests/commandtest.c | 7 +++
tests/domaincapstest.c | 3 +--
tests/fdstreamtest.c | 10 --
tests/qemuxml2argvtest.c | 3 +--
tests/securityselinuxlabeltest.c | 3 +--
tests/securityselinuxtest.c | 3 +--
tests/testutils.c| 8 ++--
tests/testutilsqemu.c| 6 ++
tests/vircgrouptest.c| 3 +--
tests/vircryptotest.c| 5 ++---
tests/virhostcputest.c | 3 +--
tests/virnetmessagetest.c| 6 ++
tests/virnetsockettest.c | 6 +-
tests/virnettlshelpers.c | 3 +--
tests/virstringtest.c| 3 +--
tests/xlconfigtest.c | 3 +--
tests/xmconfigtest.c | 3 +--
17 files changed, 26 insertions(+), 52 deletions(-)
diff --git a/tests/commandtest.c b/tests/commandtest.c
index cbbcda4e5f..df86725f0e 100644
--- a/tests/commandtest.c
+++ b/tests/commandtest.c
@@ -1057,10 +1057,9 @@ static int test27(const void *unused G_GNUC_UNUSED)
"%s%s%s" \
"END STDERR\n"
-if (VIR_ALLOC_N(buffer0, buflen) < 0 ||
-VIR_ALLOC_N(buffer1, buflen) < 0 ||
-VIR_ALLOC_N(buffer2, buflen) < 0)
-goto cleanup;
+buffer0 = g_new0(char, buflen);
+buffer1 = g_new0(char, buflen);
+buffer2 = g_new0(char, buflen);
memset(buffer0, 'H', buflen - 2);
buffer0[buflen - 2] = '\n';
diff --git a/tests/domaincapstest.c b/tests/domaincapstest.c
index f817ed5452..7a082705c6 100644
--- a/tests/domaincapstest.c
+++ b/tests/domaincapstest.c
@@ -147,8 +147,7 @@ fillXenCaps(virDomainCapsPtr domCaps)
virFirmwarePtr *firmwares;
int ret = -1;
-if (VIR_ALLOC_N(firmwares, 2) < 0)
-return ret;
+firmwares = g_new0(virFirmwarePtr, 2);
firmwares[0] = g_new0(virFirmware, 1);
firmwares[1] = g_new0(virFirmware, 1);
diff --git a/tests/fdstreamtest.c b/tests/fdstreamtest.c
index 83973137e7..7a2fe27477 100644
--- a/tests/fdstreamtest.c
+++ b/tests/fdstreamtest.c
@@ -54,9 +54,8 @@ static int testFDStreamReadCommon(const char *scratchdir,
bool blocking)
if (!(conn = virConnectOpen("test:///default")))
goto cleanup;
-if (VIR_ALLOC_N(pattern, PATTERN_LEN) < 0 ||
-VIR_ALLOC_N(buf, PATTERN_LEN) < 0)
-goto cleanup;
+pattern = g_new0(char, PATTERN_LEN);
+buf = g_new0(char, PATTERN_LEN);
for (i = 0; i < PATTERN_LEN; i++)
pattern[i] = i;
@@ -185,9 +184,8 @@ static int testFDStreamWriteCommon(const char *scratchdir,
bool blocking)
if (!(conn = virConnectOpen("test:///default")))
goto cleanup;
-if (VIR_ALLOC_N(pattern, PATTERN_LEN) < 0 ||
-VIR_ALLOC_N(buf, PATTERN_LEN) < 0)
-goto cleanup;
+pattern = g_new0(char, PATTERN_LEN);
+buf = g_new0(char, PATTERN_LEN);
for (i = 0; i < PATTERN_LEN; i++)
pattern[i] = i;
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index e93948e3fc..463e4c003d 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -753,8 +753,7 @@ mymain(void)
driver.config->nbdTLSx509certdir =
g_strdup("/etc/pki/libvirt-nbd/dummy,path");
VIR_FREE(driver.config->hugetlbfs);
-if (VIR_ALLOC_N(driver.config->hugetlbfs, 2) < 0)
-return EXIT_FAILURE;
+driver.config->hugetlbfs = g_new0(virHugeTLBFS, 2);
driver.config->nhugetlbfs = 2;
driver.config->hugetlbfs[0].mnt_dir = g_strdup("/dev/hugepages2M");
driver.config->hugetlbfs[1].mnt_dir = g_strdup("/dev/hugepages1G");
diff --git a/tests/securityselinuxlabeltest.c b/tests/securityselinuxlabeltest.c
index 2989a668b7..168acc2bdf 100644
--- a/tests/securityselinuxlabeltest.c
+++ b/tests/securityselinuxlabeltest.c
@@ -114,8 +114,7 @@ testSELinuxLoadFileList(const char *testname,
if (!(fp = fopen(path, "r")))
goto cleanup;
-if (VIR_ALLOC_N(line, 1024) < 0)
-goto cleanup;
+line = g_new0(char, 1024);
while (!feof(fp)) {
char *file = NULL, *context = NULL, *tmp;
diff --git a/tests/securityselinuxtest.c b/tests/securityselinuxtest.c
index 297cc0e53d..a75ff495eb 100644
--- a/tests/securityselinuxtest.c
+++ b/tests/securityselinuxtest.c
@@ -71,8 +71,7 @@ testBuildDomainDef(bool dynamic,
goto error;
def->virtType = VIR_DOMAIN_VIRT_KVM;
-if (VIR_ALLOC_N(def->seclabels, 1) < 0)
-goto error;
+def->seclabels = g_new0(virSecurityLabelDefPtr, 1);
secdef = g_new0(virSecurityLabelDef, 1);
diff --git a/tests/testutils.c b/tests/testutils.c
index 3f53f635fc..b747f65eea 100644
--- a/tests/testutils.c
+++ b/tests/testutils.c
@@ -211,10 +211,7 @@ virTestLoadFile(const char *file, char **buf)
tmplen = buflen = st.st_size + 1;
-if (VIR_ALLOC_N(*buf, buflen) < 0) {
-VIR_FORCE_FCLOSE(fp);
-return -1;
-}
+*buf = g_new0(char, buflen);
tmp = *buf;
(*buf)[0] = '\0';
@@ -977,8 +974,7 @@
[PATCH 09/13] virDomainSnapshotAlignDisks: Extract domain disk definition to a local variable
Extract the disk def to a local variable so that it's more obvious
what's happening and it will also allow further simplification.
Signed-off-by: Peter Krempa
---
src/conf/snapshot_conf.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index a1cee01944..aeebe2fb33 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -677,6 +677,7 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr snapdef,
for (i = 0; i < snapdef->ndisks; i++) {
virDomainSnapshotDiskDefPtr snapdisk = >disks[i];
int idx = virDomainDiskIndexByName(domdef, snapdisk->name, false);
+virDomainDiskDefPtr domdisk = NULL;
int disk_snapshot;
if (idx < 0) {
@@ -685,6 +686,8 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr snapdef,
return -1;
}
+domdisk = domdef->disks[idx];
+
if (virBitmapIsBitSet(map, idx)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("disk '%s' specified twice"),
@@ -694,7 +697,7 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr snapdef,
ignore_value(virBitmapSetBit(map, idx));
snapdisk->idx = idx;
-disk_snapshot = domdef->disks[idx]->snapshot;
+disk_snapshot = domdisk->snapshot;
if (!snapdisk->snapshot) {
if (disk_snapshot &&
(!require_match ||
@@ -722,9 +725,9 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr snapdef,
snapdisk->src->path, snapdisk->name);
return -1;
}
-if (STRNEQ(snapdisk->name, domdef->disks[idx]->dst)) {
+if (STRNEQ(snapdisk->name, domdisk->dst)) {
VIR_FREE(snapdisk->name);
-snapdisk->name = g_strdup(domdef->disks[idx]->dst);
+snapdisk->name = g_strdup(domdisk->dst);
}
}
--
2.26.2
[PATCH 10/13] virDomainSnapshotAlignDisks: remove unnecessary 'tmp' variable
The converted string is used exactly once so we can call the conversion
without storing the result in a variable.
Signed-off-by: Peter Krempa
---
src/conf/snapshot_conf.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index aeebe2fb33..10eb584a1c 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -709,12 +709,10 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr
snapdef,
snapdisk->snapshot != default_snapshot &&
!(snapdisk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE &&
disk_snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE)) {
-const char *tmp;
-
-tmp = virDomainSnapshotLocationTypeToString(default_snapshot);
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("disk '%s' must use snapshot mode '%s'"),
- snapdisk->name, tmp);
+ snapdisk->name,
+
virDomainSnapshotLocationTypeToString(default_snapshot));
return -1;
}
if (snapdisk->src->path &&
--
2.26.2
[PATCH 03/13] qemuDomainBlockRebase: Replace ternary operator with if/else
Signed-off-by: Peter Krempa --- src/qemu/qemu_driver.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9b5ff3a65c..40d11aced6 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15292,8 +15292,10 @@ qemuDomainBlockRebase(virDomainPtr dom, const char *path, const char *base, /* If we got here, we are doing a block copy rebase. */ dest = virStorageSourceNew(); -dest->type = (flags & VIR_DOMAIN_BLOCK_REBASE_COPY_DEV) ? -VIR_STORAGE_TYPE_BLOCK : VIR_STORAGE_TYPE_FILE; +if (flags & VIR_DOMAIN_BLOCK_REBASE_COPY_DEV) +dest->type = VIR_STORAGE_TYPE_BLOCK; +else +dest->type = VIR_STORAGE_TYPE_FILE; dest->path = g_strdup(base); if (flags & VIR_DOMAIN_BLOCK_REBASE_COPY_RAW) dest->format = VIR_STORAGE_FILE_RAW; -- 2.26.2
[PATCH 07/13] virDomainSnapshotAlignDisks: Rename 'disk' -> 'snapdisk'
The 'disk' variable usually refers to a definition of a disk from the
domain definition. Rename it to 'snapdisk' to be clear that we are
talking about the snapshot disk definition especially since this
function also accesses the domain disk definition.
Signed-off-by: Peter Krempa
---
src/conf/snapshot_conf.c | 54
1 file changed, 27 insertions(+), 27 deletions(-)
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index 77f53c4a1d..c835ec7333 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -674,56 +674,56 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr
snapdef,
/* Double check requested disks. */
for (i = 0; i < snapdef->ndisks; i++) {
-virDomainSnapshotDiskDefPtr disk = >disks[i];
-int idx = virDomainDiskIndexByName(snapdef->parent.dom, disk->name,
false);
+virDomainSnapshotDiskDefPtr snapdisk = >disks[i];
+int idx = virDomainDiskIndexByName(snapdef->parent.dom,
snapdisk->name, false);
int disk_snapshot;
if (idx < 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("no disk named '%s'"), disk->name);
+ _("no disk named '%s'"), snapdisk->name);
return -1;
}
if (virBitmapIsBitSet(map, idx)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("disk '%s' specified twice"),
- disk->name);
+ snapdisk->name);
return -1;
}
ignore_value(virBitmapSetBit(map, idx));
-disk->idx = idx;
+snapdisk->idx = idx;
disk_snapshot = snapdef->parent.dom->disks[idx]->snapshot;
-if (!disk->snapshot) {
+if (!snapdisk->snapshot) {
if (disk_snapshot &&
(!require_match ||
disk_snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE))
-disk->snapshot = disk_snapshot;
+snapdisk->snapshot = disk_snapshot;
else
-disk->snapshot = default_snapshot;
+snapdisk->snapshot = default_snapshot;
} else if (require_match &&
- disk->snapshot != default_snapshot &&
- !(disk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE &&
+ snapdisk->snapshot != default_snapshot &&
+ !(snapdisk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE &&
disk_snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE)) {
const char *tmp;
tmp = virDomainSnapshotLocationTypeToString(default_snapshot);
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("disk '%s' must use snapshot mode '%s'"),
- disk->name, tmp);
+ snapdisk->name, tmp);
return -1;
}
-if (disk->src->path &&
-disk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) {
+if (snapdisk->src->path &&
+snapdisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("file '%s' for disk '%s' requires "
"use of external snapshot mode"),
- disk->src->path, disk->name);
+ snapdisk->src->path, snapdisk->name);
return -1;
}
-if (STRNEQ(disk->name, snapdef->parent.dom->disks[idx]->dst)) {
-VIR_FREE(disk->name);
-disk->name = g_strdup(snapdef->parent.dom->disks[idx]->dst);
+if (STRNEQ(snapdisk->name, snapdef->parent.dom->disks[idx]->dst)) {
+VIR_FREE(snapdisk->name);
+snapdisk->name = g_strdup(snapdef->parent.dom->disks[idx]->dst);
}
}
@@ -734,24 +734,24 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr
snapdef,
return -1;
for (i = 0; i < snapdef->parent.dom->ndisks; i++) {
-virDomainSnapshotDiskDefPtr disk;
+virDomainSnapshotDiskDefPtr snapdisk;
if (virBitmapIsBitSet(map, i))
continue;
-disk = >disks[ndisks++];
-disk->src = virStorageSourceNew();
-disk->name = g_strdup(snapdef->parent.dom->disks[i]->dst);
-disk->idx = i;
+snapdisk = >disks[ndisks++];
+snapdisk->src = virStorageSourceNew();
+snapdisk->name = g_strdup(snapdef->parent.dom->disks[i]->dst);
+snapdisk->idx = i;
/* Don't snapshot empty drives */
if (virStorageSourceIsEmpty(snapdef->parent.dom->disks[i]->src))
-disk->snapshot = VIR_DOMAIN_SNAPSHOT_LOCATION_NONE;
+snapdisk->snapshot = VIR_DOMAIN_SNAPSHOT_LOCATION_NONE;
else
-disk->snapshot = snapdef->parent.dom->disks[i]->snapshot;
+snapdisk->snapshot =
[PATCH 12/13] virDomainSnapshotAlignDisks: refactor extension to all disks
Last step of the algorithm in virDomainSnapshotAlignDisks is to extend
the array of disks to all VM's disk and provide defaults. This was done
by extending the array, adding defaults at the end and then sorting it.
This requires the 'idx' variable and also a separate sorting function.
If we store the pointer to existing snapshot disk definitions in a hash
table and create a new array of snapshot disk definitions, we can fill
the new array directly by either copying the definition from the old
array or adding the default.
This avoids the sorting step and thus even the need to store the index
of the domain disk altogether.
Signed-off-by: Peter Krempa
---
src/conf/snapshot_conf.c | 48 +++-
1 file changed, 18 insertions(+), 30 deletions(-)
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index f6a827d2ff..7090e3a1b0 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -627,16 +627,6 @@
virDomainSnapshotDefAssignExternalNames(virDomainSnapshotDefPtr def)
}
-static int
-virDomainSnapshotCompareDiskIndex(const void *a, const void *b)
-{
-const virDomainSnapshotDiskDef *diska = a;
-const virDomainSnapshotDiskDef *diskb = b;
-
-/* Integer overflow shouldn't be a problem here. */
-return diska->idx - diskb->idx;
-}
-
/* Align def->disks to def->parent.dom. Sort the list of def->disks,
* filling in any missing disks or snapshot state defaults given by
* the domain, with a fallback to a passed in default. Convert paths
@@ -650,9 +640,9 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr snapdef,
bool require_match)
{
virDomainDefPtr domdef = snapdef->parent.dom;
-g_autoptr(virBitmap) map = NULL;
+g_autoptr(virHashTable) map = virHashNew(NULL);
+g_autofree virDomainSnapshotDiskDefPtr olddisks = NULL;
size_t i;
-int ndisks;
if (!domdef) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
@@ -670,9 +660,6 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr snapdef,
if (!domdef->ndisks)
return 0;
-if (!(map = virBitmapNew(domdef->ndisks)))
-return -1;
-
/* Double check requested disks. */
for (i = 0; i < snapdef->ndisks; i++) {
virDomainSnapshotDiskDefPtr snapdisk = >disks[i];
@@ -687,14 +674,15 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr
snapdef,
domdisk = domdef->disks[idx];
-if (virBitmapIsBitSet(map, idx)) {
+if (virHashHasEntry(map, domdisk->dst)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("disk '%s' specified twice"),
snapdisk->name);
return -1;
}
-ignore_value(virBitmapSetBit(map, idx));
-snapdisk->idx = idx;
+
+if (virHashAddEntry(map, domdisk->dst, snapdisk) < 0)
+return -1;
if (snapdisk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_DEFAULT) {
if (domdisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_DEFAULT &&
@@ -729,21 +717,24 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr
snapdef,
}
}
-/* Provide defaults for all remaining disks. */
-ndisks = snapdef->ndisks;
-if (VIR_EXPAND_N(snapdef->disks, snapdef->ndisks,
- domdef->ndisks - snapdef->ndisks) < 0)
-return -1;
+olddisks = g_steal_pointer(>disks);
+snapdef->disks = g_new0(virDomainSnapshotDiskDef, domdef->ndisks);
+snapdef->ndisks = domdef->ndisks;
for (i = 0; i < domdef->ndisks; i++) {
-virDomainSnapshotDiskDefPtr snapdisk;
+virDomainDiskDefPtr domdisk = domdef->disks[i];
+virDomainSnapshotDiskDefPtr snapdisk = snapdef->disks + i;
+virDomainSnapshotDiskDefPtr existing;
-if (virBitmapIsBitSet(map, i))
+/* copy existing disks */
+if ((existing = virHashLookup(map, domdisk->dst))) {
+memcpy(snapdisk, existing, sizeof(*snapdisk));
continue;
-snapdisk = >disks[ndisks++];
+}
+
+/* Provide defaults for all remaining disks. */
snapdisk->src = virStorageSourceNew();
snapdisk->name = g_strdup(domdef->disks[i]->dst);
-snapdisk->idx = i;
/* Don't snapshot empty drives */
if (virStorageSourceIsEmpty(domdef->disks[i]->src))
@@ -756,9 +747,6 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr snapdef,
snapdisk->snapshot = default_snapshot;
}
-qsort(>disks[0], snapdef->ndisks, sizeof(snapdef->disks[0]),
- virDomainSnapshotCompareDiskIndex);
-
/* Generate default external file names for external snapshot locations */
if (virDomainSnapshotDefAssignExternalNames(snapdef) < 0)
return -1;
--
2.26.2
[PATCH 11/13] virDomainSnapshotAlignDisks: clarify handing of snapshot location
Remove the use of the 'disk_snapshot' temporary variable since accessing
the disk definition now isn't that much longer to write and use expicit
value checks instead of the (non-)zero check to make it more obvious
what the code is doing.
Signed-off-by: Peter Krempa
---
src/conf/snapshot_conf.c | 16
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index 10eb584a1c..f6a827d2ff 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -678,7 +678,6 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr snapdef,
virDomainSnapshotDiskDefPtr snapdisk = >disks[i];
int idx = virDomainDiskIndexByName(domdef, snapdisk->name, false);
virDomainDiskDefPtr domdisk = NULL;
-int disk_snapshot;
if (idx < 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
@@ -697,24 +696,25 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr
snapdef,
ignore_value(virBitmapSetBit(map, idx));
snapdisk->idx = idx;
-disk_snapshot = domdisk->snapshot;
-if (!snapdisk->snapshot) {
-if (disk_snapshot &&
+if (snapdisk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_DEFAULT) {
+if (domdisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_DEFAULT &&
(!require_match ||
- disk_snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE))
-snapdisk->snapshot = disk_snapshot;
-else
+ domdisk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE)) {
+snapdisk->snapshot = domdisk->snapshot;
+} else {
snapdisk->snapshot = default_snapshot;
+}
} else if (require_match &&
snapdisk->snapshot != default_snapshot &&
!(snapdisk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE &&
- disk_snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE)) {
+ domdisk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("disk '%s' must use snapshot mode '%s'"),
snapdisk->name,
virDomainSnapshotLocationTypeToString(default_snapshot));
return -1;
}
+
if (snapdisk->src->path &&
snapdisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
--
2.26.2
[PATCH 05/13] virDomainSnapshotAlignDisks: Refactor cleanup
Use automatic pointer for the bitmap and get rid of the 'cleanup' label
and 'ret' variable.
Signed-off-by: Peter Krempa
---
src/conf/snapshot_conf.c | 33 +
1 file changed, 13 insertions(+), 20 deletions(-)
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index 87a00082ef..160f2054a4 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -649,31 +649,28 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def,
int default_snapshot,
bool require_match)
{
-int ret = -1;
-virBitmapPtr map = NULL;
+g_autoptr(virBitmap) map = NULL;
size_t i;
int ndisks;
if (!def->parent.dom) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("missing domain in snapshot"));
-goto cleanup;
+return -1;
}
if (def->ndisks > def->parent.dom->ndisks) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("too many disk snapshot requests for domain"));
-goto cleanup;
+return -1;
}
/* Unlikely to have a guest without disks but technically possible. */
-if (!def->parent.dom->ndisks) {
-ret = 0;
-goto cleanup;
-}
+if (!def->parent.dom->ndisks)
+return 0;
if (!(map = virBitmapNew(def->parent.dom->ndisks)))
-goto cleanup;
+return -1;
/* Double check requested disks. */
for (i = 0; i < def->ndisks; i++) {
@@ -684,14 +681,14 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def,
if (idx < 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("no disk named '%s'"), disk->name);
-goto cleanup;
+return -1;
}
if (virBitmapIsBitSet(map, idx)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("disk '%s' specified twice"),
disk->name);
-goto cleanup;
+return -1;
}
ignore_value(virBitmapSetBit(map, idx));
disk->idx = idx;
@@ -714,7 +711,7 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def,
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("disk '%s' must use snapshot mode '%s'"),
disk->name, tmp);
-goto cleanup;
+return -1;
}
if (disk->src->path &&
disk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) {
@@ -722,7 +719,7 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def,
_("file '%s' for disk '%s' requires "
"use of external snapshot mode"),
disk->src->path, disk->name);
-goto cleanup;
+return -1;
}
if (STRNEQ(disk->name, def->parent.dom->disks[idx]->dst)) {
VIR_FREE(disk->name);
@@ -734,7 +731,7 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def,
ndisks = def->ndisks;
if (VIR_EXPAND_N(def->disks, def->ndisks,
def->parent.dom->ndisks - def->ndisks) < 0)
-goto cleanup;
+return -1;
for (i = 0; i < def->parent.dom->ndisks; i++) {
virDomainSnapshotDiskDefPtr disk;
@@ -762,13 +759,9 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def,
/* Generate default external file names for external snapshot locations */
if (virDomainSnapshotDefAssignExternalNames(def) < 0)
-goto cleanup;
-
-ret = 0;
+return -1;
- cleanup:
-virBitmapFree(map);
-return ret;
+return 0;
}
--
2.26.2
[PATCH 02/13] virStorageVolDefParseXML: Use g_steal_pointer
Signed-off-by: Peter Krempa
---
src/conf/storage_conf.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 2b00f09d73..d53d50479b 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -1352,9 +1352,7 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool,
if ((backingStore = virXPathString("string(./backingStore/path)", ctxt))) {
def->target.backingStore = virStorageSourceNew();
def->target.backingStore->type = VIR_STORAGE_TYPE_FILE;
-
-def->target.backingStore->path = backingStore;
-backingStore = NULL;
+def->target.backingStore->path = g_steal_pointer();
if (options->formatFromString) {
g_autofree char *format = NULL;
--
2.26.2
[PATCH 08/13] virDomainSnapshotAlignDisks: Add 'domdef' local variable
There are multiple places accessing the domain definition. Extract it to
a local variable so that it's more clear what's happening.
Signed-off-by: Peter Krempa
---
src/conf/snapshot_conf.c | 27 ++-
1 file changed, 14 insertions(+), 13 deletions(-)
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index c835ec7333..a1cee01944 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -649,33 +649,34 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr
snapdef,
int default_snapshot,
bool require_match)
{
+virDomainDefPtr domdef = snapdef->parent.dom;
g_autoptr(virBitmap) map = NULL;
size_t i;
int ndisks;
-if (!snapdef->parent.dom) {
+if (!domdef) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("missing domain in snapshot"));
return -1;
}
-if (snapdef->ndisks > snapdef->parent.dom->ndisks) {
+if (snapdef->ndisks > domdef->ndisks) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("too many disk snapshot requests for domain"));
return -1;
}
/* Unlikely to have a guest without disks but technically possible. */
-if (!snapdef->parent.dom->ndisks)
+if (!domdef->ndisks)
return 0;
-if (!(map = virBitmapNew(snapdef->parent.dom->ndisks)))
+if (!(map = virBitmapNew(domdef->ndisks)))
return -1;
/* Double check requested disks. */
for (i = 0; i < snapdef->ndisks; i++) {
virDomainSnapshotDiskDefPtr snapdisk = >disks[i];
-int idx = virDomainDiskIndexByName(snapdef->parent.dom,
snapdisk->name, false);
+int idx = virDomainDiskIndexByName(domdef, snapdisk->name, false);
int disk_snapshot;
if (idx < 0) {
@@ -693,7 +694,7 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr snapdef,
ignore_value(virBitmapSetBit(map, idx));
snapdisk->idx = idx;
-disk_snapshot = snapdef->parent.dom->disks[idx]->snapshot;
+disk_snapshot = domdef->disks[idx]->snapshot;
if (!snapdisk->snapshot) {
if (disk_snapshot &&
(!require_match ||
@@ -721,33 +722,33 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr
snapdef,
snapdisk->src->path, snapdisk->name);
return -1;
}
-if (STRNEQ(snapdisk->name, snapdef->parent.dom->disks[idx]->dst)) {
+if (STRNEQ(snapdisk->name, domdef->disks[idx]->dst)) {
VIR_FREE(snapdisk->name);
-snapdisk->name = g_strdup(snapdef->parent.dom->disks[idx]->dst);
+snapdisk->name = g_strdup(domdef->disks[idx]->dst);
}
}
/* Provide defaults for all remaining disks. */
ndisks = snapdef->ndisks;
if (VIR_EXPAND_N(snapdef->disks, snapdef->ndisks,
- snapdef->parent.dom->ndisks - snapdef->ndisks) < 0)
+ domdef->ndisks - snapdef->ndisks) < 0)
return -1;
-for (i = 0; i < snapdef->parent.dom->ndisks; i++) {
+for (i = 0; i < domdef->ndisks; i++) {
virDomainSnapshotDiskDefPtr snapdisk;
if (virBitmapIsBitSet(map, i))
continue;
snapdisk = >disks[ndisks++];
snapdisk->src = virStorageSourceNew();
-snapdisk->name = g_strdup(snapdef->parent.dom->disks[i]->dst);
+snapdisk->name = g_strdup(domdef->disks[i]->dst);
snapdisk->idx = i;
/* Don't snapshot empty drives */
-if (virStorageSourceIsEmpty(snapdef->parent.dom->disks[i]->src))
+if (virStorageSourceIsEmpty(domdef->disks[i]->src))
snapdisk->snapshot = VIR_DOMAIN_SNAPSHOT_LOCATION_NONE;
else
-snapdisk->snapshot = snapdef->parent.dom->disks[i]->snapshot;
+snapdisk->snapshot = domdef->disks[i]->snapshot;
snapdisk->src->type = VIR_STORAGE_TYPE_FILE;
if (!snapdisk->snapshot)
--
2.26.2
[PATCH 01/13] virStorageSourceNew: Abort on failure
Add an abort() on the class/object allocation failures so that
virStorageSourceNew() always returns a virStorageSource and remove
checks from all callers.
Signed-off-by: Peter Krempa
---
src/conf/backup_conf.c| 7 ++
src/conf/domain_conf.c| 21 ++
src/conf/snapshot_conf.c | 7 ++
src/conf/storage_conf.c | 4 +---
src/qemu/qemu_domain.c| 21 ++
src/qemu/qemu_driver.c| 12 +++---
src/qemu/qemu_migration.c | 7 ++
src/qemu/qemu_snapshot.c | 3 +--
src/storage/storage_backend_gluster.c | 5 +
src/storage/storage_backend_logical.c | 4 +---
src/storage/storage_util.c| 10 +++--
src/util/virstoragefile.c | 32 ++-
tests/qemublocktest.c | 16 +++---
tests/virstoragetest.c| 5 +
14 files changed, 46 insertions(+), 108 deletions(-)
diff --git a/src/conf/backup_conf.c b/src/conf/backup_conf.c
index 5caba621d8..5c475239ad 100644
--- a/src/conf/backup_conf.c
+++ b/src/conf/backup_conf.c
@@ -169,8 +169,7 @@ virDomainBackupDiskDefParseXML(xmlNodePtr node,
def->state = tmp;
}
-if (!(def->store = virStorageSourceNew()))
-return -1;
+def->store = virStorageSourceNew();
if ((type = virXMLPropString(node, "type"))) {
if ((def->store->type = virStorageTypeFromString(type)) <= 0) {
@@ -500,9 +499,7 @@ virDomainBackupDefAssignStore(virDomainBackupDiskDefPtr
disk,
}
} else if (!disk->store) {
if (virStorageSourceGetActualType(src) == VIR_STORAGE_TYPE_FILE) {
-if (!(disk->store = virStorageSourceNew()))
-return -1;
-
+disk->store = virStorageSourceNew();
disk->store->type = VIR_STORAGE_TYPE_FILE;
disk->store->path = g_strdup_printf("%s.%s", src->path, suffix);
} else {
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 9289c147fe..bbe3cddadf 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2187,8 +2187,7 @@ virDomainDiskDefNew(virDomainXMLOptionPtr xmlopt)
if (VIR_ALLOC(ret) < 0)
return NULL;
-if (!(ret->src = virStorageSourceNew()))
-goto error;
+ret->src = virStorageSourceNew();
if (xmlopt &&
xmlopt->privateData.diskNew &&
@@ -2400,8 +2399,7 @@ virDomainFSDefNew(virDomainXMLOptionPtr xmlopt)
if (VIR_ALLOC(ret) < 0)
return NULL;
-if (!(ret->src = virStorageSourceNew()))
-goto cleanup;
+ret->src = virStorageSourceNew();
if (xmlopt &&
xmlopt->privateData.fsNew &&
@@ -8325,9 +8323,8 @@ virDomainHostdevSubsysSCSIHostDefParseXML(xmlNodePtr
sourcenode,
if (flags & VIR_DOMAIN_DEF_PARSE_STATUS &&
xmlopt && xmlopt->privateData.storageParse) {
if ((ctxt->node = virXPathNode("./privateData", ctxt))) {
-if (!scsihostsrc->src &&
-!(scsihostsrc->src = virStorageSourceNew()))
-return -1;
+if (!scsihostsrc->src)
+scsihostsrc->src = virStorageSourceNew();
if (xmlopt->privateData.storageParse(ctxt, scsihostsrc->src) < 0)
return -1;
}
@@ -8353,8 +8350,7 @@ virDomainHostdevSubsysSCSIiSCSIDefParseXML(xmlNodePtr
sourcenode,
/* For the purposes of command line creation, this needs to look
* like a disk storage source */
-if (!(iscsisrc->src = virStorageSourceNew()))
-return -1;
+iscsisrc->src = virStorageSourceNew();
iscsisrc->src->type = VIR_STORAGE_TYPE_NETWORK;
iscsisrc->src->protocol = VIR_STORAGE_NET_PROTOCOL_ISCSI;
@@ -9770,9 +9766,7 @@ virDomainStorageSourceParseBase(const char *type,
{
g_autoptr(virStorageSource) src = NULL;
-if (!(src = virStorageSourceNew()))
-return NULL;
-
+src = virStorageSourceNew();
src->type = VIR_STORAGE_TYPE_FILE;
if (type &&
@@ -9960,8 +9954,7 @@ virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt,
/* terminator does not have a type */
if (!(type = virXMLPropString(ctxt->node, "type"))) {
-if (!(src->backingStore = virStorageSourceNew()))
-return -1;
+src->backingStore = virStorageSourceNew();
return 0;
}
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index 1ee63b9141..87a00082ef 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -148,9 +148,7 @@ virDomainSnapshotDiskDefParseXML(xmlNodePtr node,
ctxt->node = node;
-if (!(def->src = virStorageSourceNew()))
-goto cleanup;
-
+def->src = virStorageSourceNew();
def->name = virXMLPropString(node, "name");
if (!def->name) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
@@ -744,8 +742,7 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def,
if (virBitmapIsBitSet(map, i))
[PATCH 06/13] virDomainSnapshotAlignDisks: Rename 'def' -> 'snapdef'
While this function resides in the snapshot config module the 'def'
variable is referencing the VM definition in most places. Change the
name to 'snapdef' to avoid ambiguity especially since we are also
dealing with the domain definition in this function.
Signed-off-by: Peter Krempa
---
src/conf/snapshot_conf.c | 42
1 file changed, 21 insertions(+), 21 deletions(-)
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index 160f2054a4..77f53c4a1d 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -645,7 +645,7 @@ virDomainSnapshotCompareDiskIndex(const void *a, const void
*b)
* dom->disks. If require_match, also ensure that there is no
* conflicting requests for both internal and external snapshots. */
int
-virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def,
+virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr snapdef,
int default_snapshot,
bool require_match)
{
@@ -653,29 +653,29 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def,
size_t i;
int ndisks;
-if (!def->parent.dom) {
+if (!snapdef->parent.dom) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("missing domain in snapshot"));
return -1;
}
-if (def->ndisks > def->parent.dom->ndisks) {
+if (snapdef->ndisks > snapdef->parent.dom->ndisks) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("too many disk snapshot requests for domain"));
return -1;
}
/* Unlikely to have a guest without disks but technically possible. */
-if (!def->parent.dom->ndisks)
+if (!snapdef->parent.dom->ndisks)
return 0;
-if (!(map = virBitmapNew(def->parent.dom->ndisks)))
+if (!(map = virBitmapNew(snapdef->parent.dom->ndisks)))
return -1;
/* Double check requested disks. */
-for (i = 0; i < def->ndisks; i++) {
-virDomainSnapshotDiskDefPtr disk = >disks[i];
-int idx = virDomainDiskIndexByName(def->parent.dom, disk->name, false);
+for (i = 0; i < snapdef->ndisks; i++) {
+virDomainSnapshotDiskDefPtr disk = >disks[i];
+int idx = virDomainDiskIndexByName(snapdef->parent.dom, disk->name,
false);
int disk_snapshot;
if (idx < 0) {
@@ -693,7 +693,7 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def,
ignore_value(virBitmapSetBit(map, idx));
disk->idx = idx;
-disk_snapshot = def->parent.dom->disks[idx]->snapshot;
+disk_snapshot = snapdef->parent.dom->disks[idx]->snapshot;
if (!disk->snapshot) {
if (disk_snapshot &&
(!require_match ||
@@ -721,44 +721,44 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def,
disk->src->path, disk->name);
return -1;
}
-if (STRNEQ(disk->name, def->parent.dom->disks[idx]->dst)) {
+if (STRNEQ(disk->name, snapdef->parent.dom->disks[idx]->dst)) {
VIR_FREE(disk->name);
-disk->name = g_strdup(def->parent.dom->disks[idx]->dst);
+disk->name = g_strdup(snapdef->parent.dom->disks[idx]->dst);
}
}
/* Provide defaults for all remaining disks. */
-ndisks = def->ndisks;
-if (VIR_EXPAND_N(def->disks, def->ndisks,
- def->parent.dom->ndisks - def->ndisks) < 0)
+ndisks = snapdef->ndisks;
+if (VIR_EXPAND_N(snapdef->disks, snapdef->ndisks,
+ snapdef->parent.dom->ndisks - snapdef->ndisks) < 0)
return -1;
-for (i = 0; i < def->parent.dom->ndisks; i++) {
+for (i = 0; i < snapdef->parent.dom->ndisks; i++) {
virDomainSnapshotDiskDefPtr disk;
if (virBitmapIsBitSet(map, i))
continue;
-disk = >disks[ndisks++];
+disk = >disks[ndisks++];
disk->src = virStorageSourceNew();
-disk->name = g_strdup(def->parent.dom->disks[i]->dst);
+disk->name = g_strdup(snapdef->parent.dom->disks[i]->dst);
disk->idx = i;
/* Don't snapshot empty drives */
-if (virStorageSourceIsEmpty(def->parent.dom->disks[i]->src))
+if (virStorageSourceIsEmpty(snapdef->parent.dom->disks[i]->src))
disk->snapshot = VIR_DOMAIN_SNAPSHOT_LOCATION_NONE;
else
-disk->snapshot = def->parent.dom->disks[i]->snapshot;
+disk->snapshot = snapdef->parent.dom->disks[i]->snapshot;
disk->src->type = VIR_STORAGE_TYPE_FILE;
if (!disk->snapshot)
disk->snapshot = default_snapshot;
}
-qsort(>disks[0], def->ndisks, sizeof(def->disks[0]),
+qsort(>disks[0], snapdef->ndisks, sizeof(snapdef->disks[0]),
virDomainSnapshotCompareDiskIndex);
/* Generate default external file names for external snapshot locations */
-if (virDomainSnapshotDefAssignExternalNames(def) < 0)
+if
[PATCH 04/13] qemuSnapshotCreateInactiveExternal: Don't access 'idx' of snapshot
After virDomainSnapshotAlignDisks is called the definitions of disks in
the snapshot definition and in the domain definition are in the same
order so they can be addressed using the same index.
This frees up 'idx' to be removed later.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_snapshot.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index a6e091dd09..48a4e9dd41 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -171,7 +171,7 @@ qemuSnapshotCreateInactiveExternal(virQEMUDriverPtr driver,
* create them correctly. */
for (i = 0; i < snapdef->ndisks && !reuse; i++) {
snapdisk = &(snapdef->disks[i]);
-defdisk = snapdef->parent.dom->disks[snapdisk->idx];
+defdisk = vm->def->disks[i];
if (snapdisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)
continue;
@@ -216,7 +216,7 @@ qemuSnapshotCreateInactiveExternal(virQEMUDriverPtr driver,
g_autoptr(virStorageSource) newsrc = NULL;
snapdisk = &(snapdef->disks[i]);
-defdisk = vm->def->disks[snapdisk->idx];
+defdisk = vm->def->disks[i];
if (snapdisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)
continue;
--
2.26.2
[PATCH 00/13] virStorageSourceNew failure handling and virDomainSnapshotAlignDisks refactors
A set of patches that prevent failures from virStorageSourceNew and refactor virDomainSnapshotAlignDisks for better readability. Peter Krempa (13): virStorageSourceNew: Abort on failure virStorageVolDefParseXML: Use g_steal_pointer qemuDomainBlockRebase: Replace ternary operator with if/else qemuSnapshotCreateInactiveExternal: Don't access 'idx' of snapshot virDomainSnapshotAlignDisks: Refactor cleanup virDomainSnapshotAlignDisks: Rename 'def' -> 'snapdef' virDomainSnapshotAlignDisks: Rename 'disk' -> 'snapdisk' virDomainSnapshotAlignDisks: Add 'domdef' local variable virDomainSnapshotAlignDisks: Extract domain disk definition to a local variable virDomainSnapshotAlignDisks: remove unnecessary 'tmp' variable virDomainSnapshotAlignDisks: clarify handing of snapshot location virDomainSnapshotAlignDisks: refactor extension to all disks virDomainSnapshotDiskDef: Remove 'idx' field src/conf/backup_conf.c| 7 +- src/conf/domain_conf.c| 21 ++-- src/conf/snapshot_conf.c | 160 +++--- src/conf/snapshot_conf.h | 1 - src/conf/storage_conf.c | 8 +- src/qemu/qemu_domain.c| 21 ++-- src/qemu/qemu_driver.c| 18 ++- src/qemu/qemu_migration.c | 7 +- src/qemu/qemu_snapshot.c | 7 +- src/storage/storage_backend_gluster.c | 5 +- src/storage/storage_backend_logical.c | 4 +- src/storage/storage_util.c| 10 +- src/util/virstoragefile.c | 32 ++ tests/qemublocktest.c | 16 +-- tests/virstoragetest.c| 5 +- 15 files changed, 121 insertions(+), 201 deletions(-) -- 2.26.2
[PATCH 13/13] virDomainSnapshotDiskDef: Remove 'idx' field
It's no longer needed and is valid only after virDomainSnapshotAlignDisks
is called while holding the lock.
Signed-off-by: Peter Krempa
---
src/conf/snapshot_conf.h | 1 -
1 file changed, 1 deletion(-)
diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h
index b5b1ef2718..fbc9b17c54 100644
--- a/src/conf/snapshot_conf.h
+++ b/src/conf/snapshot_conf.h
@@ -63,7 +63,6 @@ typedef struct _virDomainSnapshotDiskDef
virDomainSnapshotDiskDef;
typedef virDomainSnapshotDiskDef *virDomainSnapshotDiskDefPtr;
struct _virDomainSnapshotDiskDef {
char *name; /* name matching the dom->disks that matches name */
int snapshot; /* virDomainSnapshotLocation */
/* details of wrapper external file. src is always non-NULL.
--
2.26.2
Re: [PATCH] docs: manpages: Strip table of contents from manpages
On a Wednesday in 2020, Peter Krempa wrote: After meson conversion the man pages started to contain the table of contents. In autoconf we prevented this by a 'grep -v ::contents' in the command building the manpages. A more cultured solution is to strip out the 'contents' docutils element directly. Reported-by: Daniel P. Berrangé Signed-off-by: Peter Krempa --- docs/manpages/meson.build | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH] apparmor: Allow /usr/libexec for libxl-save-helper and pygrub
On Wed, Sep 23, 2020 at 12:35 AM Jim Fehlig wrote:
>
> Like other distros, openSUSE Tumbleweed recently changed libexecdir from
> /usr/lib to /usr/libexec. Add it as an allowed path for libxl-save-helper
> and pygrub.
Hi Jim,
ack to the intention, but I think since this should use @libexecdir@ I think.
Or did anything change that this doesn't apply anymore ... in that
case I beg your pardon.
[1]:
https://libvirt.org/git/?p=libvirt.git;a=commit;h=5c8bd31c881e99261ac098e867a79b300440731a
> Signed-off-by: Jim Fehlig
> ---
>
> I considered including /usr/lib64, but I don't think any distros are
> installing xen libexecdir targets to /usr/lib64. Happy to include it
> if I'm wrong :-).
>
> src/security/apparmor/usr.sbin.libvirtd.in | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/security/apparmor/usr.sbin.libvirtd.in
> b/src/security/apparmor/usr.sbin.libvirtd.in
> index f2030764cd..bf4563e1e8 100644
> --- a/src/security/apparmor/usr.sbin.libvirtd.in
> +++ b/src/security/apparmor/usr.sbin.libvirtd.in
> @@ -86,8 +86,8 @@ profile libvirtd @sbindir@/libvirtd
> flags=(attach_disconnected) {
>/{usr/,}lib/udev/scsi_id PUx,
>/usr/{lib,lib64}/xen-common/bin/xen-toolstack PUx,
>/usr/{lib,lib64}/xen/bin/* Ux,
> - /usr/lib/xen-*/bin/libxl-save-helper PUx,
> - /usr/lib/xen-*/bin/pygrub PUx,
> + /usr/{lib,libexec}/xen-*/bin/libxl-save-helper PUx,
> + /usr/{lib,libexec}/xen-*/bin/pygrub PUx,
>/usr/{lib,lib64,lib/qemu,libexec}/vhost-user-gpu PUx,
>/usr/{lib,lib64,lib/qemu,libexec}/virtiofsd PUx,
>
> --
> 2.28.0
>
>
--
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd
Re: [PATCH] docs: manpages: Strip table of contents from manpages
On Wed, Sep 23, 2020 at 03:17:50PM +0200, Peter Krempa wrote: > After meson conversion the man pages started to contain the table of > contents. > > In autoconf we prevented this by a 'grep -v ::contents' in the command > building the manpages. > > A more cultured solution is to strip out the 'contents' docutils element > directly. > > Reported-by: Daniel P. Berrangé > Signed-off-by: Peter Krempa > --- > docs/manpages/meson.build | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) Reviewed-by: Pavel Hrdina signature.asc Description: PGP signature
Re: [PATCH] docs: manpages: Strip table of contents from manpages
On Wed, Sep 23, 2020 at 03:17:50PM +0200, Peter Krempa wrote: > After meson conversion the man pages started to contain the table of > contents. > > In autoconf we prevented this by a 'grep -v ::contents' in the command > building the manpages. > > A more cultured solution is to strip out the 'contents' docutils element > directly. Ah, very nice and clever ! > > Reported-by: Daniel P. Berrangé > Signed-off-by: Peter Krempa > --- > docs/manpages/meson.build | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/docs/manpages/meson.build b/docs/manpages/meson.build > index 3888bb8efe..7ed1d304a4 100644 > --- a/docs/manpages/meson.build > +++ b/docs/manpages/meson.build > @@ -89,7 +89,8 @@ foreach data : docs_man_files >man_file, >input: rst_file, >output: man_file, > - command: [ rst2man_prog, '--strict', '@INPUT@', '@OUTPUT@' ], > + # 'contents' element is the table of contents which is undesired in > manpage > + command: [ rst2man_prog, '--strip-elements-with-class', 'contents', > '--strict', '@INPUT@', '@OUTPUT@' ], >install: true, >install_dir: mandir / 'man@0@'.format(data['section']), > ) 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 :|
[PATCH] docs: manpages: Strip table of contents from manpages
After meson conversion the man pages started to contain the table of contents. In autoconf we prevented this by a 'grep -v ::contents' in the command building the manpages. A more cultured solution is to strip out the 'contents' docutils element directly. Reported-by: Daniel P. Berrangé Signed-off-by: Peter Krempa --- docs/manpages/meson.build | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/manpages/meson.build b/docs/manpages/meson.build index 3888bb8efe..7ed1d304a4 100644 --- a/docs/manpages/meson.build +++ b/docs/manpages/meson.build @@ -89,7 +89,8 @@ foreach data : docs_man_files man_file, input: rst_file, output: man_file, - command: [ rst2man_prog, '--strict', '@INPUT@', '@OUTPUT@' ], + # 'contents' element is the table of contents which is undesired in manpage + command: [ rst2man_prog, '--strip-elements-with-class', 'contents', '--strict', '@INPUT@', '@OUTPUT@' ], install: true, install_dir: mandir / 'man@0@'.format(data['section']), ) -- 2.26.2
Re: [libvirt PATCH 3/4] tests: use g_new0 instead of VIR_ALLOC_N
On a Wednesday in 2020, Peter Krempa wrote: On Wed, Sep 23, 2020 at 01:04:17 +0200, Ján Tomko wrote: Signed-off-by: Ján Tomko --- [...] diff --git a/tests/securityselinuxtest.c b/tests/securityselinuxtest.c index ae4b08b9d8..fd746d1ca1 100644 --- a/tests/securityselinuxtest.c +++ b/tests/securityselinuxtest.c @@ -71,8 +71,7 @@ testBuildDomainDef(bool dynamic, goto error; def->virtType = VIR_DOMAIN_VIRT_KVM; -if (VIR_ALLOC_N(def->seclabels, 1) < 0) -goto error; +def->seclabels = g_new0(char, 1); 'def' is virDomainDefPtr thus 'def->seclabels' is not char but 'virSecurityLabelDefPtr *' :neutral_face: :palm_tree: The compiler didn't moan because it's a double pointer, but it certainly doens't have enough size nor the correct type. Actually, the compiler did not even open this file. v2 coming up Jano if (VIR_ALLOC(secdef) < 0) goto error; signature.asc Description: PGP signature
