On 09/03/2018 11:13 AM, Michal Privoznik wrote:
> 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@xxxxxxxxxx>
>>> ---
>>> 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.
>
Remember the caveat - I was going patch by patch. I see a union like
this I'm thinking something in the future would then be using a pointer
from &x.t.dom or &x.t.daemon rather than typing the whole path.
>>
>> 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.
>
Ok - so I'm not opposed to the design decision of namespacing, only
indicating it really didn't seem necessary. Perhaps "common" members
between the structs can be common to the @priv, while "specific" members
would be in a union like this. Of course, doing that leaves nothing
specific for daemon, does it. Although come to think of it, wouldn't
eventually the clientRefs, client, program, and counter be more
typically associated with daemon instead of dom? IOW: Those are
metadata lock concepts and not disk/lease concepts.
Still it's a design decision and while I think it's perhaps a bit of
overkill, there is some value vis-a-vis namespace and usage of the
virLockManagerObjectType.
> 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].
One would think that either this or the subsequent patch wouldn't allow
a "domain" type lock to use "metadata", so it would fail because
@lock->priv->type doesn't support that mixed usage.
>
> 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.
>
Yep... sounds like the distributed lock manager to me. I don't believe I
was concerned about usage of @dom vs. @daemon. I just wasn't convinced
common members to both unions need to be separated. Things specific to a
lock type would be though.
>>
>>>
>>> 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.
>
OK, that's fine. If someone does implement a sanlock or perhaps some
new style in the future, then it'd be up to them to modify this code if
necessary. Fair enough.
John
>>> + }
>>> +
>>> + 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 = ¶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)
>
> 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
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
