On Wed, Nov 11, 2015 at 6:29 PM, Gurucharan Shetty <[email protected]> wrote:
> 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 <[email protected]> wrote:
>> On Wed, Nov 11, 2015 at 2:48 PM, Ansis Atteka <[email protected]> wrote:
>>>
>>>
>>> On 11 November 2015 at 14:13, 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 as created under the "root" user.
>>>> 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.
>>>> 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 <[email protected]>
>>>
>>> Thanks for working on this, Andy.
>>
>> Thanks for your review.
>> _______________________________________________
>> dev mailing list
>> [email protected]
>> http://openvswitch.org/mailman/listinfo/dev
I see why. uid_t and gid_t are probably not defined on Windows.
Since it is breaking the build, I pushed the following patch that will
not compile vlog_change_owner() on windows. Sorry for breaking the
build.
diff --git a/include/openvswitch/vlog.h b/include/openvswitch/vlog.h
index bc16590..5309602 100644
--- a/include/openvswitch/vlog.h
+++ b/include/openvswitch/vlog.h
@@ -143,7 +143,9 @@ 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);
+#ifndef _WIN32
+void vlog_change_owner_unix(uid_t, gid_t);
+#endif
/* 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 e69517a..a471b59 100644
--- a/lib/daemon-unix.c
+++ b/lib/daemon-unix.c
@@ -849,7 +849,7 @@ 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);
+ vlog_change_owner_unix(uid, gid);
if (LINUX) {
if (LIBCAPNG) {
diff --git a/lib/vlog.c b/lib/vlog.c
index 841c7a4..18d0e33 100644
--- a/lib/vlog.c
+++ b/lib/vlog.c
@@ -430,12 +430,13 @@ vlog_reopen_log_file(void)
}
}
+#ifndef _WIN32
/* 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)
+vlog_change_owner_unix(uid_t user, gid_t group)
{
if (!log_file_name) {
return;
@@ -450,6 +451,7 @@ vlog_change_owner(uid_t user, gid_t group)
log_file_name, ovs_strerror(errno));
}
}
+#endif
/* Set debugging levels. Returns null if successful, otherwise an error
* message that the caller must free(). */
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
