Messages in this thread | | | Subject | Re: [PATCH v10 05/15] perf evlist: implement control command handling functions | From | Alexey Budankov <> | Date | Mon, 13 Jul 2020 11:24:54 +0300 |
| |
On 13.07.2020 6:20, Namhyung Kim wrote: > On Wed, Jul 8, 2020 at 4:50 PM Alexey Budankov > <alexey.budankov@linux.intel.com> wrote: >> >> >> Implement functions of initialization, finalization and processing >> of control command messages coming from control file descriptors. >> Allocate control file descriptor as descriptor at struct pollfd >> object of evsel_list for atomic poll() operation. >> >> Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com> >> --- > [SNIP] >> +static int evlist__ctlfd_recv(struct evlist *evlist, enum evlist_ctl_cmd *cmd, >> + char *cmd_data, size_t data_size) >> +{ >> + int err; >> + char c; >> + size_t bytes_read = 0; >> + >> + memset(cmd_data, 0, data_size--); > > I overlooked the '--' at the end and thought there might be > buffer overflow.. Care to add a comment?
If it wants to be more explicit then it could possibly be implemented like this:
memset(cmd_data, 0, data_size); data_size--;
> >> + >> + do { >> + err = read(evlist->ctl_fd.fd, &c, 1); > > Maybe I missed earlier discussion, but do we really want > this 1 byte read in a loop?
That was the explicit request to support commands of variable length so it implements it like that.
Alexei
> > Thanks > Namhyung > > >> + if (err > 0) { >> + if (c == '\n' || c == '\0') >> + break; >> + cmd_data[bytes_read++] = c; >> + if (bytes_read == data_size) >> + break; >> + } else { >> + if (err == -1) >> + pr_err("Failed to read from ctlfd %d: %m\n", evlist->ctl_fd.fd); >> + break; >> + } >> + } while (1); >> + >> + pr_debug("Message from ctl_fd: \"%s%s\"\n", cmd_data, >> + bytes_read == data_size ? "" : c == '\n' ? "\\n" : "\\0"); >> + >> + if (err > 0) { >> + if (!strncmp(cmd_data, EVLIST_CTL_CMD_ENABLE_TAG, >> + (sizeof(EVLIST_CTL_CMD_ENABLE_TAG)-1))) { >> + *cmd = EVLIST_CTL_CMD_ENABLE; >> + } else if (!strncmp(cmd_data, EVLIST_CTL_CMD_DISABLE_TAG, >> + (sizeof(EVLIST_CTL_CMD_DISABLE_TAG)-1))) { >> + *cmd = EVLIST_CTL_CMD_DISABLE; >> + } >> + } >> + >> + return err; >> +}
| |