Skip to content

DAOS-6685 Test: Fixed coverity issue in test code. - #4583

Merged
sylviachanoiyee merged 6 commits into
masterfrom
samirrav/DAOS-6685
Feb 17, 2021
Merged

sylviachanoiyee merged 6 commits into
masterfrom
samirrav/DAOS-6685

Conversation

@ravalsam

@ravalsam ravalsam commented Feb 8, 2021

Copy link
Copy Markdown
Contributor

Signed-off-by: Samir Raval samir.raval@intel.com

Test-tag-hw-medium: nvme,unittest

@daosbuild1 daosbuild1 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Comment thread src/common/tests_dmg_helpers.c Outdated
Comment thread src/common/tests_dmg_helpers.c Outdated
Comment thread src/common/tests_dmg_helpers.c Outdated
@daosbuild1

Copy link
Copy Markdown
Collaborator

@daosbuild1
daosbuild1 dismissed their stale review February 8, 2021 23:56

Updated patch

@daosbuild1 daosbuild1 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Comment thread src/common/tests_dmg_helpers.c Outdated
@daosbuild1

Copy link
Copy Markdown
Collaborator

Signed-off-by: Samir Raval <samir.raval@intel.com>

Test-tag-hw-medium: nvme,unittest
Signed-off-by: Samir Raval <samir.raval@intel.com>
@daosbuild1
daosbuild1 dismissed their stale review February 9, 2021 00:31

Updated patch

@daosbuild1 daosbuild1 left a comment

Copy link
Copy Markdown
Collaborator

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.

@ravalsam

ravalsam commented Feb 9, 2021

Copy link
Copy Markdown
Contributor Author

NVMe recovery test failure is because of DAOS-6764 and DAOS-6717

dev = json_object_array_get_idx(smd_dev, i);
strcpy(devices[*disks].host, strtok(host, ":") + 1);

if (strlen(host + 2) <= DSS_HOSTNAME_MAX_LEN)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does (host+2) instead of (host) points to the real host name?

devices[*disks].host is an 50bytes array:
typedef struct {
uuid_t device_id;
char state[10];
int rank;
char host[50];
int tgtidx[MAX_TEST_TARGETS_PER_DEVICE];
int n_tgtidx;
} device_list;

and
#define DSS_HOSTNAME_MAX_LEN 255
Won't there still a risk of Copy into fixed size buffer (STRING_OVERFLOW)?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

host[50] has been changed to host[DSS_HOSTNAME_MAX_LEN]. Please see the changes below..

typedef struct {
	uuid_t		device_id;
	char		state[10];
	int		rank;
	char		host[DSS_HOSTNAME_MAX_LEN];
	int		tgtidx[MAX_TEST_TARGETS_PER_DEVICE];
	int		n_tgtidx;
}  device_list;

@GitHuaKuang

Copy link
Copy Markdown
Contributor

https://scan8.coverity.com/reports.htm#v42469/p12225/fileInstanceId=39449665&defectInstanceId=11402595&mergedDefectId=313074
was added to this ticket after creation, it's in the same tests_dmg_helpers.c, will it handled in this PR or a separate PR?

@ravalsam

ravalsam commented Feb 9, 2021

Copy link
Copy Markdown
Contributor Author

https://scan8.coverity.com/reports.htm#v42469/p12225/fileInstanceId=39449665&defectInstanceId=11402595&mergedDefectId=313074
was added to this ticket after creation, it's in the same tests_dmg_helpers.c, will it handled in this PR or a separate PR?

Yes, I miss the one which was added later. let me update the current PR

Signed-off-by: Samir Raval <samir.raval@intel.com>

@daosbuild1 daosbuild1 left a comment

Copy link
Copy Markdown
Collaborator

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.

@ravalsam
ravalsam requested a review from GitHuaKuang February 10, 2021 00:09
Comment thread src/common/tests_dmg_helpers.c Outdated
if (!json_object_object_get_ex(val, "storage", &storage_info)) {
D_ERROR("unable to extract hosts from JSON\n");
return -DER_INVAL;
D_GOTO(out, rc = -DER_INVAL);

@GitHuaKuang GitHuaKuang Feb 10, 2021 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Shouldn't goto out_json in order to execute cmd_free_args(args, argcount);?

Also, the json_object_object_get_ex() call before the loop (_foreach) needs D_GOTO as well I think. So total 3 places.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, the intention was to add out_json. Thanks for catching it

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What about the if statement in line #791-#794? I think the cid pointed out this case as #1 of 2 case:

775 if (!json_object_object_get_ex(dmg_out, "host_storage_map",
776 &storage_map)) {
777 D_ERROR("unable to extract host_storage_map from JSON\n");
CID 313074 (#1 of 2): Resource leak (RESOURCE_LEAK) [select issue]
778 return -DER_INVAL;
779 }
780
781 json_object_object_foreach(storage_map, key, val) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Is this new I do not see it in defect DAOS-6685?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's in https://scan8.coverity.com/reports.htm#v42469/p12225/fileInstanceId=39449665&defectInstanceId=11402595&mergedDefectId=313074
There are two occurrences for this issue, #1 of 2 and #2 of 2, It seems #1 of 2 I copied above maybe also need to make the same change.

@ravalsam ravalsam Feb 10, 2021 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Both occurrences have been fixed.

Test-tag-hw-medium: nvme,unittest

@daosbuild1 daosbuild1 left a comment

Copy link
Copy Markdown
Collaborator

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.

Test-tag-hw-medium: nvme,unittest

@daosbuild1 daosbuild1 left a comment

Copy link
Copy Markdown
Collaborator

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.

Test-tag-hw-medium: nvme,unittest

Signed-off-by: Samir Raval <samir.raval@intel.com>

@daosbuild1 daosbuild1 left a comment

Copy link
Copy Markdown
Collaborator

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.

@ravalsam
ravalsam requested a review from GitHuaKuang February 10, 2021 23:12

@daosbuild1 daosbuild1 left a comment

Copy link
Copy Markdown
Collaborator

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.

@ravalsam
ravalsam requested a review from a team February 17, 2021 01:47
@sylviachanoiyee
sylviachanoiyee merged commit d33d0c7 into master Feb 17, 2021
@sylviachanoiyee
sylviachanoiyee deleted the samirrav/DAOS-6685 branch February 17, 2021 04:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

5 participants