[ltt-dev] [UST PATCH] Get Online targets checker
Mathieu Desnoyers
compudj at krystal.dyndns.org
Mon Mar 28 11:50:46 EDT 2011
* Matthew Khouzam (matthew.khouzam at ericsson.com) wrote:
> Added a check in get online pids to only send the pids that are
> currently online.
>
>
> Signed-off-by: Matthew Khouzam <matthew.khouzam at ericsson.com>
> ---
> libustctl/libustctl.c | 15 ++++++++++++++-
> 1 files changed, 14 insertions(+), 1 deletions(-)
>
> diff --git a/libustctl/libustctl.c b/libustctl/libustctl.c
> index d57e645..f0b1fb1 100644
> --- a/libustctl/libustctl.c
> +++ b/libustctl/libustctl.c
> @@ -92,6 +92,8 @@ pid_t *ustctl_get_online_pids(void)
> {
> struct dirent *dirent;
> DIR *dir;
> + DIR *proc_dir;
> + char proc_dir_path[PATH_MAX];
> unsigned int ret_size = 1 * sizeof(pid_t), i = 0;
>
> dir = opendir(SOCK_DIR);
> @@ -117,7 +119,18 @@ pid_t *ustctl_get_online_pids(void)
> * We need to figure out an intelligent way of solving
> * this, maybe connect-disconnect.
> */
> - if (1) {
> + snprintf( proc_dir_path, PATH_MAX, "/proc/%u/" , ret[i]) ;
please remove extra space before , and before ; and after snprintf(
> + proc_dir = opendir(proc_dir_path);
> + /*
> + * Note:
> + * maybe we should remove the empty dir in this pass too.
> + * Since the detection method is not perfect, this step
> + * is not yet implemented.
> + * A process can die, and its pid can be still taken when
> + * reading online pids.
> + */
> + if( proc_dir ) {
if (proc_dir) {
instead. (ensure white spaces are ok)
You should try running your patch trough the checkpatch.pl program
available in the Linux kernel scripts/ directory, it will tell you all
these coding style issues.
Thanks,
Mathieu
> ret_size += sizeof(pid_t);
> ret = (pid_t *) realloc(ret, ret_size);
> ++i;
> --
> 1.7.0.4
>
> On 11-03-28 11:21 AM, Mathieu Desnoyers wrote:
>> * Matthew Khouzam (matthew.khouzam at ericsson.com) wrote:
>>> Added a check in get online pids to only send the pids that are
>>> currently online.
>>>
>>> Signed-off-by: Matthew Khouzam<matthew.khouzam at ericsson.com>
>>> ---
>>> libustctl/libustctl.c | 13 ++++++++++++-
>>> 1 files changed, 12 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/libustctl/libustctl.c b/libustctl/libustctl.c
>>> index d57e645..45cc3b8 100644
>>> --- a/libustctl/libustctl.c
>>> +++ b/libustctl/libustctl.c
>>> @@ -92,6 +92,8 @@ pid_t *ustctl_get_online_pids(void)
>>> {
>>> struct dirent *dirent;
>>> DIR *dir;
>>> + DIR *proc_dir;
>>> + char proc_dir_path[PATH_MAX];
>>> unsigned int ret_size = 1 * sizeof(pid_t), i = 0;
>>>
>>> dir = opendir(SOCK_DIR);
>>> @@ -117,7 +119,16 @@ pid_t *ustctl_get_online_pids(void)
>>> * We need to figure out an intelligent way of solving
>>> * this, maybe connect-disconnect.
>>> */
>>> - if (1) {
>>> + snprintf( proc_dir_path, PATH_MAX, "/proc/%u/" , ret[i]) ;
>>> + proc_dir = opendir(proc_dir_path);
>> Small nits:
>>
>> Please use:
>>
>> /*
>> * some text.
>> * other text.
>> */
>>
>> For multiline comments.
>>
>>> + /* Note:
>>> + * maybe we should remove the empty dir in this pass too.
>>> + * Since the detection method is not perfect, this step
>>> + * is not yet implemented.
>>> + * A process can die, and it's pid can be still taken when
>> it's -> its.
>>
>>> + * reading online pids. */
>>> + if( proc_dir != NULL )
>> if (proc_dir != NULL) {
>>
>> or even better:
>>
>> if (proc_dir) {
>>
>> Thanks!
>>
>> Mathieu
>>
>>
>>
>>> + {
>>> ret_size += sizeof(pid_t);
>>> ret = (pid_t *) realloc(ret, ret_size);
>>> ++i;
>>> --
>>> 1.7.0.4
>>>
>>> On 11-03-25 11:59 PM, Mathieu Desnoyers wrote:
>>>> * Matthew Khouzam (matthew.khouzam at ericsson.com) wrote:
>>>>> Signed-off-by: Matthew Khouzam<matthew.khouzam at ericsson.com>
>>>>> ---
>>>>> libustctl/libustctl.c | 9 ++++++++-
>>>>> 1 files changed, 8 insertions(+), 1 deletions(-)
>>>>>
>>>>> diff --git a/libustctl/libustctl.c b/libustctl/libustctl.c
>>>>> index d57e645..3b5ae84 100644
>>>>> --- a/libustctl/libustctl.c
>>>>> +++ b/libustctl/libustctl.c
>>>>> @@ -92,8 +92,11 @@ pid_t *ustctl_get_online_pids(void)
>>>>> {
>>>>> struct dirent *dirent;
>>>>> DIR *dir;
>>>>> + DIR *proc_dir;
>>>>> + char proc_dir_path[80]; /* proc + / + int always smaller than 80 */
>>>> Hi Matthew,
>>>>
>>>> Please don't use hardcoded values like this. It's begging for buffer
>>>> overflows. You should use PATH_MAX instead.
>>>>
>>>>> unsigned int ret_size = 1 * sizeof(pid_t), i = 0;
>>>>> dir = opendir(SOCK_DIR);
>>>>> if (!dir) {
>>>>> @@ -117,7 +120,11 @@ pid_t *ustctl_get_online_pids(void)
>>>>> * We need to figure out an intelligent way of solving
>>>>> * this, maybe connect-disconnect.
>>>>> */
>>>>> - if (1) {
>>>>> + snprintf( proc_dir_path, 80, "/proc/%u/" , ret[i]) ;
>>>> Please use PATH_MAX here too.
>>>>
>>>> And if you ever really need to use something smaller (here we really
>>>> just don't care about a slightly larger stack size in userspace), you
>>>> should do a define at the top of the file so we know there is a
>>>> non-standard value buried in there.
>>>>
>>>> Thanks,
>>>>
>>>> Mathieu
>>>>
>>>>
>>>>> + proc_dir = opendir(proc_dir_path);
>>>>> + /* maybe we should remove the empty dir in this pass too*/
>>>>> + if( proc_dir != NULL )
>>>>> + {
>>>>> ret_size += sizeof(pid_t);
>>>>> ret = (pid_t *) realloc(ret, ret_size);
>>>>> ++i;
>>>>> --
>>>>> 1.7.0.4
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> 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
More information about the lttng-dev
mailing list