[ltt-dev] [UST PATCH 3/5] Restructure the ustctl_get_online_pids command

[Nils Carlson]

On Mar 29, 2011, at 5:56 PM, Mathieu Desnoyers wrote:

> * Nils Carlson (nils.carlson at ericsson.com) wrote:
>> Restructure the command to separate the pid gathering.
>> This will allow a root user to gather pid from multiple user
>> directories.
>>
>> Signed-off-by: Nils Carlson <nils.carlson at ericsson.com>
>> ---
>> libustctl/libustctl.c |   80 +++++++++++++++++++++++++++++ 
>> +------------------
>> 1 files changed, 50 insertions(+), 30 deletions(-)
>>
>> diff --git a/libustctl/libustctl.c b/libustctl/libustctl.c
>> index d57e645..2453a99 100644
>> --- a/libustctl/libustctl.c
>> +++ b/libustctl/libustctl.c
>> @@ -88,53 +88,73 @@ int ustctl_connect_pid(pid_t pid)
>> 	return sock;
>> }
>>
>> -pid_t *ustctl_get_online_pids(void)
>> +static void get_pids_in_dir(DIR *dir, pid_t **pid_list,
>> +			    unsigned int *pid_list_size)
>> {
>> 	struct dirent *dirent;
>> -	DIR *dir;
>> -	unsigned int ret_size = 1 * sizeof(pid_t), i = 0;
>
> Instead of using "sizeof(pid_t)" for ret_size base unit, we could just
> keep the array length, and multiply by sizeof(pid_t) when needed. This
> is what is done pretty much everywhere else in the lttng code.

Yep, can look into it. I agree, nicer to count elements than absolute  
size.
>
>> -
>> -	dir = opendir(SOCK_DIR);
>> -	if (!dir) {
>> -		return NULL;
>> -	}
>> -
>> -	pid_t *ret = (pid_t *) malloc(ret_size);
>> +	int i = 0;
>>
>> 	while ((dirent = readdir(dir))) {
>> 		if (!strcmp(dirent->d_name, ".") ||
>> -		    !strcmp(dirent->d_name, "..")) {
>> +		    !strcmp(dirent->d_name, "..") ||
>> +		    !strcmp(dirent->d_name, "ust-consumer") ||
>> +		    dirent->d_type == DT_DIR) {
>>
>> 			continue;
>> 		}
>>
>> -		if (dirent->d_type != DT_DIR &&
>> -		    !!strcmp(dirent->d_name, "ust-consumer")) {
>> -
>> -			sscanf(dirent->d_name, "%u", (unsigned int *) &ret[i]);
>> -			/* FIXME: Here we previously called pid_is_online, which
>> -			 * always returned 1, now I replaced it with just 1.
>> -			 * We need to figure out an intelligent way of solving
>> -			 * this, maybe connect-disconnect.
>> -			 */
>> -			if (1) {
>> -				ret_size += sizeof(pid_t);
>> -				ret = (pid_t *) realloc(ret, ret_size);
>> -				++i;
>> -			}
>> +		sscanf(dirent->d_name, "%u",
>> +		       (unsigned int *) &(*pid_list)[i]);
>
> I think there is a bug with this cast if the pid_list target is ever
> expected to be a 64-bit unsigned long. I would recommend putting the
> result in a local variable instead, and then copying it to the  
> pid_list
> element.

Good catch, have had some interesting cross-arch issues with stuff  
like this.
>
>> +		/* FIXME: Here we previously called pid_is_online, which
>> +		 * always returned 1, now I replaced it with just 1.
>> +		 * We need to figure out an intelligent way of solving
>> +		 * this, maybe connect-disconnect.
>> +		 */
>
> Yep, trying to "ping" the application through its listener socket  
> seems
> appropriate. This will let us make sure we have the appropriate
> credentials to talk to the application (the trace control app is  
> either
> the same user, or in the "tracing" group, or "root").

Well, the idea with the user only directories is to guarantee the  
access rights.
Together with Mathews patch checking /proc we should be well covered.

I will also make applications clean up their old sockets in the  
library destructor,
this will make things cleaner. Then the only time we will see an old  
socket is
when the same user has got the exact same pid and the old app is  
traceable
and the new one is not. We could possibly catch this as well by making  
libustctl
clean things up, removing dead sockets during the directory scan.

/Nils



>
> Thanks,
>
> Mathieu
>
>> +		if (1) {
>> +			*pid_list_size += sizeof(pid_t);
>> +			*pid_list = (pid_t *)realloc(*pid_list, *pid_list_size);
>> +			i++;
>> 		}
>> 	}
>>
>> -	ret[i] = 0; /* Array end */
>> +	(*pid_list)[i] = 0; /* Array end */
>> +}
>>
>> -	if (ret[0] == 0) {
>> -		/* No PID at all */
>> -		free(ret);
>> +pid_t *ustctl_get_online_pids(void)
>> +{
>> +	char *dir_name;
>> +	DIR *dir;
>> +	unsigned int pid_list_size = sizeof(pid_t);
>> +	pid_t *pid_list = NULL;
>> +
>> +	dir_name = ustcomm_user_sock_dir();
>> +	if (!dir_name) {
>> 		return NULL;
>> 	}
>>
>> +	dir = opendir(dir_name);
>> +	if (!dir) {
>> +		goto free_dir_name;
>> +	}
>> +
>> +	pid_list = (pid_t *) malloc(pid_list_size);
>> +
>> +	get_pids_in_dir(dir, &pid_list, &pid_list_size);
>> +
>> +	if (pid_list[0] == 0) {
>> +		/* No PID at all */
>> +		free(pid_list);
>> +		pid_list = NULL;
>> +		goto close_dir;
>> +	}
>> +
>> +close_dir:
>> 	closedir(dir);
>> -	return ret;
>> +
>> +free_dir_name:
>> +	free(dir_name);
>> +
>> +	return pid_list;
>> }
>>
>> /**
>> -- 
>> 1.7.1
>>
>>
>> _______________________________________________
>> ltt-dev mailing list
>> ltt-dev at lists.casi.polymtl.ca
>> http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev
>>
>
> -- 
> Mathieu Desnoyers
> Operating System Efficiency R&D Consultant
> EfficiOS Inc.
> http://www.efficios.com
>
> _______________________________________________
> ltt-dev mailing list
> ltt-dev at lists.casi.polymtl.ca
> http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev