2012/9/5 Kevin Wolf <kw...@redhat.com>:
> Am 05.09.2012 11:40, schrieb Stefan Hajnoczi:
>> On Wed, Sep 05, 2012 at 04:26:14PM +0800, riegama...@gmail.com wrote:
>>> From: Dunrong Huang <riegama...@gmail.com>
>>>
>>> If we failed to create temporary snapshot, the error message did not match
>>> with the error, for example:
>>>
>>> $ TMPDIR=/tmp/bad_path qemu-system-x86_64 -enable-kvm debian.qcow2 -snapshot
>>> qemu-system-x86_64: -enable-kvm: could not open disk image
>>> /home/mathslinux/Images/debian.qcow2: No such file or directory
>>>
>>> Indeed, the file which cant be created is /tmp/bad_path/vl.xxxxxx, not
>>> debian.qcow2. so the error message makes users feel confused.
>>>
>>> Signed-off-by: Dunrong Huang <riegama...@gmail.com>
>>> ---
>>> block.c | 2 ++
>>> 1 个文件被修改，插入 2 行(+)
>>>
>>> diff --git a/block.c b/block.c
>>> index 470bdcc..c9e5140 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -434,6 +434,8 @@ int get_tmp_filename(char *filename, int size)
>>> }
>>> fd = mkstemp(filename);
>>> if (fd < 0 || close(fd)) {
>>> + fprintf(stderr, "Could not create temporary snapshot in %s
>>> directory: "
>>> + "%s\n", tmpdir, strerror(errno));
>>
>> The error message is fine for fd < 0 but not for close(0) != 0. Also,
>> close(2) is allowed to change errno (even on success) so this is not
>> portable and could clobber the errno value.
>
> I don't think this error message is fine in get_tmp_filename(). This
> function happens to be used for temporary snapshots, but this is not
> part of its interface. Today it's also used in vvfat and there the
> message would only be confusing. Other use cases may be introduced in
> the future.
Sorry, I forgot that get_tmp_filename() is a public interface.
>
> If you want to introduce an error message, do it in the caller.
>
> Kevin
--
Best Regards,
Dunrong Huang
