Re: rc.subr: unset _rc_pid (fixes network after acpi S3 wakeup)

[christos%astron.com@localhost (Christos Zoulas)]

In article <58D04B65.3020106%eq.cz@localhost>, rudolf  <netbsd%eq.cz@localhost> wrote:
>Christos Zoulas wrote:
>> In article <58CEA448.2050405%eq.cz@localhost>, rudolf  <netbsd%eq.cz@localhost> wrote:
>[...]
>>> One way to avoid this is to unset the
>>> variables exported by /etc/rc and /etc/rc.subr before running rc.d commands.
>[...]
>>>
>>> --- etc/rc.subr.orig	2017-03-19 15:04:05.000000000 +0100
>>> +++ etc/rc.subr	2017-03-19 15:15:36.000000000 +0100
>>> @@ -663,18 +663,22 @@
>>> 					# setup the command to run, and run it
>>> 					#
>>> 			echo "Starting ${name}."
>>> +			exported_rc_vars="RC_PID _rc_pid \
>>> +_rc_original_stdout_fd _rc_original_stderr_fd _rc_postprocessor_fd"
>>> 			if [ -n "$_chroot" ]; then
>>> -				_doit="\
>>> +				_doit="(\
>>> +unset $exported_rc_vars; \
>>> ${_env:+env $_env }\
>>> ${_nice:+nice -n $_nice }\
>>> chroot ${_user:+-u $_user }${_group:+-g $_group }${_groups:+-G $_groups }\
>>> -$_chroot $command $rc_flags $command_args"
>>> +$_chroot $command $rc_flags $command_args)"
>>> 			else
>>> -				_doit="\
>>> +				_doit="(\
>>> +unset $exported_rc_vars; \
>>> ${_chdir:+cd $_chdir; }\
>>> ${_env:+env $_env }\
>>> ${_nice:+nice -n $_nice }\
>>> -$command $rc_flags $command_args"
>>> +$command $rc_flags $command_args)"
>>> 				if [ -n "$_user" ]; then
>>> 				    _doit="su -m $_user -c 'sh -c \"$_doit\"'"
>>> 				fi
>>
>> - why kill those only for 'start'? I guess it is only important for start,
>>    but...
>> - I don't think think that the parens are needed; after all the cd command
>>    is separate.
>
>If i read it right, the _doit string is being evaled in the current 
>shell. I wanted the change to have minimal impact so i've added the 
>parens to unset the vars in a subshell to not affect the current shell 
>and rc functions that may by launched after the run_rc_command().

There are 2 cases, but let's take things one at a time:

1. Here we are running a $_precmd; without applying environment, nice,
and chroot, etc. Perhaps we should but we don't.

			    if ! eval $_precmd && [ -z "$rc_force" ]; then
				    return 1
			    fi

2. Now the two cases for Startup:

			    # setup the command to run, and run it
			    # 
			    echo "Starting ${name}." 
			    if [ -n "$_chroot" ]; then
				    _doit="\

2a. Here's the chroot case [case 1]; this will run:

    ${_env:+env $_env }\
    ${_nice:+nice -n $_nice }\
    chroot ${_user:+-u $_user }${_group:+-g $_group }${_groups:+-G $_groups }\
    $_chroot $command $rc_flags $command_args"
			    else

2b. This is the regular case, the chroot change does not need chdir since
    it changes the root? Here we are already altering the shell context
    with chdir and not restoring it...

				    _doit="\
    ${_chdir:+cd $_chdir; }\ 
    ${_env:+env $_env }\
    ${_nice:+nice -n $_nice }\
    $command $rc_flags $command_args"
				    if [ -n "$_user" ]; then
					_doit="su -m $_user -c 'sh -c \"$_doit\"'"
				    fi
			    fi
     
			    # if the cmd failed and force
			    # isn't set, exit
			    # 
			    if ! eval $_doit && [ -z "$rc_force" ]; then
				    return 1
			    fi

3. Here's the postcmd, again with no modifiers:

			    # finally, run postcmd
			    #
			    eval $_postcmd

>The same reason (minimal impact of the change) was for unsetting the 
>vars only for 'start' and that is indeed quite arbitrary, but now when i 
>read the other "case" branches, it seems to me that only in the 'start' 
>it has any sense. Or do you think it is useful for the "eval $_postcmd" too?

I don't think that we should be changing precmd and postcmd, because nobody
has complained about the current behavior. OTOH adding a subshell changes
the semantics of the command (i.e. if the command was meant to modify the
current shell context now it will not) so I think that the minimum impact
is to set the variables to empty in the env invocation.

>In netbsd-7 version of rc.d scripts, there are only two exaples, where a 
>rc.d script runs the run_rc_command() function more then one time (and 
>(thus) it's also not the last statement in the script): sshd, 
>nfslocking. Only nfslocking may launch both it's run_rc_command()s with 
>the 'start' argument.
>
>> - Why not append those to $_env as "RC_PID= _rc_pid=" ...
>
>I've considered it, this way the subshell is not needed, but I still 
>didn't like the presence of the (blank) variables in env. Just 
>aesthetics? :-)

Yes, but it is just aesthetics...

christos