From mboxrd@z Thu Jan 1 00:00:00 1970 From: david.goulet@polymtl.ca (David Goulet) Date: Tue, 09 Nov 2010 14:19:07 -0500 Subject: [ltt-dev] [UST PATCH 2/3] Add ustcomm_trace_info struct and support functions to ustcomm In-Reply-To: <5C036F91-6009-4313-92AD-2B4743D01546@ludd.ltu.se> References: <1288889678-23413-1-git-send-email-nils.carlson@ericsson.com> <1288889678-23413-2-git-send-email-nils.carlson@ericsson.com> <4CD976C4.9080003@polymtl.ca> <507B0421-5A29-49F9-B6CF-0C4E6CD36A7B@ludd.ltu.se> <5C036F91-6009-4313-92AD-2B4743D01546@ludd.ltu.se> Message-ID: <4CD99EAB.6040603@polymtl.ca> Good. Acked-by: David Goulet 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 >>>> --- >>>> 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