staging/lustre/ptlrpc: Removes potential null dereference
PATCH v2 1/1 staging/lustre/ptlrpc: Removes potential null dereference
The lustre_msg_buf method could return NULL. Subsequent code didn't check if it's null before using it. This patch adds two checks.
--- drivers/staging/lustre/lustre/ptlrpc/sec.c | 2 ++ drivers/staging/lustre/lustre/ptlrpc/sec_plain.c | 9 +++++++-- 2 files changed, 9 insertions(+), 2 deletions(-)
drivers/staging/lustre/lustre/ptlrpc/sec.c +2 -0
@@ -2195,6 +2195,8 @@ int sptlrpc_pack_user_desc(struct lustre_msg *msg, int offset)struct ptlrpc_user_desc *pud; pud =lustre_msg_buf (msg, offset,0 ); +if (!pud) +return -EINVAL ; pud->pud_uid =from_kuid (&init_user_ns,current_uid ()); pud->pud_gid =from_kgid (&init_user_ns,current_gid ());
drivers/staging/lustre/lustre/ptlrpc/sec_plain.c +7 -2
@@ -542,6 +542,7 @@ int plain_alloc_reqbuf(struct ptlrpc_sec *sec, {__u32 buflens[PLAIN_PACK_SEGMENTS ] = {0 , };int alloc_len; +int desc; buflens[PLAIN_PACK_HDR_OFF ] =sizeof (struct plain_header ); buflens[PLAIN_PACK_MSG_OFF ] = msgsize;
@@ -574,8 +575,12 @@ int plain_alloc_reqbuf(struct ptlrpc_sec *sec,lustre_init_msg_v2 (req->rq_reqbuf,PLAIN_PACK_SEGMENTS , buflens,NULL ); req->rq_reqmsg =lustre_msg_buf (req->rq_reqbuf,PLAIN_PACK_MSG_OFF ,0 ); -if (req->rq_pack_udesc) -sptlrpc_pack_user_desc (req->rq_reqbuf,PLAIN_PACK_USER_OFF ); +if (req->rq_pack_udesc) { + desc =sptlrpc_pack_user_desc (req->rq_reqbuf, +PLAIN_PACK_USER_OFF ); +if (!desc) +return desc; + }return 0 ; }
Signature · 1 line
1.9.1
PATCH v2 1/1 staging/lustre/ptlrpc: Removes potential null dereference
This looks wrong - You return -EINVAL from sptlrpc_pack_user_desc, but then the caller checks "!desc". Desc will not be null, since you've returned -EINVAL.
- Patrick
Quoted 49 lines · Lidza Louina
The
lustre_msg_bufmethod could return NULL. Subsequent code didn't check if it's null before using it. This patch adds two checks.Signed-off-by Lidza Louina <[email protected]>--- drivers/staging/lustre/lustre/ptlrpc/sec.c | 2 ++ drivers/staging/lustre/lustre/ptlrpc/sec_plain.c | 9 +++++++-- 2 files changed, 9 insertions(+), 2 deletions(-)drivers/staging/lustre/lustre/ptlrpc/sec.c +2
-0@@ -2195,6 +2195,8 @@ int sptlrpc_pack_user_desc(struct lustre_msg *msg, int offset)struct ptlrpc_user_desc *pud; pud =lustre_msg_buf (msg, offset,0 ); +if (!pud) +return -EINVAL ; pud->pud_uid =from_kuid (&init_user_ns,current_uid ()); pud->pud_gid =from_kgid (&init_user_ns,current_gid ());drivers/staging/lustre/lustre/ptlrpc/sec_plain.c +7
-2@@ -542,6 +542,7 @@ int plain_alloc_reqbuf(struct ptlrpc_sec *sec, {__u32 buflens[PLAIN_PACK_SEGMENTS ] = {0 , };int alloc_len; +int desc; buflens[PLAIN_PACK_HDR_OFF ] =sizeof (struct plain_header ); buflens[PLAIN_PACK_MSG_OFF ] = msgsize;@@ -574,8 +575,12 @@ int plain_alloc_reqbuf(struct ptlrpc_sec *sec,lustre_init_msg_v2 (req->rq_reqbuf,PLAIN_PACK_SEGMENTS , buflens,NULL ); req->rq_reqmsg =lustre_msg_buf (req->rq_reqbuf,PLAIN_PACK_MSG_OFF ,0 ); -if (req->rq_pack_udesc) -sptlrpc_pack_user_desc (req->rq_reqbuf,PLAIN_PACK_USER_OFF ); +if (req->rq_pack_udesc) { + desc =sptlrpc_pack_user_desc (req->rq_reqbuf, +PLAIN_PACK_USER_OFF ); +if (!desc) +return desc; + }return 0 ; }
PATCH v2 1/1 staging/lustre/ptlrpc: Removes potential null dereference
Quoted 4 lines
This looks wrong - You return
-EINVALfromsptlrpc_pack_user_desc, but then the caller checks "!desc". Desc will not be null, since you've returned-EINVAL.
Actually 'if (!desc)' is equal to 'if (desc != 0). Yes it can be confusing. I recommend 'if (desc < 0)' instead to make it clearer what is being tested for.
Quoted 60 lines
- Patrick
Quoted 41 lines · Lidza Louina
The
lustre_msg_bufmethod could return NULL. Subsequent code didn't check if it's null before using it. This patch adds two checks.Signed-off-by Lidza Louina <[email protected]>--- drivers/staging/lustre/lustre/ptlrpc/sec.c | 2 ++ drivers/staging/lustre/lustre/ptlrpc/sec_plain.c | 9 +++++++-- 2 files changed, 9 insertions(+), 2 deletions(-)drivers/staging/lustre/lustre/ptlrpc/sec.c +2
-0@@ -2195,6 +2195,8 @@ int sptlrpc_pack_user_desc(struct lustre_msg *msg, int offset)struct ptlrpc_user_desc *pud; pud =lustre_msg_buf (msg, offset,0 ); +if (!pud) +return -EINVAL ; pud->pud_uid =from_kuid (&init_user_ns,current_uid ());pud->pud_gid = from_kgid(&init_user_ns, current_gid());drivers/staging/lustre/lustre/ptlrpc/sec_plain.c +1
-0@@ -542,6 +542,7 @@ int plain_alloc_reqbuf(struct ptlrpc_sec *sec, {__u32 buflens[PLAIN_PACK_SEGMENTS ] = {0 , };int alloc_len; +int desc; buflens[PLAIN_PACK_HDR_OFF ] =sizeof (struct plain_header ); buflens[PLAIN_PACK_MSG_OFF ] = msgsize;@@ -574,8 +575,12 @@ int plain_alloc_reqbuf(struct ptlrpc_sec *sec,lustre_init_msg_v2 (req->rq_reqbuf,PLAIN_PACK_SEGMENTS , buflens,NULL ); req->rq_reqmsg =lustre_msg_buf (req->rq_reqbuf,PLAIN_PACK_MSG_OFF ,0 ); - if (req->rq_pack_udesc)drivers/staging/lustre/lustre/ptlrpc/sec_plain.c int plain_alloc_reqbuf(struct ptlrpc_sec *sec, · line 586
Quoted 10 lines
drivers/staging/lustre/lustre/ptlrpc/sec_plain.c +6
-1-sptlrpc_pack_user_desc (req->rq_reqbuf,PLAIN_PACK_USER_OFF ); +if (req->rq_pack_udesc) { + desc =sptlrpc_pack_user_desc (req->rq_reqbuf, +PLAIN_PACK_USER_OFF ); +if (!desc) +return desc; + }return 0 ; }_______________________________________________
lustre-devel mailing list
lustre-devel at lists.lustre.org
http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org
PATCH v2 1/1 staging/lustre/ptlrpc: Removes potential null dereference
James,
No. You've got it backwards. 0 is false, any non-zero value is true.
if(desc) would be equal to if (desc != 0).
- Patrick
Quoted 66 lines · James Simmons
Quoted 3 lines
This looks wrong - You return
-EINVALfromsptlrpc_pack_user_desc, but then the caller checks "!desc". Desc will not be null, since you've returned-EINVAL.Actually 'if (!desc)' is equal to 'if (desc != 0). Yes it can be confusing. I recommend 'if (desc < 0)' instead to make it clearer what is being tested for.
Quoted 59 lines
- Patrick
Quoted 41 lines · Lidza Louina
The
lustre_msg_bufmethod could return NULL. Subsequent code didn't check if it's null before using it. This patch adds two checks.Signed-off-by Lidza Louina <[email protected]>--- drivers/staging/lustre/lustre/ptlrpc/sec.c | 2 ++ drivers/staging/lustre/lustre/ptlrpc/sec_plain.c | 9 +++++++-- 2 files changed, 9 insertions(+), 2 deletions(-)drivers/staging/lustre/lustre/ptlrpc/sec.c +2
-0@@ -2195,6 +2195,8 @@ int sptlrpc_pack_user_desc(struct lustre_msg *msg, int offset)struct ptlrpc_user_desc *pud; pud =lustre_msg_buf (msg, offset,0 ); +if (!pud) +return -EINVAL ; pud->pud_uid =from_kuid (&init_user_ns,current_uid ());pud->pud_gid = from_kgid(&init_user_ns, current_gid());drivers/staging/lustre/lustre/ptlrpc/sec_plain.c +1
-0@@ -542,6 +542,7 @@ int plain_alloc_reqbuf(struct ptlrpc_sec *sec, {__u32 buflens[PLAIN_PACK_SEGMENTS ] = {0 , };int alloc_len; +int desc; buflens[PLAIN_PACK_HDR_OFF ] =sizeof (struct plain_header ); buflens[PLAIN_PACK_MSG_OFF ] = msgsize;@@ -574,8 +575,12 @@ int plain_alloc_reqbuf(struct ptlrpc_sec *sec,lustre_init_msg_v2 (req->rq_reqbuf,PLAIN_PACK_SEGMENTS , buflens,NULL ); req->rq_reqmsg =lustre_msg_buf (req->rq_reqbuf,PLAIN_PACK_MSG_OFF ,0 ); - if (req->rq_pack_udesc)drivers/staging/lustre/lustre/ptlrpc/sec_plain.c int plain_alloc_reqbuf(struct ptlrpc_sec *sec, · line 586
Quoted 10 lines
drivers/staging/lustre/lustre/ptlrpc/sec_plain.c +6
-1-sptlrpc_pack_user_desc (req->rq_reqbuf,PLAIN_PACK_USER_OFF ); +if (req->rq_pack_udesc) { + desc =sptlrpc_pack_user_desc (req->rq_reqbuf, +PLAIN_PACK_USER_OFF ); +if (!desc) +return desc; + }return 0 ; }_______________________________________________
lustre-devel mailing list
lustre-devel at lists.lustre.org
http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org
PATCH v2 1/1 staging/lustre/ptlrpc: Removes potential null dereference
Quoted 6 lines
James,
No. You've got it backwards. 0 is false, any non-zero value is true.
if(desc) would be equal to if (desc != 0).
Ah, you are right. Didn't get much sleep last night :-(
Quoted 76 lines
- Patrick
Quoted 70 lines · James Simmons
Quoted 4 lines
This looks wrong - You return
-EINVALfromsptlrpc_pack_user_desc, but then
the caller checks "!desc". Desc will not be null, since you've returned-EINVAL.Actually 'if (!desc)' is equal to 'if (desc != 0). Yes it can be confusing. I recommend 'if (desc < 0)' instead to make it clearer what is being tested for.
Quoted 62 lines
- Patrick
Quoted 44 lines · Lidza Louina
The
lustre_msg_bufmethod could return NULL. Subsequent code didn't check if it's null before using it. This patch adds two checks.Signed-off-by Lidza Louina <[email protected]>--- drivers/staging/lustre/lustre/ptlrpc/sec.c | 2 ++ drivers/staging/lustre/lustre/ptlrpc/sec_plain.c | 9 +++++++-- 2 files changed, 9 insertions(+), 2 deletions(-)drivers/staging/lustre/lustre/ptlrpc/sec.c +0
-0@@ -2195,6 +2195,8 @@ int sptlrpc_pack_user_desc(struct lustre_msg *msg,int
offset)struct ptlrpc_user_desc *pud;pud = lustre_msg_buf(msg, offset, 0);drivers/staging/lustre/lustre/ptlrpc/sec.c +2
-0+if (!pud) +return -EINVAL ; pud->pud_uid =from_kuid (&init_user_ns,current_uid ()); pud->pud_gid =from_kgid (&init_user_ns,current_gid ());drivers/staging/lustre/lustre/ptlrpc/sec_plain.c +1
-0@@ -542,6 +542,7 @@ int plain_alloc_reqbuf(struct ptlrpc_sec *sec, {__u32 buflens[PLAIN_PACK_SEGMENTS ] = {0 , };int alloc_len; +int desc; buflens[PLAIN_PACK_HDR_OFF ] =sizeof (struct plain_header ); buflens[PLAIN_PACK_MSG_OFF ] = msgsize;@@ -574,8 +575,12 @@ int plain_alloc_reqbuf(struct ptlrpc_sec *sec,lustre_init_msg_v2 (req->rq_reqbuf,PLAIN_PACK_SEGMENTS , buflens,NULL ); req->rq_reqmsg =lustre_msg_buf (req->rq_reqbuf,PLAIN_PACK_MSG_OFF ,0 ); - if (req->rq_pack_udesc)drivers/staging/lustre/lustre/ptlrpc/sec_plain.c int plain_alloc_reqbuf(struct ptlrpc_sec *sec, · line 586
Quoted 10 lines
drivers/staging/lustre/lustre/ptlrpc/sec_plain.c +6
-1-sptlrpc_pack_user_desc (req->rq_reqbuf,PLAIN_PACK_USER_OFF ); +if (req->rq_pack_udesc) { + desc =sptlrpc_pack_user_desc (req->rq_reqbuf, +PLAIN_PACK_USER_OFF ); +if (!desc) +return desc; + }return 0 ; }_______________________________________________
lustre-devel mailing list
lustre-devel at lists.lustre.org
http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org
Dilger
PATCH v2 1/1 staging/lustre/ptlrpc: Removes potential null dereference
In reply to James SimmonsQuoted 6 lines · James Simmons
Quoted 4 lines
This looks wrong - You return
-EINVALfromsptlrpc_pack_user_desc, but then the caller checks "!desc". Desc will not be null, since you've returned-EINVAL.Actually 'if (!desc)' is equal to 'if (desc != 0). Yes it can be confusing.
Very confusing indeed. :-) "if (!desc)" actually means "if (desc == 0)"...
That is the main reason why I don't like those shortcuts since they require an extra mental step by the reader to determine the logic. Having the explicit comparisons like "if (desc == 0)" or "if (desc != 0)" makes the code more clear, and doesn't make the compiled binary any slower.
Quoted 2 lines
I recommend 'if (desc < 0)' instead to make it clearer what is being tested for.
That would actually fix the problem. Patrick is correct that the current patch is broken, since it is either returning zero "if (!desc)" is true, or zero at the end of the function. The error is never returned.
Lidza,
to improve this patch further, the function should really use "rc" to hold the error return instead of "desc", since "rc" is typically used for error returns, and "desc" is normally a pointer to a bulk descriptor in this code.
Also, as Oleg previously mentioned, please declare "int rc;" inside the "if (req->rq_pack_udesc)" block instead of at the top of the function, since it isn't used anywhere else.
Cheers, Andreas
Quoted 52 lines
Quoted 52 lines
- Patrick
Quoted 49 lines · Lidza Louina
The
lustre_msg_bufmethod could return NULL. Subsequent code didn't check if it's null before using it. This patch adds two checks.Signed-off-by Lidza Louina <[email protected]>--- drivers/staging/lustre/lustre/ptlrpc/sec.c | 2 ++ drivers/staging/lustre/lustre/ptlrpc/sec_plain.c | 9 +++++++-- 2 files changed, 9 insertions(+), 2 deletions(-)drivers/staging/lustre/lustre/ptlrpc/sec.c +2
-0@@ -2195,6 +2195,8 @@ int sptlrpc_pack_user_desc(struct lustre_msg *msg, int offset)struct ptlrpc_user_desc *pud; pud =lustre_msg_buf (msg, offset,0 ); +if (!pud) +return -EINVAL ; pud->pud_uid =from_kuid (&init_user_ns,current_uid ());pud->pud_gid = from_kgid(&init_user_ns, current_gid());drivers/staging/lustre/lustre/ptlrpc/sec_plain.c +7
-1@@ -542,6 +542,7 @@ int plain_alloc_reqbuf(struct ptlrpc_sec *sec, {__u32 buflens[PLAIN_PACK_SEGMENTS ] = {0 , };int alloc_len; +int desc; buflens[PLAIN_PACK_HDR_OFF ] =sizeof (struct plain_header ); buflens[PLAIN_PACK_MSG_OFF ] = msgsize;@@ -574,8 +575,12 @@ int plain_alloc_reqbuf(struct ptlrpc_sec *sec,lustre_init_msg_v2 (req->rq_reqbuf,PLAIN_PACK_SEGMENTS , buflens,NULL ); req->rq_reqmsg =lustre_msg_buf (req->rq_reqbuf,PLAIN_PACK_MSG_OFF ,0 ); - if (req->rq_pack_udesc) -sptlrpc_pack_user_desc (req->rq_reqbuf,PLAIN_PACK_USER_OFF ); +if (req->rq_pack_udesc) { + desc =sptlrpc_pack_user_desc (req->rq_reqbuf, +PLAIN_PACK_USER_OFF ); +if (!desc) +return desc; + }
Quoted 9 lines
Quoted 8 lines
drivers/staging/lustre/lustre/ptlrpc/sec_plain.c int plain_alloc_reqbuf(struct ptlrpc_sec *sec, · line 586
Quoted 2 lines
drivers/staging/lustre/lustre/ptlrpc/sec_plain.c +0
-0return 0 ; }_______________________________________________
lustre-devel mailing list
lustre-devel at lists.lustre.org
http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org
Cheers, Andreas
Signature · 4 lines
Andreas Dilger Lustre Principal Architect Intel High Performance Data Division
PATCH v2 1/1 staging/lustre/ptlrpc: Removes potential null dereference
Quoted 24 lines · Andreas
Quoted 4 lines · James Simmons
Quoted 3 lines
This looks wrong - You return
-EINVALfromsptlrpc_pack_user_desc, but then the caller checks "!desc". Desc will not be null, since you've returned-EINVAL.Actually 'if (!desc)' is equal to 'if (desc != 0). Yes it can be confusing.
Very confusing indeed. :-) "if (!desc)" actually means "if (desc == 0)"...
That is the main reason why I don't like those shortcuts since they require an extra mental step by the reader to determine the logic. Having the explicit comparisons like "if (desc == 0)" or "if (desc != 0)" makes the code more clear, and doesn't make the compiled binary any slower.
Quoted 2 lines
I recommend 'if (desc < 0)' instead to make it clearer what is being tested for.
That would actually fix the problem. Patrick is correct that the current patch is broken, since it is either returning zero "if (!desc)" is true, or zero at the end of the function. The error is never returned.
Lidza,
to improve this patch further, the function should really use "rc" to hold the error return instead of "desc", since "rc" is typically used for error returns, and "desc" is normally a pointer to a bulk descriptor in this code.
Quoted 5 lines
Also, as Oleg previously mentioned, please declare "int rc;" inside the "if (
req->rq_pack_udesc)" block instead of at the top of the function, since it isn't used anywhere else.Cheers, Andreas
Definitely, will do.
I'll change desc to rc and update the if statement and send another patch now.
Lidza
Quoted 62 lines
Quoted 53 lines
Quoted 53 lines
- Patrick
Quoted 49 lines · Lidza Louina
The
lustre_msg_bufmethod could return NULL. Subsequent code didn't check if it's null before using it. This patch adds two checks.Signed-off-by Lidza Louina <[email protected]>--- drivers/staging/lustre/lustre/ptlrpc/sec.c | 2 ++ drivers/staging/lustre/lustre/ptlrpc/sec_plain.c | 9 +++++++-- 2 files changed, 9 insertions(+), 2 deletions(-)drivers/staging/lustre/lustre/ptlrpc/sec.c +2
-0@@ -2195,6 +2195,8 @@ int sptlrpc_pack_user_desc(struct lustre_msg *msg, int offset)struct ptlrpc_user_desc *pud; pud =lustre_msg_buf (msg, offset,0 ); +if (!pud) +return -EINVAL ; pud->pud_uid =from_kuid (&init_user_ns,current_uid ());pud->pud_gid = from_kgid(&init_user_ns, current_gid());drivers/staging/lustre/lustre/ptlrpc/sec_plain.c +7
-1@@ -542,6 +542,7 @@ int plain_alloc_reqbuf(struct ptlrpc_sec *sec, {__u32 buflens[PLAIN_PACK_SEGMENTS ] = {0 , };int alloc_len; +int desc; buflens[PLAIN_PACK_HDR_OFF ] =sizeof (struct plain_header ); buflens[PLAIN_PACK_MSG_OFF ] = msgsize;@@ -574,8 +575,12 @@ int plain_alloc_reqbuf(struct ptlrpc_sec *sec,lustre_init_msg_v2 (req->rq_reqbuf,PLAIN_PACK_SEGMENTS , buflens,NULL ); req->rq_reqmsg =lustre_msg_buf (req->rq_reqbuf,PLAIN_PACK_MSG_OFF ,0 ); - if (req->rq_pack_udesc) -sptlrpc_pack_user_desc (req->rq_reqbuf,PLAIN_PACK_USER_OFF ); +if (req->rq_pack_udesc) { + desc =sptlrpc_pack_user_desc (req->rq_reqbuf, +PLAIN_PACK_USER_OFF ); +if (!desc) +return desc; + }Quoted 7 lines
Quoted 7 lines
drivers/staging/lustre/lustre/ptlrpc/sec_plain.c int plain_alloc_reqbuf(struct ptlrpc_sec *sec, · line 586
Quoted 2 lines
drivers/staging/lustre/lustre/ptlrpc/sec_plain.c +0
-0return 0 ; }_______________________________________________
lustre-devel mailing list
lustre-devel at lists.lustre.org
http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.orgCheers, Andreas
PATCH v2 1/1 staging/lustre/ptlrpc: Removes potential null dereference
In reply to DilgerWhen I read the code, I just assumed desc was a pointer and it should have been:
if (!desc) return NULL;
For me, "if (rc) " is way more readable than "if (rc != 0) ". So readability could go either way depending on what you're used to, I suppose.
It should definitely == 0 and != 0 if you are talking about the actual number zero instead of success/fail like we are here. Also it helps to use == 0 with strcmp() and friends (although half of the kernel does not know that trick yet).
The other thing which I have noticed recently is that a lot of subsystems use a mix of "if (rc) " and "if (rc < 0) ". It's annoying for Smatch because say a function only returns zero but the some of the callers check for < 0 and some check for != 0. We can't know for sure that they are equivalent.
regards,
dan carpenter
PATCH v2 1/1 staging/lustre/ptlrpc: Removes potential null dereference
Quoted 15 lines · Dan Carpenter
When I read the code, I just assumed desc was a pointer and it should have been:
if (!desc) return NULL;For me, "if (rc) " is way more readable than "if (rc != 0) ". So readability could go either way depending on what you're used to, I suppose.
It should definitely == 0 and != 0 if you are talking about the actual number zero instead of success/fail like we are here. Also it helps to use == 0 with
strcmp()and friends (although half of the kernel does not know that trick yet).
Quoted 8 lines
The other thing which I have noticed recently is that a lot of subsystems use a mix of "if (rc) " and "if (rc < 0) ". It's annoying for Smatch because say a function only returns zero but the some of the callers check for < 0 and some check for != 0. We can't know for sure that they are equivalent.
regards,
dan carpenter
Hey Dan,
if (rc < 0) and if (rc) pretty much translates to the same thing. It'll only return a negative error value if there are problems and 0 if it succeeds. I feel like the first way is more explicit, since negative numbers are usually used for errors. I've sent a 3rd version of the patch with (rc < 0).
And I'm not sure about the way other subsystems use return values. Here it should only either be less than or equal to 0 so it makes sense to me in this circumstance.
I ran smatch on my patched file ../smatch/smatch_scripts/kchecker drivers/staging/lustre/lustre/ptlrpc/sec_plain.c and it didn't find any issues with it.
Lidza
PATCH v2 1/1 staging/lustre/ptlrpc: Removes potential null dereference
Quoted 1 line · Lidza Louina
if (rc < 0) and if (rc) pretty much translates to the same thing.
I wasn't talking about this patch in particular; it's just something I have thinking about recently. For example, there are a lot of functions that don't initialize parameters if they return a non-zero value, but the caller checks for negatives. It's often tricky or impossible to determine that these mean the same thing just from analysing the code without making assumptions.
I think "if (rc) " is more common than "if (rc < 0)". I also think it's prettier code. But mostly I wish people would do it all consistently.
regards,
dan carpenter