Re: [libvirt] [PATCH] tpm: Run swtpm_setup with less parameters on incoming migration
Hi
On Sat, Jul 27, 2019 at 12:41 AM Stefan Berger wrote:
>
> In case of an incoming migration we do not need to run swtpm_setup
> with all the parameters but only want to get the benefit of it
> creating a TPM state file for us that we can then label with an
> SELinux label. The actual state will be overwritten by the in-
> coming state. So we have to pass an indicator for incomingMigration
> all the way to the command line parameter generation for swtpm_setup.
>
> Signed-off-by: Stefan Berger
iirc, I needed to pass it down as well in my slirp-helper series.
Reviewed-by: Marc-André Lureau
> ---
> src/qemu/qemu_extdevice.c | 5 ++--
> src/qemu/qemu_extdevice.h | 3 ++-
> src/qemu/qemu_process.c | 2 +-
> src/qemu/qemu_tpm.c | 49 +--
> src/qemu/qemu_tpm.h | 3 ++-
> 5 files changed, 40 insertions(+), 22 deletions(-)
>
> diff --git a/src/qemu/qemu_extdevice.c b/src/qemu/qemu_extdevice.c
> index e576bca165..af52466421 100644
> --- a/src/qemu/qemu_extdevice.c
> +++ b/src/qemu/qemu_extdevice.c
> @@ -128,7 +128,8 @@ qemuExtDevicesCleanupHost(virQEMUDriverPtr driver,
> int
> qemuExtDevicesStart(virQEMUDriverPtr driver,
> virDomainObjPtr vm,
> -qemuDomainLogContextPtr logCtxt)
> +qemuDomainLogContextPtr logCtxt,
> +bool incomingMigration)
> {
> int ret = 0;
>
> @@ -136,7 +137,7 @@ qemuExtDevicesStart(virQEMUDriverPtr driver,
> return -1;
>
> if (vm->def->tpm)
> -ret = qemuExtTPMStart(driver, vm, logCtxt);
> +ret = qemuExtTPMStart(driver, vm, logCtxt, incomingMigration);
>
> return ret;
> }
> diff --git a/src/qemu/qemu_extdevice.h b/src/qemu/qemu_extdevice.h
> index bbdb9a1cc2..5a53c79f38 100644
> --- a/src/qemu/qemu_extdevice.h
> +++ b/src/qemu/qemu_extdevice.h
> @@ -40,7 +40,8 @@ void qemuExtDevicesCleanupHost(virQEMUDriverPtr driver,
>
> int qemuExtDevicesStart(virQEMUDriverPtr driver,
> virDomainObjPtr vm,
> -qemuDomainLogContextPtr logCtxt)
> +qemuDomainLogContextPtr logCtxt,
> +bool incomingMigration)
> ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
> ATTRIBUTE_RETURN_CHECK;
>
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 75205bc121..fae18824ba 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -6774,7 +6774,7 @@ qemuProcessLaunch(virConnectPtr conn,
> if (qemuProcessGenID(vm, flags) < 0)
> goto cleanup;
>
> -if (qemuExtDevicesStart(driver, vm, logCtxt) < 0)
> +if (qemuExtDevicesStart(driver, vm, logCtxt, incoming != NULL) < 0)
> goto cleanup;
>
> VIR_DEBUG("Building emulator command line");
> diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
> index 77ef601f74..4174aa4c62 100644
> --- a/src/qemu/qemu_tpm.c
> +++ b/src/qemu/qemu_tpm.c
> @@ -453,6 +453,7 @@ qemuTPMSetupEncryption(const unsigned char *secretuuid,
> * for the user given by userid or 'tss'
> * @tpmversion: The version of the TPM, either a TPM 1.2 or TPM 2
> * @encryption: pointer to virStorageEncryption holding secret
> + * @incomingMigration: whether we have an incoming migration
> *
> * Setup the external swtpm by creating endorsement key and
> * certificates for it.
> @@ -466,7 +467,8 @@ qemuTPMEmulatorRunSetup(const char *storagepath,
> gid_t swtpm_group,
> const char *logfile,
> const virDomainTPMVersion tpmversion,
> -const unsigned char *secretuuid)
> +const unsigned char *secretuuid,
> +bool incomingMigration)
> {
> virCommandPtr cmd = NULL;
> int exitstatus;
> @@ -525,16 +527,23 @@ qemuTPMEmulatorRunSetup(const char *storagepath,
> pwdfile_fd = -1;
> }
>
> -virCommandAddArgList(cmd,
> - "--tpm-state", storagepath,
> - "--vmid", vmid,
> - "--logfile", logfile,
> - "--createek",
> - "--create-ek-cert",
> - "--create-platform-cert",
> - "--lock-nvram",
> - "--not-overwrite",
> - NULL);
> +if (!incomingMigration) {
> +virCommandAddArgList(cmd,
> + "--tpm-state", storagepath,
> + "--vmid", vmid,
> + "--logfile", logfile,
> + "--createek",
> + "--create-ek-cert",
> + "--create-platform-cert",
> + "--lock-nvram",
> + "--not-overwrite",
> + NULL);
> +} else {
> +
[libvirt] virsh command to list CPU, Memory and Storage of all the running VM's
Hi, Is there a way to find out total CPU's, Memory and Storage of all the running VM's using virsh command? For example virsh list --all list out all VM's 11dockerregistry01 running 12gitlab running Any help will be highly appreciated. Thanks in Advance. Best Regards, Kaushal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 26/41] libxl: introduce virtxend daemon
On 7/23/19 10:03 AM, Daniel P. Berrangé wrote: > The virtxend daemon will be responsible for providing the libxl API Written that way, with the 'xen' and 'd' squashed together, revives nightmares of another thing with similar name :-). However I agree it is the correct name to use here and the 'virt' prefix helps settle my stomach. > driver functionality. The libxl driver is still loaded by the main > libvirtd daemon at this stage, so virtxend must not be running at > the same time. > > This naming is slightly different than other drivers. With the libxl > driver, the user still has a 'xen:///system' URI, and we provide it > in a libvirt-daemon-xen RPM, which pulls in a > libvirt-daemon-driver-libxl RPM. > > Arguably we could rename the libxl driver to "xen" since it is the > only xen driver we have these days, and that matches how we expose it > to users in the URI naming. Nod. And the contents of xenconfig directory could likely be moved under it. Regards, Jim -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v10 19/19] backup: Prevent snapshots and checkpoints at same time
On 7/24/19 10:40 AM, Peter Krempa wrote:
> On Wed, Jul 24, 2019 at 00:56:09 -0500, Eric Blake wrote:
>> Earlier patches mentioned that the initial implementation will prevent
>> snapshots and checkpoints from being used on the same domain at once.
>> However, the actual restriction is done in this separate patch to make
>> it easier to lift that restriction via a revert, when we are finally
>> ready to tackle that integration in the future.
>>
>> Signed-off-by: Eric Blake
>>
>> if (virDomainSnapshotCreateXMLEnsureACL(domain->conn, vm->def, flags) <
>> 0)
>
> Note that also any of the block jobs (block pull, block commit) will
> mess up the bitmaps thus should be forbidden with checkpoints. Also
> block copy has the same problem, but it's questionable whether we'll
> want to copy over the bitmaps (which is way easier than with snapshots,
> which break if you use that API). If the idea is to eventually be able
> to copy bitmaps, then we should also forbid that one until it's
> implemented.
Here's what I'm squashing in to this patch. I've made a few other
adjustments through the series based on review comments and my
last-minute once-over, and am now pushing this series; any further
changes will need to be followup patches.
diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c
index 3e29e54cea..8fa928afc1 100644
--- i/src/qemu/qemu_driver.c
+++ w/src/qemu/qemu_driver.c
@@ -18476,6 +18476,12 @@ qemuDomainBlockRebase(virDomainPtr dom, const
char *path, const char *base,
if (virDomainBlockRebaseEnsureACL(dom->conn, vm->def) < 0)
goto cleanup;
+if (virDomainListCheckpoints(vm->checkpoints, NULL, dom, NULL, 0) >
0) {
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+ _("cannot perform block rebase while checkpoint
exists"));
+goto cleanup;
+}
+
/* For normal rebase (enhanced blockpull), the common code handles
* everything, including vm cleanup. */
if (!(flags & VIR_DOMAIN_BLOCK_REBASE_COPY))
@@ -18560,6 +18566,12 @@ qemuDomainBlockCopy(virDomainPtr dom, const
char *disk, const char *destxml,
if (virDomainBlockCopyEnsureACL(dom->conn, vm->def) < 0)
goto cleanup;
+if (virDomainListCheckpoints(vm->checkpoints, NULL, dom, NULL, 0) >
0) {
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+ _("cannot perform block copy while checkpoint
exists"));
+goto cleanup;
+}
+
for (i = 0; i < nparams; i++) {
virTypedParameterPtr param = [i];
@@ -18622,6 +18634,13 @@ qemuDomainBlockPull(virDomainPtr dom, const
char *path, unsigned long bandwidth,
return -1;
}
+if (virDomainListCheckpoints(vm->checkpoints, NULL, dom, NULL, 0) >
0) {
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+ _("cannot perform block pull while checkpoint
exists"));
+virDomainObjEndAPI();
+return -1;
+}
+
return qemuDomainBlockPullCommon(dom->conn->privateData,
vm, path, NULL, bandwidth, flags);
}
@@ -18668,6 +18687,12 @@ qemuDomainBlockCommit(virDomainPtr dom,
if (virDomainBlockCommitEnsureACL(dom->conn, vm->def) < 0)
goto cleanup;
+if (virDomainListCheckpoints(vm->checkpoints, NULL, dom, NULL, 0) >
0) {
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+ _("cannot perform block commit while checkpoint
exists"));
+goto cleanup;
+}
+
if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
goto cleanup;
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 1/2] tpm: Set transationStarted to false if commit failed
Set the transactionStarted to false if the commit failed. If this is not
done, then the failure path will report 'no transaction is set' and hide
more useful error reports.
Signed-off-by: Stefan Berger
Reviewed-by: Daniel P. Berrangé
---
src/qemu/qemu_security.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c
index 87209d3781..3cd6d9bd3d 100644
--- a/src/qemu/qemu_security.c
+++ b/src/qemu/qemu_security.c
@@ -476,7 +476,7 @@ qemuSecurityStartTPMEmulator(virQEMUDriverPtr driver,
if (virSecurityManagerTransactionCommit(driver->securityManager,
-1, priv->rememberOwner) < 0)
-goto cleanup;
+goto cleanup_abort;
transactionStarted = false;
if (virSecurityManagerSetChildProcessLabel(driver->securityManager,
@@ -512,6 +512,7 @@ qemuSecurityStartTPMEmulator(virQEMUDriverPtr driver,
-1, priv->rememberOwner) < 0)
VIR_WARN("Unable to run security manager transaction");
+ cleanup_abort:
virSecurityManagerTransactionAbort(driver->securityManager);
return ret;
}
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 2/2] tpm: Create empty log file if file was removed
Create an empty log file if the log file was removed, otherwise the
transaction to set the security labels on the file will fail.
Signed-off-by: Stefan Berger
Reviewed-by: Daniel P. Berrangé
---
src/qemu/qemu_tpm.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
index 7efd635831..77ef601f74 100644
--- a/src/qemu/qemu_tpm.c
+++ b/src/qemu/qemu_tpm.c
@@ -340,9 +340,13 @@ qemuTPMEmulatorPrepareHost(virDomainTPMDefPtr tpm,
logDir, vmname) < 0)
goto cleanup;
+if (!virFileExists(tpm->data.emulator.logfile) &&
+virFileTouch(tpm->data.emulator.logfile, 0644) < 0) {
+goto cleanup;
+}
+
/* ... and make sure it can be accessed by swtpm_user */
-if (virFileExists(tpm->data.emulator.logfile) &&
-chown(tpm->data.emulator.logfile, swtpm_user, swtpm_group) < 0) {
+if (chown(tpm->data.emulator.logfile, swtpm_user, swtpm_group) < 0) {
virReportSystemError(errno,
_("Could not chown on swtpm logfile %s"),
tpm->data.emulator.logfile);
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 0/2] tpm2: Properly handle a removed logfile
If the swtpm's logfile was removed by the user, we get an error 'no transaction is set' from the security manager (DAC) since the labeling of the file failed the transaction in the commit() phase. In the failure case we will try to remove the label then in the error path and run into another commit() error and overwrite a more useful error message. So in this case we just call the transaction abort function. We also create an empty log file now since swtpm doesn't seem to be able to create one itself. Stefan v1->v2: - Added R-b's Stefan Berger (2): tpm: Set transationStarted to false if commit failed tpm: Create empty log file if file was removed src/qemu/qemu_security.c | 3 ++- src/qemu/qemu_tpm.c | 8 ++-- 2 files changed, 8 insertions(+), 3 deletions(-) -- 2.21.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 0/4] Cleanups on vTPM encryption patches
This series of patches cleans up the Coverity issues reported by John. Stefan v1->v2: fixed patch 3 Stefan Berger (4): tpm: Fix memory leak and use existing variable instead tests: Call virCommandFree() in cleanup section utils: Remove ATTRIBUTE_NONNULL(1) from virCommandSetSendBuffer conf: Remove ATTRIBUTE_NONNULL(2) from virDomainCheckDeviceChanges() src/conf/domain_conf.h | 3 +-- src/qemu/qemu_tpm.c| 4 ++-- src/util/vircommand.c | 3 ++- src/util/vircommand.h | 2 +- tests/commandtest.c| 3 +-- 5 files changed, 7 insertions(+), 8 deletions(-) -- 2.20.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 4/4] conf: Remove ATTRIBUTE_NONNULL(2) from virDomainCheckDeviceChanges()
Since we are checking the 2nd parameter in the function for NULL, we need to remove ATTRIBUTE_NONNULL(2) from the prototype. Signed-off-by: Stefan Berger Reviewed-by: John Ferlan --- src/conf/domain_conf.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 285fa6c496..dc480bc7c5 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3638,5 +3638,4 @@ bool virDomainGraphicsNeedsAutoRenderNode(const virDomainGraphicsDef *graphics); int -virDomainCheckDeviceChanges(virDomainDefPtr def, virDomainDefPtr newDef) -ATTRIBUTE_NONNULL(2); +virDomainCheckDeviceChanges(virDomainDefPtr def, virDomainDefPtr newDef); -- 2.20.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 2/4] tests: Call virCommandFree() in cleanup section
Fix a potential memory leak by calling virCommandFree() in the cleanup section. Signed-off-by: Stefan Berger Reviewed-by: John Ferlan --- tests/commandtest.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/commandtest.c b/tests/commandtest.c index 991c0572b0..dfd15a2079 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -1219,8 +1219,6 @@ static int test27(const void *unused ATTRIBUTE_UNUSED) goto cleanup; } -virCommandFree(cmd); - if (!outactual || !erractual) goto cleanup; @@ -1236,6 +1234,7 @@ static int test27(const void *unused ATTRIBUTE_UNUSED) ret = 0; cleanup: +virCommandFree(cmd); VIR_FORCE_CLOSE(pipe1[0]); VIR_FORCE_CLOSE(pipe2[0]); VIR_FORCE_CLOSE(pipe1[1]); -- 2.20.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 3/4] utils: Remove ATTRIBUTE_NONNULL(1) from virCommandSetSendBuffer
Remove the ATTRIBUTE_NONNULL(1) from virCommandSetSendBuffer()
prototype since we are checking for '!cmd' and move the initialization
if 'i' after the test for '!cmd'.
Signed-off-by: Stefan Berger
Reviewed-by: John Ferlan
---
src/util/vircommand.c | 3 ++-
src/util/vircommand.h | 2 +-
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/util/vircommand.c b/src/util/vircommand.c
index 774038e536..2df71014f8 100644
--- a/src/util/vircommand.c
+++ b/src/util/vircommand.c
@@ -1775,7 +1775,7 @@ virCommandSetSendBuffer(virCommandPtr cmd,
int fd,
unsigned char *buffer, size_t buflen)
{
-size_t i = virCommandGetNumSendBuffers(cmd);
+size_t i;
if (!cmd || cmd->has_error)
return -1;
@@ -1787,6 +1787,7 @@ virCommandSetSendBuffer(virCommandPtr cmd,
return -1;
}
+i = virCommandGetNumSendBuffers(cmd);
if (VIR_REALLOC_N(cmd->sendBuffers, i + 1) < 0) {
cmd->has_error = ENOMEM;
return -1;
diff --git a/src/util/vircommand.h b/src/util/vircommand.h
index c2abc7b2c3..74574e3fb1 100644
--- a/src/util/vircommand.h
+++ b/src/util/vircommand.h
@@ -149,7 +149,7 @@ void virCommandSetWorkingDirectory(virCommandPtr cmd,
int virCommandSetSendBuffer(virCommandPtr cmd,
int fd,
unsigned char *buffer, size_t buflen)
-ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3);
+ATTRIBUTE_NONNULL(3);
void virCommandSetInputBuffer(virCommandPtr cmd,
const char *inbuf) ATTRIBUTE_NONNULL(2);
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 1/4] tpm: Fix memory leak and use existing variable instead
Use the existing variables rather then calling virTPMSwtpmXYZ().
Signed-off-by: Stefan Berger
Reviewed-by: John Ferlan
---
src/qemu/qemu_tpm.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
index 7efd635831..a8a4d734c0 100644
--- a/src/qemu/qemu_tpm.c
+++ b/src/qemu/qemu_tpm.c
@@ -508,7 +508,7 @@ qemuTPMEmulatorRunSetup(const char *storagepath,
VIR_TPM_SWTPM_SETUP_FEATURE_CMDARG_PWDFILE_FD)) {
virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED,
_("%s does not support passing a passphrase using a file "
- "descriptor"), virTPMGetSwtpmSetup());
+ "descriptor"), swtpm_setup);
goto cleanup;
}
if ((pwdfile_fd = qemuTPMSetupEncryption(secretuuid, cmd)) < 0)
@@ -648,7 +648,7 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDefPtr tpm,
if (!virTPMSwtpmCapsGet(VIR_TPM_SWTPM_FEATURE_CMDARG_PWD_FD)) {
virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED,
_("%s does not support passing passphrase via file
descriptor"),
- virTPMGetSwtpm());
+ swtpm);
goto error;
}
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] tpm: Run swtpm_setup with less parameters on incoming migration
In case of an incoming migration we do not need to run swtpm_setup
with all the parameters but only want to get the benefit of it
creating a TPM state file for us that we can then label with an
SELinux label. The actual state will be overwritten by the in-
coming state. So we have to pass an indicator for incomingMigration
all the way to the command line parameter generation for swtpm_setup.
Signed-off-by: Stefan Berger
---
src/qemu/qemu_extdevice.c | 5 ++--
src/qemu/qemu_extdevice.h | 3 ++-
src/qemu/qemu_process.c | 2 +-
src/qemu/qemu_tpm.c | 49 +--
src/qemu/qemu_tpm.h | 3 ++-
5 files changed, 40 insertions(+), 22 deletions(-)
diff --git a/src/qemu/qemu_extdevice.c b/src/qemu/qemu_extdevice.c
index e576bca165..af52466421 100644
--- a/src/qemu/qemu_extdevice.c
+++ b/src/qemu/qemu_extdevice.c
@@ -128,7 +128,8 @@ qemuExtDevicesCleanupHost(virQEMUDriverPtr driver,
int
qemuExtDevicesStart(virQEMUDriverPtr driver,
virDomainObjPtr vm,
-qemuDomainLogContextPtr logCtxt)
+qemuDomainLogContextPtr logCtxt,
+bool incomingMigration)
{
int ret = 0;
@@ -136,7 +137,7 @@ qemuExtDevicesStart(virQEMUDriverPtr driver,
return -1;
if (vm->def->tpm)
-ret = qemuExtTPMStart(driver, vm, logCtxt);
+ret = qemuExtTPMStart(driver, vm, logCtxt, incomingMigration);
return ret;
}
diff --git a/src/qemu/qemu_extdevice.h b/src/qemu/qemu_extdevice.h
index bbdb9a1cc2..5a53c79f38 100644
--- a/src/qemu/qemu_extdevice.h
+++ b/src/qemu/qemu_extdevice.h
@@ -40,7 +40,8 @@ void qemuExtDevicesCleanupHost(virQEMUDriverPtr driver,
int qemuExtDevicesStart(virQEMUDriverPtr driver,
virDomainObjPtr vm,
-qemuDomainLogContextPtr logCtxt)
+qemuDomainLogContextPtr logCtxt,
+bool incomingMigration)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
ATTRIBUTE_RETURN_CHECK;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 75205bc121..fae18824ba 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -6774,7 +6774,7 @@ qemuProcessLaunch(virConnectPtr conn,
if (qemuProcessGenID(vm, flags) < 0)
goto cleanup;
-if (qemuExtDevicesStart(driver, vm, logCtxt) < 0)
+if (qemuExtDevicesStart(driver, vm, logCtxt, incoming != NULL) < 0)
goto cleanup;
VIR_DEBUG("Building emulator command line");
diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
index 77ef601f74..4174aa4c62 100644
--- a/src/qemu/qemu_tpm.c
+++ b/src/qemu/qemu_tpm.c
@@ -453,6 +453,7 @@ qemuTPMSetupEncryption(const unsigned char *secretuuid,
* for the user given by userid or 'tss'
* @tpmversion: The version of the TPM, either a TPM 1.2 or TPM 2
* @encryption: pointer to virStorageEncryption holding secret
+ * @incomingMigration: whether we have an incoming migration
*
* Setup the external swtpm by creating endorsement key and
* certificates for it.
@@ -466,7 +467,8 @@ qemuTPMEmulatorRunSetup(const char *storagepath,
gid_t swtpm_group,
const char *logfile,
const virDomainTPMVersion tpmversion,
-const unsigned char *secretuuid)
+const unsigned char *secretuuid,
+bool incomingMigration)
{
virCommandPtr cmd = NULL;
int exitstatus;
@@ -525,16 +527,23 @@ qemuTPMEmulatorRunSetup(const char *storagepath,
pwdfile_fd = -1;
}
-virCommandAddArgList(cmd,
- "--tpm-state", storagepath,
- "--vmid", vmid,
- "--logfile", logfile,
- "--createek",
- "--create-ek-cert",
- "--create-platform-cert",
- "--lock-nvram",
- "--not-overwrite",
- NULL);
+if (!incomingMigration) {
+virCommandAddArgList(cmd,
+ "--tpm-state", storagepath,
+ "--vmid", vmid,
+ "--logfile", logfile,
+ "--createek",
+ "--create-ek-cert",
+ "--create-platform-cert",
+ "--lock-nvram",
+ "--not-overwrite",
+ NULL);
+} else {
+virCommandAddArgList(cmd,
+ "--tpm-state", storagepath,
+ "--overwrite",
+ NULL);
+}
virCommandClearCaps(cmd);
@@ -568,6 +577,7 @@ qemuTPMEmulatorRunSetup(const char *storagepath,
* @swtpmStateDir: the directory where swtpm writes the pid file and creates
the
*
[libvirt] [PATCH] backup: Fix tests/virsh-snapshot
Creating an 'exp' output file, but never comparing it against the actual output, does no one any good. :) Fixes: 280a2b41e Signed-off-by: Eric Blake --- Pushing under the trivial rule. tests/virsh-snapshot | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/virsh-snapshot b/tests/virsh-snapshot index b23d949f62..1034140cde 100755 --- a/tests/virsh-snapshot +++ b/tests/virsh-snapshot @@ -207,6 +207,8 @@ Descendants:0 Metadata: yes EOF +sed '1,/^virsh #/d; /virsh #/d' < out > out.cooked || fail=1 +compare exp out.cooked || fail=1 cat < exp || fail=1 error: invalid argument: parent s3 for moment s2 not found -- 2.20.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] RFC: security: Make sure to decrease ref count label value
I noticed that if a domain fails to restore, the ref count in the xattr
'trusted.libvirt.security.ref_selinux' keeps on increasing indefinitely
and the VM will never restore even if the root cause for the restore
failure has been removed. The reason seems to be that the code to decrease
the ref count never gets called because the block above it fails due
to virSecuritySELinuxTransactionAppend() failing. The simple solution
seems to be to revert the order in which things are done.
Signed-off-by: Stefan Berger
---
src/security/security_selinux.c | 19 +++
1 file changed, 11 insertions(+), 8 deletions(-)
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index ea20373a90..9fd29e9bca 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -1499,14 +1499,9 @@ virSecuritySELinuxRestoreFileLabel(virSecurityManagerPtr
mgr,
goto cleanup;
}
-if ((rc = virSecuritySELinuxTransactionAppend(path, NULL,
- false, recall, true)) < 0) {
-goto cleanup;
-} else if (rc > 0) {
-ret = 0;
-goto cleanup;
-}
-
+/* Recall the label so the ref count label decreases its counter
+ * even if transaction append below fails.
+ */
if (recall) {
rc = virSecuritySELinuxRecallLabel(newpath, );
if (rc == -2) {
@@ -1519,6 +1514,14 @@ virSecuritySELinuxRestoreFileLabel(virSecurityManagerPtr
mgr,
}
}
+if ((rc = virSecuritySELinuxTransactionAppend(path, NULL,
+ false, recall, true)) < 0) {
+goto cleanup;
+} else if (rc > 0) {
+ret = 0;
+goto cleanup;
+}
+
if (!recall || rc == -2) {
if (stat(newpath, ) != 0) {
VIR_WARN("cannot stat %s: %s", newpath,
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 18/41] remote: in per-driver daemons ensure that state initialize succeeds
On Tue, 2019-07-23 at 17:02 +0100, Daniel P. Berrangé wrote:
[...]
> @@ -648,15 +650,23 @@ virStateInitialize(bool privileged,
[...]
> +if (ret == VIR_DRV_STATE_INIT_ERROR) {
> VIR_ERROR(_("Initialization of %s state driver failed: %s"),
>virStateDriverTab[i]->name,
>virGetLastErrorMessage());
> return -1;
> +} else if (ret == VIR_DRV_STATE_INIT_SKIPPED &&
> + mandatory) {
You can fit this entire condition on a single line.
[...]
> +++ b/src/remote/remote_daemon.c
> @@ -794,6 +794,11 @@ static void daemonRunStateInit(void *opaque)
> * we're ready, since it can take a long time and this will
> * seriously delay OS bootup process */
> if (virStateInitialize(virNetDaemonIsPrivileged(dmn),
> +#ifdef MODULE_NAME
> + true,
> +#else /* ! MODULE_NAME */
> + false,
> +#endif /* ! MODULE_NAME */
> daemonInhibitCallback,
> dmn) < 0) {
Just like in patch 10, this is really ugly... Please change it to
something like
#ifdef MODULE_NAME
bool mandatory = true;
#else /* ! MODULE_NAME */
bool mandatory = false;
#endif /* ! MODULE_NAME */
virStateInitialize(virNetDaemonIsPrivileged(dmn),
mandatory,
daemonInhibitCallback,
dmn);
[...]
> +++ b/src/vz/vz_driver.c
> @@ -4118,36 +4118,36 @@ vzStateInitialize(bool privileged,
[...]
> /* Failing to create driver here is not fatal and only means
> * that next driver client will try once more when connecting */
> vz_driver = vzDriverObjNew();
> -return 0;
> +return VIR_DRV_STATE_INIT_COMPLETE;
Given the comment, are you sure we shouldn't do something like
if (!(vz_driver = vzDriverObjNew()))
return VIR_DRV_STATE_INIT_SKIPPED;
return VIR_DRV_STATE_INIT_COMPLETE;
here instead?
With the nits above addressed, and assuming the logic in the vz
driver either is confirmed to be fine as or is changed appropriately,
Reviewed-by: Andrea Bolognani
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 17/41] remote: refactor how list of systemd unit files is built
On Tue, 2019-07-23 at 17:02 +0100, Daniel P. Berrangé wrote: > The make logic assumes that the SYSTEMD_UNIT_FILES var can be built from > SYSTEMD_UNIT_FILES_IN by simply dropping the directory prefix and the > .in suffix. > > This won't work in future when a single .in unit file can be used to > generate multiple different units. IIUC this is mostly for sockets, correct? As in, with the entire series applied we'll have eg. SYSTEMD_UNIT_FILES += \ virtqemud.service \ virtqemud.socket \ virtqemud-ro.socket \ virtqemud-admin.socket \ $(NULL) SYSTEMD_UNIT_FILES_IN += \ qemu/virtqemud.service.in \ $(NULL) where virtqemud*.socket are not generated, as the current code would expect, from virtqemud*.socket.in, but rather from libvirtd*.socket.in - hence the need for this patch. Again IIUC there's nothing really stopping us from generating virtqemud*.service from libvirtd*.service.in, or at least from a common virtd*.service.in, since eg. virtqemud.service.in and virtlxcd.service.in are basically identical - it's just that you haven't unified the generation rules yet. Assuming I've understood the intent correctly, then the changes themselves look good, so Reviewed-by: Andrea Bolognani -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v10 08/19] backup: Parse and output checkpoint XML
On 7/24/19 5:42 PM, Eric Blake wrote: > the amount of simplifications when you can rely on the data already > being validated was rather substantial. > >> >> If there is no downside to validation though, why only do it for one >> schema. We should do it for all of the XML parsers we have. We didn't >> do this originally because we didn't have confidence in accuracy of >> the schemas, which was shown a sensible decision many times over as >> we found a great many schema bugs. > > Yes, I'd love to add more validation to more of our existing legacy > APIs, but one project at a time. Maybe a libvirt.conf or qemu.conf > switch to opt in to validation (and ignore the flags, where flags exist) > is worthwhile (then, if you do run into a schema bug, you can > temporarily weaken qemu.conf and rely on the flags on a per-API basis). > >> >> None the less, it would still be possible to turn on validation for >> all our schemas, as simply declare the current _VALIDATE flags are >> now a no-op. > > It would also be possible to add a _VALIDATE flag for checkpoints that > is initially treated as a no-op (ie. I perform the validation using the > RNG schema no matter what, regardless of the flag's presence, because it > makes the rest of my code shorter), but if we run into an RNG schema bug > down the road, we could then still have the position of omitting the > flag to bypass validation at that time. Then again, if we run into an > RNG bug where we reject something that looks like it should be valid, an > older libvirt will fail to parse the XML no matter what, then by the > time you upgrade to a version of libvirt that honors the flag to allow a > bypass, you've also upgraded to a version of libvirt that fixes the bug, > so why do you need the bypass? Thinking just a bit more: for the case of defining a new checkpoint, mandatory schema validation is still appealing (the new RNG file is pretty small, and easy enough to compare to the code in checkpoint_conf.c). But for the case of the VIR_DOMAIN_CHECKPOINT_CREATE_REDEFINE flag, which includes a full sub-element, there _is_ a risk that we still have inconsistencies between the domain XML we produce and what the schema parser will require during the REDEFINE (because we don't have schema validation always enabled for domains in general, and because is so much of a bigger beast to begin with). The last thing we need is for restarting libvirtd to fail to reload all checkpoints merely because of a schema failure on the subelement. I may still end up implementing a VALIDATE flag (maybe just at the checkpoint_conf.c level for internal use, rather than as a public API flag) that is a no-op for checkpoint creation but allows bypass of the validation of a sub-element during REDEFINE (and I would actually code it up that way too: try to validate the XML as-is; if it fails, then edit a copy of the XML to remove the element and try to revalidate the rest, rather than skipping validation altogether - so that the rest of the checkpoint code can still rely on validation rather than reinstating redundant checking code). But it is definitely material for a separate patch: > There's still time to add it in prior to the 5.6 release if we decide we > need the opt-out capability. I'm probably going to commit this patch > as-is while we continue the discussion, where we can decide if a > follow-up patch needs to make it opt-in rather than mandatory. > -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 16/41] remote: conditionalize systemd socket unit files
On Tue, 2019-07-23 at 17:02 +0100, Daniel P. Berrangé wrote: [...] > +++ b/src/remote/Makefile.inc.am > @@ -292,6 +292,9 @@ endif WITH_SASL > > LIBVIRTD_UNIT_VARS = \ > $(COMMON_UNIT_VARS) \ > + -e 's|[@]name[@]|Libvirt|g' \ > + -e 's|[@]service[@]|libvirtd|g' \ > + -e 's|[@]sockprefix[@]|libvirt|g' \ > $(NULL) Patch 19 contains this hunk: @@ -298,13 +369,34 @@ LIBVIRTD_UNIT_VARS = \ -e 's|[@]name[@]|Libvirt|g' \ -e 's|[@]service[@]|libvirtd|g' \ -e 's|[@]sockprefix[@]|libvirt|g' \ + -e 's|[@]deps[@]||g' \ + $(NULL) but it should be in this commit instead, since... [...] > +++ b/src/remote/libvirtd-admin.socket.in > @@ -1,14 +1,15 @@ > [Unit] > -Description=Libvirt admin socket > -Before=libvirtd.service > -BindsTo=libvirtd.socket > -After=libvirtd.socket > +Description=@name@ admin socket > +Before=@service@.service > +BindsTo=@service@.socket > +After=@service@.socket > +@deps@ ... you already have @deps@ here, and without the above the placeholder will show up in the output file. With that hunk squashed in, Reviewed-by: Andrea Bolognani -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] tpm: Create empty log file if file was removed
On Fri, Jul 26, 2019 at 11:51:47AM -0400, Stefan Berger wrote:
> Create an empty log file if the log file was removed, otherwise the
> transaction to set the security labels on the file will fail.
>
> Signed-off-by: Stefan Berger
> ---
> src/qemu/qemu_tpm.c | 8 ++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
> index 7efd635831..77ef601f74 100644
> --- a/src/qemu/qemu_tpm.c
> +++ b/src/qemu/qemu_tpm.c
> @@ -340,9 +340,13 @@ qemuTPMEmulatorPrepareHost(virDomainTPMDefPtr tpm,
> logDir, vmname) < 0)
> goto cleanup;
>
> +if (!virFileExists(tpm->data.emulator.logfile) &&
> +virFileTouch(tpm->data.emulator.logfile, 0644) < 0) {
> +goto cleanup;
> +}
> +
> /* ... and make sure it can be accessed by swtpm_user */
> -if (virFileExists(tpm->data.emulator.logfile) &&
> -chown(tpm->data.emulator.logfile, swtpm_user, swtpm_group) < 0) {
> +if (chown(tpm->data.emulator.logfile, swtpm_user, swtpm_group) < 0) {
> virReportSystemError(errno,
> _("Could not chown on swtpm logfile %s"),
> tpm->data.emulator.logfile);
Reviewed-by: Daniel P. Berrangé
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] tpm: Set transationStarted to false if commit failed
On Fri, Jul 26, 2019 at 11:51:46AM -0400, Stefan Berger wrote:
> Set the transactionStarted to false if the commit failed. If this is not
> done, then the failure path will report 'no transaction is set' and hide
> more useful error reports.
>
> Signed-off-by: Stefan Berger
> ---
> src/qemu/qemu_security.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c
> index 87209d3781..3f0d19eba8 100644
> --- a/src/qemu/qemu_security.c
> +++ b/src/qemu/qemu_security.c
> @@ -475,8 +475,9 @@ qemuSecurityStartTPMEmulator(virQEMUDriverPtr driver,
> }
>
> if (virSecurityManagerTransactionCommit(driver->securityManager,
> --1, priv->rememberOwner) < 0)
> -goto cleanup;
> +-1, priv->rememberOwner) < 0) {
> +goto cleanup_abort;
> +}
> transactionStarted = false;
>
> if (virSecurityManagerSetChildProcessLabel(driver->securityManager,
> @@ -512,6 +513,7 @@ qemuSecurityStartTPMEmulator(virQEMUDriverPtr driver,
> -1, priv->rememberOwner) < 0)
> VIR_WARN("Unable to run security manager transaction");
>
> + cleanup_abort:
> virSecurityManagerTransactionAbort(driver->securityManager);
> return ret;
> }
Reviewed-by: Daniel P. Berrangé
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 15/41] remote: reduce duplication in systemd unit file make rules into one
On Tue, 2019-07-23 at 17:02 +0100, Daniel P. Berrangé wrote: [...] > +COMMON_UNIT_VARS = \ > + -e 's|[@]localstatedir[@]|$(localstatedir)|g' \ > + -e 's|[@]sbindir[@]|$(sbindir)|g' \ > + -e 's|[@]sysconfdir[@]|$(sysconfdir)|g' \ > + $(NULL) > > +LIBVIRTD_UNIT_VARS = \ > + $(COMMON_UNIT_VARS) \ > + $(NULL) > > +libvirtd.service: remote/libvirtd.service.in $(top_builddir)/config.status > + $(AM_V_GEN)sed $(LIBVIRTD_UNIT_VARS) < $< > $@-t && mv $@-t $@ > > +libvirt%.socket: remote/libvirt%.socket.in $(top_builddir)/config.status > + $(AM_V_GEN)sed $(LIBVIRTD_UNIT_VARS) < $< > $@-t && mv $@-t $@ This is a fantastic refactoring that I've been thinking about spending time on for a long time, so thanks a lot for doing it! It's a bummer that we don't go further in our DRY crusade and extend it to .service files (even after the entire series has been applied), but we can always do that as a follow-up series :) Reviewed-by: Andrea Bolognani -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/2] tpm: Set transationStarted to false if commit failed
Set the transactionStarted to false if the commit failed. If this is not
done, then the failure path will report 'no transaction is set' and hide
more useful error reports.
Signed-off-by: Stefan Berger
---
src/qemu/qemu_security.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c
index 87209d3781..3f0d19eba8 100644
--- a/src/qemu/qemu_security.c
+++ b/src/qemu/qemu_security.c
@@ -475,8 +475,9 @@ qemuSecurityStartTPMEmulator(virQEMUDriverPtr driver,
}
if (virSecurityManagerTransactionCommit(driver->securityManager,
--1, priv->rememberOwner) < 0)
-goto cleanup;
+-1, priv->rememberOwner) < 0) {
+goto cleanup_abort;
+}
transactionStarted = false;
if (virSecurityManagerSetChildProcessLabel(driver->securityManager,
@@ -512,6 +513,7 @@ qemuSecurityStartTPMEmulator(virQEMUDriverPtr driver,
-1, priv->rememberOwner) < 0)
VIR_WARN("Unable to run security manager transaction");
+ cleanup_abort:
virSecurityManagerTransactionAbort(driver->securityManager);
return ret;
}
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/2] tpm: Create empty log file if file was removed
Create an empty log file if the log file was removed, otherwise the
transaction to set the security labels on the file will fail.
Signed-off-by: Stefan Berger
---
src/qemu/qemu_tpm.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
index 7efd635831..77ef601f74 100644
--- a/src/qemu/qemu_tpm.c
+++ b/src/qemu/qemu_tpm.c
@@ -340,9 +340,13 @@ qemuTPMEmulatorPrepareHost(virDomainTPMDefPtr tpm,
logDir, vmname) < 0)
goto cleanup;
+if (!virFileExists(tpm->data.emulator.logfile) &&
+virFileTouch(tpm->data.emulator.logfile, 0644) < 0) {
+goto cleanup;
+}
+
/* ... and make sure it can be accessed by swtpm_user */
-if (virFileExists(tpm->data.emulator.logfile) &&
-chown(tpm->data.emulator.logfile, swtpm_user, swtpm_group) < 0) {
+if (chown(tpm->data.emulator.logfile, swtpm_user, swtpm_group) < 0) {
virReportSystemError(errno,
_("Could not chown on swtpm logfile %s"),
tpm->data.emulator.logfile);
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/2] tpm2: Properly handle a removed logfile
If the swtpm's logfile was removed by the user, we get an error 'no transaction is set' from the security manager (DAC) since the labeling of the file failed the transaction in the commit() phase. In the failure case we will try to remove the label then in the error path and run into another commit() error and overwrite a more useful error message. So in this case we just call the transaction abort function. We also create an empty log file now since swtpm doesn't seem to be able to create one itself. Stefan Stefan Berger (2): tpm: Set transationStarted to false if commit failed tpm: Create empty log file if file was removed src/qemu/qemu_security.c | 6 -- src/qemu/qemu_tpm.c | 8 ++-- 2 files changed, 10 insertions(+), 4 deletions(-) -- 2.21.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 11/41] remote: conditionalize IP socket config in libvirtd.conf
On Fri, 2019-07-26 at 16:22 +0100, Daniel P. Berrangé wrote: > On Fri, Jul 26, 2019 at 03:59:23PM +0200, Andrea Bolognani wrote: > > On Tue, 2019-07-23 at 17:02 +0100, Daniel P. Berrangé wrote: > > > +remote/libvirtd.conf: remote/libvirtd.conf.in > > > + $(AM_V_GEN)sed \ > > > + -e '/:: CUT ENABLE_IP ::/d' \ > > > + -e '/:: END ::/d' \ > > > + -e 's/:: DAEMON_NAME ::/libvirtd/' \ > > > + < $^ > $@ > > > > Using $^ seems a bit weird considering that you have a single > > input... Any reason not to use $< here? > > it is an accident. > > > I would also use @DAEMON_NAME@ instead of ":: DAEMON_NAME ::" since > > the former style is already used everywhere for simple value > > replacement in .in files. > > The augeas files already use "::" which is why I picked that. So we'll > need another patch first to remove existing usage. Yeah, it'd make sense to use @CONFIG@ as well. With $^ replaced with $< and ":: DAEMON_NAME ::" replaced with @DAEMON_NAME@, Reviewed-by: Andrea Bolognani -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 13/41] remote: refactor & rename variables for building libvirtd
On Fri, 2019-07-26 at 16:21 +0100, Daniel P. Berrangé wrote: > On Fri, Jul 26, 2019 at 05:19:56PM +0200, Andrea Bolognani wrote: > > [...] > > > +REMOTE_DAEMON_LD_ADD += \ > > > + ../gnulib/lib/libgnu.la \ > > > + $(LIBSOCKET) \ > > > + $(NULL) > > > > As an aside, it looks like $(LIBSOCKET) is a leftover of days long > > gone and no longer used for anything. > > It is something that gnulib defines. Whether it expands to a non-empty > string on any of our supported build platforms though, I don't know. I tried removing it and ran it through the full gauntlet without getting any failures, so I'm pretty confident we don't need it. It'd be pretty weird if we did, since we have at least two other daemons already and neither of those is using it... -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 08/41] remote: conditionalize daemon name in libvirtd daemon
On Fri, 2019-07-26 at 16:24 +0100, Daniel P. Berrangé wrote: > On Fri, Jul 26, 2019 at 02:39:51PM +0200, Andrea Bolognani wrote: > > On Tue, 2019-07-23 at 17:02 +0100, Daniel P. Berrangé wrote: > > [...] > > > +++ b/src/remote/remote_driver.h > > > @@ -34,7 +34,6 @@ unsigned long remoteVersion(void); > > > #define LIBVIRTD_PRIV_UNIX_SOCKET LOCALSTATEDIR > > > "/run/libvirt/libvirt-sock" > > > #define LIBVIRTD_PRIV_UNIX_SOCKET_RO LOCALSTATEDIR > > > "/run/libvirt/libvirt-sock-ro" > > > #define LIBVIRTD_USER_UNIX_SOCKET "libvirt-sock" > > > -#define LIBVIRTD_CONFIGURATION_FILE SYSCONFDIR "/libvirt/libvirtd.conf" > > > > Oh, this was unused even before your changes, wasn't it? You should > > drop it in a separate, trivial patch. > > No it was used, but this should have been done in patch 6 instead. Right you are! I just checked @^ instead of master... The hunk should be squashed into patch 6 then. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 14/41] remote: don't hardcode /etc in the systemd units
On Tue, 2019-07-23 at 17:02 +0100, Daniel P. Berrangé wrote: [...] > +++ b/src/remote/libvirtd.service.in > @@ -20,7 +20,7 @@ Documentation=https://libvirt.org > > [Service] > Type=notify > -EnvironmentFile=-/etc/sysconfig/libvirtd > +EnvironmentFile=-@sysconfdir@/sysconfig/libvirtd At least src/locking/virtlockd.service.in src/logging/virtlogd.service.in tools/libvirt-guests.service.in need the same change; additionally, the various .aug files contain references to /etc/libvirt and I think they should be tweaked too. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 4/5] cpu: Drop unused KVM features
Most of the internally defined KVM CPUID features are not actually used
by libvirt. The QEMU driver may enable or disable them on the command
line, but we don't check for the associated CPU properties or CPUID
bits. They would be useless with QEMU 4.1 anyway since their names were
only remotely similar to the actual feature names.
Signed-off-by: Jiri Denemark
---
src/cpu/cpu_x86.c | 24
src/cpu/cpu_x86_data.h | 8
2 files changed, 32 deletions(-)
diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c
index f8a51dedf6..387c365512 100644
--- a/src/cpu/cpu_x86.c
+++ b/src/cpu/cpu_x86.c
@@ -73,24 +73,8 @@ struct _virCPUx86Feature {
} \
}
-KVM_FEATURE_DEF(VIR_CPU_x86_KVM_CLOCKSOURCE,
-0x4001, 0x0001);
-KVM_FEATURE_DEF(VIR_CPU_x86_KVM_NOP_IO_DELAY,
-0x4001, 0x0002);
-KVM_FEATURE_DEF(VIR_CPU_x86_KVM_MMU_OP,
-0x4001, 0x0004);
-KVM_FEATURE_DEF(VIR_CPU_x86_KVM_CLOCKSOURCE2,
-0x4001, 0x0008);
-KVM_FEATURE_DEF(VIR_CPU_x86_KVM_ASYNC_PF,
-0x4001, 0x0010);
-KVM_FEATURE_DEF(VIR_CPU_x86_KVM_STEAL_TIME,
-0x4001, 0x0020);
-KVM_FEATURE_DEF(VIR_CPU_x86_KVM_PV_EOI,
-0x4001, 0x0040);
KVM_FEATURE_DEF(VIR_CPU_x86_KVM_PV_UNHALT,
0x4001, 0x0080);
-KVM_FEATURE_DEF(VIR_CPU_x86_KVM_CLOCKSOURCE_STABLE_BIT,
-0x4001, 0x0100);
KVM_FEATURE_DEF(VIR_CPU_x86_HV_RUNTIME,
0x4003, 0x0001);
@@ -121,15 +105,7 @@ KVM_FEATURE_DEF(VIR_CPU_x86_HV_EVMCS,
static virCPUx86Feature x86_kvm_features[] =
{
-KVM_FEATURE(VIR_CPU_x86_KVM_CLOCKSOURCE),
-KVM_FEATURE(VIR_CPU_x86_KVM_NOP_IO_DELAY),
-KVM_FEATURE(VIR_CPU_x86_KVM_MMU_OP),
-KVM_FEATURE(VIR_CPU_x86_KVM_CLOCKSOURCE2),
-KVM_FEATURE(VIR_CPU_x86_KVM_ASYNC_PF),
-KVM_FEATURE(VIR_CPU_x86_KVM_STEAL_TIME),
-KVM_FEATURE(VIR_CPU_x86_KVM_PV_EOI),
KVM_FEATURE(VIR_CPU_x86_KVM_PV_UNHALT),
-KVM_FEATURE(VIR_CPU_x86_KVM_CLOCKSOURCE_STABLE_BIT),
KVM_FEATURE(VIR_CPU_x86_HV_RUNTIME),
KVM_FEATURE(VIR_CPU_x86_HV_SYNIC),
KVM_FEATURE(VIR_CPU_x86_HV_STIMER),
diff --git a/src/cpu/cpu_x86_data.h b/src/cpu/cpu_x86_data.h
index 85aaab709c..2607dd96b1 100644
--- a/src/cpu/cpu_x86_data.h
+++ b/src/cpu/cpu_x86_data.h
@@ -43,15 +43,7 @@ struct _virCPUx86MSR {
#define CPUX86_KVM 0x4000
#define CPUX86_EXTENDED 0x8000
-#define VIR_CPU_x86_KVM_CLOCKSOURCE "__kvm_clocksource"
-#define VIR_CPU_x86_KVM_NOP_IO_DELAY "__kvm_no_io_delay"
-#define VIR_CPU_x86_KVM_MMU_OP "__kvm_mmu_op"
-#define VIR_CPU_x86_KVM_CLOCKSOURCE2 "__kvm_clocksource2"
-#define VIR_CPU_x86_KVM_ASYNC_PF "__kvm_async_pf"
-#define VIR_CPU_x86_KVM_STEAL_TIME "__kvm_steal_time"
-#define VIR_CPU_x86_KVM_PV_EOI "__kvm_pv_eoi"
#define VIR_CPU_x86_KVM_PV_UNHALT"__kvm_pv_unhalt"
-#define VIR_CPU_x86_KVM_CLOCKSOURCE_STABLE_BIT "__kvm_clocksource_stable"
/*
* The following HyperV feature names suffixes must exactly match corresponding
--
2.22.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 5/5] qemu: Fix KVM features with QEMU 4.1
Originally the names of the KVM CPU features were only used internally
for looking up their CPUID bits. So we used "__kvm_" prefix for them to
make sure the names do not collide with normal CPU features stored in
our CPU map.
But with QEMU 4.1 we check which features were enabled or disabled by a
freshly started QEMU process using their names rather than their CPUID
bits (mostly because of MSR features). Thus we need to change our made
up internal names into the actual names used by QEMU.
Signed-off-by: Jiri Denemark
---
src/cpu/cpu_x86_data.h | 2 +-
src/qemu/qemu_command.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/cpu/cpu_x86_data.h b/src/cpu/cpu_x86_data.h
index 2607dd96b1..cc0c93dff0 100644
--- a/src/cpu/cpu_x86_data.h
+++ b/src/cpu/cpu_x86_data.h
@@ -43,7 +43,7 @@ struct _virCPUx86MSR {
#define CPUX86_KVM 0x4000
#define CPUX86_EXTENDED 0x8000
-#define VIR_CPU_x86_KVM_PV_UNHALT"__kvm_pv_unhalt"
+#define VIR_CPU_x86_KVM_PV_UNHALT "kvm_pv_unhalt"
/*
* The following HyperV feature names suffixes must exactly match corresponding
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 7b2cfa0683..fee51158a9 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7146,7 +7146,7 @@ qemuBuildCpuCommandLine(virCommandPtr cmd,
}
if (def->features[VIR_DOMAIN_FEATURE_PVSPINLOCK]) {
-qemuBuildCpuFeature(qemuCaps, , "kvm_pv_unhalt",
+qemuBuildCpuFeature(qemuCaps, , VIR_CPU_x86_KVM_PV_UNHALT,
def->features[VIR_DOMAIN_FEATURE_PVSPINLOCK] ==
VIR_TRISTATE_SWITCH_ON);
}
--
2.22.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/5] qemu: Fix hyperv features with QEMU 4.1
Originally the names of the hyperv CPU features were only used
internally for looking up their CPUID bits. So we used "__kvm_hv_"
prefix for them to make sure the names do not collide with normal CPU
features stored in our CPU map.
But with QEMU 4.1 we check which features were enabled or disabled by a
freshly started QEMU process using their names rather than their CPUID
bits (mostly because of MSR features). Thus we need to change our made
up internal names into the actual names used by QEMU. Most of the names
are only used with QEMU 4.1 and newer and the reset was introduced with
QEMU recently enough to already support spelling with "-". Thus we don't
need to define them as "hv_*" with a translation to "hv-*" for new QEMU.
Without this patch libvirt would mistakenly report all hyperv features
as unavailable and refuse to start any domain using them with QEMU 4.1.
Reported-by: Vitaly Kuznetsov
Signed-off-by: Jiri Denemark
---
src/cpu/cpu_x86_data.h | 28 ++--
src/qemu/qemu_process.c | 2 +-
2 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/src/cpu/cpu_x86_data.h b/src/cpu/cpu_x86_data.h
index f3f4d7ab9c..bb781707aa 100644
--- a/src/cpu/cpu_x86_data.h
+++ b/src/cpu/cpu_x86_data.h
@@ -56,21 +56,21 @@ struct _virCPUx86MSR {
/*
* The following HyperV feature names suffixes must exactly match corresponding
* ones defined for virDomainHyperv in domain_conf.c.
- * E.g "__kvm_runtime" -> "runtime", "__kvm_hv_spinlocks" -> "spinlocks" etc.
+ * E.g "hv-runtime" -> "runtime", "hv-spinlocks" -> "spinlocks" etc.
*/
-#define VIR_CPU_x86_KVM_HV_RUNTIME "__kvm_hv_runtime"
-#define VIR_CPU_x86_KVM_HV_SYNIC "__kvm_hv_synic"
-#define VIR_CPU_x86_KVM_HV_STIMER"__kvm_hv_stimer"
-#define VIR_CPU_x86_KVM_HV_RELAXED "__kvm_hv_relaxed"
-#define VIR_CPU_x86_KVM_HV_SPINLOCKS "__kvm_hv_spinlocks"
-#define VIR_CPU_x86_KVM_HV_VAPIC "__kvm_hv_vapic"
-#define VIR_CPU_x86_KVM_HV_VPINDEX "__kvm_hv_vpindex"
-#define VIR_CPU_x86_KVM_HV_RESET "__kvm_hv_reset"
-#define VIR_CPU_x86_KVM_HV_FREQUENCIES "__kvm_hv_frequencies"
-#define VIR_CPU_x86_KVM_HV_REENLIGHTENMENT "__kvm_hv_reenlightenment"
-#define VIR_CPU_x86_KVM_HV_TLBFLUSH "__kvm_hv_tlbflush"
-#define VIR_CPU_x86_KVM_HV_IPI "__kvm_hv_ipi"
-#define VIR_CPU_x86_KVM_HV_EVMCS "__kvm_hv_evmcs"
+#define VIR_CPU_x86_KVM_HV_RUNTIME "hv-runtime"
+#define VIR_CPU_x86_KVM_HV_SYNIC "hv-synic"
+#define VIR_CPU_x86_KVM_HV_STIMER"hv-stimer"
+#define VIR_CPU_x86_KVM_HV_RELAXED "hv-relaxed"
+#define VIR_CPU_x86_KVM_HV_SPINLOCKS "hv-spinlocks"
+#define VIR_CPU_x86_KVM_HV_VAPIC "hv-vapic"
+#define VIR_CPU_x86_KVM_HV_VPINDEX "hv-vpindex"
+#define VIR_CPU_x86_KVM_HV_RESET "hv-reset"
+#define VIR_CPU_x86_KVM_HV_FREQUENCIES "hv-frequencies"
+#define VIR_CPU_x86_KVM_HV_REENLIGHTENMENT "hv-reenlightenment"
+#define VIR_CPU_x86_KVM_HV_TLBFLUSH "hv-tlbflush"
+#define VIR_CPU_x86_KVM_HV_IPI "hv-ipi"
+#define VIR_CPU_x86_KVM_HV_EVMCS "hv-evmcs"
#define VIR_CPU_X86_DATA_INIT { 0 }
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 75205bc121..7561781848 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -4104,7 +4104,7 @@ qemuProcessVerifyHypervFeatures(virDomainDefPtr def,
if (def->hyperv_features[i] != VIR_TRISTATE_SWITCH_ON)
continue;
-if (virAsprintf(, "__kvm_hv_%s",
+if (virAsprintf(, "hv-%s",
virDomainHypervTypeToString(i)) < 0)
return -1;
--
2.22.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/5] qemu: Prefer dashes for hyperv features
Starting with QEMU 4.1, we're using the canonical feature names on the
command line and avoid aliases to prepare for possible deprecation of
all aliases in QEMU. But we do so only for features from our CPU map,
hyperv features defined in the code were unchanged and this patch fixes
it. Some features use "hv-" prefix unconditionally because they were
introduced recently enough to always support spelling with a dash.
Signed-off-by: Jiri Denemark
---
src/qemu/qemu_command.c | 17 -
.../clock-timer-hyperv-rtc.args | 2 +-
tests/qemuxml2argvdata/hyperv-panic.args| 2 +-
tests/qemuxml2argvdata/hyperv.args | 4 ++--
tests/qemuxml2argvdata/panic-double.args| 2 +-
5 files changed, 17 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 476e710257..c2f99034c8 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7133,7 +7133,7 @@ qemuBuildCpuCommandLine(virCommandPtr cmd,
!!timer->present);
} else if (timer->name == VIR_DOMAIN_TIMER_NAME_HYPERVCLOCK &&
timer->present == 1) {
-virBufferAddLit(, ",hv_time");
+virBufferAddLit(, ",hv-time");
} else if (timer->name == VIR_DOMAIN_TIMER_NAME_TSC &&
timer->frequency > 0) {
virBufferAsprintf(, ",tsc-frequency=%lu", timer->frequency);
@@ -7151,6 +7151,11 @@ qemuBuildCpuCommandLine(virCommandPtr cmd,
}
if (def->features[VIR_DOMAIN_FEATURE_HYPERV] == VIR_TRISTATE_SWITCH_ON) {
+const char *hvPrefix = "hv-";
+
+if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_CANONICAL_CPU_FEATURES))
+hvPrefix = "hv_";
+
for (i = 0; i < VIR_DOMAIN_HYPERV_LAST; i++) {
switch ((virDomainHyperv) i) {
case VIR_DOMAIN_HYPERV_RELAXED:
@@ -7166,19 +7171,21 @@ qemuBuildCpuCommandLine(virCommandPtr cmd,
case VIR_DOMAIN_HYPERV_IPI:
case VIR_DOMAIN_HYPERV_EVMCS:
if (def->hyperv_features[i] == VIR_TRISTATE_SWITCH_ON)
-virBufferAsprintf(, ",hv_%s",
+virBufferAsprintf(, ",%s%s",
+ hvPrefix,
virDomainHypervTypeToString(i));
break;
case VIR_DOMAIN_HYPERV_SPINLOCKS:
if (def->hyperv_features[i] == VIR_TRISTATE_SWITCH_ON)
-virBufferAsprintf(, ",hv_spinlocks=0x%x",
+virBufferAsprintf(, ",%s=0x%x",
+ VIR_CPU_x86_KVM_HV_SPINLOCKS,
def->hyperv_spinlocks);
break;
case VIR_DOMAIN_HYPERV_VENDOR_ID:
if (def->hyperv_features[i] == VIR_TRISTATE_SWITCH_ON)
-virBufferAsprintf(, ",hv_vendor_id=%s",
+virBufferAsprintf(, ",hv-vendor-id=%s",
def->hyperv_vendor_id);
break;
@@ -7191,7 +7198,7 @@ qemuBuildCpuCommandLine(virCommandPtr cmd,
for (i = 0; i < def->npanics; i++) {
if (def->panics[i]->model == VIR_DOMAIN_PANIC_MODEL_HYPERV) {
-virBufferAddLit(, ",hv_crash");
+virBufferAddLit(, ",hv-crash");
break;
}
}
diff --git a/tests/qemuxml2argvdata/clock-timer-hyperv-rtc.args
b/tests/qemuxml2argvdata/clock-timer-hyperv-rtc.args
index 98b7dcae1b..d75585bf0f 100644
--- a/tests/qemuxml2argvdata/clock-timer-hyperv-rtc.args
+++ b/tests/qemuxml2argvdata/clock-timer-hyperv-rtc.args
@@ -11,7 +11,7 @@ QEMU_AUDIO_DRV=none \
-name QEMUGuest1 \
-S \
-machine pc,accel=kvm,usb=off,dump-guest-core=off \
--cpu qemu32,hv_time \
+-cpu qemu32,hv-time \
-m 214 \
-realtime mlock=off \
-smp 6,sockets=6,cores=1,threads=1 \
diff --git a/tests/qemuxml2argvdata/hyperv-panic.args
b/tests/qemuxml2argvdata/hyperv-panic.args
index 3226837fbd..1ef5068aca 100644
--- a/tests/qemuxml2argvdata/hyperv-panic.args
+++ b/tests/qemuxml2argvdata/hyperv-panic.args
@@ -11,7 +11,7 @@ QEMU_AUDIO_DRV=none \
-name QEMUGuest1 \
-S \
-machine pc,accel=tcg,usb=off,dump-guest-core=off \
--cpu qemu32,hv_crash \
+-cpu qemu32,hv-crash \
-m 214 \
-realtime mlock=off \
-smp 6,sockets=6,cores=1,threads=1 \
diff --git a/tests/qemuxml2argvdata/hyperv.args
b/tests/qemuxml2argvdata/hyperv.args
index c557b6d8fe..086adaa349 100644
--- a/tests/qemuxml2argvdata/hyperv.args
+++ b/tests/qemuxml2argvdata/hyperv.args
@@ -11,8 +11,8 @@ QEMU_AUDIO_DRV=none \
-name QEMUGuest1 \
-S \
-machine pc,accel=tcg,usb=off,dump-guest-core=off \
--cpu 'qemu32,hv_relaxed,hv_vapic,hv_spinlocks=0x2fff,hv_vpindex,hv_runtime,\
-hv_synic,hv_stimer,hv_reset,hv_vendor_id=KVM Hv,hv_frequencies,\
+-cpu 'qemu32,hv_relaxed,hv_vapic,hv-spinlocks=0x2fff,hv_vpindex,hv_runtime,\
+hv_synic,hv_stimer,hv_reset,hv-vendor-id=KVM
[libvirt] [PATCH 3/5] cpu: Drop KVM_ from hyperv feature macros
All the features are hyperv features even though they are provided by
KVM with QEMU. The "KVM" part in the macro names does not make a lot of
sense.
Signed-off-by: Jiri Denemark
---
src/cpu/cpu_x86.c | 53 +
src/cpu/cpu_x86_data.h | 26 ++--
src/qemu/qemu_command.c | 2 +-
3 files changed, 41 insertions(+), 40 deletions(-)
diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c
index 55b55da784..f8a51dedf6 100644
--- a/src/cpu/cpu_x86.c
+++ b/src/cpu/cpu_x86.c
@@ -91,31 +91,32 @@ KVM_FEATURE_DEF(VIR_CPU_x86_KVM_PV_UNHALT,
0x4001, 0x0080);
KVM_FEATURE_DEF(VIR_CPU_x86_KVM_CLOCKSOURCE_STABLE_BIT,
0x4001, 0x0100);
-KVM_FEATURE_DEF(VIR_CPU_x86_KVM_HV_RUNTIME,
+
+KVM_FEATURE_DEF(VIR_CPU_x86_HV_RUNTIME,
0x4003, 0x0001);
-KVM_FEATURE_DEF(VIR_CPU_x86_KVM_HV_SYNIC,
+KVM_FEATURE_DEF(VIR_CPU_x86_HV_SYNIC,
0x4003, 0x0004);
-KVM_FEATURE_DEF(VIR_CPU_x86_KVM_HV_STIMER,
+KVM_FEATURE_DEF(VIR_CPU_x86_HV_STIMER,
0x4003, 0x0008);
-KVM_FEATURE_DEF(VIR_CPU_x86_KVM_HV_RELAXED,
+KVM_FEATURE_DEF(VIR_CPU_x86_HV_RELAXED,
0x4003, 0x0020);
-KVM_FEATURE_DEF(VIR_CPU_x86_KVM_HV_SPINLOCKS,
+KVM_FEATURE_DEF(VIR_CPU_x86_HV_SPINLOCKS,
0x4003, 0x0022);
-KVM_FEATURE_DEF(VIR_CPU_x86_KVM_HV_VAPIC,
+KVM_FEATURE_DEF(VIR_CPU_x86_HV_VAPIC,
0x4003, 0x0030);
-KVM_FEATURE_DEF(VIR_CPU_x86_KVM_HV_VPINDEX,
+KVM_FEATURE_DEF(VIR_CPU_x86_HV_VPINDEX,
0x4003, 0x0040);
-KVM_FEATURE_DEF(VIR_CPU_x86_KVM_HV_RESET,
+KVM_FEATURE_DEF(VIR_CPU_x86_HV_RESET,
0x4003, 0x0080);
-KVM_FEATURE_DEF(VIR_CPU_x86_KVM_HV_FREQUENCIES,
+KVM_FEATURE_DEF(VIR_CPU_x86_HV_FREQUENCIES,
0x4003, 0x0800);
-KVM_FEATURE_DEF(VIR_CPU_x86_KVM_HV_REENLIGHTENMENT,
+KVM_FEATURE_DEF(VIR_CPU_x86_HV_REENLIGHTENMENT,
0x4003, 0x2000);
-KVM_FEATURE_DEF(VIR_CPU_x86_KVM_HV_TLBFLUSH,
+KVM_FEATURE_DEF(VIR_CPU_x86_HV_TLBFLUSH,
0x4004, 0x0004);
-KVM_FEATURE_DEF(VIR_CPU_x86_KVM_HV_IPI,
+KVM_FEATURE_DEF(VIR_CPU_x86_HV_IPI,
0x4004, 0x0400);
-KVM_FEATURE_DEF(VIR_CPU_x86_KVM_HV_EVMCS,
+KVM_FEATURE_DEF(VIR_CPU_x86_HV_EVMCS,
0x4004, 0x4000);
static virCPUx86Feature x86_kvm_features[] =
@@ -129,19 +130,19 @@ static virCPUx86Feature x86_kvm_features[] =
KVM_FEATURE(VIR_CPU_x86_KVM_PV_EOI),
KVM_FEATURE(VIR_CPU_x86_KVM_PV_UNHALT),
KVM_FEATURE(VIR_CPU_x86_KVM_CLOCKSOURCE_STABLE_BIT),
-KVM_FEATURE(VIR_CPU_x86_KVM_HV_RUNTIME),
-KVM_FEATURE(VIR_CPU_x86_KVM_HV_SYNIC),
-KVM_FEATURE(VIR_CPU_x86_KVM_HV_STIMER),
-KVM_FEATURE(VIR_CPU_x86_KVM_HV_RELAXED),
-KVM_FEATURE(VIR_CPU_x86_KVM_HV_SPINLOCKS),
-KVM_FEATURE(VIR_CPU_x86_KVM_HV_VAPIC),
-KVM_FEATURE(VIR_CPU_x86_KVM_HV_VPINDEX),
-KVM_FEATURE(VIR_CPU_x86_KVM_HV_RESET),
-KVM_FEATURE(VIR_CPU_x86_KVM_HV_FREQUENCIES),
-KVM_FEATURE(VIR_CPU_x86_KVM_HV_REENLIGHTENMENT),
-KVM_FEATURE(VIR_CPU_x86_KVM_HV_TLBFLUSH),
-KVM_FEATURE(VIR_CPU_x86_KVM_HV_IPI),
-KVM_FEATURE(VIR_CPU_x86_KVM_HV_EVMCS),
+KVM_FEATURE(VIR_CPU_x86_HV_RUNTIME),
+KVM_FEATURE(VIR_CPU_x86_HV_SYNIC),
+KVM_FEATURE(VIR_CPU_x86_HV_STIMER),
+KVM_FEATURE(VIR_CPU_x86_HV_RELAXED),
+KVM_FEATURE(VIR_CPU_x86_HV_SPINLOCKS),
+KVM_FEATURE(VIR_CPU_x86_HV_VAPIC),
+KVM_FEATURE(VIR_CPU_x86_HV_VPINDEX),
+KVM_FEATURE(VIR_CPU_x86_HV_RESET),
+KVM_FEATURE(VIR_CPU_x86_HV_FREQUENCIES),
+KVM_FEATURE(VIR_CPU_x86_HV_REENLIGHTENMENT),
+KVM_FEATURE(VIR_CPU_x86_HV_TLBFLUSH),
+KVM_FEATURE(VIR_CPU_x86_HV_IPI),
+KVM_FEATURE(VIR_CPU_x86_HV_EVMCS),
};
typedef struct _virCPUx86Model virCPUx86Model;
diff --git a/src/cpu/cpu_x86_data.h b/src/cpu/cpu_x86_data.h
index bb781707aa..85aaab709c 100644
--- a/src/cpu/cpu_x86_data.h
+++ b/src/cpu/cpu_x86_data.h
@@ -58,19 +58,19 @@ struct _virCPUx86MSR {
* ones defined for virDomainHyperv in domain_conf.c.
* E.g "hv-runtime" -> "runtime", "hv-spinlocks" -> "spinlocks" etc.
*/
-#define VIR_CPU_x86_KVM_HV_RUNTIME "hv-runtime"
-#define VIR_CPU_x86_KVM_HV_SYNIC "hv-synic"
-#define VIR_CPU_x86_KVM_HV_STIMER"hv-stimer"
-#define VIR_CPU_x86_KVM_HV_RELAXED "hv-relaxed"
-#define VIR_CPU_x86_KVM_HV_SPINLOCKS "hv-spinlocks"
-#define VIR_CPU_x86_KVM_HV_VAPIC "hv-vapic"
-#define VIR_CPU_x86_KVM_HV_VPINDEX "hv-vpindex"
-#define VIR_CPU_x86_KVM_HV_RESET "hv-reset"
-#define VIR_CPU_x86_KVM_HV_FREQUENCIES "hv-frequencies"
-#define VIR_CPU_x86_KVM_HV_REENLIGHTENMENT "hv-reenlightenment"
-#define VIR_CPU_x86_KVM_HV_TLBFLUSH "hv-tlbflush"
-#define VIR_CPU_x86_KVM_HV_IPI "hv-ipi"
-#define VIR_CPU_x86_KVM_HV_EVMCS "hv-evmcs"
+#define VIR_CPU_x86_HV_RUNTIME "hv-runtime"
+#define VIR_CPU_x86_HV_SYNIC "hv-synic"
+#define VIR_CPU_x86_HV_STIMER
[libvirt] [PATCH 0/5] Fix hyperv and kvm features with QEMU 4.1
Originally the names of the hyperv and kvm CPU features were only used internally for looking up their CPUID bits. But with QEMU 4.1 we check which features were enabled or disabled by a freshly started QEMU process using their names rather than their CPUID bits (mostly because of MSR features). Thus we need to change our made up internal names into the actual names used by QEMU. Otherwise libvirt would mistakenly report the features as unavailable and refuse to start any domain using them with QEMU 4.1. Reported-by: Vitaly Kuznetsov Jiri Denemark (5): qemu: Fix hyperv features with QEMU 4.1 qemu: Prefer dashes for hyperv features cpu: Drop KVM_ from hyperv feature macros cpu: Drop unused KVM features qemu: Fix KVM features with QEMU 4.1 src/cpu/cpu_x86.c | 77 +++ src/cpu/cpu_x86_data.h| 38 - src/qemu/qemu_command.c | 19 +++-- src/qemu/qemu_process.c | 2 +- .../clock-timer-hyperv-rtc.args | 2 +- tests/qemuxml2argvdata/hyperv-panic.args | 2 +- tests/qemuxml2argvdata/hyperv.args| 4 +- tests/qemuxml2argvdata/panic-double.args | 2 +- 8 files changed, 61 insertions(+), 85 deletions(-) -- 2.22.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 08/41] remote: conditionalize daemon name in libvirtd daemon
On Fri, Jul 26, 2019 at 02:39:51PM +0200, Andrea Bolognani wrote: > On Tue, 2019-07-23 at 17:02 +0100, Daniel P. Berrangé wrote: > [...] > > +++ b/src/remote/remote_driver.h > > @@ -34,7 +34,6 @@ unsigned long remoteVersion(void); > > #define LIBVIRTD_PRIV_UNIX_SOCKET LOCALSTATEDIR "/run/libvirt/libvirt-sock" > > #define LIBVIRTD_PRIV_UNIX_SOCKET_RO LOCALSTATEDIR > > "/run/libvirt/libvirt-sock-ro" > > #define LIBVIRTD_USER_UNIX_SOCKET "libvirt-sock" > > -#define LIBVIRTD_CONFIGURATION_FILE SYSCONFDIR "/libvirt/libvirtd.conf" > > Oh, this was unused even before your changes, wasn't it? You should > drop it in a separate, trivial patch. No it was used, but this should have been done in patch 6 instead. 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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 06/41] remote: stop trying to print help as giant blocks of text
On Fri, Jul 26, 2019 at 02:02:18PM +0200, Andrea Bolognani wrote:
> On Tue, 2019-07-23 at 17:02 +0100, Daniel P. Berrangé wrote:
> [...]
> > +struct virOptionHelp {
> > +const char *opts;
> > +const char *help;
> > +} opthelp[] = {
> > +{ "-h | --help", N_("Display program help") },
> > +{ "-v | --verbose", N_("Verbose messages") },
> > +{ "-d | --daemon", N_("Run as a daemon & write PID file") },
> > +{ "-l | --listen", N_("Listen for TCP/IP connections") },
> > +{ "-t | --timeout ", N_("Exit after timeout period") },
> > +{ "-f | --config ", N_("Configuration file") },
> > +{ "-V | --version", N_("Display version information") },
> > +{ "-p | --pid-file ", N_("Change name of PID file") },
> > +};
>
> The way you declare the struct at the same time as you use it for a
> local variable is highly unusual in our codebase, especially
> considering that you do the former inside a function which is, at
> least as far as I'm aware, basically unprecedented in libvirt.
>
> Can you please move the struct declaration outside of the function?
>
> [...]
> > +fprintf(stderr, "%s:\n", _("TLS"));
> > +fprintf(stderr, " %s: %s\n",
> > +_("CA certificate"),
> > +privileged ? LIBVIRT_CACERT : "$HOME/.pki/libvirt/cacert.pem");
> > +fprintf(stderr, " %s: %s\n",
> > +_("Server certificate"),
> > +privileged ? LIBVIRT_SERVERCERT :
> > "$HOME/.pki/libvirt/servercert.pem");
> > +fprintf(stderr, " %s: %s\n",
> > +_("Server private key"),
> > +privileged ? LIBVIRT_SERVERKEY :
> > "$HOME/.pki/libvirt/serverkey.pem");
> > +fprintf(stderr, "\n");
>
> I think the above would work better if you used
>
> " %-18s %s\n"
>
> as the format string, which would result in
>
> TLS:
> CA certificate $HOME/.pki/libvirt/cacert.pem
> Server certificate $HOME/.pki/libvirt/servercert.pem
> Server private key $HOME/.pki/libvirt/serverkey.pem
>
> instead of
>
> TLS:
> CA certificate: $HOME/.pki/libvirt/cacert.pem
> Server certificate: $HOME/.pki/libvirt/servercert.pem
> Server private key: $HOME/.pki/libvirt/serverkey.pem
>
> as the output. Not a big deal either way, but since we're going out
> of our way to align options and their descriptions above it makes
> sense to me that we'd do the same here as well.
>
>
> After this patch, the code looks much less readable, but I understand
> why you want to do this and you also get rid of some duplication when
> it comes to the system/session daemons, so I'm overall okay with the
> change.
Yeah it is pretty ugly, but at least some of this uglyness goes away
if we adopt GLib, because its CLI parsing APIs are much better than
getopt, in particular capable of auto-generating the help output for
all args.
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 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 11/41] remote: conditionalize IP socket config in libvirtd.conf
On Fri, Jul 26, 2019 at 03:59:23PM +0200, Andrea Bolognani wrote: > On Tue, 2019-07-23 at 17:02 +0100, Daniel P. Berrangé wrote: > > +remote/libvirtd.conf: remote/libvirtd.conf.in > > + $(AM_V_GEN)sed \ > > + -e '/:: CUT ENABLE_IP ::/d' \ > > + -e '/:: END ::/d' \ > > + -e 's/:: DAEMON_NAME ::/libvirtd/' \ > > + < $^ > $@ > > Using $^ seems a bit weird considering that you have a single > input... Any reason not to use $< here? it is an accident. > I would also use @DAEMON_NAME@ instead of ":: DAEMON_NAME ::" since > the former style is already used everywhere for simple value > replacement in .in files. The augeas files already use "::" which is why I picked that. So we'll need another patch first to remove existing usage. 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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 13/41] remote: refactor & rename variables for building libvirtd
On Fri, Jul 26, 2019 at 05:19:56PM +0200, Andrea Bolognani wrote: > On Tue, 2019-07-23 at 17:02 +0100, Daniel P. Berrangé wrote: > > The same make variables will be useful for building both libvirtd and > > the split daemons, so refactor & rename variables to facilitate reuse. > > > > Automake gets annoyed if you define a variable ending LDFLAGS: > > > > src/remote/Makefile.inc.am:53: warning: variable 'REMOTE_DAEMON_LDFLAGS' is > > defined but no program or > > src/remote/Makefile.inc.am:53: library has 'REMOTE_DAEMON' as canonical > > name (possible typo) > > I'd indent this by two spaces for readabilty. > > You're recreating the pre-existing setup faithfully, so > > Reviewed-by: Andrea Bolognani > > [...] > > +REMOTE_DAEMON_LD_ADD += \ > > + ../gnulib/lib/libgnu.la \ > > + $(LIBSOCKET) \ > > + $(NULL) > > As an aside, it looks like $(LIBSOCKET) is a leftover of days long > gone and no longer used for anything. It is something that gnulib defines. Whether it expands to a non-empty string on any of our supported build platforms though, I don't know. 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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 13/41] remote: refactor & rename variables for building libvirtd
On Tue, 2019-07-23 at 17:02 +0100, Daniel P. Berrangé wrote: > The same make variables will be useful for building both libvirtd and > the split daemons, so refactor & rename variables to facilitate reuse. > > Automake gets annoyed if you define a variable ending LDFLAGS: > > src/remote/Makefile.inc.am:53: warning: variable 'REMOTE_DAEMON_LDFLAGS' is > defined but no program or > src/remote/Makefile.inc.am:53: library has 'REMOTE_DAEMON' as canonical name > (possible typo) I'd indent this by two spaces for readabilty. You're recreating the pre-existing setup faithfully, so Reviewed-by: Andrea Bolognani [...] > +REMOTE_DAEMON_LD_ADD += \ > + ../gnulib/lib/libgnu.la \ > + $(LIBSOCKET) \ > + $(NULL) As an aside, it looks like $(LIBSOCKET) is a leftover of days long gone and no longer used for anything. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/4] tests: Call virCommandFree() in cleanup section
On Fri, Jul 26, 2019 at 09:42:44AM -0400, Stefan Berger wrote: Fix a potential memory leak by calling virCommandFree() in the cleanup section. Signed-off-by: Stefan Berger --- tests/commandtest.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/commandtest.c b/tests/commandtest.c index 991c0572b0..dfd15a2079 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -1219,8 +1219,6 @@ static int test27(const void *unused ATTRIBUTE_UNUSED) goto cleanup; } -virCommandFree(cmd); - if (!outactual || !erractual) goto cleanup; @@ -1236,6 +1234,7 @@ static int test27(const void *unused ATTRIBUTE_UNUSED) ret = 0; cleanup: +virCommandFree(cmd); VIR_FORCE_CLOSE(pipe1[0]); VIR_FORCE_CLOSE(pipe2[0]); VIR_FORCE_CLOSE(pipe1[1]); We have the VIR_AUTO* macros you can use to get the compiler to free it automatically when it goes out of scope. Just declare the variable as: VIR_AUTOPTR(virCommand) cmd = NULL; Jano signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/4] Cleanups on vTPM encrytion patches
On 7/26/19 9:42 AM, Stefan Berger wrote: > This series of patches cleans up the Coverity issues reported by John. > >Stefan > > Stefan Berger (4): > tpm: Fix memory leak and use existing variable instead > tests: Call virCommandFree() in cleanup section > utils: Remove ATTRIBUTE_NONNULL(1) from virCommandSetSendBuffer > conf: Remove ATTRIBUTE_NONNULL(2) from virDomainCheckDeviceChanges() > > src/conf/domain_conf.h | 3 +-- > src/qemu/qemu_tpm.c| 4 ++-- > src/util/vircommand.h | 2 +- > tests/commandtest.c| 3 +-- > 4 files changed, 5 insertions(+), 7 deletions(-) > With the cleanup in patch3 Reviewed-by: John Ferlan (series) John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/4] utils: Remove ATTRIBUTE_NONNULL(1) from virCommandSetSendBuffer
On 7/26/19 9:42 AM, Stefan Berger wrote: > Remove the ATTRIBUTE_NONNULL(1) from virCommandSetSendBuffer() > prototype since we are checking for '!cmd'. > > Signed-off-by: Stefan Berger > --- > src/util/vircommand.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > Coverity will complain that virCommandSetSendBuffer has: size_t i = virCommandGetNumSendBuffers(cmd); if (!cmd || cmd->has_error) return -1; Since @cmd is dereffed in virCommandGetNumSendBuffers Just move the initialization to after the return -1; and things will be good. John > diff --git a/src/util/vircommand.h b/src/util/vircommand.h > index c2abc7b2c3..74574e3fb1 100644 > --- a/src/util/vircommand.h > +++ b/src/util/vircommand.h > @@ -149,7 +149,7 @@ void virCommandSetWorkingDirectory(virCommandPtr cmd, > int virCommandSetSendBuffer(virCommandPtr cmd, > int fd, > unsigned char *buffer, size_t buflen) > -ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3); > +ATTRIBUTE_NONNULL(3); > > void virCommandSetInputBuffer(virCommandPtr cmd, >const char *inbuf) ATTRIBUTE_NONNULL(2); > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 12/41] remote: conditionalize IP socket config in augeas definitions
On Tue, 2019-07-23 at 17:02 +0100, Daniel P. Berrangé wrote:
[...]
> remote/test_libvirtd.aug: remote/test_libvirtd.aug.in \
> remote/libvirtd.conf $(AUG_GENTEST)
> - $(AM_V_GEN)$(AUG_GENTEST) remote/libvirtd.conf $< > $@
> + $(AM_V_GEN)$(AUG_GENTEST) remote/libvirtd.conf \
> + $(srcdir)/remote/test_libvirtd.aug.in | \
> + $(SED) -e '/:: CUT ENABLE_IP ::/d' \
> + -e '/:: END ::/d' \
> + -e 's/:: DAEMON_NAME ::/libvirtd/' \
> + -e 's/:: DAEMON_NAME_UC ::/Libvirtd/' \
> + > $@ || rm -f $@
The indentation for sed arguments, especially the first one, is
quite awkward here.
[...]
> +++ b/src/remote/libvirtd.aug.in
> @@ -1,6 +1,6 @@
> -(* /etc/libvirt/libvirtd.conf *)
> +(* /etc/libvirt/:: DAEMON_NAME ::.conf *)
This is a pretty obvious example of ":: VARIABLE ::" being inferior
than the existing convention: compare it with the much more readable
(* /etc/libvirt/@DAEMON_NAME@.conf *)
[...]
> +++ b/src/remote/test_libvirtd.aug.in
> @@ -48,7 +54,7 @@ module Test_libvirtd =
> { "admin_max_client_requests" = "5" }
> { "log_level" = "3" }
> { "log_filters" = "1:qemu 1:libvirt 4:object 4:json 4:event 1:util" }
> -{ "log_outputs" = "3:syslog:libvirtd" }
> +{ "log_outputs" = "3:syslog::: DAEMON_NAME ::" }
And another example right here:
{ "log_outputs" = "3:syslog:@DAEMON_NAME@" }
would be much better.
With the markers used for variable substitution changed,
Reviewed-by: Andrea Bolognani
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 11/41] remote: conditionalize IP socket config in libvirtd.conf
On Tue, 2019-07-23 at 17:02 +0100, Daniel P. Berrangé wrote: > +remote/libvirtd.conf: remote/libvirtd.conf.in > + $(AM_V_GEN)sed \ > + -e '/:: CUT ENABLE_IP ::/d' \ > + -e '/:: END ::/d' \ > + -e 's/:: DAEMON_NAME ::/libvirtd/' \ > + < $^ > $@ Using $^ seems a bit weird considering that you have a single input... Any reason not to use $< here? I would also use @DAEMON_NAME@ instead of ":: DAEMON_NAME ::" since the former style is already used everywhere for simple value replacement in .in files. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] test_driver: implement virDomainMemoryStats
On Wed, Jul 24, 2019 at 4:19 PM Erik Skultety wrote:
>
> On Tue, Jul 16, 2019 at 11:36:31PM +0200, Ilias Stamatis wrote:
> > Signed-off-by: Ilias Stamatis
> > ---
> > src/test/test_driver.c | 52 ++
> > 1 file changed, 52 insertions(+)
> >
> > diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> > index fcb80c9e47..55976eedf1 100644
> > --- a/src/test/test_driver.c
> > +++ b/src/test/test_driver.c
> > @@ -6845,6 +6845,57 @@ testDomainManagedSaveRemove(virDomainPtr dom,
> > unsigned int flags)
> > return 0;
> > }
> >
> > +
> > +static int
> > +testDomainMemoryStats(virDomainPtr dom,
> > + virDomainMemoryStatPtr stats,
> > + unsigned int nr_stats,
> > + unsigned int flags)
> > +{
> > +virDomainObjPtr vm;
> > +int ret = -1;
> > +
> > +virCheckFlags(0, -1);
> > +
> > +if (!(vm = testDomObjFromDomain(dom)))
> > +return -1;
> > +
> > +if (virDomainObjCheckActive(vm) < 0)
> > +goto cleanup;
> > +
> > +ret = 0;
> > +
> > +#define STATS_SET_PARAM(name, value) \
> > +if (ret < nr_stats) { \
> > +stats[ret].tag = name; \
> > +stats[ret].val = value; \
> > +ret++; \
> > +}
> > +
> > +if (virDomainDefHasMemballoon(vm->def)) {
> > +STATS_SET_PARAM(VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON, 1024000);
>
> ^This one...
>
> > +STATS_SET_PARAM(VIR_DOMAIN_MEMORY_STAT_SWAP_IN, 0);
> > +STATS_SET_PARAM(VIR_DOMAIN_MEMORY_STAT_SWAP_OUT, 0);
> > +STATS_SET_PARAM(VIR_DOMAIN_MEMORY_STAT_MAJOR_FAULT, 0);
> > +STATS_SET_PARAM(VIR_DOMAIN_MEMORY_STAT_MINOR_FAULT, 0);
> > +STATS_SET_PARAM(VIR_DOMAIN_MEMORY_STAT_UNUSED, 791584);
> > +STATS_SET_PARAM(VIR_DOMAIN_MEMORY_STAT_AVAILABLE, 977816);
>
> ...and this one should be taken from the definition from what is reported in
> . The reasoning behind that is that you can have multiple
> machines defined and those values wouldn't make sense.
Right, I'll fix that.
> The rest can be semi-random in that the values need to be less than those I
> mentioned.
My concern here again is that we will have to send different values
every time depending on the config. E.g. currentMemory - 45000.
But is that good for testing?
Ilias
>
> > +STATS_SET_PARAM(VIR_DOMAIN_MEMORY_STAT_USABLE, 726244);
> > +STATS_SET_PARAM(VIR_DOMAIN_MEMORY_STAT_LAST_UPDATE, 627319920);
>
> I don't object to ^this unless it is affected by balloon period setting. If it
> is, then it might make sense to take it into account. I haven't checked that
> myself though, so it may simply not be worth anyway.
>
> Erik
>
> > +STATS_SET_PARAM(VIR_DOMAIN_MEMORY_STAT_DISK_CACHES, 164964);
> > +STATS_SET_PARAM(VIR_DOMAIN_MEMORY_STAT_HUGETLB_PGALLOC, 0);
> > +STATS_SET_PARAM(VIR_DOMAIN_MEMORY_STAT_HUGETLB_PGFAIL, 0);
> > +STATS_SET_PARAM(VIR_DOMAIN_MEMORY_STAT_RSS, 1131076);
> > +}
> > +
> > +#undef STATS_SET_PARAM
> > +
> > + cleanup:
> > +virDomainObjEndAPI();
> > +return ret;
> > +}
> > +
> > +
> > static int
> > testDomainMemoryPeek(virDomainPtr dom,
> > unsigned long long start,
> > @@ -7799,6 +7850,7 @@ static virHypervisorDriver testHypervisorDriver = {
> > .domainManagedSave = testDomainManagedSave, /* 1.1.4 */
> > .domainHasManagedSaveImage = testDomainHasManagedSaveImage, /* 1.1.4 */
> > .domainManagedSaveRemove = testDomainManagedSaveRemove, /* 1.1.4 */
> > +.domainMemoryStats = testDomainMemoryStats, /* 5.6.0 */
> > .domainMemoryPeek = testDomainMemoryPeek, /* 5.4.0 */
> >
> > .domainSnapshotNum = testDomainSnapshotNum, /* 1.1.4 */
> > --
> > 2.22.0
> >
> > --
> > libvir-list mailing list
> > libvir-list@redhat.com
> > https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 3/4] utils: Remove ATTRIBUTE_NONNULL(1) from virCommandSetSendBuffer
Remove the ATTRIBUTE_NONNULL(1) from virCommandSetSendBuffer() prototype since we are checking for '!cmd'. Signed-off-by: Stefan Berger --- src/util/vircommand.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/vircommand.h b/src/util/vircommand.h index c2abc7b2c3..74574e3fb1 100644 --- a/src/util/vircommand.h +++ b/src/util/vircommand.h @@ -149,7 +149,7 @@ void virCommandSetWorkingDirectory(virCommandPtr cmd, int virCommandSetSendBuffer(virCommandPtr cmd, int fd, unsigned char *buffer, size_t buflen) -ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3); +ATTRIBUTE_NONNULL(3); void virCommandSetInputBuffer(virCommandPtr cmd, const char *inbuf) ATTRIBUTE_NONNULL(2); -- 2.20.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/4] tpm: Fix memory leak and use existing variable instead
Use the existing variables rather then calling virTPMSwtpmXYZ().
Signed-off-by: Stefan Berger
---
src/qemu/qemu_tpm.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
index 7efd635831..a8a4d734c0 100644
--- a/src/qemu/qemu_tpm.c
+++ b/src/qemu/qemu_tpm.c
@@ -508,7 +508,7 @@ qemuTPMEmulatorRunSetup(const char *storagepath,
VIR_TPM_SWTPM_SETUP_FEATURE_CMDARG_PWDFILE_FD)) {
virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED,
_("%s does not support passing a passphrase using a file "
- "descriptor"), virTPMGetSwtpmSetup());
+ "descriptor"), swtpm_setup);
goto cleanup;
}
if ((pwdfile_fd = qemuTPMSetupEncryption(secretuuid, cmd)) < 0)
@@ -648,7 +648,7 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDefPtr tpm,
if (!virTPMSwtpmCapsGet(VIR_TPM_SWTPM_FEATURE_CMDARG_PWD_FD)) {
virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED,
_("%s does not support passing passphrase via file
descriptor"),
- virTPMGetSwtpm());
+ swtpm);
goto error;
}
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/4] Cleanups on vTPM encrytion patches
This series of patches cleans up the Coverity issues reported by John. Stefan Stefan Berger (4): tpm: Fix memory leak and use existing variable instead tests: Call virCommandFree() in cleanup section utils: Remove ATTRIBUTE_NONNULL(1) from virCommandSetSendBuffer conf: Remove ATTRIBUTE_NONNULL(2) from virDomainCheckDeviceChanges() src/conf/domain_conf.h | 3 +-- src/qemu/qemu_tpm.c| 4 ++-- src/util/vircommand.h | 2 +- tests/commandtest.c| 3 +-- 4 files changed, 5 insertions(+), 7 deletions(-) -- 2.20.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/4] tests: Call virCommandFree() in cleanup section
Fix a potential memory leak by calling virCommandFree() in the cleanup section. Signed-off-by: Stefan Berger --- tests/commandtest.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/commandtest.c b/tests/commandtest.c index 991c0572b0..dfd15a2079 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -1219,8 +1219,6 @@ static int test27(const void *unused ATTRIBUTE_UNUSED) goto cleanup; } -virCommandFree(cmd); - if (!outactual || !erractual) goto cleanup; @@ -1236,6 +1234,7 @@ static int test27(const void *unused ATTRIBUTE_UNUSED) ret = 0; cleanup: +virCommandFree(cmd); VIR_FORCE_CLOSE(pipe1[0]); VIR_FORCE_CLOSE(pipe2[0]); VIR_FORCE_CLOSE(pipe1[1]); -- 2.20.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 4/4] conf: Remove ATTRIBUTE_NONNULL(2) from virDomainCheckDeviceChanges()
Since we are checking the 2nd parameter in the function for NULL, we need to remove ATTRIBUTE_NONNULL(2) from the prototype. Signed-off-by: Stefan Berger --- src/conf/domain_conf.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 285fa6c496..dc480bc7c5 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3638,5 +3638,4 @@ bool virDomainGraphicsNeedsAutoRenderNode(const virDomainGraphicsDef *graphics); int -virDomainCheckDeviceChanges(virDomainDefPtr def, virDomainDefPtr newDef) -ATTRIBUTE_NONNULL(2); +virDomainCheckDeviceChanges(virDomainDefPtr def, virDomainDefPtr newDef); -- 2.20.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 10/41] remote: conditionalize IP socket usage in libvirtd daemon
On Tue, 2019-07-23 at 17:02 +0100, Daniel P. Berrangé wrote:
[...]
> @@ -988,7 +1004,13 @@ int main(int argc, char **argv) {
> int c;
> char *tmp;
>
> -c = getopt_long(argc, argv, "ldf:p:t:vVh", opts, );
> +c = getopt_long(argc, argv,
> +#ifdef ENABLE_IP
> +"ldf:p:t:vVh",
> +#else /* ! ENABLE_IP */
> +"df:p:t:vVh",
> +#endif /* ! ENABLE_IP */
> +opts, );
This looks pretty awful... Can you please do something like
#ifdef ENABLE_IP
const char *optstr = "ldf:p:t:vVh";
#else /* ! ENABLE_IP */
const char *optstr = "df:p:t:vVh";
#endif /* ! ENABLE_IP */
c = getopt_long(argc, argv, optstr, opts, );
instead?
[...]
> @@ -1003,9 +1025,11 @@ int main(int argc, char **argv) {
> case 'd':
> godaemon = 1;
> break;
One extra empty line here for clarity, please...
> +#ifdef ENABLE_IP
> case 'l':
> ipsock = 1;
> break;
> +#endif /* ! ENABLE_IP */
[...]
> +++ b/src/remote/remote_daemon_config.h
> @@ -41,21 +43,26 @@ struct daemonConfig {
>
> int auth_unix_rw;
> int auth_unix_ro;
... and one here as well.
With the above addressed,
Reviewed-by: Andrea Bolognani
[...]
> +char **sasl_allowed_username_list;
I like this approach you've taken of completely eliminating structure
members when the corresponding feature is not compiled in, and in
fact I think we should use it more extensively: for example, we
should guard sasl_allowed_username_list with IF_SASL. Out of scope
for the patch series at hand, of course! :)
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v7 15/19] tpm: Use fd to pass password to swtpm_setup and swtpm
On 7/26/19 6:47 AM, John Ferlan wrote:
On 7/25/19 2:22 PM, Stefan Berger wrote:
Allow vTPM state encryption when swtpm_setup and swtpm support
passing a passphrase using a file descriptor.
This patch enables the encryption of the vTPM state only. It does
not encrypt the state during migration, so the destination secret
does not need to have the same password at this point.
Signed-off-by: Stefan Berger
Reviewed-by: Daniel P. Berrangé
---
src/libvirt_private.syms | 2 +
src/qemu/qemu_tpm.c | 110 ++-
src/util/virtpm.c| 16 ++
src/util/virtpm.h| 3 ++
4 files changed, 129 insertions(+), 2 deletions(-)
[...]
/*
* qemuTPMEmulatorRunSetup
@@ -387,6 +448,7 @@ qemuTPMEmulatorPrepareHost(virDomainTPMDefPtr tpm,
* @logfile: The file to write the log into; it must be writable
* for the user given by userid or 'tss'
* @tpmversion: The version of the TPM, either a TPM 1.2 or TPM 2
+ * @encryption: pointer to virStorageEncryption holding secret
*
* Setup the external swtpm by creating endorsement key and
* certificates for it.
@@ -399,7 +461,8 @@ qemuTPMEmulatorRunSetup(const char *storagepath,
uid_t swtpm_user,
gid_t swtpm_group,
const char *logfile,
-const virDomainTPMVersion tpmversion)
+const virDomainTPMVersion tpmversion,
+const unsigned char *secretuuid)
{
virCommandPtr cmd = NULL;
int exitstatus;
@@ -407,6 +470,7 @@ qemuTPMEmulatorRunSetup(const char *storagepath,
char uuid[VIR_UUID_STRING_BUFLEN];
char *vmid = NULL;
VIR_AUTOFREE(char *)swtpm_setup = virTPMGetSwtpmSetup();
+VIR_AUTOCLOSE pwdfile_fd = -1;
if (!swtpm_setup)
return -1;
@@ -439,6 +503,23 @@ qemuTPMEmulatorRunSetup(const char *storagepath,
break;
}
+if (secretuuid) {
+if (!virTPMSwtpmSetupCapsGet(
+VIR_TPM_SWTPM_SETUP_FEATURE_CMDARG_PWDFILE_FD)) {
+virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED,
+_("%s does not support passing a passphrase using a file "
+ "descriptor"), virTPMGetSwtpmSetup());
Coverity complains since virTPMGetSwtpm() returns something that needs
to be free'd. I note above that @swtpm_setup is already set - is that
what was meant here?
Right. Will send patch for it. Do you want a single patch or one for
each issue?
+goto cleanup;
+}
+if ((pwdfile_fd = qemuTPMSetupEncryption(secretuuid, cmd)) < 0)
+goto cleanup;
+
+virCommandAddArg(cmd, "--pwdfile-fd");
+virCommandAddArgFormat(cmd, "%d", pwdfile_fd);
+virCommandAddArgList(cmd, "--cipher", "aes-256-cbc", NULL);
+virCommandPassFD(cmd, pwdfile_fd, VIR_COMMAND_PASS_FD_CLOSE_PARENT);
+pwdfile_fd = -1;
+}
virCommandAddArgList(cmd,
"--tpm-state", storagepath,
@@ -502,6 +583,8 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDefPtr tpm,
bool created = false;
char *pidfile;
VIR_AUTOFREE(char *) swtpm = virTPMGetSwtpm();
+VIR_AUTOCLOSE pwdfile_fd = -1;
+const unsigned char *secretuuid = NULL;
if (!swtpm)
return NULL;
@@ -510,10 +593,14 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDefPtr tpm,
, swtpm_user, swtpm_group) < 0)
return NULL;
+if (tpm->data.emulator.hassecretuuid)
+secretuuid = tpm->data.emulator.secretuuid;
+
if (created &&
qemuTPMEmulatorRunSetup(tpm->data.emulator.storagepath, vmname,
vmuuid,
privileged, swtpm_user, swtpm_group,
-tpm->data.emulator.logfile, tpm->version) < 0)
+tpm->data.emulator.logfile, tpm->version,
+secretuuid) < 0)
goto error;
unlink(tpm->data.emulator.source.data.nix.path);
@@ -556,6 +643,25 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDefPtr tpm,
virCommandAddArgFormat(cmd, "file=%s", pidfile);
VIR_FREE(pidfile);
+if (tpm->data.emulator.hassecretuuid) {
+if (!virTPMSwtpmCapsGet(VIR_TPM_SWTPM_FEATURE_CMDARG_PWD_FD)) {
+virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED,
+ _("%s does not support passing passphrase via file
descriptor"),
+ virTPMGetSwtpm());
Same, but @swtpm is used in this context
Gee.
Stefan
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 09/41] remote: conditionalize driver loading in libvirtd daemon
On Tue, 2019-07-23 at 17:02 +0100, Daniel P. Berrangé wrote:
[...]
> +++ b/src/remote/remote_daemon.c
> @@ -303,6 +303,10 @@ static int daemonErrorLogFilter(virErrorPtr err, int
> priority)
>
> static int daemonInitialize(void)
> {
> +#ifdef MODULE_NAME
> +if (virDriverLoadModule(MODULE_NAME, MODULE_NAME "Register", true) < 0)
> +return -1;
Perhaps a short comment along the lines of what you explained in
the commit message would be in order here?
Regardless,
Reviewed-by: Andrea Bolognani
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] test_driver: implement virDomainGetCPUStats
On Fri, Jul 26, 2019 at 01:24:39PM +0200, Ilias Stamatis wrote:
> On Thu, Jul 25, 2019 at 2:01 PM Erik Skultety wrote:
> >
> > On Thu, Jul 18, 2019 at 12:02:43PM +0200, Ilias Stamatis wrote:
> > > Signed-off-by: Ilias Stamatis
> > > ---
> > > src/test/test_driver.c | 131 +
> > > 1 file changed, 131 insertions(+)
> > >
> > > diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> > > index fcb80c9e47..2907c043cb 100644
> > > --- a/src/test/test_driver.c
> > > +++ b/src/test/test_driver.c
> > > @@ -3258,6 +3258,136 @@ static int testDomainSetMetadata(virDomainPtr dom,
> > > return ret;
> > > }
> > >
> > > +
> > > +static int
> > > +testDomainGetDomainTotalCpuStats(virTypedParameterPtr params,
> > > +int nparams)
> > > +{
> > > +if (nparams == 0) /* return supported number of params */
> > > +return 3;
> > > +
> > > +if (virTypedParameterAssign([0], VIR_DOMAIN_CPU_STATS_CPUTIME,
> > > +VIR_TYPED_PARAM_ULLONG, 77102913900) < 0)
> > > +return -1;
> > > +
> > > +if (nparams > 1 &&
> > > +virTypedParameterAssign([1],
> > > +VIR_DOMAIN_CPU_STATS_USERTIME,
> > > +VIR_TYPED_PARAM_ULLONG, 4565000) < 0)
> > > +return -1;
> > > +
> > > +if (nparams > 2 &&
> > > +virTypedParameterAssign([2],
> > > +VIR_DOMAIN_CPU_STATS_SYSTEMTIME,
> > > +VIR_TYPED_PARAM_ULLONG, 1139000) < 0)
> > > +return -1;
> > > +
> > > +if (nparams > 3)
> > > +nparams = 3;
> > > +
> > > +return nparams;
> > > +}
> > > +
> > > +
> > > +static int
> > > +testDomainGetPercpuStats(virTypedParameterPtr params,
> > > + unsigned int nparams,
> > > + int start_cpu,
> > > + unsigned int ncpus,
> > > + int total_cpus)
> > > +{
> > > +size_t i;
> > > +int need_cpus;
> > > +int param_idx;
> > > +int ret = -1;
> > > +
> > > +/* return the number of supported params */
> > > +if (nparams == 0 && ncpus != 0)
> > > +return 2;
> > > +
> > > +/* return total number of cpus */
> > > +if (ncpus == 0) {
> > > +ret = total_cpus;
> > > +goto cleanup;
> > > +}
> > > +
> > > +if (start_cpu >= total_cpus) {
> > > +virReportError(VIR_ERR_INVALID_ARG,
> > > + _("start_cpu %d larger than maximum of %d"),
> > > + start_cpu, total_cpus - 1);
> > > +goto cleanup;
> > > +}
> > > +
> > > +/* return percpu cputime in index 0 */
> > > +param_idx = 0;
> > > +
> > > +/* number of cpus to compute */
> > > +need_cpus = MIN(total_cpus, start_cpu + ncpus);
> > > +
> > > +for (i = 0; i < need_cpus; i++) {
> > > +if (i < start_cpu)
> > > +continue;
> > > +int idx = (i - start_cpu) * nparams + param_idx;
> > > +if (virTypedParameterAssign([idx],
> > > +VIR_DOMAIN_CPU_STATS_CPUTIME,
> > > +VIR_TYPED_PARAM_ULLONG,
> > > +202542145062 + 10 * i) < 0)
> >
> > What's the reasoning behind the formula? I'm curious, wouldn't have
> > 202542145062 + i been enough? Anyhow, the CPUTIME should be a portion of the
> > total cputime of all CPUs, your per-CPU time is much bigger by the total,
> > that doesn't sound right. I don't care about the exact numbers, but I'd
> > like to
> > see them to make sense.
> >
> > Erik
>
> The 10 * i was totally random. You are right, let's make them make sense.
>
> We can have a total CPUTIME and then divide that equally (?) by the
> number of CPUs.
I didn't want to do division really (in an ideal world we'd be okay with bit
shifts, but that's not the case unfortunately), but it's test driver, so
doesn't matter at all.
>
> The "problem" is that the number of CPUs can vary. So depending on the
> number of CPUs the test VM has been configured with, this API will
> return different numbers. Is that ok?
Yes it is, that is expected, it would be weird if a configuration with 4 vCPUs
returns data for 2. CPUTIME needs to be static though.
Erik
>
> Because I thought we would prefer to return the same fixed numbers
> every time. However then they won't make sense when the number of CPUs
> changes.
>
>
> Ilias
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 9/9] qemu: Add -blockdev support for block commit job
On Wed, Jul 24, 2019 at 11:07:36PM +0200, Peter Krempa wrote:
Introduce the handler for finalizing a block commit and active bloc
commit job which will allow to use it with blockdev.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_blockjob.c | 212 ++
src/qemu/qemu_blockjob.h | 18 ++
src/qemu/qemu_domain.c| 32 +++
src/qemu/qemu_driver.c| 50 +++--
.../blockjob-blockdev-in.xml | 15 ++
5 files changed, 311 insertions(+), 16 deletions(-)
diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
index a29af7ec48..4cbdc34b66 100644
--- a/src/qemu/qemu_blockjob.c
+++ b/src/qemu/qemu_blockjob.c
@@ -237,6 +237,43 @@ qemuBlockJobDiskNewPull(virDomainObjPtr vm,
}
+qemuBlockJobDataPtr
+qemuBlockJobDiskNewCommit(virDomainObjPtr vm,
+ virDomainDiskDefPtr disk,
+ virStorageSourcePtr topparent,
+ virStorageSourcePtr top,
+ virStorageSourcePtr base)
+{
+qemuDomainObjPrivatePtr priv = vm->privateData;
+VIR_AUTOUNREF(qemuBlockJobDataPtr) job = NULL;
+VIR_AUTOFREE(char *) jobname = NULL;
+qemuBlockJobType jobtype = QEMU_BLOCKJOB_TYPE_COMMIT;
+
+if (topparent == NULL)
+jobtype = QEMU_BLOCKJOB_TYPE_ACTIVE_COMMIT;
+
+if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV)) {
+if (virAsprintf(, "commit-%s-%s", disk->dst, top->nodeformat)
< 0)
+return NULL;
+} else {
+if (!(jobname = qemuAliasDiskDriveFromDisk(disk)))
+return NULL;
+}
+
+if (!(job = qemuBlockJobDataNew(jobtype, jobname)))
+return NULL;
+
+job->data.commit.topparent = topparent;
/me considers making
World's Top Parent
T-shirts
+job->data.commit.top = top;
+job->data.commit.base = base;
+
+if (qemuBlockJobRegister(job, vm, disk, true) < 0)
+return NULL;
+
+VIR_RETURN_PTR(job);
+}
+
+
/**
* qemuBlockJobDiskRegisterMirror:
* @job: block job to register 'mirror' chain on
@@ -18107,15 +18106,34 @@ qemuDomainBlockCommit(virDomainPtr dom,
* depending on whether the input was specified as relative or
* absolute (that is, our absolute top_canon may do the wrong
* thing if the user specified a relative name). */
+
+if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV)) {
if (blockdev)
+persistjob = true;
+jobname = job->name;
+nodetop = topSource->nodeformat;
+nodebase = baseSource->nodeformat;
+device = disk->src->nodeformat;
+if (!backingPath && top_parent &&
+!(backingPath = qemuBlockGetBackingStoreString(baseSource)))
+goto endjob;
+} else {
+device = job->name;
+}
+
qemuDomainObjEnterMonitor(driver, vm);
-basePath = qemuMonitorDiskNameLookup(priv->mon, device, disk->src,
- baseSource);
-topPath = qemuMonitorDiskNameLookup(priv->mon, device, disk->src,
-topSource);
-if (basePath && topPath)
-ret = qemuMonitorBlockCommit(priv->mon, device, NULL, false,
- topPath, NULL, basePath, NULL,
backingPath,
- speed);
+
+if (!jobname) {
+basePath = qemuMonitorDiskNameLookup(priv->mon, device, disk->src,
+ baseSource);
+topPath = qemuMonitorDiskNameLookup(priv->mon, device, disk->src,
+topSource);
+}
+
+if ((basePath && topPath) || jobname)
This is option a) I proposed in the previous patch.
+ret = qemuMonitorBlockCommit(priv->mon, device, jobname, persistjob,
+ topPath, nodetop, basePath, nodebase,
+ backingPath, speed);
+
Reviewed-by: Ján Tomko
Jano
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 08/41] remote: conditionalize daemon name in libvirtd daemon
On Tue, 2019-07-23 at 17:02 +0100, Daniel P. Berrangé wrote:
[...]
> @@ -895,7 +899,7 @@ daemonUsage(const char *argv0, bool privileged)
> fprintf(stderr, "\n");
>
> fprintf(stderr, "%s:\n", _("Configuration file (unless overridden by
> -f)"));
> -fprintf(stderr, " %s/libvirt/libvirtd.conf\n",
> +fprintf(stderr, " %s/libvirt/" DAEMON_NAME ".conf\n",
> privileged ? SYSCONFDIR : "$XDG_CONFIG_HOME");
Similarly to the previous commit, this should be
fprintf(stderr, " %s/libvirt/%s.conf\n",
DAEMON_NAME, privileged ? SYSCONFDIR : "$XDG_CONFIG_HOME");
[...]
> @@ -922,9 +926,9 @@ daemonUsage(const char *argv0, bool privileged)
>
> fprintf(stderr, "%s:\n",
> _("PID file (unless overridden by -p)"));
> -fprintf(stderr, " %s\n",
> -privileged ? LOCALSTATEDIR "/run/libvirtd.pid":
> -"$XDG_RUNTIME_DIR/libvirt/libvirtd.pid");
> +fprintf(stderr, " %s/\n",
> +privileged ? LOCALSTATEDIR "/run/" DAEMON_NAME ".pid":
> +"$XDG_RUNTIME_DIR/libvirt/" DAEMON_NAME ".pid");
The pattern suggested above and in the previous patch make even more
sense here.
[...]
> +++ b/src/remote/remote_daemon_config.c
> @@ -77,7 +77,8 @@ int
> daemonConfigFilePath(bool privileged, char **configfile)
> {
> if (privileged) {
> -if (VIR_STRDUP(*configfile, SYSCONFDIR "/libvirt/libvirtd.conf") < 0)
> +if (VIR_STRDUP(*configfile,
> + SYSCONFDIR "/libvirt/" DAEMON_NAME ".conf") < 0)
Same here - just use virAsprintf() instead of VIR_STRDUP().
[...]
> @@ -85,7 +86,7 @@ daemonConfigFilePath(bool privileged, char **configfile)
> if (!(configdir = virGetUserConfigDirectory()))
> goto error;
>
> -if (virAsprintf(configfile, "%s/libvirtd.conf", configdir) < 0) {
> +if (virAsprintf(configfile, "%s/%s.conf", configdir, DAEMON_NAME) <
> 0) {
This is what I'm talking about! ;)
[...]
> +++ b/src/remote/remote_driver.h
> @@ -34,7 +34,6 @@ unsigned long remoteVersion(void);
> #define LIBVIRTD_PRIV_UNIX_SOCKET LOCALSTATEDIR "/run/libvirt/libvirt-sock"
> #define LIBVIRTD_PRIV_UNIX_SOCKET_RO LOCALSTATEDIR
> "/run/libvirt/libvirt-sock-ro"
> #define LIBVIRTD_USER_UNIX_SOCKET "libvirt-sock"
> -#define LIBVIRTD_CONFIGURATION_FILE SYSCONFDIR "/libvirt/libvirtd.conf"
Oh, this was unused even before your changes, wasn't it? You should
drop it in a separate, trivial patch.
Going through this patch only strenghtened my convintion that what I
suggested in the previous patch is the way to go, so I urge you to
implement that suggestion; however, for the sake of being coherent,
even if you decide not to go through with it you can still consider
this
Reviewed-by: Andrea Bolognani
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 07/41] remote: conditionalize socket names in libvirtd daemon
On Tue, 2019-07-23 at 17:02 +0100, Daniel P. Berrangé wrote:
[...]
> if (config->unix_sock_dir) {
> -if (virAsprintf(sockfile, "%s/libvirt-sock", config->unix_sock_dir)
> < 0)
> +if (virAsprintf(sockfile, "%s/" SOCK_PREFIX "-sock",
> config->unix_sock_dir) < 0)
> goto cleanup;
Since you're using virAsprintf() already, I'd write this as
virAsprintf(sockfile, "%s/%s-sock", SOCK_PREFIX, config->unix_sock_dir)
instead of using static string concatenation: it looks a bit cleaner
in my opinion.
[...]
> if (privileged) {
> -if (VIR_STRDUP(*sockfile, LOCALSTATEDIR
> "/run/libvirt/libvirt-sock") < 0 ||
> -VIR_STRDUP(*rosockfile, LOCALSTATEDIR
> "/run/libvirt/libvirt-sock-ro") < 0 ||
> -VIR_STRDUP(*admsockfile, LOCALSTATEDIR
> "/run/libvirt/libvirt-admin-sock") < 0)
> +if (VIR_STRDUP(*sockfile, LOCALSTATEDIR "/run/libvirt/"
> SOCK_PREFIX "-sock") < 0 ||
> +VIR_STRDUP(*rosockfile, LOCALSTATEDIR "/run/libvirt/"
> SOCK_PREFIX "-sock-ro") < 0 ||
> +VIR_STRDUP(*admsockfile, LOCALSTATEDIR "/run/libvirt/"
> SOCK_PREFIX "-admin-sock") < 0)
> goto cleanup;
These are not using virAsprintf() but could easily be converted.
[...]
> fprintf(stderr, "%s:\n", _("Sockets"));
> -fprintf(stderr, " %s\n",
> -privileged ? LOCALSTATEDIR "/run/libvirt/libvirt-sock" :
> -"$XDG_RUNTIME_DIR/libvirt/libvirt-sock");
> +fprintf(stderr, " %s/libvirt/" SOCK_PREFIX "-sock\n",
> +privileged ? LOCALSTATEDIR "/run" :
> +"$XDG_RUNTIME_DIR");
All fprintf() calls could be converted as well, except for this one
where the conversion would require adding one extra step and thus is
probably not worth it.
While I think following this proposal would result in slightly
cleaner code, functionally both approaches are identicaly so
Reviewed-by: Andrea Bolognani
regardless of whether or not you decide to implement my suggestion.
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 8/9] qemu: Add -blockdev support for block pull job
On Wed, Jul 24, 2019 at 11:07:35PM +0200, Peter Krempa wrote:
Introduce the handler for finalizing a block pull job which will allow
to use it with blockdev.
This patch also contains some additional machinery which is required to
store all the relevant job data in the status XML which will also be
reused with other block job types.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_blockjob.c | 191 +-
src/qemu/qemu_blockjob.h | 18 ++
src/qemu/qemu_domain.c| 77 +++
src/qemu/qemu_driver.c| 33 ++-
.../blockjob-blockdev-in.xml | 4 +
5 files changed, 313 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
index 0c0ae89f10..a29af7ec48 100644
--- a/src/qemu/qemu_blockjob.c
+++ b/src/qemu/qemu_blockjob.c
+/**
+ * qemuBlockJobProcessEventCompletedPull:
+ * @driver: qemu driver object
+ * @vm: domain object
+ * @job: job data
+ * @asyncJob: qemu asynchronous job type (for monitor interaction)
+ *
+ * This function executes the finalizing steps after a successful block pull
job
+ * (block-stream in qemu terminology. The pull job copies all the data from the
+ * images in the backing chain up to the 'base' image. The 'base' image becomes
+ * the backing store of the active top level image. If 'base' was not used
+ * everything is pulled into the top level image and the top level image will
+ * cease to have backing store. All intermediate images between the active
image
+ * and base image are no longer required and can be unplugged.
+ */
+static void
+qemuBlockJobProcessEventCompletedPull(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ qemuBlockJobDataPtr job,
+ qemuDomainAsyncJob asyncJob)
+{
+virStorageSourcePtr baseparent = NULL;
+virDomainDiskDefPtr cfgdisk = NULL;
+virStorageSourcePtr cfgbase = NULL;
+virStorageSourcePtr cfgbaseparent = NULL;
+virStorageSourcePtr n;
+virStorageSourcePtr tmp;
+
+VIR_DEBUG("pull job '%s' on VM '%s' completed", job->name, vm->def->name);
+
+/* if the job isn't associated with a disk there's nothing to do */
+if (!job->disk)
+return;
+
+if ((cfgdisk = qemuBlockJobGetConfigDisk(vm, job->disk,
job->data.pull.base)))
+cfgbase = cfgdisk->src->backingStore;
+
Consider:
cfgdisk = ...();
if (cfgdisk)
cfgbase = ...;
else
...();
+if (!cfgdisk)
+qemuBlockJobClearConfigChain(vm, job->disk);
+
+/* when pulling if 'base' is right below the top image we don't have to
modify it */
+if (job->disk->src->backingStore == job->data.pull.base)
+return;
+
+if (job->data.pull.base) {
+for (n = job->disk->src->backingStore; n && n != job->data.pull.base; n =
n->backingStore) {
+/* find the image on top of 'base' */
+
+if (cfgbase) {
+cfgbaseparent = cfgbase;
+cfgbase = cfgbase->backingStore;
+}
+
+baseparent = n;
+}
+}
+
+tmp = job->disk->src->backingStore;
+job->disk->src->backingStore = job->data.pull.base;
+if (baseparent)
+baseparent->backingStore = NULL;
+qemuBlockJobEventProcessConcludedRemoveChain(driver, vm, asyncJob, tmp);
+virObjectUnref(tmp);
+
+if (cfgdisk) {
+tmp = cfgdisk->src->backingStore;
You can unref tmp directly here
+cfgdisk->src->backingStore = cfgbase;
+if (cfgbaseparent)
+cfgbaseparent->backingStore = NULL;
+virObjectUnref(tmp);
+}
+}
+
+
static void
qemuBlockJobEventProcessConcludedTransition(qemuBlockJobDataPtr job,
virQEMUDriverPtr driver,
virDomainObjPtr vm,
-qemuDomainAsyncJob asyncJob
ATTRIBUTE_UNUSED)
+qemuDomainAsyncJob asyncJob)
{
switch ((qemuBlockjobState) job->newstate) {
case QEMU_BLOCKJOB_STATE_COMPLETED:
switch ((qemuBlockJobType) job->type) {
case QEMU_BLOCKJOB_TYPE_PULL:
+qemuBlockJobProcessEventCompletedPull(driver, vm, job, asyncJob);
+break;
+
case QEMU_BLOCKJOB_TYPE_COMMIT:
case QEMU_BLOCKJOB_TYPE_ACTIVE_COMMIT:
case QEMU_BLOCKJOB_TYPE_COPY:
diff --git a/src/qemu/qemu_blockjob.h b/src/qemu/qemu_blockjob.h
index 3299207610..d5848fb72c 100644
--- a/src/qemu/qemu_blockjob.h
+++ b/src/qemu/qemu_blockjob.h
@@ -68,6 +68,15 @@ verify((int)QEMU_BLOCKJOB_TYPE_INTERNAL ==
VIR_DOMAIN_BLOCK_JOB_TYPE_LAST);
VIR_ENUM_DECL(qemuBlockjob);
+
+typedef struct _qemuBlockJobPullData qemuBlockJobPullData;
+typedef qemuBlockJobPullData *qemuBlockJobDataPullPtr;
+
+struct _qemuBlockJobPullData {
+virStorageSourcePtr base;
+};
+
+
typedef
Re: [libvirt] [PATCH 06/41] remote: stop trying to print help as giant blocks of text
On Tue, 2019-07-23 at 17:02 +0100, Daniel P. Berrangé wrote:
[...]
> +struct virOptionHelp {
> +const char *opts;
> +const char *help;
> +} opthelp[] = {
> +{ "-h | --help", N_("Display program help") },
> +{ "-v | --verbose", N_("Verbose messages") },
> +{ "-d | --daemon", N_("Run as a daemon & write PID file") },
> +{ "-l | --listen", N_("Listen for TCP/IP connections") },
> +{ "-t | --timeout ", N_("Exit after timeout period") },
> +{ "-f | --config ", N_("Configuration file") },
> +{ "-V | --version", N_("Display version information") },
> +{ "-p | --pid-file ", N_("Change name of PID file") },
> +};
The way you declare the struct at the same time as you use it for a
local variable is highly unusual in our codebase, especially
considering that you do the former inside a function which is, at
least as far as I'm aware, basically unprecedented in libvirt.
Can you please move the struct declaration outside of the function?
[...]
> +fprintf(stderr, "%s:\n", _("TLS"));
> +fprintf(stderr, " %s: %s\n",
> +_("CA certificate"),
> +privileged ? LIBVIRT_CACERT : "$HOME/.pki/libvirt/cacert.pem");
> +fprintf(stderr, " %s: %s\n",
> +_("Server certificate"),
> +privileged ? LIBVIRT_SERVERCERT :
> "$HOME/.pki/libvirt/servercert.pem");
> +fprintf(stderr, " %s: %s\n",
> +_("Server private key"),
> +privileged ? LIBVIRT_SERVERKEY :
> "$HOME/.pki/libvirt/serverkey.pem");
> +fprintf(stderr, "\n");
I think the above would work better if you used
" %-18s %s\n"
as the format string, which would result in
TLS:
CA certificate $HOME/.pki/libvirt/cacert.pem
Server certificate $HOME/.pki/libvirt/servercert.pem
Server private key $HOME/.pki/libvirt/serverkey.pem
instead of
TLS:
CA certificate: $HOME/.pki/libvirt/cacert.pem
Server certificate: $HOME/.pki/libvirt/servercert.pem
Server private key: $HOME/.pki/libvirt/serverkey.pem
as the output. Not a big deal either way, but since we're going out
of our way to align options and their descriptions above it makes
sense to me that we'd do the same here as well.
After this patch, the code looks much less readable, but I understand
why you want to do this and you also get rid of some duplication when
it comes to the system/session daemons, so I'm overall okay with the
change.
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] test_driver: implement virDomainGetCPUStats
On Thu, Jul 25, 2019 at 2:01 PM Erik Skultety wrote:
>
> On Thu, Jul 18, 2019 at 12:02:43PM +0200, Ilias Stamatis wrote:
> > Signed-off-by: Ilias Stamatis
> > ---
> > src/test/test_driver.c | 131 +
> > 1 file changed, 131 insertions(+)
> >
> > diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> > index fcb80c9e47..2907c043cb 100644
> > --- a/src/test/test_driver.c
> > +++ b/src/test/test_driver.c
> > @@ -3258,6 +3258,136 @@ static int testDomainSetMetadata(virDomainPtr dom,
> > return ret;
> > }
> >
> > +
> > +static int
> > +testDomainGetDomainTotalCpuStats(virTypedParameterPtr params,
> > +int nparams)
> > +{
> > +if (nparams == 0) /* return supported number of params */
> > +return 3;
> > +
> > +if (virTypedParameterAssign([0], VIR_DOMAIN_CPU_STATS_CPUTIME,
> > +VIR_TYPED_PARAM_ULLONG, 77102913900) < 0)
> > +return -1;
> > +
> > +if (nparams > 1 &&
> > +virTypedParameterAssign([1],
> > +VIR_DOMAIN_CPU_STATS_USERTIME,
> > +VIR_TYPED_PARAM_ULLONG, 4565000) < 0)
> > +return -1;
> > +
> > +if (nparams > 2 &&
> > +virTypedParameterAssign([2],
> > +VIR_DOMAIN_CPU_STATS_SYSTEMTIME,
> > +VIR_TYPED_PARAM_ULLONG, 1139000) < 0)
> > +return -1;
> > +
> > +if (nparams > 3)
> > +nparams = 3;
> > +
> > +return nparams;
> > +}
> > +
> > +
> > +static int
> > +testDomainGetPercpuStats(virTypedParameterPtr params,
> > + unsigned int nparams,
> > + int start_cpu,
> > + unsigned int ncpus,
> > + int total_cpus)
> > +{
> > +size_t i;
> > +int need_cpus;
> > +int param_idx;
> > +int ret = -1;
> > +
> > +/* return the number of supported params */
> > +if (nparams == 0 && ncpus != 0)
> > +return 2;
> > +
> > +/* return total number of cpus */
> > +if (ncpus == 0) {
> > +ret = total_cpus;
> > +goto cleanup;
> > +}
> > +
> > +if (start_cpu >= total_cpus) {
> > +virReportError(VIR_ERR_INVALID_ARG,
> > + _("start_cpu %d larger than maximum of %d"),
> > + start_cpu, total_cpus - 1);
> > +goto cleanup;
> > +}
> > +
> > +/* return percpu cputime in index 0 */
> > +param_idx = 0;
> > +
> > +/* number of cpus to compute */
> > +need_cpus = MIN(total_cpus, start_cpu + ncpus);
> > +
> > +for (i = 0; i < need_cpus; i++) {
> > +if (i < start_cpu)
> > +continue;
> > +int idx = (i - start_cpu) * nparams + param_idx;
> > +if (virTypedParameterAssign([idx],
> > +VIR_DOMAIN_CPU_STATS_CPUTIME,
> > +VIR_TYPED_PARAM_ULLONG,
> > +202542145062 + 10 * i) < 0)
>
> What's the reasoning behind the formula? I'm curious, wouldn't have
> 202542145062 + i been enough? Anyhow, the CPUTIME should be a portion of the
> total cputime of all CPUs, your per-CPU time is much bigger by the total,
> that doesn't sound right. I don't care about the exact numbers, but I'd like
> to
> see them to make sense.
>
> Erik
The 10 * i was totally random. You are right, let's make them make sense.
We can have a total CPUTIME and then divide that equally (?) by the
number of CPUs.
The "problem" is that the number of CPUs can vary. So depending on the
number of CPUs the test VM has been configured with, this API will
return different numbers. Is that ok?
Because I thought we would prefer to return the same fixed numbers
every time. However then they won't make sense when the number of CPUs
changes.
Ilias
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/2] test_driver: implement emulator pinning APIs
On Tue, Jul 23, 2019 at 01:37:42PM +0200, Ilias Stamatis wrote: > Ilias Stamatis (2): > test_driver: implement virDomainGetEmulatorPinInfo > test_driver: implement virDomainPinEmulator > > src/test/test_driver.c | 92 ++ > 1 file changed, 92 insertions(+) Reviewed-by: Erik Skultety -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 05/41] build: centralize rule for handling generated config files
On Tue, 2019-07-23 at 17:02 +0100, Daniel P. Berrangé wrote: > Instead of adding generated config files to CLEANFILES and BUILT_SOURCES > in each makefile, add them all at once. > > Signed-off-by: Daniel P. Berrangé > --- > src/Makefile.am | 3 +++ > src/locking/Makefile.inc.am | 8 > 2 files changed, 3 insertions(+), 8 deletions(-) Reviewed-by: Andrea Bolognani -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/4] test_driver: implement virDomainGetIOThreadInfo
On Fri, Jul 26, 2019 at 12:55:20PM +0200, Ilias Stamatis wrote:
> On Thu, Jul 25, 2019 at 5:43 PM Erik Skultety wrote:
> >
> > On Tue, Jul 23, 2019 at 12:17:57PM +0200, Ilias Stamatis wrote:
> > > Signed-off-by: Ilias Stamatis
> > > ---
> > > src/test/test_driver.c | 74 ++
> > > 1 file changed, 74 insertions(+)
> > >
> > > diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> > > index 47e28a01ec..4c6f3db8de 100644
> > > --- a/src/test/test_driver.c
> > > +++ b/src/test/test_driver.c
> > > @@ -2774,6 +2774,79 @@ testDomainPinIOThread(virDomainPtr dom,
> > > }
> > >
> > >
> > > +static int
> > > +testDomainGetIOThreadInfo(virDomainPtr dom,
> > > + virDomainIOThreadInfoPtr **info,
> > > + unsigned int flags)
> > > +{
> > > +virDomainObjPtr vm = NULL;
> > > +virDomainDefPtr def = NULL;
> > > +virBitmapPtr cpumask = NULL;
> > > +virBitmapPtr bitmap = NULL;
> > > +virDomainIOThreadInfoPtr *info_ret = NULL;
> > > +size_t i;
> > > +int hostcpus;
> > > +int ret = -1;
> > > +
> > > +virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
> > > + VIR_DOMAIN_AFFECT_CONFIG, -1);
> > > +
> > > +if (!(vm = testDomObjFromDomain(dom)))
> > > +goto cleanup;
> > > +
> > > +if (!(def = virDomainObjGetOneDef(vm, flags)))
> > > +goto cleanup;
> > > +
> > > +if (def->niothreadids == 0)
> > > +return 0;
> >
> > ^This will leave the object locked, so once you do Info, anything else after
> > that will deadlock, so goto is needed.
> >
> > Erik
>
> Of course... I don't know how I missed that. I guess I don't need to
> re-send it though only for this change?
No need, this can go straight in, I still need to look more closely on the
iothread deletion API though.
so for this one:
Reviewed-by: Erik Skultety
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/4] test_driver: implement virDomainGetIOThreadInfo
On Thu, Jul 25, 2019 at 5:43 PM Erik Skultety wrote:
>
> On Tue, Jul 23, 2019 at 12:17:57PM +0200, Ilias Stamatis wrote:
> > Signed-off-by: Ilias Stamatis
> > ---
> > src/test/test_driver.c | 74 ++
> > 1 file changed, 74 insertions(+)
> >
> > diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> > index 47e28a01ec..4c6f3db8de 100644
> > --- a/src/test/test_driver.c
> > +++ b/src/test/test_driver.c
> > @@ -2774,6 +2774,79 @@ testDomainPinIOThread(virDomainPtr dom,
> > }
> >
> >
> > +static int
> > +testDomainGetIOThreadInfo(virDomainPtr dom,
> > + virDomainIOThreadInfoPtr **info,
> > + unsigned int flags)
> > +{
> > +virDomainObjPtr vm = NULL;
> > +virDomainDefPtr def = NULL;
> > +virBitmapPtr cpumask = NULL;
> > +virBitmapPtr bitmap = NULL;
> > +virDomainIOThreadInfoPtr *info_ret = NULL;
> > +size_t i;
> > +int hostcpus;
> > +int ret = -1;
> > +
> > +virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
> > + VIR_DOMAIN_AFFECT_CONFIG, -1);
> > +
> > +if (!(vm = testDomObjFromDomain(dom)))
> > +goto cleanup;
> > +
> > +if (!(def = virDomainObjGetOneDef(vm, flags)))
> > +goto cleanup;
> > +
> > +if (def->niothreadids == 0)
> > +return 0;
>
> ^This will leave the object locked, so once you do Info, anything else after
> that will deadlock, so goto is needed.
>
> Erik
Of course... I don't know how I missed that. I guess I don't need to
re-send it though only for this change?
Ilias
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/4] test_driver: implement virDomainDelIOThread
On Thu, Jul 25, 2019 at 5:47 PM Erik Skultety wrote:
>
> On Tue, Jul 23, 2019 at 12:17:55PM +0200, Ilias Stamatis wrote:
> > Signed-off-by: Ilias Stamatis
> > ---
> > src/test/test_driver.c | 72 ++
> > 1 file changed, 72 insertions(+)
> >
> > diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> > index 313cf5e7ef..29262e4d34 100644
> > --- a/src/test/test_driver.c
> > +++ b/src/test/test_driver.c
> > @@ -2653,6 +2653,77 @@ testDomainAddIOThread(virDomainPtr dom,
> > }
> >
> >
> > +static int
> > +testDomainDelIOThread(virDomainPtr dom,
> > + unsigned int iothread_id,
> > + unsigned int flags)
> > +{
> > +virDomainObjPtr vm = NULL;
> > +virDomainDefPtr def = NULL;
> > +size_t i, j;
> > +int ret = -1;
> > +
> > +virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
> > + VIR_DOMAIN_AFFECT_CONFIG, -1);
> > +
> > +if (iothread_id == 0) {
> > +virReportError(VIR_ERR_INVALID_ARG, "%s",
> > + _("invalid value of 0 for iothread_id"));
> > +return -1;
> > +}
> > +
> > +if (!(vm = testDomObjFromDomain(dom)))
> > +goto cleanup;
> > +
> > +if (!(def = virDomainObjGetOneDef(vm, flags)))
> > +goto cleanup;
> > +
> > +if (!virDomainIOThreadIDFind(def, iothread_id)) {
> > +virReportError(VIR_ERR_INVALID_ARG,
> > + _("cannot find IOThread '%u' in iothreadids list"),
> > + iothread_id);
> > +goto cleanup;
> > +}
> > +
> > +for (i = 0; i < def->ndisks; i++) {
> > +if (def->disks[i]->iothread == iothread_id) {
> > +virReportError(VIR_ERR_INVALID_ARG,
> > + _("cannot remove IOThread %u since it "
> > + "is being used by disk '%s'"),
> > + iothread_id, def->disks[i]->dst);
> > +goto cleanup;
> > +}
> > +}
> > +
> > +for (i = 0; i < def->ncontrollers; i++) {
> > +if (def->controllers[i]->iothread == iothread_id) {
> > +virReportError(VIR_ERR_INVALID_ARG,
> > + _("cannot remove IOThread '%u' since it "
> > + "is being used by controller"),
> > + iothread_id);
> > +goto cleanup;
> > +}
> > +}
> > +
> > +for (i = 0; i < def->niothreadids; i++) {
> > +if (def->iothreadids[i]->iothread_id == iothread_id) {
> > +for (j = i + 1; j < def->niothreadids; j++)
> > +def->iothreadids[j]->autofill = false;
>
> So, I read both the commit and the commentary in the QEMU driver which added
> ^this autofill clearing hunk. I haven't tried with QEMU, but just from reading
> those, I'm still not clear why it's actually needed. Even more so in test
> driver, I tried to remove the nested loop and everything seemed to be working,
> I had half of the thread defined, half of them autofilled, removed from the
> beginning, middle of the list, basically from anywhere and the data that
> libvirt reported were intact, both in the XML and the dedicated API. Right
> now,
> it's magic to me.
>
> Erik
Tbh, I also didn't understand it when I read it but I thought since
it's there let's just copy it to make sure. Maybe somebody that
touched that code could explain? If you checked that there are no
side-effects when removing it then sure let's remove it. From the test
driver at least.
Ilias
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 04/41] build: use a common rule for checking augeas test data files
On Fri, 2019-07-26 at 10:23 +0100, Daniel P. Berrangé wrote: > On Fri, Jul 26, 2019 at 11:18:03AM +0200, Andrea Bolognani wrote: > > On Tue, 2019-07-23 at 17:02 +0100, Daniel P. Berrangé wrote: > > [...] > > > +AUG_TEST_NAMES = $(subst /,-, $(augeastest_DATA)) > > > > > > check-local: check-augeas > > > > > > -check-augeas: $(AUGEAS_DIRS:%=check-augeas-%) > > > +check-augeas: $(AUG_TEST_NAMES:%=check-augeas-%) > > > + > > > +check-augeas-%: $(augeas_DATA) $(augeastest_DATA) > > > + $(AM_V_GEN)export FILE=`echo $* | sed -e 's/.*-//'`; \ > > > + export DIR=`echo $* | sed -e 's/-.*//'`; \ > > > + if test -x '$(AUGPARSE)'; then \ > > > + '$(AUGPARSE)' -I $(srcdir)/$$DIR -I $(builddir)/$$DIR $$DIR/$$FILE; > > > \ > > > + fi > > > > How about we skip the double conversion steps and just do > > > > check-augeas: $(augeas_DATA) $(augeastest_DATA) > > $(AM_V_GEN) \ > > if test -x "$(AUGPARSE)"; then \ > > for f in $(augeastest_DATA); do \ > > DIR=$$(dirname "$$f"); \ > > FILE=$$(basename "$$f"); \ > > "$(AUGPARSE)" \ > > -I "$(srcdir)/$$DIR" -I "$(builddir)/$$DIR" \ > > "$$DIR/$$FILE"; \ > > done; \ > > fi > > .PHONY: check-augeas > > > > instead? As an added bonus, this version avoids doing any work if > > augparse is not available and is correctly marked as PHONY, which > > the rules you're replacing also were. > > This doesn't show any output for the files - I wanted to see the > make output for each file being checked, as its a useful confirmation > that we're actually processing the files we expect to have. That's only a couple small tweaks away: check-augeas: $(augeas_DATA) $(augeastest_DATA) @if test -x "$(AUGPARSE)"; then \ for f in $(augeastest_DATA); do \ DIR=$$(dirname "$$f"); \ FILE=$$(basename "$$f"); \ echo "AUGPARSE $$f"; \ "$(AUGPARSE)" \ -I "$(srcdir)/$$DIR" -I "$(builddir)/$$DIR" \ "$$DIR/$$FILE"; \ done; \ fi .PHONY: check-augeas This version even results in a more accurate output, as we're not really generating the files but rather validating them. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v7 15/19] tpm: Use fd to pass password to swtpm_setup and swtpm
On 7/25/19 2:22 PM, Stefan Berger wrote:
> Allow vTPM state encryption when swtpm_setup and swtpm support
> passing a passphrase using a file descriptor.
>
> This patch enables the encryption of the vTPM state only. It does
> not encrypt the state during migration, so the destination secret
> does not need to have the same password at this point.
>
> Signed-off-by: Stefan Berger
> Reviewed-by: Daniel P. Berrangé
> ---
> src/libvirt_private.syms | 2 +
> src/qemu/qemu_tpm.c | 110 ++-
> src/util/virtpm.c| 16 ++
> src/util/virtpm.h| 3 ++
> 4 files changed, 129 insertions(+), 2 deletions(-)
>
[...]
> /*
> * qemuTPMEmulatorRunSetup
> @@ -387,6 +448,7 @@ qemuTPMEmulatorPrepareHost(virDomainTPMDefPtr tpm,
> * @logfile: The file to write the log into; it must be writable
> * for the user given by userid or 'tss'
> * @tpmversion: The version of the TPM, either a TPM 1.2 or TPM 2
> + * @encryption: pointer to virStorageEncryption holding secret
> *
> * Setup the external swtpm by creating endorsement key and
> * certificates for it.
> @@ -399,7 +461,8 @@ qemuTPMEmulatorRunSetup(const char *storagepath,
> uid_t swtpm_user,
> gid_t swtpm_group,
> const char *logfile,
> -const virDomainTPMVersion tpmversion)
> +const virDomainTPMVersion tpmversion,
> +const unsigned char *secretuuid)
> {
> virCommandPtr cmd = NULL;
> int exitstatus;
> @@ -407,6 +470,7 @@ qemuTPMEmulatorRunSetup(const char *storagepath,
> char uuid[VIR_UUID_STRING_BUFLEN];
> char *vmid = NULL;
> VIR_AUTOFREE(char *)swtpm_setup = virTPMGetSwtpmSetup();
> +VIR_AUTOCLOSE pwdfile_fd = -1;
>
> if (!swtpm_setup)
> return -1;
> @@ -439,6 +503,23 @@ qemuTPMEmulatorRunSetup(const char *storagepath,
> break;
> }
>
> +if (secretuuid) {
> +if (!virTPMSwtpmSetupCapsGet(
> +VIR_TPM_SWTPM_SETUP_FEATURE_CMDARG_PWDFILE_FD)) {
> +virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED,
> +_("%s does not support passing a passphrase using a file "
> + "descriptor"), virTPMGetSwtpmSetup());
Coverity complains since virTPMGetSwtpm() returns something that needs
to be free'd. I note above that @swtpm_setup is already set - is that
what was meant here?
> +goto cleanup;
> +}
> +if ((pwdfile_fd = qemuTPMSetupEncryption(secretuuid, cmd)) < 0)
> +goto cleanup;
> +
> +virCommandAddArg(cmd, "--pwdfile-fd");
> +virCommandAddArgFormat(cmd, "%d", pwdfile_fd);
> +virCommandAddArgList(cmd, "--cipher", "aes-256-cbc", NULL);
> +virCommandPassFD(cmd, pwdfile_fd, VIR_COMMAND_PASS_FD_CLOSE_PARENT);
> +pwdfile_fd = -1;
> +}
>
> virCommandAddArgList(cmd,
> "--tpm-state", storagepath,
> @@ -502,6 +583,8 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDefPtr tpm,
> bool created = false;
> char *pidfile;
> VIR_AUTOFREE(char *) swtpm = virTPMGetSwtpm();
> +VIR_AUTOCLOSE pwdfile_fd = -1;
> +const unsigned char *secretuuid = NULL;
>
> if (!swtpm)
> return NULL;
> @@ -510,10 +593,14 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDefPtr tpm,
> , swtpm_user, swtpm_group) < 0)
> return NULL;
>
> +if (tpm->data.emulator.hassecretuuid)
> +secretuuid = tpm->data.emulator.secretuuid;
> +
> if (created &&
> qemuTPMEmulatorRunSetup(tpm->data.emulator.storagepath, vmname,
> vmuuid,
> privileged, swtpm_user, swtpm_group,
> -tpm->data.emulator.logfile, tpm->version) <
> 0)
> +tpm->data.emulator.logfile, tpm->version,
> +secretuuid) < 0)
> goto error;
>
> unlink(tpm->data.emulator.source.data.nix.path);
> @@ -556,6 +643,25 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDefPtr tpm,
> virCommandAddArgFormat(cmd, "file=%s", pidfile);
> VIR_FREE(pidfile);
>
> +if (tpm->data.emulator.hassecretuuid) {
> +if (!virTPMSwtpmCapsGet(VIR_TPM_SWTPM_FEATURE_CMDARG_PWD_FD)) {
> +virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED,
> + _("%s does not support passing passphrase via file
> descriptor"),
> + virTPMGetSwtpm());
Same, but @swtpm is used in this context
John
> +goto error;
> +}
> +
> +pwdfile_fd = qemuTPMSetupEncryption(tpm->data.emulator.secretuuid,
> cmd);
> +if (pwdfile_fd)
> +goto error;
> +
> +virCommandAddArg(cmd, "--key");
> +virCommandAddArgFormat(cmd, "pwdfd=%d,mode=aes-256-cbc",
> + pwdfile_fd);
> +
Re: [libvirt] [PATCH v7 14/19] tests: Extend command test to transfer large data to process on multiple fds
On 7/25/19 2:22 PM, Stefan Berger wrote:
> Add a test case to commandtest.c to test the transfer of data to a
> process who received the read-end of pipes' file descriptors. Transfer
> large (128 kb) byte streams.
>
> Extend the commandhelper.c with support for --readfd command line
> parameter and convert the data receive loop to use poll and receive data
> on multiple file descriptors (up to 3) and read data into distinct buffers
> that we grow while adding more (string) data.
>
> Signed-off-by: Stefan Berger
> Reviewed-by: Daniel P. Berrangé
> ---
> tests/commandhelper.c | 70 +++---
> tests/commandtest.c | 113 ++
> 2 files changed, 177 insertions(+), 6 deletions(-)
>
[...]
> diff --git a/tests/commandtest.c b/tests/commandtest.c
> index ce0832fb0c..991c0572b0 100644
> --- a/tests/commandtest.c
> +++ b/tests/commandtest.c
> @@ -1139,6 +1139,118 @@ static int test26(const void *unused ATTRIBUTE_UNUSED)
> return ret;
> }
>
> +static int test27(const void *unused ATTRIBUTE_UNUSED)
> +{
> +virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper");
> +int pipe1[2];
> +int pipe2[2];
> +int ret = -1;
> +size_t buflen = 1024 * 128;
> +char *buffer0 = NULL;
> +char *buffer1 = NULL;
> +char *buffer2 = NULL;
> +char *outactual = NULL;
> +char *erractual = NULL;
> +char *outexpect = NULL;
> +# define TEST27_OUTEXPECT_TEMP "BEGIN STDOUT\n" \
> +"%s%s%s" \
> +"END STDOUT\n"
> +char *errexpect = NULL;
> +# define TEST27_ERREXPECT_TEMP "BEGIN STDERR\n" \
> +"%s%s%s" \
> +"END STDERR\n"
> +
> +if (VIR_ALLOC_N(buffer0, buflen) < 0 ||
> +VIR_ALLOC_N(buffer1, buflen) < 0 ||
> +VIR_ALLOC_N(buffer2, buflen) < 0)
> +goto cleanup;
> +
> +memset(buffer0, 'H', buflen - 2);
> +buffer0[buflen - 2] = '\n';
> +buffer0[buflen - 1] = 0;
> +
> +memset(buffer1, '1', buflen - 2);
> +buffer1[buflen - 2] = '\n';
> +buffer1[buflen - 1] = 0;
> +
> +memset(buffer2, '2', buflen - 2);
> +buffer2[buflen - 2] = '\n';
> +buffer2[buflen - 1] = 0;
> +
> +if (virAsprintf(, TEST27_OUTEXPECT_TEMP,
> +buffer0, buffer1, buffer2) < 0 ||
> +virAsprintf(, TEST27_ERREXPECT_TEMP,
> +buffer0, buffer1, buffer2) < 0) {
> +printf("Could not virAsprintf expected output\n");
> +goto cleanup;
> +}
> +
> +if (pipe(pipe1) < 0 || pipe(pipe2) < 0) {
> +printf("Could not create pipe: %s\n", strerror(errno));
> +goto cleanup;
> +}
> +
> +if (virCommandSetSendBuffer(cmd, pipe1[1],
> +(unsigned char *)buffer1, buflen - 1) < 0 ||
> +virCommandSetSendBuffer(cmd, pipe2[1],
> +(unsigned char *)buffer2, buflen - 1) < 0) {
> +printf("Could not set send buffers\n");
> +goto cleanup;
> +}
> +pipe1[1] = 0;
> +pipe2[1] = 0;
> +buffer1 = NULL;
> +buffer2 = NULL;
> +
> +virCommandAddArg(cmd, "--readfd");
> +virCommandAddArgFormat(cmd, "%d", pipe1[0]);
> +virCommandPassFD(cmd, pipe1[0], 0);
> +
> +virCommandAddArg(cmd, "--readfd");
> +virCommandAddArgFormat(cmd, "%d", pipe2[0]);
> +virCommandPassFD(cmd, pipe2[0], 0);
> +
> +virCommandSetInputBuffer(cmd, buffer0);
> +virCommandSetOutputBuffer(cmd, );
> +virCommandSetErrorBuffer(cmd, );
> +
> +if (virCommandRun(cmd, NULL) < 0) {
> +printf("Cannot run child %s\n", virGetLastErrorMessage());
> +goto cleanup;
> +}
> +
> +virCommandFree(cmd);
This should be in the cleanup section; otherwise, Coverity considers
@cmd as leaked for any other path above where @cmd is allocated and we
go to cleanup.
John
> +
> +if (!outactual || !erractual)
> +goto cleanup;
> +
> +if (STRNEQ(outactual, outexpect)) {
> +virTestDifference(stderr, outexpect, outactual);
> +goto cleanup;
> +}
> +if (STRNEQ(erractual, errexpect)) {
> +virTestDifference(stderr, errexpect, erractual);
> +goto cleanup;
> +}
> +
> +ret = 0;
> +
> + cleanup:
> +VIR_FORCE_CLOSE(pipe1[0]);
> +VIR_FORCE_CLOSE(pipe2[0]);
> +VIR_FORCE_CLOSE(pipe1[1]);
> +VIR_FORCE_CLOSE(pipe2[1]);
> +VIR_FREE(buffer0);
> +VIR_FREE(buffer1);
> +VIR_FREE(buffer2);
> +VIR_FREE(outactual);
> +VIR_FREE(erractual);
> +VIR_FREE(outexpect);
> +VIR_FREE(errexpect);
> +
> +return ret;
> +}
> +
> static void virCommandThreadWorker(void *opaque)
> {
> virCommandTestDataPtr test = opaque;
> @@ -1292,6 +1404,7 @@ mymain(void)
> DO_TEST(test23);
> DO_TEST(test25);
> DO_TEST(test26);
> +DO_TEST(test27);
>
> virMutexLock(>lock);
> if (test->running) {
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v7 10/19] utils: Implement function to pass a buffer to send via a fd to virCommand
On 7/25/19 2:22 PM, Stefan Berger wrote:
> Implement virCommandSetSendBuffer() that allows the caller to pass a
> file descriptor and buffer to virCommand. virCommand will write the
> buffer into the file descriptor. That file descriptor could be the
> write end of a pipe or one of the file descriptors of a socketpair.
> The other file descriptor should be passed to the launched process to
> read the data from.
>
> Only implement the function to allocate memory for send buffers
> and to free them later on.
>
> Signed-off-by: Stefan Berger
> Reviewed-by: Daniel P. Berrangé
> ---
> src/libvirt_private.syms | 1 +
> src/util/vircommand.c| 93
> src/util/vircommand.h| 5 +++
> 3 files changed, 99 insertions(+)
>
[...]
> +/**
> + * virCommandSetSendBuffer
> + * @cmd: the command to modify
> + *
> + * Pass a buffer to virCommand that will be written into the
> + * given file descriptor. The buffer will be freed automatically
> + * and the file descriptor closed.
> + */
> +#if defined(F_SETFL)
> +int
> +virCommandSetSendBuffer(virCommandPtr cmd,
> +int fd,
> +unsigned char *buffer, size_t buflen)
> +{
> +size_t i = virCommandGetNumSendBuffers(cmd);
This call would deref @cmd before the following check for !cmd
[1] Was found by Coverity, but see below
> +
> +if (!cmd || cmd->has_error)
> +return -1;
> +
> +if (fcntl(fd, F_SETFL, O_NONBLOCK) < 0) {
> +virReportSystemError(errno, "%s",
> + _("fcntl failed to set O_NONBLOCK"));
> +cmd->has_error = errno;
> +return -1;
> +}
> +
> +if (VIR_REALLOC_N(cmd->sendBuffers, i + 1) < 0) {
> +cmd->has_error = ENOMEM;
> +return -1;
> +}
> +
> +cmd->sendBuffers[i].fd = fd;
> +cmd->sendBuffers[i].buffer = buffer;
> +cmd->sendBuffers[i].buflen = buflen;
> +cmd->sendBuffers[i].offset = 0;
> +
> +cmd->numSendBuffers++;
> +
> +return 0;
> +}
> +
> +#else /* !defined(F_SETFL) */
> +
> +int
> +virCommandSetSendBuffer(virCommandPtr cmd,
> +int fd,
> +unsigned char *buffer, size_t buflen)
> +{
> +if (!cmd || cmd->has_error)
> +return -1;
> +
> +cmd->has_error = ENOTSUP;
> +
> +return -1;
> +}
> +
> +#endif
> +
> /**
> * virCommandSetInputBuffer:
> * @cmd: the command to modify
> @@ -2867,6 +2958,8 @@ virCommandFree(virCommandPtr cmd)
> VIR_FREE(cmd->appArmorProfile);
> #endif
>
> +virCommandFreeSendBuffers(cmd);
> +
> VIR_FREE(cmd);
> }
>
> diff --git a/src/util/vircommand.h b/src/util/vircommand.h
> index 2a9ee5cdc7..c2abc7b2c3 100644
> --- a/src/util/vircommand.h
> +++ b/src/util/vircommand.h
> @@ -146,6 +146,11 @@ void virCommandAddArgList(virCommandPtr cmd,
> void virCommandSetWorkingDirectory(virCommandPtr cmd,
> const char *pwd) ATTRIBUTE_NONNULL(2);
>
> +int virCommandSetSendBuffer(virCommandPtr cmd,
> +int fd,
> +unsigned char *buffer, size_t buflen)
> +ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3);
The ATTRIBUTE_NONNULL(1) causes a Coverity build error when the function
checks !cmd - so either this is removed or the function doesn't check
for !cmd... If going with the latter, then both halves of the "#if
defined(F_SETFL)" would need to be changed.
John
> +
> void virCommandSetInputBuffer(virCommandPtr cmd,
>const char *inbuf) ATTRIBUTE_NONNULL(2);
>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/4] test_driver: implement virDomainPinIOThread
On Tue, Jul 23, 2019 at 12:17:56PM +0200, Ilias Stamatis wrote: > Signed-off-by: Ilias Stamatis > --- Reviewed-by: Erik Skultety -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v7 17/19] tpm: Check TPM XML device configuration changes after edit
On 7/25/19 2:22 PM, Stefan Berger wrote:
> Since swtpm does not support getting started without password
> once it was created with encryption enabled, we don't allow
> encryption to be removed. Similarly, we do not allow encryption
> to be added once swtpm has run. We also prevent chaning the type
> of the TPM backend since the encrypted state is still around and
> the next time one was to switch back to the emulator backend
> and forgot the encryption the TPM would not work.
>
> Signed-off-by: Stefan Berger
> Reviewed-by: Daniel P. Berrangé
> ---
> src/conf/domain_conf.c| 56 +++
> src/conf/domain_conf.h| 4 +++
> src/libvirt_private.syms | 1 +
> src/qemu/qemu_driver.c| 28
> src/qemu/qemu_extdevice.c | 2 +-
> src/qemu/qemu_extdevice.h | 3 +++
> 6 files changed, 93 insertions(+), 1 deletion(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 6673a323c6..d60ef81061 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
[...]
> +
> +int
> +virDomainCheckDeviceChanges(virDomainDefPtr def,
> +virDomainDefPtr newDef)
> +{
> +if (!def || !newDef)
Because !newDef is checked here...
> +return 0;
> +
> +return virDomainCheckTPMChanges(def, newDef);
> +}
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 8092893c2a..285fa6c496 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -3636,3 +3636,7 @@ virDomainGraphicsGetRenderNode(const
> virDomainGraphicsDef *graphics);
>
> bool
> virDomainGraphicsNeedsAutoRenderNode(const virDomainGraphicsDef *graphics);
> +
> +int
> +virDomainCheckDeviceChanges(virDomainDefPtr def, virDomainDefPtr newDef)
> +ATTRIBUTE_NONNULL(2);
This ATTRIBUTE_NONNULL(2) is unnecessary
Causes a Coverity (or whenever STATIC_ANALYSIS is set) build error.
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 8/8] qemu_driver: hook up query-cpu-model-comparison
On 7/25/19 8:26 PM, Collin Walling wrote:
[...]
+
+virCPUCompareResult
+virQEMUCapsCPUModelComparison(virQEMUCapsPtr qemuCaps,
+ const char *libDir,
+ uid_t runUid,
+ gid_t runGid,
+ virCPUDefPtr cpu_a,
+ virCPUDefPtr cpu_b,
+ bool failIncompatible)
+{
+ qemuProcessQMPPtr proc = NULL;
+ qemuMonitorCPUModelInfoPtr result = NULL;
Set ret = VIR_CPU_COMPARE_INCOMPATIBLE
+ int ret = -1;
+
+ if (!(proc = qemuProcessQMPNew(qemuCaps->binary, libDir,
+ runUid, runGid, false)))
+ goto cleanup;
+
+ if (qemuProcessQMPStart(proc) < 0)
+ goto cleanup;
+
+ if (qemuMonitorGetCPUModelComparison(proc->mon, cpu_a->model,
+ cpu_a->nfeatures,
cpu_a->features,
+ cpu_b->model,
cpu_b->nfeatures,
+ cpu_b->features, ) < 0)
+ goto cleanup;
+
+ if (STREQ(result->name, "incompatible") ||
+ STREQ(result->name, "subset"))
+ ret = VIR_CPU_COMPARE_INCOMPATIBLE;
+ else if (STREQ(result->name, "identical"))
+ ret = VIR_CPU_COMPARE_IDENTICAL;
+ else if (STREQ(result->name, "superset"))
+ ret = VIR_CPU_COMPARE_SUPERSET;
and change this:
+
+ if (failIncompatible && ret == VIR_CPU_COMPARE_INCOMPATIBLE) {
+ ret = VIR_CPU_COMPARE_ERROR;
+ virReportError(VIR_ERR_CPU_INCOMPATIBLE, NULL);
+ }
+
+ cleanup:
+ if (ret < 0)
+ virQEMUCapsLogProbeFailure(qemuCaps->binary);
To this:
cleanup:
if (failIncompatible && ret == VIR_CPU_COMPARE_INCOMPATIBLE) {
ret = VIR_CPU_COMPARE_ERROR;
virReportError(VIR_ERR_CPU_INCOMPATIBLE, NULL);
virQEMUCapsLogProbeFailure(qemuCaps->binary);
}
+
+ qemuMonitorCPUModelInfoFree(result);
+ qemuProcessQMPFree(proc);
+ return ret;
+}
And now the output will look like this when the xml contains an
erroneous CPU model or feature:
virsh hypervisor-cpu-compare cpufail.xml
CPU described in cpufail.xml is incompatible with the CPU provided by
hypervisor on the host
virsh hypervisor-cpu-compare cpufail.xml --error
error: Failed to compare hypervisor CPU with cpufail.xml
error: the CPU is incompatible with host CPU
If this output is not acceptable, then perhaps we should further explore
option 2 that I described on patch 5.
[...]
Please see my response in patch 5.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
--
Mit freundlichen Grüßen/Kind regards
Boris Fiuczynski
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Matthias Hartmann
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 5/8] qemu_monitor: implement query-cpu-model-comparison
On 7/25/19 8:26 PM, Collin Walling wrote:
+int
+qemuMonitorJSONGetCPUModelComparison(qemuMonitorPtr mon,
+ const char *model_a_name,
+ int model_a_nprops,
+ virCPUFeatureDefPtr model_a_props,
+ const char *model_b_name,
+ int model_b_nprops,
+ virCPUFeatureDefPtr model_b_props,
+ qemuMonitorCPUModelInfoPtr
*model_result)
+{
+ int ret = -1;
+ virJSONValuePtr model_a;
+ virJSONValuePtr model_b = NULL;
+ virJSONValuePtr cmd = NULL;
+ virJSONValuePtr reply = NULL;
+ virJSONValuePtr data;
+ const char *result_name;
+ virJSONValuePtr props;
+ qemuMonitorCPUModelInfoPtr compare = NULL;
+
+ if (!(model_a = qemuMonitorJSONMakeCPUModel(model_a_name,
model_a_nprops,
+ model_a_props,
true)) ||
+ !(model_b = qemuMonitorJSONMakeCPUModel(model_b_name,
model_b_nprops,
+ model_b_props, true)))
+ goto cleanup;
+
+ if (!(cmd =
qemuMonitorJSONMakeCommand("query-cpu-model-comparison",
+ "a:modela", _a,
+ "a:modelb", _b,
+ NULL)))
+ goto cleanup;
+
+ if (qemuMonitorJSONCommand(mon, cmd, ) < 0)
+ goto cleanup;
+
+ if (qemuMonitorJSONCheckError(cmd, reply) < 0)
Did you test scenarios which contain unknown cpu model name or unknown
features?
This caused errors for me during testing that resulted in e.g.
error : qemuMonitorJSONCheckError:415 : internal error: unable to
execute QEMU command 'query-cpu-model-comparison': Property '.boris'
not found
warning : virQEMUCapsLogProbeFailure:4586 : Failed to probe
capabilities for /usr/bin/qemu-system-s390x: internal error: unable to
execute QEMU command 'query-cpu-model-comparison': Property '.boris'
not found
If I think about a scenario like: I run "virsh domcapabilties" on a
new machine with new cpu model and new cpu features, new libvirt, new
qemu and use "virsh hypervisor-cpu-compare" with the output xml on my
old machine with old cpu , older libvirt and older qemu
I would expect to get as an answer: Incompatible
If my expectation to get incompatible is wrong please correct me but
an "internal error" is not what should occur.
What is the qemu specification for this?
Two routes:
1) Fix up the logic in virQEMUCapsCPUModelComparison
Please see my comments on patch 8.
2) Change the error class in the QEMU QMP code
The QMP code in QEMU currently reports a "GenericError" whenever
something goes wrong (such as an unknown CPU model or feature). A
possible solution to this would be to alter that code to set a specific
error class, have libvirt check for that specific error type in the
JSON, then handle it appropriately to print a cleaner message.
There is one drawback: we would technically be reporting this error
twice -- once in virsh, and another in the libvirt daemon (the internal
error) unless we wanted to suppress it.
This is one solution without too much messy code.
Or you prevent the generic error by not making the call at all in the
case QEMU does not know CPU model or features. To be able to do this you
either need lists of all known CPU models and features (preferable) or
you need to ask QEMU if it knows the CPU model and features by use of
other QMP commands that provide a consumable response.
I would not simply turn an QEMU internal error into an answer
"incompatible" and I am not a fan of parsing thru QEMU internal errors
that turns the answer into "incompatible".
Regarding patch 8: The option you outline in patch 8 returns "CPU is
incompatible" for every QEMU error and not only "CPU model or feature"
unknown. Therefore I think that it is not a good idea.
--
Mit freundlichen Grüßen/Kind regards
Boris Fiuczynski
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Matthias Hartmann
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v7 00/19] Add support for vTPM state encryption
On Thu, Jul 25, 2019 at 02:21:56PM -0400, Stefan Berger wrote: > This series of patches addresses the RFE in BZ 172830: > https://bugzilla.redhat.com/show_bug.cgi?id=1728030 > > This series of patches adds support for vTPM state encryption by passing > the read-end of a pipe's file descriptor to 'swtpm_setup' and 'swtpm' > where they can read a passphrase from and derive a key from that passphrase. > > The TPM's domain XML looks to enable state encryption looks like this: > > > > > > > > The vTPM secret holding the passphrase looks like this: > > > 2c9ceaba-c6ef-4f38-86fd-6e3adb2df5cd > vTPM passphrase example > > vtpm_example > > > > > The swtpm v0.2 is needed that supports the command line option > --print-capabilities returning a JSON object that identifies features added > since v0.1. One such features is the possibility to pass a passphrase via a > file descriptor. > > The patches do some refactoring of existing code on the way. This series is now pushed to GIT, thanks for your work on it 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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v7 08/19] tpm: Check whether previously found executables were updated
On Thu, Jul 25, 2019 at 02:22:04PM -0400, Stefan Berger wrote:
> Check whether previously found executables were updated and if
> so look for them again. This helps to use updated features of
> swtpm and its tools upon updating them.
>
> Signed-off-by: Stefan Berger
> Reviewed-by: Marc-André Lureau
> ---
> src/qemu/qemu_tpm.c | 1 +
> src/util/virtpm.c | 34 ++
> 2 files changed, 35 insertions(+)
>
> +if (!findit &&
> +memcmp(_mtim,
> + [i].stat->st_mtime,
You're comparing st_mtim against st_mtime, but luckily that works
due to the back magic for defining this struct.
More seriously though st_mtim is non-portable so this broke the
Windows build. I've just changed it to st_mtime and done a plain
integer comparison instead of memcmp. There's no compelling reason
for this to use nanosecond precision
> + sizeof(statbuf.st_mtim))) {
> +findit = true;
> +}
> +}
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 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 04/41] build: use a common rule for checking augeas test data files
On Fri, Jul 26, 2019 at 11:18:03AM +0200, Andrea Bolognani wrote: > On Tue, 2019-07-23 at 17:02 +0100, Daniel P. Berrangé wrote: > [...] > > +AUG_TEST_NAMES = $(subst /,-, $(augeastest_DATA)) > > > > check-local: check-augeas > > > > -check-augeas: $(AUGEAS_DIRS:%=check-augeas-%) > > +check-augeas: $(AUG_TEST_NAMES:%=check-augeas-%) > > + > > +check-augeas-%: $(augeas_DATA) $(augeastest_DATA) > > + $(AM_V_GEN)export FILE=`echo $* | sed -e 's/.*-//'`; \ > > + export DIR=`echo $* | sed -e 's/-.*//'`; \ > > + if test -x '$(AUGPARSE)'; then \ > > + '$(AUGPARSE)' -I $(srcdir)/$$DIR -I $(builddir)/$$DIR $$DIR/$$FILE; > > \ > > + fi > > How about we skip the double conversion steps and just do > > check-augeas: $(augeas_DATA) $(augeastest_DATA) > $(AM_V_GEN) \ > if test -x "$(AUGPARSE)"; then \ > for f in $(augeastest_DATA); do \ > DIR=$$(dirname "$$f"); \ > FILE=$$(basename "$$f"); \ > "$(AUGPARSE)" \ > -I "$(srcdir)/$$DIR" -I "$(builddir)/$$DIR" \ > "$$DIR/$$FILE"; \ > done; \ > fi > .PHONY: check-augeas > > instead? As an added bonus, this version avoids doing any work if > augparse is not available and is correctly marked as PHONY, which > the rules you're replacing also were. This doesn't show any output for the files - I wanted to see the make output for each file being checked, as its a useful confirmation that we're actually processing the files we expect to have. > > The rest of the changes look good. > > [...] > > bhyve/test_libvirtd_bhyve.aug: bhyve/test_libvirtd_bhyve.aug.in \ > > $(srcdir)/bhyve/bhyve.conf $(AUG_GENTEST) > > $(AM_V_GEN)$(AUG_GENTEST) $(srcdir)/bhyve/bhyve.conf $< > $@ > > Later on it would be nice to remove duplication for all these rules > as well... I don't think you do it in your series. But it's perfectly > fine not to do it right now, I just though I'd point it out :) > > -- > Andrea Bolognani / Red Hat / Virtualization > 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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 04/41] build: use a common rule for checking augeas test data files
On Tue, 2019-07-23 at 17:02 +0100, Daniel P. Berrangé wrote: [...] > +AUG_TEST_NAMES = $(subst /,-, $(augeastest_DATA)) > > check-local: check-augeas > > -check-augeas: $(AUGEAS_DIRS:%=check-augeas-%) > +check-augeas: $(AUG_TEST_NAMES:%=check-augeas-%) > + > +check-augeas-%: $(augeas_DATA) $(augeastest_DATA) > + $(AM_V_GEN)export FILE=`echo $* | sed -e 's/.*-//'`; \ > + export DIR=`echo $* | sed -e 's/-.*//'`; \ > + if test -x '$(AUGPARSE)'; then \ > + '$(AUGPARSE)' -I $(srcdir)/$$DIR -I $(builddir)/$$DIR $$DIR/$$FILE; > \ > + fi How about we skip the double conversion steps and just do check-augeas: $(augeas_DATA) $(augeastest_DATA) $(AM_V_GEN) \ if test -x "$(AUGPARSE)"; then \ for f in $(augeastest_DATA); do \ DIR=$$(dirname "$$f"); \ FILE=$$(basename "$$f"); \ "$(AUGPARSE)" \ -I "$(srcdir)/$$DIR" -I "$(builddir)/$$DIR" \ "$$DIR/$$FILE"; \ done; \ fi .PHONY: check-augeas instead? As an added bonus, this version avoids doing any work if augparse is not available and is correctly marked as PHONY, which the rules you're replacing also were. The rest of the changes look good. [...] > bhyve/test_libvirtd_bhyve.aug: bhyve/test_libvirtd_bhyve.aug.in \ > $(srcdir)/bhyve/bhyve.conf $(AUG_GENTEST) > $(AM_V_GEN)$(AUG_GENTEST) $(srcdir)/bhyve/bhyve.conf $< > $@ Later on it would be nice to remove duplication for all these rules as well... I don't think you do it in your series. But it's perfectly fine not to do it right now, I just though I'd point it out :) -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
