| Message ID | 20220505081431.934739-8-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Misc cleanups | expand |
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; 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. > 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')",
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')", >
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); 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); 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')",