[ovs-dev] [PATCH 2/2] vlog: change log file owner when switching user

Andy Zhou azhou at nicira.com
Mon Nov 9 18:43:30 UTC 2015


On Fri, Nov 6, 2015 at 11:32 AM, Ansis Atteka <ansisatteka at gmail.com> wrote:
>
>
> On 10 October 2015 at 01:07, Andy Zhou <azhou at 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 at 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 at openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev
>
>



More information about the dev mailing list