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

[Markus Armbruster]

Fabien Chouteau <address@hidden> writes:

> On 02/04/2013 11:34 AM, Markus Armbruster wrote:
>> Stefan Hajnoczi <address@hidden> writes:
>> 
>>> On Fri, Feb 01, 2013 at 06:13:51PM +0100, Fabien Chouteau wrote:
>>>>
>>>> Signed-off-by: Fabien Chouteau <address@hidden>
>>>> ---
>>>>  block.c |   13 ++++++++++---
>>>>  1 file changed, 10 insertions(+), 3 deletions(-)
>>>
>>> Hi Fabien,
>>> Please always CC address@hidden  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.