Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(172)

Issue 6720036: Slight cleanup of syslog.conf. (Closed)

Created:
9 years, 8 months ago by diandersAtChromium
Modified:
9 years, 6 months ago
Reviewers:
jrbarnette
CC:
chromium-os-reviews_chromium.org
Visibility:
Public.

Description

Slight cleanup of syslog.conf. Changes: * Avoid mkdir of /var/run/rsyslog if it already exists for a slight speedup. * Use 'install' for mkdir of /var/run/rsyslog, which allows us to get the permissions right from the start. This lets us avoid a 'chmod -R' later. * Only chmod the kmsg pipe if we created it, so we can avoid the chmod -R. * nit: use "if [ ! -p ... ]" rather than "test ||". Both are equally valid, but the if syntax seems to be more commonly used in our scripts. * Indentation / spacing as per jrbarnette suggestions. * All variables use ${VAR} syntax. Possible bugs (please reject this review if you think any of these can happen): * We now only do chown to files that we created, and only do the chown when we create them. If somehow we were relying on chowning other files, that behavior will now be broken. BUG=chromium-os:13510 TEST=Ad-hoc. Placed the meat of the script into a file (syslog.sh) so I could run it manually. Then, did: sudo dash -x syslog.sh # Validated that proper dash commands were executed sudo rm -f /var/run/rsyslog/kmsg sudo dash -x syslog.sh # Validated that proper dash commands were executed ls -al /var/run/rsyslog # Validated correct files / permissions (. and kmsg should both exist and be owned by syslog) sudo rm -rf /var/run/rsyslog sudo dash -x syslog.sh # Validated that proper dash commands were executed ls -al /var/run/rsyslog # Validated correct files / permissions (. and kmsg should both exist and be owned by syslog) Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=05873d4

Patch Set 1 #

Patch Set 2 : Use install; no hacky chown -R. #

Total comments: 4

Patch Set 3 : jrbarnette review feedback. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+11 lines, -9 lines) Patch
M syslog.conf View 1 2 2 chunks +11 lines, -9 lines 0 comments Download

Messages

Total messages: 4 (0 generated)
diandersAtChromium
Richard, Trying to finish other minor init scripts cleanups that I did. PTAL. Any other ...
9 years, 8 months ago (2011-04-04 23:17:30 UTC) #1
jrbarnette
http://codereview.chromium.org/6720036/diff/1001/syslog.conf File syslog.conf (right): http://codereview.chromium.org/6720036/diff/1001/syslog.conf#newcode1 syslog.conf:1: # Copyright (c) 2009 The Chromium OS Authors. All ...
9 years, 8 months ago (2011-04-05 17:06:16 UTC) #2
diandersAtChromium
PTAL. http://codereview.chromium.org/6720036/diff/1001/syslog.conf File syslog.conf (right): http://codereview.chromium.org/6720036/diff/1001/syslog.conf#newcode1 syslog.conf:1: # Copyright (c) 2009 The Chromium OS Authors. ...
9 years, 8 months ago (2011-04-05 21:39:12 UTC) #3
jrbarnette
9 years, 8 months ago (2011-04-05 23:00:02 UTC) #4
On 2011/04/05 21:39:12, diandersAtChromium wrote:
> PTAL.
> 
LGTM!

Thanks!

> http://codereview.chromium.org/6720036/diff/1001/syslog.conf
> File syslog.conf (right):
> 
> http://codereview.chromium.org/6720036/diff/1001/syslog.conf#newcode1
> syslog.conf:1: # Copyright (c) 2009 The Chromium OS Authors. All rights
> reserved.
> On 2011/04/05 17:06:16, jrbarnette wrote:
> > I believe we've concluded that the copyright date here should get updated.
> 
> Done.
> 
> http://codereview.chromium.org/6720036/diff/1001/syslog.conf#newcode27
> syslog.conf:27: exec /usr/sbin/rsyslogd -c4 -f /etc/rsyslog.chromeos
> On 2011/04/05 17:06:16, jrbarnette wrote:
> > So long as this is about cleanup, can you change this to invoke
> > rsyslogd without a full path?
> 
> Done.

Powered by Google App Engine
This is Rietveld 408576698