Direct host output to a specific log file.

Direct host output to a specific log file.
As of 10.8 if you don't specify a standard output/error destination file
then output from the host service is not logged at all.  This change
modifies our host service .plist to specify a specific chromoting
log file, ensures this log file is properly created
when we install the host, adds a configuration for the newsyslog
process (which will turnover/archive the log file once it goes >1MB),
and removes the log file and newsyslog configuration in the uninstaller.
It also adds some timestamps to the host startup script so that we can
better identify log events.

It also alphabetizes the strings in the .plist file.

TEST=Manual (modified files, created install package, ran and uninstalled)
BUG=171026
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=181605

[Stephen, 2013-02-07 01:04:36]

PTAL, thanks!

[Jamie, 2013-02-07 02:22:15]

https://codereview.chromium.org/12213059/diff/1/remoting/host/installer/mac/L...
File remoting/host/installer/mac/LaunchAgents/org.chromium.chromoting.plist
(left):

https://codereview.chromium.org/12213059/diff/1/remoting/host/installer/mac/L...
remoting/host/installer/mac/LaunchAgents/org.chromium.chromoting.plist:15:
<false/>
Can you add something to the CL description to indicate that you've also
alphabetized the keys?

https://codereview.chromium.org/12213059/diff/1/remoting/host/installer/mac/S...
File remoting/host/installer/mac/Scripts/remoting_postflight.sh (right):

https://codereview.chromium.org/12213059/diff/1/remoting/host/installer/mac/S...
remoting/host/installer/mac/Scripts/remoting_postflight.sh:67: chmod a+rw
/var/log/org.chromium.chromoting.log
I wonder if this would be better done as part of the host startup, when it
initializes logging? We could include some simple log rotation as well.
Unfortunately, we can't use /var in that case because it won't be writable in
general, but I'm leery of dropping a world-writable file into /var anyway.

[Stephen, 2013-02-07 03:19:03]

On 2013/02/07 02:22:15, Jamie wrote:
>https://codereview.chromium.org/12213059/diff/1/remoting/host/installer/mac/LaunchAgents/org.chromium.chromoting.plist#oldcode15
> remoting/host/installer/mac/LaunchAgents/org.chromium.chromoting.plist:15:
> <false/>
> Can you add something to the CL description to indicate that you've also
> alphabetized the keys?
>

Done.
 
>
https://codereview.chromium.org/12213059/diff/1/remoting/host/installer/mac/S...
> remoting/host/installer/mac/Scripts/remoting_postflight.sh:67: chmod a+rw
> /var/log/org.chromium.chromoting.log
> I wonder if this would be better done as part of the host startup, when it
> initializes logging? We could include some simple log rotation as well.
> Unfortunately, we can't use /var in that case because it won't be writable in
> general, but I'm leery of dropping a world-writable file into /var anyway.

I don't think this will work.  The reason I had to add code to create the log
file is that if it's not present when launchd starts the process, you get an
error reported in the system.log and no logging from the process occurs.  You
also get an error if the file exists but isn't writable by the user account the
process is running under (which is why I had to make it explicitly writable). 
In addition, the file has to have an absolute path (~/<file> isn't permitted),
so it's going to have to go somewhere in one of the system directories.  

One thing we could do is to make the file writable only by root and the user
who's installing the host.  That will work fine in the general case but if the
host is ever enabled by some other user later on, no logging will occur.

[Jamie, 2013-02-07 18:52:46]

On 2013/02/07 03:19:03, Stephen wrote:
> On 2013/02/07 02:22:15, Jamie wrote:
>
>https://codereview.chromium.org/12213059/diff/1/remoting/host/installer/mac/LaunchAgents/org.chromium.chromoting.plist#oldcode15
> > remoting/host/installer/mac/LaunchAgents/org.chromium.chromoting.plist:15:
> > <false/>
> > Can you add something to the CL description to indicate that you've also
> > alphabetized the keys?
> >
> 
> Done.
>  
> >
>
https://codereview.chromium.org/12213059/diff/1/remoting/host/installer/mac/S...
> > remoting/host/installer/mac/Scripts/remoting_postflight.sh:67: chmod a+rw
> > /var/log/org.chromium.chromoting.log
> > I wonder if this would be better done as part of the host startup, when it
> > initializes logging? We could include some simple log rotation as well.
> > Unfortunately, we can't use /var in that case because it won't be writable
in
> > general, but I'm leery of dropping a world-writable file into /var anyway.
> 
> I don't think this will work.  The reason I had to add code to create the log
> file is that if it's not present when launchd starts the process, you get an
> error reported in the system.log and no logging from the process occurs.  You
> also get an error if the file exists but isn't writable by the user account
the
> process is running under (which is why I had to make it explicitly writable). 
> In addition, the file has to have an absolute path (~/<file> isn't permitted),
> so it's going to have to go somewhere in one of the system directories.  
> 
> One thing we could do is to make the file writable only by root and the user
> who's installing the host.  That will work fine in the general case but if the
> host is ever enabled by some other user later on, no logging will occur.

That would make me feel a bit happier about having unprivileged files under
/var. I think once we have a daemon architecture, this problem goes away because
we can do our logging from the root component. Until then, creating the file
with more locked-down permissions when the host is started (not when it is
installed) should get around your concern about dealing with multiple users.

remoting/host/installer/mac/PrivilegedHelperTools/org.chromium.chromoting.me2me.sh
already has code (under the --enable flag) to set a suitable ACL to make the
config file readable; it should be straightforward to modify that to make the
log file writable. Please make sure we can log both at the login screen and
while logged.

[lambroslambrou+watch_chromium.org, 2013-02-07 19:23:50]

Logging to a specific system-wide file isn't really a good approach. Even
if we lock down the ACL (Jamie's suggestion of "when the host is started"
seems difficult), we still have the problem of no rotation. Much better to
log using system facilities. Either
1) Figure out how to enable our agent's stdout/stderr to go to the system
logs (maybe something in man launchd.conf)
2) Configure the logger in remoting_me2me_host.cc to log using system
logging APIs instead of stdout/stderr.



On Thu, Feb 7, 2013 at 10:52 AM, <jamiewalch@chromium.org> wrote:

> On 2013/02/07 03:19:03, Stephen wrote:
>
>> On 2013/02/07 02:22:15, Jamie wrote:
>>
>
>  https://codereview.chromium.**org/12213059/diff/1/remoting/**
>> host/installer/mac/**LaunchAgents/org.chromium.**
>>
chromoting.plist#oldcode15<https://codereview.chromium.org/12213059/diff/1/remoting/host/installer/mac/LaunchAgents/org.chromium.chromoting.plist#oldcode15>
>> > remoting/host/installer/mac/**LaunchAgents/org.chromium.**
>> chromoting.plist:15:
>> > <false/>
>> > Can you add something to the CL description to indicate that you've also
>> > alphabetized the keys?
>> >
>>
>
>  Done.
>>
>
>  >
>>
>
> https://codereview.chromium.**org/12213059/diff/1/remoting/**
>
host/installer/mac/Scripts/**remoting_postflight.sh#**newcode67<https://codereview.chromium.org/12213059/diff/1/remoting/host/installer/mac/Scripts/remoting_postflight.sh#newcode67>
>
>> > remoting/host/installer/mac/**Scripts/remoting_postflight.**sh:67:
>> chmod a+rw
>> > /var/log/org.chromium.**chromoting.log
>> > I wonder if this would be better done as part of the host startup, when
>> it
>> > initializes logging? We could include some simple log rotation as well.
>> > Unfortunately, we can't use /var in that case because it won't be
>> writable
>>
> in
>
>> > general, but I'm leery of dropping a world-writable file into /var
>> anyway.
>>
>
>  I don't think this will work.  The reason I had to add code to create the
>> log
>> file is that if it's not present when launchd starts the process, you get
>> an
>> error reported in the system.log and no logging from the process occurs.
>>  You
>> also get an error if the file exists but isn't writable by the user
>> account
>>
> the
>
>> process is running under (which is why I had to make it explicitly
>> writable).
>> In addition, the file has to have an absolute path (~/<file> isn't
>> permitted),
>> so it's going to have to go somewhere in one of the system directories.
>>
>
>  One thing we could do is to make the file writable only by root and the
>> user
>> who's installing the host.  That will work fine in the general case but
>> if the
>> host is ever enabled by some other user later on, no logging will occur.
>>
>
> That would make me feel a bit happier about having unprivileged files under
> /var. I think once we have a daemon architecture, this problem goes away
> because
> we can do our logging from the root component. Until then, creating the
> file
> with more locked-down permissions when the host is started (not when it is
> installed) should get around your concern about dealing with multiple
> users.
>
> remoting/host/installer/mac/**PrivilegedHelperTools/org.**
> chromium.chromoting.me2me.sh <http://org.chromium.chromoting.me2me.sh>
> already has code (under the --enable flag) to set a suitable ACL to make
> the
> config file readable; it should be straightforward to modify that to make
> the
> log file writable. Please make sure we can log both at the login screen and
> while logged.
>
>
https://codereview.chromium.**org/12213059/<https://codereview.chromium.org/1...
>

[Stephen, 2013-02-07 19:41:07]

On 2013/02/07 19:23:50, lambroslambrou+watch_chromium.org wrote:
> Logging to a specific system-wide file isn't really a good approach. Even
> if we lock down the ACL (Jamie's suggestion of "when the host is started"
> seems difficult), we still have the problem of no rotation. 

Setting the ACL at startup-time turned out to be trivial.  I agree we have no
rotation, but that's not any different from what we had before when it all got
dumped into /var/log/system.log.


>Much better to
> log using system facilities. Either
> 1) Figure out how to enable our agent's stdout/stderr to go to the system
> logs (maybe something in man launchd.conf)

Sadly I don't think there's any option for us to specify system logs via any
launchd parameter.  OS X behavior just seems to have changed between 10.7 and
10.8, in that if you don't specify a specific output for stdout/stderr, it just
goes to /dev/null instead of syslog.

> 2) Configure the logger in remoting_me2me_host.cc to log using system
> logging APIs instead of stdout/stderr.

I will look into this.
> 
> 
> 
> On Thu, Feb 7, 2013 at 10:52 AM, <mailto:jamiewalch@chromium.org> wrote:
> 
> > On 2013/02/07 03:19:03, Stephen wrote:
> >
> >> On 2013/02/07 02:22:15, Jamie wrote:
> >>
> >
> >  https://codereview.chromium.**org/12213059/diff/1/remoting/**
> >> host/installer/mac/**LaunchAgents/org.chromium.**
> >>
>
chromoting.plist#oldcode15<https://codereview.chromium.org/12213059/diff/1/remoting/host/installer/mac/LaunchAgents/org.chromium.chromoting.plist#oldcode15>
> >> > remoting/host/installer/mac/**LaunchAgents/org.chromium.**
> >> chromoting.plist:15:
> >> > <false/>
> >> > Can you add something to the CL description to indicate that you've also
> >> > alphabetized the keys?
> >> >
> >>
> >
> >  Done.
> >>
> >
> >  >
> >>
> >
> > https://codereview.chromium.**org/12213059/diff/1/remoting/**
> >
>
host/installer/mac/Scripts/**remoting_postflight.sh#**newcode67<https://codereview.chromium.org/12213059/diff/1/remoting/host/installer/mac/Scripts/remoting_postflight.sh#newcode67>
> >
> >> > remoting/host/installer/mac/**Scripts/remoting_postflight.**sh:67:
> >> chmod a+rw
> >> > /var/log/org.chromium.**chromoting.log
> >> > I wonder if this would be better done as part of the host startup, when
> >> it
> >> > initializes logging? We could include some simple log rotation as well.
> >> > Unfortunately, we can't use /var in that case because it won't be
> >> writable
> >>
> > in
> >
> >> > general, but I'm leery of dropping a world-writable file into /var
> >> anyway.
> >>
> >
> >  I don't think this will work.  The reason I had to add code to create the
> >> log
> >> file is that if it's not present when launchd starts the process, you get
> >> an
> >> error reported in the system.log and no logging from the process occurs.
> >>  You
> >> also get an error if the file exists but isn't writable by the user
> >> account
> >>
> > the
> >
> >> process is running under (which is why I had to make it explicitly
> >> writable).
> >> In addition, the file has to have an absolute path (~/<file> isn't
> >> permitted),
> >> so it's going to have to go somewhere in one of the system directories.
> >>
> >
> >  One thing we could do is to make the file writable only by root and the
> >> user
> >> who's installing the host.  That will work fine in the general case but
> >> if the
> >> host is ever enabled by some other user later on, no logging will occur.
> >>
> >
> > That would make me feel a bit happier about having unprivileged files under
> > /var. I think once we have a daemon architecture, this problem goes away
> > because
> > we can do our logging from the root component. Until then, creating the
> > file
> > with more locked-down permissions when the host is started (not when it is
> > installed) should get around your concern about dealing with multiple
> > users.
> >
> > remoting/host/installer/mac/**PrivilegedHelperTools/org.**
> > chromium.chromoting.me2me.sh <http://org.chromium.chromoting.me2me.sh>
> > already has code (under the --enable flag) to set a suitable ACL to make
> > the
> > config file readable; it should be straightforward to modify that to make
> > the
> > log file writable. Please make sure we can log both at the login screen and
> > while logged.
> >
> >
>
https://codereview.chromium.**org/12213059/%3Chttps://codereview.chromium.org...>
> >

[Stephen, 2013-02-08 22:55:56]

Okay, please take another look.  I was able to work out a way to have our log
file automatically cleaned up by the system logging service by adding a .conf
file in /etc/newsyslog.d/.  It's set to archive the log after it reaches 1MB,
and to keep the last 5 copies before deleting (older versions are also
compressed).  

Permissions to the log file have also been simplified; it's now read/writable by
root and admin, and since most CRD users will be admin, this should be fine.  On
the off chance a non-admin user runs the host we won't get any logging, but I
believe that's okay.

Lastly this change required modifying our installer and uninstaller; I've tested
those locally and they work properly.

[Jamie, 2013-02-08 23:45:10]

https://codereview.chromium.org/12213059/diff/7001/remoting/host/installer/ma...
File remoting/host/installer/mac/Config/org.chromium.chromoting.conf (right):

https://codereview.chromium.org/12213059/diff/7001/remoting/host/installer/ma...
remoting/host/installer/mac/Config/org.chromium.chromoting.conf:1: # logfilename
                           [owner:group]    mode count size when  flags
[/pid_file] [sig_num]
Can you add a Chromium copyright header, and a short description of what this
file is for (maybe just a reference to the appropriate man page).

https://codereview.chromium.org/12213059/diff/7001/remoting/host/installer/ma...
remoting/host/installer/mac/Config/org.chromium.chromoting.conf:2:
/var/log/org.chromium.chromoting.log     root:admin        664  3     1000 *    
J
Could mode be 660, so that 'other' has no permissions? I suspect umask will take
care of that for us anyway, but let's play it safe.

https://codereview.chromium.org/12213059/diff/7001/remoting/host/installer/ma...
File remoting/host/installer/mac/Scripts/remoting_postflight.sh (right):

https://codereview.chromium.org/12213059/diff/7001/remoting/host/installer/ma...
remoting/host/installer/mac/Scripts/remoting_postflight.sh:73: chmod 664
"$LOG_FILE"
As with the conf file, 660 would be safer.

[Stephen, 2013-02-09 00:09:38]

On 2013/02/08 23:45:10, Jamie wrote:
>
https://codereview.chromium.org/12213059/diff/7001/remoting/host/installer/ma...
> remoting/host/installer/mac/Config/org.chromium.chromoting.conf:1: #
logfilename
>                            [owner:group]    mode count size when  flags
> [/pid_file] [sig_num]
> Can you add a Chromium copyright header, and a short description of what this
> file is for (maybe just a reference to the appropriate man page).

Done.

>
https://codereview.chromium.org/12213059/diff/7001/remoting/host/installer/ma...
> remoting/host/installer/mac/Config/org.chromium.chromoting.conf:2:
> /var/log/org.chromium.chromoting.log     root:admin        664  3     1000 *  
 
> J
> Could mode be 660, so that 'other' has no permissions? I suspect umask will
take
> care of that for us anyway, but let's play it safe.

Done.

>
https://codereview.chromium.org/12213059/diff/7001/remoting/host/installer/ma...
> remoting/host/installer/mac/Scripts/remoting_postflight.sh:73: chmod 664
> "$LOG_FILE"
> As with the conf file, 660 would be safer.

Done.

[Jamie, 2013-02-09 00:21:32]

lgtm with nits.

https://codereview.chromium.org/12213059/diff/8011/remoting/host/installer/ma...
File remoting/host/installer/mac/Config/org.chromium.chromoting.conf (right):

https://codereview.chromium.org/12213059/diff/8011/remoting/host/installer/ma...
remoting/host/installer/mac/Config/org.chromium.chromoting.conf:1: # Copyright
(c) 2013 The Chromium Authors. All rights reserved.
Nit: No (c) any more :)

https://codereview.chromium.org/12213059/diff/8011/remoting/host/installer/ma...
remoting/host/installer/mac/Config/org.chromium.chromoting.conf:8: # for more
information.
Nit: Please wrap the comment lines at 80 characters (even if the rest of the
file can't easily be wrapped).

https://codereview.chromium.org/12213059/diff/8011/remoting/host/installer/ma...
remoting/host/installer/mac/Config/org.chromium.chromoting.conf:11:
/var/log/org.chromium.chromoting.log     root:admin       660  3     1000 *    
J
Unless 1000Kb is something you've seen recommended elsewhere as a rotation
threshold, I'd recommend increasing it; perhaps to 10Mb?

[Stephen, 2013-02-09 00:29:35]

On 2013/02/09 00:21:32, Jamie wrote:
> lgtm with nits.
> 
https://codereview.chromium.org/12213059/diff/8011/remoting/host/installer/ma...
> remoting/host/installer/mac/Config/org.chromium.chromoting.conf:1: # Copyright
> (c) 2013 The Chromium Authors. All rights reserved.
> Nit: No (c) any more :)

Fixed.

>
https://codereview.chromium.org/12213059/diff/8011/remoting/host/installer/ma...
> remoting/host/installer/mac/Config/org.chromium.chromoting.conf:8: # for more
> information.
> Nit: Please wrap the comment lines at 80 characters (even if the rest of the
> file can't easily be wrapped).

Fixed.

>
https://codereview.chromium.org/12213059/diff/8011/remoting/host/installer/ma...
> remoting/host/installer/mac/Config/org.chromium.chromoting.conf:11:
> /var/log/org.chromium.chromoting.log     root:admin       660  3     1000 *   

> J
> Unless 1000Kb is something you've seen recommended elsewhere as a rotation
> threshold, I'd recommend increasing it; perhaps to 10Mb?

1MB is used by just about every other configuration , so I'll stick with
convention.  :)

[commit-bot: I haz the power, 2013-02-09 00:29:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skonig@chromium.org/12213059/9011

[commit-bot: I haz the power, 2013-02-09 03:16:44]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-02-09 03:19:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skonig@chromium.org/12213059/9011

[commit-bot: I haz the power, 2013-02-09 06:09:16]

Change committed as 181605