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@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 :|