[PATCH] util: virExec may blocked by reading pipe if grandchild prematurely exit
From: Xu Chao
When VIR_EXEC_DAEMON is set, if virPidFileAcquirePath/virSetInherit failed,
then pipesync[0] can not be closed when granchild process exit, because
pipesync[1] still opened in child process. and then saferead in child
process may blocked forever, and left grandchild process in defunct state.
Signed-off-by: Xu Chao
Signed-off-by: Yi Wang
---
src/util/vircommand.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/util/vircommand.c b/src/util/vircommand.c
index fead373729..fa71f40d81 100644
--- a/src/util/vircommand.c
+++ b/src/util/vircommand.c
@@ -782,6 +782,9 @@ virExec(virCommand *cmd)
}
if (pid > 0) {
+/* close pipe[1], then the pipe can be closed if grandchild
+ * died prematurely */
+VIR_FORCE_CLOSE(pipesync[1]);
/* The parent expect us to have written the pid file before
* exiting. Wait here for the child to write it and signal us. */
if (cmd->pidfile &&
--
2.27.0
[PATCH] qemu: Rewrite of code pattern
This patch rewrites the pattern using early return where it is
not needed.
Signed-off-by: Kristina Hanicova
---
src/qemu/qemu_alias.c | 41 -
1 file changed, 16 insertions(+), 25 deletions(-)
diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c
index 5e35f43614..6f664fcd84 100644
--- a/src/qemu/qemu_alias.c
+++ b/src/qemu/qemu_alias.c
@@ -361,10 +361,9 @@ static int
qemuAssignDeviceSoundAlias(virDomainSoundDef *sound,
int idx)
{
-if (sound->info.alias)
-return 0;
+if (!sound->info.alias)
+sound->info.alias = g_strdup_printf("sound%d", idx);
-sound->info.alias = g_strdup_printf("sound%d", idx);
return 0;
}
@@ -373,10 +372,9 @@ static int
qemuAssignDeviceVideoAlias(virDomainVideoDef *video,
int idx)
{
-if (video->info.alias)
-return 0;
+if (!video->info.alias)
+video->info.alias = g_strdup_printf("video%d", idx);
-video->info.alias = g_strdup_printf("video%d", idx);
return 0;
}
@@ -385,10 +383,9 @@ static int
qemuAssignDeviceHubAlias(virDomainHubDef *hub,
int idx)
{
-if (hub->info.alias)
-return 0;
+if (!hub->info.alias)
+hub->info.alias = g_strdup_printf("hub%d", idx);
-hub->info.alias = g_strdup_printf("hub%d", idx);
return 0;
}
@@ -397,10 +394,9 @@ static int
qemuAssignDeviceSmartcardAlias(virDomainSmartcardDef *smartcard,
int idx)
{
-if (smartcard->info.alias)
-return 0;
+if (!smartcard->info.alias)
+smartcard->info.alias = g_strdup_printf("smartcard%d", idx);
-smartcard->info.alias = g_strdup_printf("smartcard%d", idx);
return 0;
}
@@ -409,10 +405,9 @@ static int
qemuAssignDeviceMemballoonAlias(virDomainMemballoonDef *memballoon,
int idx)
{
-if (memballoon->info.alias)
-return 0;
+if (!memballoon->info.alias)
+memballoon->info.alias = g_strdup_printf("balloon%d", idx);
-memballoon->info.alias = g_strdup_printf("balloon%d", idx);
return 0;
}
@@ -421,10 +416,9 @@ static int
qemuAssignDeviceTPMAlias(virDomainTPMDef *tpm,
int idx)
{
-if (tpm->info.alias)
-return 0;
+if (!tpm->info.alias)
+tpm->info.alias = g_strdup_printf("tpm%d", idx);
-tpm->info.alias = g_strdup_printf("tpm%d", idx);
return 0;
}
@@ -586,10 +580,8 @@ qemuAssignDeviceWatchdogAlias(virDomainWatchdogDef
*watchdog)
{
/* Currently, there's just one watchdog per domain */
-if (watchdog->info.alias)
-return 0;
-
-watchdog->info.alias = g_strdup("watchdog0");
+if (!watchdog->info.alias)
+watchdog->info.alias = g_strdup("watchdog0");
return 0;
}
@@ -621,9 +613,8 @@ qemuAssignDeviceInputAlias(virDomainDef *def,
int
qemuAssignDeviceVsockAlias(virDomainVsockDef *vsock)
{
-if (vsock->info.alias)
-return 0;
-vsock->info.alias = g_strdup("vsock0");
+if (!vsock->info.alias)
+vsock->info.alias = g_strdup("vsock0");
return 0;
}
--
2.31.1
[PATCH] qemu: Rewrite code to the pattern
I have seen this pattern a lot in the project, so I decided to
rewrite code I stumbled upon to the same pattern as well.
Signed-off-by: Kristina Hanicova
---
src/qemu/qemu_driver.c | 14 --
1 file changed, 4 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 1959b639da..b938687189 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -16231,10 +16231,9 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorSetBlockIoThrottle(priv->mon, drivealias, qdevid,
);
-if (qemuDomainObjExitMonitor(driver, vm) < 0)
-ret = -1;
-if (ret < 0)
+if (qemuDomainObjExitMonitor(driver, vm) < 0 || ret < 0)
goto endjob;
+
ret = -1;
}
@@ -16362,9 +16361,7 @@ qemuDomainGetBlockIoTune(virDomainPtr dom,
}
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorGetBlockIoThrottle(priv->mon, drivealias, qdevid,
);
-if (qemuDomainObjExitMonitor(driver, vm) < 0)
-goto endjob;
-if (ret < 0)
+if (qemuDomainObjExitMonitor(driver, vm) < 0 || ret < 0)
goto endjob;
}
@@ -17375,10 +17372,7 @@ qemuDomainSetTime(virDomainPtr dom,
if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_RTC_RESET_REINJECTION)) {
qemuDomainObjEnterMonitor(driver, vm);
rv = qemuMonitorRTCResetReinjection(priv->mon);
-if (qemuDomainObjExitMonitor(driver, vm) < 0)
-goto endjob;
-
-if (rv < 0)
+if (qemuDomainObjExitMonitor(driver, vm) < 0 || rv < 0)
goto endjob;
}
--
2.31.1
[libvirt PATCH 3/3] qemu: mock swtpm initialization in tests
The domain capabilities won't report TPM support unless SWTPM can be initialized. To avoid relying on the swtpm install in the host, mock the entire initialization method, since all it needs todo is return a non-error value. Signed-off-by: Daniel P. Berrangé --- tests/domaincapsdata/qemu_2.11.0-q35.x86_64.xml | 10 +- tests/domaincapsdata/qemu_2.11.0-tcg.x86_64.xml | 10 +- tests/domaincapsdata/qemu_2.11.0.s390x.xml| 7 ++- tests/domaincapsdata/qemu_2.11.0.x86_64.xml | 10 +- tests/domaincapsdata/qemu_2.12.0-q35.x86_64.xml | 11 ++- tests/domaincapsdata/qemu_2.12.0-tcg.x86_64.xml | 11 ++- tests/domaincapsdata/qemu_2.12.0-virt.aarch64.xml | 7 ++- tests/domaincapsdata/qemu_2.12.0.aarch64.xml | 7 ++- tests/domaincapsdata/qemu_2.12.0.ppc64.xml| 7 ++- tests/domaincapsdata/qemu_2.12.0.s390x.xml| 7 ++- tests/domaincapsdata/qemu_2.12.0.x86_64.xml | 11 ++- tests/domaincapsdata/qemu_3.0.0-q35.x86_64.xml| 11 ++- tests/domaincapsdata/qemu_3.0.0-tcg.x86_64.xml| 11 ++- tests/domaincapsdata/qemu_3.0.0.ppc64.xml | 7 ++- tests/domaincapsdata/qemu_3.0.0.s390x.xml | 7 ++- tests/domaincapsdata/qemu_3.0.0.x86_64.xml| 11 ++- tests/domaincapsdata/qemu_3.1.0-q35.x86_64.xml| 11 ++- tests/domaincapsdata/qemu_3.1.0-tcg.x86_64.xml| 11 ++- tests/domaincapsdata/qemu_3.1.0.ppc64.xml | 7 ++- tests/domaincapsdata/qemu_3.1.0.x86_64.xml| 11 ++- tests/domaincapsdata/qemu_4.0.0-q35.x86_64.xml| 11 ++- tests/domaincapsdata/qemu_4.0.0-tcg.x86_64.xml| 11 ++- tests/domaincapsdata/qemu_4.0.0-virt.aarch64.xml | 5 - tests/domaincapsdata/qemu_4.0.0.aarch64.xml | 5 - tests/domaincapsdata/qemu_4.0.0.ppc64.xml | 5 - tests/domaincapsdata/qemu_4.0.0.s390x.xml | 5 - tests/domaincapsdata/qemu_4.0.0.x86_64.xml| 11 ++- tests/domaincapsdata/qemu_4.1.0-q35.x86_64.xml| 11 ++- tests/domaincapsdata/qemu_4.1.0-tcg.x86_64.xml| 11 ++- tests/domaincapsdata/qemu_4.1.0.x86_64.xml| 11 ++- tests/domaincapsdata/qemu_4.2.0-q35.x86_64.xml| 11 ++- tests/domaincapsdata/qemu_4.2.0-tcg.x86_64.xml| 11 ++- tests/domaincapsdata/qemu_4.2.0-virt.aarch64.xml | 5 - tests/domaincapsdata/qemu_4.2.0.aarch64.xml | 5 - tests/domaincapsdata/qemu_4.2.0.ppc64.xml | 7 ++- tests/domaincapsdata/qemu_4.2.0.s390x.xml | 5 - tests/domaincapsdata/qemu_4.2.0.x86_64.xml| 11 ++- tests/domaincapsdata/qemu_5.0.0-q35.x86_64.xml| 11 ++- tests/domaincapsdata/qemu_5.0.0-tcg.x86_64.xml| 11 ++- tests/domaincapsdata/qemu_5.0.0-virt.aarch64.xml | 10 +- tests/domaincapsdata/qemu_5.0.0.aarch64.xml | 10 +- tests/domaincapsdata/qemu_5.0.0.ppc64.xml | 11 ++- tests/domaincapsdata/qemu_5.0.0.x86_64.xml| 11 ++- tests/domaincapsdata/qemu_5.1.0-q35.x86_64.xml| 11 ++- tests/domaincapsdata/qemu_5.1.0-tcg.x86_64.xml| 11 ++- tests/domaincapsdata/qemu_5.1.0.sparc.xml | 5 - tests/domaincapsdata/qemu_5.1.0.x86_64.xml| 11 ++- tests/domaincapsdata/qemu_5.2.0-q35.x86_64.xml| 11 ++- tests/domaincapsdata/qemu_5.2.0-tcg.x86_64.xml| 11 ++- tests/domaincapsdata/qemu_5.2.0-virt.aarch64.xml | 10 +- tests/domaincapsdata/qemu_5.2.0.aarch64.xml | 10 +- tests/domaincapsdata/qemu_5.2.0.ppc64.xml | 11 ++- tests/domaincapsdata/qemu_5.2.0.s390x.xml | 5 - tests/domaincapsdata/qemu_5.2.0.x86_64.xml| 11 ++- tests/domaincapsdata/qemu_6.0.0-q35.x86_64.xml| 11 ++- tests/domaincapsdata/qemu_6.0.0-tcg.x86_64.xml| 11 ++- tests/domaincapsdata/qemu_6.0.0-virt.aarch64.xml | 10 +- tests/domaincapsdata/qemu_6.0.0.aarch64.xml | 10 +- tests/domaincapsdata/qemu_6.0.0.s390x.xml | 5 - tests/domaincapsdata/qemu_6.0.0.x86_64.xml| 11 ++- tests/domaincapsdata/qemu_6.1.0-q35.x86_64.xml| 11 ++- tests/domaincapsdata/qemu_6.1.0-tcg.x86_64.xml| 11 ++- tests/domaincapsdata/qemu_6.1.0.x86_64.xml| 11 ++- tests/domaincapsdata/qemu_6.2.0-q35.x86_64.xml| 11 ++- tests/domaincapsdata/qemu_6.2.0-tcg.x86_64.xml| 11 ++- tests/domaincapsdata/qemu_6.2.0-virt.aarch64.xml | 10 +- tests/domaincapsdata/qemu_6.2.0.aarch64.xml | 10 +- tests/domaincapsdata/qemu_6.2.0.ppc64.xml | 11 ++- tests/domaincapsdata/qemu_6.2.0.x86_64.xml| 11 ++- tests/domaincapstest.c| 9 + 70 files changed, 592 insertions(+), 69 deletions(-) diff --git
[libvirt PATCH 1/3] conf: add TPM devices to domain capabilities
This adds reporting of available TPM models and backends to the domain
capabilities schema
Signed-off-by: Daniel P. Berrangé
---
docs/schemas/domaincaps.rng| 10 ++
src/conf/domain_capabilities.c | 14 ++
src/conf/domain_capabilities.h | 10 ++
3 files changed, 34 insertions(+)
diff --git a/docs/schemas/domaincaps.rng b/docs/schemas/domaincaps.rng
index 8b5267f741..1b6122507f 100644
--- a/docs/schemas/domaincaps.rng
+++ b/docs/schemas/domaincaps.rng
@@ -195,6 +195,9 @@
+
+
+
@@ -240,6 +243,13 @@
+
+
+
+
+
+
+
diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c
index 1766129092..fef1326190 100644
--- a/src/conf/domain_capabilities.c
+++ b/src/conf/domain_capabilities.c
@@ -533,6 +533,19 @@ virDomainCapsDeviceRNGFormat(virBuffer *buf,
}
+static void
+virDomainCapsDeviceTPMFormat(virBuffer *buf,
+ const virDomainCapsDeviceTPM *tpm)
+{
+FORMAT_PROLOGUE(tpm);
+
+ENUM_PROCESS(tpm, model, virDomainTPMModelTypeToString);
+ENUM_PROCESS(tpm, backendModel, virDomainTPMBackendTypeToString);
+
+FORMAT_EPILOGUE(tpm);
+}
+
+
static void
virDomainCapsDeviceFilesystemFormat(virBuffer *buf,
const virDomainCapsDeviceFilesystem
*filesystem)
@@ -652,6 +665,7 @@ virDomainCapsFormat(const virDomainCaps *caps)
virDomainCapsDeviceHostdevFormat(, >hostdev);
virDomainCapsDeviceRNGFormat(, >rng);
virDomainCapsDeviceFilesystemFormat(, >filesystem);
+virDomainCapsDeviceTPMFormat(, >tpm);
virBufferAdjustIndent(, -2);
virBufferAddLit(, "\n");
diff --git a/src/conf/domain_capabilities.h b/src/conf/domain_capabilities.h
index d44acdcd01..2fcad87fd8 100644
--- a/src/conf/domain_capabilities.h
+++ b/src/conf/domain_capabilities.h
@@ -120,6 +120,15 @@ struct _virDomainCapsDeviceRNG {
virDomainCapsEnum backendModel; /* virDomainRNGBackend */
};
+STATIC_ASSERT_ENUM(VIR_DOMAIN_TPM_MODEL_LAST);
+STATIC_ASSERT_ENUM(VIR_DOMAIN_TPM_TYPE_LAST);
+typedef struct _virDomainCapsDeviceTPM virDomainCapsDeviceTPM;
+struct _virDomainCapsDeviceTPM {
+virTristateBool supported;
+virDomainCapsEnum model; /* virDomainTPMModel */
+virDomainCapsEnum backendModel; /* virDomainTPMBackendType */
+};
+
STATIC_ASSERT_ENUM(VIR_DOMAIN_FS_DRIVER_TYPE_LAST);
typedef struct _virDomainCapsDeviceFilesystem virDomainCapsDeviceFilesystem;
struct _virDomainCapsDeviceFilesystem {
@@ -211,6 +220,7 @@ struct _virDomainCaps {
virDomainCapsDeviceHostdev hostdev;
virDomainCapsDeviceRNG rng;
virDomainCapsDeviceFilesystem filesystem;
+virDomainCapsDeviceTPM tpm;
/* add new domain devices here */
virDomainCapsFeatureGIC gic;
--
2.33.1
[libvirt PATCH 0/3] Expose TPM availability in domain capabilities
If we can report whuether TPM is available, then mgmt apps can enable it by default for new VMs. This is important because OS like Win11 consider TPM to be mandatory. Daniel P. Berrangé (3): conf: add TPM devices to domain capabilities qemu: fill in domain capabilities for TPMs qemu: mock swtpm initialization in tests docs/schemas/domaincaps.rng | 10 ++ src/conf/domain_capabilities.c| 14 src/conf/domain_capabilities.h| 10 ++ src/qemu/qemu_capabilities.c | 32 +++ src/qemu/qemu_capabilities.h | 3 ++ .../domaincapsdata/qemu_2.11.0-q35.x86_64.xml | 9 ++ .../domaincapsdata/qemu_2.11.0-tcg.x86_64.xml | 9 ++ tests/domaincapsdata/qemu_2.11.0.s390x.xml| 6 tests/domaincapsdata/qemu_2.11.0.x86_64.xml | 9 ++ .../domaincapsdata/qemu_2.12.0-q35.x86_64.xml | 10 ++ .../domaincapsdata/qemu_2.12.0-tcg.x86_64.xml | 10 ++ .../qemu_2.12.0-virt.aarch64.xml | 6 tests/domaincapsdata/qemu_2.12.0.aarch64.xml | 6 tests/domaincapsdata/qemu_2.12.0.ppc64.xml| 6 tests/domaincapsdata/qemu_2.12.0.s390x.xml| 6 tests/domaincapsdata/qemu_2.12.0.x86_64.xml | 10 ++ .../domaincapsdata/qemu_3.0.0-q35.x86_64.xml | 10 ++ .../domaincapsdata/qemu_3.0.0-tcg.x86_64.xml | 10 ++ tests/domaincapsdata/qemu_3.0.0.ppc64.xml | 6 tests/domaincapsdata/qemu_3.0.0.s390x.xml | 6 tests/domaincapsdata/qemu_3.0.0.x86_64.xml| 10 ++ .../domaincapsdata/qemu_3.1.0-q35.x86_64.xml | 10 ++ .../domaincapsdata/qemu_3.1.0-tcg.x86_64.xml | 10 ++ tests/domaincapsdata/qemu_3.1.0.ppc64.xml | 6 tests/domaincapsdata/qemu_3.1.0.x86_64.xml| 10 ++ .../domaincapsdata/qemu_4.0.0-q35.x86_64.xml | 10 ++ .../domaincapsdata/qemu_4.0.0-tcg.x86_64.xml | 10 ++ .../qemu_4.0.0-virt.aarch64.xml | 4 +++ tests/domaincapsdata/qemu_4.0.0.aarch64.xml | 4 +++ tests/domaincapsdata/qemu_4.0.0.ppc64.xml | 4 +++ tests/domaincapsdata/qemu_4.0.0.s390x.xml | 4 +++ tests/domaincapsdata/qemu_4.0.0.x86_64.xml| 10 ++ .../domaincapsdata/qemu_4.1.0-q35.x86_64.xml | 10 ++ .../domaincapsdata/qemu_4.1.0-tcg.x86_64.xml | 10 ++ tests/domaincapsdata/qemu_4.1.0.x86_64.xml| 10 ++ .../domaincapsdata/qemu_4.2.0-q35.x86_64.xml | 10 ++ .../domaincapsdata/qemu_4.2.0-tcg.x86_64.xml | 10 ++ .../qemu_4.2.0-virt.aarch64.xml | 4 +++ tests/domaincapsdata/qemu_4.2.0.aarch64.xml | 4 +++ tests/domaincapsdata/qemu_4.2.0.ppc64.xml | 6 tests/domaincapsdata/qemu_4.2.0.s390x.xml | 4 +++ tests/domaincapsdata/qemu_4.2.0.x86_64.xml| 10 ++ .../domaincapsdata/qemu_5.0.0-q35.x86_64.xml | 10 ++ .../domaincapsdata/qemu_5.0.0-tcg.x86_64.xml | 10 ++ .../qemu_5.0.0-virt.aarch64.xml | 9 ++ tests/domaincapsdata/qemu_5.0.0.aarch64.xml | 9 ++ tests/domaincapsdata/qemu_5.0.0.ppc64.xml | 10 ++ tests/domaincapsdata/qemu_5.0.0.x86_64.xml| 10 ++ .../domaincapsdata/qemu_5.1.0-q35.x86_64.xml | 10 ++ .../domaincapsdata/qemu_5.1.0-tcg.x86_64.xml | 10 ++ tests/domaincapsdata/qemu_5.1.0.sparc.xml | 4 +++ tests/domaincapsdata/qemu_5.1.0.x86_64.xml| 10 ++ .../domaincapsdata/qemu_5.2.0-q35.x86_64.xml | 10 ++ .../domaincapsdata/qemu_5.2.0-tcg.x86_64.xml | 10 ++ .../qemu_5.2.0-virt.aarch64.xml | 9 ++ tests/domaincapsdata/qemu_5.2.0.aarch64.xml | 9 ++ tests/domaincapsdata/qemu_5.2.0.ppc64.xml | 10 ++ tests/domaincapsdata/qemu_5.2.0.s390x.xml | 4 +++ tests/domaincapsdata/qemu_5.2.0.x86_64.xml| 10 ++ .../domaincapsdata/qemu_6.0.0-q35.x86_64.xml | 10 ++ .../domaincapsdata/qemu_6.0.0-tcg.x86_64.xml | 10 ++ .../qemu_6.0.0-virt.aarch64.xml | 9 ++ tests/domaincapsdata/qemu_6.0.0.aarch64.xml | 9 ++ tests/domaincapsdata/qemu_6.0.0.s390x.xml | 4 +++ tests/domaincapsdata/qemu_6.0.0.x86_64.xml| 10 ++ .../domaincapsdata/qemu_6.1.0-q35.x86_64.xml | 10 ++ .../domaincapsdata/qemu_6.1.0-tcg.x86_64.xml | 10 ++ tests/domaincapsdata/qemu_6.1.0.x86_64.xml| 10 ++ .../domaincapsdata/qemu_6.2.0-q35.x86_64.xml | 10 ++ .../domaincapsdata/qemu_6.2.0-tcg.x86_64.xml | 10 ++ .../qemu_6.2.0-virt.aarch64.xml | 9 ++ tests/domaincapsdata/qemu_6.2.0.aarch64.xml | 9 ++ tests/domaincapsdata/qemu_6.2.0.ppc64.xml | 10 ++ tests/domaincapsdata/qemu_6.2.0.x86_64.xml| 10 ++ tests/domaincapstest.c| 9 ++ 75 files changed, 661 insertions(+) -- 2.33.1
[libvirt PATCH 2/3] qemu: fill in domain capabilities for TPMs
This reports what TPM features QEMU supports, provided that swtpm is
installed in the host.
Signed-off-by: Daniel P. Berrangé
---
src/qemu/qemu_capabilities.c | 32 +++
src/qemu/qemu_capabilities.h | 3 ++
.../domaincapsdata/qemu_2.11.0-q35.x86_64.xml | 1 +
.../domaincapsdata/qemu_2.11.0-tcg.x86_64.xml | 1 +
tests/domaincapsdata/qemu_2.11.0.s390x.xml| 1 +
tests/domaincapsdata/qemu_2.11.0.x86_64.xml | 1 +
.../domaincapsdata/qemu_2.12.0-q35.x86_64.xml | 1 +
.../domaincapsdata/qemu_2.12.0-tcg.x86_64.xml | 1 +
.../qemu_2.12.0-virt.aarch64.xml | 1 +
tests/domaincapsdata/qemu_2.12.0.aarch64.xml | 1 +
tests/domaincapsdata/qemu_2.12.0.ppc64.xml| 1 +
tests/domaincapsdata/qemu_2.12.0.s390x.xml| 1 +
tests/domaincapsdata/qemu_2.12.0.x86_64.xml | 1 +
.../domaincapsdata/qemu_3.0.0-q35.x86_64.xml | 1 +
.../domaincapsdata/qemu_3.0.0-tcg.x86_64.xml | 1 +
tests/domaincapsdata/qemu_3.0.0.ppc64.xml | 1 +
tests/domaincapsdata/qemu_3.0.0.s390x.xml | 1 +
tests/domaincapsdata/qemu_3.0.0.x86_64.xml| 1 +
.../domaincapsdata/qemu_3.1.0-q35.x86_64.xml | 1 +
.../domaincapsdata/qemu_3.1.0-tcg.x86_64.xml | 1 +
tests/domaincapsdata/qemu_3.1.0.ppc64.xml | 1 +
tests/domaincapsdata/qemu_3.1.0.x86_64.xml| 1 +
.../domaincapsdata/qemu_4.0.0-q35.x86_64.xml | 1 +
.../domaincapsdata/qemu_4.0.0-tcg.x86_64.xml | 1 +
.../qemu_4.0.0-virt.aarch64.xml | 1 +
tests/domaincapsdata/qemu_4.0.0.aarch64.xml | 1 +
tests/domaincapsdata/qemu_4.0.0.ppc64.xml | 1 +
tests/domaincapsdata/qemu_4.0.0.s390x.xml | 1 +
tests/domaincapsdata/qemu_4.0.0.x86_64.xml| 1 +
.../domaincapsdata/qemu_4.1.0-q35.x86_64.xml | 1 +
.../domaincapsdata/qemu_4.1.0-tcg.x86_64.xml | 1 +
tests/domaincapsdata/qemu_4.1.0.x86_64.xml| 1 +
.../domaincapsdata/qemu_4.2.0-q35.x86_64.xml | 1 +
.../domaincapsdata/qemu_4.2.0-tcg.x86_64.xml | 1 +
.../qemu_4.2.0-virt.aarch64.xml | 1 +
tests/domaincapsdata/qemu_4.2.0.aarch64.xml | 1 +
tests/domaincapsdata/qemu_4.2.0.ppc64.xml | 1 +
tests/domaincapsdata/qemu_4.2.0.s390x.xml | 1 +
tests/domaincapsdata/qemu_4.2.0.x86_64.xml| 1 +
.../domaincapsdata/qemu_5.0.0-q35.x86_64.xml | 1 +
.../domaincapsdata/qemu_5.0.0-tcg.x86_64.xml | 1 +
.../qemu_5.0.0-virt.aarch64.xml | 1 +
tests/domaincapsdata/qemu_5.0.0.aarch64.xml | 1 +
tests/domaincapsdata/qemu_5.0.0.ppc64.xml | 1 +
tests/domaincapsdata/qemu_5.0.0.x86_64.xml| 1 +
.../domaincapsdata/qemu_5.1.0-q35.x86_64.xml | 1 +
.../domaincapsdata/qemu_5.1.0-tcg.x86_64.xml | 1 +
tests/domaincapsdata/qemu_5.1.0.sparc.xml | 1 +
tests/domaincapsdata/qemu_5.1.0.x86_64.xml| 1 +
.../domaincapsdata/qemu_5.2.0-q35.x86_64.xml | 1 +
.../domaincapsdata/qemu_5.2.0-tcg.x86_64.xml | 1 +
.../qemu_5.2.0-virt.aarch64.xml | 1 +
tests/domaincapsdata/qemu_5.2.0.aarch64.xml | 1 +
tests/domaincapsdata/qemu_5.2.0.ppc64.xml | 1 +
tests/domaincapsdata/qemu_5.2.0.s390x.xml | 1 +
tests/domaincapsdata/qemu_5.2.0.x86_64.xml| 1 +
.../domaincapsdata/qemu_6.0.0-q35.x86_64.xml | 1 +
.../domaincapsdata/qemu_6.0.0-tcg.x86_64.xml | 1 +
.../qemu_6.0.0-virt.aarch64.xml | 1 +
tests/domaincapsdata/qemu_6.0.0.aarch64.xml | 1 +
tests/domaincapsdata/qemu_6.0.0.s390x.xml | 1 +
tests/domaincapsdata/qemu_6.0.0.x86_64.xml| 1 +
.../domaincapsdata/qemu_6.1.0-q35.x86_64.xml | 1 +
.../domaincapsdata/qemu_6.1.0-tcg.x86_64.xml | 1 +
tests/domaincapsdata/qemu_6.1.0.x86_64.xml| 1 +
.../domaincapsdata/qemu_6.2.0-q35.x86_64.xml | 1 +
.../domaincapsdata/qemu_6.2.0-tcg.x86_64.xml | 1 +
.../qemu_6.2.0-virt.aarch64.xml | 1 +
tests/domaincapsdata/qemu_6.2.0.aarch64.xml | 1 +
tests/domaincapsdata/qemu_6.2.0.ppc64.xml | 1 +
tests/domaincapsdata/qemu_6.2.0.x86_64.xml| 1 +
71 files changed, 104 insertions(+)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index a4c492dde2..374909bef2 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -49,6 +49,7 @@
#include "qemu_process.h"
#include "qemu_firmware.h"
#include "virutil.h"
+#include "virtpm.h"
#include
#include
@@ -6206,6 +6207,35 @@ virQEMUCapsFillDomainDeviceFSCaps(virQEMUCaps *qemuCaps,
}
+void
+virQEMUCapsFillDomainDeviceTPMCaps(virQEMUCaps *qemuCaps,
+ virDomainCapsDeviceTPM *tpm)
+{
+if (virTPMEmulatorInit() < 0) {
+virResetLastError();
+tpm->supported = VIR_TRISTATE_BOOL_NO;
+} else {
+tpm->supported = VIR_TRISTATE_BOOL_YES;
+tpm->model.report = true;
+tpm->backendModel.report = true;
+
+if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_TPM_TIS))
+VIR_DOMAIN_CAPS_ENUM_SET(tpm->model, VIR_DOMAIN_TPM_MODEL_TIS);
+if (virQEMUCapsGet(qemuCaps,
Re: [RFC PATCH 1/3] libvirt: Introduce virDomainInjectLaunchSecret public API
On Tue, Nov 16, 2021 at 07:23:52PM -0700, Jim Fehlig wrote: > An API inject a launch secret into the domain's memory. > > Signed-off-by: Jim Fehlig > --- > include/libvirt/libvirt-domain.h | 6 > src/driver-hypervisor.h | 8 + > src/libvirt-domain.c | 50 > src/libvirt_public.syms | 5 > 4 files changed, 69 insertions(+) > > diff --git a/include/libvirt/libvirt-domain.h > b/include/libvirt/libvirt-domain.h > index 2f017c5b68..418ee4bd2d 100644 > --- a/include/libvirt/libvirt-domain.h > +++ b/include/libvirt/libvirt-domain.h > @@ -5091,6 +5091,12 @@ int virDomainGetLaunchSecurityInfo(virDomainPtr domain, > int *nparams, > unsigned int flags); > > +int virDomainInjectLaunchSecret(virDomainPtr domain, > +const char *secrethdr, > +const char *secret, > +unsigned long long injectaddr, > +unsigned int flags); I thought of a better name at last, that shows its relation to virDomainGetLaunchSecurityInfo without implying that they are the direct inverse of each other: virDomainSetLaunchSecurityState(...) Also, we whould bear in mind that the set of state parameters may be differnt for vendors other than AMD, and even later generations of AMD SEV might want more parameters. So lets use a 'virTypedParameter' array for this methodeg virDomainSetLaunchSecurityState(virDomainPtr dom, virTypedParameterPtr params, int nparams, unsigned int flags); 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] qemu: Remove 'else' branches after 'return' or 'goto'
I think it makes no sense to have else branches after return or
goto as it will never reach them in cases it should not. This
patch makes the code more readable (at least to me).
Signed-off-by: Kristina Hanicova
---
src/libvirt.c | 3 ++-
src/qemu/qemu_agent.c | 6 +++---
src/qemu/qemu_alias.c | 9 +
src/qemu/qemu_capabilities.c | 4 ++--
src/qemu/qemu_cgroup.c | 9 +++--
src/qemu/qemu_conf.c | 11 +--
src/qemu/qemu_domain_address.c | 11 +--
src/qemu/qemu_domainjob.c | 3 +--
src/qemu/qemu_driver.c | 32
9 files changed, 42 insertions(+), 46 deletions(-)
diff --git a/src/libvirt.c b/src/libvirt.c
index 80bdcd1db3..ef9fc403d0 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -663,7 +663,8 @@ virStateInitialize(bool privileged,
virStateDriverTab[i]->name,
virGetLastErrorMessage());
return -1;
-} else if (ret == VIR_DRV_STATE_INIT_SKIPPED && mandatory) {
+}
+if (ret == VIR_DRV_STATE_INIT_SKIPPED && mandatory) {
VIR_ERROR(_("Initialization of mandatory %s state driver
skipped"),
virStateDriverTab[i]->name);
return -1;
diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index c3b02569cd..75eb1620c0 100644
--- a/src/qemu/qemu_agent.c
+++ b/src/qemu/qemu_agent.c
@@ -989,8 +989,7 @@ qemuAgentCommandName(virJSONValue *cmd)
const char *name = virJSONValueObjectGetString(cmd, "execute");
if (name)
return name;
-else
-return "";
+return "";
}
static int
@@ -1029,7 +1028,8 @@ qemuAgentCheckError(virJSONValue *cmd,
qemuAgentStringifyError(error));
return -1;
-} else if (!virJSONValueObjectHasKey(reply, "return")) {
+}
+if (!virJSONValueObjectHasKey(reply, "return")) {
g_autofree char *cmdstr = virJSONValueToString(cmd, false);
g_autofree char *replystr = virJSONValueToString(reply, false);
diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c
index a36f346592..5b525415de 100644
--- a/src/qemu/qemu_alias.c
+++ b/src/qemu/qemu_alias.c
@@ -137,7 +137,8 @@ qemuAssignDeviceControllerAlias(virDomainDef *domainDef,
*/
controller->info.alias = g_strdup("pci");
return 0;
-} else if (controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT)
{
+}
+if (controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) {
/* The pcie-root controller on Q35 machinetypes uses a
* different naming convention ("pcie.0"), because it is
* hardcoded that way in qemu.
@@ -151,7 +152,8 @@ qemuAssignDeviceControllerAlias(virDomainDef *domainDef,
*/
controller->info.alias = g_strdup_printf("pci.%d", controller->idx);
return 0;
-} else if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE) {
+}
+if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE) {
/* for any machine based on e.g. I440FX or G3Beige, the
* first (and currently only) IDE controller is an integrated
* controller hardcoded with id "ide"
@@ -821,8 +823,7 @@ qemuAliasForSecret(const char *parentalias,
{
if (obj)
return g_strdup_printf("%s-%s-secret0", parentalias, obj);
-else
-return g_strdup_printf("%s-secret0", parentalias);
+return g_strdup_printf("%s-secret0", parentalias);
}
/* qemuAliasTLSObjFromSrcAlias
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index a4c492dde2..3b63996170 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -773,9 +773,9 @@ const char *virQEMUCapsArchToString(virArch arch)
{
if (arch == VIR_ARCH_I686)
return "i386";
-else if (arch == VIR_ARCH_ARMV6L || arch == VIR_ARCH_ARMV7L)
+if (arch == VIR_ARCH_ARMV6L || arch == VIR_ARCH_ARMV7L)
return "arm";
-else if (arch == VIR_ARCH_OR32)
+if (arch == VIR_ARCH_OR32)
return "or32";
return virArchToString(arch);
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index 1e7b562b33..bcdfa90759 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -608,9 +608,8 @@ qemuSetupBlkioCgroup(virDomainObj *vm)
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("Block I/O tuning is not available on this
host"));
return -1;
-} else {
-return 0;
}
+return 0;
}
return virDomainCgroupSetupBlkio(priv->cgroup, vm->def->blkio);
@@ -629,9 +628,8 @@ qemuSetupMemoryCgroup(virDomainObj *vm)
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("Memory cgroup is not available on this host"));
return -1;
-} else {
-
Re: [libvirt PATCH] ci: display installed packages at start of build
On 11/23/21 17:40, Daniel P. Berrangé wrote: > When a build fails it is helpful to know what packages were installed, > because by the time we look at the build job output, the original > container image might have changed. > > Signed-off-by: Daniel P. Berrangé > --- > .gitlab-ci.yml | 2 ++ > 1 file changed, 2 insertions(+) Reviewed-by: Michal Privoznik Michal
Re: [PATCH] qemu: Remove unnecessary variables and labels
On 11/23/21 17:34, Kristina Hanicova wrote: > This patch removes variables such as 'ret', 'rc' and others which > are easily replaced. Therefore, making the code look cleaner and > easier to understand. > > Signed-off-by: Kristina Hanicova > --- > src/driver.c | 4 +-- > src/qemu/qemu_agent.c | 4 +-- > src/qemu/qemu_alias.c | 14 +++- > src/qemu/qemu_capabilities.c | 21 +-- > src/qemu/qemu_cgroup.c | 7 ++-- > src/qemu/qemu_conf.c | 15 > src/qemu/qemu_domain_address.c | 9 ++--- > src/qemu/qemu_driver.c | 64 -- > 8 files changed, 48 insertions(+), 90 deletions(-) Reviewed-by: Michal Privoznik Michal
[PATCH 1/4] virInterfaceObjListAssignDef: Transfer definition ownership
Upon successful return from virInterfaceObjListAssignDef() the
virInterfaceObj is the owner of secret definition. To make this
ownership transfer even more visible, lets pass the definition as
a double pointer and use g_steal_pointer().
Signed-off-by: Michal Privoznik
---
src/conf/virinterfaceobj.c | 25 +++--
src/conf/virinterfaceobj.h | 2 +-
src/test/test_driver.c | 6 ++
3 files changed, 22 insertions(+), 11 deletions(-)
diff --git a/src/conf/virinterfaceobj.c b/src/conf/virinterfaceobj.c
index c5dfa6c7f5..daac74e88c 100644
--- a/src/conf/virinterfaceobj.c
+++ b/src/conf/virinterfaceobj.c
@@ -377,9 +377,8 @@ virInterfaceObjListCloneCb(void *payload,
goto error;
VIR_FREE(xml);
-if (!(obj = virInterfaceObjListAssignDef(data->dest, backup)))
+if (!(obj = virInterfaceObjListAssignDef(data->dest, )))
goto error;
-backup = NULL;
virInterfaceObjEndAPI();
virObjectUnlock(srcObj);
@@ -420,25 +419,39 @@ virInterfaceObjListClone(virInterfaceObjList *interfaces)
}
+/**
+ * virInterfaceObjListAssignDef:
+ * @interfaces: virInterface object list
+ * @def: new definition
+ *
+ * Assigns new definition to either an existing or newly created
+ * virInterface object. Upon successful return the virInterface
+ * object is the owner of @def and callers should use
+ * virInterfaceObjGetDef() if they need to access the definition
+ * as @def is set to NULL.
+ *
+ * Returns: a virInterface object instance on success, or
+ * NULL on error.
+ */
virInterfaceObj *
virInterfaceObjListAssignDef(virInterfaceObjList *interfaces,
- virInterfaceDef *def)
+ virInterfaceDef **def)
{
virInterfaceObj *obj;
virObjectRWLockWrite(interfaces);
-if ((obj = virInterfaceObjListFindByNameLocked(interfaces, def->name))) {
+if ((obj = virInterfaceObjListFindByNameLocked(interfaces, (*def)->name)))
{
virInterfaceDefFree(obj->def);
} else {
if (!(obj = virInterfaceObjNew()))
goto error;
-if (virHashAddEntry(interfaces->objsName, def->name, obj) < 0)
+if (virHashAddEntry(interfaces->objsName, (*def)->name, obj) < 0)
goto error;
virObjectRef(obj);
}
-obj->def = def;
+obj->def = g_steal_pointer(def);
virObjectRWUnlock(interfaces);
return obj;
diff --git a/src/conf/virinterfaceobj.h b/src/conf/virinterfaceobj.h
index d60faa95f2..5927484167 100644
--- a/src/conf/virinterfaceobj.h
+++ b/src/conf/virinterfaceobj.h
@@ -59,7 +59,7 @@ virInterfaceObjListClone(virInterfaceObjList *interfaces);
virInterfaceObj *
virInterfaceObjListAssignDef(virInterfaceObjList *interfaces,
- virInterfaceDef *def);
+ virInterfaceDef **def);
void
virInterfaceObjListRemove(virInterfaceObjList *interfaces,
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index e45748c5fc..92c0462170 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -1141,9 +1141,8 @@ testParseInterfaces(testDriver *privconn,
if (!def)
return -1;
-if (!(obj = virInterfaceObjListAssignDef(privconn->ifaces, def)))
+if (!(obj = virInterfaceObjListAssignDef(privconn->ifaces, )))
return -1;
-def = NULL;
virInterfaceObjSetActive(obj, true);
virInterfaceObjEndAPI();
@@ -6203,9 +6202,8 @@ testInterfaceDefineXML(virConnectPtr conn,
if ((def = virInterfaceDefParseString(xmlStr, flags)) == NULL)
goto cleanup;
-if ((obj = virInterfaceObjListAssignDef(privconn->ifaces, def)) == NULL)
+if ((obj = virInterfaceObjListAssignDef(privconn->ifaces, )) == NULL)
goto cleanup;
-def = NULL;
objdef = virInterfaceObjGetDef(obj);
ret = virGetInterface(conn, objdef->name, objdef->mac);
--
2.32.0
[PATCH 4/4] virDomainObjListAdd: Transfer definition ownership
Upon successful return from virDomainObjListAdd() the
virDomainObj is the owner of secret definition. To make this
ownership transfer even more visible, lets pass the definition as
a double pointer and use g_steal_pointer().
Signed-off-by: Michal Privoznik
---
src/bhyve/bhyve_driver.c| 6 ++
src/ch/ch_driver.c | 7 ++-
src/conf/domain_conf.c | 8
src/conf/domain_conf.h | 2 +-
src/conf/virdomainobjlist.c | 22 +-
src/conf/virdomainobjlist.h | 2 +-
src/libxl/libxl_domain.c| 3 +--
src/libxl/libxl_driver.c| 30 ++
src/libxl/libxl_migration.c | 6 ++
src/lxc/lxc_driver.c| 24
src/openvz/openvz_conf.c| 3 +--
src/openvz/openvz_driver.c | 10 --
src/qemu/qemu_driver.c | 30 ++
src/qemu/qemu_migration.c | 3 +--
src/qemu/qemu_snapshot.c| 9 +++--
src/test/test_driver.c | 17 +++--
src/vmware/vmware_conf.c| 6 ++
src/vmware/vmware_driver.c | 10 --
18 files changed, 76 insertions(+), 122 deletions(-)
diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c
index 516490f6cd..eccf9b44a8 100644
--- a/src/bhyve/bhyve_driver.c
+++ b/src/bhyve/bhyve_driver.c
@@ -533,11 +533,10 @@ bhyveDomainDefineXMLFlags(virConnectPtr conn, const char
*xml, unsigned int flag
if (bhyveDomainAssignAddresses(def, NULL) < 0)
goto cleanup;
-if (!(vm = virDomainObjListAdd(privconn->domains, def,
+if (!(vm = virDomainObjListAdd(privconn->domains, ,
privconn->xmlopt,
0, )))
goto cleanup;
-def = NULL;
vm->persistent = 1;
if (virDomainDefSave(vm->newDef ? vm->newDef : vm->def,
@@ -915,12 +914,11 @@ bhyveDomainCreateXML(virConnectPtr conn,
if (bhyveDomainAssignAddresses(def, NULL) < 0)
goto cleanup;
-if (!(vm = virDomainObjListAdd(privconn->domains, def,
+if (!(vm = virDomainObjListAdd(privconn->domains, ,
privconn->xmlopt,
VIR_DOMAIN_OBJ_LIST_ADD_LIVE |
VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, NULL)))
goto cleanup;
-def = NULL;
if (virBhyveProcessStart(conn, vm,
VIR_DOMAIN_RUNNING_BOOTED,
diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c
index 9eaf3ee499..464bcef907 100644
--- a/src/ch/ch_driver.c
+++ b/src/ch/ch_driver.c
@@ -237,15 +237,13 @@ chDomainCreateXML(virConnectPtr conn,
goto cleanup;
if (!(vm = virDomainObjListAdd(driver->domains,
- vmdef,
+ ,
driver->xmlopt,
VIR_DOMAIN_OBJ_LIST_ADD_LIVE |
VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE,
NULL)))
goto cleanup;
-vmdef = NULL;
-
if (virCHDomainObjBeginJob(vm, CH_JOB_MODIFY) < 0)
goto cleanup;
@@ -323,12 +321,11 @@ chDomainDefineXMLFlags(virConnectPtr conn, const char
*xml, unsigned int flags)
if (virDomainDefineXMLFlagsEnsureACL(conn, vmdef) < 0)
goto cleanup;
-if (!(vm = virDomainObjListAdd(driver->domains, vmdef,
+if (!(vm = virDomainObjListAdd(driver->domains, ,
driver->xmlopt,
0, NULL)))
goto cleanup;
-vmdef = NULL;
vm->persistent = 1;
dom = virGetDomain(conn, vm->def->name, vm->def->uuid, vm->def->id);
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 552d43b845..9b04071259 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -3865,7 +3865,7 @@ virDomainDefNew(virDomainXMLOption *xmlopt)
void virDomainObjAssignDef(virDomainObj *domain,
- virDomainDef *def,
+ virDomainDef **def,
bool live,
virDomainDef **oldDef)
{
@@ -3876,7 +3876,7 @@ void virDomainObjAssignDef(virDomainObj *domain,
*oldDef = domain->newDef;
else
virDomainDefFree(domain->newDef);
-domain->newDef = def;
+domain->newDef = g_steal_pointer(def);
} else {
if (live) {
/* save current configuration to be restored on domain shutdown */
@@ -3884,13 +3884,13 @@ void virDomainObjAssignDef(virDomainObj *domain,
domain->newDef = domain->def;
else
virDomainDefFree(domain->def);
-domain->def = def;
+domain->def = g_steal_pointer(def);
} else {
if (oldDef)
*oldDef = domain->def;
else
virDomainDefFree(domain->def);
-domain->def = def;
+
[PATCH 3/4] virStoragePoolObjListAdd: Transfer definition ownership
Upon successful return from virStoragePoolObjListAdd() the
virStoragePoolObj is the owner of secret definition. To make this
ownership transfer even more visible, lets pass the definition as
a double pointer and use g_steal_pointer().
Signed-off-by: Michal Privoznik
---
src/conf/virstorageobj.c | 29 -
src/conf/virstorageobj.h | 2 +-
src/storage/storage_driver.c | 5 ++---
src/test/test_driver.c | 8 +++-
4 files changed, 22 insertions(+), 22 deletions(-)
diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c
index f906c5b141..2c29339b6a 100644
--- a/src/conf/virstorageobj.c
+++ b/src/conf/virstorageobj.c
@@ -1513,20 +1513,20 @@
virStoragePoolObjSourceFindDuplicate(virStoragePoolObjList *pools,
static void
virStoragePoolObjAssignDef(virStoragePoolObj *obj,
- virStoragePoolDef *def,
+ virStoragePoolDef **def,
unsigned int flags)
{
if (virStoragePoolObjIsActive(obj) ||
virStoragePoolObjIsStarting(obj)) {
virStoragePoolDefFree(obj->newDef);
-obj->newDef = def;
+obj->newDef = g_steal_pointer(def);
} else {
if (!obj->newDef &&
flags & VIR_STORAGE_POOL_OBJ_LIST_ADD_LIVE)
obj->newDef = g_steal_pointer(>def);
virStoragePoolDefFree(obj->def);
-obj->def = def;
+obj->def = g_steal_pointer(def);
}
}
@@ -1548,11 +1548,16 @@ virStoragePoolObjAssignDef(virStoragePoolObj *obj,
* If VIR_STORAGE_POOL_OBJ_LIST_ADD_CHECK_LIVE is set in @flags
* then this will fail if the pool exists and is active.
*
+ * Upon successful return the virStoragePool object is the owner
+ * of @def and callers should use virStoragePoolObjGetDef() or
+ * virStoragePoolObjGetNewDef() if they need to access the
+ * definition as @def is set to NULL.
+ *
* Returns locked and reffed object pointer or NULL on error
*/
virStoragePoolObj *
virStoragePoolObjListAdd(virStoragePoolObjList *pools,
- virStoragePoolDef *def,
+ virStoragePoolDef **def,
unsigned int flags)
{
virStoragePoolObj *obj = NULL;
@@ -1561,10 +1566,10 @@ virStoragePoolObjListAdd(virStoragePoolObjList *pools,
virObjectRWLockWrite(pools);
-if (virStoragePoolObjSourceFindDuplicate(pools, def) < 0)
+if (virStoragePoolObjSourceFindDuplicate(pools, *def) < 0)
goto error;
-rc = virStoragePoolObjIsDuplicate(pools, def,
+rc = virStoragePoolObjIsDuplicate(pools, *def,
!!(flags &
VIR_STORAGE_POOL_OBJ_LIST_ADD_CHECK_LIVE),
);
@@ -1579,17 +1584,17 @@ virStoragePoolObjListAdd(virStoragePoolObjList *pools,
if (!(obj = virStoragePoolObjNew()))
goto error;
-virUUIDFormat(def->uuid, uuidstr);
+virUUIDFormat((*def)->uuid, uuidstr);
if (virHashAddEntry(pools->objs, uuidstr, obj) < 0)
goto error;
virObjectRef(obj);
-if (virHashAddEntry(pools->objsName, def->name, obj) < 0) {
+if (virHashAddEntry(pools->objsName, (*def)->name, obj) < 0) {
virHashRemoveEntry(pools->objs, uuidstr);
goto error;
}
virObjectRef(obj);
-obj->def = def;
+obj->def = g_steal_pointer(def);
virObjectRWUnlock(pools);
return obj;
@@ -1620,9 +1625,8 @@ virStoragePoolObjLoad(virStoragePoolObjList *pools,
return NULL;
}
-if (!(obj = virStoragePoolObjListAdd(pools, def, 0)))
+if (!(obj = virStoragePoolObjListAdd(pools, , 0)))
return NULL;
-def = NULL;
VIR_FREE(obj->configFile); /* for driver reload */
obj->configFile = g_strdup(path);
@@ -1673,10 +1677,9 @@ virStoragePoolObjLoadState(virStoragePoolObjList *pools,
}
/* create the object */
-if (!(obj = virStoragePoolObjListAdd(pools, def,
+if (!(obj = virStoragePoolObjListAdd(pools, ,
VIR_STORAGE_POOL_OBJ_LIST_ADD_CHECK_LIVE)))
return NULL;
-def = NULL;
/* XXX: future handling of some additional useful status data,
* for now, if a status file for a pool exists, the pool will be marked
diff --git a/src/conf/virstorageobj.h b/src/conf/virstorageobj.h
index 65f2ae8175..523bdec244 100644
--- a/src/conf/virstorageobj.h
+++ b/src/conf/virstorageobj.h
@@ -203,7 +203,7 @@ typedef enum {
virStoragePoolObj *
virStoragePoolObjListAdd(virStoragePoolObjList *pools,
- virStoragePoolDef *def,
+ virStoragePoolDef **def,
unsigned int flags);
int
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index 51daf6a05d..4df2c75a2b 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -746,11 +746,10 @@ storagePoolCreateXML(virConnectPtr conn,
if ((backend =
[PATCH 2/4] virSecretObjListAdd: Transfer definition ownership
Upon successful return from virSecretObjListAdd() the
virSecretObj is the owner of secret definition. To make this
ownership transfer even more visible, lets pass the definition as
a double pointer and use g_steal_pointer().
Signed-off-by: Michal Privoznik
---
src/conf/virsecretobj.c| 26 ++
src/conf/virsecretobj.h| 2 +-
src/secret/secret_driver.c | 4 ++--
3 files changed, 17 insertions(+), 15 deletions(-)
diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c
index 212cfe103c..3fbee6f6ec 100644
--- a/src/conf/virsecretobj.c
+++ b/src/conf/virsecretobj.c
@@ -313,13 +313,16 @@ virSecretObjListRemove(virSecretObjList *secrets,
* @configDir: directory to place secret config files
* @oldDef: Former secret def (e.g. a reload path perhaps)
*
- * Add the new @newdef to the secret obj table hash
+ * Add the new @newdef to the secret obj table hash. Upon
+ * successful the virSecret object is the owner of @newdef and
+ * callers should use virSecretObjGetDef() if they need to access
+ * the definition as @newdef is set to NULL.
*
* Returns: locked and ref'd secret or NULL if failure to add
*/
virSecretObj *
virSecretObjListAdd(virSecretObjList *secrets,
-virSecretDef *newdef,
+virSecretDef **newdef,
const char *configDir,
virSecretDef **oldDef)
{
@@ -333,14 +336,14 @@ virSecretObjListAdd(virSecretObjList *secrets,
if (oldDef)
*oldDef = NULL;
-virUUIDFormat(newdef->uuid, uuidstr);
+virUUIDFormat((*newdef)->uuid, uuidstr);
/* Is there a secret already matching this UUID */
if ((obj = virSecretObjListFindByUUIDLocked(secrets, uuidstr))) {
virObjectLock(obj);
objdef = obj->def;
-if (STRNEQ_NULLABLE(objdef->usage_id, newdef->usage_id)) {
+if (STRNEQ_NULLABLE(objdef->usage_id, (*newdef)->usage_id)) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("a secret with UUID %s is already defined for "
"use with %s"),
@@ -348,7 +351,7 @@ virSecretObjListAdd(virSecretObjList *secrets,
goto cleanup;
}
-if (objdef->isprivate && !newdef->isprivate) {
+if (objdef->isprivate && !(*newdef)->isprivate) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("cannot change private flag on existing secret"));
goto cleanup;
@@ -358,20 +361,20 @@ virSecretObjListAdd(virSecretObjList *secrets,
*oldDef = objdef;
else
virSecretDefFree(objdef);
-obj->def = newdef;
+obj->def = g_steal_pointer(newdef);
} else {
/* No existing secret with same UUID,
* try look for matching usage instead */
if ((obj = virSecretObjListFindByUsageLocked(secrets,
- newdef->usage_type,
- newdef->usage_id))) {
+ (*newdef)->usage_type,
+ (*newdef)->usage_id))) {
virObjectLock(obj);
objdef = obj->def;
virUUIDFormat(objdef->uuid, uuidstr);
virReportError(VIR_ERR_INTERNAL_ERROR,
_("a secret with UUID %s already defined for "
"use with %s"),
- uuidstr, newdef->usage_id);
+ uuidstr, (*newdef)->usage_id);
goto cleanup;
}
@@ -388,7 +391,7 @@ virSecretObjListAdd(virSecretObjList *secrets,
if (virHashAddEntry(secrets->objs, uuidstr, obj) < 0)
goto cleanup;
-obj->def = newdef;
+obj->def = g_steal_pointer(newdef);
virObjectRef(obj);
}
@@ -874,9 +877,8 @@ virSecretLoad(virSecretObjList *secrets,
if (virSecretLoadValidateUUID(def, file) < 0)
goto cleanup;
-if (!(obj = virSecretObjListAdd(secrets, def, configDir, NULL)))
+if (!(obj = virSecretObjListAdd(secrets, , configDir, NULL)))
goto cleanup;
-def = NULL;
if (virSecretLoadValue(obj) < 0) {
virSecretObjListRemove(secrets, obj);
diff --git a/src/conf/virsecretobj.h b/src/conf/virsecretobj.h
index e91b9518eb..93b4a46acf 100644
--- a/src/conf/virsecretobj.h
+++ b/src/conf/virsecretobj.h
@@ -50,7 +50,7 @@ virSecretObjListRemove(virSecretObjList *secrets,
virSecretObj *
virSecretObjListAdd(virSecretObjList *secrets,
-virSecretDef *newdef,
+virSecretDef **newdef,
const char *configDir,
virSecretDef **oldDef);
diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c
index 43aeae9568..de635bba3a 100644
--- a/src/secret/secret_driver.c
+++ b/src/secret/secret_driver.c
@@ -228,10 +228,10 @@
[PATCH 0/4] virXXXListAdd: Transfer definition ownership
At a lot of places we have the following pattern: virXXXDef *def = parseDef(); if (!(obj = virXXXObjListAdd(def))) goto clenaup; def = NULL; cleanup: virXXXDefFree(def); The 'def = NULL' step is necessary because the ownership of the definition was transferred onto the object. Well, this approach is fragile as it relies on developers remembering to set the variable explicitly. If the virXXXObjListAdd() would take address of @def then the explicit set to NULL can be left out. Please note, I've reworked only a few virXXXObjListAdd() functions to see whether these are desired or not. If merged, I can post patches for the rest. Michal Prívozník (4): virInterfaceObjListAssignDef: Transfer definition ownership virSecretObjListAdd: Transfer definition ownership virStoragePoolObjListAdd: Transfer definition ownership virDomainObjListAdd: Transfer definition ownership src/bhyve/bhyve_driver.c | 6 ++ src/ch/ch_driver.c | 7 ++- src/conf/domain_conf.c | 8 src/conf/domain_conf.h | 2 +- src/conf/virdomainobjlist.c | 22 +- src/conf/virdomainobjlist.h | 2 +- src/conf/virinterfaceobj.c | 25 +++-- src/conf/virinterfaceobj.h | 2 +- src/conf/virsecretobj.c | 26 ++ src/conf/virsecretobj.h | 2 +- src/conf/virstorageobj.c | 29 - src/conf/virstorageobj.h | 2 +- src/libxl/libxl_domain.c | 3 +-- src/libxl/libxl_driver.c | 30 ++ src/libxl/libxl_migration.c | 6 ++ src/lxc/lxc_driver.c | 24 src/openvz/openvz_conf.c | 3 +-- src/openvz/openvz_driver.c | 10 -- src/qemu/qemu_driver.c | 30 ++ src/qemu/qemu_migration.c| 3 +-- src/qemu/qemu_snapshot.c | 9 +++-- src/secret/secret_driver.c | 4 ++-- src/storage/storage_driver.c | 5 ++--- src/test/test_driver.c | 31 --- src/vmware/vmware_conf.c | 6 ++ src/vmware/vmware_driver.c | 10 -- 26 files changed, 137 insertions(+), 170 deletions(-) -- 2.32.0
[libvirt PATCH] ci: display installed packages at start of build
When a build fails it is helpful to know what packages were installed, because by the time we look at the build job output, the original container image might have changed. Signed-off-by: Daniel P. Berrangé --- .gitlab-ci.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index d486faca58..6ba11a0431 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -24,6 +24,7 @@ include: '/ci/gitlab.yml' key: "$CI_JOB_NAME" before_script: - *script_variables +- cat /packages.txt script: - meson setup build --werror $MESON_ARGS || (cat build/meson-logs/meson-log.txt && exit 1) - meson dist -C build --no-tests @@ -43,6 +44,7 @@ include: '/ci/gitlab.yml' key: "$CI_JOB_NAME" before_script: - *script_variables +- cat /packages.txt script: - meson setup build --werror $MESON_OPTS || (cat build/meson-logs/meson-log.txt && exit 1) - meson compile -C build -- 2.33.1
[PATCH] qemu: Remove unnecessary variables and labels
This patch removes variables such as 'ret', 'rc' and others which
are easily replaced. Therefore, making the code look cleaner and
easier to understand.
Signed-off-by: Kristina Hanicova
---
src/driver.c | 4 +--
src/qemu/qemu_agent.c | 4 +--
src/qemu/qemu_alias.c | 14 +++-
src/qemu/qemu_capabilities.c | 21 +--
src/qemu/qemu_cgroup.c | 7 ++--
src/qemu/qemu_conf.c | 15
src/qemu/qemu_domain_address.c | 9 ++---
src/qemu/qemu_driver.c | 64 --
8 files changed, 48 insertions(+), 90 deletions(-)
diff --git a/src/driver.c b/src/driver.c
index 329d493a50..9ae95cb4c3 100644
--- a/src/driver.c
+++ b/src/driver.c
@@ -52,7 +52,6 @@ virDriverLoadModule(const char *name,
bool required)
{
g_autofree char *modfile = NULL;
-int ret;
VIR_DEBUG("Module load %s", name);
@@ -64,8 +63,7 @@ virDriverLoadModule(const char *name,
"LIBVIRT_DRIVER_DIR")))
return -1;
-ret = virModuleLoad(modfile, regfunc, required);
-return ret;
+return virModuleLoad(modfile, regfunc, required);
}
diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index c3b02569cd..08436a9705 100644
--- a/src/qemu/qemu_agent.c
+++ b/src/qemu/qemu_agent.c
@@ -176,7 +176,6 @@ qemuAgentOpenUnix(const char *socketpath)
{
struct sockaddr_un addr;
int agentfd;
-int ret = -1;
if ((agentfd = socket(AF_UNIX, SOCK_STREAM, 0)) < 0) {
virReportSystemError(errno,
@@ -199,8 +198,7 @@ qemuAgentOpenUnix(const char *socketpath)
goto error;
}
-ret = connect(agentfd, (struct sockaddr *), sizeof(addr));
-if (ret < 0) {
+if (connect(agentfd, (struct sockaddr *), sizeof(addr)) < 0) {
virReportSystemError(errno, "%s",
_("failed to connect to agent socket"));
goto error;
diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c
index a36f346592..5e35f43614 100644
--- a/src/qemu/qemu_alias.c
+++ b/src/qemu/qemu_alias.c
@@ -744,16 +744,13 @@ qemuAssignDeviceAliases(virDomainDef *def, virQEMUCaps
*qemuCaps)
char *
qemuAliasDiskDriveFromDisk(const virDomainDiskDef *disk)
{
-char *ret;
-
if (!disk->info.alias) {
virReportError(VIR_ERR_INVALID_ARG, "%s",
_("disk does not have an alias"));
return NULL;
}
-ret = g_strdup_printf("%s%s", QEMU_DRIVE_HOST_PREFIX, disk->info.alias);
-return ret;
+return g_strdup_printf("%s%s", QEMU_DRIVE_HOST_PREFIX, disk->info.alias);
}
@@ -780,18 +777,15 @@ qemuAliasDiskDriveSkipPrefix(const char *dev_name)
char *
qemuAliasFromHostdev(const virDomainHostdevDef *hostdev)
{
-char *ret;
-
if (!hostdev->info->alias) {
virReportError(VIR_ERR_INVALID_ARG, "%s",
_("hostdev does not have an alias"));
return NULL;
}
-ret = g_strdup_printf("%s-%s",
-
virDomainDeviceAddressTypeToString(hostdev->info->type),
- hostdev->info->alias);
-return ret;
+return g_strdup_printf("%s-%s",
+
virDomainDeviceAddressTypeToString(hostdev->info->type),
+ hostdev->info->alias);
}
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index a4c492dde2..33797469a6 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -3070,7 +3070,6 @@ virQEMUCapsGetCPUFeatures(virQEMUCaps *qemuCaps,
g_auto(GStrv) list = NULL;
size_t i;
size_t n;
-int ret = -1;
*features = NULL;
modelInfo = virQEMUCapsGetCPUModelInfo(qemuCaps, virtType);
@@ -3091,12 +3090,10 @@ virQEMUCapsGetCPUFeatures(virQEMUCaps *qemuCaps,
}
*features = g_steal_pointer();
-if (migratable && !modelInfo->migratability)
-ret = 1;
-else
-ret = 0;
-return ret;
+if (migratable && !modelInfo->migratability)
+return 1;
+return 0;
}
@@ -5237,15 +5234,13 @@ virQEMUCapsProbeQMPSchemaCapabilities(virQEMUCaps
*qemuCaps,
virDomainVirtType
virQEMUCapsGetVirtType(virQEMUCaps *qemuCaps)
{
-virDomainVirtType type;
if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM))
-type = VIR_DOMAIN_VIRT_KVM;
-else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_TCG))
-type = VIR_DOMAIN_VIRT_QEMU;
-else
-type = VIR_DOMAIN_VIRT_NONE;
+return VIR_DOMAIN_VIRT_KVM;
+
+if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_TCG))
+return VIR_DOMAIN_VIRT_QEMU;
-return type;
+return VIR_DOMAIN_VIRT_NONE;
}
int
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index 1e7b562b33..c07058d2f8 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -336,18 +336,15 @@ static int
qemuSetupTPMCgroup(virDomainObj *vm,
virDomainTPMDef *dev)
{
-int ret = 0;
-
Re: [libvirt PATCH 4/4] tests: pcivpdtest: check return value of virCreateAnonymousFile
On Tue, Nov 23, 2021 at 04:49:52PM +0100, Ján Tomko wrote:
On a Tuesday in 2021, Martin Kletzander wrote:
On Tue, Nov 23, 2021 at 03:40:46PM +0100, Kristina Hanicova wrote:
On Tue, Nov 23, 2021 at 3:20 PM Ján Tomko wrote:
Fixes: 59c1bc3a0e25e6d725db41990f11e0b53137115d
Fixes: 43820e4b8037680ec451761216750c6b139db67a
Fixes: 600f580d623ae4077ddeb6c7cb24f8a315a7c73b
Signed-off-by: Ján Tomko
---
tests/virpcivpdtest.c | 20
1 file changed, 20 insertions(+)
diff --git a/tests/virpcivpdtest.c b/tests/virpcivpdtest.c
index 284350fe29..62c51cdeb9 100644
--- a/tests/virpcivpdtest.c
+++ b/tests/virpcivpdtest.c
@@ -446,6 +446,8 @@ testVirPCIVPDReadVPDBytes(const void *opaque
G_GNUC_UNUSED)
buf = g_malloc0(dataLen);
fd = virCreateAnonymousFile(fullVPDExample, dataLen);
+if (fd < 0)
+return -1;
I would prefer if you rewrote this before merging as:
if ((fd = virCreateAnonymousFile(fullVPDExample, dataLen)) < 0)
return -1;
(The whole patch.) I think it looks cleaner, but that's just my preference.
It might look cleaner for some until you get to the point of having more
parentheses there and we already had some issues with that. I, for one,
prefer an extra line or even an extra variable in some cases, but
similarly to you it is only my preference and we do not have a coding
style for this, unfortunately. We even have your preferred style used
in the libvirt coding style page [0]. And guess what, we have that
I got a SIGSEGV trying to access [0], so I'll go with the other
suggestion.
lol, it was a lazy reference, only gets resolved when someone wants to
access it, I guess you do not have userfaultd support (although one
might argue that I am the living proof of userfault-duh in this case).
[0] https://libvirt.org/coding-style.html#semicolons
wrong! Look:
while ((rc = waitpid(pid, , 0) == -1) &&
errno == EINTR); // ok
while ((rc = waitpid(pid, , 0) == -1) &&
errno == EINTR) { // Better
/* nothing */
}
I would hope that proves my point, but because we had this discussion a
couple of times already I know there are lot of libvirt developers
(probably in the majority) who prefer cramming as much into that one
line, so I guess we won't achieve a consensus on this one ;)
Just my €.02 ;)
Thank you for your 0,51 Kč ;)
Jano
Have a nice day,
Martin
Re: [libvirt PATCH 4/4] tests: pcivpdtest: check return value of virCreateAnonymousFile
On a Tuesday in 2021, Martin Kletzander wrote:
On Tue, Nov 23, 2021 at 03:40:46PM +0100, Kristina Hanicova wrote:
On Tue, Nov 23, 2021 at 3:20 PM Ján Tomko wrote:
Fixes: 59c1bc3a0e25e6d725db41990f11e0b53137115d
Fixes: 43820e4b8037680ec451761216750c6b139db67a
Fixes: 600f580d623ae4077ddeb6c7cb24f8a315a7c73b
Signed-off-by: Ján Tomko
---
tests/virpcivpdtest.c | 20
1 file changed, 20 insertions(+)
diff --git a/tests/virpcivpdtest.c b/tests/virpcivpdtest.c
index 284350fe29..62c51cdeb9 100644
--- a/tests/virpcivpdtest.c
+++ b/tests/virpcivpdtest.c
@@ -446,6 +446,8 @@ testVirPCIVPDReadVPDBytes(const void *opaque
G_GNUC_UNUSED)
buf = g_malloc0(dataLen);
fd = virCreateAnonymousFile(fullVPDExample, dataLen);
+if (fd < 0)
+return -1;
I would prefer if you rewrote this before merging as:
if ((fd = virCreateAnonymousFile(fullVPDExample, dataLen)) < 0)
return -1;
(The whole patch.) I think it looks cleaner, but that's just my preference.
It might look cleaner for some until you get to the point of having more
parentheses there and we already had some issues with that. I, for one,
prefer an extra line or even an extra variable in some cases, but
similarly to you it is only my preference and we do not have a coding
style for this, unfortunately. We even have your preferred style used
in the libvirt coding style page [0]. And guess what, we have that
I got a SIGSEGV trying to access [0], so I'll go with the other
suggestion.
wrong! Look:
while ((rc = waitpid(pid, , 0) == -1) &&
errno == EINTR); // ok
while ((rc = waitpid(pid, , 0) == -1) &&
errno == EINTR) { // Better
/* nothing */
}
I would hope that proves my point, but because we had this discussion a
couple of times already I know there are lot of libvirt developers
(probably in the majority) who prefer cramming as much into that one
line, so I guess we won't achieve a consensus on this one ;)
Just my €.02 ;)
Thank you for your 0,51 Kč ;)
Jano
Have a nice day,
Martin
signature.asc
Description: PGP signature
Re: [libvirt PATCH 0/3] ci: improve mingw coverage
On 11/23/21 14:51, Daniel P. Berrangé wrote:
> This ensures at least one mingw job is gating for pipelines
>
> Daniel P. Berrangé (3):
> ci: replace Fedora 33 with Fedora 35
> ci: refresh variables/dockerfiles with latest content
> ci: run a mingw64 job on stable Fedora
>
> ci/cirrus/freebsd-12.vars | 7 +-
> ci/cirrus/freebsd-13.vars | 7 +-
> ci/cirrus/freebsd-current.vars| 7 +-
> ci/cirrus/macos-11.vars | 7 +-
> ci/containers/centos-8.Dockerfile | 7 +-
> ci/containers/centos-stream-8.Dockerfile | 7 +-
> .../fedora-35-cross-mingw32.Dockerfile| 92 +++
> .../fedora-35-cross-mingw64.Dockerfile| 92 +++
> ...ora-33.Dockerfile => fedora-35.Dockerfile} | 2 +-
> .../fedora-rawhide-cross-mingw32.Dockerfile | 2 +-
> .../fedora-rawhide-cross-mingw64.Dockerfile | 2 +-
> ci/containers/fedora-rawhide.Dockerfile | 2 +-
> ci/containers/opensuse-tumbleweed.Dockerfile | 2 +-
> ci/gitlab.yml | 50 ++
> ci/manifest.yml | 13 ++-
> 15 files changed, 254 insertions(+), 45 deletions(-)
> create mode 100644 ci/containers/fedora-35-cross-mingw32.Dockerfile
> create mode 100644 ci/containers/fedora-35-cross-mingw64.Dockerfile
> rename ci/containers/{fedora-33.Dockerfile => fedora-35.Dockerfile} (98%)
>
Reviewed-by: Michal Privoznik
Michal
Re: [libvirt PATCH 4/4] tests: pcivpdtest: check return value of virCreateAnonymousFile
On Tue, Nov 23, 2021 at 03:40:46PM +0100, Kristina Hanicova wrote:
On Tue, Nov 23, 2021 at 3:20 PM Ján Tomko wrote:
Fixes: 59c1bc3a0e25e6d725db41990f11e0b53137115d
Fixes: 43820e4b8037680ec451761216750c6b139db67a
Fixes: 600f580d623ae4077ddeb6c7cb24f8a315a7c73b
Signed-off-by: Ján Tomko
---
tests/virpcivpdtest.c | 20
1 file changed, 20 insertions(+)
diff --git a/tests/virpcivpdtest.c b/tests/virpcivpdtest.c
index 284350fe29..62c51cdeb9 100644
--- a/tests/virpcivpdtest.c
+++ b/tests/virpcivpdtest.c
@@ -446,6 +446,8 @@ testVirPCIVPDReadVPDBytes(const void *opaque
G_GNUC_UNUSED)
buf = g_malloc0(dataLen);
fd = virCreateAnonymousFile(fullVPDExample, dataLen);
+if (fd < 0)
+return -1;
I would prefer if you rewrote this before merging as:
if ((fd = virCreateAnonymousFile(fullVPDExample, dataLen)) < 0)
return -1;
(The whole patch.) I think it looks cleaner, but that's just my preference.
It might look cleaner for some until you get to the point of having more
parentheses there and we already had some issues with that. I, for one,
prefer an extra line or even an extra variable in some cases, but
similarly to you it is only my preference and we do not have a coding
style for this, unfortunately. We even have your preferred style used
in the libvirt coding style page [0]. And guess what, we have that
wrong! Look:
while ((rc = waitpid(pid, , 0) == -1) &&
errno == EINTR); // ok
while ((rc = waitpid(pid, , 0) == -1) &&
errno == EINTR) { // Better
/* nothing */
}
I would hope that proves my point, but because we had this discussion a
couple of times already I know there are lot of libvirt developers
(probably in the majority) who prefer cramming as much into that one
line, so I guess we won't achieve a consensus on this one ;)
Just my €.02 ;)
Have a nice day,
Martin
signature.asc
Description: PGP signature
Re: [PATCH v3 2/2] Use virProcessGetStat
On a Tuesday in 2021, Martin Kletzander wrote:
This eliminates one incorrect parsing implementation which relied on the
command field not having a closing bracket. This possibility is already
tested against in the virProcessGetStat() tests.
Signed-off-by: Martin Kletzander
---
src/qemu/qemu_driver.c | 34 ++
src/util/virprocess.c | 48 ++
2 files changed, 13 insertions(+), 69 deletions(-)
[..]
-tokens = g_strsplit(tmp, " ", 0);
-if (!tokens ||
-g_strv_length(tokens) < 20) {
+if (virStrToLong_ull(proc_stat[21], NULL, 10, timestamp) < 0) {
VIR_PROCESS_STAT_STARTTIME
virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Cannot find start time in %s"),
- filename);
+ _("Cannot parse start time %s for pid %d"),
+ proc_stat[21], (int)pid);
return -1;
}
Reviewed-by: Ján Tomko
Jano
signature.asc
Description: PGP signature
Re: [PATCH v3 1/2] util: Add virProcessGetStat
On a Tuesday in 2021, Martin Kletzander wrote: This reads and separates all fields from /proc//stat or /proc//task//stat as there are easy mistakes to be done in the implementation. Some tests are added to show it works correctly. No number parsing is done as it would be unused for most of the fields most, if not all, of the time. No struct is used for the result as the length can vary (new fields can be added in the future). Signed-off-by: Martin Kletzander --- src/libvirt_private.syms | 1 + src/util/virprocess.c | 77 +++ src/util/virprocess.h | 66 tests/meson.build | 1 + tests/virprocessstatdata/complex/stat | 2 + tests/virprocessstatdata/simple/stat | 1 + tests/virprocessstattest.c| 88 +++ 7 files changed, 236 insertions(+) create mode 100644 tests/virprocessstatdata/complex/stat create mode 100644 tests/virprocessstatdata/simple/stat create mode 100644 tests/virprocessstattest.c Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [libvirt PATCH 0/4] Coverity fixes
On Tue, Nov 23, 2021 at 3:21 PM Ján Tomko wrote: > Ján Tomko (4): > tools: virt-host-validate: fix memory leak > vbox: fix vboxCapsInit > ch: fix logic in virCHMonitorBuildPtyJson > tests: pcivpdtest: check return value of virCreateAnonymousFile > > src/ch/ch_monitor.c | 4 +--- > src/vbox/vbox_common.c| 1 - > tests/virpcivpdtest.c | 20 > tools/virt-host-validate-ch.c | 2 +- > 4 files changed, 22 insertions(+), 5 deletions(-) > > -- > 2.31.1 > > Reviewed-by: Kristína Hanicová
Re: [libvirt PATCH 4/4] tests: pcivpdtest: check return value of virCreateAnonymousFile
On Tue, Nov 23, 2021 at 3:20 PM Ján Tomko wrote: > Fixes: 59c1bc3a0e25e6d725db41990f11e0b53137115d > Fixes: 43820e4b8037680ec451761216750c6b139db67a > Fixes: 600f580d623ae4077ddeb6c7cb24f8a315a7c73b > Signed-off-by: Ján Tomko > --- > tests/virpcivpdtest.c | 20 > 1 file changed, 20 insertions(+) > > diff --git a/tests/virpcivpdtest.c b/tests/virpcivpdtest.c > index 284350fe29..62c51cdeb9 100644 > --- a/tests/virpcivpdtest.c > +++ b/tests/virpcivpdtest.c > @@ -446,6 +446,8 @@ testVirPCIVPDReadVPDBytes(const void *opaque > G_GNUC_UNUSED) > buf = g_malloc0(dataLen); > > fd = virCreateAnonymousFile(fullVPDExample, dataLen); > +if (fd < 0) > +return -1; > I would prefer if you rewrote this before merging as: if ((fd = virCreateAnonymousFile(fullVPDExample, dataLen)) < 0) return -1; (The whole patch.) I think it looks cleaner, but that's just my preference. readBytes = virPCIVPDReadVPDBytes(fd, buf, dataLen, 0, ); > > @@ -482,6 +484,9 @@ testVirPCIVPDParseVPDStringResource(const void *opaque > G_GNUC_UNUSED) > > dataLen = G_N_ELEMENTS(stringResExample); > fd = virCreateAnonymousFile(stringResExample, dataLen); > +if (fd < 0) > +return -1; > + > result = virPCIVPDParseVPDLargeResourceString(fd, 0, dataLen, , > res); > VIR_FORCE_CLOSE(fd); > > @@ -552,6 +557,9 @@ testVirPCIVPDParseFullVPD(const void *opaque > G_GNUC_UNUSED) > > dataLen = G_N_ELEMENTS(fullVPDExample); > fd = virCreateAnonymousFile(fullVPDExample, dataLen); > +if (fd < 0) > +return -1; > + > res = virPCIVPDParse(fd); > VIR_FORCE_CLOSE(fd); > > @@ -620,6 +628,9 @@ testVirPCIVPDParseZeroLengthRW(const void *opaque > G_GNUC_UNUSED) > > dataLen = G_N_ELEMENTS(fullVPDExample); > fd = virCreateAnonymousFile(fullVPDExample, dataLen); > +if (fd < 0) > +return -1; > + > res = virPCIVPDParse(fd); > VIR_FORCE_CLOSE(fd); > > @@ -670,6 +681,9 @@ testVirPCIVPDParseNoRW(const void *opaque > G_GNUC_UNUSED) > > dataLen = G_N_ELEMENTS(fullVPDExample); > fd = virCreateAnonymousFile(fullVPDExample, dataLen); > +if (fd < 0) > +return -1; > + > res = virPCIVPDParse(fd); > VIR_FORCE_CLOSE(fd); > > @@ -723,6 +737,9 @@ testVirPCIVPDParseFullVPDSkipInvalidKeywords(const > void *opaque G_GNUC_UNUSED) > > dataLen = G_N_ELEMENTS(fullVPDExample); > fd = virCreateAnonymousFile(fullVPDExample, dataLen); > +if (fd < 0) > +return -1; > + > res = virPCIVPDParse(fd); > VIR_FORCE_CLOSE(fd); > > @@ -776,6 +793,9 @@ testVirPCIVPDParseFullVPDSkipInvalidValues(const void > *opaque G_GNUC_UNUSED) > > dataLen = G_N_ELEMENTS(fullVPDExample); > fd = virCreateAnonymousFile(fullVPDExample, dataLen); > +if (fd < 0) > +return -1; > + > res = virPCIVPDParse(fd); > VIR_FORCE_CLOSE(fd); > > -- > 2.31.1 > > Kristína
Re: [libvirt PATCH 0/2] snapshot revert fix followup
On a Tuesday in 2021, Pavel Hrdina wrote: Pavel Hrdina (2): qemu_monitor: remove unused load snapshot code virsh: man: update snapshot-revert description docs/manpages/virsh.rst | 4 src/qemu/qemu_monitor.c | 11 --- src/qemu/qemu_monitor.h | 1 - src/qemu/qemu_monitor_text.c | 36 src/qemu/qemu_monitor_text.h | 1 - 5 files changed, 4 insertions(+), 49 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
[PATCH v7 0/2] Dirty Ring support (Libvirt)
From: "Hyman Huang(黄勇)" v7 - rebase on master - modify the following points according to the advice given by Peter 1. skip the -accel switch and reuse the existing commit d20ebdda2 'qemu: Switch to -accel' 2. remove the post-parse function and do the parse work in virDomainFeaturesKVMDefParse once for all 3. throw an error if "size" not specified when kvm-dirty-ring feature enabled in xml 4. fix the memory leak when parsing xml 5. use macro VIR_ROUND_UP_POWER_OF_TWO to check power-of-two 6. put error messages in one line 7. squash the last 2 commit into 1 8. add test for kvm-dirty-ring feature Thanks for the careful reviews made by Peter. Please review, Thanks! Hyman Ping for this series. I still keep thinking the dirty ring feature is something good to have for libvirt. qemu-6.1 has supported dirty ring feature and followed up with the commit 0e21bf24 "support dirtyrate at the granualrity of vcpu", which is a typical usage scenario of dirty ring. another usage scenario may be the implementation of per-vcpu auto-converge during live migration which is already being reviewed. so we can make full use of dirty ring feature if libvirt supports. and any corrections and comments about this series would be very appreciated. Please review, Thanks! Hyman v6 - rebase on master v5,v4: blank, just make v6 be the the latest version. v3 - rebase master and fix the confilict when apply "conf: introduce dirty_ring_size in struct "_virDomainDef" to current master. v2 - split patchset into 4 patches - leave out the tcg case when building commandline. - handle the VIR_DOMAIN_KVM_DIRTY_RING case independently in , virDomainFeatureDefParse and virDomainDefFeaturesCheckABIStability, do not integrate it with other cases... - add dirty ring size check in virDomainDefFeaturesCheckABIStability - modify zero checks on integers of dirty ring size in a explicit way. - set the default value of dirty ring size in a post-parser callback. - check the absence of kvm_feature in a explicit way. - code clean of virTristateSwitchTypeToString function. this version's modification base on Peter's advices mostly, thanks a lot, please review ! v1 since qemu has introduced a dirty ring feature in 6.1.0, may be it's the right time to introduce dirty ring in libvirt meanwhile. this patch add feature named 'dirty-ring', which enable dirty ring feature when starting vm. to try this out, three things has done in this patchset: - introduce QEMU_CAPS_ACCEL so the the libvirt can use it to select the right option when specifying the accelerator type. - switch the option "-machine accel=xxx" to "-accel xxx" when specifying accelerator type once libvirt build QEMU command line, so that dirty-ring-size property can be passed to qemu when start vm. - introduce dirty_ring_size to hold the ring size configured by user and pass dirty_ring_size when building qemu commandline if dirty ring feature enabled. though dirty ring is per-cpu logically, the size of dirty ring is registered by 'struct kvm' in QEMU. so we would like to place the dirty_ring_size as a property of vm in Libvirt as the QEMU do. the dirty ring feature is disabled by default, and if enabled, the default value of ring size if 4096 if size not configured. for more details about dirty ring and "-accel" option, please refer to: https://lore.kernel.org/qemu-devel/20210108165050.406906-10-pet...@redhat.com/ https://lore.kernel.org/qemu-devel/3aa73987-40e8-3619-0723-9f17f7385...@redhat.com/ please review, Thanks! Best Regards ! Hyman Huang(黄勇) (2): qemu: support dirty ring feature tests: add test for kvm-dirty-ring feature docs/formatdomain.rst | 18 --- docs/schemas/domaincommon.rng | 10 src/conf/domain_conf.c| 54 +++ src/conf/domain_conf.h| 4 ++ src/qemu/qemu_command.c | 12 + tests/qemuxml2argvdata/kvm-features-off.xml | 1 + tests/qemuxml2argvdata/kvm-features.args | 2 +- tests/qemuxml2argvdata/kvm-features.xml | 1 + tests/qemuxml2xmloutdata/kvm-features-off.xml | 1 + tests/qemuxml2xmloutdata/kvm-features.xml | 1 + 10 files changed, 95 insertions(+), 9 deletions(-) -- 2.27.0
Re: [PATCH 12/12] util: xml: Remove virXMLPropStringLimit and virXPathStringLimit
On a Monday in 2021, Peter Krempa wrote: The functions have very difficult semantics where callers are not able to tell whether the property is missing or failed the length check. Only the latter produces errors. Since usage of the functions was phased out, remove them completely to avoid further broken code. Signed-off-by: Peter Krempa --- src/libvirt_private.syms | 2 -- src/util/virxml.c| 62 src/util/virxml.h| 8 -- 3 files changed, 72 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
[PATCH v7 1/2] qemu: support dirty ring feature
From: Hyman Huang(黄勇) Dirty ring feature was introduced in qemu-6.1.0, this patch add the corresponding feature named 'dirty-ring', which enable dirty ring feature when starting vm. To implement the dirty-ring feature, dirty_ring_size in struct "_virDomainDef" is introduced to hold the dirty ring size configured in xml, and it will be used as dirty-ring-size property of kvm accelerator when building qemu commandline, it is something like "-accel dirty-ring-size=xxx". To enable the feature, the following XML needs to be added to the guest's domain description: If property "state=on", property "size" must be specified, which should be power of 2 and range in [1024, 65526]. Signed-off-by: Hyman Huang(黄勇) --- docs/formatdomain.rst | 18 ++-- docs/schemas/domaincommon.rng | 10 +++ src/conf/domain_conf.c| 54 +++ src/conf/domain_conf.h| 4 +++ src/qemu/qemu_command.c | 12 5 files changed, 90 insertions(+), 8 deletions(-) diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index eb8c973cf1..ea69b61c70 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -1843,6 +1843,7 @@ Hypervisors may allow certain CPU / machine features to be toggled on/off. + @@ -1925,14 +1926,15 @@ are: ``kvm`` Various features to change the behavior of the KVM hypervisor. - == === - FeatureDescription Value Since - == === - hidden Hide the KVM hypervisor from standard MSR based discovery on, off :since:`1.2.8 (QEMU 2.1.0)` - hint-dedicated Allows a guest to enable optimizations when running on dedicated vCPUs on, off :since:`5.7.0 (QEMU 2.12.0)` - poll-control Decrease IO completion latency by introducing a grace period of busy waiting on, off :since:`6.10.0 (QEMU 4.2)` - pv-ipi Paravirtualized send IPIs on, off :since:`7.10.0 (QEMU 3.1)` - == === + == == + FeatureDescription Value Since + == == + hidden Hide the KVM hypervisor from standard MSR based discovery on, off :since:`1.2.8 (QEMU 2.1.0)` + hint-dedicated Allows a guest to enable optimizations when running on dedicated vCPUs on, off :since:`5.7.0 (QEMU 2.12.0)` + poll-control Decrease IO completion latency by introducing a grace period of busy waiting on, off :since:`6.10.0 (QEMU 4.2)` + pv-ipi Paravirtualized send IPIs on, off :since:`7.10.0 (QEMU 3.1)` + dirty-ring Enable dirty ring feature on, off; size - must be power of 2, range [1024,65536] :since:`7.10.0 (QEMU 6.1)` + == == ``xen`` Various features to change the behavior of the Xen hypervisor. diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index f01b7a6470..5f9fe3cc58 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -7212,6 +7212,16 @@ + + + + + + + + + + diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 552d43b845..6f3c925b55 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -205,6 +205,7 @@ VIR_ENUM_IMPL(virDomainKVM, "hint-dedicated", "poll-control", "pv-ipi", + "dirty-ring", ); VIR_ENUM_IMPL(virDomainXen, @@ -17589,6 +17590,25 @@
[PATCH v7 2/2] tests: add test for kvm-dirty-ring feature
From: Hyman Huang(黄勇) Update the KVM feature tests for dirty ring. Signed-off-by: Hyman Huang(黄勇) --- tests/qemuxml2argvdata/kvm-features-off.xml | 1 + tests/qemuxml2argvdata/kvm-features.args | 2 +- tests/qemuxml2argvdata/kvm-features.xml | 1 + tests/qemuxml2xmloutdata/kvm-features-off.xml | 1 + tests/qemuxml2xmloutdata/kvm-features.xml | 1 + 5 files changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/qemuxml2argvdata/kvm-features-off.xml b/tests/qemuxml2argvdata/kvm-features-off.xml index a1004a206b..fb7cbaf061 100644 --- a/tests/qemuxml2argvdata/kvm-features-off.xml +++ b/tests/qemuxml2argvdata/kvm-features-off.xml @@ -15,6 +15,7 @@ + diff --git a/tests/qemuxml2argvdata/kvm-features.args b/tests/qemuxml2argvdata/kvm-features.args index 371c382b47..1789363736 100644 --- a/tests/qemuxml2argvdata/kvm-features.args +++ b/tests/qemuxml2argvdata/kvm-features.args @@ -12,7 +12,7 @@ QEMU_AUDIO_DRV=none \ -S \ -object secret,id=masterKey0,format=raw,file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \ -machine pc,usb=off,dump-guest-core=off \ --accel kvm \ +-accel kvm,dirty-ring-size=4096 \ -cpu host,kvm=off,kvm-hint-dedicated=on,kvm-poll-control=on \ -m 214 \ -realtime mlock=off \ diff --git a/tests/qemuxml2argvdata/kvm-features.xml b/tests/qemuxml2argvdata/kvm-features.xml index 51229a6c37..900431c4ff 100644 --- a/tests/qemuxml2argvdata/kvm-features.xml +++ b/tests/qemuxml2argvdata/kvm-features.xml @@ -15,6 +15,7 @@ + diff --git a/tests/qemuxml2xmloutdata/kvm-features-off.xml b/tests/qemuxml2xmloutdata/kvm-features-off.xml index 52a0ef0065..7ee6525cd9 100644 --- a/tests/qemuxml2xmloutdata/kvm-features-off.xml +++ b/tests/qemuxml2xmloutdata/kvm-features-off.xml @@ -15,6 +15,7 @@ + diff --git a/tests/qemuxml2xmloutdata/kvm-features.xml b/tests/qemuxml2xmloutdata/kvm-features.xml index 72e66fcbf5..8ce3a2b987 100644 --- a/tests/qemuxml2xmloutdata/kvm-features.xml +++ b/tests/qemuxml2xmloutdata/kvm-features.xml @@ -15,6 +15,7 @@ + -- 2.27.0
Re: [PATCH 11/12] virSecurityLabelDefParseXML: Don't use virXMLPropStringLimit
On a Monday in 2021, Peter Krempa wrote: The function produces an error which is ignored in this code path. Signed-off-by: Peter Krempa --- src/conf/domain_conf.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH 10/12] virSecurityDeviceLabelDefParseXML: Don't use 'virXPathStringLimit'
On a Monday in 2021, Peter Krempa wrote: virXPathStringLimit doesn't give callers a way to differentiate between the queried XPath being empty and the length limit being exceeded. This means that the callers is completely ignoring the error. Move the length check into the caller. Signed-off-by: Peter Krempa --- src/conf/domain_conf.c | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH 09/12] virSecurityDeviceLabelDefParseXML: Use automatic memory clearing for temp strings
On a Monday in 2021, Peter Krempa wrote:
On Mon, Nov 22, 2021 at 18:12:29 +0100, Peter Krempa wrote:
Apart from code simplification the refactor of 'model' fixes an unlikely
memory leak of the string if a duplicate model is found.
While the coversion of 'label' variable may seem unnecessary it will
come in handy in the next patch.
Signed-off-by: Peter Krempa
---
src/conf/domain_conf.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index bd9da0744d..e829511ac5 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -8016,7 +8016,10 @@
virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDef ***seclabels_rtn,
size_t nseclabels = 0;
int n;
size_t i, j;
-char *model, *relabel, *label, *labelskip;
+g_autofree char *model = NULL;
+g_autofree char *relabel = NULL;
+g_autofree char *label = NULL;
+g_autofree char *labelskip = NULL;
g_autofree xmlNodePtr *list = NULL;
if ((n = virXPathNodeSet("./seclabel", ctxt, )) < 0)
@@ -8041,7 +8044,7 @@
virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDef ***seclabels_rtn,
goto error;
}
}
-seclabels[i]->model = model;
+seclabels[i]->model = g_steal_pointer();
}
relabel = virXMLPropString(list[i], "relabel");
Forgot to squash the following diff into this commit:
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 17ba810467..16dea34890 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -8017,10 +8017,6 @@
virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDef ***seclabels_rtn,
size_t nseclabels = 0;
int n;
size_t i, j;
-g_autofree char *model = NULL;
-g_autofree char *relabel = NULL;
-g_autofree char *label = NULL;
-g_autofree char *labelskip = NULL;
g_autofree xmlNodePtr *list = NULL;
if ((n = virXPathNodeSet("./seclabel", ctxt, )) < 0)
@@ -8034,6 +8030,11 @@
virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDef ***seclabels_rtn,
seclabels[i] = g_new0(virSecurityDeviceLabelDef, 1);
for (i = 0; i < n; i++) {
+g_autofree char *model = NULL;
+g_autofree char *relabel = NULL;
+g_autofree char *label = NULL;
+g_autofree char *labelskip = NULL;
+
/* get model associated to this override */
Reviewed-by: Ján Tomko
Jano
signature.asc
Description: PGP signature
Re: [PATCH 08/12] virSecurityLabelDefParseXML: Don't use 'virXPathStringLimit'
On a Monday in 2021, Peter Krempa wrote: virXPathStringLimit doesn't give callers a way to differentiate between the queried XPath being empty and the length limit being exceeded. This means that callers are either overwriting the error message or ignoring it altogether. Move the length checks into the caller. Signed-off-by: Peter Krempa --- src/conf/domain_conf.c | 22 ++ 1 file changed, 14 insertions(+), 8 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH 06/12] virSecurityLabelDefParseXML: Remove pointless 'error' label
On a Monday in 2021, Peter Krempa wrote: Signed-off-by: Peter Krempa --- src/conf/domain_conf.c | 19 --- 1 file changed, 8 insertions(+), 11 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH 07/12] virNodeDeviceCapVPDParseCustomFields: Don't use 'virXPathStringLimit'
On a Monday in 2021, Peter Krempa wrote: virXPathStringLimit doesn't give callers a way to differentiate between the queried XPath being empty and the length limit being exceeded. This means that callers are overwriting the error message. Move the length checks into the caller. Signed-off-by: Peter Krempa --- src/conf/node_device_conf.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH 05/12] virSecurityLabelDefParseXML: Use automatic freeing for 'seclabel'
On a Monday in 2021, Peter Krempa wrote: Signed-off-by: Peter Krempa --- src/conf/domain_conf.c | 7 +++ 1 file changed, 3 insertions(+), 4 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH 04/12] virSecurityLabelDefParseXML: Don't reuse temporary string 'p'
On a Monday in 2021, Peter Krempa wrote:
Use separate variables for 'model' and 'relabel' properties.
Signed-off-by: Peter Krempa
---
src/conf/domain_conf.c | 20
src/util/virseclabel.h | 1 +
2 files changed, 9 insertions(+), 12 deletions(-)
diff --git a/src/util/virseclabel.h b/src/util/virseclabel.h
index eca4d09d24..7e62f8a2e2 100644
--- a/src/util/virseclabel.h
+++ b/src/util/virseclabel.h
@@ -43,6 +43,7 @@ struct _virSecurityLabelDef {
};
+
Without this suspicious empty line:
Reviewed-by: Ján Tomko
Jano
/* Security configuration for device */
typedef struct _virSecurityDeviceLabelDef virSecurityDeviceLabelDef;
struct _virSecurityDeviceLabelDef {
--
2.31.1
signature.asc
Description: PGP signature
Re: [PATCH 03/12] virSecurityLabelDefParseXML: Directly assign strings into appropriate variables
On a Monday in 2021, Peter Krempa wrote: 'seclabel->label', 'seclabel->imagelabel' and 'seclabel->baselabel' are populated by stealing the pointer from the 'p' temporary string. Remove the extra step. Signed-off-by: Peter Krempa --- src/conf/domain_conf.c | 20 1 file changed, 8 insertions(+), 12 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
[libvirt PATCH 2/2] virsh: man: update snapshot-revert description
We've changed the behavior of this API that from now on it will always restart the VM process and we are no longer able to revert to snapshots created by libvirt older then 0.9.5. Signed-off-by: Pavel Hrdina --- docs/manpages/virsh.rst | 4 1 file changed, 4 insertions(+) diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index 5f5ccfeafe..39636a565e 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -7293,6 +7293,10 @@ no vm state leaves the domain in an inactive state. Passing either the transient domains cannot be inactive, it is required to use one of these flags when reverting to a disk snapshot of a transient domain. +Since libvirt 7.10.0 the VM process is always restarted so the following +paragraph is no longer valid. If the snapshot metadata lacks the full +VM XML it's no longer possible to revert to such snapshot. + There are a number of cases where a snapshot revert involves extra risk, which requires the use of *--force* to proceed: -- 2.31.1
[libvirt PATCH 0/2] snapshot revert fix followup
Pavel Hrdina (2): qemu_monitor: remove unused load snapshot code virsh: man: update snapshot-revert description docs/manpages/virsh.rst | 4 src/qemu/qemu_monitor.c | 11 --- src/qemu/qemu_monitor.h | 1 - src/qemu/qemu_monitor_text.c | 36 src/qemu/qemu_monitor_text.h | 1 - 5 files changed, 4 insertions(+), 49 deletions(-) -- 2.31.1
[libvirt PATCH 1/2] qemu_monitor: remove unused load snapshot code
Recent cleanup of snapshot revert code made these function unused.
Signed-off-by: Pavel Hrdina
---
src/qemu/qemu_monitor.c | 11 ---
src/qemu/qemu_monitor.h | 1 -
src/qemu/qemu_monitor_text.c | 36
src/qemu/qemu_monitor_text.h | 1 -
4 files changed, 49 deletions(-)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index ac988e063b..26b59801b8 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -3016,17 +3016,6 @@ qemuMonitorCreateSnapshot(qemuMonitor *mon, const char
*name)
return qemuMonitorTextCreateSnapshot(mon, name);
}
-int
-qemuMonitorLoadSnapshot(qemuMonitor *mon, const char *name)
-{
-VIR_DEBUG("name=%s", name);
-
-QEMU_CHECK_MONITOR(mon);
-
-/* there won't ever be a direct QMP replacement for this function */
-return qemuMonitorTextLoadSnapshot(mon, name);
-}
-
int
qemuMonitorDeleteSnapshot(qemuMonitor *mon, const char *name)
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 0dd7b1c4e2..99ecebc648 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -1057,7 +1057,6 @@ int qemuMonitorDriveDel(qemuMonitor *mon,
const char *drivestr);
int qemuMonitorCreateSnapshot(qemuMonitor *mon, const char *name);
-int qemuMonitorLoadSnapshot(qemuMonitor *mon, const char *name);
int qemuMonitorDeleteSnapshot(qemuMonitor *mon, const char *name);
int qemuMonitorTransaction(qemuMonitor *mon, virJSONValue **actions)
diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c
index 6a1a913055..0ca7f5a470 100644
--- a/src/qemu/qemu_monitor_text.c
+++ b/src/qemu/qemu_monitor_text.c
@@ -144,42 +144,6 @@ qemuMonitorTextCreateSnapshot(qemuMonitor *mon,
return 0;
}
-int qemuMonitorTextLoadSnapshot(qemuMonitor *mon, const char *name)
-{
-g_autofree char *cmd = NULL;
-g_autofree char *reply = NULL;
-
-cmd = g_strdup_printf("loadvm \"%s\"", name);
-
-if (qemuMonitorJSONHumanCommand(mon, cmd, ))
-return -1;
-
-if (strstr(reply, "No block device supports snapshots")) {
-virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("this domain does not have a device to load
snapshots"));
-return -1;
-} else if (strstr(reply, "Could not find snapshot")) {
-virReportError(VIR_ERR_OPERATION_INVALID,
- _("the snapshot '%s' does not exist, and was not
loaded"),
- name);
-return -1;
-} else if (strstr(reply, "Snapshots not supported on device")) {
-virReportError(VIR_ERR_OPERATION_INVALID,
- _("Failed to load snapshot: %s"), reply);
-return -1;
-} else if (strstr(reply, "Could not open VM state file") ||
- strstr(reply, "Error: ") ||
- (strstr(reply, "Error") &&
-(strstr(reply, "while loading VM state") ||
- strstr(reply, "while activating snapshot on" {
-virReportError(VIR_ERR_OPERATION_FAILED,
- _("Failed to load snapshot: %s"), reply);
-return -1;
-}
-
-return 0;
-}
-
int qemuMonitorTextDeleteSnapshot(qemuMonitor *mon, const char *name)
{
g_autofree char *cmd = NULL;
diff --git a/src/qemu/qemu_monitor_text.h b/src/qemu/qemu_monitor_text.h
index c8177d3b3b..d959fc8889 100644
--- a/src/qemu/qemu_monitor_text.h
+++ b/src/qemu/qemu_monitor_text.h
@@ -32,5 +32,4 @@ int qemuMonitorTextDriveDel(qemuMonitor *mon,
const char *drivestr);
int qemuMonitorTextCreateSnapshot(qemuMonitor *mon, const char *name);
-int qemuMonitorTextLoadSnapshot(qemuMonitor *mon, const char *name);
int qemuMonitorTextDeleteSnapshot(qemuMonitor *mon, const char *name);
--
2.31.1
[libvirt PATCH 4/4] tests: pcivpdtest: check return value of virCreateAnonymousFile
Fixes: 59c1bc3a0e25e6d725db41990f11e0b53137115d Fixes: 43820e4b8037680ec451761216750c6b139db67a Fixes: 600f580d623ae4077ddeb6c7cb24f8a315a7c73b Signed-off-by: Ján Tomko --- tests/virpcivpdtest.c | 20 1 file changed, 20 insertions(+) diff --git a/tests/virpcivpdtest.c b/tests/virpcivpdtest.c index 284350fe29..62c51cdeb9 100644 --- a/tests/virpcivpdtest.c +++ b/tests/virpcivpdtest.c @@ -446,6 +446,8 @@ testVirPCIVPDReadVPDBytes(const void *opaque G_GNUC_UNUSED) buf = g_malloc0(dataLen); fd = virCreateAnonymousFile(fullVPDExample, dataLen); +if (fd < 0) +return -1; readBytes = virPCIVPDReadVPDBytes(fd, buf, dataLen, 0, ); @@ -482,6 +484,9 @@ testVirPCIVPDParseVPDStringResource(const void *opaque G_GNUC_UNUSED) dataLen = G_N_ELEMENTS(stringResExample); fd = virCreateAnonymousFile(stringResExample, dataLen); +if (fd < 0) +return -1; + result = virPCIVPDParseVPDLargeResourceString(fd, 0, dataLen, , res); VIR_FORCE_CLOSE(fd); @@ -552,6 +557,9 @@ testVirPCIVPDParseFullVPD(const void *opaque G_GNUC_UNUSED) dataLen = G_N_ELEMENTS(fullVPDExample); fd = virCreateAnonymousFile(fullVPDExample, dataLen); +if (fd < 0) +return -1; + res = virPCIVPDParse(fd); VIR_FORCE_CLOSE(fd); @@ -620,6 +628,9 @@ testVirPCIVPDParseZeroLengthRW(const void *opaque G_GNUC_UNUSED) dataLen = G_N_ELEMENTS(fullVPDExample); fd = virCreateAnonymousFile(fullVPDExample, dataLen); +if (fd < 0) +return -1; + res = virPCIVPDParse(fd); VIR_FORCE_CLOSE(fd); @@ -670,6 +681,9 @@ testVirPCIVPDParseNoRW(const void *opaque G_GNUC_UNUSED) dataLen = G_N_ELEMENTS(fullVPDExample); fd = virCreateAnonymousFile(fullVPDExample, dataLen); +if (fd < 0) +return -1; + res = virPCIVPDParse(fd); VIR_FORCE_CLOSE(fd); @@ -723,6 +737,9 @@ testVirPCIVPDParseFullVPDSkipInvalidKeywords(const void *opaque G_GNUC_UNUSED) dataLen = G_N_ELEMENTS(fullVPDExample); fd = virCreateAnonymousFile(fullVPDExample, dataLen); +if (fd < 0) +return -1; + res = virPCIVPDParse(fd); VIR_FORCE_CLOSE(fd); @@ -776,6 +793,9 @@ testVirPCIVPDParseFullVPDSkipInvalidValues(const void *opaque G_GNUC_UNUSED) dataLen = G_N_ELEMENTS(fullVPDExample); fd = virCreateAnonymousFile(fullVPDExample, dataLen); +if (fd < 0) +return -1; + res = virPCIVPDParse(fd); VIR_FORCE_CLOSE(fd); -- 2.31.1
[libvirt PATCH 3/4] ch: fix logic in virCHMonitorBuildPtyJson
There is a leftover 'ptys' variable, which we only assign
to and one assignment to 'content', where we add an empty
'pty' object.
Remove 'ptys'.
Fixes: 93accefd9eca1bc3d7e923a979ab2d1b8a312ff7
Signed-off-by: Ján Tomko
---
src/ch/ch_monitor.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c
index 691d1ce64b..12c10da874 100644
--- a/src/ch/ch_monitor.c
+++ b/src/ch/ch_monitor.c
@@ -88,8 +88,6 @@ virCHMonitorBuildCPUJson(virJSONValue *content, virDomainDef
*vmdef)
static int
virCHMonitorBuildPTYJson(virJSONValue *content, virDomainDef *vmdef)
{
-virJSONValue *ptys = virJSONValueNewObject();
-
if (vmdef->nconsoles) {
g_autoptr(virJSONValue) pty = virJSONValueNewObject();
if (virJSONValueObjectAppendString(pty, "mode", "Pty") < 0)
@@ -100,7 +98,7 @@ virCHMonitorBuildPTYJson(virJSONValue *content, virDomainDef
*vmdef)
if (vmdef->nserials) {
g_autoptr(virJSONValue) pty = virJSONValueNewObject();
-if (virJSONValueObjectAppendString(ptys, "mode", "Pty") < 0)
+if (virJSONValueObjectAppendString(pty, "mode", "Pty") < 0)
return -1;
if (virJSONValueObjectAppend(content, "serial", ) < 0)
return -1;
--
2.31.1
[libvirt PATCH 2/4] vbox: fix vboxCapsInit
There is a stray mis-indented 'return NULL' left after a recent refactor. Fixes: c18d9e23fafabcfbb80481e0705931036b8e7331 Signed-off-by: Ján Tomko --- src/vbox/vbox_common.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 62c8e22cb5..72f1b9c466 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -102,7 +102,6 @@ vboxCapsInit(void) guest = virCapabilitiesAddGuest(caps, VIR_DOMAIN_OSTYPE_HVM, caps->host.arch, NULL, NULL, 0, NULL); -return NULL; virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_VBOX, NULL, NULL, 0, NULL); -- 2.31.1
[libvirt PATCH 1/4] tools: virt-host-validate: fix memory leak
virHostValidateGetCPUFlags returns an allocated virBitmap and
it needs to be freed.
Fixes: a0ec7165e3bbc416478740f6d2e8fef2ece18602
Signed-off-by: Ján Tomko
---
tools/virt-host-validate-ch.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/virt-host-validate-ch.c b/tools/virt-host-validate-ch.c
index b00fdd0e90..b26f82738d 100644
--- a/tools/virt-host-validate-ch.c
+++ b/tools/virt-host-validate-ch.c
@@ -28,7 +28,7 @@
int virHostValidateCh(void)
{
int ret = 0;
-virBitmap *flags;
+g_autoptr(virBitmap) flags = NULL;
bool hasHwVirt = false;
bool hasVirtFlag = false;
virArch arch = virArchFromHost();
--
2.31.1
[libvirt PATCH 0/4] Coverity fixes
Ján Tomko (4): tools: virt-host-validate: fix memory leak vbox: fix vboxCapsInit ch: fix logic in virCHMonitorBuildPtyJson tests: pcivpdtest: check return value of virCreateAnonymousFile src/ch/ch_monitor.c | 4 +--- src/vbox/vbox_common.c| 1 - tests/virpcivpdtest.c | 20 tools/virt-host-validate-ch.c | 2 +- 4 files changed, 22 insertions(+), 5 deletions(-) -- 2.31.1
[libvirt PATCH 3/3] ci: run a mingw64 job on stable Fedora
Both of the current mingw jobs are marked as 'allow_failure' because they are running against Fedora rawhide which is an unstable distro. We need at least one mingw job to be gating to more reliably detect problems. This introduces dockerfiles for both mingw variants on Fedora 35 and sets the mingw64 build to run on Fedora 34, and mingw32 on Fedora rawhide. Signed-off-by: Daniel P. Berrangé --- .../fedora-35-cross-mingw32.Dockerfile| 92 +++ .../fedora-35-cross-mingw64.Dockerfile| 92 +++ ci/gitlab.yml | 30 -- ci/manifest.yml | 11 ++- 4 files changed, 216 insertions(+), 9 deletions(-) create mode 100644 ci/containers/fedora-35-cross-mingw32.Dockerfile create mode 100644 ci/containers/fedora-35-cross-mingw64.Dockerfile diff --git a/ci/containers/fedora-35-cross-mingw32.Dockerfile b/ci/containers/fedora-35-cross-mingw32.Dockerfile new file mode 100644 index 00..0a26a1f9bd --- /dev/null +++ b/ci/containers/fedora-35-cross-mingw32.Dockerfile @@ -0,0 +1,92 @@ +# THIS FILE WAS AUTO-GENERATED +# +# $ lcitool manifest ci/manifest.yml +# +# https://gitlab.com/libvirt/libvirt-ci + +FROM registry.fedoraproject.org/fedora:35 + +RUN dnf install -y nosync && \ +echo -e '#!/bin/sh\n\ +if test -d /usr/lib64\n\ +then\n\ +export LD_PRELOAD=/usr/lib64/nosync/nosync.so\n\ +else\n\ +export LD_PRELOAD=/usr/lib/nosync/nosync.so\n\ +fi\n\ +exec "$@"' > /usr/bin/nosync && \ +chmod +x /usr/bin/nosync && \ +nosync dnf update -y && \ +nosync dnf install -y \ +augeas \ +bash-completion \ +ca-certificates \ +ccache \ +cpp \ +cppi \ +diffutils \ +dnsmasq \ +dwarves \ +ebtables \ +firewalld-filesystem \ +git \ +glibc-langpack-en \ +grep \ +iproute \ +iproute-tc \ +iptables \ +iscsi-initiator-utils \ +kmod \ +libxml2 \ +libxslt \ +lvm2 \ +make \ +meson \ +nfs-utils \ +ninja-build \ +numad \ +parted \ +perl-base \ +polkit \ +python3 \ +python3-docutils \ +python3-flake8 \ +qemu-img \ +radvd \ +rpcgen \ +rpm-build \ +scrub \ +sed \ +sheepdog \ +zfs-fuse && \ +nosync dnf autoremove -y && \ +nosync dnf clean all -y + +RUN nosync dnf install -y \ +mingw32-curl \ +mingw32-dbus \ +mingw32-dlfcn \ +mingw32-gcc \ +mingw32-gettext \ +mingw32-glib2 \ +mingw32-gnutls \ +mingw32-headers \ +mingw32-libssh2 \ +mingw32-libxml2 \ +mingw32-pkg-config \ +mingw32-portablexdr \ +mingw32-readline && \ +nosync dnf clean all -y && \ +rpm -qa | sort > /packages.txt && \ +mkdir -p /usr/libexec/ccache-wrappers && \ +ln -s /usr/bin/ccache /usr/libexec/ccache-wrappers/i686-w64-mingw32-cc && \ +ln -s /usr/bin/ccache /usr/libexec/ccache-wrappers/i686-w64-mingw32-gcc + +ENV LANG "en_US.UTF-8" +ENV MAKE "/usr/bin/make" +ENV NINJA "/usr/bin/ninja" +ENV PYTHON "/usr/bin/python3" +ENV CCACHE_WRAPPERSDIR "/usr/libexec/ccache-wrappers" + +ENV ABI "i686-w64-mingw32" +ENV MESON_OPTS "--cross-file=/usr/share/mingw/toolchain-mingw32.meson" diff --git a/ci/containers/fedora-35-cross-mingw64.Dockerfile b/ci/containers/fedora-35-cross-mingw64.Dockerfile new file mode 100644 index 00..e2a3ba9d6d --- /dev/null +++ b/ci/containers/fedora-35-cross-mingw64.Dockerfile @@ -0,0 +1,92 @@ +# THIS FILE WAS AUTO-GENERATED +# +# $ lcitool manifest ci/manifest.yml +# +# https://gitlab.com/libvirt/libvirt-ci + +FROM registry.fedoraproject.org/fedora:35 + +RUN dnf install -y nosync && \ +echo -e '#!/bin/sh\n\ +if test -d /usr/lib64\n\ +then\n\ +export LD_PRELOAD=/usr/lib64/nosync/nosync.so\n\ +else\n\ +export LD_PRELOAD=/usr/lib/nosync/nosync.so\n\ +fi\n\ +exec "$@"' > /usr/bin/nosync && \ +chmod +x /usr/bin/nosync && \ +nosync dnf update -y && \ +nosync dnf install -y \ +augeas \ +bash-completion \ +ca-certificates \ +ccache \ +cpp \ +cppi \ +diffutils \ +dnsmasq \ +dwarves \ +ebtables \ +firewalld-filesystem \ +git \ +glibc-langpack-en \ +grep \ +iproute \ +iproute-tc \ +iptables \ +iscsi-initiator-utils \ +kmod \ +libxml2 \ +libxslt \ +lvm2 \ +make \ +meson \ +nfs-utils \ +ninja-build \ +numad \ +parted \ +perl-base \ +polkit \ +python3 \ +python3-docutils \ +python3-flake8 \ +qemu-img \ +radvd \ +rpcgen \ +rpm-build \ +scrub \ +sed \ +
[libvirt PATCH 0/3] ci: improve mingw coverage
This ensures at least one mingw job is gating for pipelines
Daniel P. Berrangé (3):
ci: replace Fedora 33 with Fedora 35
ci: refresh variables/dockerfiles with latest content
ci: run a mingw64 job on stable Fedora
ci/cirrus/freebsd-12.vars | 7 +-
ci/cirrus/freebsd-13.vars | 7 +-
ci/cirrus/freebsd-current.vars| 7 +-
ci/cirrus/macos-11.vars | 7 +-
ci/containers/centos-8.Dockerfile | 7 +-
ci/containers/centos-stream-8.Dockerfile | 7 +-
.../fedora-35-cross-mingw32.Dockerfile| 92 +++
.../fedora-35-cross-mingw64.Dockerfile| 92 +++
...ora-33.Dockerfile => fedora-35.Dockerfile} | 2 +-
.../fedora-rawhide-cross-mingw32.Dockerfile | 2 +-
.../fedora-rawhide-cross-mingw64.Dockerfile | 2 +-
ci/containers/fedora-rawhide.Dockerfile | 2 +-
ci/containers/opensuse-tumbleweed.Dockerfile | 2 +-
ci/gitlab.yml | 50 ++
ci/manifest.yml | 13 ++-
15 files changed, 254 insertions(+), 45 deletions(-)
create mode 100644 ci/containers/fedora-35-cross-mingw32.Dockerfile
create mode 100644 ci/containers/fedora-35-cross-mingw64.Dockerfile
rename ci/containers/{fedora-33.Dockerfile => fedora-35.Dockerfile} (98%)
--
2.33.1
[libvirt PATCH 2/3] ci: refresh variables/dockerfiles with latest content
- The Cirrus CI variables are now sorted - The dockerfiles update commands changed for some distros - Meson in CentOS is now new enough to use Signed-off-by: Daniel P. Berrangé --- ci/cirrus/freebsd-12.vars | 7 +-- ci/cirrus/freebsd-13.vars | 7 +-- ci/cirrus/freebsd-current.vars| 7 +-- ci/cirrus/macos-11.vars | 7 +-- ci/containers/centos-8.Dockerfile | 7 +-- ci/containers/centos-stream-8.Dockerfile | 7 +-- ci/containers/fedora-rawhide-cross-mingw32.Dockerfile | 2 +- ci/containers/fedora-rawhide-cross-mingw64.Dockerfile | 2 +- ci/containers/fedora-rawhide.Dockerfile | 2 +- ci/containers/opensuse-tumbleweed.Dockerfile | 2 +- 10 files changed, 26 insertions(+), 24 deletions(-) diff --git a/ci/cirrus/freebsd-12.vars b/ci/cirrus/freebsd-12.vars index 4318b255e9..4845d8d461 100644 --- a/ci/cirrus/freebsd-12.vars +++ b/ci/cirrus/freebsd-12.vars @@ -4,10 +4,13 @@ # # https://gitlab.com/libvirt/libvirt-ci -PACKAGING_COMMAND='pkg' CCACHE='/usr/local/bin/ccache' +CPAN_PKGS='' +CROSS_PKGS='' MAKE='/usr/local/bin/gmake' NINJA='/usr/local/bin/ninja' -PYTHON='/usr/local/bin/python3' +PACKAGING_COMMAND='pkg' PIP3='/usr/local/bin/pip-3.8' PKGS='augeas bash-completion ca_root_nss ccache cppi curl cyrus-sasl dbus diffutils diskscrub dnsmasq fusefs-libs gettext git glib gmake gnugrep gnutls gsed libpcap libpciaccess libssh libssh2 libxml2 libxslt meson ninja perl5 pkgconf polkit py38-docutils py38-flake8 python3 qemu radvd readline yajl' +PYPI_PKGS='' +PYTHON='/usr/local/bin/python3' diff --git a/ci/cirrus/freebsd-13.vars b/ci/cirrus/freebsd-13.vars index 4318b255e9..4845d8d461 100644 --- a/ci/cirrus/freebsd-13.vars +++ b/ci/cirrus/freebsd-13.vars @@ -4,10 +4,13 @@ # # https://gitlab.com/libvirt/libvirt-ci -PACKAGING_COMMAND='pkg' CCACHE='/usr/local/bin/ccache' +CPAN_PKGS='' +CROSS_PKGS='' MAKE='/usr/local/bin/gmake' NINJA='/usr/local/bin/ninja' -PYTHON='/usr/local/bin/python3' +PACKAGING_COMMAND='pkg' PIP3='/usr/local/bin/pip-3.8' PKGS='augeas bash-completion ca_root_nss ccache cppi curl cyrus-sasl dbus diffutils diskscrub dnsmasq fusefs-libs gettext git glib gmake gnugrep gnutls gsed libpcap libpciaccess libssh libssh2 libxml2 libxslt meson ninja perl5 pkgconf polkit py38-docutils py38-flake8 python3 qemu radvd readline yajl' +PYPI_PKGS='' +PYTHON='/usr/local/bin/python3' diff --git a/ci/cirrus/freebsd-current.vars b/ci/cirrus/freebsd-current.vars index 4318b255e9..4845d8d461 100644 --- a/ci/cirrus/freebsd-current.vars +++ b/ci/cirrus/freebsd-current.vars @@ -4,10 +4,13 @@ # # https://gitlab.com/libvirt/libvirt-ci -PACKAGING_COMMAND='pkg' CCACHE='/usr/local/bin/ccache' +CPAN_PKGS='' +CROSS_PKGS='' MAKE='/usr/local/bin/gmake' NINJA='/usr/local/bin/ninja' -PYTHON='/usr/local/bin/python3' +PACKAGING_COMMAND='pkg' PIP3='/usr/local/bin/pip-3.8' PKGS='augeas bash-completion ca_root_nss ccache cppi curl cyrus-sasl dbus diffutils diskscrub dnsmasq fusefs-libs gettext git glib gmake gnugrep gnutls gsed libpcap libpciaccess libssh libssh2 libxml2 libxslt meson ninja perl5 pkgconf polkit py38-docutils py38-flake8 python3 qemu radvd readline yajl' +PYPI_PKGS='' +PYTHON='/usr/local/bin/python3' diff --git a/ci/cirrus/macos-11.vars b/ci/cirrus/macos-11.vars index 065d86aa45..6bc25aa380 100644 --- a/ci/cirrus/macos-11.vars +++ b/ci/cirrus/macos-11.vars @@ -4,10 +4,13 @@ # # https://gitlab.com/libvirt/libvirt-ci -PACKAGING_COMMAND='brew' CCACHE='/usr/local/bin/ccache' +CPAN_PKGS='' +CROSS_PKGS='' MAKE='/usr/local/bin/gmake' NINJA='/usr/local/bin/ninja' -PYTHON='/usr/local/bin/python3' +PACKAGING_COMMAND='brew' PIP3='/usr/local/bin/pip3' PKGS='augeas bash-completion ccache cppi curl dbus diffutils dnsmasq docutils flake8 gettext git glib gnu-sed gnutls grep libiscsi libpcap libssh libssh2 libxml2 libxslt make meson ninja perl pkg-config python3 qemu readline rpcgen scrub yajl' +PYPI_PKGS='' +PYTHON='/usr/local/bin/python3' diff --git a/ci/containers/centos-8.Dockerfile b/ci/containers/centos-8.Dockerfile index 5ac1e45459..05793fee4c 100644 --- a/ci/containers/centos-8.Dockerfile +++ b/ci/containers/centos-8.Dockerfile @@ -63,6 +63,7 @@ RUN dnf update -y && \ libxslt \ lvm2 \ make \ +meson \ netcf-devel \ nfs-utils \ ninja-build \ @@ -76,9 +77,6 @@ RUN dnf update -y && \ python3 \ python3-docutils \ python3-flake8 \ -python3-pip \ -python3-setuptools \ -python3-wheel \ qemu-img \ radvd \ readline-devel \ @@ -99,9 +97,6 @@ RUN dnf update -y && \ ln -s /usr/bin/ccache /usr/libexec/ccache-wrappers/clang && \ ln -s /usr/bin/ccache /usr/libexec/ccache-wrappers/gcc -RUN pip3 install \ - meson==0.56.0 - ENV LANG "en_US.UTF-8" ENV MAKE
[libvirt PATCH 1/3] ci: replace Fedora 33 with Fedora 35
Signed-off-by: Daniel P. Berrangé
---
...ora-33.Dockerfile => fedora-35.Dockerfile} | 2 +-
ci/gitlab.yml | 20 +--
ci/manifest.yml | 4 ++--
3 files changed, 13 insertions(+), 13 deletions(-)
rename ci/containers/{fedora-33.Dockerfile => fedora-35.Dockerfile} (98%)
diff --git a/ci/containers/fedora-33.Dockerfile
b/ci/containers/fedora-35.Dockerfile
similarity index 98%
rename from ci/containers/fedora-33.Dockerfile
rename to ci/containers/fedora-35.Dockerfile
index 0025e66d6a..1c5cb2c8ca 100644
--- a/ci/containers/fedora-33.Dockerfile
+++ b/ci/containers/fedora-35.Dockerfile
@@ -4,7 +4,7 @@
#
# https://gitlab.com/libvirt/libvirt-ci
-FROM registry.fedoraproject.org/fedora:33
+FROM registry.fedoraproject.org/fedora:35
RUN dnf install -y nosync && \
echo -e '#!/bin/sh\n\
diff --git a/ci/gitlab.yml b/ci/gitlab.yml
index 70e80f51b7..964e93d6d4 100644
--- a/ci/gitlab.yml
+++ b/ci/gitlab.yml
@@ -115,18 +115,18 @@ x86_64-debian-sid-container:
NAME: debian-sid
-x86_64-fedora-33-container:
+x86_64-fedora-34-container:
extends: .container_job
allow_failure: false
variables:
-NAME: fedora-33
+NAME: fedora-34
-x86_64-fedora-34-container:
+x86_64-fedora-35-container:
extends: .container_job
allow_failure: false
variables:
-NAME: fedora-34
+NAME: fedora-35
x86_64-fedora-rawhide-container:
@@ -425,22 +425,22 @@ x86_64-debian-sid:
NAME: debian-sid
-x86_64-fedora-33:
+x86_64-fedora-34:
extends: .native_build_job
needs:
-- x86_64-fedora-33-container
+- x86_64-fedora-34-container
allow_failure: false
variables:
-NAME: fedora-33
+NAME: fedora-34
-x86_64-fedora-34:
+x86_64-fedora-35:
extends: .native_build_job
needs:
-- x86_64-fedora-34-container
+- x86_64-fedora-35-container
allow_failure: false
variables:
-NAME: fedora-34
+NAME: fedora-35
x86_64-fedora-rawhide:
diff --git a/ci/manifest.yml b/ci/manifest.yml
index 49d5fe7064..460fdb4d34 100644
--- a/ci/manifest.yml
+++ b/ci/manifest.yml
@@ -125,10 +125,10 @@ targets:
- arch: s390x
allow-failure: true
- fedora-33: x86_64
-
fedora-34: x86_64
+ fedora-35: x86_64
+
fedora-rawhide:
jobs:
- arch: x86_64
--
2.33.1
Re: [PATCH 1/1] bash-completion: fix variable leaks of "IFS" and "word"
On 11/23/21 13:50, Koichi Murase wrote: > Thank you for reviewing the patch. > > 2021年11月23日(火) 21:37 Michal Prívozník : >> [...] >> >> The word variable is also used only inside a while loop (not visible in >> the context). So it can be declared local there instead of here. I'll >> fix that before pushing. > > OK. > >> However, we do require developers to provide signoff for their patches >> to signal their compliance with DCO: >> >> https://libvirt.org/hacking.html#developer-certificate-of-origin >> >> No need to resend the whole patch, it's sufficient if you reply with >> your SoB line. > > OK. Are just the following lines sufficient? > -- > [PATCH 1/1] bash-completion: fix variable leaks of "IFS" and "word" > > Signed-off-by: Koichi Murase Perfect. Reviewed-by: Michal Privoznik and pushed. Congratulations on your first libvirt contribution! Michal
Re: [PATCH 02/12] virSecurityLabelDef: Declare 'type' as 'virDomainSeclabelType'
On a Monday in 2021, Peter Krempa wrote: Use the appropriate enum type instead of an int and fix the XML parser and one missing fully populated switch. Signed-off-by: Peter Krempa --- src/conf/domain_conf.c | 14 +- src/security/security_selinux.c | 3 ++- src/util/virseclabel.h | 2 +- 3 files changed, 8 insertions(+), 11 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH 01/12] util: seclabel: Define autoptr cleanup func for virSecurityLabelDef and virSecurityDeviceLabelDef
On a Monday in 2021, Peter Krempa wrote: Signed-off-by: Peter Krempa --- src/util/virseclabel.h | 3 +++ 1 file changed, 3 insertions(+) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH 1/1] bash-completion: fix variable leaks of "IFS" and "word"
Thank you for reviewing the patch. 2021年11月23日(火) 21:37 Michal Prívozník : > [...] > > The word variable is also used only inside a while loop (not visible in > the context). So it can be declared local there instead of here. I'll > fix that before pushing. OK. > However, we do require developers to provide signoff for their patches > to signal their compliance with DCO: > > https://libvirt.org/hacking.html#developer-certificate-of-origin > > No need to resend the whole patch, it's sufficient if you reply with > your SoB line. OK. Are just the following lines sufficient? -- [PATCH 1/1] bash-completion: fix variable leaks of "IFS" and "word" Signed-off-by: Koichi Murase -- Thank you, Koichi
Re: [PATCH 1/1] bash-completion: fix variable leaks of "IFS" and "word"
On 11/14/21 07:06, Koichi Murase wrote:
> ---
> tools/bash-completion/vsh.in | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/tools/bash-completion/vsh.in b/tools/bash-completion/vsh.in
> index 70ade50a02..4399ff0a64 100644
> --- a/tools/bash-completion/vsh.in
> +++ b/tools/bash-completion/vsh.in
> @@ -4,7 +4,7 @@
>
> _@command@_complete()
> {
> -local words cword c=0 i=0 cur RO URI CMDLINE INPUT A
> +local words cword c=0 i=0 cur word RO URI CMDLINE INPUT A
The word variable is also used only inside a while loop (not visible in
the context). So it can be declared local there instead of here. I'll
fix that before pushing.
However, we do require developers to provide signoff for their patches
to signal their compliance with DCO:
https://libvirt.org/hacking.html#developer-certificate-of-origin
No need to resend the whole patch, it's sufficient if you reply with
your SoB line.
Michal
Re: [PATCH 7/7] util: Make client-side polkit work even with polkit disabled
On Tue, Nov 23, 2021 at 11:56:40AM +, Daniel P. Berrangé wrote: On Tue, Nov 23, 2021 at 12:55:00PM +0100, Martin Kletzander wrote: On Mon, Nov 22, 2021 at 10:07:39AM +, Daniel P. Berrangé wrote: > On Mon, Nov 22, 2021 at 11:03:53AM +0100, Martin Kletzander wrote: > > On Mon, Nov 22, 2021 at 09:23:01AM +, Daniel P. Berrangé wrote: > > > On Sun, Nov 21, 2021 at 12:10:08AM +0100, Martin Kletzander wrote: > > > > The reason for this is twofold: > > > > > > > > - the polkit build option is documented for UNIX socket access checks > > > > > > > > - there is no server-side change or dbus call done when enabling this as it only > > > > starts a polkit agent on the client-side (actually only in virsh) and does not > > > > need any requirements (starting is skipped if pkttyagent is not installed) > > > > > > > > Also move the conditional implementation to the bottom of the file so that it > > > > does not look like the whole file is build conditionally and the common > > > > functions are at the top. > > > > > > Does this still work correctly on Windows if we try by default ? > > > > > > > Any call to virPolkitAgentAvailable() should return false on Windows > > unless PKTTYAGENT (/usr/bin/pkttyagent) exists. While thinking about it > > now I will change that virFileExists() call to virFileIsExecutable() to > > catch even more possible issues. Anyway since that should return false > > on Windows (and I hope my presumptions are correct) then > > virPolkitAgentCreate() should report an error, just like it would > > without this patch if connecting to polkit-guarded libvirtd socket > > (e.g. through ssh). It should actually return a better error with this > > patch applied. > > > > And ctermid() is a POSIX function, not sure what that returns on > > windows, but it should not even get there as the first check is done > > against the existence/executability of PKTTYAGENT. > > ctermid() doesn't exist in Windows, so it shouldn't even compile if we > try to build with it ! A conditional willbe needed I expect. > Yeah, with my limited (and mostly forgotten Windows experience) I was too much reliant on our builds which all have allow_failure: true for mingw builds. Sorry for that, I'll fix that up. We need to move at least one of our mingw builds off rawhide onto the stable Fedora, so we can remove the allow_failure flag. And thinking about it I will just remove this patch, there is no point in that any more. 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 :| signature.asc Description: PGP signature
Re: [PATCH] meson: fix cpuset_getaffinity() detection
On 11/23/21 12:57, Roman Bogorodskiy wrote: > The cpuset_getaffinity() function is checked in sys/cpuset.h to see if > BSD CPU affinity APIs are available. This check requires including > sys/param.h to work properly, otherwise the test program fails with > unrelated errors like: > > /usr/include/sys/cpuset.h:155:1: error: unknown type name > '__BEGIN_DECLS' > __BEGIN_DECLS > ^ > /usr/include/sys/cpuset.h:156:12: error: unknown type name 'cpusetid_t'; > did you mean 'cpuset_t'? > int cpuset(cpusetid_t *); > > and so forth. > > Signed-off-by: Roman Bogorodskiy > --- > meson.build | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Michal Privoznik Michal
Re: [PATCH] meson: fix cpuset_getaffinity() detection
On Tue, Nov 23, 2021 at 03:57:56PM +0400, Roman Bogorodskiy wrote:
The cpuset_getaffinity() function is checked in sys/cpuset.h to see if
BSD CPU affinity APIs are available. This check requires including
sys/param.h to work properly, otherwise the test program fails with
unrelated errors like:
/usr/include/sys/cpuset.h:155:1: error: unknown type name
'__BEGIN_DECLS'
__BEGIN_DECLS
^
/usr/include/sys/cpuset.h:156:12: error: unknown type name 'cpusetid_t';
did you mean 'cpuset_t'?
int cpuset(cpusetid_t *);
and so forth.
Signed-off-by: Roman Bogorodskiy
Reviewed-by: Martin Kletzander
---
meson.build | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/meson.build b/meson.build
index e4f36e8574..ad0cd44aca 100644
--- a/meson.build
+++ b/meson.build
@@ -709,7 +709,7 @@ if (cc.has_header_symbol('net/if_bridgevar.h', 'BRDGSFD',
prefix: brd_required_h
endif
# Check for BSD CPU affinity availability
-if cc.has_header_symbol('sys/cpuset.h', 'cpuset_getaffinity')
+if cc.has_header_symbol('sys/cpuset.h', 'cpuset_getaffinity', prefix: '#include
')
conf.set('WITH_BSD_CPU_AFFINITY', 1)
endif
--
2.33.1
signature.asc
Description: PGP signature
[PATCH] meson: fix cpuset_getaffinity() detection
The cpuset_getaffinity() function is checked in sys/cpuset.h to see if
BSD CPU affinity APIs are available. This check requires including
sys/param.h to work properly, otherwise the test program fails with
unrelated errors like:
/usr/include/sys/cpuset.h:155:1: error: unknown type name
'__BEGIN_DECLS'
__BEGIN_DECLS
^
/usr/include/sys/cpuset.h:156:12: error: unknown type name 'cpusetid_t';
did you mean 'cpuset_t'?
int cpuset(cpusetid_t *);
and so forth.
Signed-off-by: Roman Bogorodskiy
---
meson.build | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/meson.build b/meson.build
index e4f36e8574..ad0cd44aca 100644
--- a/meson.build
+++ b/meson.build
@@ -709,7 +709,7 @@ if (cc.has_header_symbol('net/if_bridgevar.h', 'BRDGSFD',
prefix: brd_required_h
endif
# Check for BSD CPU affinity availability
-if cc.has_header_symbol('sys/cpuset.h', 'cpuset_getaffinity')
+if cc.has_header_symbol('sys/cpuset.h', 'cpuset_getaffinity', prefix:
'#include ')
conf.set('WITH_BSD_CPU_AFFINITY', 1)
endif
--
2.33.1
Re: [PATCH 7/7] util: Make client-side polkit work even with polkit disabled
On Tue, Nov 23, 2021 at 12:55:00PM +0100, Martin Kletzander wrote: > On Mon, Nov 22, 2021 at 10:07:39AM +, Daniel P. Berrangé wrote: > > On Mon, Nov 22, 2021 at 11:03:53AM +0100, Martin Kletzander wrote: > > > On Mon, Nov 22, 2021 at 09:23:01AM +, Daniel P. Berrangé wrote: > > > > On Sun, Nov 21, 2021 at 12:10:08AM +0100, Martin Kletzander wrote: > > > > > The reason for this is twofold: > > > > > > > > > > - the polkit build option is documented for UNIX socket access checks > > > > > > > > > > - there is no server-side change or dbus call done when enabling this > > > > > as it only > > > > > starts a polkit agent on the client-side (actually only in virsh) > > > > > and does not > > > > > need any requirements (starting is skipped if pkttyagent is not > > > > > installed) > > > > > > > > > > Also move the conditional implementation to the bottom of the file so > > > > > that it > > > > > does not look like the whole file is build conditionally and the > > > > > common > > > > > functions are at the top. > > > > > > > > Does this still work correctly on Windows if we try by default ? > > > > > > > > > > Any call to virPolkitAgentAvailable() should return false on Windows > > > unless PKTTYAGENT (/usr/bin/pkttyagent) exists. While thinking about it > > > now I will change that virFileExists() call to virFileIsExecutable() to > > > catch even more possible issues. Anyway since that should return false > > > on Windows (and I hope my presumptions are correct) then > > > virPolkitAgentCreate() should report an error, just like it would > > > without this patch if connecting to polkit-guarded libvirtd socket > > > (e.g. through ssh). It should actually return a better error with this > > > patch applied. > > > > > > And ctermid() is a POSIX function, not sure what that returns on > > > windows, but it should not even get there as the first check is done > > > against the existence/executability of PKTTYAGENT. > > > > ctermid() doesn't exist in Windows, so it shouldn't even compile if we > > try to build with it ! A conditional willbe needed I expect. > > > > Yeah, with my limited (and mostly forgotten Windows experience) I was > too much reliant on our builds which all have allow_failure: true for > mingw builds. Sorry for that, I'll fix that up. We need to move at least one of our mingw builds off rawhide onto the stable Fedora, so we can remove the allow_failure flag. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 3/7] util: Add virPolkitAgentAvailable
On Mon, Nov 22, 2021 at 04:38:42PM +0100, Ján Tomko wrote: On a Sunday in 2021, Martin Kletzander wrote: With this function we can decide whether to try running the polkit text agent only if it is available, removing a potential needless error saying that the agent binary does not exist, which is useful especially when running the agent before knowing whether it is going to be needed. Signed-off-by: Martin Kletzander --- src/libvirt_private.syms | 1 + src/util/virpolkit.c | 44 src/util/virpolkit.h | 1 + 3 files changed, 46 insertions(+) Reviewed-by: Ján Tomko I changed the virFileExists to virFileIsExecutable as mentioned in the thread for patch 7/7, so I'll amend it here. Hope that's fine, if not then take it as a trivial change ;) Jano signature.asc Description: PGP signature
Re: [PATCH 7/7] util: Make client-side polkit work even with polkit disabled
On Mon, Nov 22, 2021 at 10:07:39AM +, Daniel P. Berrangé wrote: On Mon, Nov 22, 2021 at 11:03:53AM +0100, Martin Kletzander wrote: On Mon, Nov 22, 2021 at 09:23:01AM +, Daniel P. Berrangé wrote: > On Sun, Nov 21, 2021 at 12:10:08AM +0100, Martin Kletzander wrote: > > The reason for this is twofold: > > > > - the polkit build option is documented for UNIX socket access checks > > > > - there is no server-side change or dbus call done when enabling this as it only > > starts a polkit agent on the client-side (actually only in virsh) and does not > > need any requirements (starting is skipped if pkttyagent is not installed) > > > > Also move the conditional implementation to the bottom of the file so that it > > does not look like the whole file is build conditionally and the common > > functions are at the top. > > Does this still work correctly on Windows if we try by default ? > Any call to virPolkitAgentAvailable() should return false on Windows unless PKTTYAGENT (/usr/bin/pkttyagent) exists. While thinking about it now I will change that virFileExists() call to virFileIsExecutable() to catch even more possible issues. Anyway since that should return false on Windows (and I hope my presumptions are correct) then virPolkitAgentCreate() should report an error, just like it would without this patch if connecting to polkit-guarded libvirtd socket (e.g. through ssh). It should actually return a better error with this patch applied. And ctermid() is a POSIX function, not sure what that returns on windows, but it should not even get there as the first check is done against the existence/executability of PKTTYAGENT. ctermid() doesn't exist in Windows, so it shouldn't even compile if we try to build with it ! A conditional willbe needed I expect. Yeah, with my limited (and mostly forgotten Windows experience) I was too much reliant on our builds which all have allow_failure: true for mingw builds. Sorry for that, I'll fix that up. 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 :| signature.asc Description: PGP signature
Re: [PATCH] meson: improve CPU affinity routines check
Martin Kletzander wrote:
> On Mon, Nov 22, 2021 at 03:22:11PM +0400, Roman Bogorodskiy wrote:
> > Martin Kletzander wrote:
> >
> >> On Sun, Nov 21, 2021 at 07:58:55PM +0400, Roman Bogorodskiy wrote:
> >> >Recently, FreeBSD has got sched_get/setaffinity(3) implementations and
> >> >the sched.h header as well [1]. To make these routines visible,
> >> >users have to define _WITH_CPU_SET_T.
> >> >
> >> >This breaks current detection. Specifically, meson sees the
> >> >sched_getaffinity() symbol and defines WITH_SCHED_GETAFFINITY. This
> >> >define unlocks Linux implementation of virProcessSetAffinity() and other
> >> >functions, which fails to build on FreeBSD because cpu_set_t is not
> >> >visible as _WITH_CPU_SET_T is not defined.
> >> >
> >> >For now, change detection to the following:
> >> >
> >> > - Instead of checking sched_getaffinity(), check if 'cpu_set_t' is
> >> > available through sched.h
> >> > - Explicitly check the sched.h header instead of assuming its presence
> >> > if WITH_SCHED_SETSCHEDULER is defined
> >> >
> >>
> >> Makes sense, although even the sched_getaffinity() is guarded by the new
> >> _WITH_CPU_SET_T so there should be no error with the current code, am I
> >
> >Nope, as I tried to explain in the commit message, the current code is
> >not working properly as it's using cc.has_function() for 'sched_getaffinity'
> >and this check passes because it's checking the specified function in
> >the standard library, so it works regardless of _WITH_CPU_SET_T. And as
> >it finds the sched_getaffinity(), the build still fails because we don't
> >set _WITH_CPU_SET_T.
> >
>
> Looking at the patches you linked to it seems like sched_getaffinity()
> is hidden under #ifdef _WITH_CPU_SET_T, which made me think that
> cc.has_function() should not find it.
Apparently, has_function() doesn't need it to present in the header, at
least as I understand from this snippet from meson-logs/meson-log.txt:
Running compile:
Working directory: /usr/home/novel/code/libvirt/build/meson-private/tmpzkxagnh0
Command line: ccache cc
/usr/home/novel/code/libvirt/build/meson-private/tmpzkxagnh0/testfile.c -o
/usr/home/novel/code/libvirt/build/meson-private/tmpzkxagnh0/output.exe
-D_FILE_OFFSET_BITS=64 -O0 -Werror=implicit-function-declaration -std=gnu99
Code:
#define sched_getaffinity meson_disable_define_of_sched_getaffinity
#include
#undef sched_getaffinity
#ifdef __cplusplus
extern "C"
#endif
char sched_getaffinity (void);
#if defined __stub_sched_getaffinity || defined
__stub___sched_getaffinity
fail fail fail this function is not going to work
#endif
int main(void) {
return sched_getaffinity ();
}
Compiler stdout:
Compiler stderr:
Checking for function "sched_getaffinity" : YES
> Anyway the code is fine as it is, I was just wondering.
>
> Reviewed-by: Martin Kletzander
Thanks, will push shortly.
PS Studying meson-log.txt, I also noticed that detection of
cpuset_getaffinity() is also not working properly, I'll address it
separately.
> >> right? And if I understand correctly we cannot use the FreeBSD
> >> implementation as is because they do not have all the CPU_* macros we
> >> need, right?
> >
> >Frankly speaking, I haven't yet tested this implementation. Currently,
> >virprocess.c is using the original FreeBSD interface:
> >cpuset_(get|set)affinity.
> >Generally, it would be a good thing to use the same implementation for
> >both Linux and FreeBSD, however, this Linux-compatible interface in
> >FreeBSD is not yet available in any FreeBSD release, so we'll have to keep
> >the old code for some time anyway, and also I need to test if it
> >actually works for us and figure out how to better detect and pass this
> >_WITH_CPU_SET_T
> >with meson.
> >
>
> Maybe one day =)
>
> >> >1:
> >> >https://cgit.freebsd.org/src/commit/?id=43736b71dd051212d5c55be9fa21c45993017fbb
> >> >https://cgit.freebsd.org/src/commit/?id=160b4b922b6021848b6b48afc894d16b879b7af2
> >> >https://cgit.freebsd.org/src/commit/?id=90fa9705d5cd29cf11c5dc7319299788dec2546a
> >> >
> >> >Signed-off-by: Roman Bogorodskiy
> >> >---
> >> > meson.build | 4 +++-
> >> > src/util/virprocess.c | 8
> >> > 2 files changed, 7 insertions(+), 5 deletions(-)
> >> >
> >> >diff --git a/meson.build b/meson.build
> >> >index 9022bcfdc9..e4f36e8574 100644
> >> >--- a/meson.build
> >> >+++ b/meson.build
> >> >@@ -553,7 +553,6 @@ functions = [
> >> > 'posix_fallocate',
> >> > 'posix_memalign',
> >> > 'prlimit',
> >> >- 'sched_getaffinity',
> >> > 'sched_setscheduler',
> >> > 'setgroups',
> >> > 'setns',
> >> >@@ -602,6 +601,7 @@ headers = [
> >> > 'net/if.h',
> >> > 'pty.h',
> >> > 'pwd.h',
> >> >+ 'sched.h',
> >> > 'sys/auxv.h',
> >> > 'sys/ioctl.h',
> >> > 'sys/mount.h',
> >> >@@ -671,6 +671,8 @@ symbols = [
> >> >
> >> > # Check for BSD approach for setting MAC addr
> >> > [
Re: [PATCH] meson: improve CPU affinity routines check
On Mon, Nov 22, 2021 at 03:22:11PM +0400, Roman Bogorodskiy wrote:
Martin Kletzander wrote:
On Sun, Nov 21, 2021 at 07:58:55PM +0400, Roman Bogorodskiy wrote:
>Recently, FreeBSD has got sched_get/setaffinity(3) implementations and
>the sched.h header as well [1]. To make these routines visible,
>users have to define _WITH_CPU_SET_T.
>
>This breaks current detection. Specifically, meson sees the
>sched_getaffinity() symbol and defines WITH_SCHED_GETAFFINITY. This
>define unlocks Linux implementation of virProcessSetAffinity() and other
>functions, which fails to build on FreeBSD because cpu_set_t is not
>visible as _WITH_CPU_SET_T is not defined.
>
>For now, change detection to the following:
>
> - Instead of checking sched_getaffinity(), check if 'cpu_set_t' is
> available through sched.h
> - Explicitly check the sched.h header instead of assuming its presence
> if WITH_SCHED_SETSCHEDULER is defined
>
Makes sense, although even the sched_getaffinity() is guarded by the new
_WITH_CPU_SET_T so there should be no error with the current code, am I
Nope, as I tried to explain in the commit message, the current code is
not working properly as it's using cc.has_function() for 'sched_getaffinity'
and this check passes because it's checking the specified function in
the standard library, so it works regardless of _WITH_CPU_SET_T. And as
it finds the sched_getaffinity(), the build still fails because we don't
set _WITH_CPU_SET_T.
Looking at the patches you linked to it seems like sched_getaffinity()
is hidden under #ifdef _WITH_CPU_SET_T, which made me think that
cc.has_function() should not find it.
Anyway the code is fine as it is, I was just wondering.
Reviewed-by: Martin Kletzander
right? And if I understand correctly we cannot use the FreeBSD
implementation as is because they do not have all the CPU_* macros we
need, right?
Frankly speaking, I haven't yet tested this implementation. Currently,
virprocess.c is using the original FreeBSD interface: cpuset_(get|set)affinity.
Generally, it would be a good thing to use the same implementation for
both Linux and FreeBSD, however, this Linux-compatible interface in
FreeBSD is not yet available in any FreeBSD release, so we'll have to keep
the old code for some time anyway, and also I need to test if it
actually works for us and figure out how to better detect and pass this
_WITH_CPU_SET_T
with meson.
Maybe one day =)
>1:
>https://cgit.freebsd.org/src/commit/?id=43736b71dd051212d5c55be9fa21c45993017fbb
>https://cgit.freebsd.org/src/commit/?id=160b4b922b6021848b6b48afc894d16b879b7af2
>https://cgit.freebsd.org/src/commit/?id=90fa9705d5cd29cf11c5dc7319299788dec2546a
>
>Signed-off-by: Roman Bogorodskiy
>---
> meson.build | 4 +++-
> src/util/virprocess.c | 8
> 2 files changed, 7 insertions(+), 5 deletions(-)
>
>diff --git a/meson.build b/meson.build
>index 9022bcfdc9..e4f36e8574 100644
>--- a/meson.build
>+++ b/meson.build
>@@ -553,7 +553,6 @@ functions = [
> 'posix_fallocate',
> 'posix_memalign',
> 'prlimit',
>- 'sched_getaffinity',
> 'sched_setscheduler',
> 'setgroups',
> 'setns',
>@@ -602,6 +601,7 @@ headers = [
> 'net/if.h',
> 'pty.h',
> 'pwd.h',
>+ 'sched.h',
> 'sys/auxv.h',
> 'sys/ioctl.h',
> 'sys/mount.h',
>@@ -671,6 +671,8 @@ symbols = [
>
> # Check for BSD approach for setting MAC addr
> [ 'net/if_dl.h', 'link_addr', '#include \n#include
' ],
>+
>+ [ 'sched.h', 'cpu_set_t' ],
> ]
>
> if host_machine.system() == 'linux'
>diff --git a/src/util/virprocess.c b/src/util/virprocess.c
>index 6de3f36f52..81de90200e 100644
>--- a/src/util/virprocess.c
>+++ b/src/util/virprocess.c
>@@ -35,7 +35,7 @@
> # include
> # include
> #endif
>-#if WITH_SCHED_SETSCHEDULER
>+#if WITH_SCHED_H
> # include
> #endif
>
>@@ -480,7 +480,7 @@ int virProcessKillPainfully(pid_t pid, bool force)
> return virProcessKillPainfullyDelay(pid, force, 0, false);
> }
>
>-#if WITH_SCHED_GETAFFINITY
>+#if WITH_DECL_CPU_SET_T
>
> int virProcessSetAffinity(pid_t pid, virBitmap *map, bool quiet)
> {
>@@ -626,7 +626,7 @@ virProcessGetAffinity(pid_t pid)
> return ret;
> }
>
>-#else /* WITH_SCHED_GETAFFINITY */
>+#else /* WITH_DECL_CPU_SET_T */
>
> int virProcessSetAffinity(pid_t pid G_GNUC_UNUSED,
> virBitmap *map G_GNUC_UNUSED,
>@@ -646,7 +646,7 @@ virProcessGetAffinity(pid_t pid G_GNUC_UNUSED)
> _("Process CPU affinity is not supported on this
platform"));
> return NULL;
> }
>-#endif /* WITH_SCHED_GETAFFINITY */
>+#endif /* WITH_DECL_CPU_SET_T */
>
>
> int virProcessGetPids(pid_t pid, size_t *npids, pid_t **pids)
>--
>2.33.1
>
Roman Bogorodskiy
signature.asc
Description: PGP signature
[PATCH v3 1/2] util: Add virProcessGetStat
This reads and separates all fields from /proc//stat or
/proc//task//stat as there are easy mistakes to be done in the
implementation. Some tests are added to show it works correctly. No number
parsing is done as it would be unused for most of the fields most, if not all,
of the time. No struct is used for the result as the length can vary (new
fields can be added in the future).
Signed-off-by: Martin Kletzander
---
src/libvirt_private.syms | 1 +
src/util/virprocess.c | 77 +++
src/util/virprocess.h | 66
tests/meson.build | 1 +
tests/virprocessstatdata/complex/stat | 2 +
tests/virprocessstatdata/simple/stat | 1 +
tests/virprocessstattest.c| 88 +++
7 files changed, 236 insertions(+)
create mode 100644 tests/virprocessstatdata/complex/stat
create mode 100644 tests/virprocessstatdata/simple/stat
create mode 100644 tests/virprocessstattest.c
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index a7bc50a4d16d..7a4e7d7d6f40 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -3099,6 +3099,7 @@ virProcessGetMaxMemLock;
virProcessGetNamespaces;
virProcessGetPids;
virProcessGetStartTime;
+virProcessGetStat;
virProcessGroupGet;
virProcessGroupKill;
virProcessKill;
diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index 6de3f36f529c..aab51962acfc 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -1721,3 +1721,80 @@ virProcessSetScheduler(pid_t pid G_GNUC_UNUSED,
}
#endif /* !WITH_SCHED_SETSCHEDULER */
+
+/*
+ * Get all stat fields for a process based on pid and tid:
+ * - pid == 0 && tid == 0 => /proc/self/stat
+ * - pid != 0 && tid == 0 => /proc//stat
+ * - pid == 0 && tid != 0 => /proc/self/task//stat
+ * - pid != 0 && tid != 0 => /proc//task//stat
+ * and return them as array of strings.
+ */
+GStrv
+virProcessGetStat(pid_t pid,
+ pid_t tid)
+{
+int len = 10 * 1024; /* 10kB ought to be enough for everyone */
+g_autofree char *buf = NULL;
+g_autofree char *path = NULL;
+GStrv rest = NULL;
+GStrv ret = NULL;
+char *comm = NULL;
+char *rparen = NULL;
+size_t nrest = 0;
+
+if (pid) {
+if (tid)
+path = g_strdup_printf("/proc/%d/task/%d/stat", (int)pid,
(int)tid);
+else
+path = g_strdup_printf("/proc/%d/stat", (int)pid);
+} else {
+if (tid)
+path = g_strdup_printf("/proc/self/task/%d/stat", (int)tid);
+else
+path = g_strdup("/proc/self/stat");
+}
+
+len = virFileReadAllQuiet(path, len, );
+if (len < 0)
+return NULL;
+
+/* eliminate trailing spaces */
+while (len > 0 && g_ascii_isspace(buf[--len]))
+ buf[len] = '\0';
+
+/* Find end of the first field */
+if (!(comm = strchr(buf, ' ')))
+return NULL;
+*comm = '\0';
+
+/* Check start of the second field (filename of the executable, in
+ * parentheses) */
+comm++;
+if (*comm != '(')
+return NULL;
+comm++;
+
+/* Check end of the second field (last closing parenthesis) */
+rparen = strrchr(comm, ')');
+if (!rparen)
+return NULL;
+*rparen = '\0';
+
+/* We need to check that the next char is not '\0', but why not just opt in
+ * for the safer way of checking whether it is ' ' (space) instead */
+if (rparen[1] != ' ')
+return NULL;
+
+rest = g_strsplit(rparen + 2, " ", 0);
+nrest = g_strv_length(rest);
+ret = g_new0(char *, nrest + 3);
+ret[0] = g_strdup(buf);
+ret[1] = g_strdup(comm);
+memcpy(ret + 2, rest, nrest * sizeof(char *));
+
+/* Do not use g_strfreev() as individual elements they were moved to @ret.
*/
+VIR_FREE(rest);
+
+return ret;
+}
diff --git a/src/util/virprocess.h b/src/util/virprocess.h
index 9910331a0caa..82b740396499 100644
--- a/src/util/virprocess.h
+++ b/src/util/virprocess.h
@@ -117,6 +117,72 @@ int virProcessSetupPrivateMountNS(void);
int virProcessSetScheduler(pid_t pid,
virProcessSchedPolicy policy,
int priority);
+
+GStrv virProcessGetStat(pid_t pid, pid_t tid);
+
+/* These constants are modelled after proc(5) */
+enum {
+VIR_PROCESS_STAT_PID,
+VIR_PROCESS_STAT_COMM,
+VIR_PROCESS_STAT_STATE,
+VIR_PROCESS_STAT_PPID,
+VIR_PROCESS_STAT_PGRP,
+VIR_PROCESS_STAT_SESSION,
+VIR_PROCESS_STAT_TTY_NR,
+VIR_PROCESS_STAT_TPGID,
+VIR_PROCESS_STAT_FLAGS,
+VIR_PROCESS_STAT_MINFLT,
+VIR_PROCESS_STAT_CMINFLT,
+VIR_PROCESS_STAT_MAJFLT,
+VIR_PROCESS_STAT_CMAJFLT,
+VIR_PROCESS_STAT_UTIME,
+VIR_PROCESS_STAT_STIME,
+VIR_PROCESS_STAT_CUTIME,
+VIR_PROCESS_STAT_CSTIME,
+VIR_PROCESS_STAT_PRIORITY,
+VIR_PROCESS_STAT_NICE,
+VIR_PROCESS_STAT_NUM_THREADS,
+VIR_PROCESS_STAT_ITREALVALUE,
+
[PATCH v3 0/2] Fix /proc/*/stat parsing
While working on some polkit stuff I found out that we are inconsistent with the way we parse /proc/*/stat files, so I added a new helper instead along with some tests. Unfortunately using it for the thing I wanted is not really viable in the end, so it "violates" the Rule of three, but at least it does something correctly. v3: - Added an enum with names for the fields of the file - Made this not limited to linux as that was the way it worked in qemu before v2: - https://listman.redhat.com/archives/libvir-list/2021-November/msg00606.html - Fixed open64 by just using virFileReadAllQuiet instead of g_file_get_contents - Removed some leftover unused variables - Still do not know why my cirrus builds fail v1: - https://listman.redhat.com/archives/libvir-list/2021-November/msg00580.html Martin Kletzander (2): util: Add virProcessGetStat Use virProcessGetStat src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c| 34 ++- src/util/virprocess.c | 125 +- src/util/virprocess.h | 66 ++ tests/meson.build | 1 + tests/virprocessstatdata/complex/stat | 2 + tests/virprocessstatdata/simple/stat | 1 + tests/virprocessstattest.c| 88 ++ 8 files changed, 249 insertions(+), 69 deletions(-) create mode 100644 tests/virprocessstatdata/complex/stat create mode 100644 tests/virprocessstatdata/simple/stat create mode 100644 tests/virprocessstattest.c -- 2.34.0
[PATCH v3 2/2] Use virProcessGetStat
This eliminates one incorrect parsing implementation which relied on the
command field not having a closing bracket. This possibility is already
tested against in the virProcessGetStat() tests.
Signed-off-by: Martin Kletzander
---
src/qemu/qemu_driver.c | 34 ++
src/util/virprocess.c | 48 ++
2 files changed, 13 insertions(+), 69 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index d954635dde2a..16d449913ca7 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1399,36 +1399,18 @@ qemuGetSchedInfo(unsigned long long *cpuWait,
static int
qemuGetProcessInfo(unsigned long long *cpuTime, int *lastCpu, long *vm_rss,
- pid_t pid, int tid)
+ pid_t pid, pid_t tid)
{
-g_autofree char *proc = NULL;
-FILE *pidinfo;
+g_auto(GStrv) proc_stat = virProcessGetStat(pid, tid);
unsigned long long usertime = 0, systime = 0;
long rss = 0;
int cpu = 0;
-/* In general, we cannot assume pid_t fits in int; but /proc parsing
- * is specific to Linux where int works fine. */
-if (tid)
-proc = g_strdup_printf("/proc/%d/task/%d/stat", (int)pid, tid);
-else
-proc = g_strdup_printf("/proc/%d/stat", (int)pid);
-if (!proc)
-return -1;
-
-pidinfo = fopen(proc, "r");
-
-/* See 'man proc' for information about what all these fields are. We're
- * only interested in a very few of them */
-if (!pidinfo ||
-fscanf(pidinfo,
- /* pid -> stime */
- "%*d (%*[^)]) %*c %*d %*d %*d %*d %*d %*u %*u %*u %*u %*u %llu
%llu"
- /* cutime -> endcode */
- "%*d %*d %*d %*d %*d %*d %*u %*u %ld %*u %*u %*u"
- /* startstack -> processor */
- "%*u %*u %*u %*u %*u %*u %*u %*u %*u %*u %*d %d",
- , , , ) != 4) {
+if (!proc_stat ||
+virStrToLong_ullp(proc_stat[VIR_PROCESS_STAT_UTIME], NULL, 10,
) < 0 ||
+virStrToLong_ullp(proc_stat[VIR_PROCESS_STAT_STIME], NULL, 10,
) < 0 ||
+virStrToLong_l(proc_stat[VIR_PROCESS_STAT_RSS], NULL, 10, ) < 0 ||
+virStrToLong_i(proc_stat[VIR_PROCESS_STAT_PROCESSOR], NULL, 10, )
< 0) {
VIR_WARN("cannot parse process status data");
}
@@ -1450,8 +1432,6 @@ qemuGetProcessInfo(unsigned long long *cpuTime, int
*lastCpu, long *vm_rss,
VIR_DEBUG("Got status for %d/%d user=%llu sys=%llu cpu=%d rss=%ld",
(int)pid, tid, usertime, systime, cpu, rss);
-VIR_FORCE_FCLOSE(pidinfo);
-
return 0;
}
diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index aab51962acfc..20d04bad6f26 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -1153,56 +1153,20 @@ virProcessSetMaxCoreSize(pid_t pid G_GNUC_UNUSED,
int virProcessGetStartTime(pid_t pid,
unsigned long long *timestamp)
{
-char *tmp;
-int len;
-g_autofree char *filename = NULL;
-g_autofree char *buf = NULL;
-g_auto(GStrv) tokens = NULL;
-
-filename = g_strdup_printf("/proc/%llu/stat", (long long)pid);
-
-if ((len = virFileReadAll(filename, 1024, )) < 0)
-return -1;
+g_auto(GStrv) proc_stat = virProcessGetStat(pid, 0);
-/* start time is the token at index 19 after the '(process name)' entry -
since only this
- * field can contain the ')' character, search backwards for this to avoid
malicious
- * processes trying to fool us
- */
-
-if (!(tmp = strrchr(buf, ')'))) {
+if (!proc_stat || g_strv_length(proc_stat) < 22) {
virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Cannot find start time in %s"),
- filename);
+ _("Cannot find start time for pid %d"), (int)pid);
return -1;
}
-tmp += 2; /* skip ') ' */
-if ((tmp - buf) >= len) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Cannot find start time in %s"),
- filename);
-return -1;
-}
-
-tokens = g_strsplit(tmp, " ", 0);
-if (!tokens ||
-g_strv_length(tokens) < 20) {
+if (virStrToLong_ull(proc_stat[21], NULL, 10, timestamp) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Cannot find start time in %s"),
- filename);
+ _("Cannot parse start time %s for pid %d"),
+ proc_stat[21], (int)pid);
return -1;
}
-
-if (virStrToLong_ull(tokens[19],
- NULL,
- 10,
- timestamp) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Cannot parse start time %s in %s"),
- tokens[19], filename);
-return -1;
-}
-
return 0;
}
#elif defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
--
2.34.0
Re: [PATCH v2 2/2] Use virProcessGetStat
On Mon, Nov 22, 2021 at 09:59:07AM +, Daniel P. Berrangé wrote:
On Mon, Nov 22, 2021 at 10:34:38AM +0100, Martin Kletzander wrote:
On Mon, Nov 22, 2021 at 09:18:41AM +, Daniel P. Berrangé wrote:
> On Sun, Nov 21, 2021 at 12:04:26AM +0100, Martin Kletzander wrote:
> > This eliminates one incorrect parsing implementation.
>
> Please explain what was being done wrongly / what was the
> effect of the bug ?
>
One of the implementations was just looking for first closing
parenthesis to find the end of the command name, which should be done by
looking at the _last_ closing parenthesis. This might fail in a very
small corner case which is tested for in the first patch. But you are
right, I should add this to the commit message. Will do in v3.
> >
> > Signed-off-by: Martin Kletzander
> > ---
> > src/qemu/qemu_driver.c | 33 ++---
> > src/util/virprocess.c | 48 ++
> > 2 files changed, 12 insertions(+), 69 deletions(-)
> >
> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > index d954635dde2a..0468d6aaf314 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c
> > @@ -1399,36 +1399,17 @@ qemuGetSchedInfo(unsigned long long *cpuWait,
> >
> > static int
> > qemuGetProcessInfo(unsigned long long *cpuTime, int *lastCpu, long *vm_rss,
> > - pid_t pid, int tid)
> > + pid_t pid, pid_t tid)
> > {
> > -g_autofree char *proc = NULL;
> > -FILE *pidinfo;
> > +g_auto(GStrv) proc_stat = virProcessGetStat(pid, tid);
> > unsigned long long usertime = 0, systime = 0;
> > long rss = 0;
> > int cpu = 0;
> >
> > -/* In general, we cannot assume pid_t fits in int; but /proc parsing
> > - * is specific to Linux where int works fine. */
> > -if (tid)
> > -proc = g_strdup_printf("/proc/%d/task/%d/stat", (int)pid, tid);
> > -else
> > -proc = g_strdup_printf("/proc/%d/stat", (int)pid);
> > -if (!proc)
> > -return -1;
> > -
> > -pidinfo = fopen(proc, "r");
> > -
> > -/* See 'man proc' for information about what all these fields are.
We're
> > - * only interested in a very few of them */
> > -if (!pidinfo ||
> > -fscanf(pidinfo,
> > - /* pid -> stime */
> > - "%*d (%*[^)]) %*c %*d %*d %*d %*d %*d %*u %*u %*u %*u %*u %llu
%llu"
> > - /* cutime -> endcode */
> > - "%*d %*d %*d %*d %*d %*d %*u %*u %ld %*u %*u %*u"
> > - /* startstack -> processor */
> > - "%*u %*u %*u %*u %*u %*u %*u %*u %*u %*u %*d %d",
> > - , , , ) != 4) {
> > +if (virStrToLong_ullp(proc_stat[13], NULL, 10, ) < 0 ||
> > +virStrToLong_ullp(proc_stat[14], NULL, 10, ) < 0 ||
> > +virStrToLong_l(proc_stat[23], NULL, 10, ) < 0 ||
> > +virStrToLong_i(proc_stat[38], NULL, 10, ) < 0) {
>
> Since you're adding a formal API, I think we'd benefit from
> constants for these array indexes in virprocess.h
>
I was thinking about that and also tried figuring out how to encode the
proper field types in the header file. But since we are not doing lot
of /proc/*/stat parsing in our codebase I though that would be an
overkill. I'll add at least the constants.
BTW ,didn't mean we need constants for all 40+ fields - just the ones
we actually use.
oops, too late, at least it is now complete =)
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 :|
signature.asc
Description: PGP signature
Re: [PATCH 08/12] virSecurityLabelDefParseXML: Don't use 'virXPathStringLimit'
On Mon, Nov 22, 2021 at 21:01:58 +0100, Tim Wiederhake wrote:
> On Mon, 2021-11-22 at 18:12 +0100, Peter Krempa wrote:
> > virXPathStringLimit doesn't give callers a way to differentiate
> > between
> > the queried XPath being empty and the length limit being exceeded.
> >
> > This means that callers are either overwriting the error message or
> > ignoring it altogether.
> >
> > Move the length checks into the caller.
> >
> > Signed-off-by: Peter Krempa
> > ---
> > src/conf/domain_conf.c | 22 ++
> > 1 file changed, 14 insertions(+), 8 deletions(-)
> >
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index ee44bbbd4b..bd9da0744d 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -7871,9 +7871,9 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr
> > ctxt,
> > if (seclabel->type == VIR_DOMAIN_SECLABEL_STATIC ||
> > (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) &&
> > seclabel->type != VIR_DOMAIN_SECLABEL_NONE)) {
> > - seclabel->label = virXPathStringLimit("string(./label[1])",
> > -
> > VIR_SECURITY_LABEL_BUFLEN-1, ctxt);
> > - if (!seclabel->label) {
> > + seclabel->label = virXPathString("string(./label[1])",
> > ctxt);
> > + if (!seclabel->label ||
> > + strlen(seclabel->label) >= VIR_SECURITY_LABEL_BUFLEN -
> > 1) {
>
> What is your opinion on using strnlen instead? Not necessarily for
> security reasons, but since we do not care for the exact string length
> if it is above a certain size, we can return early.
Yeah, selling it as security is impossible because we already have at
least two copies of the XML in memory.
As a optimization sure, but let's see whether it doesn't make the code
too bulky as the lenght constant appears twice in the expression.
>
> > virReportError(VIR_ERR_XML_ERROR,
> > "%s", _("security label is missing"));
> > return NULL;
> > @@ -7884,9 +7884,10 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr
> > ctxt,
> > if (seclabel->relabel &&
> > (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) &&
> > seclabel->type != VIR_DOMAIN_SECLABEL_NONE)) {
> > - seclabel->imagelabel =
> > virXPathStringLimit("string(./imagelabel[1])",
> > -
> > VIR_SECURITY_LABEL_BUFLEN-1, ctxt);
> > - if (!seclabel->imagelabel) {
> > + seclabel->imagelabel =
> > virXPathString("string(./imagelabel[1])", ctxt);
> > +
> > + if (!seclabel->imagelabel ||
> > + strlen(seclabel->imagelabel) >=
> > VIR_SECURITY_LABEL_BUFLEN - 1) {
> > virReportError(VIR_ERR_XML_ERROR,
> > "%s", _("security imagelabel is
> > missing"));
> > return NULL;
> > @@ -7895,8 +7896,13 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr
> > ctxt,
> >
> > /* Only parse baselabel for dynamic label type */
> > if (seclabel->type == VIR_DOMAIN_SECLABEL_DYNAMIC) {
> > - seclabel->baselabel =
> > virXPathStringLimit("string(./baselabel[1])",
> > -
> > VIR_SECURITY_LABEL_BUFLEN-1, ctxt);
> > + seclabel->baselabel =
> > virXPathString("string(./baselabel[1])", ctxt);
> > +
> > + if (seclabel->baselabel &&
> > + strlen(seclabel->baselabel) >= VIR_SECURITY_LABEL_BUFLEN
> > - 1) {
> > + g_free(seclabel->baselabel);
> > + seclabel->baselabel = NULL;
> > + }
>
> g_clear_pointer?
Yeah, I've actually used it in later patch.
Re: [libvirt PATCH 06.5/12] test: snapshot revert: properly emulate starting CPUs
On Tue, Nov 23, 2021 at 09:20:20 +0100, Pavel Hrdina wrote: > When active snapshot is reverted we stop CPUs in order to load the > snapshot but we never start the CPUs again. > > Signed-off-by: Pavel Hrdina > --- > src/test/test_driver.c | 3 +++ > tests/virsh-snapshot | 14 +++--- > 2 files changed, 10 insertions(+), 7 deletions(-) Reviewed-by: Peter Krempa
Re: [PATCH 09/12] virSecurityDeviceLabelDefParseXML: Use automatic memory clearing for temp strings
On Mon, Nov 22, 2021 at 21:02:01 +0100, Tim Wiederhake wrote:
> On Mon, 2021-11-22 at 18:12 +0100, Peter Krempa wrote:
> > Apart from code simplification the refactor of 'model' fixes an
> > unlikely
> > memory leak of the string if a duplicate model is found.
> >
> > While the coversion of 'label' variable may seem unnecessary it will
> > come in handy in the next patch.
> >
> > Signed-off-by: Peter Krempa
> > ---
> > src/conf/domain_conf.c | 14 +++---
> > 1 file changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index bd9da0744d..e829511ac5 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -8016,7 +8016,10 @@
> > virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDef
> > ***seclabels_rtn,
> > size_t nseclabels = 0;
> > int n;
> > size_t i, j;
> > - char *model, *relabel, *label, *labelskip;
> > + g_autofree char *model = NULL;
> > + g_autofree char *relabel = NULL;
> > + g_autofree char *label = NULL;
> > + g_autofree char *labelskip = NULL;
> > g_autofree xmlNodePtr *list = NULL;
> >
> > if ((n = virXPathNodeSet("./seclabel", ctxt, )) < 0)
> > @@ -8041,7 +8044,7 @@
> > virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDef
> > ***seclabels_rtn,
> > goto error;
> > }
> > }
> > - seclabels[i]->model = model;
> > + seclabels[i]->model = g_steal_pointer();
> > }
> >
> > relabel = virXMLPropString(list[i], "relabel");
> > @@ -8050,10 +8053,8 @@
> > virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDef
> > ***seclabels_rtn,
> > virReportError(VIR_ERR_XML_ERROR,
> > _("invalid security relabel value
> > %s"),
> > relabel);
> > - VIR_FREE(relabel);
> > goto error;
> > }
> > - VIR_FREE(relabel);
> > } else {
> > seclabels[i]->relabel = true;
> > }
> > @@ -8063,14 +8064,13 @@
> > virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDef
> > ***seclabels_rtn,
> > seclabels[i]->labelskip = false;
> > if (labelskip && !(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE))
> > ignore_value(virStringParseYesNo(labelskip,
> > [i]->labelskip));
> > - VIR_FREE(labelskip);
> >
> > ctxt->node = list[i];
> > label = virXPathStringLimit("string(./label)",
> > VIR_SECURITY_LABEL_BUFLEN-1,
> > ctxt);
> > - seclabels[i]->label = label;
> > + seclabels[i]->label = g_steal_pointer();
> >
> > - if (label && !seclabels[i]->relabel) {
> > + if (seclabels[i]->label && !seclabels[i]->relabel) {
> > virReportError(VIR_ERR_XML_ERROR,
> > _("Cannot specify a label if relabelling
> > is "
> > "turned off. model=%s"),
>
> Theese look like they could be simplified to three calls to
> `virXMLPropTristateBool`
Indeed, 'relabel' and 'labelksip' can be parsed into a temp tristate and
then set the boolean in the label from it. I don't want to go further to
refactor all security labelling code to a real tristate.
[libvirt PATCH 06.5/12] test: snapshot revert: properly emulate starting CPUs
When active snapshot is reverted we stop CPUs in order to load the
snapshot but we never start the CPUs again.
Signed-off-by: Pavel Hrdina
---
src/test/test_driver.c | 3 +++
tests/virsh-snapshot | 14 +++---
2 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index c17ed9d2a4..985f08ea1f 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -9144,6 +9144,9 @@ testDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
virObjectUnref(event);
event = NULL;
+virDomainObjSetState(vm, VIR_DOMAIN_RUNNING,
+ VIR_DOMAIN_PAUSED_FROM_SNAPSHOT);
+
if (was_stopped) {
/* Transition 2 */
event = virDomainEventLifecycleNewFromObj(vm,
diff --git a/tests/virsh-snapshot b/tests/virsh-snapshot
index 289de5b2db..4c64bb537b 100755
--- a/tests/virsh-snapshot
+++ b/tests/virsh-snapshot
@@ -137,25 +137,25 @@ Domain snapshot s1 deleted
Name Creation Time State
-
s3 TIMESTAMP running
- s7 TIMESTAMP paused
+ s7 TIMESTAMP running
Name Creation Time State
-
s2 TIMESTAMP running
- s4 TIMESTAMP paused
- s5 TIMESTAMP paused
- s8 TIMESTAMP paused
+ s4 TIMESTAMP running
+ s5 TIMESTAMP running
+ s8 TIMESTAMP running
Name Creation Time State Parent
--
s3 TIMESTAMP running
- s6 TIMESTAMP pauseds3
- s7 TIMESTAMP paused
+ s6 TIMESTAMP running s3
+ s7 TIMESTAMP running
Name Creation Time State
-
s2 TIMESTAMP running
- s6 TIMESTAMP paused
+ s6 TIMESTAMP running
s2
s4
--
2.31.1
