Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 22 Jul 2020 09:34:23 -0300
From:   Arnaldo Carvalho de Melo <acme@kernel.org>
To:     Alexey Budankov <alexey.budankov@linux.intel.com>
Cc:     Jiri Olsa <jolsa@redhat.com>, Namhyung Kim <namhyung@kernel.org>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Andi Kleen <ak@linux.intel.com>,
        linux-kernel <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v12 05/15] perf evlist: implement control command
 handling functions
Message-ID: <20200722123423.GK77866@kernel.org>
References: <8d91c3a0-3db4-0a7a-ae13-299adb444bd6@linux.intel.com>
 <62518ceb-1cc9-2aba-593b-55408d07c1bf@linux.intel.com>
 <20200722122734.GJ77866@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200722122734.GJ77866@kernel.org>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Em Wed, Jul 22, 2020 at 09:27:34AM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Fri, Jul 17, 2020 at 10:01:33AM +0300, Alexey Budankov escreveu:
> > 
> > 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>
> > Acked-by: Jiri Olsa <jolsa@redhat.com>
> > Acked-by: Namhyung Kim <namhyung@kernel.org>
> 
> I'm applying locally for testing, but can you answer what is the reason
> for that newline in the CMD_ACK_TAG? I tried looking at previous
> discussion but couldn't quickly find any reasoning for that
> 
> - Arnaldo
> 
> > +++ b/tools/perf/util/evlist.h
> > @@ -359,4 +359,22 @@ void perf_evlist__force_leader(struct evlist *evlist);
> >  struct evsel *perf_evlist__reset_weak_group(struct evlist *evlist,
> >  						 struct evsel *evsel,
> >  						bool close);
> > +#define EVLIST_CTL_CMD_ENABLE_TAG  "enable"
> > +#define EVLIST_CTL_CMD_DISABLE_TAG "disable"
> > +#define EVLIST_CTL_CMD_ACK_TAG     "ack\n"
> 
> What for? That '\n'?

Oh, I see, this is not a _command_, it is a _response_, and the sender
expects a newline, right?

So I think this needs to be marked clearly as:

// Commands accepted by 'perf record'

#define EVLIST_CTL_CMD_ENABLE_TAG  "enable"
#define EVLIST_CTL_CMD_DISABLE_TAG "disable"


// Responses sent by 'perf record'

#define EVLIST_CTL_RSP_ACK     "ack"

And then make the response add the newline, as conceivably we could have
something like "nack", "invalid", "enoperm", etc, no?

Anyway, this can be done on top of this series, so I'm continuing
processing it locally.

- Arnaldo