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.
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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
