[libvirt] [PATCHv1.5 0/8] Gluster security driver support and snapshot file pre-creation
I'm posting a rebased version as John notified me that the original series cannot be applied cleanly any more. Peter Krempa (8): storage: Implement storage driver helper to chown disk images storage: Add witness for checking storage volume use in security driver security: DAC: Remove superfluous link resolution security: DAC: Introduce callback to perform image chown security: DAC: Plumb usage of chown callback qemu: Implement DAC driver chown callback to co-operate with storage drv storage: Implement virStorageFileCreate for local and gluster files qemu: snapshot: Use storage driver to pre-create snapshot file src/qemu/qemu_driver.c | 66 ++++++++++++++++---- src/security/security_dac.c | 109 +++++++++++++++++++++++----------- src/security/security_dac.h | 3 + src/security/security_manager.c | 4 +- src/security/security_manager.h | 19 +++++- src/storage/storage_backend.h | 6 ++ src/storage/storage_backend_fs.c | 29 +++++++++ src/storage/storage_backend_gluster.c | 27 +++++++++ src/storage/storage_driver.c | 58 ++++++++++++++++++ src/storage/storage_driver.h | 3 + 10 files changed, 277 insertions(+), 47 deletions(-) -- 2.0.0
Gluster storage works on a similar principle to NFS where it takes the uid and gid of the actual process and uses it to access the storage volume on the remote server. This introduces a need to chown storage files on gluster via native API. --- src/storage/storage_backend.h | 6 ++++++ src/storage/storage_backend_fs.c | 12 ++++++++++++ src/storage/storage_backend_gluster.c | 12 ++++++++++++ src/storage/storage_driver.c | 28 ++++++++++++++++++++++++++++ src/storage/storage_driver.h | 1 + 5 files changed, 59 insertions(+) diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index e48da5b..a93df5d 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -244,6 +244,11 @@ typedef int (*virStorageFileBackendAccess)(virStorageSourcePtr src, int mode); +typedef int +(*virStorageFileBackendChown)(virStorageSourcePtr src, + uid_t uid, + gid_t gid); + virStorageFileBackendPtr virStorageFileBackendForType(int type, int protocol); virStorageFileBackendPtr virStorageFileBackendForTypeInternal(int type, int protocol, @@ -269,6 +274,7 @@ struct _virStorageFileBackend { virStorageFileBackendUnlink storageFileUnlink; virStorageFileBackendStat storageFileStat; virStorageFileBackendAccess storageFileAccess; + virStorageFileBackendChown storageFileChown; }; #endif /* __VIR_STORAGE_BACKEND_H__ */ diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 40cc1f4..2af5ab5 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -1455,6 +1455,15 @@ virStorageFileBackendFileAccess(virStorageSourcePtr src, } +static int +virStorageFileBackendFileChown(virStorageSourcePtr src, + uid_t uid, + gid_t gid) +{ + return chown(src->path, uid, gid); +} + + virStorageFileBackend virStorageFileBackendFile = { .type = VIR_STORAGE_TYPE_FILE, @@ -1465,6 +1474,7 @@ virStorageFileBackend virStorageFileBackendFile = { .storageFileStat = virStorageFileBackendFileStat, .storageFileReadHeader = virStorageFileBackendFileReadHeader, .storageFileAccess = virStorageFileBackendFileAccess, + .storageFileChown = virStorageFileBackendFileChown, .storageFileGetUniqueIdentifier = virStorageFileBackendFileGetUniqueIdentifier, }; @@ -1479,6 +1489,7 @@ virStorageFileBackend virStorageFileBackendBlock = { .storageFileStat = virStorageFileBackendFileStat, .storageFileReadHeader = virStorageFileBackendFileReadHeader, .storageFileAccess = virStorageFileBackendFileAccess, + .storageFileChown = virStorageFileBackendFileChown, .storageFileGetUniqueIdentifier = virStorageFileBackendFileGetUniqueIdentifier, }; @@ -1491,6 +1502,7 @@ virStorageFileBackend virStorageFileBackendDir = { .backendDeinit = virStorageFileBackendFileDeinit, .storageFileAccess = virStorageFileBackendFileAccess, + .storageFileChown = virStorageFileBackendFileChown, .storageFileGetUniqueIdentifier = virStorageFileBackendFileGetUniqueIdentifier, }; diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index 0f8f0f3..052f58d 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -762,6 +762,17 @@ virStorageFileBackendGlusterGetUniqueIdentifier(virStorageSourcePtr src) } +static int +virStorageFileBackendGlusterChown(virStorageSourcePtr src, + uid_t uid, + gid_t gid) +{ + virStorageFileBackendGlusterPrivPtr priv = src->drv->priv; + + return glfs_chown(priv->vol, src->path, uid, gid); +} + + virStorageFileBackend virStorageFileBackendGluster = { .type = VIR_STORAGE_TYPE_NETWORK, .protocol = VIR_STORAGE_NET_PROTOCOL_GLUSTER, @@ -773,6 +784,7 @@ virStorageFileBackend virStorageFileBackendGluster = { .storageFileStat = virStorageFileBackendGlusterStat, .storageFileReadHeader = virStorageFileBackendGlusterReadHeader, .storageFileAccess = virStorageFileBackendGlusterAccess, + .storageFileChown = virStorageFileBackendGlusterChown, .storageFileGetUniqueIdentifier = virStorageFileBackendGlusterGetUniqueIdentifier, diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 3830867..efd79b9 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -2613,6 +2613,34 @@ virStorageFileAccess(virStorageSourcePtr src, } +/** + * virStorageFileChown: Change owner of a storage file + * + * @src: storage file to change owner of + * @uid: new owner id + * @gid: new group id + * + * Returns 0 on success, -1 on error and sets errno. No libvirt + * error is reported. Returns -2 if the operation isn't supported + * by libvirt storage backend. + */ +int +virStorageFileChown(virStorageSourcePtr src, + uid_t uid, + gid_t gid) +{ + if (!virStorageFileIsInitialized(src) || + !src->drv->backend->storageFileChown) { + errno = ENOSYS; + return -2; + } + + VIR_DEBUG("chown of storage file %p to %d:%d", src, uid, gid); + + return src->drv->backend->storageFileChown(src, uid, gid); +} + + /* Recursive workhorse for virStorageFileGetMetadata. */ static int virStorageFileGetMetadataRecurse(virStorageSourcePtr src, diff --git a/src/storage/storage_driver.h b/src/storage/storage_driver.h index bd9e9ed..eefd766 100644 --- a/src/storage/storage_driver.h +++ b/src/storage/storage_driver.h @@ -43,6 +43,7 @@ ssize_t virStorageFileReadHeader(virStorageSourcePtr src, char **buf); const char *virStorageFileGetUniqueIdentifier(virStorageSourcePtr src); int virStorageFileAccess(virStorageSourcePtr src, int mode); +int virStorageFileChown(virStorageSourcePtr src, uid_t uid, gid_t gid); int virStorageFileGetMetadata(virStorageSourcePtr src, uid_t uid, gid_t gid, -- 2.0.0
On 07/22/2014 03:20 AM, Peter Krempa wrote:
Gluster storage works on a similar principle to NFS where it takes the uid and gid of the actual process and uses it to access the storage volume on the remote server. This introduces a need to chown storage files on gluster via native API. --- src/storage/storage_backend.h | 6 ++++++ src/storage/storage_backend_fs.c | 12 ++++++++++++ src/storage/storage_backend_gluster.c | 12 ++++++++++++ src/storage/storage_driver.c | 28 ++++++++++++++++++++++++++++ src/storage/storage_driver.h | 1 + 5 files changed, 59 insertions(+)
ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 07/22/2014 05:20 AM, Peter Krempa wrote:
Gluster storage works on a similar principle to NFS where it takes the uid and gid of the actual process and uses it to access the storage volume on the remote server. This introduces a need to chown storage files on gluster via native API. --- src/storage/storage_backend.h | 6 ++++++ src/storage/storage_backend_fs.c | 12 ++++++++++++ src/storage/storage_backend_gluster.c | 12 ++++++++++++ src/storage/storage_driver.c | 28 ++++++++++++++++++++++++++++ src/storage/storage_driver.h | 1 + 5 files changed, 59 insertions(+)
ACK John
With my intended use of storage driver assist to chown files on remote storage we will need a witness that will tell us whether the given storage volume supports operations needed by the storage driver. --- src/storage/storage_driver.c | 30 ++++++++++++++++++++++++++++++ src/storage/storage_driver.h | 2 ++ 2 files changed, 32 insertions(+) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index efd79b9..b397c6e 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -2357,6 +2357,36 @@ virStorageFileSupportsBackingChainTraversal(virStorageSourcePtr src) backend->storageFileAccess; } + +/** + * virStorageFileSupportsSecurityDriver: + * + * @src: a storage file structure + * + * Check if a storage file supports operations needed by the security + * driver to perform labelling */ +bool +virStorageFileSupportsSecurityDriver(virStorageSourcePtr src) +{ + int actualType = virStorageSourceGetActualType(src); + virStorageFileBackendPtr backend; + + if (!src) + return false; + + if (src->drv) { + backend = src->drv->backend; + } else { + if (!(backend = virStorageFileBackendForTypeInternal(actualType, + src->protocol, + false))) + return false; + } + + return !!backend->storageFileChown; +} + + void virStorageFileDeinit(virStorageSourcePtr src) { diff --git a/src/storage/storage_driver.h b/src/storage/storage_driver.h index eefd766..9592dd8 100644 --- a/src/storage/storage_driver.h +++ b/src/storage/storage_driver.h @@ -45,6 +45,8 @@ const char *virStorageFileGetUniqueIdentifier(virStorageSourcePtr src); int virStorageFileAccess(virStorageSourcePtr src, int mode); int virStorageFileChown(virStorageSourcePtr src, uid_t uid, gid_t gid); +bool virStorageFileSupportsSecurityDriver(virStorageSourcePtr src); + int virStorageFileGetMetadata(virStorageSourcePtr src, uid_t uid, gid_t gid, bool allow_probe) -- 2.0.0
On 07/22/2014 03:20 AM, Peter Krempa wrote:
With my intended use of storage driver assist to chown files on remote storage we will need a witness that will tell us whether the given storage volume supports operations needed by the storage driver. --- src/storage/storage_driver.c | 30 ++++++++++++++++++++++++++++++ src/storage/storage_driver.h | 2 ++ 2 files changed, 32 insertions(+)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 07/22/2014 05:20 AM, Peter Krempa wrote:
With my intended use of storage driver assist to chown files on remote storage we will need a witness that will tell us whether the given storage volume supports operations needed by the storage driver. --- src/storage/storage_driver.c | 30 ++++++++++++++++++++++++++++++ src/storage/storage_driver.h | 2 ++ 2 files changed, 32 insertions(+)
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index efd79b9..b397c6e 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -2357,6 +2357,36 @@ virStorageFileSupportsBackingChainTraversal(virStorageSourcePtr src) backend->storageFileAccess; }
+ +/** + * virStorageFileSupportsSecurityDriver: + * + * @src: a storage file structure + * + * Check if a storage file supports operations needed by the security + * driver to perform labelling */
NIT: Closing comment on new line. ACK John
When restoring security labels in the dac driver the code would resolve the file path and use the resolved one to be chown-ed. The setting code doesn't do that. Remove the unnecessary code. --- src/security/security_dac.c | 19 +------------------ 1 file changed, 1 insertion(+), 18 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 4d2a9d6..cdb2735 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -264,27 +264,10 @@ virSecurityDACSetOwnership(const char *path, uid_t uid, gid_t gid) static int virSecurityDACRestoreSecurityFileLabel(const char *path) { - struct stat buf; - int rc = -1; - char *newpath = NULL; - VIR_INFO("Restoring DAC user and group on '%s'", path); - if (virFileResolveLink(path, &newpath) < 0) { - virReportSystemError(errno, - _("cannot resolve symlink %s"), path); - goto err; - } - - if (stat(newpath, &buf) != 0) - goto err; - /* XXX record previous ownership */ - rc = virSecurityDACSetOwnership(newpath, 0, 0); - - err: - VIR_FREE(newpath); - return rc; + return virSecurityDACSetOwnership(path, 0, 0); } -- 2.0.0
On 07/22/2014 05:20 AM, Peter Krempa wrote:
When restoring security labels in the dac driver the code would resolve the file path and use the resolved one to be chown-ed. The setting code doesn't do that. Remove the unnecessary code. --- src/security/security_dac.c | 19 +------------------ 1 file changed, 1 insertion(+), 18 deletions(-)
Code has been there since it was first introduced by commit id '15f5eaa0'. I see the security_selinux.c does the same thing and it predates security_dac.c... Digging deeper finds commit id 'c86afc85e' which added searching for the path for some unknown/unlogged reason. One can only wonder why it was felt the restore needed link resolution, but yet not necessary on the set. While the Set functions don't necessarily have it, looking at all callers of virFileResolveLink() causes me to believe there was some reason (or some path) that perhaps didn't store the Resolved link and this code is just ensuring that. Perhaps Eric or DanB could chime in on this as the restore is a fairly generic path, while the set paths are usually fairly specific. Not 100% comfortable with giving an ACK John
diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 4d2a9d6..cdb2735 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -264,27 +264,10 @@ virSecurityDACSetOwnership(const char *path, uid_t uid, gid_t gid) static int virSecurityDACRestoreSecurityFileLabel(const char *path) { - struct stat buf; - int rc = -1; - char *newpath = NULL; - VIR_INFO("Restoring DAC user and group on '%s'", path);
- if (virFileResolveLink(path, &newpath) < 0) { - virReportSystemError(errno, - _("cannot resolve symlink %s"), path); - goto err; - } - - if (stat(newpath, &buf) != 0) - goto err; - /* XXX record previous ownership */ - rc = virSecurityDACSetOwnership(newpath, 0, 0); - - err: - VIR_FREE(newpath); - return rc; + return virSecurityDACSetOwnership(path, 0, 0); }
On 07/22/2014 03:20 AM, Peter Krempa wrote:
When restoring security labels in the dac driver the code would resolve the file path and use the resolved one to be chown-ed. The setting code doesn't do that. Remove the unnecessary code.
chown() on a symlink changes the underlying file, not the link itself; you need the BSD extension lchown() to change the owner of a symlink (and even then, changing the owner of a symlink seldom has any noticeable impact - per 'man 7 symlink' on Linux, "The only time that the ownership of a symbolic link matters is when the link is being removed or renamed in a directory that has the sticky bit set"). So resolving a symlink before chown()ing it is pointless, since chown() will resolve it anyways, and we really don't need to care about lchown(). Likewise, on Linux, chmod() cannot alter a symlink to anything other than a pointless 0777 access mode. BSD is a bit different - there, lchown() coupled with chmod() can be used to alter whether a user can resolve through the symlink in pathname resolution, depending on the mount parameters of the current file system. But this is still a seldom used extension to POSIX.
--- src/security/security_dac.c | 19 +------------------ 1 file changed, 1 insertion(+), 18 deletions(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
To integrate the security driver with the storage driver we need to pass a callback for a function that will chown storage volumes. Introduce and document the callback prototype. --- src/qemu/qemu_driver.c | 3 ++- src/security/security_dac.c | 9 +++++++++ src/security/security_dac.h | 3 +++ src/security/security_manager.c | 4 +++- src/security/security_manager.h | 19 ++++++++++++++++++- 5 files changed, 35 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index eae23d3..a5a9e0f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -374,7 +374,8 @@ qemuSecurityInit(virQEMUDriverPtr driver) cfg->allowDiskFormatProbing, cfg->securityDefaultConfined, cfg->securityRequireConfined, - cfg->dynamicOwnership))) + cfg->dynamicOwnership, + NULL))) goto error; if (!stack) { if (!(stack = virSecurityManagerNewStack(mgr))) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index cdb2735..1fb0c86 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -51,6 +51,7 @@ struct _virSecurityDACData { int ngroups; bool dynamicOwnership; char *baselabel; + virSecurityManagerDACChownCallback chownCallback; }; typedef struct _virSecurityDACCallbackData virSecurityDACCallbackData; @@ -87,6 +88,14 @@ virSecurityDACSetDynamicOwnership(virSecurityManagerPtr mgr, priv->dynamicOwnership = dynamicOwnership; } +void +virSecurityDACSetChownCallback(virSecurityManagerPtr mgr, + virSecurityManagerDACChownCallback chownCallback) +{ + virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); + priv->chownCallback = chownCallback; +} + /* returns 1 if label isn't found, 0 on success, -1 on error */ static int ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) diff --git a/src/security/security_dac.h b/src/security/security_dac.h index dbcf56f..846cefb 100644 --- a/src/security/security_dac.h +++ b/src/security/security_dac.h @@ -32,4 +32,7 @@ int virSecurityDACSetUserAndGroup(virSecurityManagerPtr mgr, void virSecurityDACSetDynamicOwnership(virSecurityManagerPtr mgr, bool dynamic); +void virSecurityDACSetChownCallback(virSecurityManagerPtr mgr, + virSecurityManagerDACChownCallback chownCallback); + #endif /* __VIR_SECURITY_DAC */ diff --git a/src/security/security_manager.c b/src/security/security_manager.c index 8a45e04..8671620 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -152,7 +152,8 @@ virSecurityManagerNewDAC(const char *virtDriver, bool allowDiskFormatProbing, bool defaultConfined, bool requireConfined, - bool dynamicOwnership) + bool dynamicOwnership, + virSecurityManagerDACChownCallback chownCallback) { virSecurityManagerPtr mgr = virSecurityManagerNewDriver(&virSecurityDriverDAC, @@ -170,6 +171,7 @@ virSecurityManagerNewDAC(const char *virtDriver, } virSecurityDACSetDynamicOwnership(mgr, dynamicOwnership); + virSecurityDACSetChownCallback(mgr, chownCallback); return mgr; } diff --git a/src/security/security_manager.h b/src/security/security_manager.h index 97b6a2e..156f882 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -25,6 +25,7 @@ # include "domain_conf.h" # include "vircommand.h" +# include "virstoragefile.h" typedef struct _virSecurityManager virSecurityManager; typedef virSecurityManager *virSecurityManagerPtr; @@ -39,13 +40,29 @@ virSecurityManagerPtr virSecurityManagerNewStack(virSecurityManagerPtr primary); int virSecurityManagerStackAddNested(virSecurityManagerPtr stack, virSecurityManagerPtr nested); +/** + * virSecurityManagerDACChownCallback: + * @src: Storage file to chown + * @uid: target uid + * @gid: target gid + * + * A function callback to chown image files described by the disk source struct + * @src. The callback shall return 0 on success, -1 on error and errno set (no + * libvirt error reported) OR -2 and a libvirt error reported. */ +typedef int +(*virSecurityManagerDACChownCallback)(virStorageSourcePtr src, + uid_t uid, + gid_t gid); + + virSecurityManagerPtr virSecurityManagerNewDAC(const char *virtDriver, uid_t user, gid_t group, bool allowDiskFormatProbing, bool defaultConfined, bool requireConfined, - bool dynamicOwnership); + bool dynamicOwnership, + virSecurityManagerDACChownCallback chownCallback); int virSecurityManagerPreFork(virSecurityManagerPtr mgr); void virSecurityManagerPostFork(virSecurityManagerPtr mgr); -- 2.0.0
On 07/22/2014 05:20 AM, Peter Krempa wrote:
To integrate the security driver with the storage driver we need to pass a callback for a function that will chown storage volumes.
Introduce and document the callback prototype.
ACK Although I'm still not sure I completely follow how or what role the cfg->user and cfg->group 'play'.... or if there needs to be a relationship with the chownCallback. John
--- src/qemu/qemu_driver.c | 3 ++- src/security/security_dac.c | 9 +++++++++ src/security/security_dac.h | 3 +++ src/security/security_manager.c | 4 +++- src/security/security_manager.h | 19 ++++++++++++++++++- 5 files changed, 35 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index eae23d3..a5a9e0f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -374,7 +374,8 @@ qemuSecurityInit(virQEMUDriverPtr driver) cfg->allowDiskFormatProbing, cfg->securityDefaultConfined, cfg->securityRequireConfined, - cfg->dynamicOwnership))) + cfg->dynamicOwnership, + NULL))) goto error; if (!stack) { if (!(stack = virSecurityManagerNewStack(mgr))) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index cdb2735..1fb0c86 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -51,6 +51,7 @@ struct _virSecurityDACData { int ngroups; bool dynamicOwnership; char *baselabel; + virSecurityManagerDACChownCallback chownCallback; };
typedef struct _virSecurityDACCallbackData virSecurityDACCallbackData; @@ -87,6 +88,14 @@ virSecurityDACSetDynamicOwnership(virSecurityManagerPtr mgr, priv->dynamicOwnership = dynamicOwnership; }
+void +virSecurityDACSetChownCallback(virSecurityManagerPtr mgr, + virSecurityManagerDACChownCallback chownCallback) +{ + virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); + priv->chownCallback = chownCallback; +} + /* returns 1 if label isn't found, 0 on success, -1 on error */ static int ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) diff --git a/src/security/security_dac.h b/src/security/security_dac.h index dbcf56f..846cefb 100644 --- a/src/security/security_dac.h +++ b/src/security/security_dac.h @@ -32,4 +32,7 @@ int virSecurityDACSetUserAndGroup(virSecurityManagerPtr mgr, void virSecurityDACSetDynamicOwnership(virSecurityManagerPtr mgr, bool dynamic);
+void virSecurityDACSetChownCallback(virSecurityManagerPtr mgr, + virSecurityManagerDACChownCallback chownCallback); + #endif /* __VIR_SECURITY_DAC */ diff --git a/src/security/security_manager.c b/src/security/security_manager.c index 8a45e04..8671620 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -152,7 +152,8 @@ virSecurityManagerNewDAC(const char *virtDriver, bool allowDiskFormatProbing, bool defaultConfined, bool requireConfined, - bool dynamicOwnership) + bool dynamicOwnership, + virSecurityManagerDACChownCallback chownCallback) { virSecurityManagerPtr mgr = virSecurityManagerNewDriver(&virSecurityDriverDAC, @@ -170,6 +171,7 @@ virSecurityManagerNewDAC(const char *virtDriver, }
virSecurityDACSetDynamicOwnership(mgr, dynamicOwnership); + virSecurityDACSetChownCallback(mgr, chownCallback);
return mgr; } diff --git a/src/security/security_manager.h b/src/security/security_manager.h index 97b6a2e..156f882 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -25,6 +25,7 @@
# include "domain_conf.h" # include "vircommand.h" +# include "virstoragefile.h"
typedef struct _virSecurityManager virSecurityManager; typedef virSecurityManager *virSecurityManagerPtr; @@ -39,13 +40,29 @@ virSecurityManagerPtr virSecurityManagerNewStack(virSecurityManagerPtr primary); int virSecurityManagerStackAddNested(virSecurityManagerPtr stack, virSecurityManagerPtr nested);
+/** + * virSecurityManagerDACChownCallback: + * @src: Storage file to chown + * @uid: target uid + * @gid: target gid + * + * A function callback to chown image files described by the disk source struct + * @src. The callback shall return 0 on success, -1 on error and errno set (no + * libvirt error reported) OR -2 and a libvirt error reported. */ +typedef int +(*virSecurityManagerDACChownCallback)(virStorageSourcePtr src, + uid_t uid, + gid_t gid); + + virSecurityManagerPtr virSecurityManagerNewDAC(const char *virtDriver, uid_t user, gid_t group, bool allowDiskFormatProbing, bool defaultConfined, bool requireConfined, - bool dynamicOwnership); + bool dynamicOwnership, + virSecurityManagerDACChownCallback chownCallback);
int virSecurityManagerPreFork(virSecurityManagerPtr mgr); void virSecurityManagerPostFork(virSecurityManagerPtr mgr);
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) { + 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; + } else { struct stat sb; - int chown_errno = errno; - if (stat(path, &sb) >= 0) { + if (!path) { + if (!src || !src->path) + return 0; + + if (!virStorageSourceIsLocalStorage(src)) + return 0; + + path = src->path; + } + + 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) { 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); +} + + +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); } -- 2.0.0
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); }
On 07/22/14 17:59, John Ferlan wrote:
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
Hmm, yes, if control reaches here with "path" equal to NULL, and ..
+ rc = priv->chownCallback(src, uid, gid);
... the chown callback fails to chown the file, then ...
+ + /* 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?
... yes this will try to *printf a NULL string. So I'll add an initialization of path to something sane in the branch using the callback as at that point the "path" variable will be used only to report errors. Our reporting of "path" in case of remote storage sucks anyways so I'm planning on adding a function that will format the path of the source for error messages. At that point I'll revisit this (maybe even today as the number of broken error messages I've seen in the last month is vast and I'm not helping it currently)
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.
I'll push this patch with "path" initialized to something sane and revisit it when tweaking the error messages for functions using virStorageSource struct.
John
+}
Peter
Use the storage driver to chown remote images. --- src/qemu/qemu_driver.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 47 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a5a9e0f..c0e6485 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -327,6 +327,52 @@ qemuAutostartDomains(virQEMUDriverPtr driver) virObjectUnref(cfg); } + +static int +qemuSecurityChownCallback(virStorageSourcePtr src, + uid_t uid, + gid_t gid) +{ + struct stat sb; + int save_errno = 0; + int ret = -1; + + if (!virStorageFileSupportsSecurityDriver(src)) + return 0; + + if (virStorageSourceIsLocalStorage(src)) { + /* use direct chmod for local files so that the file doesn't + * need to be initialized */ + if (stat(src->path, &sb) >= 0) { + if (sb.st_uid == uid && + sb.st_gid == gid) { + /* It's alright, there's nothing to change anyway. */ + return 0; + } + } + + return chown(src->path, uid, gid); + } + + /* storage file init reports errors, return -2 on failure */ + if (virStorageFileInit(src) < 0) + return -2; + + if (virStorageFileChown(src, uid, gid) < 0) { + save_errno = errno; + goto cleanup; + } + + ret = 0; + + cleanup: + virStorageFileDeinit(src); + errno = save_errno; + + return ret; +} + + static int qemuSecurityInit(virQEMUDriverPtr driver) { @@ -375,7 +421,7 @@ qemuSecurityInit(virQEMUDriverPtr driver) cfg->securityDefaultConfined, cfg->securityRequireConfined, cfg->dynamicOwnership, - NULL))) + qemuSecurityChownCallback))) goto error; if (!stack) { if (!(stack = virSecurityManagerNewStack(mgr))) -- 2.0.0
On 07/22/2014 05:21 AM, Peter Krempa wrote:
Use the storage driver to chown remote images. --- src/qemu/qemu_driver.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 47 insertions(+), 1 deletion(-)
ACK - essentially does what virSecurityDACSetOwnership did other than the Restore path which used to ResolveLink & stat() (e.g. patch 3) John
Add backends for this frontend function so that we can use it in the snapshot creation code. --- src/storage/storage_backend_fs.c | 17 +++++++++++++++++ src/storage/storage_backend_gluster.c | 15 +++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 2af5ab5..378c553 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -1387,6 +1387,22 @@ virStorageFileBackendFileInit(virStorageSourcePtr src) static int +virStorageFileBackendFileCreate(virStorageSourcePtr src) +{ + int fd = -1; + + if ((fd = virFileOpenAs(src->path, O_WRONLY | O_TRUNC | O_CREAT, 0, + src->drv->uid, src->drv->gid, 0)) < 0) { + errno = -fd; + return -1; + } + + VIR_FORCE_CLOSE(fd); + return 0; +} + + +static int virStorageFileBackendFileUnlink(virStorageSourcePtr src) { return unlink(src->path); @@ -1470,6 +1486,7 @@ virStorageFileBackend virStorageFileBackendFile = { .backendInit = virStorageFileBackendFileInit, .backendDeinit = virStorageFileBackendFileDeinit, + .storageFileCreate = virStorageFileBackendFileCreate, .storageFileUnlink = virStorageFileBackendFileUnlink, .storageFileStat = virStorageFileBackendFileStat, .storageFileReadHeader = virStorageFileBackendFileReadHeader, diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index 052f58d..38d02ac 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -634,6 +634,20 @@ virStorageFileBackendGlusterInit(virStorageSourcePtr src) static int +virStorageFileBackendGlusterCreate(virStorageSourcePtr src) +{ + virStorageFileBackendGlusterPrivPtr priv = src->drv->priv; + glfs_fd_t *fd = NULL; + + if (!(fd = glfs_open(priv->vol, src->path, O_CREAT | O_TRUNC | O_WRONLY))) + return -1; + + ignore_value(glfs_close(fd)); + return 0; +} + + +static int virStorageFileBackendGlusterUnlink(virStorageSourcePtr src) { virStorageFileBackendGlusterPrivPtr priv = src->drv->priv; @@ -780,6 +794,7 @@ virStorageFileBackend virStorageFileBackendGluster = { .backendInit = virStorageFileBackendGlusterInit, .backendDeinit = virStorageFileBackendGlusterDeinit, + .storageFileCreate = virStorageFileBackendGlusterCreate, .storageFileUnlink = virStorageFileBackendGlusterUnlink, .storageFileStat = virStorageFileBackendGlusterStat, .storageFileReadHeader = virStorageFileBackendGlusterReadHeader, -- 2.0.0
On 07/22/2014 05:21 AM, Peter Krempa wrote:
Add backends for this frontend function so that we can use it in the snapshot creation code. --- src/storage/storage_backend_fs.c | 17 +++++++++++++++++ src/storage/storage_backend_gluster.c | 15 +++++++++++++++ 2 files changed, 32 insertions(+)
I suppose this more or less answers my earlier question about how cfg->user and cfg->group get to play... Just have to wait long enough I guess. ACK John
Move the last operation done on local files to the storage driver API. --- src/qemu/qemu_driver.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c0e6485..f0cf1c6 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12855,7 +12855,6 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, char *source = NULL; const char *formatStr = NULL; int ret = -1; - int fd = -1; bool need_unlink = false; if (snap->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) { @@ -12873,7 +12872,7 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, if (virStorageSourceInitChainElement(newDiskSrc, disk->src, false) < 0) goto cleanup; - if (virStorageFileInit(newDiskSrc) < 0) + if (qemuDomainStorageFileInit(driver, vm, newDiskSrc) < 0) goto cleanup; if (qemuGetDriveSourceString(newDiskSrc, NULL, &source) < 0) @@ -12889,15 +12888,13 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, } /* pre-create the image file so that we can label it before handing it to qemu */ - /* XXX we should switch to storage driver based pre-creation of the image */ - if (virStorageSourceIsLocalStorage(newDiskSrc)) { - if (!reuse && newDiskSrc->type != VIR_STORAGE_TYPE_BLOCK) { - fd = qemuOpenFile(driver, vm, source, O_WRONLY | O_TRUNC | O_CREAT, - &need_unlink, NULL); - if (fd < 0) - goto cleanup; - VIR_FORCE_CLOSE(fd); + if (!reuse && newDiskSrc->type != VIR_STORAGE_TYPE_BLOCK) { + if (virStorageFileCreate(newDiskSrc) < 0) { + virReportSystemError(errno, _("failed to create image file '%s'"), + source); + goto cleanup; } + need_unlink = true; } /* set correct security, cgroup and locking options on the new image */ -- 2.0.0
On 07/22/2014 05:21 AM, Peter Krempa wrote:
Move the last operation done on local files to the storage driver API. --- src/qemu/qemu_driver.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-)
ACK John
On 07/22/14 18:30, John Ferlan wrote:
On 07/22/2014 05:21 AM, Peter Krempa wrote:
Move the last operation done on local files to the storage driver API. --- src/qemu/qemu_driver.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-)
ACK
John
I went through the series and fixed few small problems pointed out. I'll revisit one of the functions when I'll add a formatter for printing path of a virStorageSource struct. Thanks Peter
participants (3)
-
Eric Blake -
John Ferlan -
Peter Krempa