[Qemu-trivial] [PATCH] get_tmp_filename: add explicit error message

[Qemu-trivial] [PATCH] get_tmp_filename: add explicit error message

[Fabien Chouteau]

Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
---
 block.c |   13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/block.c b/block.c
index ba67c0d..3bf8eda 100644
--- a/block.c
+++ b/block.c
@@ -428,9 +428,16 @@ int get_tmp_filename(char *filename, int size)
     /* GetTempFileName requires that its output buffer (4th param)
        have length MAX_PATH or greater.  */
     assert(size >= MAX_PATH);
-    return (GetTempPath(MAX_PATH, temp_dir)
-            && GetTempFileName(temp_dir, "qem", 0, filename)
-            ? 0 : -GetLastError());
+    if (GetTempPath(MAX_PATH, temp_dir) == 0) {
+        fprintf(stderr, "GetTempPath() error: %d\n", GetLastError());
+        return -GetLastError();
+    }
+    if (GetTempFileName(temp_dir, "qem", 0, filename) == 0) {
+        fprintf(stderr, "GetTempFileName(%s) error: %d\n", temp_dir,
+                GetLastError());
+        return -GetLastError();
+    }
+    return 0;
 #else
     int fd;
     const char *tmpdir;
-- 
1.7.9.5

Re: [Qemu-trivial] [PATCH] get_tmp_filename: add explicit error message

[Stefan Hajnoczi]

On Fri, Feb 01, 2013 at 06:13:51PM +0100, Fabien Chouteau wrote:
> 
> Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
> ---
>  block.c |   13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)

Hi Fabien,
Please always CC qemu-devel@nongnu.org.  All patches must be on
qemu-devel so that the community can review them - not everyone
subscribes to qemu-trivial.

Thanks,
Stefan

> diff --git a/block.c b/block.c
> index ba67c0d..3bf8eda 100644
> --- a/block.c
> +++ b/block.c
> @@ -428,9 +428,16 @@ int get_tmp_filename(char *filename, int size)
>      /* GetTempFileName requires that its output buffer (4th param)
>         have length MAX_PATH or greater.  */
>      assert(size >= MAX_PATH);
> -    return (GetTempPath(MAX_PATH, temp_dir)
> -            && GetTempFileName(temp_dir, "qem", 0, filename)
> -            ? 0 : -GetLastError());
> +    if (GetTempPath(MAX_PATH, temp_dir) == 0) {
> +        fprintf(stderr, "GetTempPath() error: %d\n", GetLastError());
> +        return -GetLastError();
> +    }
> +    if (GetTempFileName(temp_dir, "qem", 0, filename) == 0) {
> +        fprintf(stderr, "GetTempFileName(%s) error: %d\n", temp_dir,
> +                GetLastError());
> +        return -GetLastError();
> +    }
> +    return 0;
>  #else
>      int fd;
>      const char *tmpdir;
> -- 
> 1.7.9.5
> 

Re: [Qemu-trivial] [Qemu-devel] [PATCH] get_tmp_filename: add explicit error message

[Markus Armbruster]

Stefan Hajnoczi <stefanha@redhat.com> writes:

> On Fri, Feb 01, 2013 at 06:13:51PM +0100, Fabien Chouteau wrote:
>> 
>> Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
>> ---
>>  block.c |   13 ++++++++++---
>>  1 file changed, 10 insertions(+), 3 deletions(-)
>
> Hi Fabien,
> Please always CC qemu-devel@nongnu.org.  All patches must be on
> qemu-devel so that the community can review them - not everyone
> subscribes to qemu-trivial.
>
> Thanks,
> Stefan
>
>> diff --git a/block.c b/block.c
>> index ba67c0d..3bf8eda 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -428,9 +428,16 @@ int get_tmp_filename(char *filename, int size)
>>      /* GetTempFileName requires that its output buffer (4th param)
>>         have length MAX_PATH or greater.  */
>>      assert(size >= MAX_PATH);
>> -    return (GetTempPath(MAX_PATH, temp_dir)
>> -            && GetTempFileName(temp_dir, "qem", 0, filename)
>> -            ? 0 : -GetLastError());
>> +    if (GetTempPath(MAX_PATH, temp_dir) == 0) {
>> +        fprintf(stderr, "GetTempPath() error: %d\n", GetLastError());
>> +        return -GetLastError();
>> +    }
>> +    if (GetTempFileName(temp_dir, "qem", 0, filename) == 0) {
>> +        fprintf(stderr, "GetTempFileName(%s) error: %d\n", temp_dir,
>> +                GetLastError());
>> +        return -GetLastError();
>> +    }
>> +    return 0;
>>  #else
>>      int fd;
>>      const char *tmpdir;

get_tmp_filename() is not supposed to print to stderr, that's the
caller's job.

Re: [Qemu-trivial] [Qemu-devel] [PATCH] get_tmp_filename: add explicit error message

[Fabien Chouteau]

On 02/04/2013 11:34 AM, Markus Armbruster wrote:
> Stefan Hajnoczi <stefanha@redhat.com> writes:
> 
>> On Fri, Feb 01, 2013 at 06:13:51PM +0100, Fabien Chouteau wrote:
>>>
>>> Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
>>> ---
>>>  block.c |   13 ++++++++++---
>>>  1 file changed, 10 insertions(+), 3 deletions(-)
>>
>> Hi Fabien,
>> Please always CC qemu-devel@nongnu.org.  All patches must be on
>> qemu-devel so that the community can review them - not everyone
>> subscribes to qemu-trivial.
>>
>> Thanks,
>> Stefan
>>
>>> diff --git a/block.c b/block.c
>>> index ba67c0d..3bf8eda 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -428,9 +428,16 @@ int get_tmp_filename(char *filename, int size)
>>>      /* GetTempFileName requires that its output buffer (4th param)
>>>         have length MAX_PATH or greater.  */
>>>      assert(size >= MAX_PATH);
>>> -    return (GetTempPath(MAX_PATH, temp_dir)
>>> -            && GetTempFileName(temp_dir, "qem", 0, filename)
>>> -            ? 0 : -GetLastError());
>>> +    if (GetTempPath(MAX_PATH, temp_dir) == 0) {
>>> +        fprintf(stderr, "GetTempPath() error: %d\n", GetLastError());
>>> +        return -GetLastError();
>>> +    }
>>> +    if (GetTempFileName(temp_dir, "qem", 0, filename) == 0) {
>>> +        fprintf(stderr, "GetTempFileName(%s) error: %d\n", temp_dir,
>>> +                GetLastError());
>>> +        return -GetLastError();
>>> +    }
>>> +    return 0;
>>>  #else
>>>      int fd;
>>>      const char *tmpdir;
>
> get_tmp_filename() is not supposed to print to stderr, that's the
> caller's job.
>

Why? The caller doesn't know the difference between Windows/Linux
implementation. And the error handling would have to be duplicated.

It's not the first time I add error output in Windows code. Specially in
block/, when there's an error, the only thing you get is: "operation not
permitted". It's not very helpful and you have to dig in the code to
find which function failed.

-- 
Fabien Chouteau

Re: [Qemu-trivial] [Qemu-devel] [PATCH] get_tmp_filename: add explicit error message

[Markus Armbruster]

Fabien Chouteau <chouteau@adacore.com> writes:

> On 02/04/2013 11:34 AM, Markus Armbruster wrote:
>> Stefan Hajnoczi <stefanha@redhat.com> writes:
>> 
>>> On Fri, Feb 01, 2013 at 06:13:51PM +0100, Fabien Chouteau wrote:
>>>>
>>>> Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
>>>> ---
>>>>  block.c |   13 ++++++++++---
>>>>  1 file changed, 10 insertions(+), 3 deletions(-)
>>>
>>> Hi Fabien,
>>> Please always CC qemu-devel@nongnu.org.  All patches must be on
>>> qemu-devel so that the community can review them - not everyone
>>> subscribes to qemu-trivial.
>>>
>>> Thanks,
>>> Stefan
>>>
>>>> diff --git a/block.c b/block.c
>>>> index ba67c0d..3bf8eda 100644
>>>> --- a/block.c
>>>> +++ b/block.c
>>>> @@ -428,9 +428,16 @@ int get_tmp_filename(char *filename, int size)
>>>>      /* GetTempFileName requires that its output buffer (4th param)
>>>>         have length MAX_PATH or greater.  */
>>>>      assert(size >= MAX_PATH);
>>>> -    return (GetTempPath(MAX_PATH, temp_dir)
>>>> -            && GetTempFileName(temp_dir, "qem", 0, filename)
>>>> -            ? 0 : -GetLastError());
>>>> +    if (GetTempPath(MAX_PATH, temp_dir) == 0) {
>>>> +        fprintf(stderr, "GetTempPath() error: %d\n", GetLastError());
>>>> +        return -GetLastError();
>>>> +    }
>>>> +    if (GetTempFileName(temp_dir, "qem", 0, filename) == 0) {
>>>> +        fprintf(stderr, "GetTempFileName(%s) error: %d\n", temp_dir,
>>>> +                GetLastError());
>>>> +        return -GetLastError();
>>>> +    }
>>>> +    return 0;
>>>>  #else
>>>>      int fd;
>>>>      const char *tmpdir;
>>
>> get_tmp_filename() is not supposed to print to stderr, that's the
>> caller's job.
>>
>
> Why? The caller doesn't know the difference between Windows/Linux
> implementation. And the error handling would have to be duplicated.

The function's (implied) contract is to return an error code without
printing anything.  If you want to change the contract to include
reporting the error, you need to implement it both for both arms of the
#ifdef.  You also have to demonstrate that all callers are happy with
the change of contract.

Regardless, printing to stderr is wrong.  The function can be called on
behalf of a monitor command, and then the error needs to be printed to
the correct monitor.  error_report() can do that for you, and more.

> It's not the first time I add error output in Windows code. Specially in
> block/, when there's an error, the only thing you get is: "operation not
> permitted". It's not very helpful and you have to dig in the code to
> find which function failed.

Good error reporting is hard.  Knowledge about the error and its context
gets lost as you move up the call chain.  Knowledge about how to report
errors gets lost as you move down.

Re: [Qemu-trivial] [Qemu-devel] [PATCH] get_tmp_filename: add explicit error message

[Fabien Chouteau]

On 02/04/2013 01:24 PM, Markus Armbruster wrote:
> Fabien Chouteau <chouteau@adacore.com> writes:
> 
>> On 02/04/2013 11:34 AM, Markus Armbruster wrote:
>>
>> Why? The caller doesn't know the difference between Windows/Linux
>> implementation. And the error handling would have to be duplicated.
> 
> The function's (implied) contract is to return an error code without
> printing anything.  If you want to change the contract to include
> reporting the error, you need to implement it both for both arms of the
> #ifdef.  You also have to demonstrate that all callers are happy with
> the change of contract.
> 
> Regardless, printing to stderr is wrong.  The function can be called on
> behalf of a monitor command, and then the error needs to be printed to
> the correct monitor.  error_report() can do that for you, and more.
> 

Alright, so I will use error_report() and do it for both Linux and Windows.

>> It's not the first time I add error output in Windows code. Specially in
>> block/, when there's an error, the only thing you get is: "operation not
>> permitted". It's not very helpful and you have to dig in the code to
>> find which function failed.
> 
> Good error reporting is hard.  Knowledge about the error and its context
> gets lost as you move up the call chain.  Knowledge about how to report
> errors gets lost as you move down.
> 

You're right, and in my opinion, no error reporting is the worst case.

-- 
Fabien Chouteau

Re: [Qemu-trivial] [Qemu-devel] [PATCH] get_tmp_filename: add explicit error message

[Andreas Färber]

Am 04.02.2013 14:33, schrieb Fabien Chouteau:
> On 02/04/2013 01:24 PM, Markus Armbruster wrote:
>>
>> Good error reporting is hard.  Knowledge about the error and its context
>> gets lost as you move up the call chain.  Knowledge about how to report
>> errors gets lost as you move down.
>>
> 
> You're right, and in my opinion, no error reporting is the worst case.

The best solution would be to pass an Error **errp argument and use
error_setg() on it, as done for visitors and QOM. Then the caller can
decide whether to pass NULL and the callee can report detailed errors.
Invasive change obviously and thus not suitable for 1.4.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg