diff mbox

[ovs-dev,additional,--user,changes,v4,2/3] vlog: change log file owner when switching user

Message ID CACzMAJJ-XFPm_PP6bTEAze+ONb7SdnLr3k=JcvPyfnU=3tpRHw@mail.gmail.com
State Accepted
Headers show

Commit Message

Andy Zhou Nov. 12, 2015, 3 a.m. UTC 
On Wed, Nov 11, 2015 at 6:29 PM, Gurucharan Shetty <shettyg@nicira.com> 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 <azhou@nicira.com> wrote:
>> On Wed, Nov 11, 2015 at 2:48 PM, Ansis Atteka <ansisatteka@gmail.com> wrote:
>>>
>>>
>>> On 11 November 2015 at 14:13, 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 as created under the "root" user.
>>>> 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.
>>>> 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 <aatteka@nicira.com>
>>>
>>> Thanks for working on this, Andy.
>>
>> Thanks for your review.
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> 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 mbox

Patch

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(). */