Re: [PATCH v2 07/15] qga: use qemu_open_cloexec() for safe_open_or_create()

[Marc-André Lureau]

Hi

On Thu, May 5, 2022 at 1:33 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> marcandre.lureau@redhat.com writes:
>
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > The function takes care of setting CLOEXEC, and reporting error.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  qga/commands-posix.c | 11 +++--------
> >  1 file changed, 3 insertions(+), 8 deletions(-)
> >
> > diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> > index 0ef049650e31..8ebc327c5e02 100644
> > --- a/qga/commands-posix.c
> > +++ b/qga/commands-posix.c
> > @@ -370,21 +370,16 @@ safe_open_or_create(const char *path, const char 
> > *mode, Error **errp)
> >       * 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);
> > +    fd = qemu_open_cloexec(path, oflag | ((oflag & O_CREAT) ? O_EXCL : 0), 
> > 0, errp);
> >      if (fd == -1 && errno == EEXIST) {
> > +        g_clear_pointer(errp, error_free);
>
> Aha, this is where you really need ERRP_GUARD().
>
> Elsewhere, we use
>
>            error_free(errp);
>            errp = NULL;
>

More like:

        error_free(*errp);
        *errp = NULL;

compared to:

       g_clear_pointer(errp, error_free);

I have a preference ;) but I will switch to the former for now.

> If one of these two ways is superior, we should use the superior one
> everywhere.
>
> If it's a wash, we should stick to the prevalent way.
>
> >          oflag &= ~(unsigned)O_CREAT;
> > -        fd = open(path, oflag);
> > +        fd = qemu_open_cloexec(path, oflag, 0, errp);
> >      }
> >      if (fd == -1) {
> > -        error_setg_errno(errp, errno,
> > -                         "failed to open file '%s' "
> > -                         "(mode: '%s')",
> > -                         path, mode);
>
> This changes the error message, doesn't it?
>
> Not an objection, just something that might be worth noting in the
> commit message.
>

ok

> >          goto end;
> >      }
> >
> > -    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')",
>