Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1281768 > unrolled thread

Re: [PATCH 21/40] staging: lustre: improve LNet clean up code and API

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 21/40] staging: lustre: improve LNet clean up code and API Dan Carpenter <dan.carpenter@oracle.com> - 2015-12-02 14:10 +0100
    Re: [lustre-devel] [PATCH 21/40] staging: lustre: improve LNet clean  up code and API Alexander Zarochentsev <alexander.zarochentsev@seagate.com> - 2015-12-02 15:00 +0100
      Re: [lustre-devel] [PATCH 21/40] staging: lustre: improve LNet clean  up code and API Dan Carpenter <dan.carpenter@oracle.com> - 2015-12-02 15:10 +0100

#1281768 — Re: [PATCH 21/40] staging: lustre: improve LNet clean up code and API

Actually we're going to have to redo so much code that it's not worth it
for me to review the rest of these patches.  Please just look over
everything again:

 BAD:	return -1;
GOOD:	return -EINVAL;

 BAD:  failed0:
GOOD:  free_something:

 BAD:	if (rc != 0)
GOOD:	if (rc)

Do one thing per patch.
Do not introduce a bug and then fix it in a later patch.
Check ioc_len more carefully.
Don't make the code look ugly just to please checkpatch.pl.
Do error handling not success handling.
Try to avoid indenting a far to the right.


regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1281815 — Re: [lustre-devel] [PATCH 21/40] staging: lustre: improve LNet clean up code and API

Hello,

On Wed, Dec 2, 2015 at 3:59 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> Actually we're going to have to redo so much code that it's not worth it
> for me to review the rest of these patches.  Please just look over
> everything again:
>
>  BAD:   return -1;
> GOOD:   return -EINVAL;
>
>  BAD:  failed0:
> GOOD:  free_something:
>
>  BAD:   if (rc != 0)
> GOOD:   if (rc)

The latest suggestion is not correct,
from http://wiki.lustre.org/Lustre_Coding_Guidelines :
Conditional boolean (if (expr)), scalar (if (val != 0)) and pointer
(if (ptr != NULL)) expressions should be written consistently.

Thanks,

-- 
Alexander Zarochentsev
Seagate Technology, LLC
www.seagate.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1281825 — Re: [lustre-devel] [PATCH 21/40] staging: lustre: improve LNet clean up code and API

On Wed, Dec 02, 2015 at 04:20:59PM +0300, Alexander Zarochentsev wrote:
> >  BAD:   if (rc != 0)
> > GOOD:   if (rc)
> 
> The latest suggestion is not correct,
> from http://wiki.lustre.org/Lustre_Coding_Guidelines :
> Conditional boolean (if (expr)), scalar (if (val != 0)) and pointer
> (if (ptr != NULL)) expressions should be written consistently.

Kernel style trumps Lustre style.  Double negative don't not hurt
readability.  != NULL is a checkpatch.pl warning.  Also comparisons like
== false or == true are checkpatch warnings.

I can think of two places where comparing with zero is appropriate and
those are:

1)  If you are talking about the numer zero.

	if (x == 0 || x == 2)

2) strcmp() and other *cmp() functions.

	if (strcmp(foo, bar) == 0)   /* foo and bar are the same */
	if (strcmp(foo, bar) < 0)    /* foo less than bar */
	if (strcmp(foo, bar) != 0)   /* foo not the same as bar */

For "if (rc) {" a zero return doesn't mean zero, it means success so
comparing against zero is bad style.

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web