[libvirt] [PATCH v2 03/18] security: Include security_util

[Daniel P. Berrangé]

On Thu, Nov 29, 2018 at 02:52:18PM +0100, Michal Privoznik wrote:
> This file implements wrappers over XATTR getter/setter. It
> ensures the proper XATTR namespace is used.
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/security/Makefile.inc.am |   2 +
>  src/security/security_util.c | 226 +++++++++++++++++++++++++++++++++++
>  src/security/security_util.h |  32 +++++
>  3 files changed, 260 insertions(+)
>  create mode 100644 src/security/security_util.c
>  create mode 100644 src/security/security_util.h
> 


> +/**
> + * virSecurityGetRememberedLabel:
> + * @name: security driver name
> + * @path: file name
> + * @label: label
> + *
> + * For given @path and security driver (@name) fetch remembered
> + * @label. The caller must not restore label if an error is
> + * indicated or if @label is NULL upon return.
> + *
> + * Returns: 0 on success,
> + *         -1 otherwise (with error reported)
> + */
> +int
> +virSecurityGetRememberedLabel(const char *name,
> +                              const char *path,
> +                              char **label)
> +{
> +    char *ref_name = NULL;
> +    char *attr_name = NULL;
> +    char *value = NULL;
> +    unsigned int refcount = 0;
> +    int ret = -1;
> +
> +    *label = NULL;
> +
> +    if (!(ref_name = virSecurityGetRefCountAttrName(name)))
> +        goto cleanup;
> +
> +    if (virFileGetXAtrr(path, ref_name, &value) < 0) {
> +        if (errno == ENOSYS || errno == ENODATA || errno == ENOTSUP) {
> +            ret = 0;
> +        } else {
> +            virReportSystemError(errno,
> +                                 _("Unable to get XATTR %s on %s"),
> +                                 ref_name,
> +                                 path);
> +        }
> +        goto cleanup;
> +    }
> +
> +    if (virStrToLong_ui(value, NULL, 10, &refcount) < 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("malformed refcount %s on %s"),
> +                       value, path);
> +        goto cleanup;
> +    }
> +
> +    VIR_FREE(value);
> +
> +    refcount--;
> +
> +    if (refcount > 0) {
> +        if (virAsprintf(&value, "%u", refcount) < 0)
> +            goto cleanup;
> +
> +        if (virFileSetXAtrr(path, ref_name, value) < 0)
> +            goto cleanup;
> +    } else {
> +        if (virFileRemoveXAttr(path, ref_name) < 0)
> +            goto cleanup;
> +
> +        if (!(attr_name = virSecurityGetAttrName(name)))
> +            goto cleanup;
> +
> +        if (virFileGetXAtrr(path, attr_name, label) < 0)
> +            goto cleanup;

I'm not understanding why we only fetch the label value in
this half of the conditional ? Shouldn't we be unconditionally
getting the label, regardless of the updated refcount value.

If not, the method description could have an explanation of
intended behaviour.

> +
> +        if (virFileRemoveXAttr(path, attr_name) < 0)
> +            goto cleanup;
> +    }
> +
> +    ret = 0;
> + cleanup:
> +    VIR_FREE(value);
> +    VIR_FREE(attr_name);
> +    VIR_FREE(ref_name);
> +    return ret;
> +}
> +
> +
> +int
> +virSecuritySetRememberedLabel(const char *name,
> +                              const char *path,
> +                              const char *label)
> +{
> +    char *ref_name = NULL;
> +    char *attr_name = NULL;
> +    char *value = NULL;
> +    unsigned int refcount = 0;
> +    int ret = -1;
> +
> +    if (!(ref_name = virSecurityGetRefCountAttrName(name)))
> +        goto cleanup;
> +
> +    if (virFileGetXAtrr(path, ref_name, &value) < 0) {
> +        if (errno == ENOSYS || errno == ENOTSUP) {
> +            ret = 0;
> +            goto cleanup;
> +        } else if (errno != ENODATA) {
> +            virReportSystemError(errno,
> +                                 _("Unable to get XATTR %s on %s"),
> +                                 ref_name,
> +                                 path);
> +            goto cleanup;
> +        }
> +    }
> +
> +    if (value &&
> +        virStrToLong_ui(value, NULL, 10, &refcount) < 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("malformed refcount %s on %s"),
> +                       value, path);
> +        goto cleanup;
> +    }
> +
> +    VIR_FREE(value);
> +
> +    refcount++;
> +
> +    if (refcount == 1) {
> +        if (!(attr_name = virSecurityGetAttrName(name)))
> +            goto cleanup;
> +
> +        if (virFileSetXAtrr(path, attr_name, label) < 0)
> +            goto cleanup;
> +    }

Do we need to have a

     } else {
        .... check the currently remember label matches
	      what was passed into this method ?
     }

if not, can you add API docs for this method explainng the
intended semantics when label is already remembered.

> +
> +    if (virAsprintf(&value, "%u", refcount) < 0)
> +        goto cleanup;
> +
> +    if (virFileSetXAtrr(path, ref_name, value) < 0)
> +        goto cleanup;
> +
> +    ret = 0;
> + cleanup:
> +    VIR_FREE(value);
> +    VIR_FREE(attr_name);
> +    VIR_FREE(ref_name);
> +    return ret;
> +}

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|