[PATCH 0/3] Couple of almost trivial patches
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
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@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
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@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
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@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
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@redhat.com> Jano
participants (2)
-
Ján Tomko -
Michal Privoznik