| Message ID | 20220602143410.1986117-1-twilson@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v2,1/2] Handle re-used pids in pidfile_is_running | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | warning | apply and check: warning |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
Bleep bloop. Greetings Terry Wilson, I am a robot and I have tried out your patch.
Thanks for your contribution.
I encountered some error that I wasn't expecting. See the details below.
checkpatch:
WARNING: Line is 124 characters long (recommended limit is 79)
#36 FILE: utilities/ovn-ctl:52:
test -e "$pidfile" && [ -s "$pidfile" ] && pid=`cat "$pidfile"` && pid_exists "$pid" && pid_exe_matches "$pid" "$binary"
Lines checked: 42, Warnings: 1, Errors: 0
Please check this out. If you feel there has been an error, please email aconole@redhat.com
Thanks,
0-day Robot
On Thu, Jun 2, 2022 at 10:34 AM Terry Wilson <twilson@redhat.com> wrote: > > Since pids can be re-used, it is necessary to check that the > process that is running with a pid matches the one that we expect. > > This adds the ability to optionally pass a 'binary' argument to > pidfile_is_running, and if it is passed to match the binary against > /proc/$pid/exe. > > Signed-off-by: Terry Wilson <twilson@redhat.com> Acked-by: Ihar Hrachyshka <ihrachys@redhat.com> > --- > utilities/ovn-ctl | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/utilities/ovn-ctl b/utilities/ovn-ctl > index d733aa42d..41fa89770 100755 > --- a/utilities/ovn-ctl > +++ b/utilities/ovn-ctl > @@ -40,9 +40,16 @@ ovn_ic_db_conf_file="$ovn_etcdir/ovn-ic-db-params.conf" > ## start ## > ## ----- ## > > +pid_exe_matches () { > + pid=$1 > + binary=$2 > + [ -z "$binary" -o `readlink /proc/$pid/exe` = "$binary" ] > +} > + > pidfile_is_running () { > pidfile=$1 > - test -e "$pidfile" && [ -s "$pidfile" ] && pid=`cat "$pidfile"` && pid_exists "$pid" > + binary=$2 > + test -e "$pidfile" && [ -s "$pidfile" ] && pid=`cat "$pidfile"` && pid_exists "$pid" && pid_exe_matches "$pid" "$binary" > } >/dev/null 2>&1 > > stop_nb_ovsdb() { > -- > 2.34.3 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 6/2/22 16:34, Terry Wilson wrote: > Since pids can be re-used, it is necessary to check that the > process that is running with a pid matches the one that we expect. > > This adds the ability to optionally pass a 'binary' argument to > pidfile_is_running, and if it is passed to match the binary against > /proc/$pid/exe. > > Signed-off-by: Terry Wilson <twilson@redhat.com> > --- > utilities/ovn-ctl | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/utilities/ovn-ctl b/utilities/ovn-ctl > index d733aa42d..41fa89770 100755 > --- a/utilities/ovn-ctl > +++ b/utilities/ovn-ctl > @@ -40,9 +40,16 @@ ovn_ic_db_conf_file="$ovn_etcdir/ovn-ic-db-params.conf" > ## start ## > ## ----- ## > > +pid_exe_matches () { > + pid=$1 > + binary=$2 > + [ -z "$binary" -o `readlink /proc/$pid/exe` = "$binary" ] Hi, Terry. Good catch! Though, I think, it will be better to just check the 'comm' as OVS does instead of comparing the full path to the binary. This will protect us from running the same daemon from different locations in a general case, i.e. during development or an upgrade if the binary suddenly moved. 'readlink' also seems to not be very reliable, because it will add '(deleted)' to the end of the result if the binary was deleted or replaced. That also might be a problem during updates where the binary is updated on the disk but the process is still running. Best regards, Ilya Maximets. > +} > + > pidfile_is_running () { > pidfile=$1 > - test -e "$pidfile" && [ -s "$pidfile" ] && pid=`cat "$pidfile"` && pid_exists "$pid" > + binary=$2 > + test -e "$pidfile" && [ -s "$pidfile" ] && pid=`cat "$pidfile"` && pid_exists "$pid" && pid_exe_matches "$pid" "$binary" > } >/dev/null 2>&1 > > stop_nb_ovsdb() {
diff --git a/utilities/ovn-ctl b/utilities/ovn-ctl index d733aa42d..41fa89770 100755 --- a/utilities/ovn-ctl +++ b/utilities/ovn-ctl @@ -40,9 +40,16 @@ ovn_ic_db_conf_file="$ovn_etcdir/ovn-ic-db-params.conf" ## start ## ## ----- ## +pid_exe_matches () { + pid=$1 + binary=$2 + [ -z "$binary" -o `readlink /proc/$pid/exe` = "$binary" ] +} + pidfile_is_running () { pidfile=$1 - test -e "$pidfile" && [ -s "$pidfile" ] && pid=`cat "$pidfile"` && pid_exists "$pid" + binary=$2 + test -e "$pidfile" && [ -s "$pidfile" ] && pid=`cat "$pidfile"` && pid_exists "$pid" && pid_exe_matches "$pid" "$binary" } >/dev/null 2>&1 stop_nb_ovsdb() {
Since pids can be re-used, it is necessary to check that the process that is running with a pid matches the one that we expect. This adds the ability to optionally pass a 'binary' argument to pidfile_is_running, and if it is passed to match the binary against /proc/$pid/exe. Signed-off-by: Terry Wilson <twilson@redhat.com> --- utilities/ovn-ctl | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)