Re: [PATCH v3 04/15] qga: flatten safe_open_or_create()

[Markus Armbruster]

marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> There is a bit too much nesting in the function, this can be simplified
> a bit to improve readability.
>
> This also helps with the following error handling changes.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  qga/commands-posix.c | 122 ++++++++++++++++++++++---------------------
>  1 file changed, 62 insertions(+), 60 deletions(-)
>
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 69f209af87..15eb7cb77d 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -339,73 +339,75 @@ find_open_flag(const char *mode_str, Error **errp)
>  static FILE *
>  safe_open_or_create(const char *path, const char *mode, Error **errp)
>  {
> -    Error *local_err = NULL;
>      int oflag;
> +    int fd = -1;
> +    FILE *f = NULL;
> +
> +    oflag = find_open_flag(mode, errp);
> +    if (oflag < 0) {
> +        goto end;
> +    }
> +
> +    /* If the caller wants / allows creation of a new file, we implement it
> +     * with a two step process: open() + (open() / fchmod()).
> +     *
> +     * First we insist on creating the file exclusively as a new file. If
> +     * that succeeds, we're free to set any file-mode bits on it. (The
> +     * motivation is that we want to set those file-mode bits independently
> +     * of the current umask.)
> +     *
> +     * If the exclusive creation fails because the file already exists
> +     * (EEXIST is not possible for any other reason), we just attempt to
> +     * open the file, but in this case we won't be allowed to change the
> +     * file-mode bits on the preexistent file.
> +     *
> +     * The pathname should never disappear between the two open()s in
> +     * practice. If it happens, then someone very likely tried to race us.
> +     * In this case just go ahead and report the ENOENT from the second
> +     * open() to the caller.
> +     *
> +     * If the caller wants to open a preexistent file, then the first
> +     * open() is decisive and its third argument is ignored, and the second
> +     * open() and the fchmod() are never called.
> +     */
> +    fd = open(path, oflag | ((oflag & O_CREAT) ? O_EXCL : 0), 0);
> +    if (fd == -1 && errno == EEXIST) {
> +        oflag &= ~(unsigned)O_CREAT;
> +        fd = open(path, oflag);
> +    }
> +    if (fd == -1) {
> +        error_setg_errno(errp, errno,
> +                         "failed to open file '%s' "
> +                         "(mode: '%s')",

No need to split this string.

> +                         path, mode);
> +        goto end;
> +    }
>  
> -    oflag = find_open_flag(mode, &local_err);
> -    if (local_err == NULL) {
> -        int fd;
> -
> -        /* If the caller wants / allows creation of a new file, we implement 
> it
> -         * with a two step process: open() + (open() / fchmod()).
> -         *
> -         * First we insist on creating the file exclusively as a new file. If
> -         * that succeeds, we're free to set any file-mode bits on it. (The
> -         * motivation is that we want to set those file-mode bits 
> independently
> -         * of the current umask.)
> -         *
> -         * If the exclusive creation fails because the file already exists
> -         * (EEXIST is not possible for any other reason), we just attempt to
> -         * open the file, but in this case we won't be allowed to change the
> -         * file-mode bits on the preexistent file.
> -         *
> -         * The pathname should never disappear between the two open()s in
> -         * practice. If it happens, then someone very likely tried to race 
> us.
> -         * In this case just go ahead and report the ENOENT from the second
> -         * open() to the caller.
> -         *
> -         * If the caller wants to open a preexistent file, then the first
> -         * open() is decisive and its third argument is ignored, and the 
> second
> -         * open() and the fchmod() are never called.
> -         */
> -        fd = open(path, oflag | ((oflag & O_CREAT) ? O_EXCL : 0), 0);
> -        if (fd == -1 && errno == EEXIST) {
> -            oflag &= ~(unsigned)O_CREAT;
> -            fd = open(path, oflag);
> -        }
> +    qemu_set_cloexec(fd);
>  
> -        if (fd == -1) {
> -            error_setg_errno(&local_err, errno, "failed to open file '%s' "
> -                             "(mode: '%s')", path, mode);
> -        } else {
> -            qemu_set_cloexec(fd);
> +    if ((oflag & O_CREAT) && fchmod(fd, DEFAULT_NEW_FILE_MODE) == -1) {
> +        error_setg_errno(errp, errno,
> +                         "failed to set permission 0%03o on new file '%s' 
> (mode: '%s')",

But I'd split this one.

> +                         (unsigned)DEFAULT_NEW_FILE_MODE, path, mode);
> +        goto end;
> +    }
>  
> -            if ((oflag & O_CREAT) && fchmod(fd, DEFAULT_NEW_FILE_MODE) == 
> -1) {
> -                error_setg_errno(&local_err, errno, "failed to set 
> permission "
> -                                 "0%03o on new file '%s' (mode: '%s')",
> -                                 (unsigned)DEFAULT_NEW_FILE_MODE, path, 
> mode);
> -            } else {
> -                FILE *f;
> -
> -                f = fdopen(fd, mode);
> -                if (f == NULL) {
> -                    error_setg_errno(&local_err, errno, "failed to associate 
> "
> -                                     "stdio stream with file descriptor %d, "
> -                                     "file '%s' (mode: '%s')", fd, path, 
> mode);
> -                } else {
> -                    return f;
> -                }
> -            }
> +    f = fdopen(fd, mode);
> +    if (f == NULL) {
> +        error_setg_errno(errp, errno,
> +                         "failed to associate stdio stream with file 
> descriptor %d, "
> +                         "file '%s' (mode: '%s')",

And I'd split this one like

                            "failed to associate stdio stream with"
                            " file descriptor %d, file '%s' (mode: '%s')",

> +                         fd, path, mode);
> +    }
>  
> -            close(fd);
> -            if (oflag & O_CREAT) {
> -                unlink(path);
> -            }
> +end:

Cases here:

1. find_open_flag() or open() failed (first two goto):
   @fd is -1, @f is NULL

2. open() succeeded, fchmod() failed (third goto), or
   open() succeeded, fdopen() failed (fall through):
   @fd is open, @f is NULL

3. open() and fdopen() succeeded:
   @fd and @f are open

> +    if (f == NULL && fd != -1) {

This is case 2.

> +        close(fd);
> +        if (oflag & O_CREAT) {
> +            unlink(path);
>          }
>      }
> -
> -    error_propagate(errp, local_err);
> -    return NULL;
> +    return f;

Works.

Case 1 could return NULL instead of goto end.  The condition for
close(fd) then becomes if (!f).  Feels simpler to me, but I guess it's a
matter of taste.

>  }
>  
>  int64_t qmp_guest_file_open(const char *path, bool has_mode, const char 
> *mode,

None of the above is serious, so
Reviewed-by: Markus Armbruster <armbru@redhat.com>