Re: [PATCH perf/core v5 02/15] perf-buildid-cache: Use path/to/bin/buildid/elf instead of path/to/bin/buildid

[Masami Hiramatsu]

On Thu, 28 Apr 2016 10:22:23 +0900
Namhyung Kim  wrote:

> Hi Masami,
> 
> On Thu, Apr 28, 2016 at 03:37:23AM +0900, Masami Hiramatsu wrote:
> > From: Masami Hiramatsu 
> > 
> > Use path/to/bin/buildid/elf instead of path/to/bin/buildid
> > to store corresponding elf binary.
> > This also stores vdso in buildid/vdso, kallsyms in buildid/kallsyms.
> > 
> > Note that the existing caches are not updated until user adds
> > or updates the cache. Anyway, if there is the old style build-id
> > cache it falls back to use it. (IOW, it is backward compatible)
> > 
> > Signed-off-by: Masami Hiramatsu 
> > Signed-off-by: Masami Hiramatsu 
> > ---
> >  Changes in v5:
> >   - Support old style buildid caches.
> > ---
> >  tools/perf/util/build-id.c |   80 
> > ++--
> >  tools/perf/util/dso.h  |5 +++
> >  tools/perf/util/symbol.c   |   15 ++--
> >  3 files changed, 76 insertions(+), 24 deletions(-)
> > 
> > diff --git a/tools/perf/util/build-id.c b/tools/perf/util/build-id.c
> > index b6ecf87..b035483 100644
> > --- a/tools/perf/util/build-id.c
> > +++ b/tools/perf/util/build-id.c
> > @@ -144,7 +144,8 @@ static int asnprintf(char **strp, size_t size, const 
> > char *fmt, ...)
> > return ret;
> >  }
> >  
> > -static char *build_id__filename(const char *sbuild_id, char *bf, size_t 
> > size)
> > +static char *build_id_cache__linkname(const char *sbuild_id, char *bf,
> > + size_t size)
> >  {
> > char *tmp = bf;
> > int ret = asnprintf(, size, "%s/.build-id/%.2s/%s", buildid_dir,
> > @@ -154,15 +155,45 @@ static char *build_id__filename(const char 
> > *sbuild_id, char *bf, size_t size)
> > return bf;
> >  }
> >  
> > +static bool __is_regular_file(const char *pathname)
> > +{
> > +   struct stat sb;
> > +   return stat(pathname, ) == 0 && S_ISREG(sb.st_mode);
> > +}
> 
> It looks like that the is_regular_file() is already in the
> util/util.c

Nice catch!!
OK, it should be used.

Thanks!

-- 
Masami Hiramatsu 

Re: [PATCH perf/core v5 02/15] perf-buildid-cache: Use path/to/bin/buildid/elf instead of path/to/bin/buildid

[Masami Hiramatsu]

On Thu, 28 Apr 2016 10:22:23 +0900
Namhyung Kim  wrote:

> Hi Masami,
> 
> On Thu, Apr 28, 2016 at 03:37:23AM +0900, Masami Hiramatsu wrote:
> > From: Masami Hiramatsu 
> > 
> > Use path/to/bin/buildid/elf instead of path/to/bin/buildid
> > to store corresponding elf binary.
> > This also stores vdso in buildid/vdso, kallsyms in buildid/kallsyms.
> > 
> > Note that the existing caches are not updated until user adds
> > or updates the cache. Anyway, if there is the old style build-id
> > cache it falls back to use it. (IOW, it is backward compatible)
> > 
> > Signed-off-by: Masami Hiramatsu 
> > Signed-off-by: Masami Hiramatsu 
> > ---
> >  Changes in v5:
> >   - Support old style buildid caches.
> > ---
> >  tools/perf/util/build-id.c |   80 
> > ++--
> >  tools/perf/util/dso.h  |5 +++
> >  tools/perf/util/symbol.c   |   15 ++--
> >  3 files changed, 76 insertions(+), 24 deletions(-)
> > 
> > diff --git a/tools/perf/util/build-id.c b/tools/perf/util/build-id.c
> > index b6ecf87..b035483 100644
> > --- a/tools/perf/util/build-id.c
> > +++ b/tools/perf/util/build-id.c
> > @@ -144,7 +144,8 @@ static int asnprintf(char **strp, size_t size, const 
> > char *fmt, ...)
> > return ret;
> >  }
> >  
> > -static char *build_id__filename(const char *sbuild_id, char *bf, size_t 
> > size)
> > +static char *build_id_cache__linkname(const char *sbuild_id, char *bf,
> > + size_t size)
> >  {
> > char *tmp = bf;
> > int ret = asnprintf(, size, "%s/.build-id/%.2s/%s", buildid_dir,
> > @@ -154,15 +155,45 @@ static char *build_id__filename(const char 
> > *sbuild_id, char *bf, size_t size)
> > return bf;
> >  }
> >  
> > +static bool __is_regular_file(const char *pathname)
> > +{
> > +   struct stat sb;
> > +   return stat(pathname, ) == 0 && S_ISREG(sb.st_mode);
> > +}
> 
> It looks like that the is_regular_file() is already in the
> util/util.c

Nice catch!!
OK, it should be used.

Thanks!

-- 
Masami Hiramatsu 

Re: [PATCH perf/core v5 02/15] perf-buildid-cache: Use path/to/bin/buildid/elf instead of path/to/bin/buildid

[Masami Hiramatsu]

On Wed, 27 Apr 2016 18:12:32 -0300
Arnaldo Carvalho de Melo  wrote:

> Em Thu, Apr 28, 2016 at 03:37:23AM +0900, Masami Hiramatsu escreveu:
> > From: Masami Hiramatsu 
> > 
> > Use path/to/bin/buildid/elf instead of path/to/bin/buildid
> > to store corresponding elf binary.
> > This also stores vdso in buildid/vdso, kallsyms in buildid/kallsyms.
> > 
> > Note that the existing caches are not updated until user adds
> > or updates the cache. Anyway, if there is the old style build-id
> > cache it falls back to use it. (IOW, it is backward compatible)
> 
> I reworded this to:
> 
> "If there is old style build-id cache content, i.e. links to
> path/to/bin/buildid/aa/bbccdd, it will still be accessible, IOW, this is
> backard compatible."
> 
> This part "existing caches are not updated until the user updates de
> cache" sounds confusing.

Ah, I see. Thank you!

> 
> Anyway, applied.
> 
> - Arnaldo


-- 
Masami Hiramatsu 

Re: [PATCH perf/core v5 02/15] perf-buildid-cache: Use path/to/bin/buildid/elf instead of path/to/bin/buildid

[Masami Hiramatsu]

On Wed, 27 Apr 2016 18:12:32 -0300
Arnaldo Carvalho de Melo  wrote:

> Em Thu, Apr 28, 2016 at 03:37:23AM +0900, Masami Hiramatsu escreveu:
> > From: Masami Hiramatsu 
> > 
> > Use path/to/bin/buildid/elf instead of path/to/bin/buildid
> > to store corresponding elf binary.
> > This also stores vdso in buildid/vdso, kallsyms in buildid/kallsyms.
> > 
> > Note that the existing caches are not updated until user adds
> > or updates the cache. Anyway, if there is the old style build-id
> > cache it falls back to use it. (IOW, it is backward compatible)
> 
> I reworded this to:
> 
> "If there is old style build-id cache content, i.e. links to
> path/to/bin/buildid/aa/bbccdd, it will still be accessible, IOW, this is
> backard compatible."
> 
> This part "existing caches are not updated until the user updates de
> cache" sounds confusing.

Ah, I see. Thank you!

> 
> Anyway, applied.
> 
> - Arnaldo


-- 
Masami Hiramatsu 

Re: [PATCH perf/core v5 02/15] perf-buildid-cache: Use path/to/bin/buildid/elf instead of path/to/bin/buildid

[Masami Hiramatsu]

On Wed, 27 Apr 2016 18:23:50 -0300
Arnaldo Carvalho de Melo  wrote:

> Em Thu, Apr 28, 2016 at 03:37:23AM +0900, Masami Hiramatsu escreveu:
> > From: Masami Hiramatsu 
> > 
> > Use path/to/bin/buildid/elf instead of path/to/bin/buildid
> > to store corresponding elf binary.
> > This also stores vdso in buildid/vdso, kallsyms in buildid/kallsyms.
> > 
> > Note that the existing caches are not updated until user adds
> > or updates the cache. Anyway, if there is the old style build-id
> > cache it falls back to use it. (IOW, it is backward compatible)
> > 
> > Signed-off-by: Masami Hiramatsu 
> > Signed-off-by: Masami Hiramatsu 
> > ---
> >  Changes in v5:
> >   - Support old style buildid caches.
> 
> > +   /* Remove old style build-id cache */
> > +   if (__is_regular_file(dir_name))
> > +   if (unlink(dir_name))
> > +   goto out_free;
> 
> Shouldn't this just fail and require the user to use --force/-f or purge
> first?

No, I don't think so, because the old style build-id is useless for
probe-cache in anyway, and after applying this patch, perf can use
new one seemlessly. So let just renew it :)

Thank you,

-- 
Masami Hiramatsu 

Re: [PATCH perf/core v5 02/15] perf-buildid-cache: Use path/to/bin/buildid/elf instead of path/to/bin/buildid

[Masami Hiramatsu]

On Wed, 27 Apr 2016 18:23:50 -0300
Arnaldo Carvalho de Melo  wrote:

> Em Thu, Apr 28, 2016 at 03:37:23AM +0900, Masami Hiramatsu escreveu:
> > From: Masami Hiramatsu 
> > 
> > Use path/to/bin/buildid/elf instead of path/to/bin/buildid
> > to store corresponding elf binary.
> > This also stores vdso in buildid/vdso, kallsyms in buildid/kallsyms.
> > 
> > Note that the existing caches are not updated until user adds
> > or updates the cache. Anyway, if there is the old style build-id
> > cache it falls back to use it. (IOW, it is backward compatible)
> > 
> > Signed-off-by: Masami Hiramatsu 
> > Signed-off-by: Masami Hiramatsu 
> > ---
> >  Changes in v5:
> >   - Support old style buildid caches.
> 
> > +   /* Remove old style build-id cache */
> > +   if (__is_regular_file(dir_name))
> > +   if (unlink(dir_name))
> > +   goto out_free;
> 
> Shouldn't this just fail and require the user to use --force/-f or purge
> first?

No, I don't think so, because the old style build-id is useless for
probe-cache in anyway, and after applying this patch, perf can use
new one seemlessly. So let just renew it :)

Thank you,

-- 
Masami Hiramatsu 

Re: [PATCH perf/core v5 02/15] perf-buildid-cache: Use path/to/bin/buildid/elf instead of path/to/bin/buildid

[Namhyung Kim]

Hi Masami,

On Thu, Apr 28, 2016 at 03:37:23AM +0900, Masami Hiramatsu wrote:
> From: Masami Hiramatsu 
> 
> Use path/to/bin/buildid/elf instead of path/to/bin/buildid
> to store corresponding elf binary.
> This also stores vdso in buildid/vdso, kallsyms in buildid/kallsyms.
> 
> Note that the existing caches are not updated until user adds
> or updates the cache. Anyway, if there is the old style build-id
> cache it falls back to use it. (IOW, it is backward compatible)
> 
> Signed-off-by: Masami Hiramatsu 
> Signed-off-by: Masami Hiramatsu 
> ---
>  Changes in v5:
>   - Support old style buildid caches.
> ---
>  tools/perf/util/build-id.c |   80 
> ++--
>  tools/perf/util/dso.h  |5 +++
>  tools/perf/util/symbol.c   |   15 ++--
>  3 files changed, 76 insertions(+), 24 deletions(-)
> 
> diff --git a/tools/perf/util/build-id.c b/tools/perf/util/build-id.c
> index b6ecf87..b035483 100644
> --- a/tools/perf/util/build-id.c
> +++ b/tools/perf/util/build-id.c
> @@ -144,7 +144,8 @@ static int asnprintf(char **strp, size_t size, const char 
> *fmt, ...)
>   return ret;
>  }
>  
> -static char *build_id__filename(const char *sbuild_id, char *bf, size_t size)
> +static char *build_id_cache__linkname(const char *sbuild_id, char *bf,
> +   size_t size)
>  {
>   char *tmp = bf;
>   int ret = asnprintf(, size, "%s/.build-id/%.2s/%s", buildid_dir,
> @@ -154,15 +155,45 @@ static char *build_id__filename(const char *sbuild_id, 
> char *bf, size_t size)
>   return bf;
>  }
>  
> +static bool __is_regular_file(const char *pathname)
> +{
> + struct stat sb;
> + return stat(pathname, ) == 0 && S_ISREG(sb.st_mode);
> +}

It looks like that the is_regular_file() is already in the
util/util.c

Thanks,
Namhyung


> +
> +static const char *build_id_cache__basename(bool is_kallsyms, bool is_vdso)
> +{
> + return is_kallsyms ? "kallsyms" : (is_vdso ? "vdso" : "elf");
> +}
> +
>  char *dso__build_id_filename(const struct dso *dso, char *bf, size_t size)
>  {
> - char build_id_hex[SBUILD_ID_SIZE];
> + bool is_kallsyms = dso__is_kallsyms((struct dso *)dso);
> + bool is_vdso = dso__is_vdso((struct dso *)dso);
> + char sbuild_id[SBUILD_ID_SIZE];
> + char *linkname;
> + bool alloc = (bf == NULL);
> + int ret;
>  
>   if (!dso->has_build_id)
>   return NULL;
>  
> - build_id__sprintf(dso->build_id, sizeof(dso->build_id), build_id_hex);
> - return build_id__filename(build_id_hex, bf, size);
> + build_id__sprintf(dso->build_id, sizeof(dso->build_id), sbuild_id);
> + linkname = build_id_cache__linkname(sbuild_id, NULL, 0);
> + if (!linkname)
> + return NULL;
> +
> + /* Check if old style build_id cache */
> + if (__is_regular_file(linkname))
> + ret = asnprintf(, size, "%s", linkname);
> + else
> + ret = asnprintf(, size, "%s/%s", linkname,
> +  build_id_cache__basename(is_kallsyms, is_vdso));
> + if (ret < 0 || (!alloc && size < (unsigned int)ret))
> + bf = NULL;
> + free(linkname);
> +
> + return bf;
>  }
>  
>  bool dso__build_id_is_kmod(const struct dso *dso, char *bf, size_t size)
> @@ -341,7 +372,8 @@ void disable_buildid_cache(void)
>  }
>  
>  static char *build_id_cache__dirname_from_path(const char *name,
> -bool is_kallsyms, bool is_vdso)
> +bool is_kallsyms, bool is_vdso,
> +const char *sbuild_id)
>  {
>   char *realname = (char *)name, *filename;
>   bool slash = is_kallsyms || is_vdso;
> @@ -352,8 +384,9 @@ static char *build_id_cache__dirname_from_path(const char 
> *name,
>   return NULL;
>   }
>  
> - if (asprintf(, "%s%s%s", buildid_dir, slash ? "/" : "",
> -  is_vdso ? DSO__NAME_VDSO : realname) < 0)
> + if (asprintf(, "%s%s%s%s%s", buildid_dir, slash ? "/" : "",
> +  is_vdso ? DSO__NAME_VDSO : realname,
> +  sbuild_id ? "/" : "", sbuild_id ?: "") < 0)
>   filename = NULL;
>  
>   if (!slash)
> @@ -372,7 +405,8 @@ int build_id_cache__list_build_ids(const char *pathname,
>   int ret = 0;
>  
>   list = strlist__new(NULL, NULL);
> - dir_name = build_id_cache__dirname_from_path(pathname, false, false);
> + dir_name = build_id_cache__dirname_from_path(pathname, false, false,
> +  NULL);
>   if (!list || !dir_name) {
>   ret = -ENOMEM;
>   goto out;
> @@ -407,7 +441,7 @@ int build_id_cache__add_s(const char *sbuild_id, const 
> char *name,
>  {
>   const size_t size = PATH_MAX;
>   char *realname = NULL, *filename = NULL, *dir_name = NULL,
> -

Re: [PATCH perf/core v5 02/15] perf-buildid-cache: Use path/to/bin/buildid/elf instead of path/to/bin/buildid

[Namhyung Kim]

Hi Masami,

On Thu, Apr 28, 2016 at 03:37:23AM +0900, Masami Hiramatsu wrote:
> From: Masami Hiramatsu 
> 
> Use path/to/bin/buildid/elf instead of path/to/bin/buildid
> to store corresponding elf binary.
> This also stores vdso in buildid/vdso, kallsyms in buildid/kallsyms.
> 
> Note that the existing caches are not updated until user adds
> or updates the cache. Anyway, if there is the old style build-id
> cache it falls back to use it. (IOW, it is backward compatible)
> 
> Signed-off-by: Masami Hiramatsu 
> Signed-off-by: Masami Hiramatsu 
> ---
>  Changes in v5:
>   - Support old style buildid caches.
> ---
>  tools/perf/util/build-id.c |   80 
> ++--
>  tools/perf/util/dso.h  |5 +++
>  tools/perf/util/symbol.c   |   15 ++--
>  3 files changed, 76 insertions(+), 24 deletions(-)
> 
> diff --git a/tools/perf/util/build-id.c b/tools/perf/util/build-id.c
> index b6ecf87..b035483 100644
> --- a/tools/perf/util/build-id.c
> +++ b/tools/perf/util/build-id.c
> @@ -144,7 +144,8 @@ static int asnprintf(char **strp, size_t size, const char 
> *fmt, ...)
>   return ret;
>  }
>  
> -static char *build_id__filename(const char *sbuild_id, char *bf, size_t size)
> +static char *build_id_cache__linkname(const char *sbuild_id, char *bf,
> +   size_t size)
>  {
>   char *tmp = bf;
>   int ret = asnprintf(, size, "%s/.build-id/%.2s/%s", buildid_dir,
> @@ -154,15 +155,45 @@ static char *build_id__filename(const char *sbuild_id, 
> char *bf, size_t size)
>   return bf;
>  }
>  
> +static bool __is_regular_file(const char *pathname)
> +{
> + struct stat sb;
> + return stat(pathname, ) == 0 && S_ISREG(sb.st_mode);
> +}

It looks like that the is_regular_file() is already in the
util/util.c

Thanks,
Namhyung


> +
> +static const char *build_id_cache__basename(bool is_kallsyms, bool is_vdso)
> +{
> + return is_kallsyms ? "kallsyms" : (is_vdso ? "vdso" : "elf");
> +}
> +
>  char *dso__build_id_filename(const struct dso *dso, char *bf, size_t size)
>  {
> - char build_id_hex[SBUILD_ID_SIZE];
> + bool is_kallsyms = dso__is_kallsyms((struct dso *)dso);
> + bool is_vdso = dso__is_vdso((struct dso *)dso);
> + char sbuild_id[SBUILD_ID_SIZE];
> + char *linkname;
> + bool alloc = (bf == NULL);
> + int ret;
>  
>   if (!dso->has_build_id)
>   return NULL;
>  
> - build_id__sprintf(dso->build_id, sizeof(dso->build_id), build_id_hex);
> - return build_id__filename(build_id_hex, bf, size);
> + build_id__sprintf(dso->build_id, sizeof(dso->build_id), sbuild_id);
> + linkname = build_id_cache__linkname(sbuild_id, NULL, 0);
> + if (!linkname)
> + return NULL;
> +
> + /* Check if old style build_id cache */
> + if (__is_regular_file(linkname))
> + ret = asnprintf(, size, "%s", linkname);
> + else
> + ret = asnprintf(, size, "%s/%s", linkname,
> +  build_id_cache__basename(is_kallsyms, is_vdso));
> + if (ret < 0 || (!alloc && size < (unsigned int)ret))
> + bf = NULL;
> + free(linkname);
> +
> + return bf;
>  }
>  
>  bool dso__build_id_is_kmod(const struct dso *dso, char *bf, size_t size)
> @@ -341,7 +372,8 @@ void disable_buildid_cache(void)
>  }
>  
>  static char *build_id_cache__dirname_from_path(const char *name,
> -bool is_kallsyms, bool is_vdso)
> +bool is_kallsyms, bool is_vdso,
> +const char *sbuild_id)
>  {
>   char *realname = (char *)name, *filename;
>   bool slash = is_kallsyms || is_vdso;
> @@ -352,8 +384,9 @@ static char *build_id_cache__dirname_from_path(const char 
> *name,
>   return NULL;
>   }
>  
> - if (asprintf(, "%s%s%s", buildid_dir, slash ? "/" : "",
> -  is_vdso ? DSO__NAME_VDSO : realname) < 0)
> + if (asprintf(, "%s%s%s%s%s", buildid_dir, slash ? "/" : "",
> +  is_vdso ? DSO__NAME_VDSO : realname,
> +  sbuild_id ? "/" : "", sbuild_id ?: "") < 0)
>   filename = NULL;
>  
>   if (!slash)
> @@ -372,7 +405,8 @@ int build_id_cache__list_build_ids(const char *pathname,
>   int ret = 0;
>  
>   list = strlist__new(NULL, NULL);
> - dir_name = build_id_cache__dirname_from_path(pathname, false, false);
> + dir_name = build_id_cache__dirname_from_path(pathname, false, false,
> +  NULL);
>   if (!list || !dir_name) {
>   ret = -ENOMEM;
>   goto out;
> @@ -407,7 +441,7 @@ int build_id_cache__add_s(const char *sbuild_id, const 
> char *name,
>  {
>   const size_t size = PATH_MAX;
>   char *realname = NULL, *filename = NULL, *dir_name = NULL,
> -  *linkname = zalloc(size), *targetname, *tmp;
> +  *linkname = zalloc(size), 

Re: [PATCH perf/core v5 02/15] perf-buildid-cache: Use path/to/bin/buildid/elf instead of path/to/bin/buildid

[Arnaldo Carvalho de Melo]

Em Thu, Apr 28, 2016 at 03:37:23AM +0900, Masami Hiramatsu escreveu:
> From: Masami Hiramatsu 
> 
> Use path/to/bin/buildid/elf instead of path/to/bin/buildid
> to store corresponding elf binary.
> This also stores vdso in buildid/vdso, kallsyms in buildid/kallsyms.
> 
> Note that the existing caches are not updated until user adds
> or updates the cache. Anyway, if there is the old style build-id
> cache it falls back to use it. (IOW, it is backward compatible)
> 
> Signed-off-by: Masami Hiramatsu 
> Signed-off-by: Masami Hiramatsu 
> ---
>  Changes in v5:
>   - Support old style buildid caches.

> + /* Remove old style build-id cache */
> + if (__is_regular_file(dir_name))
> + if (unlink(dir_name))
> + goto out_free;

Shouldn't this just fail and require the user to use --force/-f or purge
first?

- Arnaldo

>   if (mkdir_p(dir_name, 0755))
>   goto out_free;
>  
> - if (asprintf(, "%s/%s", dir_name, sbuild_id) < 0) {
> + /* Save the allocated buildid dirname */
> + if (asprintf(, "%s/%s", dir_name,
> +  build_id_cache__basename(is_kallsyms, is_vdso)) < 0) {
>   filename = NULL;
>   goto out_free;
>   }
> @@ -437,7 +479,7 @@ int build_id_cache__add_s(const char *sbuild_id, const 
> char *name,
>   goto out_free;
>   }
>  
> - if (!build_id__filename(sbuild_id, linkname, size))
> + if (!build_id_cache__linkname(sbuild_id, linkname, size))
>   goto out_free;
>   tmp = strrchr(linkname, '/');
>   *tmp = '\0';
> @@ -446,10 +488,10 @@ int build_id_cache__add_s(const char *sbuild_id, const 
> char *name,
>   goto out_free;
>  
>   *tmp = '/';
> - targetname = filename + strlen(buildid_dir) - 5;
> - memcpy(targetname, "../..", 5);
> + tmp = dir_name + strlen(buildid_dir) - 5;
> + memcpy(tmp, "../..", 5);
>  
> - if (symlink(targetname, linkname) == 0)
> + if (symlink(tmp, linkname) == 0)
>   err = 0;
>  out_free:
>   if (!is_kallsyms)
> @@ -474,7 +516,7 @@ static int build_id_cache__add_b(const u8 *build_id, 
> size_t build_id_size,
>  bool build_id_cache__cached(const char *sbuild_id)
>  {
>   bool ret = false;
> - char *filename = build_id__filename(sbuild_id, NULL, 0);
> + char *filename = build_id_cache__linkname(sbuild_id, NULL, 0);
>  
>   if (filename && !access(filename, F_OK))
>   ret = true;
> @@ -493,7 +535,7 @@ int build_id_cache__remove_s(const char *sbuild_id)
>   if (filename == NULL || linkname == NULL)
>   goto out_free;
>  
> - if (!build_id__filename(sbuild_id, linkname, size))
> + if (!build_id_cache__linkname(sbuild_id, linkname, size))
>   goto out_free;
>  
>   if (access(linkname, F_OK))
> @@ -511,7 +553,7 @@ int build_id_cache__remove_s(const char *sbuild_id)
>   tmp = strrchr(linkname, '/') + 1;
>   snprintf(tmp, size - (tmp - linkname), "%s", filename);
>  
> - if (unlink(linkname))
> + if (rm_rf(linkname))
>   goto out_free;
>  
>   err = 0;
> @@ -523,7 +565,7 @@ out_free:
>  
>  static int dso__cache_build_id(struct dso *dso, struct machine *machine)
>  {
> - bool is_kallsyms = dso->kernel && dso->long_name[0] != '/';
> + bool is_kallsyms = dso__is_kallsyms(dso);
>   bool is_vdso = dso__is_vdso(dso);
>   const char *name = dso->long_name;
>   char nm[PATH_MAX];
> diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
> index 0953280..76d79d0 100644
> --- a/tools/perf/util/dso.h
> +++ b/tools/perf/util/dso.h
> @@ -349,6 +349,11 @@ static inline bool dso__is_kcore(struct dso *dso)
>  dso->binary_type == DSO_BINARY_TYPE__GUEST_KCORE;
>  }
>  
> +static inline bool dso__is_kallsyms(struct dso *dso)
> +{
> + return dso->kernel && dso->long_name[0] != '/';
> +}
> +
>  void dso__free_a2l(struct dso *dso);
>  
>  enum dso_type dso__type(struct dso *dso, struct machine *machine);
> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index 415c4f6..9463c7d 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -1685,13 +1685,18 @@ static char *dso__find_kallsyms(struct dso *dso, 
> struct map *map)
>   if (!find_matching_kcore(map, path, sizeof(path)))
>   return strdup(path);
>  
> - scnprintf(path, sizeof(path), "%s/[kernel.kallsyms]/%s",
> + scnprintf(path, sizeof(path), "%s/[kernel.kallsyms]/%s/kallsyms",
> buildid_dir, sbuild_id);
> -
> + /* Try old style kallsyms cache */
>   if (access(path, F_OK)) {
> - pr_err("No kallsyms or vmlinux with build-id %s was found\n",
> -sbuild_id);
> - return NULL;
> + scnprintf(path, sizeof(path), "%s/[kernel.kallsyms]/%s",
> +   buildid_dir, sbuild_id);

Re: [PATCH perf/core v5 02/15] perf-buildid-cache: Use path/to/bin/buildid/elf instead of path/to/bin/buildid

[Arnaldo Carvalho de Melo]

Em Thu, Apr 28, 2016 at 03:37:23AM +0900, Masami Hiramatsu escreveu:
> From: Masami Hiramatsu 
> 
> Use path/to/bin/buildid/elf instead of path/to/bin/buildid
> to store corresponding elf binary.
> This also stores vdso in buildid/vdso, kallsyms in buildid/kallsyms.
> 
> Note that the existing caches are not updated until user adds
> or updates the cache. Anyway, if there is the old style build-id
> cache it falls back to use it. (IOW, it is backward compatible)
> 
> Signed-off-by: Masami Hiramatsu 
> Signed-off-by: Masami Hiramatsu 
> ---
>  Changes in v5:
>   - Support old style buildid caches.

> + /* Remove old style build-id cache */
> + if (__is_regular_file(dir_name))
> + if (unlink(dir_name))
> + goto out_free;

Shouldn't this just fail and require the user to use --force/-f or purge
first?

- Arnaldo

>   if (mkdir_p(dir_name, 0755))
>   goto out_free;
>  
> - if (asprintf(, "%s/%s", dir_name, sbuild_id) < 0) {
> + /* Save the allocated buildid dirname */
> + if (asprintf(, "%s/%s", dir_name,
> +  build_id_cache__basename(is_kallsyms, is_vdso)) < 0) {
>   filename = NULL;
>   goto out_free;
>   }
> @@ -437,7 +479,7 @@ int build_id_cache__add_s(const char *sbuild_id, const 
> char *name,
>   goto out_free;
>   }
>  
> - if (!build_id__filename(sbuild_id, linkname, size))
> + if (!build_id_cache__linkname(sbuild_id, linkname, size))
>   goto out_free;
>   tmp = strrchr(linkname, '/');
>   *tmp = '\0';
> @@ -446,10 +488,10 @@ int build_id_cache__add_s(const char *sbuild_id, const 
> char *name,
>   goto out_free;
>  
>   *tmp = '/';
> - targetname = filename + strlen(buildid_dir) - 5;
> - memcpy(targetname, "../..", 5);
> + tmp = dir_name + strlen(buildid_dir) - 5;
> + memcpy(tmp, "../..", 5);
>  
> - if (symlink(targetname, linkname) == 0)
> + if (symlink(tmp, linkname) == 0)
>   err = 0;
>  out_free:
>   if (!is_kallsyms)
> @@ -474,7 +516,7 @@ static int build_id_cache__add_b(const u8 *build_id, 
> size_t build_id_size,
>  bool build_id_cache__cached(const char *sbuild_id)
>  {
>   bool ret = false;
> - char *filename = build_id__filename(sbuild_id, NULL, 0);
> + char *filename = build_id_cache__linkname(sbuild_id, NULL, 0);
>  
>   if (filename && !access(filename, F_OK))
>   ret = true;
> @@ -493,7 +535,7 @@ int build_id_cache__remove_s(const char *sbuild_id)
>   if (filename == NULL || linkname == NULL)
>   goto out_free;
>  
> - if (!build_id__filename(sbuild_id, linkname, size))
> + if (!build_id_cache__linkname(sbuild_id, linkname, size))
>   goto out_free;
>  
>   if (access(linkname, F_OK))
> @@ -511,7 +553,7 @@ int build_id_cache__remove_s(const char *sbuild_id)
>   tmp = strrchr(linkname, '/') + 1;
>   snprintf(tmp, size - (tmp - linkname), "%s", filename);
>  
> - if (unlink(linkname))
> + if (rm_rf(linkname))
>   goto out_free;
>  
>   err = 0;
> @@ -523,7 +565,7 @@ out_free:
>  
>  static int dso__cache_build_id(struct dso *dso, struct machine *machine)
>  {
> - bool is_kallsyms = dso->kernel && dso->long_name[0] != '/';
> + bool is_kallsyms = dso__is_kallsyms(dso);
>   bool is_vdso = dso__is_vdso(dso);
>   const char *name = dso->long_name;
>   char nm[PATH_MAX];
> diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
> index 0953280..76d79d0 100644
> --- a/tools/perf/util/dso.h
> +++ b/tools/perf/util/dso.h
> @@ -349,6 +349,11 @@ static inline bool dso__is_kcore(struct dso *dso)
>  dso->binary_type == DSO_BINARY_TYPE__GUEST_KCORE;
>  }
>  
> +static inline bool dso__is_kallsyms(struct dso *dso)
> +{
> + return dso->kernel && dso->long_name[0] != '/';
> +}
> +
>  void dso__free_a2l(struct dso *dso);
>  
>  enum dso_type dso__type(struct dso *dso, struct machine *machine);
> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index 415c4f6..9463c7d 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -1685,13 +1685,18 @@ static char *dso__find_kallsyms(struct dso *dso, 
> struct map *map)
>   if (!find_matching_kcore(map, path, sizeof(path)))
>   return strdup(path);
>  
> - scnprintf(path, sizeof(path), "%s/[kernel.kallsyms]/%s",
> + scnprintf(path, sizeof(path), "%s/[kernel.kallsyms]/%s/kallsyms",
> buildid_dir, sbuild_id);
> -
> + /* Try old style kallsyms cache */
>   if (access(path, F_OK)) {
> - pr_err("No kallsyms or vmlinux with build-id %s was found\n",
> -sbuild_id);
> - return NULL;
> + scnprintf(path, sizeof(path), "%s/[kernel.kallsyms]/%s",
> +   buildid_dir, sbuild_id);
> +
> + if (access(path, F_OK)) {
> + pr_err("No 

Re: [PATCH perf/core v5 02/15] perf-buildid-cache: Use path/to/bin/buildid/elf instead of path/to/bin/buildid

[Arnaldo Carvalho de Melo]

Em Thu, Apr 28, 2016 at 03:37:23AM +0900, Masami Hiramatsu escreveu:
> From: Masami Hiramatsu 
> 
> Use path/to/bin/buildid/elf instead of path/to/bin/buildid
> to store corresponding elf binary.
> This also stores vdso in buildid/vdso, kallsyms in buildid/kallsyms.
> 
> Note that the existing caches are not updated until user adds
> or updates the cache. Anyway, if there is the old style build-id
> cache it falls back to use it. (IOW, it is backward compatible)

I reworded this to:

"If there is old style build-id cache content, i.e. links to
path/to/bin/buildid/aa/bbccdd, it will still be accessible, IOW, this is
backard compatible."

This part "existing caches are not updated until the user updates de
cache" sounds confusing.

Anyway, applied.

- Arnaldo

Re: [PATCH perf/core v5 02/15] perf-buildid-cache: Use path/to/bin/buildid/elf instead of path/to/bin/buildid

[Arnaldo Carvalho de Melo]

Em Thu, Apr 28, 2016 at 03:37:23AM +0900, Masami Hiramatsu escreveu:
> From: Masami Hiramatsu 
> 
> Use path/to/bin/buildid/elf instead of path/to/bin/buildid
> to store corresponding elf binary.
> This also stores vdso in buildid/vdso, kallsyms in buildid/kallsyms.
> 
> Note that the existing caches are not updated until user adds
> or updates the cache. Anyway, if there is the old style build-id
> cache it falls back to use it. (IOW, it is backward compatible)

I reworded this to:

"If there is old style build-id cache content, i.e. links to
path/to/bin/buildid/aa/bbccdd, it will still be accessible, IOW, this is
backard compatible."

This part "existing caches are not updated until the user updates de
cache" sounds confusing.

Anyway, applied.

- Arnaldo