[ltt-dev] [UST PATCH 2/3] Add ustcomm_trace_info struct and support functions to ustcomm

[nils.carlson@ludd.ltu.se (Nils Carlson)]

Some commentary on the comments below... :-)

On Nov 9, 2010, at 5:28 PM, David Goulet wrote:

> 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?)

Can add a comment. What it does is that it packs the data and assigns  
relative pointers, so the NULL there is the relative position of  
trace_inf->trace within the data field. The unpack function restores  
these pointers by adding the address of the data field to each of them.

>
>> +
>> +	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
>

Well, we use the term *marker and *channel to denote their names, so  
it makes sense to do the same with *trace inside ustcomm.
The good news is then that ustcomm is internally consistent.

Will add a comment explaining how our home-made RPC works.

/Nils

> 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
>
> _______________________________________________
> ltt-dev mailing list
> ltt-dev at lists.casi.polymtl.ca
> http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev