On 23.06.2020 17:56, Jiri Olsa wrote:
> On Wed, Jun 17, 2020 at 11:36:46AM +0300, Alexey Budankov wrote:
>
> 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--);
>> +
>> + do {
>> + err = read(evlist->ctl_fd.fd, &c, 1);
>> + 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");
>
> do you want to display the message above only if (err > 0) ?
I do, only for err >= 0. For err == -1 there is already the error message.
>
>> +
>> + if (err > 0) {
>> + if (!strncmp(cmd_data, EVLIST_CTL_CMD_ENABLE_TAG,
>> + strlen(EVLIST_CTL_CMD_ENABLE_TAG))) {
>> + *cmd = EVLIST_CTL_CMD_ENABLE;
>> + } else if (!strncmp(cmd_data, EVLIST_CTL_CMD_DISABLE_TAG,
>> + strlen(EVLIST_CTL_CMD_DISABLE_TAG))) {
>> + *cmd = EVLIST_CTL_CMD_DISABLE;
>> + }
>> + }
>> +
>> + return err;
>> +}
>
> SNIP
>
>> + int ctlfd_pos = evlist->ctl_fd.pos;
>> + struct pollfd *entries = evlist->core.pollfd.entries;
>> +
>> + if (ctlfd_pos == -1 || !entries[ctlfd_pos].revents)
>> + return 0;
>> +
>> + if (entries[ctlfd_pos].revents & POLLIN) {
>> + err = evlist__ctlfd_recv(evlist, cmd, cmd_data,
>> + EVLIST_CTL_CMD_MAX_LEN);
>> + if (err > 0) {
>> + switch (*cmd) {
>> + case EVLIST_CTL_CMD_ENABLE:
>> + evlist__enable(evlist);
>> + break;
>> + case EVLIST_CTL_CMD_DISABLE:
>> + evlist__disable(evlist);
>> + break;
>> + case EVLIST_CTL_CMD_ACK:
>> + case EVLIST_CTL_CMD_UNSUPPORTED:
>> + default:
>> + pr_debug("ctlfd: unsupported %d\n", *cmd);
>> + break;
>> + }
>> + if (!(*cmd == EVLIST_CTL_CMD_ACK || *cmd ==
>> EVLIST_CTL_CMD_UNSUPPORTED))
>> + evlist__ctlfd_ack(evlist);
>> + }
>> + }
>> +
>> + if (entries[ctlfd_pos].revents & (POLLHUP | POLLERR))
>> + evlist__finalize_ctlfd(evlist);
>
> should this error be propagated via err?
I don't think so. Currently it returns 0 only and there is no need to
stop collection or skip processing in caller in this case.
~Alexey
>
> jirka
>
