Re: [PATCH v2 05/15] qga: flatten safe_open_or_create()

[Markus Armbruster <armbru@redhat.com>]

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()?

I'm not sure the commit reduces "branching", but it certainly reduces
nesting, which I find improves readability.

> 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;
  
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.

| +        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,