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

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

[riegamaths]

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));
         return -errno;
     }
     return 0;
-- 
1.7.12

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

[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.

Please split into:

if (fd < 0) {
    ...
}
if (close(fd) != 0) {
    ...
}

Stefan

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

[Kevin Wolf]

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

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

[Dunrong Huang]

2012/9/5 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.
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