Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1290826 > unrolled thread

[PATCH v3 00/17] perf tools: Move perf subcommand framework to a library

Back to article view | Back to linux.kernel

Contents

  [PATCH v3 00/17] perf tools: Move perf subcommand framework to a library Josh Poimboeuf <jpoimboe@redhat.com> - 2015-12-14 05:20 +0100
    [PATCH v3 06/17] perf build: Rename LIB_PATH -> API_PATH Josh Poimboeuf <jpoimboe@redhat.com> - 2015-12-14 05:20 +0100
      Re: [PATCH v3 06/17] perf build: Rename LIB_PATH -> API_PATH Jiri Olsa <jolsa@redhat.com> - 2015-12-14 11:40 +0100
        Re: [PATCH v3 06/17] perf build: Rename LIB_PATH -> API_PATH Arnaldo Carvalho de Melo <acme@kernel.org> - 2015-12-14 16:20 +0100
      [tip:perf/core] perf build: Rename LIB_PATH -> API_PATH tip-bot for Josh Poimboeuf <tipbot@zytor.com> - 2015-12-18 09:50 +0100
    [PATCH v3 02/17] tools build: Fix feature Makefile dependencies for 'O=' Josh Poimboeuf <jpoimboe@redhat.com> - 2015-12-14 05:20 +0100
      Re: [PATCH v3 02/17] tools build: Fix feature Makefile dependencies  for 'O=' Jiri Olsa <jolsa@redhat.com> - 2015-12-14 11:10 +0100
        Re: [PATCH v3 02/17] tools build: Fix feature Makefile dependencies  for 'O=' Josh Poimboeuf <jpoimboe@redhat.com> - 2015-12-14 15:10 +0100
          [PATCH v3.1 02/17] tools build: Fix feature Makefile dependencies  for 'O=' Josh Poimboeuf <jpoimboe@redhat.com> - 2015-12-14 15:30 +0100
            Re: [PATCH v3.1 02/17] tools build: Fix feature Makefile  dependencies for 'O=' Arnaldo Carvalho de Melo <acme@kernel.org> - 2015-12-14 16:20 +0100
            Re: [PATCH v3.1 02/17] tools build: Fix feature Makefile  dependencies for 'O=' Jiri Olsa <jolsa@redhat.com> - 2015-12-14 16:30 +0100
              Re: [PATCH v3.1 02/17] tools build: Fix feature Makefile  dependencies for 'O=' Josh Poimboeuf <jpoimboe@redhat.com> - 2015-12-14 16:40 +0100
                Re: [PATCH v3.1 02/17] tools build: Fix feature Makefile  dependencies for 'O=' Jiri Olsa <jolsa@redhat.com> - 2015-12-14 17:00 +0100
                  Re: [PATCH v3.1 02/17] tools build: Fix feature Makefile  dependencies for 'O=' Josh Poimboeuf <jpoimboe@redhat.com> - 2015-12-14 17:10 +0100
                  [PATCH v3.2] tools build: Fix feature Makefile issues with 'O=' Josh Poimboeuf <jpoimboe@redhat.com> - 2015-12-14 19:40 +0100
    [PATCH v3 08/17] perf: Remove check for unused PERF_PAGER_IN_USE Josh Poimboeuf <jpoimboe@redhat.com> - 2015-12-14 05:20 +0100
      [tip:perf/core] perf tools:   Remove check for unused PERF_PAGER_IN_USE tip-bot for Josh Poimboeuf <tipbot@zytor.com> - 2015-12-18 10:00 +0100
    [PATCH v3 05/17] perf build: Fix 'make clean' Josh Poimboeuf <jpoimboe@redhat.com> - 2015-12-14 05:30 +0100
      Re: [PATCH v3 05/17] perf build: Fix 'make clean' Jiri Olsa <jolsa@redhat.com> - 2015-12-14 11:20 +0100
        Re: [PATCH v3 05/17] perf build: Fix 'make clean' Arnaldo Carvalho de Melo <acme@kernel.org> - 2015-12-14 16:20 +0100
      [tip:perf/core] perf build: Fix 'make clean' tip-bot for Josh Poimboeuf <tipbot@zytor.com> - 2015-12-18 09:50 +0100
    [PATCH v3 13/17] perf: Document the fact that parse_options*() may exit Josh Poimboeuf <jpoimboe@redhat.com> - 2015-12-14 05:30 +0100
    [PATCH v3 09/17] perf: Move help_unknown_cmd() to its own file Josh Poimboeuf <jpoimboe@redhat.com> - 2015-12-14 05:30 +0100
      [tip:perf/core] perf tools: Move help_unknown_cmd()   to its own file tip-bot for Josh Poimboeuf <tipbot@zytor.com> - 2015-12-18 10:00 +0100
    [PATCH v3 12/17] perf: Convert parse-options.c internal functions to static Josh Poimboeuf <jpoimboe@redhat.com> - 2015-12-14 05:30 +0100
      [tip:perf/core] perf tools:   Convert parse-options.c internal functions to static tip-bot for Josh Poimboeuf <tipbot@zytor.com> - 2015-12-18 10:00 +0100
    [PATCH v3 01/17] perf build: Remove unnecessary line in Makefile.feature Josh Poimboeuf <jpoimboe@redhat.com> - 2015-12-14 05:30 +0100
      Re: [PATCH v3 01/17] perf build: Remove unnecessary line in  Makefile.feature Jiri Olsa <jolsa@redhat.com> - 2015-12-14 11:10 +0100
        Re: [PATCH v3 01/17] perf build: Remove unnecessary line in  Makefile.feature Arnaldo Carvalho de Melo <acme@kernel.org> - 2015-12-14 16:20 +0100
      [tip:perf/core] perf build:   Remove unnecessary line in Makefile.feature tip-bot for Josh Poimboeuf <tipbot@zytor.com> - 2015-12-18 09:50 +0100
    [PATCH v3 17/17] tools subcmd: Rename subcmd header include guards Josh Poimboeuf <jpoimboe@redhat.com> - 2015-12-14 05:30 +0100
    [PATCH v3 15/17] perf: Finalize subcmd independence Josh Poimboeuf <jpoimboe@redhat.com> - 2015-12-14 05:30 +0100
      Re: [PATCH v3 15/17] perf: Finalize subcmd independence Jiri Olsa <jolsa@redhat.com> - 2015-12-14 11:40 +0100
        Re: [PATCH v3 15/17] perf: Finalize subcmd independence Josh Poimboeuf <jpoimboe@redhat.com> - 2015-12-14 15:20 +0100
          Re: [PATCH v3 15/17] perf: Finalize subcmd independence Jiri Olsa <jolsa@redhat.com> - 2015-12-14 16:30 +0100
          Re: [PATCH v3 15/17] perf: Finalize subcmd independence Arnaldo Carvalho de Melo <acme@kernel.org> - 2015-12-14 17:00 +0100
    [PATCH v3 11/17] perf: Remove 'perf' from subcmd function and variable names Josh Poimboeuf <jpoimboe@redhat.com> - 2015-12-14 05:30 +0100
    [PATCH v3 16/17] perf subcmd: Create subcmd library Josh Poimboeuf <jpoimboe@redhat.com> - 2015-12-14 05:30 +0100
    [PATCH v3 04/17] perf test: remove tarpkg at end of test Josh Poimboeuf <jpoimboe@redhat.com> - 2015-12-14 05:30 +0100
      [tip:perf/core] perf test: Remove tarpkg at end of test tip-bot for Josh Poimboeuf <tipbot@zytor.com> - 2015-12-18 09:50 +0100
    [PATCH v3 14/17] perf: Remove subcmd dependencies on strbuf Josh Poimboeuf <jpoimboe@redhat.com> - 2015-12-14 05:30 +0100
      Re: [PATCH v3 14/17] perf: Remove subcmd dependencies on strbuf Jiri Olsa <jolsa@redhat.com> - 2015-12-14 11:30 +0100
        Re: [PATCH v3 14/17] perf: Remove subcmd dependencies on strbuf Josh Poimboeuf <jpoimboe@redhat.com> - 2015-12-14 15:20 +0100
      Re: [PATCH v3 14/17] perf: Remove subcmd dependencies on strbuf Arnaldo Carvalho de Melo <acme@kernel.org> - 2015-12-14 16:50 +0100
        Re: [PATCH v3 14/17] perf: Remove subcmd dependencies on strbuf Josh Poimboeuf <jpoimboe@redhat.com> - 2015-12-14 17:10 +0100
          Re: [PATCH v3 14/17] perf: Remove subcmd dependencies on strbuf Arnaldo Carvalho de Melo <acme@kernel.org> - 2015-12-14 19:00 +0100
            Re: [PATCH v3 14/17] perf: Remove subcmd dependencies on strbuf Josh Poimboeuf <jpoimboe@redhat.com> - 2015-12-14 19:10 +0100

Page 3 of 3 — ← Prev page 1 2 [3]

#1290843 — [PATCH v3 14/17] perf: Remove subcmd dependencies on strbuf

Introduce and use new astrcat() and astrcatf() functions which replace
the strbuf functionality for subcmd.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 tools/perf/util/exec_cmd.c      | 27 +++++++++++++-------------
 tools/perf/util/help.c          | 14 ++++++--------
 tools/perf/util/parse-options.c | 42 +++++++++++++++++++++--------------------
 tools/perf/util/subcmd-util.h   | 24 +++++++++++++++++++++++
 4 files changed, 66 insertions(+), 41 deletions(-)
 create mode 100644 tools/perf/util/subcmd-util.h

diff --git a/tools/perf/util/exec_cmd.c b/tools/perf/util/exec_cmd.c
index 1e8d31f..d4f6dda 100644
--- a/tools/perf/util/exec_cmd.c
+++ b/tools/perf/util/exec_cmd.c
@@ -3,6 +3,7 @@
 #include "quote.h"
 
 #include <string.h>
+#include "subcmd-util.h"
 
 #define MAX_ARGS	32
 #define UNDEFINED	"UNDEFINED"
@@ -26,14 +27,14 @@ void exec_cmd_init(const char *exec_name, const char *prefix,
 
 char *system_path(const char *path)
 {
-	struct strbuf d = STRBUF_INIT;
+	char *buf = NULL;
 
 	if (is_absolute_path(path))
 		return strdup(path);
 
-	strbuf_addf(&d, "%s/%s", cfg_prefix, path);
-	path = strbuf_detach(&d, NULL);
-	return (char *)path;
+	astrcatf(&buf, "%s/%s", cfg_prefix, path);
+
+	return buf;
 }
 
 const char *extract_argv0_path(const char *argv0)
@@ -80,22 +81,22 @@ char *get_argv_exec_path(void)
 	return system_path(cfg_exec_path);
 }
 
-static void add_path(struct strbuf *out, const char *path)
+static void add_path(char **out, const char *path)
 {
 	if (path && *path) {
 		if (is_absolute_path(path))
-			strbuf_addstr(out, path);
+			astrcat(out, path);
 		else
-			strbuf_addstr(out, make_nonrelative_path(path));
+			astrcat(out, make_nonrelative_path(path));
 
-		strbuf_addch(out, PATH_SEP);
+		astrcat(out, ":");
 	}
 }
 
 void setup_path(void)
 {
 	const char *old_path = getenv("PATH");
-	struct strbuf new_path = STRBUF_INIT;
+	char *new_path = NULL;
 	char *tmp = get_argv_exec_path();
 
 	add_path(&new_path, tmp);
@@ -103,13 +104,13 @@ void setup_path(void)
 	free(tmp);
 
 	if (old_path)
-		strbuf_addstr(&new_path, old_path);
+		astrcat(&new_path, old_path);
 	else
-		strbuf_addstr(&new_path, "/usr/local/bin:/usr/bin:/bin");
+		astrcat(&new_path, "/usr/local/bin:/usr/bin:/bin");
 
-	setenv("PATH", new_path.buf, 1);
+	setenv("PATH", new_path, 1);
 
-	strbuf_release(&new_path);
+	free(new_path);
 }
 
 static const char **prepare_exec_cmd(const char **argv)
diff --git a/tools/perf/util/help.c b/tools/perf/util/help.c
index 89fc81b..303a347 100644
--- a/tools/perf/util/help.c
+++ b/tools/perf/util/help.c
@@ -2,6 +2,7 @@
 #include "../builtin.h"
 #include "exec_cmd.h"
 #include "help.h"
+#include "subcmd-util.h"
 
 void add_cmdname(struct cmdnames *cmds, const char *name, size_t len)
 {
@@ -119,8 +120,7 @@ static void list_commands_in_dir(struct cmdnames *cmds,
 	int prefix_len;
 	DIR *dir = opendir(path);
 	struct dirent *de;
-	struct strbuf buf = STRBUF_INIT;
-	int len;
+	char *buf = NULL;
 
 	if (!dir)
 		return;
@@ -128,8 +128,7 @@ static void list_commands_in_dir(struct cmdnames *cmds,
 		prefix = "perf-";
 	prefix_len = strlen(prefix);
 
-	strbuf_addf(&buf, "%s/", path);
-	len = buf.len;
+	astrcatf(&buf, "%s/", path);
 
 	while ((de = readdir(dir)) != NULL) {
 		int entlen;
@@ -137,9 +136,8 @@ static void list_commands_in_dir(struct cmdnames *cmds,
 		if (prefixcmp(de->d_name, prefix))
 			continue;
 
-		strbuf_setlen(&buf, len);
-		strbuf_addstr(&buf, de->d_name);
-		if (!is_executable(buf.buf))
+		astrcat(&buf, de->d_name);
+		if (!is_executable(buf))
 			continue;
 
 		entlen = strlen(de->d_name) - prefix_len;
@@ -149,7 +147,7 @@ static void list_commands_in_dir(struct cmdnames *cmds,
 		add_cmdname(cmds, de->d_name + prefix_len, entlen);
 	}
 	closedir(dir);
-	strbuf_release(&buf);
+	free(buf);
 }
 
 void load_command_list(const char *prefix,
diff --git a/tools/perf/util/parse-options.c b/tools/perf/util/parse-options.c
index ac49525..0189f78 100644
--- a/tools/perf/util/parse-options.c
+++ b/tools/perf/util/parse-options.c
@@ -1,4 +1,5 @@
 #include "util.h"
+#include "subcmd-util.h"
 #include "parse-options.h"
 #include "cache.h"
 #include "header.h"
@@ -7,7 +8,7 @@
 #define OPT_SHORT 1
 #define OPT_UNSET 2
 
-static struct strbuf error_buf = STRBUF_INIT;
+char *error_buf;
 
 static const char *cfg_exec_name = "UNDEFINED";
 
@@ -510,19 +511,18 @@ int parse_options_subcommand(int argc, const char **argv, const struct option *o
 
 	/* build usage string if it's not provided */
 	if (subcommands && !usagestr[0]) {
-		struct strbuf buf = STRBUF_INIT;
+		char *buf = NULL;
+
+		astrcatf(&buf, "%s %s [<options>] {", cfg_exec_name, argv[0]);
 
-		strbuf_addf(&buf, "%s %s [<options>] {",
-			    cfg_exec_name, argv[0]);
 		for (int i = 0; subcommands[i]; i++) {
 			if (i)
-				strbuf_addstr(&buf, "|");
-			strbuf_addstr(&buf, subcommands[i]);
+				astrcat(&buf, "|");
+			astrcat(&buf, subcommands[i]);
 		}
-		strbuf_addstr(&buf, "}");
+		astrcat(&buf, "}");
 
-		usagestr[0] = strdup(buf.buf);
-		strbuf_release(&buf);
+		usagestr[0] = buf;
 	}
 
 	parse_options_start(&ctx, argc, argv, flags);
@@ -547,13 +547,11 @@ int parse_options_subcommand(int argc, const char **argv, const struct option *o
 		putchar('\n');
 		exit(130);
 	default: /* PARSE_OPT_UNKNOWN */
-		if (ctx.argv[0][1] == '-') {
-			strbuf_addf(&error_buf, "unknown option `%s'",
-				    ctx.argv[0] + 2);
-		} else {
-			strbuf_addf(&error_buf, "unknown switch `%c'",
-				    *ctx.opt);
-		}
+		if (ctx.argv[0][1] == '-')
+			astrcatf(&error_buf, "unknown option `%s'",
+				 ctx.argv[0] + 2);
+		else
+			astrcatf(&error_buf, "unknown switch `%c'", *ctx.opt);
 		usage_with_options(usagestr, options);
 	}
 
@@ -736,9 +734,9 @@ static int usage_with_options_internal(const char * const *usagestr,
 
 	setup_pager();
 
-	if (strbuf_avail(&error_buf)) {
-		fprintf(stderr, "  Error: %s\n", error_buf.buf);
-		strbuf_release(&error_buf);
+	if (error_buf) {
+		fprintf(stderr, "  Error: %s\n", error_buf);
+		zfree(&error_buf);
 	}
 
 	fprintf(stderr, "\n Usage: %s\n", *usagestr++);
@@ -782,11 +780,15 @@ void usage_with_options_msg(const char * const *usagestr,
 			    const struct option *opts, const char *fmt, ...)
 {
 	va_list ap;
+	char *tmp = error_buf;
 
 	va_start(ap, fmt);
-	strbuf_addv(&error_buf, fmt, ap);
+	if (vasprintf(&error_buf, fmt, ap) == -1)
+		die("vasprintf failed");
 	va_end(ap);
 
+	free(tmp);
+
 	usage_with_options_internal(usagestr, opts, 0, NULL);
 	exit(129);
 }
diff --git a/tools/perf/util/subcmd-util.h b/tools/perf/util/subcmd-util.h
new file mode 100644
index 0000000..98fb9f9
--- /dev/null
+++ b/tools/perf/util/subcmd-util.h
@@ -0,0 +1,24 @@
+#ifndef __PERF_SUBCMD_UTIL_H
+#define __PERF_SUBCMD_UTIL_H
+
+#include <stdio.h>
+
+#define astrcatf(out, fmt, ...)						\
+({									\
+	char *tmp = *(out);						\
+	if (asprintf((out), "%s" fmt, tmp ?: "", ## __VA_ARGS__) == -1)	\
+		die("asprintf failed");					\
+	free(tmp);							\
+})
+
+static inline void astrcat(char **out, const char *add)
+{
+	char *tmp = *out;
+
+	if (asprintf(out, "%s%s", tmp ?: "", add) == -1)
+		die("asprintf failed");
+
+	free(tmp);
+}
+
+#endif /* __PERF_SUBCMD_UTIL_H */
-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1291070 — Re: [PATCH v3 14/17] perf: Remove subcmd dependencies on strbuf

On Sun, Dec 13, 2015 at 10:18:14PM -0600, Josh Poimboeuf wrote:
> Introduce and use new astrcat() and astrcatf() functions which replace
> the strbuf functionality for subcmd.

hum, would it be better then to put strbuf into lib as well?

jirka
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1291224 — Re: [PATCH v3 14/17] perf: Remove subcmd dependencies on strbuf

On Mon, Dec 14, 2015 at 11:25:51AM +0100, Jiri Olsa wrote:
> On Sun, Dec 13, 2015 at 10:18:14PM -0600, Josh Poimboeuf wrote:
> > Introduce and use new astrcat() and astrcatf() functions which replace
> > the strbuf functionality for subcmd.
> 
> hum, would it be better then to put strbuf into lib as well?

Based on the v2 discussion, I think we don't want to move it to lib,
because it's also used by perf, and it's outside the scope of 'subcmd'.

But we could certainly duplicate it for internal use by the library.

Arnaldo, what do you think?

-- 
Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1291298 — Re: [PATCH v3 14/17] perf: Remove subcmd dependencies on strbuf

Em Sun, Dec 13, 2015 at 10:18:14PM -0600, Josh Poimboeuf escreveu:
> Introduce and use new astrcat() and astrcatf() functions which replace
> the strbuf functionality for subcmd.

<SNIP>
 
> diff --git a/tools/perf/util/subcmd-util.h b/tools/perf/util/subcmd-util.h
> new file mode 100644
> index 0000000..98fb9f9
> --- /dev/null
> +++ b/tools/perf/util/subcmd-util.h
> @@ -0,0 +1,24 @@
> +#ifndef __PERF_SUBCMD_UTIL_H
> +#define __PERF_SUBCMD_UTIL_H
> +
> +#include <stdio.h>
> +
> +#define astrcatf(out, fmt, ...)						\
> +({									\
> +	char *tmp = *(out);						\
> +	if (asprintf((out), "%s" fmt, tmp ?: "", ## __VA_ARGS__) == -1)	\
> +		die("asprintf failed");					\
> +	free(tmp);							\
> +})

Hey, don't add die() calls, please.

> +
> +static inline void astrcat(char **out, const char *add)
> +{
> +	char *tmp = *out;
> +
> +	if (asprintf(out, "%s%s", tmp ?: "", add) == -1)
> +		die("asprintf failed");
> +
> +	free(tmp);

Ditto.

And I think that this should go into tools/include/string.h and
tools/lib/string.c, no?

We should try to look at the kernel and try to follow naming, semantics,
etc as much as possible. The kernel doesn't have a astrcat, just
kasprintf() (that is in linux/kernel.h, perhaps because in userland
asprintf is in stdio.h, not in string.h) , wonder how something like
astrcat is done there... Doing some research now.

- Arnaldo

> +}
> +
> +#endif /* __PERF_SUBCMD_UTIL_H */
> -- 
> 2.4.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1291313 — Re: [PATCH v3 14/17] perf: Remove subcmd dependencies on strbuf

On Mon, Dec 14, 2015 at 12:44:21PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Sun, Dec 13, 2015 at 10:18:14PM -0600, Josh Poimboeuf escreveu:
> > Introduce and use new astrcat() and astrcatf() functions which replace
> > the strbuf functionality for subcmd.
> 
> <SNIP>
>  
> > diff --git a/tools/perf/util/subcmd-util.h b/tools/perf/util/subcmd-util.h
> > new file mode 100644
> > index 0000000..98fb9f9
> > --- /dev/null
> > +++ b/tools/perf/util/subcmd-util.h
> > @@ -0,0 +1,24 @@
> > +#ifndef __PERF_SUBCMD_UTIL_H
> > +#define __PERF_SUBCMD_UTIL_H
> > +
> > +#include <stdio.h>
> > +
> > +#define astrcatf(out, fmt, ...)						\
> > +({									\
> > +	char *tmp = *(out);						\
> > +	if (asprintf((out), "%s" fmt, tmp ?: "", ## __VA_ARGS__) == -1)	\
> > +		die("asprintf failed");					\
> > +	free(tmp);							\
> > +})
> 
> Hey, don't add die() calls, please.
> 
> > +
> > +static inline void astrcat(char **out, const char *add)
> > +{
> > +	char *tmp = *out;
> > +
> > +	if (asprintf(out, "%s%s", tmp ?: "", add) == -1)
> > +		die("asprintf failed");
> > +
> > +	free(tmp);
> 
> Ditto.

This replaces strbuf, which also calls die() when allocations fail.  So
this duplicates the existing die-on-allocation-error functionality and
is nothing "new" from my perspective.

Do you want me to change all the callers (and callers' callers, etc) of
these functions to check for errors?

> And I think that this should go into tools/include/string.h and
> tools/lib/string.c, no?

If these functions simply duplicate some of strbuf's functionality and
they aren't used outside of libsubcmd then I don't see any reason to do
that.

> We should try to look at the kernel and try to follow naming, semantics,
> etc as much as possible. The kernel doesn't have a astrcat, just
> kasprintf() (that is in linux/kernel.h, perhaps because in userland
> asprintf is in stdio.h, not in string.h) , wonder how something like
> astrcat is done there... Doing some research now.

Ok.

-- 
Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1291399 — Re: [PATCH v3 14/17] perf: Remove subcmd dependencies on strbuf

Em Mon, Dec 14, 2015 at 10:05:37AM -0600, Josh Poimboeuf escreveu:
> On Mon, Dec 14, 2015 at 12:44:21PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Sun, Dec 13, 2015 at 10:18:14PM -0600, Josh Poimboeuf escreveu:
> > > Introduce and use new astrcat() and astrcatf() functions which replace
> > > the strbuf functionality for subcmd.
> > 
> > <SNIP>
> >  
> > > diff --git a/tools/perf/util/subcmd-util.h b/tools/perf/util/subcmd-util.h
> > > new file mode 100644
> > > index 0000000..98fb9f9
> > > --- /dev/null
> > > +++ b/tools/perf/util/subcmd-util.h
> > > @@ -0,0 +1,24 @@
> > > +#ifndef __PERF_SUBCMD_UTIL_H
> > > +#define __PERF_SUBCMD_UTIL_H
> > > +
> > > +#include <stdio.h>
> > > +
> > > +#define astrcatf(out, fmt, ...)						\
> > > +({									\
> > > +	char *tmp = *(out);						\
> > > +	if (asprintf((out), "%s" fmt, tmp ?: "", ## __VA_ARGS__) == -1)	\
> > > +		die("asprintf failed");					\
> > > +	free(tmp);							\
> > > +})
> > 
> > Hey, don't add die() calls, please.
> > 
> > > +
> > > +static inline void astrcat(char **out, const char *add)
> > > +{
> > > +	char *tmp = *out;
> > > +
> > > +	if (asprintf(out, "%s%s", tmp ?: "", add) == -1)
> > > +		die("asprintf failed");
> > > +
> > > +	free(tmp);
> > 
> > Ditto.
> 
> This replaces strbuf, which also calls die() when allocations fail.  So
> this duplicates the existing die-on-allocation-error functionality and
> is nothing "new" from my perspective.
> 
> Do you want me to change all the callers (and callers' callers, etc) of
> these functions to check for errors?

Fair enough, we could do it later, but yeah, ultimately we should call
all die() calls.
 
> > And I think that this should go into tools/include/string.h and
> > tools/lib/string.c, no?
> 
> If these functions simply duplicate some of strbuf's functionality and
> they aren't used outside of libsubcmd then I don't see any reason to do
> that.

Well, we might as well follow that principle, i.e. as soon as there are
more users, we move it.

But removing strbuf and using something already in libc or something
equal or slighly similar to what is in the kernel is something I like to
have in place.
 
> > We should try to look at the kernel and try to follow naming, semantics,
> > etc as much as possible. The kernel doesn't have a astrcat, just
> > kasprintf() (that is in linux/kernel.h, perhaps because in userland
> > asprintf is in stdio.h, not in string.h) , wonder how something like
> > astrcat is done there... Doing some research now.
> 
> Ok.
> 
> -- 
> Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1291406 — Re: [PATCH v3 14/17] perf: Remove subcmd dependencies on strbuf

On Mon, Dec 14, 2015 at 02:50:12PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, Dec 14, 2015 at 10:05:37AM -0600, Josh Poimboeuf escreveu:
> > On Mon, Dec 14, 2015 at 12:44:21PM -0300, Arnaldo Carvalho de Melo wrote:
> > > Em Sun, Dec 13, 2015 at 10:18:14PM -0600, Josh Poimboeuf escreveu:
> > > > Introduce and use new astrcat() and astrcatf() functions which replace
> > > > the strbuf functionality for subcmd.
> > > 
> > > <SNIP>
> > >  
> > > > diff --git a/tools/perf/util/subcmd-util.h b/tools/perf/util/subcmd-util.h
> > > > new file mode 100644
> > > > index 0000000..98fb9f9
> > > > --- /dev/null
> > > > +++ b/tools/perf/util/subcmd-util.h
> > > > @@ -0,0 +1,24 @@
> > > > +#ifndef __PERF_SUBCMD_UTIL_H
> > > > +#define __PERF_SUBCMD_UTIL_H
> > > > +
> > > > +#include <stdio.h>
> > > > +
> > > > +#define astrcatf(out, fmt, ...)						\
> > > > +({									\
> > > > +	char *tmp = *(out);						\
> > > > +	if (asprintf((out), "%s" fmt, tmp ?: "", ## __VA_ARGS__) == -1)	\
> > > > +		die("asprintf failed");					\
> > > > +	free(tmp);							\
> > > > +})
> > > 
> > > Hey, don't add die() calls, please.
> > > 
> > > > +
> > > > +static inline void astrcat(char **out, const char *add)
> > > > +{
> > > > +	char *tmp = *out;
> > > > +
> > > > +	if (asprintf(out, "%s%s", tmp ?: "", add) == -1)
> > > > +		die("asprintf failed");
> > > > +
> > > > +	free(tmp);
> > > 
> > > Ditto.
> > 
> > This replaces strbuf, which also calls die() when allocations fail.  So
> > this duplicates the existing die-on-allocation-error functionality and
> > is nothing "new" from my perspective.
> > 
> > Do you want me to change all the callers (and callers' callers, etc) of
> > these functions to check for errors?
> 
> Fair enough, we could do it later, but yeah, ultimately we should call
> all die() calls.
>  
> > > And I think that this should go into tools/include/string.h and
> > > tools/lib/string.c, no?
> > 
> > If these functions simply duplicate some of strbuf's functionality and
> > they aren't used outside of libsubcmd then I don't see any reason to do
> > that.
> 
> Well, we might as well follow that principle, i.e. as soon as there are
> more users, we move it.
> 
> But removing strbuf and using something already in libc or something
> equal or slighly similar to what is in the kernel is something I like to
> have in place.

Ok, so just to clarify, is this patch ok in it's current state or is
there anything you'd like me to change for v4?  I didn't see anything
similar to astrcat() or astrcatf() in glibc or in the kernel.

> > > We should try to look at the kernel and try to follow naming, semantics,
> > > etc as much as possible. The kernel doesn't have a astrcat, just
> > > kasprintf() (that is in linux/kernel.h, perhaps because in userland
> > > asprintf is in stdio.h, not in string.h) , wonder how something like
> > > astrcat is done there... Doing some research now.
> > 
> > Ok.

-- 
Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Page 3 of 3 — ← Prev page 1 2 [3]

Back to top | Article view | linux.kernel

csiph-web