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

[David Goulet]

Good.

Acked-by: David Goulet <david.goulet at polymtl.ca>

On 10-11-09 01:12 PM, Nils Carlson wrote:
> 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
>

-- 
David Goulet
LTTng project, DORSAL Lab.

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