marcandre.lur...@redhat.com writes:
> From: Marc-André Lureau <marcandre.lur...@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.lur...@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 <arm...@redhat.com>
