From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:54592) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T9IOE-0006c8-0D for qemu-devel@nongnu.org; Wed, 05 Sep 2012 12:23:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1T9IO4-00025H-G2 for qemu-devel@nongnu.org; Wed, 05 Sep 2012 12:23:41 -0400 Sender: Paolo Bonzini Message-ID: <50477C74.7050606@redhat.com> Date: Wed, 05 Sep 2012 18:23:16 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1346851582-9296-1-git-send-email-riegamaths@gmail.com> <50477269.8050807@redhat.com> <87d320e9uk.fsf@blackfin.pond.sub.org> In-Reply-To: <87d320e9uk.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH] block: Don't forget to delete temporary file List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: qemu-trivial , Eric Blake , qemu-devel , riegamaths@gmail.com Il 05/09/2012 18:02, Markus Armbruster ha scritto: > Paolo Bonzini writes: > >> Il 05/09/2012 15:26, riegamaths@gmail.com ha scritto: >>> From: Dunrong Huang >>> >>> The caller would not delete temporary file after failed get_tmp_filename(). >>> >>> Signed-off-by: Dunrong Huang >>> --- >>> 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