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

DAOS-6808 rpc: Fix cleanuping resources #4807

Merged
merged 2 commits into from
Mar 17, 2021
Merged

DAOS-6808 rpc: Fix cleanuping resources #4807

merged 2 commits into from
Mar 17, 2021

Conversation

dmiter
Copy link
Contributor

@dmiter dmiter commented Feb 26, 2021 •
edited
Loading

Split this patch into several small patches. This patch is for following:

  • Fix reference counting for rpc
  • Free memory in an error path

daosbuild1
daosbuild1 reviewed Feb 26, 2021
View reviewed changes
Copy link
Collaborator

@daosbuild1 daosbuild1 left a comment

Choose a reason for hiding this comment

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

LGTM. No errors found by checkpatch.

@daosbuild1
Copy link
Collaborator

Test stage Functional on Leap 15 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-4807/1/execution/node/1309/log

@daosbuild1
Copy link
Collaborator

Test stage Functional_Hardware_Medium completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-4807/1/execution/node/1466/log

daosbuild1
daosbuild1 reviewed Feb 28, 2021
View reviewed changes
Copy link
Collaborator

@daosbuild1 daosbuild1 left a comment

Choose a reason for hiding this comment

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

LGTM. No errors found by checkpatch.

daosbuild1
daosbuild1 reviewed Feb 28, 2021
View reviewed changes
Copy link
Collaborator

@daosbuild1 daosbuild1 left a comment

Choose a reason for hiding this comment

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

LGTM. No errors found by checkpatch.

@dmiter dmiter force-pushed the deremin/DAOS-6808 branch from 5ae3c3c to fb50b48 Compare March 1, 2021 21:02
daosbuild1
daosbuild1 reviewed Mar 1, 2021
View reviewed changes
Copy link
Collaborator

@daosbuild1 daosbuild1 left a comment

Choose a reason for hiding this comment

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

LGTM. No errors found by checkpatch.

@dmiter dmiter force-pushed the deremin/DAOS-6808 branch from fb50b48 to e43f076 Compare March 1, 2021 21:17
daosbuild1
daosbuild1 reviewed Mar 1, 2021
View reviewed changes
Copy link
Collaborator

@daosbuild1 daosbuild1 left a comment

Choose a reason for hiding this comment

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

LGTM. No errors found by checkpatch.

@daosbuild1
Copy link
Collaborator

Test stage Functional_Hardware_Small completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-4807/5/execution/node/1391/log

@daosbuild1
Copy link
Collaborator

Test stage Functional_Hardware_Large completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-4807/5/execution/node/1420/log

@daosbuild1
Copy link
Collaborator

Test stage Functional_Hardware_Medium completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-4807/5/execution/node/1449/log

@daosbuild1
Copy link
Collaborator

Test stage Functional on CentOS 7 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-4807/5/execution/node/1528/log

@dmiter dmiter force-pushed the deremin/DAOS-6808 branch from e43f076 to fb6cc2d Compare March 2, 2021 09:12
daosbuild1
daosbuild1 reviewed Mar 2, 2021
View reviewed changes
Copy link
Collaborator

@daosbuild1 daosbuild1 left a comment

Choose a reason for hiding this comment

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

LGTM. No errors found by checkpatch.

@daosbuild1
Copy link
Collaborator

Test stage Functional_Hardware_Medium completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-4807/6/execution/node/1393/log

@daosbuild1
Copy link
Collaborator

Test stage Functional_Hardware_Small completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-4807/6/execution/node/1421/log

@daosbuild1
Copy link
Collaborator

Test stage Functional_Hardware_Large completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-4807/6/execution/node/1408/log

@daosbuild1
Copy link
Collaborator

Test stage Functional on CentOS 7 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-4807/6/execution/node/1530/log

@dmiter dmiter force-pushed the deremin/DAOS-6808 branch from fb6cc2d to 4c71cf1 Compare March 2, 2021 13:45
@dmiter dmiter changed the title DAOS-6808 rpc: cleanup RPC related code DAOS-6808 misc: Fix cleanuping resources Mar 2, 2021
daosbuild1
daosbuild1 reviewed Mar 2, 2021
View reviewed changes
Copy link
Collaborator

@daosbuild1 daosbuild1 left a comment

Choose a reason for hiding this comment

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

LGTM. No errors found by checkpatch.

@dmiter dmiter force-pushed the deremin/DAOS-6808 branch from 4c71cf1 to f6da5b0 Compare March 2, 2021 20:47
daosbuild1
daosbuild1 reviewed Mar 2, 2021
View reviewed changes
Copy link
Collaborator

@daosbuild1 daosbuild1 left a comment

Choose a reason for hiding this comment

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

LGTM. No errors found by checkpatch.

@dmiter dmiter force-pushed the deremin/DAOS-6808 branch from f6da5b0 to 37b3d86 Compare March 2, 2021 20:57
daosbuild1
daosbuild1 reviewed Mar 2, 2021
View reviewed changes
Copy link
Collaborator

@daosbuild1 daosbuild1 left a comment

Choose a reason for hiding this comment

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

LGTM. No errors found by checkpatch.

@daosbuild1
Copy link
Collaborator

Test stage Scan CentOS 7 RPMs completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-4807/9/execution/node/1152/log

@dmiter dmiter force-pushed the deremin/DAOS-6808 branch from 37b3d86 to 6dcc439 Compare March 3, 2021 09:55
@dmiter dmiter requested a review from liw March 12, 2021 11:20
jolivier23
jolivier23 requested changes Mar 12, 2021
View reviewed changes
Copy link
Contributor

@jolivier23 jolivier23 left a comment

Choose a reason for hiding this comment

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

I think this patch needs to be broken up. It covers way too many areas of expertise making it difficult for any one person to review. For example, the patch to dfs would be easily reviewed and landed on its own.

mchaarawi
mchaarawi reviewed Mar 12, 2021
View reviewed changes
src/client/dfs/dfs.c Outdated
@@ -2968,16 +2968,14 @@ dfs_release(dfs_obj_t *obj)
break;
default:
D_ERROR("Invalid entry type (not a dir, file, symlink).\n");
rc = EINVAL;
rc = -DER_IO_INVAL;
Copy link
Contributor

@mchaarawi mchaarawi Mar 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

this is wrong? we should return positive errno.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh my bad actually. we do call daos_der2errno later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, exactly. I changed to always translate errors through daos_der2errno() at return.

@johannlombardi
Copy link
Contributor

Please rebase.

@daosbuild1
Copy link
Collaborator

Test stage Functional_Hardware_Medium completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-4807/22/execution/node/1326/log

DAOS-6808 rpc: Fix cleanuping resources
9581d5f 
- Fix reference counting for rpc
- Free memory in an error path

Signed-off-by: Dmitry Eremin <dmitry.eremin@intel.com>
@dmiter dmiter force-pushed the deremin/DAOS-6808 branch from 1c95c5f to 9581d5f Compare March 15, 2021 12:30
@dmiter dmiter changed the title DAOS-6808 misc: Fix cleanuping resources DAOS-6808 rpc: Fix cleanuping resources Mar 15, 2021
daosbuild1
daosbuild1 reviewed Mar 15, 2021
View reviewed changes
Copy link
Collaborator

@daosbuild1 daosbuild1 left a comment

Choose a reason for hiding this comment

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

LGTM. No errors found by checkpatch.

@dmiter dmiter requested review from jolivier23 and mchaarawi March 15, 2021 12:35
daosbuild1
daosbuild1 reviewed Mar 15, 2021
View reviewed changes
Copy link
Collaborator

@daosbuild1 daosbuild1 left a comment

Choose a reason for hiding this comment

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

LGTM. No errors found by checkpatch.

liw
liw approved these changes Mar 16, 2021
View reviewed changes
Copy link
Contributor

@liw liw left a comment

Choose a reason for hiding this comment

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

src/{container,pool,mgmt} changes look good to me.

@dmiter
Copy link
Contributor Author

dmiter commented Mar 16, 2021

FTEST_pool.PoolCreateTests failed because of DAOS-7016

Nasf-Fan
Nasf-Fan reviewed Mar 17, 2021
View reviewed changes
src/object/srv_obj_remote.c
D_ERROR("crt_req_send failed, rc "DF_RC"\n", DP_RC(rc));
crt_req_decref(req);
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that crt_req_send() cannot guarantee the complete_cb() will always be called for some failure cases. So we still may leak something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to @liuxuezhao and my investigation crt_req_send() always call callback even on error path.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, crt_req_send() can ensure the completion_cb is called as long as user provided.

@Nasf-Fan Nasf-Fan self-requested a review March 17, 2021 04:51
liuxuezhao
liuxuezhao approved these changes Mar 17, 2021
View reviewed changes
src/object/srv_obj_remote.c
D_ERROR("crt_req_send failed, rc "DF_RC"\n", DP_RC(rc));
crt_req_decref(req);
Copy link
Contributor

Choose a reason for hiding this comment

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

yes, crt_req_send() can ensure the completion_cb is called as long as user provided.

@dmiter dmiter requested a review from a team March 17, 2021 14:19
@johannlombardi johannlombardi merged commit 3d23147 into master Mar 17, 2021
@johannlombardi johannlombardi deleted the deremin/DAOS-6808 branch March 17, 2021 14:22
sydneyvanda added a commit that referenced this pull request Mar 24, 2021
@sydneyvanda
DAOS-6681 sdl: Various coverity defects in object and EC code (#4919)
be332fb 
CID 316292: Unintentional integer overflow
CID 316293: Explicit null dereferenced
CID 316294: Explicit null dereferenced
CID 316295: Explicit null dereferenced
CID 316297: Unused value
CID 316299: Missing break in switch
CID 316300: Missing break in switch

Also includes updates from DAOS-6806 rpc: Fix cleanup resources (#4807)
(srv_ec_aggregate.c file updates only)

Signed-off-by: Sydney Vanda <sydney.m.vanda@intel.com>
@sydneyvanda sydneyvanda mentioned this pull request Mar 24, 2021
Merged
jolivier23 pushed a commit that referenced this pull request Mar 29, 2021
@sydneyvanda
DAOS-6681 sdl: Various coverity defects in object and EC code (#4919) (…
f6b74f4 
…#5159)

CID 316292: Unintentional integer overflow
CID 316293: Explicit null dereferenced
CID 316294: Explicit null dereferenced
CID 316295: Explicit null dereferenced
CID 316297: Unused value
CID 316299: Missing break in switch
CID 316300: Missing break in switch

Also includes updates from DAOS-6806 rpc: Fix cleanup resources (#4807)
(srv_ec_aggregate.c file updates only)

Signed-off-by: Sydney Vanda <sydney.m.vanda@intel.com>
dmiter pushed a commit that referenced this pull request Apr 6, 2021
DAOS-6808 rpc: Fix cleanuping resources (#4807)
5dbb40a 
- Fix reference counting for rpc
- Free memory in an error path

Signed-off-by: Dmitry Eremin <dmitry.eremin@intel.com>
dmiter pushed a commit that referenced this pull request May 11, 2021
DAOS-6808 rpc: Fix cleanuping resources (#4807)
75a6f36 
- Fix reference counting for rpc
- Free memory in an error path

Signed-off-by: Dmitry Eremin <dmitry.eremin@intel.com>
dmiter pushed a commit that referenced this pull request May 27, 2021
DAOS-6808 rpc: Fix cleanuping resources (#4807)
c08967a 
- Fix reference counting for rpc
- Free memory in an error path

Signed-off-by: Dmitry Eremin <dmitry.eremin@intel.com>
dmiter pushed a commit that referenced this pull request Jun 7, 2021
DAOS-6808 rpc: Fix cleanuping resources (#4807)
fc468a2 
- Fix reference counting for rpc
- Free memory in an error path

Test-tag: pr daily_regression
Signed-off-by: Dmitry Eremin <dmitry.eremin@intel.com>
johannlombardi pushed a commit that referenced this pull request Jun 11, 2021
DAOS-6808 rpc: Fix cleanuping resources (#4807) (#5339)
835766b 
- Fix reference counting for rpc
- Free memory in an error path

Test-tag: pr daily_regression
Signed-off-by: Dmitry Eremin <dmitry.eremin@intel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daosbuild1 daosbuild1 daosbuild1 left review comments

@liw liw liw approved these changes

@liuxuezhao liuxuezhao liuxuezhao approved these changes

@Nasf-Fan Nasf-Fan Nasf-Fan approved these changes

@jgmoore-or jgmoore-or Awaiting requested review from jgmoore-or

@gnailzenh gnailzenh Awaiting requested review from gnailzenh

@johannlombardi johannlombardi Awaiting requested review from johannlombardi

@ashleypittman ashleypittman Awaiting requested review from ashleypittman

@wangdi1 wangdi1 Awaiting requested review from wangdi1

@frostedcmos frostedcmos Awaiting requested review from frostedcmos

@jolivier23 jolivier23 Awaiting requested review from jolivier23

@mchaarawi mchaarawi Awaiting requested review from mchaarawi

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.