Hi
On Thu, May 5, 2022 at 3:20 PM Markus Armbruster <arm...@redhat.com> wrote:
> marcandre.lur...@redhat.com writes:
>
> > From: Marc-André Lureau <marcandre.lur...@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.lur...@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,
>
>
>
--
Marc-André Lureau
