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.

Sign up for GitHub

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

Jump to bottom

rgw/file: missing brackets around if statement #37537

Merged
merged 1 commit into from Oct 13, 2020
Merged

rgw/file: missing brackets around if statement #37537

merged 1 commit into from Oct 13, 2020

Conversation

yuvalif
Copy link
Contributor

@yuvalif yuvalif commented Oct 4, 2020

also some small style issues
issues were detected by pvs-studio static analyzer

Signed-off-by: Yuval Lifshitz ylifshit@redhat.com

Show available Jenkins commands
  • jenkins retest this please
  • jenkins test classic perf
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox

@yuvalif yuvalif requested a review from cbodley October 4, 2020 14:10
@yuvalif yuvalif force-pushed the missing_brackets_in_rgw_file branch from bbb2513 to 002652f Compare October 5, 2020 05:00
mattbenjamin
mattbenjamin requested changes Oct 5, 2020
View reviewed changes
Copy link
Contributor

@mattbenjamin mattbenjamin left a comment

Choose a reason for hiding this comment

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

the brace fix is needed, however looks like this might introduce damage at l. 459; I don't prefer pvs-studio enforced style regarding the "s" alias

src/rgw/rgw_file.cc Outdated
@@ -1520,11 +1523,11 @@ namespace rgw {
}

int RGWWriteRequest::exec_start() {
struct req_state* s = get_state();
struct req_state* _s = get_state();
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like naming variables _something, esp local

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok. will rename to "state".
a better fix was to change "s" in the parent where its is "protected" to "private" - but this would probably cause too many code changes in other places...

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, that's a good solution

src/rgw/rgw_file.h Outdated
@@ -456,11 +456,6 @@ namespace rgw {
st->st_nlink = state.nlink;
break;
case RGW_FS_TYPE_FILE:
st->st_nlink = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see how we want to do this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in the original version st->st_link = 1; was under both RGW_FS_TYPE_FILE and RGW_FS_TYPE_SYMBOLIC_LINK
so, either we can collapse them to one case, or we need to fix this line in one of the cases

Copy link
Contributor

Choose a reason for hiding this comment

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

In this case, we're kind of inventing a value for reporting purposes, but I'd like them to be distinct for the different object types for clarity. I'm actually not clear how having this assignment in disjoint cases is a problem, maybe I'm missing something obvious.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

originally, there were 2 different cases with exactly the same code.
so, either, we can collapse them to one case, or there was a copy&paste bug in the original code.
but if you think it is ok to duplicate the code in the 2 cases, i can revert the change

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, there is no problem having 2 cases that take the same action, in this case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok. will revert

src/rgw/rgw_file.h Show resolved Hide resolved
@yuvalif yuvalif force-pushed the missing_brackets_in_rgw_file branch from 002652f to 48104cd Compare October 6, 2020 16:26
@yuvalif
Copy link
Contributor Author

yuvalif commented Oct 6, 2020

jenkins test signed

@yuvalif
Copy link
Contributor Author

yuvalif commented Oct 6, 2020

jenkins test make check

yuvalif
yuvalif commented Oct 7, 2020
View reviewed changes
src/rgw/rgw_file.h
<< " prefix=" << prefix << " "
<< " pref path=" << name << " (not chomped)"
<< " target = " << path << ""
<< dendl;
matched = true;
/* match-dir case (trailing '/') */
if (name == prefix + "/")
if (name == prefix + "/") {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mattbenjamin seems like this fix have broken the folllowing s3 test:

020-10-05T12:18:54.519 INFO:teuthology.orchestra.run.smithi119.stderr:======================================================================
2020-10-05T12:18:54.519 INFO:teuthology.orchestra.run.smithi119.stderr:FAIL: s3tests_boto3.functional.test_s3.test_bucket_listv2_delimiter_basic
2020-10-05T12:18:54.519 INFO:teuthology.orchestra.run.smithi119.stderr:----------------------------------------------------------------------
2020-10-05T12:18:54.520 INFO:teuthology.orchestra.run.smithi119.stderr:Traceback (most recent call last):
2020-10-05T12:18:54.520 INFO:teuthology.orchestra.run.smithi119.stderr:  File "/home/ubuntu/cephtest/s3-tests/virtualenv/lib/python3.6/site-packages/nose/case.py", line 198, in runTest
2020-10-05T12:18:54.520 INFO:teuthology.orchestra.run.smithi119.stderr:    self.test(*self.arg)
2020-10-05T12:18:54.520 INFO:teuthology.orchestra.run.smithi119.stderr:  File "/home/ubuntu/cephtest/s3-tests/s3tests_boto3/functional/test_s3.py", line 232, in test_bucket_listv2_delimiter_basic
2020-10-05T12:18:54.520 INFO:teuthology.orchestra.run.smithi119.stderr:    eq(response['KeyCount'], len(prefixes) + len(keys))
2020-10-05T12:18:54.520 INFO:teuthology.orchestra.run.smithi119.stderr:AssertionError: 1 != 3

either the fix is incorrect or the test has a bug?

Copy link
Contributor

Choose a reason for hiding this comment

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

this change in rgw_file.{h,cc} couldn't change the expected results of that s3-test, I don't think, I can't explain that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you are right. it is failing on master as well (when I run s3test locally).

@tchaikov tchaikov changed the title missing brackets around if statement in rgw_file rgw/file: missing brackets around if statement Oct 9, 2020
@tchaikov
Copy link
Contributor

tchaikov commented Oct 9, 2020

the title of the commit message reads like a bug report. ideally, the title of the commit message should use the imperative mood. see also https://github.com/ceph/ceph/blob/master/SubmittingPatches.rst#3-describe-your-changes . please use git commit --amend and git push -f for revising the commit message and its title.

@yuvalif yuvalif removed the needs-qa label Oct 11, 2020
@yuvalif
Copy link
Contributor Author

yuvalif commented Oct 11, 2020

recent teuthology results see here: #37534 (comment)

@yuvalif
rgw/file: add missing brackets around if statement
fb7c5b4 
also some small style issues
issues were detected by pvs-studio static analyzer

Signed-off-by: Yuval Lifshitz <ylifshit@redhat.com>
@yuvalif yuvalif force-pushed the missing_brackets_in_rgw_file branch from 48104cd to fb7c5b4 Compare October 11, 2020 06:52
@yuvalif
Copy link
Contributor Author

yuvalif commented Oct 11, 2020

the title of the commit message reads like a bug report. ideally, the title of the commit message should use the imperative mood. see also https://github.com/ceph/ceph/blob/master/SubmittingPatches.rst#3-describe-your-changes . please use git commit --amend and git push -f for revising the commit message and its title.

done

mattbenjamin
mattbenjamin approved these changes Oct 11, 2020
View reviewed changes
Copy link
Contributor

@mattbenjamin mattbenjamin left a comment

Choose a reason for hiding this comment

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

lgtm

@yuvalif
Copy link
Contributor Author

yuvalif commented Oct 11, 2020

jenkins test make check

@yuvalif yuvalif merged commit 9e5f319 into ceph:master Oct 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattbenjamin mattbenjamin mattbenjamin approved these changes

@cbodley cbodley Awaiting requested review from cbodley

Assignees
No one assigned
Labels
bug-fix rgw
Projects
None yet
Milestone
No milestone
3 participants
@yuvalif @tchaikov @mattbenjamin