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

[Markus Armbruster]

I agree with you that the existing error reporting is too unspecific in
many cases, and I applaud your attempt to do something about it, but I'm
afraid this patch creates as many problems as it solves.  Details below.

Fabien Chouteau <address@hidden> writes:

> Signed-off-by: Fabien Chouteau <address@hidden>
> ---
>  block.c |   15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/block.c b/block.c
> index ba67c0d..79fe01b 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) {
> +        error_report("%s: GetTempPath() error: %d\n", __func__, 
> GetLastError());
> +        return -GetLastError();
> +    }
> +    if (GetTempFileName(temp_dir, "qem", 0, filename) == 0) {
> +        error_report("%s: GetTempFileName(%s) error: %d\n", __func__, 
> temp_dir,
> +                GetLastError());
> +        return -GetLastError();
> +    }
> +    return 0;
>  #else
>      int fd;
>      const char *tmpdir;
> @@ -442,9 +449,11 @@ int get_tmp_filename(char *filename, int size)
>      }
>      fd = mkstemp(filename);
>      if (fd < 0) {
> +        error_report("%s: mkstemp() error: %s\n", __func__, strerror(errno));
>          return -errno;
>      }
>      if (close(fd) != 0) {
> +        error_report("%s: close() error: %s\n", __func__, strerror(errno));
>          unlink(filename);
>          return -errno;
>      }

In my review of v1, I wrote "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 [...] have to
demonstrate that all callers are happy with the change of contract."  So
let's check the two callers of get_tmp_filename():

1. bdrv_open()

   Complex function, can fail in many ways.  Returns an error code.
   Does not report errors; that's left to its callers.

   Your patch effectively changes bdrv_open() to report the error in one
   of its failure modes.

   For callers that report bdrv_open() failure to the user, we then get
   two error messages: the one you add, followed by a less specific one
   from further up the call chain.  Reporting the same error multiple
   times is not nice.

   For callers that neglect to report bdrv_open() failure to the user
   even though they should (if such buggy callers exist), you fix the
   problem for one failure mode only.

   For callers that handle bdrv_open() failure without reporting it to
   the user, you add an unwanted error message.  Not good.  You haven't
   demonstrated that no such callers exist.

2. vvfat.c's enable_write_target()

   bdrv_vvfat's bdrv_file_open() method vvfat_open() is the only caller.
   It's called by bdrv_open() (covered by 1.), and by bdrv_file_open().
   Like bdrv_open(), bdrv_file_open() returns an error code, and leaves
   error reporting to its caller.  Same issues as above.

Apart from these fundamental gaps, the new error message needs polish.
Say mkstemp() fails ENOSPC.  Gets reported roughly like this:

    qemu-system-x86_64: -drive file=f16.img: get_tmp_filename: mkstemp() error: 
No space left on device
    qemu-system-x86_64: -drive file=f16.img: could not open disk image f16.img: 
No space left on device

The second message talks to the user in user terms.  That's proper.  The
first one talks source code instead.  From a user's point of view,
"get_tmp_filename" and "mkstemp() error" are gobbledygook.  At best,
they can help him guessing what the problem might be.