One of the patches seems to break windows build.
https://ci.appveyor.com/project/blp/ovs/build/1.0.916
On Wed, Nov 11, 2015 at 6:12 PM, Andy Zhou <az...@nicira.com> wrote:
> On Wed, Nov 11, 2015 at 2:48 PM, Ansis Atteka <ansisatt...@gmail.com> wrote:
>>
>>
>> On 11 November 2015 at 14:13, Andy Zhou <az...@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 as created under the "root" user.
>>> This patch fix the log file ownership to the user specified with --user.
>>>
>>> Signed-off-by: Andy Zhou <az...@nicira.com>
>>>
>>> ---
>>> v1->v2: Add a comment on vlog_change_owner return code.
>>> v2->v3: no change
>>> v3->v4: Reword the commit message. change vlog_change_owner() to void.
>>> ---
>>> include/openvswitch/vlog.h | 1 +
>>> lib/daemon-unix.c | 6 +++++-
>>> lib/vlog.c | 22 +++++++++++++++++++++-
>>> 3 files changed, 27 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/include/openvswitch/vlog.h b/include/openvswitch/vlog.h
>>> index f6bb3ab..bc16590 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);
>>> +void 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 0125745..e69517a 100644
>>> --- a/lib/daemon-unix.c
>>> +++ b/lib/daemon-unix.c
>>> @@ -739,7 +739,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);
>>> }
>>> }
>>> @@ -847,6 +847,10 @@ daemon_become_new_user_linux(bool access_datapath
>>> OVS_UNUSED)
>>> static void
>>> daemon_become_new_user__(bool access_datapath)
>>> {
>>> + /* If vlog file has been created, change its owner to the non-root
>>> user
>>> + * as specifed by the --user option. */
>>> + vlog_change_owner(uid, gid);
>>> +
>>> if (LINUX) {
>>> if (LIBCAPNG) {
>>> daemon_become_new_user_linux(access_datapath);
>>> diff --git a/lib/vlog.c b/lib/vlog.c
>>> index da31e6f..ade0a45 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,26 @@ 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. */
>>> +void
>>> +vlog_change_owner(uid_t user, gid_t group)
>>> +{
>>> + int error = 0;
>>> +
>>> + if (log_file_name) {
>>> + ovs_mutex_lock(&log_file_mutex);
>>> + error = chown(log_file_name, user, group);
>>> + ovs_mutex_unlock(&log_file_mutex);
>>> + }
>>> +
>>> + if (error) {
>>> + VLOG_FATAL("Failed to change log file ownership.");
>>
>> I would print errno value here and the file name you are actually trying to
>> change the ownership for. It would simply provide a hint to the users on
>> what was actually wrong, if it failed.
>>
>> VLOG_FATAL("Failed to change %s ownership: %s", log_file_name,
>> ovs_strerror(errno));
>>
>> And early return from function if log_file_name is NULL to make code look
>> better.
>>
>
> Pushed to master with both changes as suggested.
>>> + }
>>> +}
>>> +
>>
>> Otherwise, Acked-by: Ansis Atteka <aatt...@nicira.com>
>>
>> Thanks for working on this, Andy.
>
> Thanks for your review.
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
