On Tue, Nov 10, 2015 at 10:06 PM, Ansis Atteka <[email protected]> wrote: > On Mon, Nov 9, 2015 at 12:43 PM, Andy Zhou <[email protected]> wrote: >> vlog log file can be created when parsing --log-file option, before >> switching user, in case the --user option is also specified. While this >> does not directly cause errors for the running daemons, it can >> leave the log files on the disk looks confusing with a mixture of > s/looks/to look > >> ownership settings, since the packaging script supply the --user > > Not sure "with a mixture of ownership settings" is the right way to > describe the situation one would be getting into. I would simply state > that the log file would be created under different user. Is this > simpler to say the thing you intended to? > > actually it is init.d script and not packaging scripts that are > providing --user argument. Packaging scripts are debian > [post|pre|[rm|inst] scripts
O.K. I will reword the commit message. > >> option to all daemons usually changes owner of OVS log files into >> non-root as well. This patch fix the log file ownership to the >> user specified with --user. >> >> Signed-off-by: Andy Zhou <[email protected]> >> >> --- >> v1->v2: Add a comment on vlog_change_owner return code. >> --- >> include/openvswitch/vlog.h | 1 + >> lib/daemon-unix.c | 13 ++++++++++--- >> lib/vlog.c | 25 ++++++++++++++++++++++++- >> 3 files changed, 35 insertions(+), 4 deletions(-) >> >> 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 645a3ac..e28459a 100644 >> --- a/lib/daemon-unix.c >> +++ b/lib/daemon-unix.c >> @@ -744,7 +744,7 @@ daemon_switch_group(gid_t real, gid_t effective, >> { >> if ((setresgid(real, effective, saved) == -1) || >> !gid_verify(real, effective, saved)) { >> - VLOG_FATAL("%s: fail to switch group to gid as %d, aborting", >> + VLOG_FATAL("%s: failed to switch group to gid as %d, aborting", >> pidfile, gid); >> } >> } >> @@ -852,12 +852,19 @@ daemon_become_new_user_linux(bool access_datapath >> OVS_UNUSED) >> static void >> daemon_become_new_user__(bool access_datapath) >> { >> - /* Make sure we exectue this function at most once as a root >> - * process. */ >> + /* Execute this function at most once. After this function, >> + * this process will no longer be a root process. */ >> if (getuid() || geteuid()) { >> 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 > > I am inclined to use past tense in log messages if you want to inform > user that something you executed in the past failed. >> + "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..f921701 100644 >> --- a/lib/vlog.c >> +++ b/lib/vlog.c >> @@ -105,7 +105,7 @@ DEFINE_STATIC_PER_THREAD_DATA(unsigned int, msg_num, 0); >> * All of the following is protected by 'log_file_mutex', which nests inside >> * pattern_rwlock. */ >> static struct ovs_mutex log_file_mutex = OVS_MUTEX_INITIALIZER; >> -static char *log_file_name OVS_GUARDED_BY(log_file_mutex); >> +static char *log_file_name = NULL OVS_GUARDED_BY(log_file_mutex); >> static int log_fd OVS_GUARDED_BY(log_file_mutex) = -1; >> static struct async_append *log_writer OVS_GUARDED_BY(log_file_mutex); >> static bool log_async OVS_GUARDED_BY(log_file_mutex); >> @@ -430,6 +430,29 @@ 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. >> + * >> + * Return 0 if log file has not been created. Otherwise the return >> + * code is the same as specified by chown(2). */ > > Here are few conventions that one can use for his functions to signal error: > 1. Return 0 on success -1 on error and retain errno (this is what most > libc calls do). > 2. Return 0 on success and integer error code on failure (this is > widely used in linux kernel) > 3. Return true on success and false on failure. > 4. don't return anything if the caller on failure would terminate > program anyway. Just terminate from callee and keep convention simple. > > Actually I think that #4 is the best for this case because it seems > that caller would call VLOG_FATAL() anyway in case of failure, right? > So why not do it from the callee in the first place and keep things > simple? > > Also, it took some time for me to read the convention that 0 is when > file did not exist and then you "tunnel through" chown error code > which is basically "1. Return 0 on success -1 on error and retain > errno." > O.K. I will change the return type to void. > > >> +int >> +vlog_change_owner(uid_t user, gid_t group) >> +{ >> + int error; >> + >> + if (log_file_name) { >> + ovs_mutex_lock(&log_file_mutex); >> + error = chown(log_file_name, user, group); >> + ovs_mutex_unlock(&log_file_mutex); >> + } else { >> + error = 0; >> + } >> + >> + 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 >> [email protected] >> http://openvswitch.org/mailman/listinfo/dev _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
