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@redhat.com>
---
src/locking/lock_driver.h | 2 +
src/locking/lock_driver_lockd.c | 291 ++++++++++++++++++++++++--------------
src/locking/lock_driver_sanlock.c | 37 +++--
3 files changed, 214 insertions(+), 116 deletions(-)
diff --git a/src/locking/lock_driver.h b/src/locking/lock_driver.h
index 8b7cccc521..a9d2041c30 100644
--- a/src/locking/lock_driver.h
+++ b/src/locking/lock_driver.h
@@ -42,6 +42,8 @@ typedef enum {
typedef enum {
/* The managed object is a virtual guest domain */
VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN = 0,
+ /* The managed object is a daemon (e.g. libvirtd) */
+ VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON = 1,
} virLockManagerObjectType;
typedef enum {
diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c
index ca825e6026..8580d12340 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;
size_t nresources;
virLockManagerLockDaemonResourcePtr resources;
@@ -156,10 +167,24 @@ virLockManagerLockDaemonConnectionRegister(virLockManagerPtr lock,
memset(&args, 0, sizeof(args));
args.flags = 0;
- memcpy(args.owner.uuid, priv->uuid, VIR_UUID_BUFLEN);
- args.owner.name = priv->name;
- args.owner.id = priv->id;
- args.owner.pid = priv->pid;
+
+ switch (priv->type) {
+ case VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN:
+ memcpy(args.owner.uuid, priv->t.dom.uuid, VIR_UUID_BUFLEN);
+ args.owner.name = priv->t.dom.name;
+ args.owner.id = priv->t.dom.id;
+ args.owner.pid = priv->t.dom.pid;
+ break;
+
+ case VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON:
+ memcpy(args.owner.uuid, priv->t.daemon.uuid, VIR_UUID_BUFLEN);
+ args.owner.name = priv->t.daemon.name;
+ args.owner.pid = priv->t.daemon.pid;
+ break;
+
+ default:
+ return -1;
+ }
if (virNetClientProgramCall(program,
client,
@@ -391,7 +416,18 @@ virLockManagerLockDaemonPrivateFree(virLockManagerLockDaemonPrivatePtr priv)
}
VIR_FREE(priv->resources);
- VIR_FREE(priv->name);
+ switch (priv->type) {
+ case VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN:
+ VIR_FREE(priv->t.dom.name);
+ break;
+
+ case VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON:
+ VIR_FREE(priv->t.daemon.name);
+ break;
+
+ default:
+ break;
+ }
VIR_FREE(priv);
}
@@ -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;
+ } 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;
+
default:
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Unknown lock manager object type %d"),
@@ -494,107 +566,119 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock,
if (flags & VIR_LOCK_MANAGER_RESOURCE_READONLY)
return 0;
- switch (type) {
- case VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK:
- if (params || nparams) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Unexpected parameters for disk resource"));
- goto cleanup;
- }
- if (!driver->autoDiskLease) {
- if (!(flags & (VIR_LOCK_MANAGER_RESOURCE_SHARED |
- VIR_LOCK_MANAGER_RESOURCE_READONLY)))
- priv->hasRWDisks = true;
- return 0;
- }
+ switch (priv->type) {
+ case VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN:
- /* XXX we should somehow pass in TYPE=BLOCK info
- * from the domain_lock code, instead of assuming /dev
- */
- if (STRPREFIX(name, "/dev") &&
- driver->lvmLockSpaceDir) {
- VIR_DEBUG("Trying to find an LVM UUID for %s", name);
- if (virStorageFileGetLVMKey(name, &newName) < 0)
+ switch (type) {
+ case VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK:
+ if (params || nparams) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Unexpected parameters for disk resource"));
goto cleanup;
+ }
+ if (!driver->autoDiskLease) {
+ if (!(flags & (VIR_LOCK_MANAGER_RESOURCE_SHARED |
+ VIR_LOCK_MANAGER_RESOURCE_READONLY)))
+ priv->hasRWDisks = true;
+ return 0;
+ }
- if (newName) {
- VIR_DEBUG("Got an LVM UUID %s for %s", newName, name);
- if (VIR_STRDUP(newLockspace, driver->lvmLockSpaceDir) < 0)
+ /* XXX we should somehow pass in TYPE=BLOCK info
+ * from the domain_lock code, instead of assuming /dev
+ */
+ if (STRPREFIX(name, "/dev") &&
+ driver->lvmLockSpaceDir) {
+ VIR_DEBUG("Trying to find an LVM UUID for %s", name);
+ if (virStorageFileGetLVMKey(name, &newName) < 0)
goto cleanup;
- autoCreate = true;
- break;
+
+ if (newName) {
+ VIR_DEBUG("Got an LVM UUID %s for %s", newName, name);
+ if (VIR_STRDUP(newLockspace, driver->lvmLockSpaceDir) < 0)
+ goto cleanup;
+ autoCreate = true;
+ break;
+ }
+ virResetLastError();
+ /* Fallback to generic non-block code */
}
- virResetLastError();
- /* Fallback to generic non-block code */
- }
- if (STRPREFIX(name, "/dev") &&
- driver->scsiLockSpaceDir) {
- VIR_DEBUG("Trying to find an SCSI ID for %s", name);
- if (virStorageFileGetSCSIKey(name, &newName) < 0)
- goto cleanup;
+ if (STRPREFIX(name, "/dev") &&
+ driver->scsiLockSpaceDir) {
+ VIR_DEBUG("Trying to find an SCSI ID for %s", name);
+ if (virStorageFileGetSCSIKey(name, &newName) < 0)
+ goto cleanup;
+
+ if (newName) {
+ VIR_DEBUG("Got an SCSI ID %s for %s", newName, name);
+ if (VIR_STRDUP(newLockspace, driver->scsiLockSpaceDir) < 0)
+ goto cleanup;
+ autoCreate = true;
+ break;
+ }
+ virResetLastError();
+ /* Fallback to generic non-block code */
+ }
- if (newName) {
- VIR_DEBUG("Got an SCSI ID %s for %s", newName, name);
- if (VIR_STRDUP(newLockspace, driver->scsiLockSpaceDir) < 0)
+ if (driver->fileLockSpaceDir) {
+ if (VIR_STRDUP(newLockspace, driver->fileLockSpaceDir) < 0)
+ goto cleanup;
+ if (virCryptoHashString(VIR_CRYPTO_HASH_SHA256, name, &newName) < 0)
goto cleanup;
autoCreate = true;
- break;
+ VIR_DEBUG("Using indirect lease %s for %s", newName, name);
+ } else {
+ if (VIR_STRDUP(newLockspace, "") < 0)
+ goto cleanup;
+ if (VIR_STRDUP(newName, name) < 0)
+ goto cleanup;
+ VIR_DEBUG("Using direct lease for %s", name);
}
- virResetLastError();
- /* Fallback to generic non-block code */
- }
- if (driver->fileLockSpaceDir) {
- if (VIR_STRDUP(newLockspace, driver->fileLockSpaceDir) < 0)
- goto cleanup;
- if (virCryptoHashString(VIR_CRYPTO_HASH_SHA256, name, &newName) < 0)
+ break;
+ case VIR_LOCK_MANAGER_RESOURCE_TYPE_LEASE: {
+ size_t i;
+ char *path = NULL;
+ char *lockspace = NULL;
+ for (i = 0; i < nparams; i++) {
+ if (STREQ(params[i].key, "offset")) {
+ if (params[i].value.ul != 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Offset must be zero for this lock manager"));
+ goto cleanup;
+ }
+ } else if (STREQ(params[i].key, "lockspace")) {
+ lockspace = params[i].value.str;
+ } else if (STREQ(params[i].key, "path")) {
+ path = params[i].value.str;
+ } else {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unexpected parameter %s for lease resource"),
+ params[i].key);
+ goto cleanup;
+ }
+ }
+ if (!path || !lockspace) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Missing path or lockspace for lease resource"));
goto cleanup;
- autoCreate = true;
- VIR_DEBUG("Using indirect lease %s for %s", newName, name);
- } else {
- if (VIR_STRDUP(newLockspace, "") < 0)
+ }
+ if (virAsprintf(&newLockspace, "%s/%s",
+ path, lockspace) < 0)
goto cleanup;
if (VIR_STRDUP(newName, name) < 0)
goto cleanup;
- VIR_DEBUG("Using direct lease for %s", name);
- }
+ } break;
+ default:
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unknown lock manager object type %d for domain lock object"),
+ type);
+ goto cleanup;
+ }
break;
- case VIR_LOCK_MANAGER_RESOURCE_TYPE_LEASE: {
- size_t i;
- char *path = NULL;
- char *lockspace = NULL;
- for (i = 0; i < nparams; i++) {
- if (STREQ(params[i].key, "offset")) {
- if (params[i].value.ul != 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Offset must be zero for this lock manager"));
- goto cleanup;
- }
- } else if (STREQ(params[i].key, "lockspace")) {
- lockspace = params[i].value.str;
- } else if (STREQ(params[i].key, "path")) {
- path = params[i].value.str;
- } else {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Unexpected parameter %s for lease resource"),
- params[i].key);
- goto cleanup;
- }
- }
- if (!path || !lockspace) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Missing path or lockspace for lease resource"));
- goto cleanup;
- }
- if (virAsprintf(&newLockspace, "%s/%s",
- path, lockspace) < 0)
- goto cleanup;
- if (VIR_STRDUP(newName, name) < 0)
- goto cleanup;
- } break;
+ case VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON:
default:
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Unknown lock manager object type %d"),
@@ -639,7 +723,8 @@ static int virLockManagerLockDaemonAcquire(virLockManagerPtr lock,
virCheckFlags(VIR_LOCK_MANAGER_ACQUIRE_REGISTER_ONLY |
VIR_LOCK_MANAGER_ACQUIRE_RESTRICT, -1);
- if (priv->nresources == 0 &&
+ if (priv->type == VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN &&
+ priv->nresources == 0 &&
priv->hasRWDisks &&
driver->requireLeaseForDisks) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
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;
+ }
}
+ break;
+
+ case VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON:
+ default:
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unknown lock manager object type %d"),
+ type);
+ goto error;
}
/* Sanlock needs process registration, but the only way how to probe
--
2.16.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 09/10/2018 05:36 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@redhat.com>
> ---
> src/locking/lock_driver.h | 2 +
> src/locking/lock_driver_lockd.c | 291 ++++++++++++++++++++++++--------------
> src/locking/lock_driver_sanlock.c | 37 +++--
> 3 files changed, 214 insertions(+), 116 deletions(-)
>
[...]
> @@ -156,10 +167,24 @@ virLockManagerLockDaemonConnectionRegister(virLockManagerPtr lock,
> memset(&args, 0, sizeof(args));
>
> args.flags = 0;
> - memcpy(args.owner.uuid, priv->uuid, VIR_UUID_BUFLEN);
> - args.owner.name = priv->name;
> - args.owner.id = priv->id;
> - args.owner.pid = priv->pid;
> +
> + switch (priv->type) {
> + case VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN:
> + memcpy(args.owner.uuid, priv->t.dom.uuid, VIR_UUID_BUFLEN);
> + args.owner.name = priv->t.dom.name;
> + args.owner.id = priv->t.dom.id;
> + args.owner.pid = priv->t.dom.pid;
> + break;
> +
> + case VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON:
> + memcpy(args.owner.uuid, priv->t.daemon.uuid, VIR_UUID_BUFLEN);
> + args.owner.name = priv->t.daemon.name;
> + args.owner.pid = priv->t.daemon.pid;
> + break;
> +
> + default:
Should there be an error message? Since virNetClientProgramCall would
provide one on error and it seems callers would expect it... So we don't
end up with an error happened, but we have no clue where.
> + return -1;
> + }
>
> if (virNetClientProgramCall(program,
> client,
[...]
I suspect you can make the right choice above...
Reviewed-by: John Ferlan <jferlan@redhat.com>
John
[...]
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 09/17/2018 06:17 PM, John Ferlan wrote:
>
>
> On 09/10/2018 05:36 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@redhat.com>
>> ---
>> src/locking/lock_driver.h | 2 +
>> src/locking/lock_driver_lockd.c | 291 ++++++++++++++++++++++++--------------
>> src/locking/lock_driver_sanlock.c | 37 +++--
>> 3 files changed, 214 insertions(+), 116 deletions(-)
>>
>
> [...]
>
>> @@ -156,10 +167,24 @@ virLockManagerLockDaemonConnectionRegister(virLockManagerPtr lock,
>> memset(&args, 0, sizeof(args));
>>
>> args.flags = 0;
>> - memcpy(args.owner.uuid, priv->uuid, VIR_UUID_BUFLEN);
>> - args.owner.name = priv->name;
>> - args.owner.id = priv->id;
>> - args.owner.pid = priv->pid;
>> +
>> + switch (priv->type) {
>> + case VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN:
>> + memcpy(args.owner.uuid, priv->t.dom.uuid, VIR_UUID_BUFLEN);
>> + args.owner.name = priv->t.dom.name;
>> + args.owner.id = priv->t.dom.id;
>> + args.owner.pid = priv->t.dom.pid;
>> + break;
>> +
>> + case VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON:
>> + memcpy(args.owner.uuid, priv->t.daemon.uuid, VIR_UUID_BUFLEN);
>> + args.owner.name = priv->t.daemon.name;
>> + args.owner.pid = priv->t.daemon.pid;
>> + break;
>> +
>> + default:
>
> Should there be an error message? Since virNetClientProgramCall would
> provide one on error and it seems callers would expect it... So we don't
> end up with an error happened, but we have no clue where.
The 'default' label exists merely to shut gcc up. However, I'll copy the
error message from virLockManagerLockDaemonNew().
>
>> + return -1;
>> + }
>>
>> if (virNetClientProgramCall(program,
>> client,
>
> [...]
>
> I suspect you can make the right choice above...
>
> Reviewed-by: John Ferlan <jferlan@redhat.com>
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.