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

[John Ferlan]

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

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