* Nils Carlson ([email protected]) wrote:
> On 05/16/2011 04:23 PM, Mathieu Desnoyers wrote:
>> * Nils Carlson ([email protected]) wrote:
>>> Previously libustctl would list all pids. This patch changes this
>>> so only online pids are listed. This is done by appending on each
>>> socket name the mtime from the proc/<pid> directory. This way a
>>> socket can be checked to see if the appended mtime matches the
>>> mtime of the proc dir for the current pid, thus allowing us to
>>> distinguish between old and new socket files.
>>
>> Hi Nils,
>>
>> I'm OK with the approach. I have a few cosmetic comments below,
>>
>>>
>>> Signed-off-by: Nils Carlson<[email protected]>
>>> ---
>>> libust/tracectl.c | 9 +++-
>>> libustcomm/ustcomm.c | 150
>>> ++++++++++++++++++++++++++++++++++++++++++++-----
>>> libustcomm/ustcomm.h | 7 ++
>>> libustctl/libustctl.c | 18 +-----
>>> 4 files changed, 153 insertions(+), 31 deletions(-)
>>>
>>> diff --git a/libust/tracectl.c b/libust/tracectl.c
>>> index bc0a07c..fb9130c 100644
>>> --- a/libust/tracectl.c
>>> +++ b/libust/tracectl.c
>>> @@ -1228,12 +1228,19 @@ static struct ustcomm_sock * init_app_socket(int
>>> epoll_fd)
>>> char *dir_name, *sock_name;
>>> int result;
>>> struct ustcomm_sock *sock = NULL;
>>> + time_t mtime;
>>>
>>> dir_name = ustcomm_user_sock_dir();
>>> if (!dir_name)
>>> return NULL;
>>>
>>> - result = asprintf(&sock_name, "%s/%d", dir_name, (int)getpid());
>>> + mtime = ustcomm_pid_st_mtime(getpid());
>>> + if (!mtime) {
>>> + goto free_dir_name;
>>> + }
>>> +
>>> + result = asprintf(&sock_name, "%s/%d.%ld", dir_name,
>>> + (int)getpid(), (long)mtime);
>>> if (result< 0) {
>>> ERR("string overflow allocating socket name, "
>>> "UST thread bailing");
>>> diff --git a/libustcomm/ustcomm.c b/libustcomm/ustcomm.c
>>> index ed6d8f1..17bd9d9 100644
>>> --- a/libustcomm/ustcomm.c
>>> +++ b/libustcomm/ustcomm.c
>>> @@ -22,6 +22,7 @@
>>> #include<sys/types.h>
>>> #include<signal.h>
>>> #include<errno.h>
>>> +#include<limits.h>
>>> #include<sys/socket.h>
>>> #include<sys/un.h>
>>> #include<unistd.h>
>>> @@ -550,6 +551,125 @@ char *ustcomm_user_sock_dir(void)
>>> return sock_dir;
>>> }
>>>
>>> +static int time_and_pid_from_socket_name(char *sock_name, unsigned long
>>> *time,
>>> + pid_t *pid)
>>> +{
>>> + char *saveptr, *pid_m_time_str;
>>> + char *sock_basename = strdup(basename(sock_name));
>>> +
>>> + if (!sock_basename) {
>>> + return -1;
>>> + }
>>> +
>>> + /* This is the pid */
>>> + pid_m_time_str = strtok_r(sock_basename, ".",&saveptr);
>>> + if (!pid_m_time_str) {
>>> + goto out_err;
>>> + }
>>> +
>>> + errno = 0;
>>> + *pid = (pid_t)strtoul(pid_m_time_str, NULL, 10);
>>> + if (errno) {
>>
>> Errno is a TLS variable, while the value returned by strtoul is directly
>> accessible from registers. So I would tend to think that using errno all
>> the time will bloat our code, causing TLS accesses when not necessary.
>> And I fear that on systems lacking TLS support (such as Android), they
>> might simply use a shared variable modified concurrently across threads,
>> which could be racy. So I would prefer to use the return value of the
>> functions to trigger the error, and use the errno value only to print
>> the error message in more details. Otherwise, I'm worried that we might
>> end up either missing errors or triggering on a false-positive from the
>> wrong thread.
> Please read the man page for strtoul. Functions such as this one don't
> return anything to indicate errors, errno is the only way to check.
It returns ULONG_MAX, which is, I have to admit, a valid value, so
cannot be used to check for errors. Good point!
>
>>
>>> + goto out_err;
>>> + }
>>> +
>>> + /* This should be the time-stamp */
>>> + pid_m_time_str = strtok_r(NULL, ".",&saveptr);
>>> + if (!pid_m_time_str) {
>>> + goto out_err;
>>> + }
>>> +
>>> + errno = 0;
>>> + *time = strtoul(pid_m_time_str, NULL, 10);
>>> + if (errno) {
>>> + goto out_err;
>>> + }
>>> +
>>> + return 0;
>>> +
>>> +out_err:
>>> + free(sock_basename);
>>> + return -1;
>>> +}
>>> +
>>> +time_t ustcomm_pid_st_mtime(pid_t pid)
>>> +{
>>> + struct stat proc_stat;
>>> + char proc_name[PATH_MAX + 1];
>>> +
>>> + if (snprintf(proc_name, PATH_MAX, "/proc/%ld", (long)pid)< 0) {
>>> + return 0;
>>> + }
>>> +
>>> + if (stat(proc_name,&proc_stat)) {
>>> + return 0;
>>> + }
>>> +
>>> + return proc_stat.st_mtime;
>>> +}
>>> +
>>> +int ustcomm_is_socket_live(char *sock_name, pid_t *read_pid)
>>> +{
>>> + time_t time_from_pid;
>>> + unsigned long time_from_sock;
>>> + pid_t pid;
>>> +
>>> + if (time_and_pid_from_socket_name(sock_name,&time_from_sock,&pid)) {
>>> + return 0;
>>> + }
>>> +
>>> + if (read_pid) {
>>> + *read_pid = pid;
>>> + }
>>> +
>>> + time_from_pid = ustcomm_pid_st_mtime(pid);
>>> + if (!time_from_pid) {
>>> + return 0;
>>> + }
>>> +
>>> + if (((unsigned long)time_from_pid) == time_from_sock) {
>>
>> Please use: (unsigned long) time_from_pid (with the space) (typical in
>> kernel coding style, which we try to follow).
>
> Done, though as usual this isn't consistent with already existing code.
Yeah.. I'll probably do a cleanup at some point this summer.
>>
>>> + return 1;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int ustcomm_get_sock_name(char *dir_name, pid_t pid, char
>>> *sock_name)
>>> +{
>>> + struct dirent *dirent;
>>> + char sock_path_base[101];
>>> + int len;
>>> + DIR *dir = opendir(dir_name);
>>> +
>>> + snprintf(sock_path_base, 100, "%ld.", (long)pid);
>>> + len = strlen(sock_path_base);
>>> +
>>> + while ((dirent = readdir(dir))) {
>>> + if (!strcmp(dirent->d_name, ".") ||
>>> + !strcmp(dirent->d_name, "..") ||
>>> + !strcmp(dirent->d_name, "ust-consumer") ||
>>> + dirent->d_type == DT_DIR ||
>>> + strncmp(dirent->d_name, sock_path_base, len)) {
>>> + continue;
>>> + }
>>> +
>>> + if (ustcomm_is_socket_live(dirent->d_name, NULL)) {
>>> + printf("found %s\n", dirent->d_name);
>>
>> Should this message be only printed in verbose mode ?
>
> Darn, debug! :-) Will fix the rest.
Thanks!
Mathieu
>
> /Nils
>
--
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com
_______________________________________________
ltt-dev mailing list
[email protected]
http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev
