On 6/23/22 22:26, Steven Rostedt wrote:
> On Thu, 16 Jun 2022 10:44:43 +0200
> Daniel Bristot de Oliveira <bristot@xxxxxxxxxx> wrote:
[ removing comments that I agreed and changed the code/log accordingly ]
>>
>> "monitoring_on"
>> - It is an on/off general switcher for monitoring. Note
>> that it does not disable enabled monitors, but stop the per-entity
>> monitors of monitoring the events received from the system.
>> It resambles the "tracing_on" switcher.
>
> You mean that the tracepoints are still attached, but the process of
> monitoring isn't doing anything?
correct, I am now mentioning it in the comment.
[...]
>> +static int disable_monitor(struct rv_monitor_def *mdef)
>> +{
>> + if (mdef->monitor->enabled) {
>> + mdef->monitor->enabled = 0;
>> + mdef->monitor->stop();
>> + }
>> +
>> + mdef->enabled = 0;
>
> What's the difference between mdef->enabled and mdef->monitor->enabled?
Ooops, the mdef->enabled is a leftover... removing mdef->enabled.
>> + return 0;
>> +}
>> +
[...]
>> +static int create_monitor_dir(struct rv_monitor_def *mdef)
>> +{
>> + struct dentry *root = get_monitors_root();
>> + struct dentry *tmp;
>> + const char *name = mdef->monitor->name;
>> + int retval = 0;
>> +
>> + mdef->root_d = rv_create_dir(name, root);
>> +
>> + if (!mdef->root_d)
>> + return -ENOMEM;
>> +
>> + tmp = rv_create_file("enable", 0600,
>
> I'd recommend make the modes (0600) into macros. I recently changed
> these for tracing, and having them hard coded was a pain.
>
> #define RV_FILE_READ 0600
>
Added:
#define RV_MODE_WRITE TRACE_MODE_WRITE
#define RV_MODE_READ TRACE_MODE_READ
>> + mdef->root_d, mdef,
>> + &interface_enable_fops);
>> + if (!tmp) {
>> + retval = -ENOMEM;
>> + goto out_remove_root;
>> + }
>> +
>> + tmp = rv_create_file("desc", 0400,
>
> Same here, and in all other cases.
>
>> + mdef->root_d, mdef,
>> + &interface_desc_fops);
>> + if (!tmp) {
>> + retval = -ENOMEM;
>> + goto out_remove_root;
>> + }
>> +
>> + return retval;
>> +
>> +out_remove_root:
>> + rv_remove(mdef->root_d);
>> + return retval;
>> +}
[...]
>> +static ssize_t
>> +enabled_monitors_write(struct file *filp, const char __user *user_buf,
>> + size_t count, loff_t *ppos)
>> +{
>> + char buff[MAX_RV_MONITOR_NAME_SIZE+1];
>> + struct rv_monitor_def *mdef;
>> + int retval = -EINVAL;
>> + bool enable = true;
>> + char *ptr = buff;
>> + int len;
>> +
>> + if (count < 1 || count > MAX_RV_MONITOR_NAME_SIZE+1)
>> + return -EINVAL;
>> +
>> + memset(buff, 0, sizeof(buff));
>> +
>> + retval = simple_write_to_buffer(buff, sizeof(buff)-1, ppos, user_buf,
>> + count);
>> + if (!retval)
>> + return -EFAULT;
>> +
>> + if (buff[0] == '!') {
>> + enable = false;
>> + ptr++;
>> + }
>> +
>> + len = strlen(ptr);
>> + if (!len)
>> + return count;
>> + /*
>> + * remove \n
>> + */
>> + ptr[len-1] = '\0';
>
> Are you sure there's an '\n' here?
>
> One could just do "write(fd, "monitor", 7)"
>
> Perhaps use strim()
ack.
>
>> +
>> + mutex_lock(&rv_interface_lock);
>> +
>> + retval = -EINVAL;
>> +
>> + list_for_each_entry(mdef, &rv_monitors_list, list) {
>> + if (strcmp(ptr, mdef->monitor->name) == 0) {
>
> BTW, you could do:
>
> if (strcmp(ptr, mdef->monitor->name) != 0)
> continue;
>
> And then get rid of an extra indent below.
>
>> + /*
>> + * Monitor found!
>> + */
>> + if (enable)
>> + retval = enable_monitor(mdef);
>> + else
>> + retval = disable_monitor(mdef);
>> +
>> + if (retval)
>> + goto out;
>
> Why not just break?
>
> In fact, you could just do:
>
> if (!retval)
> retval = count;
> break;
yep, it looks better.
-- Daniel
