[ovs-dev] [PATCH ovn] checkpatch: Avoid a case of catastrophic backtracking

[Frode Nordahl]

On Fri, Sep 3, 2021 at 3:25 PM Aaron Conole <aconole at redhat.com> wrote:
>
> Frode Nordahl <frode.nordahl at canonical.com> writes:
>
> > On Thu, Sep 2, 2021 at 4:04 PM Aaron Conole <aconole at redhat.com> wrote:
> >>
> >> Hi Frode (et. al.),
> >>
> >> Frode Nordahl writes:
> >>
> >> > The checkpatch script will hang forever on line 282 of a otherwise
> >> > valid patch [0].  While the root of the issue is probably
> >> > somewhere between the regex itself and the Python re
> >> > implementation, this patch provides an early return to avoid this
> >> > specific issue.
> >> >
> >> > Also fix the docstring for the if_and_for_end_with_bracket_check
> >> > function.
> >> >
> >> > 0:
> >> > https://patchwork.ozlabs.org/project/ovn/patch/20210819110857.2229769-8-frode.nordahl@canonical.com/
> >> > Signed-off-by: Frode Nordahl <frode.nordahl at canonical.com>
> >> > ---
> >>
> >> Sorry, I was on PTO when this patch came in and missed it.  I don't
> >> agree with your proposed fix.  It's a bit too conditional (and I was
> >> able to build a test-case that breaks it anyway).
> >>
> >> I see it's already been applied to OVN, but I plan to propose the
> >> following for OVS repository (and suggest a similar change to OVN).
> >>
> >> The change should be safe because we already validate that we are only
> >> in a for/if/while block, so the early bail to True should be okay.
> >>
> >> WDYT?
> >
> > No worries, I had to propose something as apparently I was clogging up
> > the CI, which is not nice :-) Your approach does look more generally
> > useful indeed and I would have no issue with having mine replaced by
> > it.
>
> Cool!
>
> > However, it does end up with the opposite verdict on the line in
> > question, was that intentional? From my reading of the test, it checks
> > that when there is room for a bracket at the end of a line for a
> > conditional control block it should be there, does that not mean we
> > can put it on the next line when there is not room?
>
> I'm not sure what you mean.
>
> In Documentation/internals/contributing/coding-style.rst
> Section: Statements, I don't see anything about putting the brace on a
> separate line.  So, I think such error case is correctly flagged.  Maybe
> there could be an exception placed in the coding style - not sure.  It's
> a different discussion, though.

I stand corrected.

I have no issue with breaking that statement over multiple lines like
the rest of us, just found it peculiar when I first dug into why it
got stuck on it.

> I'll propose this patch formally for OVS.  I can also submit the patch
> for OVN, if you think that makes sense.

Yes, having them in sync would be great.

Cheers!

-- 
Frode Nordahl