[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 07/15] qga: use qemu_open_cloexec() for safe_open_or_creat
From: |
Marc-André Lureau |
Subject: |
Re: [PATCH v2 07/15] qga: use qemu_open_cloexec() for safe_open_or_create() |
Date: |
Fri, 13 May 2022 12:02:26 +0200 |
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')",
>
- [PATCH v2 00/15] Misc cleanups, marcandre . lureau, 2022/05/05
- [PATCH v2 01/15] include: move qemu_*_exec_dir() to cutils, marcandre . lureau, 2022/05/05
- [PATCH v2 02/15] util/win32: simplify qemu_get_local_state_dir(), marcandre . lureau, 2022/05/05
- [PATCH v2 03/15] tests: make libqmp buildable for win32, marcandre . lureau, 2022/05/05
- [PATCH v2 04/15] include: adjust header guards after renaming, marcandre . lureau, 2022/05/05
- [PATCH v2 07/15] qga: use qemu_open_cloexec() for safe_open_or_create(), marcandre . lureau, 2022/05/05
- [PATCH v2 05/15] qga: flatten safe_open_or_create(), marcandre . lureau, 2022/05/05
- [PATCH v2 06/15] osdep: export qemu_open_cloexec(), marcandre . lureau, 2022/05/05
- [PATCH v2 08/15] qga: throw an Error in ga_channel_open(), marcandre . lureau, 2022/05/05
- [PATCH v2 09/15] qga: replace qemu_open_old() with qemu_open_cloexec(), marcandre . lureau, 2022/05/05