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

[John Ferlan]

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 at 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);
+        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