9:51 a.m.
While reviewing Tim's patches [1] I've found couple of places in CH
driver that are downright bugs. Fix them.
1: https://listman.redhat.com/archives/libvir-list/2022-February/msg00226.html
Michal Prívozník (6):
ch_driver: Don't lock driver when getting version
ch_driver: Don't lock driver when looking up domains
chDomainCreateXML: Drop spurious driver unlock
ch_driver: End job properly on failed chDomainCreateXML()
ch_driver: Introduce and use virCHDomainRemoveInactive()
ch_process: Check whether domain is already running before starting it
src/ch/ch_domain.c | 9 +++++++++
src/ch/ch_domain.h | 4 ++++
src/ch/ch_driver.c | 32 +++++++++++---------------------
src/ch/ch_process.c | 6 ++++++
4 files changed, 30 insertions(+), 21 deletions(-)
--
2.34.1
9:51 a.m.
New subject: [PATCH 1/6] ch_driver: Don't lock driver when getting version
In chConnectGetVersion() the CH driver is locked in order to read
driver->version. This is needless, because not only is the
version set with driver unlocked (chStateInitialize() calls
chExtractVersion() which sets the version), but the version is
practically immutable. Once driver initialized itself it's never
changed.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/ch/ch_driver.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c
index e751ddba19..7c3b45ca2f 100644
--- a/src/ch/ch_driver.c
+++ b/src/ch/ch_driver.c
@@ -106,9 +106,7 @@ static int chConnectGetVersion(virConnectPtr conn,
if (virConnectGetVersionEnsureACL(conn) < 0)
return -1;
- chDriverLock(driver);
*version = driver->version;
- chDriverUnlock(driver);
return 0;
}
--
2.34.1
9:51 a.m.
New subject: [PATCH 2/6] ch_driver: Don't lock driver when looking up domains
There is no need to lock whole driver when accessing
virDomainObjList. Those APIs were specifically tailored to be
thread safe (when we were dropping QEMU driver lock). Don't
resurrect old history.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/ch/ch_driver.c | 9 ---------
1 file changed, 9 deletions(-)
diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c
index 7c3b45ca2f..2ed33c3446 100644
--- a/src/ch/ch_driver.c
+++ b/src/ch/ch_driver.c
@@ -363,11 +363,9 @@ chDomainUndefine(virDomainPtr dom)
static int chDomainIsActive(virDomainPtr dom)
{
- virCHDriver *driver = dom->conn->privateData;
virDomainObj *vm;
int ret = -1;
- chDriverLock(driver);
if (!(vm = virCHDomainObjFromDomain(dom)))
goto cleanup;
@@ -378,7 +376,6 @@ static int chDomainIsActive(virDomainPtr dom)
cleanup:
virDomainObjEndAPI(&vm);
- chDriverUnlock(driver);
return ret;
}
@@ -636,9 +633,7 @@ static virDomainPtr chDomainLookupByID(virConnectPtr conn,
virDomainObj *vm;
virDomainPtr dom = NULL;
- chDriverLock(driver);
vm = virDomainObjListFindByID(driver->domains, id);
- chDriverUnlock(driver);
if (!vm) {
virReportError(VIR_ERR_NO_DOMAIN,
@@ -663,9 +658,7 @@ static virDomainPtr chDomainLookupByName(virConnectPtr conn,
virDomainObj *vm;
virDomainPtr dom = NULL;
- chDriverLock(driver);
vm = virDomainObjListFindByName(driver->domains, name);
- chDriverUnlock(driver);
if (!vm) {
virReportError(VIR_ERR_NO_DOMAIN,
@@ -690,9 +683,7 @@ static virDomainPtr chDomainLookupByUUID(virConnectPtr conn,
virDomainObj *vm;
virDomainPtr dom = NULL;
- chDriverLock(driver);
vm = virDomainObjListFindByUUID(driver->domains, uuid);
- chDriverUnlock(driver);
if (!vm) {
char uuidstr[VIR_UUID_STRING_BUFLEN];
--
2.34.1
9:51 a.m.
New subject: [PATCH 3/6] chDomainCreateXML: Drop spurious driver unlock
Inside chDomainCreateXML(), towards the end, the driver is
unlocked even though there is no corresponding driver lock call
before that. Drop it.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/ch/ch_driver.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c
index 2ed33c3446..b023f7e3d3 100644
--- a/src/ch/ch_driver.c
+++ b/src/ch/ch_driver.c
@@ -239,7 +239,6 @@ chDomainCreateXML(virConnectPtr conn,
virDomainObjListRemove(driver->domains, vm);
}
virDomainObjEndAPI(&vm);
- chDriverUnlock(driver);
return dom;
}
--
2.34.1
9:51 a.m.
New subject: [PATCH 4/6] ch_driver: End job properly on failed chDomainCreateXML()
When creating a domain failed, then the virCHDomainObjEndJob()
would be jumped over. Fix this by creating enjob label and fixing
one goto.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/ch/ch_driver.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c
index b023f7e3d3..cd156a222b 100644
--- a/src/ch/ch_driver.c
+++ b/src/ch/ch_driver.c
@@ -228,10 +228,11 @@ chDomainCreateXML(virConnectPtr conn,
goto cleanup;
if (virCHProcessStart(driver, vm, VIR_DOMAIN_RUNNING_BOOTED) < 0)
- goto cleanup;
+ goto endjob;
dom = virGetDomain(conn, vm->def->name, vm->def->uuid,
vm->def->id);
+ endjob:
virCHDomainObjEndJob(vm);
cleanup:
--
2.34.1
9:51 a.m.
New subject: [PATCH 5/6] ch_driver: Introduce and use virCHDomainRemoveInactive()
There are few places where a call to virDomainObjListRemove() is
guarded with !vm->persistent check. And there are some places
which are missing this check completely (leading us to losing a
domain). To prevent such mistakes introduce
virCHDomainRemoveInactive() which does the check for us. Also
replace all occurrences of virDomainObjListRemove() with the call
to the new function.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/ch/ch_domain.c | 9 +++++++++
src/ch/ch_domain.h | 4 ++++
src/ch/ch_driver.c | 17 +++++++++--------
3 files changed, 22 insertions(+), 8 deletions(-)
diff --git a/src/ch/ch_domain.c b/src/ch/ch_domain.c
index 0b4dbbf142..25f581c1c3 100644
--- a/src/ch/ch_domain.c
+++ b/src/ch/ch_domain.c
@@ -136,6 +136,15 @@ virCHDomainObjEndJob(virDomainObj *obj)
virCondSignal(&priv->job.cond);
}
+void
+virCHDomainRemoveInactive(virCHDriver *driver,
+ virDomainObj *vm)
+{
+ if (vm->persistent) {
+ virDomainObjListRemove(driver->domains, vm);
+ }
+}
+
static void *
virCHDomainObjPrivateAlloc(void *opaque)
{
diff --git a/src/ch/ch_domain.h b/src/ch/ch_domain.h
index c1d3be212e..11a20a874a 100644
--- a/src/ch/ch_domain.h
+++ b/src/ch/ch_domain.h
@@ -88,6 +88,10 @@ virCHDomainObjBeginJob(virDomainObj *obj, enum virCHDomainJob job)
void
virCHDomainObjEndJob(virDomainObj *obj);
+void
+virCHDomainRemoveInactive(virCHDriver *driver,
+ virDomainObj *vm);
+
int
virCHDomainRefreshThreadInfo(virDomainObj *vm);
diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c
index cd156a222b..ef74a00bf7 100644
--- a/src/ch/ch_driver.c
+++ b/src/ch/ch_driver.c
@@ -237,7 +237,7 @@ chDomainCreateXML(virConnectPtr conn,
cleanup:
if (vm && !dom) {
- virDomainObjListRemove(driver->domains, vm);
+ virCHDomainRemoveInactive(driver, vm);
}
virDomainObjEndAPI(&vm);
return dom;
@@ -342,10 +342,9 @@ chDomainUndefineFlags(virDomainPtr dom,
goto cleanup;
}
- if (virDomainObjIsActive(vm)) {
- vm->persistent = 0;
- } else {
- virDomainObjListRemove(driver->domains, vm);
+ vm->persistent = 0;
+ if (!virDomainObjIsActive(vm)) {
+ virCHDomainRemoveInactive(driver, vm);
}
ret = 0;
@@ -608,12 +607,14 @@ chDomainDestroyFlags(virDomainPtr dom, unsigned int flags)
if (virDomainObjCheckActive(vm) < 0)
goto endjob;
- ret = virCHProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_DESTROYED);
+ if (virCHProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_DESTROYED) < 0)
+ goto endjob;
+
+ virCHDomainRemoveInactive(driver, vm);
+ ret = 0;
endjob:
virCHDomainObjEndJob(vm);
- if (!vm->persistent)
- virDomainObjListRemove(driver->domains, vm);
cleanup:
virDomainObjEndAPI(&vm);
--
2.34.1
9:51 a.m.
New subject: [PATCH 6/6] ch_process: Check whether domain is already running before starting it
There are two places where a domain can be started in CH driver:
chDomainCreateXML() and chDomainCreateWithFlags(). Both acquire a
job (good), but neither of them checks whether the domain isn't
already running. This is wrong. Fortunately, both function call
the very same virCHProcessStart() rendering it the best place for
such check.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/ch/ch_process.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/src/ch/ch_process.c b/src/ch/ch_process.c
index 6d9a286e8a..00d94ddcbe 100644
--- a/src/ch/ch_process.c
+++ b/src/ch/ch_process.c
@@ -470,6 +470,12 @@ virCHProcessStart(virCHDriver *driver,
g_autofree int *nicindexes = NULL;
size_t nnicindexes = 0;
+ if (virDomainObjIsActive(vm)) {
+ virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("VM is already active"));
+ return -1;
+ }
+
if (!priv->monitor) {
/* And we can get the first monitor connection now too */
if (!(priv->monitor = virCHProcessConnectMonitor(driver, vm))) {
--
2.34.1
2:32 a.m.
On Thu, 2022-02-10 at 16:51 +0100, Michal Privoznik wrote:
While reviewing Tim's patches [1] I've found couple of places
in CH
driver that are downright bugs. Fix them.
1:
https://listman.redhat.com/archives/libvir-list/2022-February/msg00226.html
Michal Prívozník (6):
ch_driver: Don't lock driver when getting version
ch_driver: Don't lock driver when looking up domains
chDomainCreateXML: Drop spurious driver unlock
ch_driver: End job properly on failed chDomainCreateXML()
ch_driver: Introduce and use virCHDomainRemoveInactive()
ch_process: Check whether domain is already running before starting
it
src/ch/ch_domain.c | 9 +++++++++
src/ch/ch_domain.h | 4 ++++
src/ch/ch_driver.c | 32 +++++++++++---------------------
src/ch/ch_process.c | 6 ++++++
4 files changed, 30 insertions(+), 21 deletions(-)
Reviewed-by: Tim Wiederhake <twiederh(a)redhat.com>
1051
days inactive
1052
days old
7 comments
2 participants
participants (2)
-
Michal Privoznik -
Tim Wiederhake