Re: [Qemu-trivial] [Qemu-devel] [PATCH] block: output more error messages if failed to create temporary snapshot

[Dunrong Huang]

2012/9/5 Kevin Wolf <address@hidden>:
> Am 05.09.2012 11:40, schrieb Stefan Hajnoczi:
>> On Wed, Sep 05, 2012 at 04:26:14PM +0800, address@hidden wrote:
>>> From: Dunrong Huang <address@hidden>
>>>
>>> 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 <address@hidden>
>>> ---
>>>  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