| Message ID | 1444464456-27941-2-git-send-email-azhou@nicira.com |
|---|---|
| State | Superseded |
| Headers | show |
On 10 October 2015 at 01:07, Andy Zhou <azhou@nicira.com> wrote: > vlog log file can be created when parsing --log-file option, before switch user, in case the --user option is also specified. This > this does not read fluently. How about: s/switch user/switching user? does not directly causing errors for the running daemons, but leaves > s/causing/cause > the log files on disk as owned by root. It can be confusing at best. > > This patch fixes the log file ownership setting to match with the > daemon that writes to it. > Signed-off-by: Andy Zhou <azhou@nicira.com> > --- > include/openvswitch/vlog.h | 1 + > lib/daemon-unix.c | 7 +++++++ > lib/vlog.c | 14 ++++++++++++++ > 3 files changed, 22 insertions(+) > > diff --git a/include/openvswitch/vlog.h b/include/openvswitch/vlog.h > index f6bb3ab..139dfb9 100644 > --- a/include/openvswitch/vlog.h > +++ b/include/openvswitch/vlog.h > @@ -143,6 +143,7 @@ void vlog_set_verbosity(const char *arg); > void vlog_set_pattern(enum vlog_destination, const char *pattern); > int vlog_set_log_file(const char *file_name); > int vlog_reopen_log_file(void); > +int vlog_change_owner(uid_t, gid_t); > > /* Configure method how vlog should send messages to syslog server. */ > void vlog_set_syslog_method(const char *method); > diff --git a/lib/daemon-unix.c b/lib/daemon-unix.c > index cafa397..e31dbc4 100644 > --- a/lib/daemon-unix.c > +++ b/lib/daemon-unix.c > @@ -856,6 +856,13 @@ daemon_become_new_user__(bool access_datapath) > return; > } > > + /* If vlog file has been created, change its owner to the non-root > user > + * as specifed by the --user option. */ > + if (vlog_change_owner(uid, gid)) { > + VLOG_FATAL("%s: fail to change owner of the log file from root " > s/fail/failed > + "to user %s", pidfile, user); > If log file doesn't yet exist on filesystem, wouldn't you call here VLOG_FATAL? Based on "man 2 chown" it seems that it can return ENOENT if file does not exist. > + } > + > if (LINUX) { > if (LIBCAPNG) { > daemon_become_new_user_linux(access_datapath); > diff --git a/lib/vlog.c b/lib/vlog.c > index da31e6f..56b8db8 100644 > --- a/lib/vlog.c > +++ b/lib/vlog.c > @@ -430,6 +430,20 @@ vlog_reopen_log_file(void) > } > } > > +/* In case a log file exists, change its owner to new 'user' and 'group'. > + * > + * This is useful for handling cases where the --log-file option is > + * specified ahead of the --user option. */ > I think we typically define what is return value in function comments (see CodingStyle.md). +int > +vlog_change_owner(uid_t user, gid_t group) > +{ > + ovs_mutex_lock(&log_file_mutex); > + int error = log_file_name ? chown(log_file_name, user, group) : 0; > + ovs_mutex_unlock(&log_file_mutex); > + > + return error; > +} > + > /* Set debugging levels. Returns null if successful, otherwise an error > * message that the caller must free(). */ > char * > -- > 1.9.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev >
On Fri, Nov 6, 2015 at 11:32 AM, Ansis Atteka <ansisatteka@gmail.com> wrote: > > > On 10 October 2015 at 01:07, Andy Zhou <azhou@nicira.com> wrote: >> >> vlog log file can be created when parsing --log-file option, before >> >> switch user, in case the --user option is also specified. This > > this does not read fluently. How about: > > s/switch user/switching user? > > >> does not directly causing errors for the running daemons, but leaves > > s/causing/cause >> >> the log files on disk as owned by root. It can be confusing at best. >> >> This patch fixes the log file ownership setting to match with the >> daemon that writes to it. >> >> >> Signed-off-by: Andy Zhou <azhou@nicira.com> >> --- >> include/openvswitch/vlog.h | 1 + >> lib/daemon-unix.c | 7 +++++++ >> lib/vlog.c | 14 ++++++++++++++ >> 3 files changed, 22 insertions(+) >> >> diff --git a/include/openvswitch/vlog.h b/include/openvswitch/vlog.h >> index f6bb3ab..139dfb9 100644 >> --- a/include/openvswitch/vlog.h >> +++ b/include/openvswitch/vlog.h >> @@ -143,6 +143,7 @@ void vlog_set_verbosity(const char *arg); >> void vlog_set_pattern(enum vlog_destination, const char *pattern); >> int vlog_set_log_file(const char *file_name); >> int vlog_reopen_log_file(void); >> +int vlog_change_owner(uid_t, gid_t); >> >> /* Configure method how vlog should send messages to syslog server. */ >> void vlog_set_syslog_method(const char *method); >> diff --git a/lib/daemon-unix.c b/lib/daemon-unix.c >> index cafa397..e31dbc4 100644 >> --- a/lib/daemon-unix.c >> +++ b/lib/daemon-unix.c >> @@ -856,6 +856,13 @@ daemon_become_new_user__(bool access_datapath) >> return; >> } >> >> + /* If vlog file has been created, change its owner to the non-root >> user >> + * as specifed by the --user option. */ >> + if (vlog_change_owner(uid, gid)) { >> + VLOG_FATAL("%s: fail to change owner of the log file from root " > > s/fail/failed >> >> + "to user %s", pidfile, user); > > If log file doesn't yet exist on filesystem, wouldn't you call here > VLOG_FATAL? Based on "man 2 chown" it seems that it can return ENOENT if > file does not exist. If the log file does not yet exist, vlog_change_owner suppose to return 0. >> >> + } >> + >> if (LINUX) { >> if (LIBCAPNG) { >> daemon_become_new_user_linux(access_datapath); >> diff --git a/lib/vlog.c b/lib/vlog.c >> index da31e6f..56b8db8 100644 >> --- a/lib/vlog.c >> +++ b/lib/vlog.c >> @@ -430,6 +430,20 @@ vlog_reopen_log_file(void) >> } >> } >> >> +/* In case a log file exists, change its owner to new 'user' and 'group'. >> + * >> + * This is useful for handling cases where the --log-file option is >> + * specified ahead of the --user option. */ > > > I think we typically define what is return value in function comments (see > CodingStyle.md). Sure, I will add a comment about the return value. > >> +int >> +vlog_change_owner(uid_t user, gid_t group) >> +{ >> + ovs_mutex_lock(&log_file_mutex); >> + int error = log_file_name ? chown(log_file_name, user, group) : 0; >> >> + ovs_mutex_unlock(&log_file_mutex); >> + >> + return error; >> +} >> + >> /* Set debugging levels. Returns null if successful, otherwise an error >> * message that the caller must free(). */ >> char * >> -- >> 1.9.1 >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> http://openvswitch.org/mailman/listinfo/dev > >
diff --git a/include/openvswitch/vlog.h b/include/openvswitch/vlog.h index f6bb3ab..139dfb9 100644 --- a/include/openvswitch/vlog.h +++ b/include/openvswitch/vlog.h @@ -143,6 +143,7 @@ void vlog_set_verbosity(const char *arg); void vlog_set_pattern(enum vlog_destination, const char *pattern); int vlog_set_log_file(const char *file_name); int vlog_reopen_log_file(void); +int vlog_change_owner(uid_t, gid_t); /* Configure method how vlog should send messages to syslog server. */ void vlog_set_syslog_method(const char *method); diff --git a/lib/daemon-unix.c b/lib/daemon-unix.c index cafa397..e31dbc4 100644 --- a/lib/daemon-unix.c +++ b/lib/daemon-unix.c @@ -856,6 +856,13 @@ daemon_become_new_user__(bool access_datapath) return; } + /* If vlog file has been created, change its owner to the non-root user + * as specifed by the --user option. */ + if (vlog_change_owner(uid, gid)) { + VLOG_FATAL("%s: fail to change owner of the log file from root " + "to user %s", pidfile, user); + } + if (LINUX) { if (LIBCAPNG) { daemon_become_new_user_linux(access_datapath); diff --git a/lib/vlog.c b/lib/vlog.c index da31e6f..56b8db8 100644 --- a/lib/vlog.c +++ b/lib/vlog.c @@ -430,6 +430,20 @@ vlog_reopen_log_file(void) } } +/* In case a log file exists, change its owner to new 'user' and 'group'. + * + * This is useful for handling cases where the --log-file option is + * specified ahead of the --user option. */ +int +vlog_change_owner(uid_t user, gid_t group) +{ + ovs_mutex_lock(&log_file_mutex); + int error = log_file_name ? chown(log_file_name, user, group) : 0; + ovs_mutex_unlock(&log_file_mutex); + + return error; +} + /* Set debugging levels. Returns null if successful, otherwise an error * message that the caller must free(). */ char *
vlog log file can be created when parsing --log-file option, before switch user, in case the --user option is also specified. This does not directly causing errors for the running daemons, but leaves the log files on disk as owned by root. It can be confusing at best. This patch fixes the log file ownership setting to match with the daemon that writes to it. Signed-off-by: Andy Zhou <azhou@nicira.com> --- include/openvswitch/vlog.h | 1 + lib/daemon-unix.c | 7 +++++++ lib/vlog.c | 14 ++++++++++++++ 3 files changed, 22 insertions(+)