Note: This bug is displayed in read-only format because the product is no longer active in Red Hat Bugzilla.

Bug 1589627

Summary:	Improve image locking error handling
Product:	Red Hat Enterprise Linux Advanced Virtualization	Reporter:	Fam Zheng <famz>
Component:	qemu-kvm	Assignee:	Hanna Czenczek <hreitz>
Status:	CLOSED DUPLICATE	QA Contact:	Tingting Mao <timao>
Severity:	high	Docs Contact:
Priority:	high
Version:	---	CC:	aliang, areis, coli, guillaume.pavese, gveitmic, lolyu, mkalinin, ngu, obockows, pingl, rbalakri, timao, virt-maint, xuwei
Target Milestone:	rc
Target Release:	---
Hardware:	Unspecified
OS:	Unspecified
Whiteboard:
Fixed In Version:		Doc Type:	If docs needed, set a value
Doc Text:		Story Points:	---
Clone Of:		Environment:
Last Closed:	2019-06-14 02:22:51 UTC	Type:	Bug
Regression:	---	Mount Type:	---
Documentation:	---	CRM:
Verified Versions:		Category:	---
oVirt Team:	---	RHEL 7.3 requirements from Atomic Host:
Cloudforms Team:	---	Target Upstream Version:
Embargoed:

Description Fam Zheng 2018-06-11 02:16:46 UTC

QEMU code uses "abort()" in certain cases where image locking acquisition options are not expected to fail. This is risky. One example is bug 1588873. Another is 1584982. Although when the abort happens, it is often a misbehave of some other parts of the system rather than QEMU itself, effectively asserting about the external resources is not robust program logic.

Specifically, the code in question is usually written as:

  blk_set_perm(blk, 0, BLK_PERM_ALL, &error_abort);

where error_abort should be replaced by local_err and error_report().

Comment 2 Fam Zheng 2018-06-11 02:18:43 UTC

Also, it's fairly easy to avoid the locked bits re-acquire waste as described in bug 1588873, like this:

commit 1986f53e8958cd8e893d5658f9cb7209551cf98b (image-lock-skip-bits)
Author: Fam Zheng <famz>
Date:   Tue Jun 5 13:11:36 2018 +0800

    file-posix: Skip already locked/unlocked bits

    Signed-off-by: Fam Zheng <famz>

diff --git a/block/file-posix.c b/block/file-posix.c
index 5a602cfe37..5cc9caaa23 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -650,13 +650,14 @@ static int raw_apply_lock_bytes(BDRVRawState *s,

     PERM_FOREACH(i) {
         int off = RAW_LOCK_PERM_BASE + i;
-        if (perm_lock_bits & (1ULL << i)) {
+        uint64_t bit = (1ULL << i);
+        if ((perm_lock_bits & bit) && !(s->perm & bit)) {
             ret = qemu_lock_fd(s->lock_fd, off, 1, false);
             if (ret) {
                 error_setg(errp, "Failed to lock byte %d", off);
                 return ret;
             }
-        } else if (unlock) {
+        } else if (unlock && (s->perm & bit)) {
             ret = qemu_unlock_fd(s->lock_fd, off, 1);
             if (ret) {
                 error_setg(errp, "Failed to unlock byte %d", off);
@@ -666,13 +667,14 @@ static int raw_apply_lock_bytes(BDRVRawState *s,
     }
     PERM_FOREACH(i) {
         int off = RAW_LOCK_SHARED_BASE + i;
-        if (shared_perm_lock_bits & (1ULL << i)) {
+        uint64_t bit = (1ULL << i);
+        if ((shared_perm_lock_bits & bit) && !(s->shared_perm & bit)) {
             ret = qemu_lock_fd(s->lock_fd, off, 1, false);
             if (ret) {
                 error_setg(errp, "Failed to lock byte %d", off);
                 return ret;
             }
-        } else if (unlock) {
+        } else if (unlock && (s->shared_perm & bit)) {
             ret = qemu_unlock_fd(s->lock_fd, off, 1);
             if (ret) {
                 error_setg(errp, "Failed to unlock byte %d", off);

Comment 4 Ademar Reis 2018-06-11 20:26:12 UTC

See also Bug 1526313