| Message ID | 1447101793-13804-2-git-send-email-azhou@nicira.com |
|---|---|
| State | Rejected |
| Headers | show |
On Mon, Nov 9, 2015 at 12:43 PM, Andy Zhou <azhou@nicira.com> 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 > 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 <azhou@nicira.com> > > --- > 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." > +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 > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev
On Tue, Nov 10, 2015 at 10:06 PM, Ansis Atteka <aatteka@nicira.com> wrote: > On Mon, Nov 9, 2015 at 12:43 PM, Andy Zhou <azhou@nicira.com> 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 <azhou@nicira.com> >> >> --- >> 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 >> 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 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 " + "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). */ +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 *
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 ownership settings, since the packaging script supply the --user 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 <azhou@nicira.com> --- 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(-)