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: I don't see much benefit to that but I can do that change. 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. We error out .. .. 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. 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. 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