| Message ID | 20220514084010.119558-3-hepeng.0320@bytedance.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,ovs-dev,v3,1/3] ovs-rcu: add rcu_barrier | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | warning | apply and check: warning |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
| ovsrobot/intel-ovs-compilation | success | test: success |
Bleep bloop. Greetings Peng He, 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: ERROR: Author Peng He <xnhp0320@gmail.com> needs to sign off. WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: Peng He <hepeng.0320@bytedance.com> Lines checked: 81, Warnings: 1, Errors: 1 Please check this out. If you feel there has been an error, please email aconole@redhat.com Thanks, 0-day Robot
On 14 May 2022, at 10:40, Peng He wrote: > Signed-off-by: Peng He <hepeng.0320@bytedance.com> > --- > utilities/checkpatch.py | 32 +++++++++++++++++++++++++++++--- > 1 file changed, 29 insertions(+), 3 deletions(-) > > diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py > index 8c7faa419..67c517b69 100755 > --- a/utilities/checkpatch.py > +++ b/utilities/checkpatch.py > @@ -619,6 +619,8 @@ def regex_function_factory(func_name): > def regex_error_factory(description): > return lambda: print_error(description) > Need extra newline, install flake8 and you will get the error at compile time. > +def regex_warn_factory(description): > + return lambda: print_warning(description) Need extra newline, install flake8 and you will get the error at compile time. > > std_functions = [ > ('malloc', 'Use xmalloc() in place of malloc()'), > @@ -636,6 +638,7 @@ std_functions = [ > ('assert', 'Use ovs_assert() in place of assert()'), > ('error', 'Use ovs_error() in place of error()'), > ] > + > checks += [ > {'regex': r'(\.c|\.h)(\.in)?$', > 'match_name': None, > @@ -644,6 +647,21 @@ checks += [ > 'print': regex_error_factory(description)} > for (function_name, description) in std_functions] > > +experimental_api = [ I do not think this is an experimental API, I would call it something like a suspicious API maybe? > + ('ovsrcu_barrier', > + 'lib/ovs-rcu.c', > + 'Are you sure you need to use ovsrcu_barrier(),' Here you need to add a space at the end, as the error message now looks like: WARNING: Are you sure you need to use ovsrcu_barrier(),in most cases ovsrcu_synchronize() will be fine? > + 'in most cases ovsrcu_synchronize() will be fine?'), > + ] > + > +checks += [ > + {'regex': r'(\.c)(\.in)?$', > + 'match_name': lambda x: x != location, > + 'prereq': lambda x: not is_comment_line(x), > + 'check': regex_function_factory(function_name), > + 'print': regex_warn_factory(description)} > + for (function_name, location, description) in experimental_api] > + > > def regex_operator_factory(operator): > regex = re.compile(r'^[^#][^"\']*[^ "]%s[^ "\'][^"]*' % operator) > @@ -676,12 +694,20 @@ def get_file_type_checks(filename): > global checks > checkList = [] > for check in checks: > + regex_check = True > + match_check = True > + > if check['regex'] is None and check['match_name'] is None: > checkList.append(check) > + continue > + > if check['regex'] is not None and \ > - re.compile(check['regex']).search(filename) is not None: > - checkList.append(check) > - elif check['match_name'] is not None and check['match_name'](filename): > + re.compile(check['regex']).search(filename) is None: > + regex_check = False > + elif check['match_name'] is not None and not check['match_name'](filename): Line is too long so you need to break it up: utilities/checkpatch.py:709:80: E501 line too long (83 > 79 characters) > + match_check = False > + > + if regex_check and match_check: > checkList.append(check) Would it not make more sense to re-write the above elif cases to a single case? > return checkList > > -- > 2.25.1
Eelco Chaudron <echaudro@redhat.com> 于2022年5月16日周一 17:12写道： > > > On 14 May 2022, at 10:40, Peng He wrote: > > > Signed-off-by: Peng He <hepeng.0320@bytedance.com> > > --- > > utilities/checkpatch.py | 32 +++++++++++++++++++++++++++++--- > > 1 file changed, 29 insertions(+), 3 deletions(-) > > > > diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py > > index 8c7faa419..67c517b69 100755 > > --- a/utilities/checkpatch.py > > +++ b/utilities/checkpatch.py > > @@ -619,6 +619,8 @@ def regex_function_factory(func_name): > > def regex_error_factory(description): > > return lambda: print_error(description) > > > > Need extra newline, install flake8 and you will get the error at compile > time. > > > +def regex_warn_factory(description): > > + return lambda: print_warning(description) > > Need extra newline, install flake8 and you will get the error at compile > time. > > > > > std_functions = [ > > ('malloc', 'Use xmalloc() in place of malloc()'), > > @@ -636,6 +638,7 @@ std_functions = [ > > ('assert', 'Use ovs_assert() in place of assert()'), > > ('error', 'Use ovs_error() in place of error()'), > > ] > > + > > checks += [ > > {'regex': r'(\.c|\.h)(\.in)?$', > > 'match_name': None, > > @@ -644,6 +647,21 @@ checks += [ > > 'print': regex_error_factory(description)} > > for (function_name, description) in std_functions] > > > > +experimental_api = [ > > I do not think this is an experimental API, I would call it something like > a suspicious API maybe? > or change to easy_to_misused_api? > > > + ('ovsrcu_barrier', > > + 'lib/ovs-rcu.c', > > + 'Are you sure you need to use ovsrcu_barrier(),' > > Here you need to add a space at the end, as the error message now looks > like: > > WARNING: Are you sure you need to use ovsrcu_barrier(),in most cases > ovsrcu_synchronize() will be fine? > > > + 'in most cases ovsrcu_synchronize() will be fine?'), > > + ] > > + > > +checks += [ > > + {'regex': r'(\.c)(\.in)?$', > > + 'match_name': lambda x: x != location, > > + 'prereq': lambda x: not is_comment_line(x), > > + 'check': regex_function_factory(function_name), > > + 'print': regex_warn_factory(description)} > > + for (function_name, location, description) in experimental_api] > > + > > > > def regex_operator_factory(operator): > > regex = re.compile(r'^[^#][^"\']*[^ "]%s[^ "\'][^"]*' % operator) > > @@ -676,12 +694,20 @@ def get_file_type_checks(filename): > > global checks > > checkList = [] > > for check in checks: > > + regex_check = True > > + match_check = True > > + > > if check['regex'] is None and check['match_name'] is None: > > checkList.append(check) > > + continue > > + > > if check['regex'] is not None and \ > > - re.compile(check['regex']).search(filename) is not None: > > - checkList.append(check) > > - elif check['match_name'] is not None and > check['match_name'](filename): > > + re.compile(check['regex']).search(filename) is None: > > + regex_check = False > > + elif check['match_name'] is not None and not > check['match_name'](filename): > > Line is too long so you need to break it up: > > utilities/checkpatch.py:709:80: E501 line too long (83 > 79 characters) > > > > + match_check = False > > + > > + if regex_check and match_check: > > checkList.append(check) > > Would it not make more sense to re-write the above elif cases to a single > case? > > > return checkList > > > > -- > > 2.25.1 > > will send a new version， thanks for the detailed review.
On 16 May 2022, at 12:19, Peng He wrote: > Eelco Chaudron <echaudro@redhat.com> 于2022年5月16日周一 17:12写道： > >> >> >> On 14 May 2022, at 10:40, Peng He wrote: >> >>> Signed-off-by: Peng He <hepeng.0320@bytedance.com> >>> --- >>> utilities/checkpatch.py | 32 +++++++++++++++++++++++++++++--- >>> 1 file changed, 29 insertions(+), 3 deletions(-) >>> >>> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py >>> index 8c7faa419..67c517b69 100755 >>> --- a/utilities/checkpatch.py >>> +++ b/utilities/checkpatch.py >>> @@ -619,6 +619,8 @@ def regex_function_factory(func_name): >>> def regex_error_factory(description): >>> return lambda: print_error(description) >>> >> >> Need extra newline, install flake8 and you will get the error at compile >> time. >> >>> +def regex_warn_factory(description): >>> + return lambda: print_warning(description) >> >> Need extra newline, install flake8 and you will get the error at compile >> time. >> >>> >>> std_functions = [ >>> ('malloc', 'Use xmalloc() in place of malloc()'), >>> @@ -636,6 +638,7 @@ std_functions = [ >>> ('assert', 'Use ovs_assert() in place of assert()'), >>> ('error', 'Use ovs_error() in place of error()'), >>> ] >>> + >>> checks += [ >>> {'regex': r'(\.c|\.h)(\.in)?$', >>> 'match_name': None, >>> @@ -644,6 +647,21 @@ checks += [ >>> 'print': regex_error_factory(description)} >>> for (function_name, description) in std_functions] >>> >>> +experimental_api = [ >> >> I do not think this is an experimental API, I would call it something like >> a suspicious API maybe? >> > > or change to easy_to_misused_api? This is fine by me to. >> >>> + ('ovsrcu_barrier', >>> + 'lib/ovs-rcu.c', >>> + 'Are you sure you need to use ovsrcu_barrier(),' >> >> Here you need to add a space at the end, as the error message now looks >> like: >> >> WARNING: Are you sure you need to use ovsrcu_barrier(),in most cases >> ovsrcu_synchronize() will be fine? >> >>> + 'in most cases ovsrcu_synchronize() will be fine?'), >>> + ] >>> + >>> +checks += [ >>> + {'regex': r'(\.c)(\.in)?$', >>> + 'match_name': lambda x: x != location, >>> + 'prereq': lambda x: not is_comment_line(x), >>> + 'check': regex_function_factory(function_name), >>> + 'print': regex_warn_factory(description)} >>> + for (function_name, location, description) in experimental_api] >>> + >>> >>> def regex_operator_factory(operator): >>> regex = re.compile(r'^[^#][^"\']*[^ "]%s[^ "\'][^"]*' % operator) >>> @@ -676,12 +694,20 @@ def get_file_type_checks(filename): >>> global checks >>> checkList = [] >>> for check in checks: >>> + regex_check = True >>> + match_check = True >>> + >>> if check['regex'] is None and check['match_name'] is None: >>> checkList.append(check) >>> + continue >>> + >>> if check['regex'] is not None and \ >>> - re.compile(check['regex']).search(filename) is not None: >>> - checkList.append(check) >>> - elif check['match_name'] is not None and >> check['match_name'](filename): >>> + re.compile(check['regex']).search(filename) is None: >>> + regex_check = False >>> + elif check['match_name'] is not None and not >> check['match_name'](filename): >> >> Line is too long so you need to break it up: >> >> utilities/checkpatch.py:709:80: E501 line too long (83 > 79 characters) >> >> >>> + match_check = False >>> + >>> + if regex_check and match_check: >>> checkList.append(check) >> >> Would it not make more sense to re-write the above elif cases to a single >> case? >> >>> return checkList >>> >>> -- >>> 2.25.1 >> >> > will send a new version， thanks for the detailed review. You can also add my Signed-off-by: if you make the suggested changes to keep the robot happy. //Eelco
The only issue I have is that I have to use the company's email address to submit patches while I am using the personal email to subscribe to the maillist. So the bot will always warn about the author needing to sign-off. Perhaps in the future I would use both email addresses to avoid this warning. Eelco Chaudron <echaudro@redhat.com> 于2022年5月16日周一 19:36写道： > > > On 16 May 2022, at 12:19, Peng He wrote: > > > Eelco Chaudron <echaudro@redhat.com> 于2022年5月16日周一 17:12写道： > > > >> > >> > >> On 14 May 2022, at 10:40, Peng He wrote: > >> > >>> Signed-off-by: Peng He <hepeng.0320@bytedance.com> > >>> --- > >>> utilities/checkpatch.py | 32 +++++++++++++++++++++++++++++--- > >>> 1 file changed, 29 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py > >>> index 8c7faa419..67c517b69 100755 > >>> --- a/utilities/checkpatch.py > >>> +++ b/utilities/checkpatch.py > >>> @@ -619,6 +619,8 @@ def regex_function_factory(func_name): > >>> def regex_error_factory(description): > >>> return lambda: print_error(description) > >>> > >> > >> Need extra newline, install flake8 and you will get the error at compile > >> time. > >> > >>> +def regex_warn_factory(description): > >>> + return lambda: print_warning(description) > >> > >> Need extra newline, install flake8 and you will get the error at compile > >> time. > >> > >>> > >>> std_functions = [ > >>> ('malloc', 'Use xmalloc() in place of malloc()'), > >>> @@ -636,6 +638,7 @@ std_functions = [ > >>> ('assert', 'Use ovs_assert() in place of assert()'), > >>> ('error', 'Use ovs_error() in place of error()'), > >>> ] > >>> + > >>> checks += [ > >>> {'regex': r'(\.c|\.h)(\.in)?$', > >>> 'match_name': None, > >>> @@ -644,6 +647,21 @@ checks += [ > >>> 'print': regex_error_factory(description)} > >>> for (function_name, description) in std_functions] > >>> > >>> +experimental_api = [ > >> > >> I do not think this is an experimental API, I would call it something > like > >> a suspicious API maybe? > >> > > > > or change to easy_to_misused_api? > > This is fine by me to. > > >> > >>> + ('ovsrcu_barrier', > >>> + 'lib/ovs-rcu.c', > >>> + 'Are you sure you need to use ovsrcu_barrier(),' > >> > >> Here you need to add a space at the end, as the error message now looks > >> like: > >> > >> WARNING: Are you sure you need to use ovsrcu_barrier(),in most cases > >> ovsrcu_synchronize() will be fine? > >> > >>> + 'in most cases ovsrcu_synchronize() will be fine?'), > >>> + ] > >>> + > >>> +checks += [ > >>> + {'regex': r'(\.c)(\.in)?$', > >>> + 'match_name': lambda x: x != location, > >>> + 'prereq': lambda x: not is_comment_line(x), > >>> + 'check': regex_function_factory(function_name), > >>> + 'print': regex_warn_factory(description)} > >>> + for (function_name, location, description) in experimental_api] > >>> + > >>> > >>> def regex_operator_factory(operator): > >>> regex = re.compile(r'^[^#][^"\']*[^ "]%s[^ "\'][^"]*' % operator) > >>> @@ -676,12 +694,20 @@ def get_file_type_checks(filename): > >>> global checks > >>> checkList = [] > >>> for check in checks: > >>> + regex_check = True > >>> + match_check = True > >>> + > >>> if check['regex'] is None and check['match_name'] is None: > >>> checkList.append(check) > >>> + continue > >>> + > >>> if check['regex'] is not None and \ > >>> - re.compile(check['regex']).search(filename) is not None: > >>> - checkList.append(check) > >>> - elif check['match_name'] is not None and > >> check['match_name'](filename): > >>> + re.compile(check['regex']).search(filename) is None: > >>> + regex_check = False > >>> + elif check['match_name'] is not None and not > >> check['match_name'](filename): > >> > >> Line is too long so you need to break it up: > >> > >> utilities/checkpatch.py:709:80: E501 line too long (83 > 79 characters) > >> > >> > >>> + match_check = False > >>> + > >>> + if regex_check and match_check: > >>> checkList.append(check) > >> > >> Would it not make more sense to re-write the above elif cases to a > single > >> case? > >> > >>> return checkList > >>> > >>> -- > >>> 2.25.1 > >> > >> > > will send a new version， thanks for the detailed review. > > You can also add my Signed-off-by: if you make the suggested changes to > keep the robot happy. > > //Eelco > >
On 17 May 2022, at 7:49, Peng He wrote: > The only issue I have is that I have to use the company's email address to > submit patches > while I am using the personal email to subscribe to the maillist. > > So the bot will always warn about the author needing to sign-off. > Perhaps in the future I would use both email addresses to avoid this > warning. There is this error on patch 1/3: ERROR: Co-author Eelco Chaudron <echaudro@redhat.com> needs to sign off. So I guess you need to add my sign-off also in that patch. > > > Eelco Chaudron <echaudro@redhat.com> 于2022年5月16日周一 19:36写道： > >> >> >> On 16 May 2022, at 12:19, Peng He wrote: >> >>> Eelco Chaudron <echaudro@redhat.com> 于2022年5月16日周一 17:12写道： >>> >>>> >>>> >>>> On 14 May 2022, at 10:40, Peng He wrote: >>>> >>>>> Signed-off-by: Peng He <hepeng.0320@bytedance.com> >>>>> --- >>>>> utilities/checkpatch.py | 32 +++++++++++++++++++++++++++++--- >>>>> 1 file changed, 29 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py >>>>> index 8c7faa419..67c517b69 100755 >>>>> --- a/utilities/checkpatch.py >>>>> +++ b/utilities/checkpatch.py >>>>> @@ -619,6 +619,8 @@ def regex_function_factory(func_name): >>>>> def regex_error_factory(description): >>>>> return lambda: print_error(description) >>>>> >>>> >>>> Need extra newline, install flake8 and you will get the error at compile >>>> time. >>>> >>>>> +def regex_warn_factory(description): >>>>> + return lambda: print_warning(description) >>>> >>>> Need extra newline, install flake8 and you will get the error at compile >>>> time. >>>> >>>>> >>>>> std_functions = [ >>>>> ('malloc', 'Use xmalloc() in place of malloc()'), >>>>> @@ -636,6 +638,7 @@ std_functions = [ >>>>> ('assert', 'Use ovs_assert() in place of assert()'), >>>>> ('error', 'Use ovs_error() in place of error()'), >>>>> ] >>>>> + >>>>> checks += [ >>>>> {'regex': r'(\.c|\.h)(\.in)?$', >>>>> 'match_name': None, >>>>> @@ -644,6 +647,21 @@ checks += [ >>>>> 'print': regex_error_factory(description)} >>>>> for (function_name, description) in std_functions] >>>>> >>>>> +experimental_api = [ >>>> >>>> I do not think this is an experimental API, I would call it something >> like >>>> a suspicious API maybe? >>>> >>> >>> or change to easy_to_misused_api? >> >> This is fine by me to. >> >>>> >>>>> + ('ovsrcu_barrier', >>>>> + 'lib/ovs-rcu.c', >>>>> + 'Are you sure you need to use ovsrcu_barrier(),' >>>> >>>> Here you need to add a space at the end, as the error message now looks >>>> like: >>>> >>>> WARNING: Are you sure you need to use ovsrcu_barrier(),in most cases >>>> ovsrcu_synchronize() will be fine? >>>> >>>>> + 'in most cases ovsrcu_synchronize() will be fine?'), >>>>> + ] >>>>> + >>>>> +checks += [ >>>>> + {'regex': r'(\.c)(\.in)?$', >>>>> + 'match_name': lambda x: x != location, >>>>> + 'prereq': lambda x: not is_comment_line(x), >>>>> + 'check': regex_function_factory(function_name), >>>>> + 'print': regex_warn_factory(description)} >>>>> + for (function_name, location, description) in experimental_api] >>>>> + >>>>> >>>>> def regex_operator_factory(operator): >>>>> regex = re.compile(r'^[^#][^"\']*[^ "]%s[^ "\'][^"]*' % operator) >>>>> @@ -676,12 +694,20 @@ def get_file_type_checks(filename): >>>>> global checks >>>>> checkList = [] >>>>> for check in checks: >>>>> + regex_check = True >>>>> + match_check = True >>>>> + >>>>> if check['regex'] is None and check['match_name'] is None: >>>>> checkList.append(check) >>>>> + continue >>>>> + >>>>> if check['regex'] is not None and \ >>>>> - re.compile(check['regex']).search(filename) is not None: >>>>> - checkList.append(check) >>>>> - elif check['match_name'] is not None and >>>> check['match_name'](filename): >>>>> + re.compile(check['regex']).search(filename) is None: >>>>> + regex_check = False >>>>> + elif check['match_name'] is not None and not >>>> check['match_name'](filename): >>>> >>>> Line is too long so you need to break it up: >>>> >>>> utilities/checkpatch.py:709:80: E501 line too long (83 > 79 characters) >>>> >>>> >>>>> + match_check = False >>>>> + >>>>> + if regex_check and match_check: >>>>> checkList.append(check) >>>> >>>> Would it not make more sense to re-write the above elif cases to a >> single >>>> case? >>>> >>>>> return checkList >>>>> >>>>> -- >>>>> 2.25.1 >>>> >>>> >>> will send a new version， thanks for the detailed review. >> >> You can also add my Signed-off-by: if you make the suggested changes to >> keep the robot happy. >> >> //Eelco >> >> > > -- > hepeng
You mean I can ignore the following warnings? " checkpatch: ERROR: Author Peng He <xnhp0320@gmail.com> needs to sign off. WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: Peng He <hepeng.0320@bytedance.com> Lines checked: 98, Warnings: 1, Errors: 1 " Eelco Chaudron <echaudro@redhat.com> 于2022年5月17日周二 14:26写道： > > > On 17 May 2022, at 7:49, Peng He wrote: > > > The only issue I have is that I have to use the company's email address > to > > submit patches > > while I am using the personal email to subscribe to the maillist. > > > > So the bot will always warn about the author needing to sign-off. > > Perhaps in the future I would use both email addresses to avoid this > > warning. > > There is this error on patch 1/3: > > ERROR: Co-author Eelco Chaudron <echaudro@redhat.com> needs to sign off. > > So I guess you need to add my sign-off also in that patch. > > > > > > Eelco Chaudron <echaudro@redhat.com> 于2022年5月16日周一 19:36写道： > > > >> > >> > >> On 16 May 2022, at 12:19, Peng He wrote: > >> > >>> Eelco Chaudron <echaudro@redhat.com> 于2022年5月16日周一 17:12写道： > >>> > >>>> > >>>> > >>>> On 14 May 2022, at 10:40, Peng He wrote: > >>>> > >>>>> Signed-off-by: Peng He <hepeng.0320@bytedance.com> > >>>>> --- > >>>>> utilities/checkpatch.py | 32 +++++++++++++++++++++++++++++--- > >>>>> 1 file changed, 29 insertions(+), 3 deletions(-) > >>>>> > >>>>> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py > >>>>> index 8c7faa419..67c517b69 100755 > >>>>> --- a/utilities/checkpatch.py > >>>>> +++ b/utilities/checkpatch.py > >>>>> @@ -619,6 +619,8 @@ def regex_function_factory(func_name): > >>>>> def regex_error_factory(description): > >>>>> return lambda: print_error(description) > >>>>> > >>>> > >>>> Need extra newline, install flake8 and you will get the error at > compile > >>>> time. > >>>> > >>>>> +def regex_warn_factory(description): > >>>>> + return lambda: print_warning(description) > >>>> > >>>> Need extra newline, install flake8 and you will get the error at > compile > >>>> time. > >>>> > >>>>> > >>>>> std_functions = [ > >>>>> ('malloc', 'Use xmalloc() in place of malloc()'), > >>>>> @@ -636,6 +638,7 @@ std_functions = [ > >>>>> ('assert', 'Use ovs_assert() in place of assert()'), > >>>>> ('error', 'Use ovs_error() in place of error()'), > >>>>> ] > >>>>> + > >>>>> checks += [ > >>>>> {'regex': r'(\.c|\.h)(\.in)?$', > >>>>> 'match_name': None, > >>>>> @@ -644,6 +647,21 @@ checks += [ > >>>>> 'print': regex_error_factory(description)} > >>>>> for (function_name, description) in std_functions] > >>>>> > >>>>> +experimental_api = [ > >>>> > >>>> I do not think this is an experimental API, I would call it something > >> like > >>>> a suspicious API maybe? > >>>> > >>> > >>> or change to easy_to_misused_api? > >> > >> This is fine by me to. > >> > >>>> > >>>>> + ('ovsrcu_barrier', > >>>>> + 'lib/ovs-rcu.c', > >>>>> + 'Are you sure you need to use ovsrcu_barrier(),' > >>>> > >>>> Here you need to add a space at the end, as the error message now > looks > >>>> like: > >>>> > >>>> WARNING: Are you sure you need to use ovsrcu_barrier(),in most cases > >>>> ovsrcu_synchronize() will be fine? > >>>> > >>>>> + 'in most cases ovsrcu_synchronize() will be fine?'), > >>>>> + ] > >>>>> + > >>>>> +checks += [ > >>>>> + {'regex': r'(\.c)(\.in)?$', > >>>>> + 'match_name': lambda x: x != location, > >>>>> + 'prereq': lambda x: not is_comment_line(x), > >>>>> + 'check': regex_function_factory(function_name), > >>>>> + 'print': regex_warn_factory(description)} > >>>>> + for (function_name, location, description) in experimental_api] > >>>>> + > >>>>> > >>>>> def regex_operator_factory(operator): > >>>>> regex = re.compile(r'^[^#][^"\']*[^ "]%s[^ "\'][^"]*' % > operator) > >>>>> @@ -676,12 +694,20 @@ def get_file_type_checks(filename): > >>>>> global checks > >>>>> checkList = [] > >>>>> for check in checks: > >>>>> + regex_check = True > >>>>> + match_check = True > >>>>> + > >>>>> if check['regex'] is None and check['match_name'] is None: > >>>>> checkList.append(check) > >>>>> + continue > >>>>> + > >>>>> if check['regex'] is not None and \ > >>>>> - re.compile(check['regex']).search(filename) is not None: > >>>>> - checkList.append(check) > >>>>> - elif check['match_name'] is not None and > >>>> check['match_name'](filename): > >>>>> + re.compile(check['regex']).search(filename) is None: > >>>>> + regex_check = False > >>>>> + elif check['match_name'] is not None and not > >>>> check['match_name'](filename): > >>>> > >>>> Line is too long so you need to break it up: > >>>> > >>>> utilities/checkpatch.py:709:80: E501 line too long (83 > 79 > characters) > >>>> > >>>> > >>>>> + match_check = False > >>>>> + > >>>>> + if regex_check and match_check: > >>>>> checkList.append(check) > >>>> > >>>> Would it not make more sense to re-write the above elif cases to a > >> single > >>>> case? > >>>> > >>>>> return checkList > >>>>> > >>>>> -- > >>>>> 2.25.1 > >>>> > >>>> > >>> will send a new version， thanks for the detailed review. > >> > >> You can also add my Signed-off-by: if you make the suggested changes to > >> keep the robot happy. > >> > >> //Eelco > >> > >> > > > > -- > > hepeng > >
On 17 May 2022, at 9:20, Peng He wrote: > You mean I can ignore the following warnings? > > " > checkpatch: > ERROR: Author Peng He <xnhp0320@gmail.com> needs to sign off. > WARNING: Unexpected sign-offs from developers who are not authors or > co-authors or committers: Peng He <hepeng.0320@bytedance.com> > Lines checked: 98, Warnings: 1, Errors: 1 > " I think these are because of your mixed email address. Ilya can probably answer this, but I think he will fix this on commit, Ilya? > > Eelco Chaudron <echaudro@redhat.com> 于2022年5月17日周二 14:26写道： > >> >> >> On 17 May 2022, at 7:49, Peng He wrote: >> >>> The only issue I have is that I have to use the company's email address >> to >>> submit patches >>> while I am using the personal email to subscribe to the maillist. >>> >>> So the bot will always warn about the author needing to sign-off. >>> Perhaps in the future I would use both email addresses to avoid this >>> warning. >> >> There is this error on patch 1/3: >> >> ERROR: Co-author Eelco Chaudron <echaudro@redhat.com> needs to sign off. >> >> So I guess you need to add my sign-off also in that patch. >>> >>> >>> Eelco Chaudron <echaudro@redhat.com> 于2022年5月16日周一 19:36写道： >>> >>>> >>>> >>>> On 16 May 2022, at 12:19, Peng He wrote: >>>> >>>>> Eelco Chaudron <echaudro@redhat.com> 于2022年5月16日周一 17:12写道： >>>>> >>>>>> >>>>>> >>>>>> On 14 May 2022, at 10:40, Peng He wrote: >>>>>> >>>>>>> Signed-off-by: Peng He <hepeng.0320@bytedance.com> >>>>>>> --- >>>>>>> utilities/checkpatch.py | 32 +++++++++++++++++++++++++++++--- >>>>>>> 1 file changed, 29 insertions(+), 3 deletions(-) >>>>>>> >>>>>>> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py >>>>>>> index 8c7faa419..67c517b69 100755 >>>>>>> --- a/utilities/checkpatch.py >>>>>>> +++ b/utilities/checkpatch.py >>>>>>> @@ -619,6 +619,8 @@ def regex_function_factory(func_name): >>>>>>> def regex_error_factory(description): >>>>>>> return lambda: print_error(description) >>>>>>> >>>>>> >>>>>> Need extra newline, install flake8 and you will get the error at >> compile >>>>>> time. >>>>>> >>>>>>> +def regex_warn_factory(description): >>>>>>> + return lambda: print_warning(description) >>>>>> >>>>>> Need extra newline, install flake8 and you will get the error at >> compile >>>>>> time. >>>>>> >>>>>>> >>>>>>> std_functions = [ >>>>>>> ('malloc', 'Use xmalloc() in place of malloc()'), >>>>>>> @@ -636,6 +638,7 @@ std_functions = [ >>>>>>> ('assert', 'Use ovs_assert() in place of assert()'), >>>>>>> ('error', 'Use ovs_error() in place of error()'), >>>>>>> ] >>>>>>> + >>>>>>> checks += [ >>>>>>> {'regex': r'(\.c|\.h)(\.in)?$', >>>>>>> 'match_name': None, >>>>>>> @@ -644,6 +647,21 @@ checks += [ >>>>>>> 'print': regex_error_factory(description)} >>>>>>> for (function_name, description) in std_functions] >>>>>>> >>>>>>> +experimental_api = [ >>>>>> >>>>>> I do not think this is an experimental API, I would call it something >>>> like >>>>>> a suspicious API maybe? >>>>>> >>>>> >>>>> or change to easy_to_misused_api? >>>> >>>> This is fine by me to. >>>> >>>>>> >>>>>>> + ('ovsrcu_barrier', >>>>>>> + 'lib/ovs-rcu.c', >>>>>>> + 'Are you sure you need to use ovsrcu_barrier(),' >>>>>> >>>>>> Here you need to add a space at the end, as the error message now >> looks >>>>>> like: >>>>>> >>>>>> WARNING: Are you sure you need to use ovsrcu_barrier(),in most cases >>>>>> ovsrcu_synchronize() will be fine? >>>>>> >>>>>>> + 'in most cases ovsrcu_synchronize() will be fine?'), >>>>>>> + ] >>>>>>> + >>>>>>> +checks += [ >>>>>>> + {'regex': r'(\.c)(\.in)?$', >>>>>>> + 'match_name': lambda x: x != location, >>>>>>> + 'prereq': lambda x: not is_comment_line(x), >>>>>>> + 'check': regex_function_factory(function_name), >>>>>>> + 'print': regex_warn_factory(description)} >>>>>>> + for (function_name, location, description) in experimental_api] >>>>>>> + >>>>>>> >>>>>>> def regex_operator_factory(operator): >>>>>>> regex = re.compile(r'^[^#][^"\']*[^ "]%s[^ "\'][^"]*' % >> operator) >>>>>>> @@ -676,12 +694,20 @@ def get_file_type_checks(filename): >>>>>>> global checks >>>>>>> checkList = [] >>>>>>> for check in checks: >>>>>>> + regex_check = True >>>>>>> + match_check = True >>>>>>> + >>>>>>> if check['regex'] is None and check['match_name'] is None: >>>>>>> checkList.append(check) >>>>>>> + continue >>>>>>> + >>>>>>> if check['regex'] is not None and \ >>>>>>> - re.compile(check['regex']).search(filename) is not None: >>>>>>> - checkList.append(check) >>>>>>> - elif check['match_name'] is not None and >>>>>> check['match_name'](filename): >>>>>>> + re.compile(check['regex']).search(filename) is None: >>>>>>> + regex_check = False >>>>>>> + elif check['match_name'] is not None and not >>>>>> check['match_name'](filename): >>>>>> >>>>>> Line is too long so you need to break it up: >>>>>> >>>>>> utilities/checkpatch.py:709:80: E501 line too long (83 > 79 >> characters) >>>>>> >>>>>> >>>>>>> + match_check = False >>>>>>> + >>>>>>> + if regex_check and match_check: >>>>>>> checkList.append(check) >>>>>> >>>>>> Would it not make more sense to re-write the above elif cases to a >>>> single >>>>>> case? >>>>>> >>>>>>> return checkList >>>>>>> >>>>>>> -- >>>>>>> 2.25.1 >>>>>> >>>>>> >>>>> will send a new version， thanks for the detailed review. >>>> >>>> You can also add my Signed-off-by: if you make the suggested changes to >>>> keep the robot happy. >>>> >>>> //Eelco >>>> >>>> >>> >>> -- >>> hepeng >> >> > > -- > hepeng
looks like I am replying to the wrong thread. will send a new email to the latest one ... Eelco Chaudron <echaudro@redhat.com> 于2022年5月17日周二 16:51写道： > > > On 17 May 2022, at 9:20, Peng He wrote: > > > You mean I can ignore the following warnings? > > > > " > > checkpatch: > > ERROR: Author Peng He <xnhp0320@gmail.com> needs to sign off. > > WARNING: Unexpected sign-offs from developers who are not authors or > > co-authors or committers: Peng He <hepeng.0320@bytedance.com> > > Lines checked: 98, Warnings: 1, Errors: 1 > > " > > I think these are because of your mixed email address. Ilya can probably > answer this, but I think he will fix this on commit, Ilya? > > > > > Eelco Chaudron <echaudro@redhat.com> 于2022年5月17日周二 14:26写道： > > > >> > >> > >> On 17 May 2022, at 7:49, Peng He wrote: > >> > >>> The only issue I have is that I have to use the company's email address > >> to > >>> submit patches > >>> while I am using the personal email to subscribe to the maillist. > >>> > >>> So the bot will always warn about the author needing to sign-off. > >>> Perhaps in the future I would use both email addresses to avoid this > >>> warning. > >> > >> There is this error on patch 1/3: > >> > >> ERROR: Co-author Eelco Chaudron <echaudro@redhat.com> needs to sign > off. > >> > >> So I guess you need to add my sign-off also in that patch. > >>> > >>> > >>> Eelco Chaudron <echaudro@redhat.com> 于2022年5月16日周一 19:36写道： > >>> > >>>> > >>>> > >>>> On 16 May 2022, at 12:19, Peng He wrote: > >>>> > >>>>> Eelco Chaudron <echaudro@redhat.com> 于2022年5月16日周一 17:12写道： > >>>>> > >>>>>> > >>>>>> > >>>>>> On 14 May 2022, at 10:40, Peng He wrote: > >>>>>> > >>>>>>> Signed-off-by: Peng He <hepeng.0320@bytedance.com> > >>>>>>> --- > >>>>>>> utilities/checkpatch.py | 32 +++++++++++++++++++++++++++++--- > >>>>>>> 1 file changed, 29 insertions(+), 3 deletions(-) > >>>>>>> > >>>>>>> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py > >>>>>>> index 8c7faa419..67c517b69 100755 > >>>>>>> --- a/utilities/checkpatch.py > >>>>>>> +++ b/utilities/checkpatch.py > >>>>>>> @@ -619,6 +619,8 @@ def regex_function_factory(func_name): > >>>>>>> def regex_error_factory(description): > >>>>>>> return lambda: print_error(description) > >>>>>>> > >>>>>> > >>>>>> Need extra newline, install flake8 and you will get the error at > >> compile > >>>>>> time. > >>>>>> > >>>>>>> +def regex_warn_factory(description): > >>>>>>> + return lambda: print_warning(description) > >>>>>> > >>>>>> Need extra newline, install flake8 and you will get the error at > >> compile > >>>>>> time. > >>>>>> > >>>>>>> > >>>>>>> std_functions = [ > >>>>>>> ('malloc', 'Use xmalloc() in place of malloc()'), > >>>>>>> @@ -636,6 +638,7 @@ std_functions = [ > >>>>>>> ('assert', 'Use ovs_assert() in place of assert()'), > >>>>>>> ('error', 'Use ovs_error() in place of error()'), > >>>>>>> ] > >>>>>>> + > >>>>>>> checks += [ > >>>>>>> {'regex': r'(\.c|\.h)(\.in)?$', > >>>>>>> 'match_name': None, > >>>>>>> @@ -644,6 +647,21 @@ checks += [ > >>>>>>> 'print': regex_error_factory(description)} > >>>>>>> for (function_name, description) in std_functions] > >>>>>>> > >>>>>>> +experimental_api = [ > >>>>>> > >>>>>> I do not think this is an experimental API, I would call it > something > >>>> like > >>>>>> a suspicious API maybe? > >>>>>> > >>>>> > >>>>> or change to easy_to_misused_api? > >>>> > >>>> This is fine by me to. > >>>> > >>>>>> > >>>>>>> + ('ovsrcu_barrier', > >>>>>>> + 'lib/ovs-rcu.c', > >>>>>>> + 'Are you sure you need to use ovsrcu_barrier(),' > >>>>>> > >>>>>> Here you need to add a space at the end, as the error message now > >> looks > >>>>>> like: > >>>>>> > >>>>>> WARNING: Are you sure you need to use ovsrcu_barrier(),in most > cases > >>>>>> ovsrcu_synchronize() will be fine? > >>>>>> > >>>>>>> + 'in most cases ovsrcu_synchronize() will be fine?'), > >>>>>>> + ] > >>>>>>> + > >>>>>>> +checks += [ > >>>>>>> + {'regex': r'(\.c)(\.in)?$', > >>>>>>> + 'match_name': lambda x: x != location, > >>>>>>> + 'prereq': lambda x: not is_comment_line(x), > >>>>>>> + 'check': regex_function_factory(function_name), > >>>>>>> + 'print': regex_warn_factory(description)} > >>>>>>> + for (function_name, location, description) in > experimental_api] > >>>>>>> + > >>>>>>> > >>>>>>> def regex_operator_factory(operator): > >>>>>>> regex = re.compile(r'^[^#][^"\']*[^ "]%s[^ "\'][^"]*' % > >> operator) > >>>>>>> @@ -676,12 +694,20 @@ def get_file_type_checks(filename): > >>>>>>> global checks > >>>>>>> checkList = [] > >>>>>>> for check in checks: > >>>>>>> + regex_check = True > >>>>>>> + match_check = True > >>>>>>> + > >>>>>>> if check['regex'] is None and check['match_name'] is None: > >>>>>>> checkList.append(check) > >>>>>>> + continue > >>>>>>> + > >>>>>>> if check['regex'] is not None and \ > >>>>>>> - re.compile(check['regex']).search(filename) is not > None: > >>>>>>> - checkList.append(check) > >>>>>>> - elif check['match_name'] is not None and > >>>>>> check['match_name'](filename): > >>>>>>> + re.compile(check['regex']).search(filename) is None: > >>>>>>> + regex_check = False > >>>>>>> + elif check['match_name'] is not None and not > >>>>>> check['match_name'](filename): > >>>>>> > >>>>>> Line is too long so you need to break it up: > >>>>>> > >>>>>> utilities/checkpatch.py:709:80: E501 line too long (83 > 79 > >> characters) > >>>>>> > >>>>>> > >>>>>>> + match_check = False > >>>>>>> + > >>>>>>> + if regex_check and match_check: > >>>>>>> checkList.append(check) > >>>>>> > >>>>>> Would it not make more sense to re-write the above elif cases to a > >>>> single > >>>>>> case? > >>>>>> > >>>>>>> return checkList > >>>>>>> > >>>>>>> -- > >>>>>>> 2.25.1 > >>>>>> > >>>>>> > >>>>> will send a new version， thanks for the detailed review. > >>>> > >>>> You can also add my Signed-off-by: if you make the suggested changes > to > >>>> keep the robot happy. > >>>> > >>>> //Eelco > >>>> > >>>> > >>> > >>> -- > >>> hepeng > >> > >> > > > > -- > > hepeng > >
diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py index 8c7faa419..67c517b69 100755 --- a/utilities/checkpatch.py +++ b/utilities/checkpatch.py @@ -619,6 +619,8 @@ def regex_function_factory(func_name): def regex_error_factory(description): return lambda: print_error(description) +def regex_warn_factory(description): + return lambda: print_warning(description) std_functions = [ ('malloc', 'Use xmalloc() in place of malloc()'), @@ -636,6 +638,7 @@ std_functions = [ ('assert', 'Use ovs_assert() in place of assert()'), ('error', 'Use ovs_error() in place of error()'), ] + checks += [ {'regex': r'(\.c|\.h)(\.in)?$', 'match_name': None, @@ -644,6 +647,21 @@ checks += [ 'print': regex_error_factory(description)} for (function_name, description) in std_functions] +experimental_api = [ + ('ovsrcu_barrier', + 'lib/ovs-rcu.c', + 'Are you sure you need to use ovsrcu_barrier(),' + 'in most cases ovsrcu_synchronize() will be fine?'), + ] + +checks += [ + {'regex': r'(\.c)(\.in)?$', + 'match_name': lambda x: x != location, + 'prereq': lambda x: not is_comment_line(x), + 'check': regex_function_factory(function_name), + 'print': regex_warn_factory(description)} + for (function_name, location, description) in experimental_api] + def regex_operator_factory(operator): regex = re.compile(r'^[^#][^"\']*[^ "]%s[^ "\'][^"]*' % operator) @@ -676,12 +694,20 @@ def get_file_type_checks(filename): global checks checkList = [] for check in checks: + regex_check = True + match_check = True + if check['regex'] is None and check['match_name'] is None: checkList.append(check) + continue + if check['regex'] is not None and \ - re.compile(check['regex']).search(filename) is not None: - checkList.append(check) - elif check['match_name'] is not None and check['match_name'](filename): + re.compile(check['regex']).search(filename) is None: + regex_check = False + elif check['match_name'] is not None and not check['match_name'](filename): + match_check = False + + if regex_check and match_check: checkList.append(check) return checkList
Signed-off-by: Peng He <hepeng.0320@bytedance.com> --- utilities/checkpatch.py | 32 +++++++++++++++++++++++++++++--- 1 file changed, 29 insertions(+), 3 deletions(-)