On 10/13/2015 03:11 PM, John Ferlan wrote:
On 10/13/2015 08:50 AM, Peter Krempa wrote:
On Fri, Oct 09, 2015 at 09:34:07 -0400, John Ferlan wrote:
After a successful qemu-img/qcow-create of the backing file, if we fail to stat the file, change it owner/group, or mode, then the cleanup path should delete the file.
Also moved the virCommandSetUID/virCommandSetGID inside the condition used to actually run the command rather than randomly setting and not using it if the file had been created. The 'cmd' buffer is only used if we need to create.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index a375fe0..7d0de63 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c
...
@@ -737,6 +740,8 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, ret = 0;
cleanup: + if (ret < 0 && filecreated) + unlink(vol->target.path);
This might not work if the volume was created with different uid/gid as the process that is attempting to delete it (in case of e.g. NFS.).
Ugh... this function was really strange especially that chown/chmod after a create on an NETFS target. The net result if the first pile of 'ifs' passes is a creation in a NETFS pool using either 'qemu-img' or 'qcow-create' under the uid/gid from the vol->target.{uid|gid}. So yes, we'd have to virFileRemove that.
The other creation would able to unlink directly, although I suppose a revector into virFileRemove would work, although it'd be passing uid, gid == -1, -1.
I know you don't necessarily prefer inline diffs, but this one's fairly short:
- if (ret < 0 && filecreated) - unlink(vol->target.path); + if (ret < 0 && filecreated) { + if (netfs) + virFileDelete(vol->target.path, vol->target.uid, vol->target.gid);
virFileRemove(vol->target.path, vol->target.perms->uid, vol->target.perms->gid); What I get for typing and not compiling ;-) John
+ else + unlink(vol->target.path); + }
where 'netfs' is a new bool set when we create in that first if
Does this look more reasonable?
John
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list