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); }