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

DAOS-13996 pool: Fix invalid D_FREE in pool_glance #12683

Merged
merged 2 commits into from
Jul 31, 2023
Merged

DAOS-13996 pool: Fix invalid D_FREE in pool_glance #12683

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dae028b
DAOS-13996 pool: Fix invalid D_FREE in pool_glance
liw Jul 21, 2023
3aea445
Merge branch 'feature/cat_recovery' into liw/fix-map_buf-free
liw Jul 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions src/pool/srv_pool.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1540,8 +1540,6 @@ ds_pool_svc_load(struct rdb_tx *tx, uuid_t uuid, rdb_path_t *root, uint32_t *glo
*global_version_out = global_version;
*map_buf_out = map_buf;
*map_version_out = map_version;
if (rc != 0)
D_FREE(map_buf);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

[Note] This is probably caused by previous merged with master.

Copy link
Contributor

Choose a reason for hiding this comment

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

or set "*map_version_out = NULL" at above L1519 (rc != 0) case, seems in that case the map_buf ptr in stack possibly not init.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@liuxuezhao, it is intentional that ds_pool_svc_load changes *map_version_out only when returning zero. Callers shall only expect an output in *map_version_out if ds_pool_svc_load returns zero.

out:
return rc;
}
Expand Down Expand Up @@ -7730,4 +7728,4 @@ struct cont_svc *
ds_pool_ps2cs(struct ds_pool_svc *ds_svc)
{
return pool_ds2svc(ds_svc)->ps_cont_svc;
}
}
7 changes: 4 additions & 3 deletions src/pool/srv_pool_check.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* (C) Copyright 2022 Intel Corporation.
* (C) Copyright 2022-2023 Intel Corporation.
*
* SPDX-License-Identifier: BSD-2-Clause-Patent
*/
Expand Down Expand Up @@ -84,15 +84,16 @@ pool_glance(uuid_t uuid, char *path, struct ds_pool_clue *clue_out)
}

rc = ds_pool_svc_load(&tx, uuid, &root, &global_version, &map_buf, &clue.psc_map_version);
if (rc == DER_UNINIT) {
if (rc == 0) {
D_FREE(map_buf);
} else if (rc == DER_UNINIT) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just want to know why ds_pool_svc_load() return positive number for empty pool map instead of "-DER_UNINIT"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because that is a important, non-error scenario that will almost 100% happen during the creation of a PS, and I was afraid that functions called by ds_pool_svc_load may return -DER_UNINIT for other unrelated scenarios. Hence, I chose a positive return value many years ago.

If I were to make the same choice again today, I might prefer defining an enum with a positive value.

Copy link
Contributor

Choose a reason for hiding this comment

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

Because that is a important, non-error scenario that will almost 100% happen during the creation of a PS, and I was afraid that functions called by ds_pool_svc_load may return -DER_UNINIT for other unrelated scenarios. Hence, I chose a positive return value many years ago.

If I were to make the same choice again today, I might prefer defining an enum with a positive value.

I think you can do that in master independently.

clue.psc_map_version = 0;
rc = 0;
} else if (rc != 0) {
goto out_label;
}

memcpy(clue_out->pc_svc_clue, &clue, sizeof(clue));
D_FREE(map_buf);
out_label:
if (rc != 0) {
D_FREE(clue_out->pc_label);
Expand Down