[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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




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 redhat com>
> ---
>  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 = &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)

> +            }
>          }
> +        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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]