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

prov/sockets: Coverity scan fix and minor code cleanup #1104

Closed
wants to merge 2 commits into from
Closed

prov/sockets: Coverity scan fix and minor code cleanup #1104

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6d4339e
prov/sockets: Coverity scan fix
Jun 23, 2015
97c91f1
prov/sockets: Removed redundant handling of pe_entry attributes
Jun 23, 2015
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
16 changes: 10 additions & 6 deletions prov/sockets/src/sock_progress.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1403,10 +1403,10 @@ static int sock_pe_process_rx_send(struct sock_pe *pe, struct sock_rx_ctx *rx_ct
return 0;
len += SOCK_CQ_DATA_SIZE;
}

if (pe_entry->done_len == len && !pe_entry->pe.rx.rx_entry) {
data_len = pe_entry->msg_hdr.msg_len - len;

data_len = pe_entry->msg_hdr.msg_len - len;

if (pe_entry->done_len == len && !pe_entry->pe.rx.rx_entry) {
Copy link
Member

Choose a reason for hiding this comment

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

nit - please watch introducing unneeded white space, especially at the end of a line. Some patch tools will reject patches containing this.

fastlock_acquire(&rx_ctx->lock);
sock_pe_progress_buffered_rx(rx_ctx);

Expand All @@ -1429,7 +1429,6 @@ static int sock_pe_process_rx_send(struct sock_pe *pe, struct sock_rx_ctx *rx_ct
rx_entry->data = pe_entry->data;
rx_entry->ignore = 0;
rx_entry->comp = pe_entry->comp;
pe_entry->context = rx_entry->context;

Copy link
Member

Choose a reason for hiding this comment

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

Why was this changed?

The change to setting data_len looks like a valid fix, versus a code cleanup. The change to context needs more explanation.

Never mind - I was reading the data_len change wrong -- it looks like a simple cleanup. Reading the source, I see why the context change was done, but the patch description should explain that better. Reading just the patch, the change looks wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, the description should have been clearer. Updating the description

if (pe_entry->msg_hdr.flags & FI_REMOTE_CQ_DATA)
rx_entry->flags |= FI_REMOTE_CQ_DATA;
Expand All @@ -1445,7 +1444,7 @@ static int sock_pe_process_rx_send(struct sock_pe *pe, struct sock_rx_ctx *rx_ct

rx_entry = pe_entry->pe.rx.rx_entry;
done_data = pe_entry->done_len - len;
pe_entry->data_len = pe_entry->msg_hdr.msg_len - len;
pe_entry->data_len = data_len;
rem = pe_entry->data_len - done_data;
used = rx_entry->used;

Expand Down Expand Up @@ -2464,6 +2463,11 @@ static void sock_thread_set_affinity(char *s)
int j, first, last,stride;
cpu_set_t mycpuset;
pthread_t mythread;

if(!s) {
SOCK_LOG_DBG("Invalid FI_SOCKETS_PE_AFFINITY value\n");
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

This check shouldn't be needed. This comes from fi_var_get_str(). If that call returns success, the return string will not be NULL.

Copy link
Member

Choose a reason for hiding this comment

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

The coverity issue deals with the saveptra variable:

CID 100409 (#1 of 1): Explicit null dereferenced (FORWARD_NULL)6. var_deref_model: Passing &saveptra to __strtok_r_1c, which dereferences null saveptra. [show details]

This is not an actual issue. The checker just can't distinguish the first call to strtok_r, versus a subsequent call. I marked this defect as invalid in coverity. Hopefully that works to dismiss it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also found that the check is not needed but since it was reported by coverity, I added it. Thanks for reporting it in coverity.

Copy link
Member

Choose a reason for hiding this comment

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

If you re-read the coverity report, the issue is not the 's' parameter into strtok_r, but the 'saveptra' parameter. This change would not have fixed the issue.


mythread = pthread_self();
CPU_ZERO(&mycpuset);
Expand Down