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

client: priority to verify the correctness of the "flag" #12897

Merged
merged 1 commit into from Feb 22, 2017

Conversation

Projects
None yet
3 participants
@renhwztetecs
Member

renhwztetecs commented Jan 12, 2017

Signed-off-by: huanwen ren ren.huanwen@zte.com.cn

client: priority to verify the correctness of the "flag"
Signed-off-by: huanwen ren <ren.huanwen@zte.com.cn>
@@ -6019,7 +6019,8 @@ int Client::get_or_create(Inode *dir, const char* name,
{
// lookup
ldout(cct, 20) << "get_or_create " << *dir << " name " << name << dendl;
dir->open_dir();
Dir *dircheck = dir->open_dir();
assert(dircheck);

This comment has been minimized.

@gregsfortytwo

gregsfortytwo Jan 12, 2017

Member

I don't understand. Inode::open_dir() is conditioned on this and won't return NULL.

This comment has been minimized.

@jcsp

jcsp Feb 6, 2017

Contributor

Yep, as greg says, this is a redundant change.

}
bool is_enable() const {
return max_bytes || max_files;
return max_bytes > 0 || max_files > 0 ;

This comment has been minimized.

@gregsfortytwo

gregsfortytwo Jan 12, 2017

Member

Did you find specific problems with this, or just concerned the values could be negative?

This comment has been minimized.

@jcsp

jcsp Feb 6, 2017

Contributor

Github seems to have eaten a comment (which I saw by email) here:

@gregsfortytwo @jcsp
Not found the problem, just concerned the values could be negative,
and the same time, return a bool type better than int64_t type, because the is_enable()'return is bool type,and avoid the values could be negative.

So I don't think any of that is a reason to put the > 0 here -- it would be reasonable to suggest that max_bytes should be unsigned, but given that it is signed, I don't think it is logical to interpret a "-1" value as making is_enable() return false.

@jcsp

This comment has been minimized.

Contributor

jcsp commented Jan 26, 2017

@renhwztetecs could you have a look at greg's comments please?

@renhwztetecs

This comment has been minimized.

Member

renhwztetecs commented Feb 6, 2017

@jcsp
Does my understanding have an error? :-)

@jcsp

This comment has been minimized.

Contributor

jcsp commented Feb 6, 2017

@renhwztetecs I'm happy with the "client: priority to verify the correctness of the "flag"" commit, but the others just seem unnecessary to me

@renhwztetecs renhwztetecs changed the title from client: fixed call the open_dir() and quota_info_t.is_enable to client: priority to verify the correctness of the "flag" Feb 7, 2017

@renhwztetecs

This comment has been minimized.

Member

renhwztetecs commented Feb 7, 2017

@gregsfortytwo @jcsp
thanks
I update it

@jcsp

jcsp approved these changes Feb 22, 2017

@jcsp jcsp merged commit 64eae76 into ceph:master Feb 22, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details

@renhwztetecs renhwztetecs deleted the renhwztetecs:renhw-wip-client-cleanup branch Mar 1, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment