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

libbe: Avoid double printing cloning errors. #685

Closed
wants to merge 1 commit into from

Conversation

jgrafton
Copy link
Contributor

@jgrafton jgrafton commented Mar 6, 2023

be_clone calls be_clone_cb and both call set_error on the return error path. set_error prints the error resulting in a double print. be_clone_cb should just return the error code and allow be_clone to print it.

PR: 265248
Reported by: Graham Perrin

@bsdimp
Copy link
Member

bsdimp commented Mar 6, 2023

this looks good to me. @kevans91 ?

@bsdimp
Copy link
Member

bsdimp commented Mar 7, 2023

I was premature in my review...
There's 3 other set_error() calls in be_clone_cb(). I think they should all be fixed up.

@jgrafton
Copy link
Contributor Author

jgrafton commented Mar 8, 2023

I was premature in my review... There's 3 other set_error() calls in be_clone_cb(). I think they should all be fixed up.

Agreed. I originally limited the change scope to the referenced PR but it's an obvious fix.

Copy link
Contributor

@kevans91 kevans91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@jlduran
Copy link
Contributor

jlduran commented Mar 9, 2023

Doesn't this line also need to just return the err?:

return (set_error(ldc->lbh, err));

be_clone calls be_clone_cb and both call set_error on the return
error path.  set_error prints the error resulting in a double print.
be_clone_cb should just return the error code and allow be_clone
to print it.

PR: 265248
Reported by: Graham Perrin
@jgrafton
Copy link
Contributor Author

Doesn't this line also need to just return the err?:

Yeah, you're correct. I can't think of any reason it should set the error in be_clone_cb at all.

@bsdimp
Copy link
Member

bsdimp commented Mar 15, 2023

OK. looks good.

@bsdimp bsdimp closed this Mar 15, 2023
freebsd-git pushed a commit that referenced this pull request Mar 15, 2023
be_clone calls be_clone_cb and both call set_error on the return
error path.  set_error prints the error resulting in a double print.
be_clone_cb should just return the error code and allow be_clone
to print it.

PR: 265248
Reported by: Graham Perrin
Reviewed by: imp, kevans
Pull Request: #685
bsdjhb pushed a commit to bsdjhb/cheribsd that referenced this pull request Apr 19, 2023
be_clone calls be_clone_cb and both call set_error on the return
error path.  set_error prints the error resulting in a double print.
be_clone_cb should just return the error code and allow be_clone
to print it.

PR: 265248
Reported by: Graham Perrin
Reviewed by: imp, kevans
Pull Request: freebsd/freebsd-src#685
@emaste emaste added the merged label Jun 12, 2023
freebsd-git pushed a commit that referenced this pull request Feb 19, 2024
be_clone calls be_clone_cb and both call set_error on the return
error path.  set_error prints the error resulting in a double print.
be_clone_cb should just return the error code and allow be_clone
to print it.

PR: 265248
Reported by: Graham Perrin
Reviewed by: imp, kevans
Pull Request: #685

(cherry picked from commit 8e933d9)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants