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

[Arnaldo Carvalho de Melo <acme@kernel.org>]

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