Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

zfs-0.7.2/module/zfs/zfs_vnops.c:4565]: (style) Suspicious condition #6685

Closed
dcb314 opened this issue Sep 26, 2017 · 3 comments · Fixed by #6719
Closed

zfs-0.7.2/module/zfs/zfs_vnops.c:4565]: (style) Suspicious condition #6685

dcb314 opened this issue Sep 26, 2017 · 3 comments · Fixed by #6719
Labels
good first issue Indicates a good issue for first-time contributors

Comments

@dcb314
Copy link

dcb314 commented Sep 26, 2017

zfs-0.7.2/module/zfs/zfs_vnops.c:4565]: (style) Suspicious condition (assignment + comparison); Clarify expression with parentheses.

Source code is

   if ((error = zfs_getattr(ip, &vap, 0, CRED()) != 0))

maybe better code

   if ((error = zfs_getattr(ip, &vap, 0, CRED())) != 0)
@behlendorf behlendorf added the good first issue Indicates a good issue for first-time contributors label Sep 26, 2017
@tcharding
Copy link
Contributor

Correct me if I am wrong but the original code clobbers the return value from zfs_getattr(). Is this intended? I'm brand new round here so I can't say but it seems unlikely. At least we should have a comment if that is intended.

How about (inline with other code in this file)

if ((error = zfs_getattr(ip, &vap, 0, CRED()))
        return (error);

I'm guessing you guys don't like (I better re-read the coding conventions ;)

error =  zfs_getattr(ip, &vap, 0, CRED());
if (error)
	return (error);

@behlendorf
Copy link
Contributor

This is definitely a bug. The intent here is to return error when it's non-zero. @tcharding let's go with the inline fix. Both styles are allowed by the coding conventions and are used on a case by case basis depending on which is clear for for the specific code block. It would be great if you could open a new PR with the fix.

@tcharding
Copy link
Contributor

Cool. I'll do the PR shortly. thanks @behlendorf

behlendorf pushed a commit that referenced this issue Oct 6, 2017
Currently `if` statement includes an assignment (from a function return
value) and a equality check. The parenthesis are in the incorrect place,
currently the code clobbers the function return value because of this.

We can fix this by simplifying the `if` statement.

`if (foo != 0)`

can be more succinctly expressed as

`if (foo)`

Remove the equality check, add parenthesis to correct the statement.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: George Melikov <mail@gmelikov.ru>
Reviewed-by: Chris Dunlop <chris@onthe.net.au>
Signed-off-by: Tobin C. Harding <me@tobin.cc>
Closes #6685 
Close #6719
aerusso pushed a commit to aerusso/zfs that referenced this issue Oct 11, 2017
Currently `if` statement includes an assignment (from a function return
value) and a equality check. The parenthesis are in the incorrect place,
currently the code clobbers the function return value because of this.

We can fix this by simplifying the `if` statement.

`if (foo != 0)`

can be more succinctly expressed as

`if (foo)`

Remove the equality check, add parenthesis to correct the statement.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: George Melikov <mail@gmelikov.ru>
Reviewed-by: Chris Dunlop <chris@onthe.net.au>
Signed-off-by: Tobin C. Harding <me@tobin.cc>
Closes openzfs#6685 
Close openzfs#6719
tonyhutter pushed a commit that referenced this issue Oct 16, 2017
Currently `if` statement includes an assignment (from a function return
value) and a equality check. The parenthesis are in the incorrect place,
currently the code clobbers the function return value because of this.

We can fix this by simplifying the `if` statement.

`if (foo != 0)`

can be more succinctly expressed as

`if (foo)`

Remove the equality check, add parenthesis to correct the statement.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: George Melikov <mail@gmelikov.ru>
Reviewed-by: Chris Dunlop <chris@onthe.net.au>
Signed-off-by: Tobin C. Harding <me@tobin.cc>
Closes #6685
Close #6719
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Indicates a good issue for first-time contributors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants