Re: [Qemu-trivial] [PATCH] block: Don't forget to delete temporary file

[Paolo Bonzini]

Il 05/09/2012 18:02, Markus Armbruster ha scritto:
> Paolo Bonzini <address@hidden> writes:
> 
>> Il 05/09/2012 15:26, address@hidden ha scritto:
>>> From: Dunrong Huang <address@hidden>
>>>
>>> The caller would not delete temporary file after failed get_tmp_filename().
>>>
>>> Signed-off-by: Dunrong Huang <address@hidden>
>>> ---
>>>  block.c | 6 +++++-
>>>  1 个文件被修改，插入 5 行(+)，删除 1 行(-)
>>>
>>> diff --git a/block.c b/block.c
>>> index 074987e..2bc9f75 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -433,7 +433,11 @@ int get_tmp_filename(char *filename, int size)
>>>          return -EOVERFLOW;
>>>      }
>>>      fd = mkstemp(filename);
>>> -    if (fd < 0 || close(fd)) {
>>> +    if (fd < 0) {
>>> +        return -errno;
>>> +    }
>>> +    if (close(fd) != 0) {
>>> +        unlink(filename);
>>>          return -errno;
>>>      }
>>>      return 0;
>>>
>>
>> Not necessary, mkstemp will not create a file if it returns an error.
> 
> Read the patch once more :)

Oops. :)

I wondered about that check for close() errors, perhaps an error in
close could just be swallowed?  Since we don't care about the content of
the file after close (it's empty), we also don't care about errors
closing it.

However, perhaps there were errors writing the directory entry...
Should any errors writing the directory entry be reported by open(),
i.e. by mkstemp()?   If not, and the dcache entry is evicted, someone
could create a different file with the same name as ours.  But then, not
even a successful close() guarantees that the data has reached the disk.

And finally, the whole get_tmp_filename is unsafe because there is a
race window between closing and reopening the file, if the directory is
writable and does not have the sticky bit.

So the patch is an improvement, but there is still something unpleasing
in this code...

Paolo