Re: [PATCH v4 4/6] util: introduce qemu_open and qemu_create with error reporting

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> qemu_open_old() works like open(): set errno and return -1 on failure.
> It has even more failure modes, though.  Reporting the error clearly
> to users is basically impossible for many of them.
>
> Our standard cure for "errno is too coarse" is the Error object.
> Introduce two new helper methods:
>
>   int qemu_open(const char *name, int flags, Error **errp);
>   int qemu_create(const char *name, int flags, mode_t mode, Error **errp);
>
> Note that with this design we no longer require or even accept the
> O_CREAT flag. Avoiding overloading the two distinct operations
> means we can avoid variable arguments which would prevent 'errp' from
> being the last argument. It also gives us a guarantee that the 'mode' is
> given when creating files, avoiding a latent security bug.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  include/qemu/osdep.h |  6 ++++++
>  util/osdep.c         | 21 +++++++++++++++++----
>  2 files changed, 23 insertions(+), 4 deletions(-)
>
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 18333e9006..13a821845b 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -497,7 +497,13 @@ int qemu_madvise(void *addr, size_t len, int advice);
>  int qemu_mprotect_rwx(void *addr, size_t size);
>  int qemu_mprotect_none(void *addr, size_t size);
>  
> +/*
> + * Don't introduce new usage of this function, prefer the following
> + * qemu_open/qemu_create that take an "Error **errp"
> + */
>  int qemu_open_old(const char *name, int flags, ...);
> +int qemu_open(const char *name, int flags, Error **errp);
> +int qemu_create(const char *name, int flags, mode_t mode, Error **errp);
>  int qemu_close(int fd);
>  int qemu_unlink(const char *name);
>  #ifndef _WIN32
> diff --git a/util/osdep.c b/util/osdep.c
> index 9c7118d3cb..a4956fbf6b 100644
> --- a/util/osdep.c
> +++ b/util/osdep.c
> @@ -344,10 +344,7 @@ qemu_open_internal(const char *name, int flags, mode_t 
> mode, Error **errp)
>  #endif /* ! O_CLOEXEC */
>  
>      if (ret == -1) {
> -        const char *action = "open";
> -        if (flags & O_CREAT) {
> -            action = "create";
> -        }
> +        const char *action = flags & O_CREAT ? "create" : "open";
>          error_setg_errno(errp, errno, "Could not %s '%s' flags 0x%x",
>                           action, name, flags);
>      }

Squash this hunk into PATCH 3, please.

> @@ -357,6 +354,22 @@ qemu_open_internal(const char *name, int flags, mode_t 
> mode, Error **errp)
>  }
>  
>  
> +int qemu_open(const char *name, int flags, Error **errp)
> +{
> +    assert(!(flags & O_CREAT));
> +
> +    return qemu_open_internal(name, flags, 0, errp);
> +}
> +
> +
> +int qemu_create(const char *name, int flags, mode_t mode, Error **errp)
> +{
> +    assert(!(flags & O_CREAT));
> +
> +    return qemu_open_internal(name, flags | O_CREAT, mode, errp);
> +}
> +
> +
>  int qemu_open_old(const char *name, int flags, ...)
>  {
>      va_list ap;

Other than that:
Reviewed-by: Markus Armbruster <armbru@redhat.com>