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

[Kevin Wolf <kwolf@redhat.com>]

Am 05.09.2012 11:40, schrieb Stefan Hajnoczi:
> On Wed, Sep 05, 2012 at 04:26:14PM +0800, riegamaths@gmail.com wrote:
>> From: Dunrong Huang <riegamaths@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 <riegamaths@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.

If you want to introduce an error message, do it in the caller.

Kevin