[ltt-dev] [PATCH v3 4/4] Add ltt-trace-control module

[Zhaolei]

Hello, Mathieu

Thanks for your review.

Here is my answer for your review in different mails.

>> Todo:
>>   Make ltt-relay and ltt-relay-locked to create their dir in ltt's root dir as:
>>   debugfs/ltt/relay and debugfs/ltt/relay-locked
>> 
> 
> Maybe it's not that important to do the distinction between relay and
> relay-locked using subdirectories. It will just confuse users.
> 
> The only thing I care about is that if a user creates a trace backed
> with "relay" and then another trace with the same name, but backed with
> "relay-locked", it does not crash the kernel and return the appropriate
> error message. I think it will all work fine as-is now.
Thanks, I agree.

>> +static inline struct dentry *dir_lookup(struct dentry *parent,
> 
> static should be enough here, without "inline". The compiler will inline
> it anyway. It's mostly a matter of habits : generally, inline is only
> used for headers. Given this is a C file, let's let the compiler decide
> if it wants to inline it or not.
Thanks, I agree.

>> + switch (cmd[0]) {
>> + case 'Y':
>> + case 'y':
>> + case '1':
>> + err = ltt_trace_start(
>> + file->f_dentry->d_parent->d_name.name);
> 
> does it need to be on 2 lines ? (80 cols ?)
Thanks, I agree.

>> + /*
>> + * ltt_trace_start return +ERRNO,
>> + * so we can't use IS_ERR_VALUE(err)
>> + */
>> + if (err) {
>> + printk(KERN_ERR
>> + "enabled_write: ltt_trace_start failed: %d\n",
>> + err);
>> + err = -EPERM;
>> + goto err_start_trace;
>> + }
>> + break;
>> + case 'N':
>> + case 'n':
>> + case '0':
>> + err = ltt_trace_stop(
>> + file->f_dentry->d_parent->d_name.name);
> 
> does it need to be on 2 lines ?
> 
Thanks, I agree.

>> + if (!i) {
> 
> Hrm ? What is special about i == 0 ?
> 
>> + /* debugfs/control/trace_name/channel/cpu/mode */
>> + tmp_den = debugfs_create_file("mode", S_IWUSR,
>> + channel_den, NULL, &ltt_mode_operations);
Because we need to create control file of
ebugfs/control/trace_name/channel/cpu/mode only in cpu channel's dir as your
suggestion.

But IMHO, mode is not necessary now, because we can control each channel's
overwrite.
So maybe I'd like to remove conception of "mode" in ltt's kernel source.
It will delete some struct member, some function, and modify some function,
so we can do it with another patch.
User-mode tools can have conception of "mode". They can set each channel's
overwrite attribute by mode.
(OR let user-mode tools get rid of  conception of mode, and control each
channel's overwrite attribute directly)

I am considering how to make user-mode tools both flexible and simple by use
new ltt-trace-control.ko.
1: Is user-mode tools need to know "setup" period of a trace
2: Is user-mode tools need to control each channel's subbuf cnt and size
3: Is user-mode tools need to control each channel's overwrite
4: Is user-mode tools need to select trace's channel(relay or relay-locked)
(Current version of lttctl is "no" for all above questions)

More powerful control is not bad, but maybe have risk of makeing our user
hard to understand.

What about your opinion?

>> + err = ltt_trace_destroy(trace_name);
>> + /*
>> + * ltt_trace_destroy return +ERRNO,
>> + * so we can't use IS_ERR_VALUE(err)
> 
> ltt_trace_destroy returns a int, not a pointer to some variable which
> can have to take negative value to be an error. In this case, just
> checking for non-zero return value is OK. Do we really need to leave
> this comment here ?
Thanks, I'll remove these comments.
(But IMHO, return -ERRNO is better, because it is more common in kernel,
and we can use IS_ERR_VALUE to check it.)

>> +module_init(ltt_trace_control_init);
>> +module_exit(ltt_trace_control_exit);
>> +
>> +MODULE_LICENSE("GPL");
> 
> You could add :
> 
> MODULE_AUTHOR(...);
> MODULE_DESCRIPTION(...);
Thanks, I'll add it.

B.R.
Zhaolei