Remaining work needed for moving Lustre out of staging

Remaining work needed for moving Lustre out of staging

[Dilger, Andreas]

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?

Cheers, Andreas

[lustre-devel] Remaining work needed for moving Lustre out of staging

[Dilger, Andreas]

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?

Cheers, Andreas

Re: Remaining work needed for moving Lustre out of staging

[gregkh]

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...

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...

thanks,

greg k-h

[lustre-devel] Remaining work needed for moving Lustre out of staging

[gregkh at linuxfoundation.org]

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...

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...

thanks,

greg k-h

Re: Remaining work needed for moving Lustre out of staging

[Oleg Drokin]

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.
Thanks.

Bye,
    Oleg

[lustre-devel] Remaining work needed for moving Lustre out of staging

[Oleg Drokin]

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.
Thanks.

Bye,
    Oleg

Re: Remaining work needed for moving Lustre out of staging

[Greg KH]

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?

thanks,

greg k-h

[lustre-devel] Remaining work needed for moving Lustre out of staging

[Greg KH]

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?

thanks,

greg k-h

Re: [lustre-devel] Remaining work needed for moving Lustre out of staging

[Oleg Drokin]

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?

[lustre-devel] Remaining work needed for moving Lustre out of staging

[Oleg Drokin]

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?

Re: [lustre-devel] Remaining work needed for moving Lustre out of staging

[Greg KH]

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

[lustre-devel] Remaining work needed for moving Lustre out of staging

[Greg KH]

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

Re: [lustre-devel] Remaining work needed for moving Lustre out of staging

[Andreas Dilger]

[-- 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 --]

[lustre-devel] Remaining work needed for moving Lustre out of staging

[Andreas Dilger]

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>

Re: Remaining work needed for moving Lustre out of staging

[Mike Marshall]

[-- 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

[lustre-devel] Remaining work needed for moving Lustre out of staging

[Mike Marshall]

> 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>

Re: [lustre-devel] Remaining work needed for moving Lustre out of staging

[Oleg Drokin]

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
> 

[lustre-devel] Remaining work needed for moving Lustre out of staging

[Oleg Drokin]

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
> 

Re: [lustre-devel] Remaining work needed for moving Lustre out of staging

[Mike Marshall]

> 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
>>
>

[lustre-devel] Remaining work needed for moving Lustre out of staging

[Mike Marshall]

> 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
>>
>

Re: Remaining work needed for moving Lustre out of staging

[James Simmons]

> 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 think he means the use of linux list structures as a parameter for the
ioctls in LNet selftest. The work is being done under:

https://jira.hpdd.intel.com/browse/LU-8915

No patch ready just yet.

[lustre-devel] Remaining work needed for moving Lustre out of staging

[James Simmons]

> 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 think he means the use of linux list structures as a parameter for the
ioctls in LNet selftest. The work is being done under:

https://jira.hpdd.intel.com/browse/LU-8915

No patch ready just yet.

Re: [lustre-devel] Remaining work needed for moving Lustre out of staging

[James Simmons]

> > 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.

[lustre-devel] Remaining work needed for moving Lustre out of staging

[James Simmons]

> > 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.