Re: [libvirt] [PATCH 08/12] storage: Cleanup failures virStorageBackendCreateExecCommand

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(a)redhat.com&gt; >> --- >> 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);

+ 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(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list