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

[Nils Carlson]

But maybe in lieu of my comments you can ack this patch and I'll  
submit another one commenting this code and the pre-existing code that  
does the same stuff? Would make things a bit easier.. ;-)

/Nils
On Nov 9, 2010, at 6:45 PM, Nils Carlson wrote:

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