* Re: [lustre-devel] Remaining work needed for moving Lustre out of staging
@ 2016-12-06 17:50 ` Oleg Drokin
0 siblings, 0 replies; 24+ messages in thread
From: Oleg Drokin @ 2016-12-06 17:50 UTC (permalink / raw)
To: Greg KH; +Cc: linux-fsdevel, James Simmons, lustre-devel@lists.lustre.org List
On Dec 6, 2016, at 11:15 AM, Greg KH wrote:
> On Tue, Dec 06, 2016 at 10:34:03AM -0500, Oleg Drokin wrote:
>>
>> On Dec 3, 2016, at 3:55 AM, gregkh@linuxfoundation.org wrote:
>>
>>> On Fri, Dec 02, 2016 at 09:53:08PM +0000, Dilger, Andreas wrote:
>>>> Al,
>>>> Greg recently raised the issue of what still needs to be done to
>>>> move the Lustre code out of staging/ and into the fs/ tree.
>>>>
>>>> James has been doing a great job of cleaning up various checkpatch
>>>> issues and keeping the code updated with the latest fixes, but we
>>>> were wondering what you were aware of that needed to be cleaned
>>>> up in Lustre?
>>>
>>> Is the whole "mixing kernel structures in userspace structures" all
>>> resolved now? For some reason I thought that you had kernel locks being
>>> passed to userspace and then back into the kernel, but it's been a long
>>> time since I last looked...
>>
>> While we certainly had our share of mixing user/kernelspace structures,
>> I don't think we ever passed anything with locks around back and forth.
>>
>> I just did a brief check and I don't see anything glaring on this particular front.
>>
>>> If you feel you are ready for a "real" review, I'll be glad to go over
>>> the code before the vfs people look at it, just let me know. No need to
>>> bother them if you still have basic things wrong that I can find�
>>
>> I think this would be beneficial at this stage.
>
> I see loads of checkpatch.pl warnings and a few errors, how about fixing
> all of them up first?
>
There are 16 errors,
of them:
8 are false positives in drivers/staging/lustre/lnet/libcfs/hash.c
1 false positive in drivers/staging/lustre/lnet/libcfs/tracefile.h
1 false positive in drivers/staging/lustre/lustre/include/lustre/lustre_idl.h
1 I think false positive in drivers/staging/lustre/lustre/include/lustre/lustre_idl.h (PLDLMRES)
2 false positives in drivers/staging/lustre/lustre/include/lustre/lustre_user.h
an error I don't really understand, what does it want us to do here?
ERROR: trailing statements should be on next line
#196951: FILE: drivers/staging/lustre/lustre/mdc/mdc_request.c:1063:
+ for (end_dirent = ent; ent;
+ end_dirent = ent, ent = lu_dirent_next(ent));
leaving 2 real ones, one missing space in drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c
and 1 spaces instead of tabs in drivers/staging/lustre/lnet/klnds/socklnd/socklnd.h
I'll have patches shortly for these.
On the warning front, we have 879 line over 80 characters, but a bunch
of those are due to long error messages that we cannot split into a multiline thing anyway.
71 Prefer 'unsigned int' to bare use of 'unsigned' that is a relatively new one
22 new typedefs (all in lnet, I drove out all the lustre ones, so I guess I can tackle these too).
a bunch of comment style problems
a few "function definition argument � should also have an identifier name" that
seems to be a new one too, I don't remember seeing this before.
And a bunch of rarer ones for the total of 1243 (823 in just Lustre).
Multiple classes of them are actually not easily fixable or false positives too.
Do you really want all of these fixed too somehow?
^ permalink raw reply [flat|nested] 24+ messages in thread
* [lustre-devel] Remaining work needed for moving Lustre out of staging
2016-12-06 17:50 ` Oleg Drokin
@ 2016-12-06 18:05 ` Greg KH
-1 siblings, 0 replies; 24+ messages in thread
From: Greg KH @ 2016-12-06 18:05 UTC (permalink / raw)
To: Oleg Drokin
Cc: linux-fsdevel, James Simmons, lustre-devel@lists.lustre.org List
On Tue, Dec 06, 2016 at 12:50:08PM -0500, Oleg Drokin wrote:
>
> On Dec 6, 2016, at 11:15 AM, Greg KH wrote:
>
> > On Tue, Dec 06, 2016 at 10:34:03AM -0500, Oleg Drokin wrote:
> >>
> >> On Dec 3, 2016, at 3:55 AM, gregkh at linuxfoundation.org wrote:
> >>
> >>> On Fri, Dec 02, 2016 at 09:53:08PM +0000, Dilger, Andreas wrote:
> >>>> Al,
> >>>> Greg recently raised the issue of what still needs to be done to
> >>>> move the Lustre code out of staging/ and into the fs/ tree.
> >>>>
> >>>> James has been doing a great job of cleaning up various checkpatch
> >>>> issues and keeping the code updated with the latest fixes, but we
> >>>> were wondering what you were aware of that needed to be cleaned
> >>>> up in Lustre?
> >>>
> >>> Is the whole "mixing kernel structures in userspace structures" all
> >>> resolved now? For some reason I thought that you had kernel locks being
> >>> passed to userspace and then back into the kernel, but it's been a long
> >>> time since I last looked...
> >>
> >> While we certainly had our share of mixing user/kernelspace structures,
> >> I don't think we ever passed anything with locks around back and forth.
> >>
> >> I just did a brief check and I don't see anything glaring on this particular front.
> >>
> >>> If you feel you are ready for a "real" review, I'll be glad to go over
> >>> the code before the vfs people look at it, just let me know. No need to
> >>> bother them if you still have basic things wrong that I can find?
> >>
> >> I think this would be beneficial at this stage.
> >
> > I see loads of checkpatch.pl warnings and a few errors, how about fixing
> > all of them up first?
> >
>
> There are 16 errors,
> of them:
> 8 are false positives in drivers/staging/lustre/lnet/libcfs/hash.c
> 1 false positive in drivers/staging/lustre/lnet/libcfs/tracefile.h
> 1 false positive in drivers/staging/lustre/lustre/include/lustre/lustre_idl.h
> 1 I think false positive in drivers/staging/lustre/lustre/include/lustre/lustre_idl.h (PLDLMRES)
> 2 false positives in drivers/staging/lustre/lustre/include/lustre/lustre_user.h
>
> an error I don't really understand, what does it want us to do here?
> ERROR: trailing statements should be on next line
> #196951: FILE: drivers/staging/lustre/lustre/mdc/mdc_request.c:1063:
> + for (end_dirent = ent; ent;
> + end_dirent = ent, ent = lu_dirent_next(ent));
That's odd, I think the perl script just got confused.
> leaving 2 real ones, one missing space in drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c
> and 1 spaces instead of tabs in drivers/staging/lustre/lnet/klnds/socklnd/socklnd.h
>
> I'll have patches shortly for these.
>
> On the warning front, we have 879 line over 80 characters, but a bunch
> of those are due to long error messages that we cannot split into a multiline thing anyway.
Change to use a "sane" logging function and then all of those will go
away. You all should do that anyway, the macro mess you have now is
looney.
> 71 Prefer 'unsigned int' to bare use of 'unsigned' that is a relatively new one
> 22 new typedefs (all in lnet, I drove out all the lustre ones, so I guess I can tackle these too).
> a bunch of comment style problems
> a few "function definition argument ? should also have an identifier name" that
> seems to be a new one too, I don't remember seeing this before.
>
> And a bunch of rarer ones for the total of 1243 (823 in just Lustre).
> Multiple classes of them are actually not easily fixable or false positives too.
> Do you really want all of these fixed too somehow?
The real ones, yes. Why wouldn't you?
And work on that logging mess if at all possible as well :)
thanks,
greg k-h
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [lustre-devel] Remaining work needed for moving Lustre out of staging
@ 2016-12-06 18:05 ` Greg KH
0 siblings, 0 replies; 24+ messages in thread
From: Greg KH @ 2016-12-06 18:05 UTC (permalink / raw)
To: Oleg Drokin
Cc: linux-fsdevel, James Simmons, lustre-devel@lists.lustre.org List
On Tue, Dec 06, 2016 at 12:50:08PM -0500, Oleg Drokin wrote:
>
> On Dec 6, 2016, at 11:15 AM, Greg KH wrote:
>
> > On Tue, Dec 06, 2016 at 10:34:03AM -0500, Oleg Drokin wrote:
> >>
> >> On Dec 3, 2016, at 3:55 AM, gregkh@linuxfoundation.org wrote:
> >>
> >>> On Fri, Dec 02, 2016 at 09:53:08PM +0000, Dilger, Andreas wrote:
> >>>> Al,
> >>>> Greg recently raised the issue of what still needs to be done to
> >>>> move the Lustre code out of staging/ and into the fs/ tree.
> >>>>
> >>>> James has been doing a great job of cleaning up various checkpatch
> >>>> issues and keeping the code updated with the latest fixes, but we
> >>>> were wondering what you were aware of that needed to be cleaned
> >>>> up in Lustre?
> >>>
> >>> Is the whole "mixing kernel structures in userspace structures" all
> >>> resolved now? For some reason I thought that you had kernel locks being
> >>> passed to userspace and then back into the kernel, but it's been a long
> >>> time since I last looked...
> >>
> >> While we certainly had our share of mixing user/kernelspace structures,
> >> I don't think we ever passed anything with locks around back and forth.
> >>
> >> I just did a brief check and I don't see anything glaring on this particular front.
> >>
> >>> If you feel you are ready for a "real" review, I'll be glad to go over
> >>> the code before the vfs people look at it, just let me know. No need to
> >>> bother them if you still have basic things wrong that I can find…
> >>
> >> I think this would be beneficial at this stage.
> >
> > I see loads of checkpatch.pl warnings and a few errors, how about fixing
> > all of them up first?
> >
>
> There are 16 errors,
> of them:
> 8 are false positives in drivers/staging/lustre/lnet/libcfs/hash.c
> 1 false positive in drivers/staging/lustre/lnet/libcfs/tracefile.h
> 1 false positive in drivers/staging/lustre/lustre/include/lustre/lustre_idl.h
> 1 I think false positive in drivers/staging/lustre/lustre/include/lustre/lustre_idl.h (PLDLMRES)
> 2 false positives in drivers/staging/lustre/lustre/include/lustre/lustre_user.h
>
> an error I don't really understand, what does it want us to do here?
> ERROR: trailing statements should be on next line
> #196951: FILE: drivers/staging/lustre/lustre/mdc/mdc_request.c:1063:
> + for (end_dirent = ent; ent;
> + end_dirent = ent, ent = lu_dirent_next(ent));
That's odd, I think the perl script just got confused.
> leaving 2 real ones, one missing space in drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c
> and 1 spaces instead of tabs in drivers/staging/lustre/lnet/klnds/socklnd/socklnd.h
>
> I'll have patches shortly for these.
>
> On the warning front, we have 879 line over 80 characters, but a bunch
> of those are due to long error messages that we cannot split into a multiline thing anyway.
Change to use a "sane" logging function and then all of those will go
away. You all should do that anyway, the macro mess you have now is
looney.
> 71 Prefer 'unsigned int' to bare use of 'unsigned' that is a relatively new one
> 22 new typedefs (all in lnet, I drove out all the lustre ones, so I guess I can tackle these too).
> a bunch of comment style problems
> a few "function definition argument … should also have an identifier name" that
> seems to be a new one too, I don't remember seeing this before.
>
> And a bunch of rarer ones for the total of 1243 (823 in just Lustre).
> Multiple classes of them are actually not easily fixable or false positives too.
> Do you really want all of these fixed too somehow?
The real ones, yes. Why wouldn't you?
And work on that logging mess if at all possible as well :)
thanks,
greg k-h
^ permalink raw reply [flat|nested] 24+ messages in thread
* [lustre-devel] Remaining work needed for moving Lustre out of staging
2016-12-06 17:50 ` Oleg Drokin
@ 2016-12-06 18:05 ` Andreas Dilger
-1 siblings, 0 replies; 24+ messages in thread
From: Andreas Dilger @ 2016-12-06 18:05 UTC (permalink / raw)
To: Oleg Drokin
Cc: Greg KH, linux-fsdevel, lustre-devel@lists.lustre.org List,
James Simmons
On Dec 6, 2016, at 10:50 AM, Oleg Drokin <oleg.drokin@intel.com> wrote:
>
> On Dec 6, 2016, at 11:15 AM, Greg KH wrote:
>> I see loads of checkpatch.pl warnings and a few errors, how about fixing
>> all of them up first?
>
> There are 16 errors,
> of them:
> 8 are false positives in drivers/staging/lustre/lnet/libcfs/hash.c
> 1 false positive in drivers/staging/lustre/lnet/libcfs/tracefile.h
> 1 false positive in drivers/staging/lustre/lustre/include/lustre/lustre_idl.h
> 1 I think false positive in drivers/staging/lustre/lustre/include/lustre/lustre_idl.h (PLDLMRES)
> 2 false positives in drivers/staging/lustre/lustre/include/lustre/lustre_user.h
>
> an error I don't really understand, what does it want us to do here?
> ERROR: trailing statements should be on next line
> #196951: FILE: drivers/staging/lustre/lustre/mdc/mdc_request.c:1063:
> + for (end_dirent = ent; ent;
> + end_dirent = ent, ent = lu_dirent_next(ent));
>
> leaving 2 real ones, one missing space in drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c
> and 1 spaces instead of tabs in drivers/staging/lustre/lnet/klnds/socklnd/socklnd.h
>
> I'll have patches shortly for these.
>
> On the warning front, we have 879 line over 80 characters, but a bunch
> of those are due to long error messages that we cannot split into a multiline thing anyway.
> 71 Prefer 'unsigned int' to bare use of 'unsigned' that is a relatively new one
> 22 new typedefs (all in lnet, I drove out all the lustre ones, so I guess I can tackle these too).
I think James already has a patch series getting rid of typedefs from LNet?
> a bunch of comment style problems
> a few "function definition argument ? should also have an identifier name" that
> seems to be a new one too, I don't remember seeing this before.
>
> And a bunch of rarer ones for the total of 1243 (823 in just Lustre).
> Multiple classes of them are actually not easily fixable or false positives too.
> Do you really want all of these fixed too somehow?
Cheers, Andreas
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Message signed with OpenPGP using GPGMail
URL: <http://lists.lustre.org/pipermail/lustre-devel-lustre.org/attachments/20161206/3f0d95e5/attachment.pgp>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [lustre-devel] Remaining work needed for moving Lustre out of staging
@ 2016-12-06 18:05 ` Andreas Dilger
0 siblings, 0 replies; 24+ messages in thread
From: Andreas Dilger @ 2016-12-06 18:05 UTC (permalink / raw)
To: Oleg Drokin
Cc: Greg KH, linux-fsdevel, lustre-devel@lists.lustre.org List,
James Simmons
[-- Attachment #1: Type: text/plain, Size: 2113 bytes --]
On Dec 6, 2016, at 10:50 AM, Oleg Drokin <oleg.drokin@intel.com> wrote:
>
> On Dec 6, 2016, at 11:15 AM, Greg KH wrote:
>> I see loads of checkpatch.pl warnings and a few errors, how about fixing
>> all of them up first?
>
> There are 16 errors,
> of them:
> 8 are false positives in drivers/staging/lustre/lnet/libcfs/hash.c
> 1 false positive in drivers/staging/lustre/lnet/libcfs/tracefile.h
> 1 false positive in drivers/staging/lustre/lustre/include/lustre/lustre_idl.h
> 1 I think false positive in drivers/staging/lustre/lustre/include/lustre/lustre_idl.h (PLDLMRES)
> 2 false positives in drivers/staging/lustre/lustre/include/lustre/lustre_user.h
>
> an error I don't really understand, what does it want us to do here?
> ERROR: trailing statements should be on next line
> #196951: FILE: drivers/staging/lustre/lustre/mdc/mdc_request.c:1063:
> + for (end_dirent = ent; ent;
> + end_dirent = ent, ent = lu_dirent_next(ent));
>
> leaving 2 real ones, one missing space in drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c
> and 1 spaces instead of tabs in drivers/staging/lustre/lnet/klnds/socklnd/socklnd.h
>
> I'll have patches shortly for these.
>
> On the warning front, we have 879 line over 80 characters, but a bunch
> of those are due to long error messages that we cannot split into a multiline thing anyway.
> 71 Prefer 'unsigned int' to bare use of 'unsigned' that is a relatively new one
> 22 new typedefs (all in lnet, I drove out all the lustre ones, so I guess I can tackle these too).
I think James already has a patch series getting rid of typedefs from LNet?
> a bunch of comment style problems
> a few "function definition argument … should also have an identifier name" that
> seems to be a new one too, I don't remember seeing this before.
>
> And a bunch of rarer ones for the total of 1243 (823 in just Lustre).
> Multiple classes of them are actually not easily fixable or false positives too.
> Do you really want all of these fixed too somehow?
Cheers, Andreas
[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* [lustre-devel] Remaining work needed for moving Lustre out of staging
2016-12-06 18:05 ` Andreas Dilger
@ 2016-12-07 19:57 ` James Simmons
-1 siblings, 0 replies; 24+ messages in thread
From: James Simmons @ 2016-12-07 19:57 UTC (permalink / raw)
To: Andreas Dilger
Cc: Oleg Drokin, Greg KH, linux-fsdevel,
lustre-devel@lists.lustre.org List
> > On the warning front, we have 879 line over 80 characters, but a bunch
> > of those are due to long error messages that we cannot split into a multiline thing anyway.
> > 71 Prefer 'unsigned int' to bare use of 'unsigned' that is a relatively new one
> > 22 new typedefs (all in lnet, I drove out all the lustre ones, so I guess I can tackle these too).
>
> I think James already has a patch series getting rid of typedefs from LNet?
I created a big patch some time ago but I need to break it into smaller
pieces. We should sync our tools to be able to build against these changes
as well. I pushed patches to change our tools.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [lustre-devel] Remaining work needed for moving Lustre out of staging
@ 2016-12-07 19:57 ` James Simmons
0 siblings, 0 replies; 24+ messages in thread
From: James Simmons @ 2016-12-07 19:57 UTC (permalink / raw)
To: Andreas Dilger
Cc: Oleg Drokin, Greg KH, linux-fsdevel,
lustre-devel@lists.lustre.org List
> > On the warning front, we have 879 line over 80 characters, but a bunch
> > of those are due to long error messages that we cannot split into a multiline thing anyway.
> > 71 Prefer 'unsigned int' to bare use of 'unsigned' that is a relatively new one
> > 22 new typedefs (all in lnet, I drove out all the lustre ones, so I guess I can tackle these too).
>
> I think James already has a patch series getting rid of typedefs from LNet?
I created a big patch some time ago but I need to break it into smaller
pieces. We should sync our tools to be able to build against these changes
as well. I pushed patches to change our tools.
^ permalink raw reply [flat|nested] 24+ messages in thread
* [lustre-devel] Remaining work needed for moving Lustre out of staging
@ 2016-12-06 18:07 ` Mike Marshall
0 siblings, 0 replies; 24+ messages in thread
From: Mike Marshall @ 2016-12-06 18:07 UTC (permalink / raw)
To: Oleg Drokin
Cc: linux-fsdevel, Greg KH,
lustre-devel-aLEFhgZF4x6X6Mz3xDxJMA@public.gmane.org List,
James Simmons
> long error messages that we cannot split into a multiline thing anyway.
I split numerous Orangefs ones at substitution characters, which
didn't break grepability. No one complained, and sure made
stuff look nicer to me. You might be able to do that some places?
-Mike
On Tue, Dec 6, 2016 at 12:50 PM, Oleg Drokin <oleg.drokin@intel.com> wrote:
>
> On Dec 6, 2016, at 11:15 AM, Greg KH wrote:
>
> > On Tue, Dec 06, 2016 at 10:34:03AM -0500, Oleg Drokin wrote:
> >>
> >> On Dec 3, 2016, at 3:55 AM, gregkh at linuxfoundation.org wrote:
> >>
> >>> On Fri, Dec 02, 2016 at 09:53:08PM +0000, Dilger, Andreas wrote:
> >>>> Al,
> >>>> Greg recently raised the issue of what still needs to be done to
> >>>> move the Lustre code out of staging/ and into the fs/ tree.
> >>>>
> >>>> James has been doing a great job of cleaning up various checkpatch
> >>>> issues and keeping the code updated with the latest fixes, but we
> >>>> were wondering what you were aware of that needed to be cleaned
> >>>> up in Lustre?
> >>>
> >>> Is the whole "mixing kernel structures in userspace structures" all
> >>> resolved now? For some reason I thought that you had kernel locks
> being
> >>> passed to userspace and then back into the kernel, but it's been a long
> >>> time since I last looked...
> >>
> >> While we certainly had our share of mixing user/kernelspace structures,
> >> I don't think we ever passed anything with locks around back and forth.
> >>
> >> I just did a brief check and I don't see anything glaring on this
> particular front.
> >>
> >>> If you feel you are ready for a "real" review, I'll be glad to go over
> >>> the code before the vfs people look at it, just let me know. No need
> to
> >>> bother them if you still have basic things wrong that I can find?
> >>
> >> I think this would be beneficial at this stage.
> >
> > I see loads of checkpatch.pl warnings and a few errors, how about fixing
> > all of them up first?
> >
>
> There are 16 errors,
> of them:
> 8 are false positives in drivers/staging/lustre/lnet/libcfs/hash.c
> 1 false positive in drivers/staging/lustre/lnet/libcfs/tracefile.h
> 1 false positive in drivers/staging/lustre/lustre/
> include/lustre/lustre_idl.h
> 1 I think false positive in drivers/staging/lustre/lustre/include/lustre/lustre_idl.h
> (PLDLMRES)
> 2 false positives in drivers/staging/lustre/lustre/
> include/lustre/lustre_user.h
>
> an error I don't really understand, what does it want us to do here?
> ERROR: trailing statements should be on next line
> #196951: FILE: drivers/staging/lustre/lustre/mdc/mdc_request.c:1063:
> + for (end_dirent = ent; ent;
> + end_dirent = ent, ent = lu_dirent_next(ent));
>
> leaving 2 real ones, one missing space in drivers/staging/lustre/lnet/
> klnds/o2iblnd/o2iblnd_cb.c
> and 1 spaces instead of tabs in drivers/staging/lustre/lnet/
> klnds/socklnd/socklnd.h
>
> I'll have patches shortly for these.
>
> On the warning front, we have 879 line over 80 characters, but a bunch
> of those are due to long error messages that we cannot split into a
> multiline thing anyway.
> 71 Prefer 'unsigned int' to bare use of 'unsigned' that is a relatively
> new one
> 22 new typedefs (all in lnet, I drove out all the lustre ones, so I guess
> I can tackle these too).
> a bunch of comment style problems
> a few "function definition argument ? should also have an identifier name"
> that
> seems to be a new one too, I don't remember seeing this before.
>
> And a bunch of rarer ones for the total of 1243 (823 in just Lustre).
> Multiple classes of them are actually not easily fixable or false
> positives too.
> Do you really want all of these fixed too somehow?
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.lustre.org/pipermail/lustre-devel-lustre.org/attachments/20161206/52f5e557/attachment-0001.htm>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Remaining work needed for moving Lustre out of staging
@ 2016-12-06 18:07 ` Mike Marshall
0 siblings, 0 replies; 24+ messages in thread
From: Mike Marshall @ 2016-12-06 18:07 UTC (permalink / raw)
To: Oleg Drokin
Cc: linux-fsdevel, Greg KH,
lustre-devel-aLEFhgZF4x6X6Mz3xDxJMA@public.gmane.org List,
James Simmons
[-- Attachment #1.1: Type: text/plain, Size: 4095 bytes --]
> long error messages that we cannot split into a multiline thing anyway.
I split numerous Orangefs ones at substitution characters, which
didn't break grepability. No one complained, and sure made
stuff look nicer to me. You might be able to do that some places?
-Mike
On Tue, Dec 6, 2016 at 12:50 PM, Oleg Drokin <oleg.drokin-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
>
> On Dec 6, 2016, at 11:15 AM, Greg KH wrote:
>
> > On Tue, Dec 06, 2016 at 10:34:03AM -0500, Oleg Drokin wrote:
> >>
> >> On Dec 3, 2016, at 3:55 AM, gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org wrote:
> >>
> >>> On Fri, Dec 02, 2016 at 09:53:08PM +0000, Dilger, Andreas wrote:
> >>>> Al,
> >>>> Greg recently raised the issue of what still needs to be done to
> >>>> move the Lustre code out of staging/ and into the fs/ tree.
> >>>>
> >>>> James has been doing a great job of cleaning up various checkpatch
> >>>> issues and keeping the code updated with the latest fixes, but we
> >>>> were wondering what you were aware of that needed to be cleaned
> >>>> up in Lustre?
> >>>
> >>> Is the whole "mixing kernel structures in userspace structures" all
> >>> resolved now? For some reason I thought that you had kernel locks
> being
> >>> passed to userspace and then back into the kernel, but it's been a long
> >>> time since I last looked...
> >>
> >> While we certainly had our share of mixing user/kernelspace structures,
> >> I don't think we ever passed anything with locks around back and forth.
> >>
> >> I just did a brief check and I don't see anything glaring on this
> particular front.
> >>
> >>> If you feel you are ready for a "real" review, I'll be glad to go over
> >>> the code before the vfs people look at it, just let me know. No need
> to
> >>> bother them if you still have basic things wrong that I can find…
> >>
> >> I think this would be beneficial at this stage.
> >
> > I see loads of checkpatch.pl warnings and a few errors, how about fixing
> > all of them up first?
> >
>
> There are 16 errors,
> of them:
> 8 are false positives in drivers/staging/lustre/lnet/libcfs/hash.c
> 1 false positive in drivers/staging/lustre/lnet/libcfs/tracefile.h
> 1 false positive in drivers/staging/lustre/lustre/
> include/lustre/lustre_idl.h
> 1 I think false positive in drivers/staging/lustre/lustre/include/lustre/lustre_idl.h
> (PLDLMRES)
> 2 false positives in drivers/staging/lustre/lustre/
> include/lustre/lustre_user.h
>
> an error I don't really understand, what does it want us to do here?
> ERROR: trailing statements should be on next line
> #196951: FILE: drivers/staging/lustre/lustre/mdc/mdc_request.c:1063:
> + for (end_dirent = ent; ent;
> + end_dirent = ent, ent = lu_dirent_next(ent));
>
> leaving 2 real ones, one missing space in drivers/staging/lustre/lnet/
> klnds/o2iblnd/o2iblnd_cb.c
> and 1 spaces instead of tabs in drivers/staging/lustre/lnet/
> klnds/socklnd/socklnd.h
>
> I'll have patches shortly for these.
>
> On the warning front, we have 879 line over 80 characters, but a bunch
> of those are due to long error messages that we cannot split into a
> multiline thing anyway.
> 71 Prefer 'unsigned int' to bare use of 'unsigned' that is a relatively
> new one
> 22 new typedefs (all in lnet, I drove out all the lustre ones, so I guess
> I can tackle these too).
> a bunch of comment style problems
> a few "function definition argument … should also have an identifier name"
> that
> seems to be a new one too, I don't remember seeing this before.
>
> And a bunch of rarer ones for the total of 1243 (823 in just Lustre).
> Multiple classes of them are actually not easily fixable or false
> positives too.
> Do you really want all of these fixed too somehow?
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
[-- Attachment #1.2: Type: text/html, Size: 5325 bytes --]
[-- Attachment #2: Type: text/plain, Size: 188 bytes --]
_______________________________________________
lustre-devel mailing list
lustre-devel-aLEFhgZF4x6X6Mz3xDxJMA@public.gmane.org
http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org
^ permalink raw reply [flat|nested] 24+ messages in thread
* [lustre-devel] Remaining work needed for moving Lustre out of staging
2016-12-06 18:07 ` Mike Marshall
@ 2016-12-06 18:14 ` Oleg Drokin
-1 siblings, 0 replies; 24+ messages in thread
From: Oleg Drokin @ 2016-12-06 18:14 UTC (permalink / raw)
To: Mike Marshall
Cc: Greg KH, linux-fsdevel, James Simmons,
lustre-devel@lists.lustre.org List
On Dec 6, 2016, at 1:07 PM, Mike Marshall wrote:
> > long error messages that we cannot split into a multiline thing anyway.
>
> I split numerous Orangefs ones at substitution characters, which
> didn't break grepability. No one complained, and sure made
> stuff look nicer to me. You might be able to do that some places?
Checkpatch still complains about this I believe.
>
> -Mike
>
> On Tue, Dec 6, 2016 at 12:50 PM, Oleg Drokin <oleg.drokin@intel.com> wrote:
>
> On Dec 6, 2016, at 11:15 AM, Greg KH wrote:
>
> > On Tue, Dec 06, 2016 at 10:34:03AM -0500, Oleg Drokin wrote:
> >>
> >> On Dec 3, 2016, at 3:55 AM, gregkh at linuxfoundation.org wrote:
> >>
> >>> On Fri, Dec 02, 2016 at 09:53:08PM +0000, Dilger, Andreas wrote:
> >>>> Al,
> >>>> Greg recently raised the issue of what still needs to be done to
> >>>> move the Lustre code out of staging/ and into the fs/ tree.
> >>>>
> >>>> James has been doing a great job of cleaning up various checkpatch
> >>>> issues and keeping the code updated with the latest fixes, but we
> >>>> were wondering what you were aware of that needed to be cleaned
> >>>> up in Lustre?
> >>>
> >>> Is the whole "mixing kernel structures in userspace structures" all
> >>> resolved now? For some reason I thought that you had kernel locks being
> >>> passed to userspace and then back into the kernel, but it's been a long
> >>> time since I last looked...
> >>
> >> While we certainly had our share of mixing user/kernelspace structures,
> >> I don't think we ever passed anything with locks around back and forth.
> >>
> >> I just did a brief check and I don't see anything glaring on this particular front.
> >>
> >>> If you feel you are ready for a "real" review, I'll be glad to go over
> >>> the code before the vfs people look at it, just let me know. No need to
> >>> bother them if you still have basic things wrong that I can find?
> >>
> >> I think this would be beneficial at this stage.
> >
> > I see loads of checkpatch.pl warnings and a few errors, how about fixing
> > all of them up first?
> >
>
> There are 16 errors,
> of them:
> 8 are false positives in drivers/staging/lustre/lnet/libcfs/hash.c
> 1 false positive in drivers/staging/lustre/lnet/libcfs/tracefile.h
> 1 false positive in drivers/staging/lustre/lustre/include/lustre/lustre_idl.h
> 1 I think false positive in drivers/staging/lustre/lustre/include/lustre/lustre_idl.h (PLDLMRES)
> 2 false positives in drivers/staging/lustre/lustre/include/lustre/lustre_user.h
>
> an error I don't really understand, what does it want us to do here?
> ERROR: trailing statements should be on next line
> #196951: FILE: drivers/staging/lustre/lustre/mdc/mdc_request.c:1063:
> + for (end_dirent = ent; ent;
> + end_dirent = ent, ent = lu_dirent_next(ent));
>
> leaving 2 real ones, one missing space in drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c
> and 1 spaces instead of tabs in drivers/staging/lustre/lnet/klnds/socklnd/socklnd.h
>
> I'll have patches shortly for these.
>
> On the warning front, we have 879 line over 80 characters, but a bunch
> of those are due to long error messages that we cannot split into a multiline thing anyway.
> 71 Prefer 'unsigned int' to bare use of 'unsigned' that is a relatively new one
> 22 new typedefs (all in lnet, I drove out all the lustre ones, so I guess I can tackle these too).
> a bunch of comment style problems
> a few "function definition argument ? should also have an identifier name" that
> seems to be a new one too, I don't remember seeing this before.
>
> And a bunch of rarer ones for the total of 1243 (823 in just Lustre).
> Multiple classes of them are actually not easily fixable or false positives too.
> Do you really want all of these fixed too somehow?
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [lustre-devel] Remaining work needed for moving Lustre out of staging
@ 2016-12-06 18:14 ` Oleg Drokin
0 siblings, 0 replies; 24+ messages in thread
From: Oleg Drokin @ 2016-12-06 18:14 UTC (permalink / raw)
To: Mike Marshall
Cc: Greg KH, linux-fsdevel, James Simmons,
lustre-devel@lists.lustre.org List
On Dec 6, 2016, at 1:07 PM, Mike Marshall wrote:
> > long error messages that we cannot split into a multiline thing anyway.
>
> I split numerous Orangefs ones at substitution characters, which
> didn't break grepability. No one complained, and sure made
> stuff look nicer to me. You might be able to do that some places?
Checkpatch still complains about this I believe.
>
> -Mike
>
> On Tue, Dec 6, 2016 at 12:50 PM, Oleg Drokin <oleg.drokin@intel.com> wrote:
>
> On Dec 6, 2016, at 11:15 AM, Greg KH wrote:
>
> > On Tue, Dec 06, 2016 at 10:34:03AM -0500, Oleg Drokin wrote:
> >>
> >> On Dec 3, 2016, at 3:55 AM, gregkh@linuxfoundation.org wrote:
> >>
> >>> On Fri, Dec 02, 2016 at 09:53:08PM +0000, Dilger, Andreas wrote:
> >>>> Al,
> >>>> Greg recently raised the issue of what still needs to be done to
> >>>> move the Lustre code out of staging/ and into the fs/ tree.
> >>>>
> >>>> James has been doing a great job of cleaning up various checkpatch
> >>>> issues and keeping the code updated with the latest fixes, but we
> >>>> were wondering what you were aware of that needed to be cleaned
> >>>> up in Lustre?
> >>>
> >>> Is the whole "mixing kernel structures in userspace structures" all
> >>> resolved now? For some reason I thought that you had kernel locks being
> >>> passed to userspace and then back into the kernel, but it's been a long
> >>> time since I last looked...
> >>
> >> While we certainly had our share of mixing user/kernelspace structures,
> >> I don't think we ever passed anything with locks around back and forth.
> >>
> >> I just did a brief check and I don't see anything glaring on this particular front.
> >>
> >>> If you feel you are ready for a "real" review, I'll be glad to go over
> >>> the code before the vfs people look at it, just let me know. No need to
> >>> bother them if you still have basic things wrong that I can find�
> >>
> >> I think this would be beneficial at this stage.
> >
> > I see loads of checkpatch.pl warnings and a few errors, how about fixing
> > all of them up first?
> >
>
> There are 16 errors,
> of them:
> 8 are false positives in drivers/staging/lustre/lnet/libcfs/hash.c
> 1 false positive in drivers/staging/lustre/lnet/libcfs/tracefile.h
> 1 false positive in drivers/staging/lustre/lustre/include/lustre/lustre_idl.h
> 1 I think false positive in drivers/staging/lustre/lustre/include/lustre/lustre_idl.h (PLDLMRES)
> 2 false positives in drivers/staging/lustre/lustre/include/lustre/lustre_user.h
>
> an error I don't really understand, what does it want us to do here?
> ERROR: trailing statements should be on next line
> #196951: FILE: drivers/staging/lustre/lustre/mdc/mdc_request.c:1063:
> + for (end_dirent = ent; ent;
> + end_dirent = ent, ent = lu_dirent_next(ent));
>
> leaving 2 real ones, one missing space in drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c
> and 1 spaces instead of tabs in drivers/staging/lustre/lnet/klnds/socklnd/socklnd.h
>
> I'll have patches shortly for these.
>
> On the warning front, we have 879 line over 80 characters, but a bunch
> of those are due to long error messages that we cannot split into a multiline thing anyway.
> 71 Prefer 'unsigned int' to bare use of 'unsigned' that is a relatively new one
> 22 new typedefs (all in lnet, I drove out all the lustre ones, so I guess I can tackle these too).
> a bunch of comment style problems
> a few "function definition argument � should also have an identifier name" that
> seems to be a new one too, I don't remember seeing this before.
>
> And a bunch of rarer ones for the total of 1243 (823 in just Lustre).
> Multiple classes of them are actually not easily fixable or false positives too.
> Do you really want all of these fixed too somehow?
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* [lustre-devel] Remaining work needed for moving Lustre out of staging
2016-12-06 18:14 ` Oleg Drokin
@ 2016-12-06 18:52 ` Mike Marshall
-1 siblings, 0 replies; 24+ messages in thread
From: Mike Marshall @ 2016-12-06 18:52 UTC (permalink / raw)
To: Oleg Drokin
Cc: Greg KH, linux-fsdevel, James Simmons,
lustre-devel@lists.lustre.org List
> Checkpatch still complains about this I believe.
Yes. But if you leave the line longer than 80 characters,
it complains about that too. I hoped that since the point
was not to break grepability, it was better to make the
code look nicer. This is the first chance I've had to
say what I'd been doing when numerous senior
developers were looking, if my method is worse
than leaving the long lines, I'll quit doing it...
-Mike
On Tue, Dec 6, 2016 at 1:14 PM, Oleg Drokin <oleg.drokin@intel.com> wrote:
>
> On Dec 6, 2016, at 1:07 PM, Mike Marshall wrote:
>
>> > long error messages that we cannot split into a multiline thing anyway.
>>
>> I split numerous Orangefs ones at substitution characters, which
>> didn't break grepability. No one complained, and sure made
>> stuff look nicer to me. You might be able to do that some places?
>
> Checkpatch still complains about this I believe.
>
>>
>> -Mike
>>
>> On Tue, Dec 6, 2016 at 12:50 PM, Oleg Drokin <oleg.drokin@intel.com> wrote:
>>
>> On Dec 6, 2016, at 11:15 AM, Greg KH wrote:
>>
>> > On Tue, Dec 06, 2016 at 10:34:03AM -0500, Oleg Drokin wrote:
>> >>
>> >> On Dec 3, 2016, at 3:55 AM, gregkh at linuxfoundation.org wrote:
>> >>
>> >>> On Fri, Dec 02, 2016 at 09:53:08PM +0000, Dilger, Andreas wrote:
>> >>>> Al,
>> >>>> Greg recently raised the issue of what still needs to be done to
>> >>>> move the Lustre code out of staging/ and into the fs/ tree.
>> >>>>
>> >>>> James has been doing a great job of cleaning up various checkpatch
>> >>>> issues and keeping the code updated with the latest fixes, but we
>> >>>> were wondering what you were aware of that needed to be cleaned
>> >>>> up in Lustre?
>> >>>
>> >>> Is the whole "mixing kernel structures in userspace structures" all
>> >>> resolved now? For some reason I thought that you had kernel locks being
>> >>> passed to userspace and then back into the kernel, but it's been a long
>> >>> time since I last looked...
>> >>
>> >> While we certainly had our share of mixing user/kernelspace structures,
>> >> I don't think we ever passed anything with locks around back and forth.
>> >>
>> >> I just did a brief check and I don't see anything glaring on this particular front.
>> >>
>> >>> If you feel you are ready for a "real" review, I'll be glad to go over
>> >>> the code before the vfs people look at it, just let me know. No need to
>> >>> bother them if you still have basic things wrong that I can find?
>> >>
>> >> I think this would be beneficial at this stage.
>> >
>> > I see loads of checkpatch.pl warnings and a few errors, how about fixing
>> > all of them up first?
>> >
>>
>> There are 16 errors,
>> of them:
>> 8 are false positives in drivers/staging/lustre/lnet/libcfs/hash.c
>> 1 false positive in drivers/staging/lustre/lnet/libcfs/tracefile.h
>> 1 false positive in drivers/staging/lustre/lustre/include/lustre/lustre_idl.h
>> 1 I think false positive in drivers/staging/lustre/lustre/include/lustre/lustre_idl.h (PLDLMRES)
>> 2 false positives in drivers/staging/lustre/lustre/include/lustre/lustre_user.h
>>
>> an error I don't really understand, what does it want us to do here?
>> ERROR: trailing statements should be on next line
>> #196951: FILE: drivers/staging/lustre/lustre/mdc/mdc_request.c:1063:
>> + for (end_dirent = ent; ent;
>> + end_dirent = ent, ent = lu_dirent_next(ent));
>>
>> leaving 2 real ones, one missing space in drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c
>> and 1 spaces instead of tabs in drivers/staging/lustre/lnet/klnds/socklnd/socklnd.h
>>
>> I'll have patches shortly for these.
>>
>> On the warning front, we have 879 line over 80 characters, but a bunch
>> of those are due to long error messages that we cannot split into a multiline thing anyway.
>> 71 Prefer 'unsigned int' to bare use of 'unsigned' that is a relatively new one
>> 22 new typedefs (all in lnet, I drove out all the lustre ones, so I guess I can tackle these too).
>> a bunch of comment style problems
>> a few "function definition argument ? should also have an identifier name" that
>> seems to be a new one too, I don't remember seeing this before.
>>
>> And a bunch of rarer ones for the total of 1243 (823 in just Lustre).
>> Multiple classes of them are actually not easily fixable or false positives too.
>> Do you really want all of these fixed too somehow?
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
>> the body of a message to majordomo at vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [lustre-devel] Remaining work needed for moving Lustre out of staging
@ 2016-12-06 18:52 ` Mike Marshall
0 siblings, 0 replies; 24+ messages in thread
From: Mike Marshall @ 2016-12-06 18:52 UTC (permalink / raw)
To: Oleg Drokin
Cc: Greg KH, linux-fsdevel, James Simmons,
lustre-devel@lists.lustre.org List
> Checkpatch still complains about this I believe.
Yes. But if you leave the line longer than 80 characters,
it complains about that too. I hoped that since the point
was not to break grepability, it was better to make the
code look nicer. This is the first chance I've had to
say what I'd been doing when numerous senior
developers were looking, if my method is worse
than leaving the long lines, I'll quit doing it...
-Mike
On Tue, Dec 6, 2016 at 1:14 PM, Oleg Drokin <oleg.drokin@intel.com> wrote:
>
> On Dec 6, 2016, at 1:07 PM, Mike Marshall wrote:
>
>> > long error messages that we cannot split into a multiline thing anyway.
>>
>> I split numerous Orangefs ones at substitution characters, which
>> didn't break grepability. No one complained, and sure made
>> stuff look nicer to me. You might be able to do that some places?
>
> Checkpatch still complains about this I believe.
>
>>
>> -Mike
>>
>> On Tue, Dec 6, 2016 at 12:50 PM, Oleg Drokin <oleg.drokin@intel.com> wrote:
>>
>> On Dec 6, 2016, at 11:15 AM, Greg KH wrote:
>>
>> > On Tue, Dec 06, 2016 at 10:34:03AM -0500, Oleg Drokin wrote:
>> >>
>> >> On Dec 3, 2016, at 3:55 AM, gregkh@linuxfoundation.org wrote:
>> >>
>> >>> On Fri, Dec 02, 2016 at 09:53:08PM +0000, Dilger, Andreas wrote:
>> >>>> Al,
>> >>>> Greg recently raised the issue of what still needs to be done to
>> >>>> move the Lustre code out of staging/ and into the fs/ tree.
>> >>>>
>> >>>> James has been doing a great job of cleaning up various checkpatch
>> >>>> issues and keeping the code updated with the latest fixes, but we
>> >>>> were wondering what you were aware of that needed to be cleaned
>> >>>> up in Lustre?
>> >>>
>> >>> Is the whole "mixing kernel structures in userspace structures" all
>> >>> resolved now? For some reason I thought that you had kernel locks being
>> >>> passed to userspace and then back into the kernel, but it's been a long
>> >>> time since I last looked...
>> >>
>> >> While we certainly had our share of mixing user/kernelspace structures,
>> >> I don't think we ever passed anything with locks around back and forth.
>> >>
>> >> I just did a brief check and I don't see anything glaring on this particular front.
>> >>
>> >>> If you feel you are ready for a "real" review, I'll be glad to go over
>> >>> the code before the vfs people look at it, just let me know. No need to
>> >>> bother them if you still have basic things wrong that I can find…
>> >>
>> >> I think this would be beneficial at this stage.
>> >
>> > I see loads of checkpatch.pl warnings and a few errors, how about fixing
>> > all of them up first?
>> >
>>
>> There are 16 errors,
>> of them:
>> 8 are false positives in drivers/staging/lustre/lnet/libcfs/hash.c
>> 1 false positive in drivers/staging/lustre/lnet/libcfs/tracefile.h
>> 1 false positive in drivers/staging/lustre/lustre/include/lustre/lustre_idl.h
>> 1 I think false positive in drivers/staging/lustre/lustre/include/lustre/lustre_idl.h (PLDLMRES)
>> 2 false positives in drivers/staging/lustre/lustre/include/lustre/lustre_user.h
>>
>> an error I don't really understand, what does it want us to do here?
>> ERROR: trailing statements should be on next line
>> #196951: FILE: drivers/staging/lustre/lustre/mdc/mdc_request.c:1063:
>> + for (end_dirent = ent; ent;
>> + end_dirent = ent, ent = lu_dirent_next(ent));
>>
>> leaving 2 real ones, one missing space in drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c
>> and 1 spaces instead of tabs in drivers/staging/lustre/lnet/klnds/socklnd/socklnd.h
>>
>> I'll have patches shortly for these.
>>
>> On the warning front, we have 879 line over 80 characters, but a bunch
>> of those are due to long error messages that we cannot split into a multiline thing anyway.
>> 71 Prefer 'unsigned int' to bare use of 'unsigned' that is a relatively new one
>> 22 new typedefs (all in lnet, I drove out all the lustre ones, so I guess I can tackle these too).
>> a bunch of comment style problems
>> a few "function definition argument … should also have an identifier name" that
>> seems to be a new one too, I don't remember seeing this before.
>>
>> And a bunch of rarer ones for the total of 1243 (823 in just Lustre).
>> Multiple classes of them are actually not easily fixable or false positives too.
>> Do you really want all of these fixed too somehow?
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>
^ permalink raw reply [flat|nested] 24+ messages in thread