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@xxxxxxxxxx> >>> --- >>> 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@xxxxxxxxxx > https://www.redhat.com/mailman/listinfo/libvir-list > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list