From mboxrd@z Thu Jan  1 00:00:00 1970
From: david.goulet@polymtl.ca (David Goulet)
Date: Tue, 09 Nov 2010 11:28:52 -0500
Subject: [ltt-dev] [UST PATCH 2/3] Add ustcomm_trace_info struct and
 support functions to ustcomm
In-Reply-To: <1288889678-23413-2-git-send-email-nils.carlson@ericsson.com>
References: <1288889678-23413-1-git-send-email-nils.carlson@ericsson.com>
	<1288889678-23413-2-git-send-email-nils.carlson@ericsson.com>
Message-ID: <4CD976C4.9080003@polymtl.ca>

Comments below.

On 10-11-04 12:54 PM, Nils Carlson wrote:
>
> Signed-off-by: Nils Carlson<nils.carlson at ericsson.com>
> ---
>   libustcomm/ustcomm.c |   32 ++++++++++++++++++++++++++++++++
>   libustcomm/ustcomm.h |   11 +++++++++++
>   2 files changed, 43 insertions(+), 0 deletions(-)
>
> diff --git a/libustcomm/ustcomm.c b/libustcomm/ustcomm.c
> index fe8fea2..7d0fe00 100644
> --- a/libustcomm/ustcomm.c
> +++ b/libustcomm/ustcomm.c
> @@ -621,6 +621,38 @@ char * ustcomm_restore_ptr(char *ptr, char *data_field, int data_field_size)
>   	return data_field + (long)ptr;
>   }
>
> +int ustcomm_pack_trace_info(struct ustcomm_header *header,
> +			    struct ustcomm_trace_info *trace_inf,
> +			    const char *trace)
> +{
> +	int offset = 0;
> +
> +	trace_inf->trace = ustcomm_print_data(trace_inf->data,
> +					      sizeof(trace_inf->data),
> +					&offset,
> +					      trace);

In order to understand this "data flow", I put some printf here to 
output me the "trace_inf->trace" (that in my understanding, should be 
the trace name with some other stuff...?) but each time it's NULL

It appears that you are passing "trace" to ustcomm_print_data and it 
using it as a "format" and results in having :

trace_inf->trace = NULL
trace_inf->data = "auto"

I'm wondering if this is correct ? (Maybe a little comment on print_data 
function to tell what kind of output it's generating?)

> +
> +	if (trace_inf->trace == USTCOMM_POISON_PTR) {
> +		return -ENOMEM;
> +	}
> +
> +	header->size = COMPUTE_MSG_SIZE(trace_inf, offset);
> +
> +	return 0;
> +}
> +
> +
> +int ustcomm_unpack_trace_info(struct ustcomm_trace_info *trace_inf)
> +{
> +	trace_inf->trace = ustcomm_restore_ptr(trace_inf->trace,
> +					       trace_inf->data,
> +					       sizeof(trace_inf->data));
> +	if (!trace_inf->trace) {
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
>
>   int ustcomm_pack_channel_info(struct ustcomm_header *header,
>   			      struct ustcomm_channel_info *ch_inf,
> diff --git a/libustcomm/ustcomm.h b/libustcomm/ustcomm.h
> index f62250c..d548bc1 100644
> --- a/libustcomm/ustcomm.h
> +++ b/libustcomm/ustcomm.h
> @@ -78,6 +78,11 @@ enum tracectl_commands {
>   	STOP_TRACE,
>   };
>
> +struct ustcomm_trace_info {
> +	char *trace;

Would not be better to call the above variable something like 
"trace_name" because it's becoming quite confusing in the code with 
"trace" all over the place.

If so, maybe change the name all over the code from const char *trace to 
const char *trace_name

Some other comment in the next patch but it follow these a bit.

Thanks
David

> +	char data[USTCOMM_DATA_SIZE];
> +};
> +
>   struct ustcomm_channel_info {
>   	char *channel;
>   	unsigned int subbuf_size;
> @@ -172,6 +177,12 @@ extern char * ustcomm_restore_ptr(char *ptr, char *data_field,
>   	(size_t) (long)(struct_ptr)->data - (long)(struct_ptr) + (offset)
>
>   /* Packing and unpacking functions, making life easier */
> +extern int ustcomm_pack_trace_info(struct ustcomm_header *header,
> +				   struct ustcomm_trace_info *trace_inf,
> +				   const char *trace);
> +
> +extern int ustcomm_unpack_trace_info(struct ustcomm_trace_info *trace_inf);
> +
>   extern int ustcomm_pack_channel_info(struct ustcomm_header *header,
>   				     struct ustcomm_channel_info *ch_inf,
>   				     const char *channel);

-- 
David Goulet
LTTng project, DORSAL Lab.

PGP/GPG : 1024D/16BD8563
BE3C 672B 9331 9796 291A  14C6 4AF7 C14B 16BD 8563