marcandre.lureau@redhat.com writes:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> There is a bit too much branching in the function, this can be
> simplified a bit, and have a common exit point thanks to ERRP_PROPAGATE.
Do you mean ERRP_GUARD()?
yes
I'm not sure the commit reduces "branching", but it certainly reduces
nesting, which I find improves readability.
ok
> This also helps with the following error handling changes.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> qga/commands-posix.c | 124 ++++++++++++++++++++++---------------------
> 1 file changed, 63 insertions(+), 61 deletions(-)
I think the diff is easier to review with space change ignored:
| diff --git a/qga/commands-posix.c b/qga/commands-posix.c
| index 78f2f21001..b4b19d3eb4 100644
| --- a/qga/commands-posix.c
| +++ b/qga/commands-posix.c
| @@ -315,12 +315,14 @@ 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;
| + ERRP_GUARD();
| + int oflag, fd = -1;
I'd prefer
+ ERRP_GUARD();
int oflag;
+ int fd = -1;
ok
because it's slightly less churn, and I dislike variables with and
without initializer in the same declaration. Matter of taste.
| + FILE *f = NULL;
|
| - oflag = find_open_flag(mode, &local_err);
| - if (local_err == NULL) {
| - int fd;
| + oflag = find_open_flag(mode, errp);
| + if (*errp) {
I'd prefer
if (oflag < 0) {
No need for ERRP_GUARD() then, as far as I can tell.
"qga: use qemu_open_cloexec() for safe_open_or_create()" expects it to be non-null though, I can move it there.
| + goto end;
| + }
|
| /* If the caller wants / allows creation of a new file, we implement it
| * with a two step process: open() + (open() / fchmod()).
| @@ -349,39 +351,39 @@ safe_open_or_create(const char *path, const char *mode, Error **errp)
| oflag &= ~(unsigned)O_CREAT;
| fd = open(path, oflag);
| }
| -
| if (fd == -1) {
| - error_setg_errno(&local_err, errno, "failed to open file '%s' "
| - "(mode: '%s')", path, mode);
| - } else {
| + error_setg_errno(errp, errno,
| + "failed to open file '%s' "
| + "(mode: '%s')",
| + path, mode);
| + goto end;
| + }
| +
| qemu_set_cloexec(fd);
|
| 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')",
| + error_setg_errno(errp, errno,
| + "failed to set permission 0%03o on new file '%s' (mode: '%s')",
| (unsigned)DEFAULT_NEW_FILE_MODE, path, mode);
| - } else {
| - FILE *f;
| + goto end;
| + }
|
| 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;
| - }
| + error_setg_errno(errp, errno,
| + "failed to associate stdio stream with file descriptor %d, "
| + "file '%s' (mode: '%s')",
| + fd, path, mode);
| }
|
| +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,