Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use cloexec in more places #44

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

use cloexec in more places #44

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ea3f399
use CLOEXEC
vapier Jun 1, 2017
4057792
update gitignore files
vapier Jun 1, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ Makefile
Makefile.in
aclocal.m4
autom4te.cache
compile
config.cache
config.guess
config.h
config.h.in
Expand All @@ -10,6 +12,7 @@ config.status
config.sub
configure
depcomp
INSTALL
install-sh
libtool
ltmain.sh
Expand Down
5 changes: 5 additions & 0 deletions m4/.gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
/libtool.m4
/ltoptions.m4
/ltsugar.m4
/ltversion.m4
/lt~obsolete.m4
1 change: 1 addition & 0 deletions rfc3195/.gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
/liblogging-rfc3195.pc
5 changes: 5 additions & 0 deletions stdlog/.gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
/liblogging-stdlog.pc
/stdlog.3
/stdlogctl
/stdlogctl.1
/tester
2 changes: 1 addition & 1 deletion stdlog/file.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ static void
file_open(stdlog_channel_t ch)
{
if (ch->d.file.fd == -1) {
if((ch->d.file.fd = open(ch->d.file.name, O_WRONLY|O_CREAT|O_APPEND, 0660)) < 0)
if((ch->d.file.fd = open(ch->d.file.name, O_WRONLY|O_CREAT|O_APPEND|O_CLOEXEC, 0660)) < 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prior to this change I could fork off a child worker process (or multiple processes) and it/they could continue to log with this channel. After this change, the child(ren) will not be able to log and log() calls will simply return -1. Is it thought that the caller of stdlog_log() should implement retry logic that includes re-opening the channel with -1 is returned? The man page doesn't mention that. If that isn't (current) expected (or practiced) behavior, I think this is a breaking change.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CLOEXEC affects fd handling across execs, not across forks. so if you have an app using liblogging and it forks and does its own processing, all of that continues to work. what doesn't work is if you tried to exec a program and that program tried to write to the fd directly (because the handle would be closed automatically by the kernel). although that design pattern is kind of broken in the first place as how would that program know what fd to even write to in the first place ?

Copy link
Contributor

@dabright dabright Jun 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course it does! Silly me, what was I thinking? Kindly proceed to commit!

return;
}
}
Expand Down
8 changes: 8 additions & 0 deletions stdlog/stdlog-intern.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,14 @@
#include <sys/un.h>
#include "stdlog.h"

#ifndef O_CLOEXEC
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i didn't really see a better place to stuff these fallback defines, but i'm not super familiar with the codebase ...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the code used on any system that does not define O_CLOEXEC?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, if these flags are not defined then I think rather than defining them to be no-op flags it would be better to call fcntl() to set the close-on-exec flag as a more complete fallback solution.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't know about usage on other platforms, but it's also a common fallback for older Linux systems that don't have it

yes, adding fallback to fcntl would be a more complete solution, but i figured it wasn't worth the effort. if someone wants to, they could make that change easily on top of my change here, so we at least get incremental improvements.

#define O_CLOEXEC 0
#endif

#ifndef SOCK_CLOEXEC
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question here as for O_CLOEXEC, although SOCK_CLOEXEC seems to be newer and is probably less likely to be defined than O_CLOEXEC.

In any case, assuming that these fallback defines are needed, I can't think of a better place for them offhand.

#define SOCK_CLOEXEC 0
#endif

#define __STDLOG_MSGBUF_SIZE 4096
#ifndef STDLOG_INTERN_H_INCLUDED
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this isn't a part of your change, but I find it odd that the include guard is not at the top of the file protecting all the includes and definitions in this file. Can anyone say why it is done this way?

#define STDLOG_INTERN_H_INCLUDED
Expand Down
2 changes: 1 addition & 1 deletion stdlog/uxsock.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ static void
uxs_open(stdlog_channel_t ch)
{
if (ch->d.uxs.sock == -1) {
if((ch->d.uxs.sock = socket(AF_UNIX, SOCK_DGRAM, 0)) < 0)
if((ch->d.uxs.sock = socket(AF_UNIX, SOCK_DGRAM|SOCK_CLOEXEC, 0)) < 0)
return;
memset(&ch->d.uxs.addr, 0, sizeof(ch->d.uxs.addr));
ch->d.uxs.addr.sun_family = AF_UNIX;
Expand Down