| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4F3334F7.70809@gmail.com>
Date: Wed, 08 Feb 2012 19:52:39 -0700
From: David Ahern <dsahern@...il.com>
To: Namhyung Kim <namhyung@...il.com>
CC: acme@...stprotocols.net, linux-kernel@...r.kernel.org,
mingo@...e.hu, peterz@...radead.org, fweisbec@...il.com,
paulus@...ba.org, tglx@...utronix.de
Subject: Re: [PATCH] perf tools: Allow multiple threads or processes in record,
stat, top
On 02/08/2012 06:19 PM, Namhyung Kim wrote:
>> diff --git a/tools/perf/util/thread_map.c b/tools/perf/util/thread_map.c
>> index 3d4b6c5..e793c16 100644
>> --- a/tools/perf/util/thread_map.c
>> +++ b/tools/perf/util/thread_map.c
>> @@ -6,7 +6,10 @@
>> #include <sys/types.h>
>> #include <sys/stat.h>
>> #include <unistd.h>
>> +#include <ctype.h>
>
> I was trying to remove ctype.h, you might use util.h here.
Right, knew that. But, in this case I am adding a call to isdigit which
means a direct dependency on ctype.h. I would prefer a direct
relationship versus an indirect via util.h
>
>
>> +#include <string.h>
>> #include "thread_map.h"
>> +#include "debug.h"
>>
>> /* Skip "." and ".." directories */
>> static int filter(const struct dirent *dir)
>> @@ -152,6 +155,155 @@ struct thread_map *thread_map__new(pid_t pid, pid_t tid, uid_t uid)
>> return thread_map__new_by_tid(tid);
>> }
>>
>> +
>> +static pid_t get_next_task_id(char **str)
>> +{
>> + char *p, *pend;
>> + pid_t ret = -1;
>> + bool adv_pend = false;
>> +
>> + pend = p = *str;
>> +
>> + /* empty strings */
>> + if (*pend == '\0')
>> + return -1;
>> +
>> + /* only expecting digits separated by commas */
>> + while (isdigit(*pend))
>> + ++pend;
>> +
>> + if (*pend == ',') {
>> + *pend = '\0';
>> + adv_pend = true;
>> + /* catch strings ending in a comma - like '1234,' */
>> + if (*(pend+1) == '\0') {
>> + ui__error("task id strings should not end in a comma\n");
>> + goto out;
>> + }
>> + }
>> +
>> + /* only convert if all characters are valid */
>> + if (*pend == '\0') {
>> + ret = atoi(p);
>> + if (adv_pend)
>> + pend++;
>> + *str = pend;
>> + }
>> +
>> +out:
>> + return ret;
>> +}
>> +
>> +static struct thread_map *thread_map__new_by_pid_str(const char *pid_str)
>> +{
>> + struct thread_map *threads = NULL;
>> + char name[256];
>> + int items, total_tasks = 0;
>> + struct dirent **namelist = NULL;
>> + int i, j = 0;
>> + char *ostr, *str;
>> + pid_t pid;
>> +
>> + ostr = strdup(pid_str);
>> + if (!ostr)
>> + return NULL;
>> + str = ostr;
>> +
>> + while (*str) {
>> + pid = get_next_task_id(&str);
>> + if (pid< 0) {
>> + free(threads);
>> + threads = NULL;
>> + break;
>> + }
>> +
>> + sprintf(name, "/proc/%d/task", pid);
>> + items = scandir(name,&namelist, filter, NULL);
>> + if (items<= 0)
>> + break;
>> +
>> + total_tasks += items;
>> + if (threads)
>> + threads = realloc(threads,
>> + sizeof(*threads) + sizeof(pid_t)*total_tasks);
>> + else
>> + threads = malloc(sizeof(*threads) + sizeof(pid_t)*items);
>> +
>> + if (threads) {
>> + for (i = 0; i< items; i++)
>> + threads->map[j++] = atoi(namelist[i]->d_name);
>> + threads->nr = total_tasks;
>> + }
>> +
>> + for (i = 0; i< items; i++)
>> + free(namelist[i]);
>> + free(namelist);
>> +
>> + if (!threads)
>> + break;
>> + }
>> +
>> + free(ostr);
>> +
>> + return threads;
>> +}
>> +
>> +static struct thread_map *thread_map__new_by_tid_str(const char *tid_str)
>> +{
>> + struct thread_map *threads = NULL;
>> + char *str;
>> + int ntasks = 0;
>> + pid_t tid;
>> +
>> + /* perf-stat expects threads to be generated even if tid not given */
>> + if (!tid_str) {
>> + threads = malloc(sizeof(*threads) + sizeof(pid_t));
>> + threads->map[1] = -1;
>> + threads->nr = 1;
>> + return threads;
>> + }
>> +
>> + str = strdup(tid_str);
>> + if (!str)
>> + return NULL;
>> +
>> + while (*str) {
>> + tid = get_next_task_id(&str);
>> + if (tid < 0) {
>> + free(threads);
>> + threads = NULL;
>> + break;
>> + }
>> +
>> + ntasks++;
>> + if (threads)
>> + threads = realloc(threads,
>> + sizeof(*threads) + sizeof(pid_t) * ntasks);
>> + else
>> + threads = malloc(sizeof(*threads) + sizeof(pid_t));
>> +
>> + if (threads == NULL)
>> + break;
>> +
>> + threads->map[ntasks-1] = tid;
>> + threads->nr = ntasks;
>> + }
>> +
>> + return threads;
>> +}
>> +
>
> This will ends up being a code duplication. How about something like below?
>
> while (*str) {
> tid = get_next_task_id(&str);
> if (tid < 0)
> goto out_err;
>
> tmp = thread_map__new_by_tid(tid); /* and for pid too */
> if (tmp == NULL)
> goto out_err;
>
> threads = thread_map__merge(threads, tmp); /* should be implemented */
> if (threads == NULL)
> break;
> }
>
The pid and tid functions are implemented differently. The commonality
is parsing a CSV string in a while loop.
>
>> +struct thread_map *thread_map__new_str(const char *pid, const char *tid,
>> + uid_t uid)
>> +{
>> + if (pid)
>> + return thread_map__new_by_pid_str(pid);
>> +
>> + if (!tid&& uid != UINT_MAX)
>> + return thread_map__new_by_uid(uid);
>> +
>> + return thread_map__new_by_tid_str(tid);
>> +}
>> +
>> void thread_map__delete(struct thread_map *threads)
>> {
>> free(threads);
> [snip]
>> diff --git a/tools/perf/util/usage.c b/tools/perf/util/usage.c
>> index d0c0139..1240bd6 100644
>> --- a/tools/perf/util/usage.c
>> +++ b/tools/perf/util/usage.c
>> @@ -83,7 +83,7 @@ void warning(const char *warn, ...)
>> va_end(params);
>> }
>>
>> -uid_t parse_target_uid(const char *str, pid_t tid, pid_t pid)
>> +uid_t parse_target_uid(const char *str, const char *tid, const char *pid)
>> {
>> struct passwd pwd, *result;
>> char buf[1024];
>> @@ -91,8 +91,8 @@ uid_t parse_target_uid(const char *str, pid_t tid, pid_t pid)
>> if (str == NULL)
>> return UINT_MAX;
>>
>> - /* CPU and PID are mutually exclusive */
>> - if (tid > 0 || pid > 0) {
>> + /* UUID and PID are mutually exclusive */
>
> s/UUID/UID/ ?
Yes. I was trying to fix the CPU typo when the function was created.
Will remove the extra 'U'.
David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists