5:02 a.m.
These stem out from my review of Marc-André's patches:
https://www.redhat.com/archives/libvir-list/2020-January/msg00648.html
Michal Prívozník (3):
virpidfile: Set correct retval in virPidFileReadPath()
qemu: Don't explicitly remove pidfile after
virPidFileForceCleanupPath()
qemu_migration: Rearrange some checks in qemuMigrationSrcIsAllowed()
src/qemu/qemu_migration.c | 66 +++++++++++++++++-----------------
src/qemu/qemu_process.c | 9 +----
src/qemu/qemu_vhost_user_gpu.c | 10 +-----
src/util/virpidfile.c | 2 +-
4 files changed, 36 insertions(+), 51 deletions(-)
--
2.24.1
5:02 a.m.
New subject: [PATCH 1/3] virpidfile: Set correct retval in virPidFileReadPath()
The virPidFileReadPath() function is supposed to return 0 on
success or a negative value on failure. But the negative value
has a special meaning - it's negated errno. Therefore, when
converting string to int we shouldn't return -1 which translates
to EPERM. Returning EINVAL looks closer to the truth.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/virpidfile.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/virpidfile.c b/src/util/virpidfile.c
index a8a743504d..d5aa5f4f84 100644
--- a/src/util/virpidfile.c
+++ b/src/util/virpidfile.c
@@ -130,7 +130,7 @@ int virPidFileReadPath(const char *path,
if (virStrToLong_ll(pidstr, &endptr, 10, &pid_value) < 0 ||
!(*endptr == '\0' || g_ascii_isspace(*endptr)) ||
(pid_t) pid_value != pid_value) {
- rc = -1;
+ rc = -EINVAL;
goto cleanup;
}
--
2.24.1
5:02 a.m.
New subject: [PATCH 2/3] qemu: Don't explicitly remove pidfile after virPidFileForceCleanupPath()
In two places where virPidFileForceCleanupPath() is called, we
try to unlink() the pidfile again. This is needless because
virPidFileForceCleanupPath() has done just that.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_process.c | 9 +--------
src/qemu/qemu_vhost_user_gpu.c | 10 +---------
2 files changed, 2 insertions(+), 17 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index bf987a3bc3..8c1ed76677 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -2802,14 +2802,7 @@ qemuProcessKillManagedPRDaemon(virDomainObjPtr vm)
if (virPidFileForceCleanupPath(pidfile) < 0) {
VIR_WARN("Unable to kill pr-helper process");
} else {
- if (unlink(pidfile) < 0 &&
- errno != ENOENT) {
- virReportSystemError(errno,
- _("Unable to remove stale pidfile %s"),
- pidfile);
- } else {
- priv->prDaemonRunning = false;
- }
+ priv->prDaemonRunning = false;
}
virErrorRestore(&orig_err);
}
diff --git a/src/qemu/qemu_vhost_user_gpu.c b/src/qemu/qemu_vhost_user_gpu.c
index 51244f9b35..ae1f530338 100644
--- a/src/qemu/qemu_vhost_user_gpu.c
+++ b/src/qemu/qemu_vhost_user_gpu.c
@@ -223,16 +223,8 @@ void qemuExtVhostUserGPUStop(virQEMUDriverPtr driver,
}
virErrorPreserveLast(&orig_err);
- if (virPidFileForceCleanupPath(pidfile) < 0) {
+ if (virPidFileForceCleanupPath(pidfile) < 0)
VIR_WARN("Unable to kill vhost-user-gpu process");
- } else {
- if (unlink(pidfile) < 0 &&
- errno != ENOENT) {
- virReportSystemError(errno,
- _("Unable to remove stale pidfile %s"),
- pidfile);
- }
- }
virErrorRestore(&orig_err);
}
--
2.24.1
5:02 a.m.
New subject: [PATCH 3/3] qemu_migration: Rearrange some checks in qemuMigrationSrcIsAllowed()
Firstly, the check for disk I/O error can be moved into 'if
(!offline)' section a few lines below.
Secondly, checks for vmstate and slirp should be moved under the
same section because they reflect live state of a domain. For
offline migration no QEMU is involved and thus these restrictions
are not valid.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_migration.c | 66 +++++++++++++++++++--------------------
1 file changed, 33 insertions(+), 33 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index ceac81c960..a307c5ebe2 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -1185,43 +1185,25 @@ qemuMigrationSrcIsAllowed(virQEMUDriverPtr driver,
nsnapshots);
return false;
}
-
- /* cancel migration if disk I/O error is emitted while migrating */
- if (flags & VIR_MIGRATE_ABORT_ON_ERROR &&
- !(flags & VIR_MIGRATE_OFFLINE) &&
- virDomainObjGetState(vm, &pauseReason) == VIR_DOMAIN_PAUSED &&
- pauseReason == VIR_DOMAIN_PAUSED_IOERROR) {
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("cannot migrate domain with I/O error"));
- return false;
- }
- }
-
- if (virHashSize(priv->dbusVMStates) > 0 &&
- !virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DBUS_VMSTATE)) {
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("domain requires dbus-vmstate support"));
- return false;
- }
-
- for (i = 0; i < vm->def->nnets; i++) {
- virDomainNetDefPtr net = vm->def->nets[i];
- qemuSlirpPtr slirp = QEMU_DOMAIN_NETWORK_PRIVATE(net)->slirp;
-
- if (slirp && !qemuSlirpHasFeature(slirp, QEMU_SLIRP_FEATURE_MIGRATE)) {
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("a slirp-helper cannot be migrated"));
- return false;
- }
}
/* following checks don't make sense for offline migration */
if (!(flags & VIR_MIGRATE_OFFLINE)) {
- if (remote &&
- qemuProcessAutoDestroyActive(driver, vm)) {
- virReportError(VIR_ERR_OPERATION_INVALID,
- "%s", _("domain is marked for auto
destroy"));
- return false;
+ if (remote) {
+ /* cancel migration if disk I/O error is emitted while migrating */
+ if (flags & VIR_MIGRATE_ABORT_ON_ERROR &&
+ virDomainObjGetState(vm, &pauseReason) == VIR_DOMAIN_PAUSED
&&
+ pauseReason == VIR_DOMAIN_PAUSED_IOERROR) {
+ virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("cannot migrate domain with I/O error"));
+ return false;
+ }
+
+ if (qemuProcessAutoDestroyActive(driver, vm)) {
+ virReportError(VIR_ERR_OPERATION_INVALID,
+ "%s", _("domain is marked for auto
destroy"));
+ return false;
+ }
}
@@ -1280,6 +1262,24 @@ qemuMigrationSrcIsAllowed(virQEMUDriverPtr driver,
_("migration with shmem device is not supported"));
return false;
}
+
+ if (virHashSize(priv->dbusVMStates) > 0 &&
+ !virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DBUS_VMSTATE)) {
+ virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("domain requires dbus-vmstate support"));
+ return false;
+ }
+
+ for (i = 0; i < vm->def->nnets; i++) {
+ virDomainNetDefPtr net = vm->def->nets[i];
+ qemuSlirpPtr slirp = QEMU_DOMAIN_NETWORK_PRIVATE(net)->slirp;
+
+ if (slirp && !qemuSlirpHasFeature(slirp, QEMU_SLIRP_FEATURE_MIGRATE))
{
+ virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("a slirp-helper cannot be migrated"));
+ return false;
+ }
+ }
}
return true;
--
2.24.1
5:14 a.m.
On Thu, Feb 20, 2020 at 12:02:34PM +0100, Michal Privoznik wrote:
These stem out from my review of Marc-André's patches:
https://www.redhat.com/archives/libvir-list/2020-January/msg00648.html
Michal Prívozník (3):
virpidfile: Set correct retval in virPidFileReadPath()
qemu: Don't explicitly remove pidfile after
virPidFileForceCleanupPath()
qemu_migration: Rearrange some checks in qemuMigrationSrcIsAllowed()
src/qemu/qemu_migration.c | 66 +++++++++++++++++-----------------
src/qemu/qemu_process.c | 9 +----
src/qemu/qemu_vhost_user_gpu.c | 10 +-----
src/util/virpidfile.c | 2 +-
4 files changed, 36 insertions(+), 51 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
1767
days inactive
1767
days old
4 comments
2 participants
participants (2)
-
Ján Tomko -
Michal Privoznik