From mboxrd@z Thu Jan 1 00:00:00 1970 From: nils.carlson@ludd.ltu.se (Nils Carlson) Date: Tue, 9 Nov 2010 19:12:02 +0100 Subject: [ltt-dev] [UST PATCH 2/3] Add ustcomm_trace_info struct and support functions to ustcomm In-Reply-To: <507B0421-5A29-49F9-B6CF-0C4E6CD36A7B@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> Message-ID: <5C036F91-6009-4313-92AD-2B4743D01546@ludd.ltu.se> 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