| Message ID | 1448019200-87207-5-git-send-email-azhou@ovn.org |
|---|---|
| State | Changes Requested |
| Headers | show |
Hi Andy, On Fri, Nov 20, 2015 at 03:33:18AM -0800, Andy Zhou wrote: > Rafactor common directory existence check and ownership check into > a common function. Move daemon's default directory to $RUNDIR, since > the process may not able to write core file to "/" anymore after the > user change. > > Signed-off-by: Andy Zhou <azhou@ovn.org> > > --- > v1->v2: * Drop using 'stat -c" > * ADD $OVS_GROUP != root in addition to $OVS_USER != root check > --- > utilities/ovs-lib.in | 37 ++++++++++++++++++++++++++++--------- > 1 file changed, 28 insertions(+), 9 deletions(-) > > diff --git a/utilities/ovs-lib.in b/utilities/ovs-lib.in > index ad223c0..ad9c9f4 100644 > --- a/utilities/ovs-lib.in > +++ b/utilities/ovs-lib.in > @@ -70,8 +70,6 @@ ovs_ctl () { > > VERSION='@VERSION@' > > -DAEMON_CWD=/ > - > LC_ALL=C; export LC_ALL > > ## ------------- ## > @@ -154,6 +152,23 @@ pid_comm_check () { > [ "$1" = "`cat /proc/$2/comm`" ] > } > > +# Make sure the directory '$1' exits. If not, crate it. If yes, make sure > +# its group ownership agrees with $OVS_GROUP. If not, chown on all files > +# within it. We don't enforce $OVS_USER to allow for multiple users that > +# shares $OVS_GROUP. > +directory_check() { > + dir=$1 Some care has been taken to always quote $dir below, and it seems that the same care has been taken by callers regarding the parameter passed to directory_check. However, in order for things to hold together I think that $1 also needs to be quoted when assigning dir above. > + > + if test -d "$dir"; then > + # Change the ownership of the top level directory and the first > + # level files below it. > + chown "$OVS_USER":"$OVS_GROUP" "$dir" > + find "$dir" -maxdepth 1 -type f -exec chown "$OVS_USER":"$OVS_GROUP" {} \; > + else > + install -d -m 775 -o "$OVS_USER" -g "$OVS_GROUP" "$dir" > + fi > +} > + > start_daemon () { > priority=$1 > wrapper=$2 [snip]
On Fri, Nov 20, 2015 at 03:33:18AM -0800, Andy Zhou wrote: > Rafactor common directory existence check and ownership check into > a common function. Move daemon's default directory to $RUNDIR, since > the process may not able to write core file to "/" anymore after the > user change. > > Signed-off-by: Andy Zhou <azhou@ovn.org> > > --- > v1->v2: * Drop using 'stat -c" > * ADD $OVS_GROUP != root in addition to $OVS_USER != root check > --- > utilities/ovs-lib.in | 37 ++++++++++++++++++++++++++++--------- > 1 file changed, 28 insertions(+), 9 deletions(-) > > diff --git a/utilities/ovs-lib.in b/utilities/ovs-lib.in > index ad223c0..ad9c9f4 100644 > --- a/utilities/ovs-lib.in > +++ b/utilities/ovs-lib.in > @@ -70,8 +70,6 @@ ovs_ctl () { > > VERSION='@VERSION@' > > -DAEMON_CWD=/ > - > LC_ALL=C; export LC_ALL > > ## ------------- ## > @@ -154,6 +152,23 @@ pid_comm_check () { > [ "$1" = "`cat /proc/$2/comm`" ] > } > > +# Make sure the directory '$1' exits. If not, crate it. If yes, make sure > +# its group ownership agrees with $OVS_GROUP. If not, chown on all files > +# within it. We don't enforce $OVS_USER to allow for multiple users that > +# shares $OVS_GROUP. > +directory_check() { > + dir=$1 > + > + if test -d "$dir"; then > + # Change the ownership of the top level directory and the first > + # level files below it. > + chown "$OVS_USER":"$OVS_GROUP" "$dir" > + find "$dir" -maxdepth 1 -type f -exec chown "$OVS_USER":"$OVS_GROUP" {} \; > + else > + install -d -m 775 -o "$OVS_USER" -g "$OVS_GROUP" "$dir" > + fi > +} > + > start_daemon () { > priority=$1 > wrapper=$2 > @@ -161,20 +176,24 @@ start_daemon () { > daemon=$1 > strace="" > > - # drop core files in a sensible place > - test -d "$DAEMON_CWD" || install -d -m 755 -o "$OVS_USER" -g "$OVS_GROUP" "$DAEMON_CWD" > - set "$@" --no-chdir > - cd "$DAEMON_CWD" > - > # log file > - test -d "$logdir" || install -d -m 755 -o "$OVS_USER" -g "$OVS_GROUP" "$logdir" > + directory_check "$logdir" > set "$@" --log-file="$logdir/$daemon.log" > > # pidfile and monitoring > - test -d "$rundir" || install -d -m 755 -o "$OVS_USER" -g "$OVS_GROUP" "$rundir" > + directory_check "$rundir" > set "$@" --pidfile="$rundir/$daemon.pid" > set "$@" --detach --monitor > > + # drop core files in a sensible place > + cd "$rundir" > + set "$@" --no-chdir This depends on many things. One is that systemd-coredump(8) handles core dump properly. Another is that core(5) which might point to something else different. The systemd also provides WorkingDirectory= to set specific workdir, but we can't use that if the initialization enforces something else. Anyway, this patch isn't changing anything other the workdir from / to $rundir, which makes more sense. > + > + # add --user for non root user > + if test "$OVS_USER" != "root" || test "$OVS_GROUP" != "root"; then > + set "$@" --user="$OVS_USER":"$OVS_GROUP" > + fi > + > # wrapper > case $wrapper in > valgrind) Acked-by: Flavio Leitner <fbl@sysclose.org> > -- > 1.8.3.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev
On Thu, Nov 26, 2015 at 02:12:51PM +0900, Simon Horman wrote: > Hi Andy, > > On Fri, Nov 20, 2015 at 03:33:18AM -0800, Andy Zhou wrote: > > Rafactor common directory existence check and ownership check into > > a common function. Move daemon's default directory to $RUNDIR, since > > the process may not able to write core file to "/" anymore after the > > user change. > > > > Signed-off-by: Andy Zhou <azhou@ovn.org> > > > > --- > > v1->v2: * Drop using 'stat -c" > > * ADD $OVS_GROUP != root in addition to $OVS_USER != root check > > --- > > utilities/ovs-lib.in | 37 ++++++++++++++++++++++++++++--------- > > 1 file changed, 28 insertions(+), 9 deletions(-) > > > > diff --git a/utilities/ovs-lib.in b/utilities/ovs-lib.in > > index ad223c0..ad9c9f4 100644 > > --- a/utilities/ovs-lib.in > > +++ b/utilities/ovs-lib.in > > @@ -70,8 +70,6 @@ ovs_ctl () { > > > > VERSION='@VERSION@' > > > > -DAEMON_CWD=/ > > - > > LC_ALL=C; export LC_ALL > > > > ## ------------- ## > > @@ -154,6 +152,23 @@ pid_comm_check () { > > [ "$1" = "`cat /proc/$2/comm`" ] > > } > > > > +# Make sure the directory '$1' exits. If not, crate it. If yes, make sure > > +# its group ownership agrees with $OVS_GROUP. If not, chown on all files > > +# within it. We don't enforce $OVS_USER to allow for multiple users that > > +# shares $OVS_GROUP. > > +directory_check() { > > + dir=$1 > > Some care has been taken to always quote $dir below, > and it seems that the same care has been taken by callers > regarding the parameter passed to directory_check. > > However, in order for things to hold together I think that $1 also needs to > be quoted when assigning dir above. There's no word splitting on the right side of an assignment in shell, so it's OK in this case.
diff --git a/utilities/ovs-lib.in b/utilities/ovs-lib.in index ad223c0..ad9c9f4 100644 --- a/utilities/ovs-lib.in +++ b/utilities/ovs-lib.in @@ -70,8 +70,6 @@ ovs_ctl () { VERSION='@VERSION@' -DAEMON_CWD=/ - LC_ALL=C; export LC_ALL ## ------------- ## @@ -154,6 +152,23 @@ pid_comm_check () { [ "$1" = "`cat /proc/$2/comm`" ] } +# Make sure the directory '$1' exits. If not, crate it. If yes, make sure +# its group ownership agrees with $OVS_GROUP. If not, chown on all files +# within it. We don't enforce $OVS_USER to allow for multiple users that +# shares $OVS_GROUP. +directory_check() { + dir=$1 + + if test -d "$dir"; then + # Change the ownership of the top level directory and the first + # level files below it. + chown "$OVS_USER":"$OVS_GROUP" "$dir" + find "$dir" -maxdepth 1 -type f -exec chown "$OVS_USER":"$OVS_GROUP" {} \; + else + install -d -m 775 -o "$OVS_USER" -g "$OVS_GROUP" "$dir" + fi +} + start_daemon () { priority=$1 wrapper=$2 @@ -161,20 +176,24 @@ start_daemon () { daemon=$1 strace="" - # drop core files in a sensible place - test -d "$DAEMON_CWD" || install -d -m 755 -o "$OVS_USER" -g "$OVS_GROUP" "$DAEMON_CWD" - set "$@" --no-chdir - cd "$DAEMON_CWD" - # log file - test -d "$logdir" || install -d -m 755 -o "$OVS_USER" -g "$OVS_GROUP" "$logdir" + directory_check "$logdir" set "$@" --log-file="$logdir/$daemon.log" # pidfile and monitoring - test -d "$rundir" || install -d -m 755 -o "$OVS_USER" -g "$OVS_GROUP" "$rundir" + directory_check "$rundir" set "$@" --pidfile="$rundir/$daemon.pid" set "$@" --detach --monitor + # drop core files in a sensible place + cd "$rundir" + set "$@" --no-chdir + + # add --user for non root user + if test "$OVS_USER" != "root" || test "$OVS_GROUP" != "root"; then + set "$@" --user="$OVS_USER":"$OVS_GROUP" + fi + # wrapper case $wrapper in valgrind)
Rafactor common directory existence check and ownership check into a common function. Move daemon's default directory to $RUNDIR, since the process may not able to write core file to "/" anymore after the user change. Signed-off-by: Andy Zhou <azhou@ovn.org> --- v1->v2: * Drop using 'stat -c" * ADD $OVS_GROUP != root in addition to $OVS_USER != root check --- utilities/ovs-lib.in | 37 ++++++++++++++++++++++++++++--------- 1 file changed, 28 insertions(+), 9 deletions(-)