2022-10-22 10:45:54

by Yang Jihong

[permalink] [raw]
Subject: [PATCH 0/3] perf daemon/config: Fix a few minor issues

Yang Jihong (3):
perf daemon: Check err before calling setup_server_config in
__cmd_start
perf daemon: Complete supported subcommand in help message
perf config: Add missing newline on pr_warning call in home_perfconfig

tools/perf/builtin-daemon.c | 4 ++--
tools/perf/util/config.c | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)

--
2.30.GIT


2022-10-22 10:57:10

by Yang Jihong

[permalink] [raw]
Subject: [PATCH 3/3] perf config: Add missing newline on pr_warning call in home_perfconfig

fix missing newline on pr_warning call in home_perfconfig.

Before:
# perf record
File /home/yangjihong/.perfconfig not owned by current user or root, ignoring it.Couldn't synthesize bpf events.

After:
# perf record
File /home/yangjihong/.perfconfig not owned by current user or root, ignoring it.
Couldn't synthesize bpf events.
Signed-off-by: Yang Jihong <[email protected]>
---
tools/perf/util/config.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
index 3f2ae19a1dd4..658170b8dcef 100644
--- a/tools/perf/util/config.c
+++ b/tools/perf/util/config.c
@@ -556,7 +556,7 @@ static char *home_perfconfig(void)

config = strdup(mkpath("%s/.perfconfig", home));
if (config == NULL) {
- pr_warning("Not enough memory to process %s/.perfconfig, ignoring it.", home);
+ pr_warning("Not enough memory to process %s/.perfconfig, ignoring it.\n", home);
return NULL;
}

@@ -564,7 +564,7 @@ static char *home_perfconfig(void)
goto out_free;

if (st.st_uid && (st.st_uid != geteuid())) {
- pr_warning("File %s not owned by current user or root, ignoring it.", config);
+ pr_warning("File %s not owned by current user or root, ignoring it.\n", config);
goto out_free;
}

--
2.30.GIT

2022-10-22 11:00:05

by Yang Jihong

[permalink] [raw]
Subject: [PATCH 1/3] perf daemon: Check err before calling setup_server_config in __cmd_start

If err!=0 before calling setup_server_config and reconfig==true,
setup_server_config function may return 0 and err becomes 0.
As a result, previous error is overwritten, need to check value of err first.

Signed-off-by: Yang Jihong <[email protected]>
---
tools/perf/builtin-daemon.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/builtin-daemon.c b/tools/perf/builtin-daemon.c
index 6cb3f6cc36d0..b82bd902602a 100644
--- a/tools/perf/builtin-daemon.c
+++ b/tools/perf/builtin-daemon.c
@@ -1333,7 +1333,7 @@ static int __cmd_start(struct daemon *daemon, struct option parent_options[],
if (fda.entries[signal_pos].revents & POLLIN)
err = handle_signalfd(daemon) < 0;

- if (reconfig)
+ if (!err && reconfig)
err = setup_server_config(daemon);
}
}
--
2.30.GIT

2022-10-22 11:04:04

by Yang Jihong

[permalink] [raw]
Subject: [PATCH 2/3] perf daemon: Complete supported subcommand in help message

perf daemon supports start, signale, stop and ping subcommands, complete it

Before:
# perf daemon -h
Usage: perf daemon start [<options>]
or: perf daemon [<options>]

-v, --verbose be more verbose
-x, --field-separator[=<field separator>]
print counts with custom separator
--base <directory>
base directory
--config <config file>
config file path

After:
# perf daemon -h

Usage: perf daemon {start|signal|stop|ping} [<options>]
or: perf daemon [<options>]

-v, --verbose be more verbose
-x, --field-separator[=<field separator>]
print counts with custom separator
--base <directory>
base directory
--config <config file>
config file path

Signed-off-by: Yang Jihong <[email protected]>
---
tools/perf/builtin-daemon.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/builtin-daemon.c b/tools/perf/builtin-daemon.c
index b82bd902602a..48698e7fd987 100644
--- a/tools/perf/builtin-daemon.c
+++ b/tools/perf/builtin-daemon.c
@@ -100,7 +100,7 @@ static struct daemon __daemon = {
};

static const char * const daemon_usage[] = {
- "perf daemon start [<options>]",
+ "perf daemon {start|signal|stop|ping} [<options>]",
"perf daemon [<options>]",
NULL
};
--
2.30.GIT

2022-10-24 12:39:34

by Arnaldo Carvalho de Melo

[permalink] [raw]
Subject: Re: [PATCH 1/3] perf daemon: Check err before calling setup_server_config in __cmd_start

Em Mon, Oct 24, 2022 at 08:46:44AM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Sat, Oct 22, 2022 at 05:27:33PM +0800, Yang Jihong escreveu:
> > If err!=0 before calling setup_server_config and reconfig==true,
> > setup_server_config function may return 0 and err becomes 0.
> > As a result, previous error is overwritten, need to check value of err first.

Applied patches 2 and 3 meanwhile, thanks.

- Arnaldo

> > Signed-off-by: Yang Jihong <[email protected]>
> > ---
> > tools/perf/builtin-daemon.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/tools/perf/builtin-daemon.c b/tools/perf/builtin-daemon.c
> > index 6cb3f6cc36d0..b82bd902602a 100644
> > --- a/tools/perf/builtin-daemon.c
> > +++ b/tools/perf/builtin-daemon.c
> > @@ -1333,7 +1333,7 @@ static int __cmd_start(struct daemon *daemon, struct option parent_options[],
> > if (fda.entries[signal_pos].revents & POLLIN)
> > err = handle_signalfd(daemon) < 0;
> >
> > - if (reconfig)
> > + if (!err && reconfig)
> > err = setup_server_config(daemon);
> > }
>
> Expanding the context a bit:
>
> while (!done && !err) {
> err = daemon__reconfig(daemon);
>
> if (!err && fdarray__poll(&fda, -1)) {
> bool reconfig = false;
>
> if (fda.entries[sock_pos].revents & POLLIN)
> err = handle_server_socket(daemon, sock_fd);
> if (fda.entries[file_pos].revents & POLLIN)
> err = handle_config_changes(daemon, conf_fd, &reconfig);
> if (fda.entries[signal_pos].revents & POLLIN)
> err = handle_signalfd(daemon) < 0;
>
> if (!err && reconfig)
> err = setup_server_config(daemon);
> }
> }
>
> The err you're checking may be the last one, that may have overwritten
> 'err' from handle_config_changes(), perhaps moving things around helps?
> I.e.:
> if (!err && fdarray__poll(&fda, -1)) {
> bool reconfig = false;
>
> if (fda.entries[sock_pos].revents & POLLIN)
> err = handle_server_socket(daemon, sock_fd);
> if (fda.entries[signal_pos].revents & POLLIN)
> err = handle_signalfd(daemon) < 0;
> if (fda.entries[file_pos].revents & POLLIN)
> err = handle_config_changes(daemon, conf_fd, &reconfig);
>
> if (!err && reconfig)
> err = setup_server_config(daemon);
> }
>
>
> ?
>
> Jiri?
>
> - Arnaldo
>
> > }
> > --
> > 2.30.GIT
>
> --
>
> - Arnaldo

--

- Arnaldo

2022-10-24 16:22:29

by Arnaldo Carvalho de Melo

[permalink] [raw]
Subject: Re: [PATCH 1/3] perf daemon: Check err before calling setup_server_config in __cmd_start

Em Sat, Oct 22, 2022 at 05:27:33PM +0800, Yang Jihong escreveu:
> If err!=0 before calling setup_server_config and reconfig==true,
> setup_server_config function may return 0 and err becomes 0.
> As a result, previous error is overwritten, need to check value of err first.
>
> Signed-off-by: Yang Jihong <[email protected]>
> ---
> tools/perf/builtin-daemon.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/perf/builtin-daemon.c b/tools/perf/builtin-daemon.c
> index 6cb3f6cc36d0..b82bd902602a 100644
> --- a/tools/perf/builtin-daemon.c
> +++ b/tools/perf/builtin-daemon.c
> @@ -1333,7 +1333,7 @@ static int __cmd_start(struct daemon *daemon, struct option parent_options[],
> if (fda.entries[signal_pos].revents & POLLIN)
> err = handle_signalfd(daemon) < 0;
>
> - if (reconfig)
> + if (!err && reconfig)
> err = setup_server_config(daemon);
> }

Expanding the context a bit:

while (!done && !err) {
err = daemon__reconfig(daemon);

if (!err && fdarray__poll(&fda, -1)) {
bool reconfig = false;

if (fda.entries[sock_pos].revents & POLLIN)
err = handle_server_socket(daemon, sock_fd);
if (fda.entries[file_pos].revents & POLLIN)
err = handle_config_changes(daemon, conf_fd, &reconfig);
if (fda.entries[signal_pos].revents & POLLIN)
err = handle_signalfd(daemon) < 0;

if (!err && reconfig)
err = setup_server_config(daemon);
}
}

The err you're checking may be the last one, that may have overwritten
'err' from handle_config_changes(), perhaps moving things around helps?
I.e.:
if (!err && fdarray__poll(&fda, -1)) {
bool reconfig = false;

if (fda.entries[sock_pos].revents & POLLIN)
err = handle_server_socket(daemon, sock_fd);
if (fda.entries[signal_pos].revents & POLLIN)
err = handle_signalfd(daemon) < 0;
if (fda.entries[file_pos].revents & POLLIN)
err = handle_config_changes(daemon, conf_fd, &reconfig);

if (!err && reconfig)
err = setup_server_config(daemon);
}


?

Jiri?

- Arnaldo

> }
> --
> 2.30.GIT

--

- Arnaldo