3:40 p.m.
On 08/27/2018 04:08 AM, Michal Privoznik wrote:
We will want virtlockd to lock files on behalf of libvirtd and
not qemu process, because it is libvirtd that needs an exclusive
access not qemu. This requires new lock context.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/locking/lock_driver.h | 2 +
src/locking/lock_driver_lockd.c | 110 +++++++++++++++++++++++++++++++-------
src/locking/lock_driver_sanlock.c | 37 ++++++++-----
3 files changed, 117 insertions(+), 32 deletions(-)
Caveat to my comments - I didn't read all the conversations in the
previous series... So if using unions was something agreed upon, then
mea culpa for being too lazy to go read ;-)
[...]
diff --git a/src/locking/lock_driver_lockd.c
b/src/locking/lock_driver_lockd.c
index ca825e6026..8ca0cf5426 100644
--- a/src/locking/lock_driver_lockd.c
+++ b/src/locking/lock_driver_lockd.c
@@ -56,10 +56,21 @@ struct _virLockManagerLockDaemonResource {
};
struct _virLockManagerLockDaemonPrivate {
- unsigned char uuid[VIR_UUID_BUFLEN];
- char *name;
- int id;
- pid_t pid;
+ virLockManagerObjectType type;
+ union {
+ struct {
+ unsigned char uuid[VIR_UUID_BUFLEN];
+ char *name;
+ int id;
+ pid_t pid;
+ } dom;
+
+ struct {
+ unsigned char uuid[VIR_UUID_BUFLEN];
+ char *name;
+ pid_t pid;
+ } daemon;
+ } t;
Something tells me it'd be better if @dom and @daemon were typedef'd types.
Still unless the lock can be shared why separate things? An @id == -1
could signify a lock using @uuid, @name, and @pid is not a @dom lock.
using @type is fine as well.
I see nothing by the end of the series adding a new type and since the
members essentially overlap, it's really not clear why a union should be
used.
size_t nresources;
virLockManagerLockDaemonResourcePtr resources;
@@ -156,10 +167,24 @@ virLockManagerLockDaemonConnectionRegister(virLockManagerPtr lock,
[...]
@@ -420,46 +456,82 @@ static int
virLockManagerLockDaemonNew(virLockManagerPtr lock,
if (VIR_ALLOC(priv) < 0)
return -1;
- switch (type) {
+ priv->type = type;
+
+ switch ((virLockManagerObjectType) type) {
case VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN:
for (i = 0; i < nparams; i++) {
if (STREQ(params[i].key, "uuid")) {
- memcpy(priv->uuid, params[i].value.uuid, VIR_UUID_BUFLEN);
+ memcpy(priv->t.dom.uuid, params[i].value.uuid, VIR_UUID_BUFLEN);
} else if (STREQ(params[i].key, "name")) {
- if (VIR_STRDUP(priv->name, params[i].value.str) < 0)
+ if (VIR_STRDUP(priv->t.dom.name, params[i].value.str) < 0)
goto cleanup;
} else if (STREQ(params[i].key, "id")) {
- priv->id = params[i].value.iv;
+ priv->t.dom.id = params[i].value.iv;
} else if (STREQ(params[i].key, "pid")) {
- priv->pid = params[i].value.iv;
+ priv->t.dom.pid = params[i].value.iv;
} else if (STREQ(params[i].key, "uri")) {
/* ignored */
} else {
virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Unexpected parameter %s for object"),
+ _("Unexpected parameter %s for domain
object"),
params[i].key);
goto cleanup;
}
}
- if (priv->id == 0) {
+ if (priv->t.dom.id == 0) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Missing ID parameter for domain object"));
goto cleanup;
}
- if (priv->pid == 0)
+ if (priv->t.dom.pid == 0)
VIR_DEBUG("Missing PID parameter for domain object");
- if (!priv->name) {
+ if (!priv->t.dom.name) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Missing name parameter for domain object"));
goto cleanup;
}
- if (!virUUIDIsValid(priv->uuid)) {
+ if (!virUUIDIsValid(priv->t.dom.uuid)) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Missing UUID parameter for domain object"));
goto cleanup;
}
break;
+ case VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON:
+ for (i = 0; i < nparams; i++) {
+ if (STREQ(params[i].key, "uuid")) {
+ memcpy(priv->t.daemon.uuid, params[i].value.uuid, VIR_UUID_BUFLEN);
+ } else if (STREQ(params[i].key, "name")) {
+ if (VIR_STRDUP(priv->t.daemon.name, params[i].value.str) < 0)
+ goto cleanup;
+ } else if (STREQ(params[i].key, "pid")) {
+ priv->t.daemon.pid = params[i].value.iv;
So what happens if "id" and/or "uri" are in params? For DOMAIN we
ignore "uri", should that be done here (for both)?
+ } else {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unexpected parameter %s for daemon
object"),
+ params[i].key);
+ goto cleanup;
+ }
+ }
+
+ if (!virUUIDIsValid(priv->t.daemon.uuid)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Missing UUID parameter for daemon object"));
+ goto cleanup;
+ }
+ if (!priv->t.daemon.name) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Missing name parameter for daemon object"));
+ goto cleanup;
+ }
+ if (priv->t.daemon.pid == 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Missing PID parameter for daemon object"));
+ goto cleanup;
+ }
+ break;
+
Yeah, still nothing here really leads me to say we really need a union.
Checking for that id == 0 could still happen if we set it to -1.
I'm not against the model, just not fully on board (yet).
default:
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Unknown lock manager object type %d"),
diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c
index 39c2f94a76..fe422d3be6 100644
--- a/src/locking/lock_driver_sanlock.c
+++ b/src/locking/lock_driver_sanlock.c
@@ -513,21 +513,32 @@ static int virLockManagerSanlockNew(virLockManagerPtr lock,
priv->flags = flags;
- for (i = 0; i < nparams; i++) {
- param = ¶ms[i];
+ switch ((virLockManagerObjectType) type) {
+ case VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN:
+ for (i = 0; i < nparams; i++) {
+ param = ¶ms[i];
- if (STREQ(param->key, "uuid")) {
- memcpy(priv->vm_uuid, param->value.uuid, 16);
- } else if (STREQ(param->key, "name")) {
- if (VIR_STRDUP(priv->vm_name, param->value.str) < 0)
- goto error;
- } else if (STREQ(param->key, "pid")) {
- priv->vm_pid = param->value.iv;
- } else if (STREQ(param->key, "id")) {
- priv->vm_id = param->value.ui;
- } else if (STREQ(param->key, "uri")) {
- priv->vm_uri = param->value.cstr;
+ if (STREQ(param->key, "uuid")) {
+ memcpy(priv->vm_uuid, param->value.uuid, 16);
+ } else if (STREQ(param->key, "name")) {
+ if (VIR_STRDUP(priv->vm_name, param->value.str) < 0)
+ goto error;
+ } else if (STREQ(param->key, "pid")) {
+ priv->vm_pid = param->value.iv;
+ } else if (STREQ(param->key, "id")) {
+ priv->vm_id = param->value.ui;
+ } else if (STREQ(param->key, "uri")) {
+ priv->vm_uri = param->value.cstr;
I know it's existing, but doesn't this one make you pause and go,
hmmmm... does param->value.cstr ever get free'd anywhere. doesn't seem
so from my quick searches (some constcfg->uri a/k/a qemu:///system or
qemu:///session)
+ }
}
+ break;
+
+ case VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON:
+ default:
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unknown lock manager object type %d"),
Technically one is unsupported and the rest are unknown, it it really
matters.
+ type);
+ goto error;
}
/* Sanlock needs process registration, but the only way how to probe
Let's see if I get a response from you before I finish reviewing or if I
decide by the end that I've changed my mind for the R-By...
John