[ltt-dev] [UST PATCH] libustctl: cleanup ustctl_get_online_pids functions

[Mathieu Desnoyers]

* Nils Carlson (nils.carlson at ericsson.com) wrote:
> Cleanup the ustctl_get_online_pids functions, checking more errors
> and making the allocation strategy sane.
> 
> Signed-off-by: Nils Carlson <nils.carlson at ericsson.com>
> ---
>  libustctl/libustctl.c |   65 +++++++++++++++++++++++++++++++++++++-----------
>  1 files changed, 50 insertions(+), 15 deletions(-)
> 
> diff --git a/libustctl/libustctl.c b/libustctl/libustctl.c
> index 356d5ef..1911434 100644
> --- a/libustctl/libustctl.c
> +++ b/libustctl/libustctl.c
> @@ -88,11 +88,12 @@ int ustctl_connect_pid(pid_t pid)
>  	return sock;
>  }
> 
> -static void get_pids_in_dir(DIR *dir, pid_t **pid_list,
> +static int get_pids_in_dir(DIR *dir, pid_t **pid_list,
> +			    unsigned int *pid_list_index,
>  			    unsigned int *pid_list_size)
>  {
>  	struct dirent *dirent;
> -	unsigned int read_pid;
> +	long read_pid;
>  
>  	while ((dirent = readdir(dir))) {
>  		if (!strcmp(dirent->d_name, ".") ||
> @@ -103,29 +104,43 @@ static void get_pids_in_dir(DIR *dir, pid_t **pid_list,
>  			continue;
>  		}
>  
> -		sscanf(dirent->d_name, "%u", &read_pid);
> +		errno = 0;
> +		read_pid = strtol(dirent->d_name, NULL, 10);
> +		if (errno) {
> +			continue;
> +		}
>  
> -		(*pid_list)[*pid_list_size - 1] = read_pid;
> -		/* FIXME: Here we previously called pid_is_online, which
> +		/*
> +		 * 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) {
> -			(*pid_list_size)++;
> -			*pid_list = realloc(*pid_list,
> -					    *pid_list_size * sizeof(pid_t));
> +
> +			(*pid_list)[(*pid_list_index)++] = read_pid;
> +
> +			if (*pid_list_index == *pid_list_size) {
> +				*pid_list_size *= 2;
> +				*pid_list = realloc(*pid_list,
> +						    *pid_list_size * sizeof(pid_t));
> +				if (!*pid_list) {
> +					return -1;

Instead of returning here and duplicating the error handing code in the
caller, can you instead do:

					goto error;


> +				}
> +			}
>  		}
>  	}
>  
> -	(*pid_list)[*pid_list_size - 1] = 0; /* Array end */
> +	(*pid_list)[*pid_list_index] = 0; /* Array end */
> +
> +	return 0;

error:
	(*pid_list)[0] = 0;
	return -1;

Thanks,

Mathieu

>  }
>  
>  static pid_t *get_pids_non_root(void)
>  {
>  	char *dir_name;
>  	DIR *dir;
> -	unsigned int pid_list_size = 1;
> +	unsigned int pid_list_index = 0, pid_list_size = 1;
>  	pid_t *pid_list = NULL;
>  
>  	dir_name = ustcomm_user_sock_dir();
> @@ -143,13 +158,15 @@ static pid_t *get_pids_non_root(void)
>  		goto close_dir;
>  	}
>  
> -	get_pids_in_dir(dir, &pid_list, &pid_list_size);
> +	if (get_pids_in_dir(dir, &pid_list, &pid_list_index, &pid_list_size)) {
> +		/* if any errors are encountered, force freeing of the list */
> +		pid_list[0] = 0;
> +	}
>  
>  	if (pid_list[0] == 0) {
>  		/* No PID at all */
>  		free(pid_list);
>  		pid_list = NULL;
> -		goto close_dir;
>  	}
>  
>  close_dir:
> @@ -165,9 +182,10 @@ static pid_t *get_pids_root(void)
>  {
>  	char *dir_name;
>  	DIR *tmp_dir, *dir;
> -	unsigned int pid_list_size = 1;
> +	unsigned int pid_list_index = 0, pid_list_size = 1;
>  	pid_t *pid_list = NULL;
>  	struct dirent *dirent;
> +	int result;
>  
>  	tmp_dir = opendir(USER_TMP_DIR);
>  	if (!tmp_dir) {
> @@ -184,7 +202,8 @@ static pid_t *get_pids_root(void)
>  		if (!strncmp(dirent->d_name, USER_SOCK_DIR_BASE,
>  			     strlen(USER_SOCK_DIR_BASE))) {
>  
> -			if (asprintf(&dir_name, USER_TMP_DIR "/%s", dirent->d_name) < 0) {
> +			if (asprintf(&dir_name, USER_TMP_DIR "/%s",
> +				     dirent->d_name) < 0) {
>  				goto close_tmp_dir;
>  			}
>  
> @@ -196,12 +215,28 @@ static pid_t *get_pids_root(void)
>  				continue;
>  			}
>  
> -			get_pids_in_dir(dir, &pid_list, &pid_list_size);
> +			result = get_pids_in_dir(dir, &pid_list, &pid_list_index,
> +						 &pid_list_size);
>  
>  			closedir(dir);
> +
> +			if (result) {
> +				/*
> +				 * if any errors are encountered,
> +				 * force freeing of the list
> +				 */
> +				pid_list[0] = 0;
> +				break;
> +			}
>  		}
>  	}
>  
> +	if (pid_list[0] == 0) {
> +		/* No PID at all */
> +		free(pid_list);
> +		pid_list = NULL;
> +	}
> +
>  close_tmp_dir:
>  	closedir(tmp_dir);
>  
> -- 
> 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