[ltt-dev] [UST PATCH] Introduce a new communication protocol for UST

[Nils Carlson]

On Wed, 13 Oct 2010, David Goulet wrote:

> Comments below :
>
> On 10-10-12 03:59 AM, Nils Carlson wrote:
>>
>> +static struct ustcomm_header _receive_header;
>> +static struct ustcomm_header *receive_header =&_receive_header;
>
> Just tell me why you do this little trick here and across the code?
>

It's mostly because it's better to work with pointers and -> than with 
structs and members because if you revise the code there is a big risk you 
will en up having to change . to -> in a lot of places.

>> +     result = ustcomm_pack_buffer_info(&send_header,
>> +                                     &buf_inf,
>> +                                       channel,
>> +                                       cpu);
>
> Just wondering since we've agreed to follow kernel syntax code, is it
> "correct" to have each arguments on a different line?

Yep, you don't have much choice when you have an 80 char limit, and if
you're splitting across multiple lines this makes it easier to read than
figuring out which lines contained multiple arguments.

>> -     end:
>> +     channel->subbuf_size = power;
>> +     DBG("the set_subbuf_size for the requested channel is %zd", channel->subbuf_size);
>
> Just curious here why %zd. I've test it and format ‘%zd’ expects type
> ‘signed size_t’ but subbuf_size is unsigned int...


Vestige. Fixed.

>>
>> -static int do_cmd_get_subbuffer(const char *recvbuf, int sock)
>> +static int get_subbuffer(const char *trace_name, const char *ch_name,
>> +                      int ch_cpu, long *consumed_old)
>
> I liked the "do_cmd" prefix for "readability" to indicate that this
> function is actually used for a command... don't you think ? (self
> documented code)
>
> Also, libustd.c and this file has now the same function name for a lot
> of them. Don't you think things will get much more confusing ? (or maybe
> you did that on purpose ?)


do_cmd made the lines so long. That's pretty much why I got rid of it. And 
there is a certain symmetry in that functions that call a library function 
over a socket have the same name. Though in a way it feels like we're 
re-inventing RPC. It's almost a pity solaris doors aren't available. :-)

>
> This comment applies to the next function (process_simple..)
>
> Have you consider the possibility of arguments given by the client ? As
> an example, if we consider using trace name rather then hardcoded
> "auto", we will need to pass it all the way here...
>
> Ex : lttctl
>
> lttctl -c trace1 -o ...
>
> we still don't have this features in ustctl but maybe think about future
> work ?


Yep, that'll be the next thing I start on after this patch, actually, 
preparing way for that was in a way the whole point of the last two 
patches. I really didn't want to start on implementing that through string 
parsing.

>> +/* Simple commands are those which need only respond with a return value. */
>> +static int process_simple_client_cmd(int command, char *recv_buf)
>>   {
>>       int result;
>> -     char trace_name[] = "auto";
>>       char trace_type[] = "ustrelay";
>> -     int len;
>
> Should the following case be indented from the switch(command) ?

No, see kernel coding standards. This is fine to save space.


>> +     switch(command) {
>> +     case SET_SOCK_PATH:
>> +     {
>> +             struct ustcomm_sock_path *sock_msg;
>> +             sock_msg = (struct ustcomm_sock_path *)recv_buf;
>> +             sock_msg->sock_path =
>> +                     ustcomm_restore_ptr(sock_msg->sock_path,
>> +                                         sock_msg->data,
>> +                                         sizeof(sock_msg->data));
>> +             if (!sock_msg->sock_path) {
>> +
>> +                     return -EINVAL;
>>                       ERR("ltt_trace_alloc failed");
>> -                     return -1;
>> +                     return result;
>>               }
>>               if (!result) {
>
> I'm wondering here if it's correct... if result = -1, we still execute
> inform_consumer_daemon... on a quick look, I don't think we want that ??
>

Bit confused here... A unary logical negation operator (!) turns 0 into 1 
and everything else into 0, so inform_consumer_daemon is only called if 
result == 0. Also there is an if statement just above checking for less 
than 0.

>>                       inform_consumer_daemon(trace_name);
>> @@ -992,138 +646,381 @@ static int process_client_cmd(char *recvbuf, int sock)
>>               result = ltt_trace_start(trace_name);
>>               if (result<  0) {
>>                       ERR("ltt_trace_start failed");
>> -                     return -1;
>> +                     return result;
>>               }
>> -     } else if (!strcmp(recvbuf, "trace_stop")) {
>> +
>> +             return 0;
>> +     case STOP_TRACE:
>> -             do_cmd_get_n_subbufs(recvbuf, sock);
>> -     } else if (nth_token_is(recvbuf, "get_subbuf_size", 0) == 1) {
>> -             do_cmd_get_subbuf_size(recvbuf, sock);
>> -     } else if (nth_token_is(recvbuf, "load_probe_lib", 0) == 1) {
>> -             char *libfile;
>> +             return 0;
>> +     case FORCE_SUBBUF_SWITCH:
>> +             force_subbuf_switch();
>
> I think a future patch should be done for a return code about this
> function... a fail switch will go unotice :(


Yepp, probably a good idea. I'll put a fixme here actually...

>> -
>>   #define GET_SUBBUF_OK 1
>>   #define GET_SUBBUF_DONE 0
>>   #define GET_SUBBUF_DIED 2
>> @@ -53,134 +48,94 @@
>>
>>   #define UNIX_PATH_MAX 108
>
> Since we are here chuping everything :P, this define, I have absolutely
> NO IDEA why it's set to 108... linux/limits.h has a PATH_MAX defined at
> 4096...

This is the size of the char array in the struct... I've removed most of 
these, but this one is still left. It should also be fixed though so I'll 
put a fixme here...


Thanks for all the help!

/Nils