Hi
On Thu, May 5, 2022 at 1:33 PM Markus Armbruster <arm...@redhat.com> wrote:
>
> marcandre.lur...@redhat.com writes:
>
> > From: Marc-André Lureau <marcandre.lur...@redhat.com>
> >
> > The function takes care of setting CLOEXEC, and reporting error.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lur...@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')",
>
