Re: [libvirt] [PATCH v3 11/28] lock_driver: Introduce new VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON

On 08/30/2018 10:40 PM, John Ferlan wrote: > > > 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&gt; >> --- >> 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. I don't see much benefit to that but I can do that change.

> > 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. The idea is that in virLockManagerLockNew() one specifies which type of lock they want (whether it's a domain or daemon type of lock) and any subsequent call to virLockManager*() will either succeed or fail if they want to work with wrong object. So this can be viewed as namespacing.

Basically I want to avoid this code pattern: lock = virLockManagerNew(virLockManagerPluginGetDriver(plugin), VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN, ARRAY_CARDINALITY(params), params, flags))); virLockManagerAddResource(lock, VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK, src->path, 0, NULL, 0); virLockManagerAddResource(lock, VIR_LOCK_MANAGER_RESOURCE_TYPE_METADATA, src->path, 0, NULL, 0); This is a buggy pattern, because OBJECT_TYPE_DOMAIN is associated with the domain (the owner of the lock is the domain, the ownerPID is set from domain, etc.). However, we want RESOURCE_TYPE_METADATA to be associated with the libvirtd and not the domain [1].

1 - the reason for the metadata lock to be associated with the libvirtd and not the domain is very simple: it's libvirtd who is changing the metadata so it should grab the lock too. Also, if registered owner of a lock disappears (e.g. due to a crash) virlockd releases all the locks the owner had. And if you view metadata locking from this angle, it is obvious that we want the metadata lock to be released if libvirtd crashes. Not the domain. Consider the following scenario: two hosts (A and B), two domains running (D1 on A and D2 on B) and both hosts want to plug disk X into respective domains. HostA: libvirtd locks path X for metadata HostB: libvirtd locks path X for metadata, but since the path is already locked (by hostA) this call will just wait for unlock HostA: libvirtd wants to proceed to chown() but it crashes instead (e.g. segfault from another thread or something) At this point, we want the lock to be released and allow HostB to proceed with locking. If the lock was associated with domain, then the lock wouldn't be released and HostB wouldn't be able to plug in the disk.

> >> >> 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)? We error out .. > >> + } else { >> + virReportError(VIR_ERR_INTERNAL_ERROR, >> + _("Unexpected parameter %s for daemon object"), >> + params[i].key); >> + goto cleanup; >> + } .. here. The uri is ignored for virtlockd driver because it's used in sanlock. And the whole point of drivers is to have a single API with multiple implementations. Therefore if somebody calls: virLockManagerParam params[] = { ... { .type = VIR_LOCK_MANAGER_PARAM_TYPE_CSTRING, .key = "uri", .value = { .cstr = uri }, }, ... }; lock = virLockManagerNew(virLockManagerPluginGetDriver(plugin), VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN, ARRAY_CARDINALITY(params), params, flags); they don't have to care whether driver in use is lockd or sanlock. They have unified APIs to call. Since we don need that luxury for OBJECT_TYPE_DAEMON, we should error right out letting caller know they passed unsupported parameter.

>> + } >> + >> + 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). Sure, we can have one big struct and use just a portion of it. I don't care that much. To me it's just cosmetics. I've chosen union because it's more memory friendly perhaps? For instance: struct { int A; /* for domain */ int B; /* for daemon */ }; struct { union { int A; int B; }; }; The former struct will be 2 ints big while the latter only one int big. And because we will never use both at the same time (due to my explanation above) I went with the latter. It's used in other areas of the code too. From them all I'd pick _virDomainDeviceDef or _virDomainChrSourceDef as an example. > >> 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 = &params[i]; >> + switch ((virLockManagerObjectType) type) { >> + case VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN: >> + for (i = 0; i < nparams; i++) { >> + param = &params[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) That's right. And it never should! cstr is a const char: struct _virLockManagerParam { int type; const char *key; union { int iv; long long l; unsigned int ui; unsigned long long ul; double d; char *str; const char *cstr; unsigned char uuid[16]; } value; }; (oh look, another union ;-)) Michal