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')",
+ 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')",
+ (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')",
+ fd, path, mode);
+ }
- close(fd);
- if (oflag & O_CREAT) {
- unlink(path);
- }
+end:
+ if (f == NULL && fd != -1) {
+ close(fd);
+ if (oflag & O_CREAT) {
+ unlink(path);
}
}
-
- error_propagate(errp, local_err);
- return NULL;
+ return f;
}
int64_t qmp_guest_file_open(const char *path, bool has_mode, const char *mode,
--
2.36.1
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>
© 2016 - 2024 Red Hat, Inc.