On 07/22/2014 05:20 AM, Peter Krempa wrote:
> Use the callback to set disk and storage image labels by modifying the
> existing functions and adding wrappers to avoid refactoring a lot of the
> code.
> ---
> src/security/security_dac.c | 89 +++++++++++++++++++++++++++++++++++----------
> 1 file changed, 69 insertions(+), 20 deletions(-)
>
> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index 1fb0c86..989329f 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -230,23 +230,54 @@ virSecurityDACPreFork(virSecurityManagerPtr mgr)
> }
>
> static int
> -virSecurityDACSetOwnership(const char *path, uid_t uid, gid_t gid)
> +virSecurityDACSetOwnershipInternal(virSecurityDACDataPtr priv,
> + virStorageSourcePtr src,
> + const char *path,
> + uid_t uid,
> + gid_t gid)
> {
> + int rc;
> + int chown_errno;
> +
> VIR_INFO("Setting DAC user and group on '%s' to '%ld:%ld'",
> - path, (long) uid, (long) gid);
> + NULLSTR(src ? src->path : path), (long) uid, (long) gid);
> +
> + if (priv && src && priv->chownCallback) {
This is the 'src' option and 'path' is NULL
> + rc = priv->chownCallback(src, uid, gid);
> +
> + /* on -2 returned an error was already reported */
> + if (rc == -2)
> + return -1;
>
> - if (chown(path, uid, gid) < 0) {
> + /* on -1 only errno was set */
> + chown_errno = errno;
Thus rc == -1, path == NULL
> + } else {
> struct stat sb;
> - int chown_errno = errno;
>
> - if (stat(path, &sb) >= 0) {
> + if (!path) {
> + if (!src || !src->path)
Maybe a "if !src && !path" return 0 should be before the VIR_INFO...
Maybe with a VIR_DEBUG that'll help someone some day figure out why they
aren't getting what they were expecting. Just a thought...
> + return 0;
> +
> + if (!virStorageSourceIsLocalStorage(src))
> + return 0;
> +
> + path = src->path;
Reusing a passed parameter is where things get dicey for me.
> + }
> +
> + rc = chown(path, uid, gid);
> + chown_errno = errno;
> +
> + if (rc < 0 &&
> + stat(path, &sb) >= 0) {
> if (sb.st_uid == uid &&
> sb.st_gid == gid) {
> /* It's alright, there's nothing to change anyway. */
> return 0;
> }
> }
> + }
>
> + if (rc < 0) {
I think we can get here with path == NULL and rc == -1, resulting in
subsequent usage of '%s' for path to have 'nil', right?
> if (chown_errno == EOPNOTSUPP || chown_errno == EINVAL) {
> VIR_INFO("Setting user and group to '%ld:%ld' on '%s' not "
> "supported by filesystem",
> @@ -270,13 +301,31 @@ virSecurityDACSetOwnership(const char *path, uid_t uid, gid_t gid)
> return 0;
> }
>
> +
> static int
> -virSecurityDACRestoreSecurityFileLabel(const char *path)
> +virSecurityDACSetOwnership(const char *path, uid_t uid, gid_t gid)
> +{
> + return virSecurityDACSetOwnershipInternal(NULL, NULL, path, uid, gid);
> +}
> +
> +
> +static int
> +virSecurityDACRestoreSecurityFileLabelInternal(virSecurityDACDataPtr priv,
> + virStorageSourcePtr src,
> + const char *path)
> {
> - VIR_INFO("Restoring DAC user and group on '%s'", path);
> + VIR_INFO("Restoring DAC user and group on '%s'",
> + NULLSTR(src ? src->path : path));
>
> /* XXX record previous ownership */
> - return virSecurityDACSetOwnership(path, 0, 0);
> + return virSecurityDACSetOwnershipInternal(priv, src, path, 0, 0);
I know this just follows the previous code, but in the big picture - how
does this "play" in with future patches where fs & gluster will
seemingly go though this path?
ACK - in general with the 'path' issue above understood. For this code,
I'm mostly curious.
John
> +}
> +
> +
> +static int
> +virSecurityDACRestoreSecurityFileLabel(const char *path)
> +{
> + return virSecurityDACRestoreSecurityFileLabelInternal(NULL, NULL, path);
> }
>
>
> @@ -294,10 +343,6 @@ virSecurityDACSetSecurityImageLabel(virSecurityManagerPtr mgr,
> if (!priv->dynamicOwnership)
> return 0;
>
> - /* XXX: Add support for gluster DAC permissions */
> - if (!src->path || !virStorageSourceIsLocalStorage(src))
> - return 0;
> -
> secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME);
> if (secdef && !secdef->relabel)
> return 0;
> @@ -315,7 +360,7 @@ virSecurityDACSetSecurityImageLabel(virSecurityManagerPtr mgr,
> return -1;
> }
>
> - return virSecurityDACSetOwnership(src->path, user, group);
> + return virSecurityDACSetOwnershipInternal(priv, src, NULL, user, group);
> }
>
>
> @@ -349,9 +394,6 @@ virSecurityDACRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr,
> if (!priv->dynamicOwnership)
> return 0;
>
> - if (!src->path || !virStorageSourceIsLocalStorage(src))
> - return 0;
> -
> /* Don't restore labels on readoly/shared disks, because other VMs may
> * still be accessing these. Alternatively we could iterate over all
> * running domains and try to figure out if it is in use, but this would
> @@ -373,9 +415,16 @@ virSecurityDACRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr,
> * ownership, because that kills access on the destination host which is
> * sub-optimal for the guest VM's I/O attempts :-) */
> if (migrated) {
> - int rc = virFileIsSharedFS(src->path);
> - if (rc < 0)
> - return -1;
> + int rc = 1;
> +
> + if (virStorageSourceIsLocalStorage(src)) {
> + if (!src->path)
> + return 0;
> +
> + if ((rc = virFileIsSharedFS(src->path)) < 0)
> + return -1;
> + }
> +
> if (rc == 1) {
> VIR_DEBUG("Skipping image label restore on %s because FS is shared",
> src->path);
> @@ -383,7 +432,7 @@ virSecurityDACRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr,
> }
> }
>
> - return virSecurityDACRestoreSecurityFileLabel(src->path);
> + return virSecurityDACRestoreSecurityFileLabelInternal(priv, src, NULL);
> }
>
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list