Code for registering triggers assumes it's possible to register more than one trigger at a time. In fact, it's unimplemented and there doesn't seem to be a reason to do that. Remove the n_registered param from event_trigger_register() and fix up callers. Doing so simplifies the logic in event_trigger_register to the point that it just becomes a wrapper calling event_command.reg(). It also removes the problematic call to event_command.unreg() in case of failure. A new function, event_trigger_unregister() is also added for callers to call themselves. The changes to trace_events_hist.c simply allow compilation; a separate patch follows which updates the hist triggers to work correctly with the new changes. Link: https://lkml.kernel.org/r/6149fec7a139d93e84fa4535672fb5bef88006b0.1644010575.git.zanussi@kernel.org
Showing the changes to kernel/trace (3 of 3 files). Show all
Unified · Split
| @@ -1629,10 +1629,11 @@ extern void event_trigger_reset_filter(struct event_command *cmd_ops, · 1628 unmodified lines | ||
| 1629 | 1629 | extern int event_trigger_register(struct event_command *cmd_ops, |
| 1630 | 1630 | struct trace_event_file *file, |
| 1631 | 1631 | char *glob, |
| 1632 | − char *cmd, | |
| 1633 | − char *trigger, | |
| 1634 | − struct event_trigger_data *trigger_data, | |
| 1635 | − int *n_registered); | |
| 1632 | + struct event_trigger_data *trigger_data); | |
| 1633 | +extern void event_trigger_unregister(struct event_command *cmd_ops, | |
| 1634 | + struct trace_event_file *file, | |
| 1635 | + char *glob, | |
| 1636 | + struct event_trigger_data *trigger_data); | |
| 1636 | 1637 | |
| 1637 | 1638 | /** |
| 1638 | 1639 | * struct event_trigger_ops - callbacks for trace event triggers |
| @@ -6287,7 +6287,7 @@ static int event_hist_trigger_parse(struct event_command *cmd_ops, · 6286 unmodified lines | ||
| 6287 | 6287 | goto out_free; |
| 6288 | 6288 | } |
| 6289 | 6289 | |
| 6290 | − cmd_ops->unreg(glob+1, trigger_data, file); | |
| 6290 | + event_trigger_unregister(cmd_ops, file, glob+1, trigger_data); | |
| 6291 | 6291 | se_name = trace_event_name(file->event_call); |
| 6292 | 6292 | se = find_synth_event(se_name); |
| 6293 | 6293 | if (se) |
| @@ -6296,13 +6296,10 @@ static int event_hist_trigger_parse(struct event_command *cmd_ops, · 2 unmodified lines | ||
| 6296 | 6296 | goto out_free; |
| 6297 | 6297 | } |
| 6298 | 6298 | |
| 6299 | − ret = cmd_ops->reg(glob, trigger_data, file); | |
| 6300 | − /* | |
| 6301 | − * The above returns on success the # of triggers registered, | |
| 6302 | − * but if it didn't register any it returns zero. Consider no | |
| 6303 | − * triggers registered a failure too. | |
| 6304 | − */ | |
| 6305 | − if (!ret) { | |
| 6299 | + ret = event_trigger_register(cmd_ops, file, glob, trigger_data); | |
| 6300 | + if (ret) | |
| 6301 | + goto out_free; | |
| 6302 | + if (ret == 0) { | |
| 6306 | 6303 | if (!(attrs->pause || attrs->cont || attrs->clear)) |
| 6307 | 6304 | ret = -ENOENT; |
| 6308 | 6305 | goto out_free; |
| @@ -6331,15 +6328,13 @@ enable: · 22 unmodified lines | ||
| 6331 | 6328 | se = find_synth_event(se_name); |
| 6332 | 6329 | if (se) |
| 6333 | 6330 | se->ref++; |
| 6334 | − /* Just return zero, not the number of registered triggers */ | |
| 6335 | − ret = 0; | |
| 6336 | 6331 | out: |
| 6337 | 6332 | if (ret == 0) |
| 6338 | 6333 | hist_err_clear(); |
| 6339 | 6334 | |
| 6340 | 6335 | return ret; |
| 6341 | 6336 | out_unreg: |
| 6342 | − cmd_ops->unreg(glob+1, trigger_data, file); | |
| 6337 | + event_trigger_unregister(cmd_ops, file, glob+1, trigger_data); | |
| 6343 | 6338 | out_free: |
| 6344 | 6339 | if (cmd_ops->set_filter) |
| 6345 | 6340 | cmd_ops->set_filter(NULL, trigger_data, NULL); |
| @@ -587,13 +587,12 @@ static int register_trigger(char *glob, · 586 unmodified lines | ||
| 587 | 587 | } |
| 588 | 588 | |
| 589 | 589 | list_add_rcu(&data->list, &file->triggers); |
| 590 | − ret++; | |
| 591 | 590 | |
| 592 | 591 | update_cond_flag(file); |
| 593 | − if (trace_event_trigger_enable_disable(file, 1) < 0) { | |
| 592 | + ret = trace_event_trigger_enable_disable(file, 1); | |
| 593 | + if (ret < 0) { | |
| 594 | 594 | list_del_rcu(&data->list); |
| 595 | 595 | update_cond_flag(file); |
| 596 | − ret--; | |
| 597 | 596 | } |
| 598 | 597 | out: |
| 599 | 598 | return ret; |
| @@ -927,48 +926,37 @@ void event_trigger_reset_filter(struct event_command *cmd_ops, · 327 unmodified lines | ||
| 927 | 926 | * @cmd_ops: The event_command operations for the trigger |
| 928 | 927 | * @file: The event file for the trigger's event |
| 929 | 928 | * @glob: The trigger command string, with optional remove(!) operator |
| 930 | − * @cmd: The cmd string | |
| 931 | − * @param: The param string | |
| 932 | 929 | * @trigger_data: The trigger_data for the trigger |
| 933 | − * @n_registered: optional outparam, the number of triggers registered | |
| 934 | 930 | * |
| 935 | 931 | * Register an event trigger. The @cmd_ops are used to call the |
| 936 | − * cmd_ops->reg() function which actually does the registration. The | |
| 937 | − * cmd_ops->reg() function returns the number of triggers registered, | |
| 938 | − * which is assigned to n_registered, if n_registered is non-NULL. | |
| 932 | + * cmd_ops->reg() function which actually does the registration. | |
| 939 | 933 | * |
| 940 | 934 | * Return: 0 on success, errno otherwise |
| 941 | 935 | */ |
| 942 | 936 | int event_trigger_register(struct event_command *cmd_ops, |
| 943 | 937 | struct trace_event_file *file, |
| 944 | 938 | char *glob, |
| 945 | − char *cmd, | |
| 946 | − char *param, | |
| 947 | − struct event_trigger_data *trigger_data, | |
| 948 | − int *n_registered) | |
| 939 | + struct event_trigger_data *trigger_data) | |
| 949 | 940 | { |
| 950 | − int ret; | |
| 951 | − | |
| 952 | − if (n_registered) | |
| 953 | − *n_registered = 0; | |
| 954 | − | |
| 955 | − ret = cmd_ops->reg(glob, trigger_data, file); | |
| 956 | − /* | |
| 957 | − * The above returns on success the # of functions enabled, | |
| 958 | − * but if it didn't find any functions it returns zero. | |
| 959 | − * Consider no functions a failure too. | |
| 960 | − */ | |
| 961 | − if (!ret) { | |
| 962 | − cmd_ops->unreg(glob, trigger_data, file); | |
| 963 | − ret = -ENOENT; | |
| 964 | − } else if (ret > 0) { | |
| 965 | − if (n_registered) | |
| 966 | − *n_registered = ret; | |
| 967 | − /* Just return zero, not the number of enabled functions */ | |
| 968 | − ret = 0; | |
| 969 | − } | |
| 941 | + return cmd_ops->reg(glob, trigger_data, file); | |
| 942 | +} | |
| 970 | 943 | |
| 971 | − return ret; | |
| 944 | +/** | |
| 945 | + * event_trigger_unregister - unregister an event trigger | |
| 946 | + * @cmd_ops: The event_command operations for the trigger | |
| 947 | + * @file: The event file for the trigger's event | |
| 948 | + * @glob: The trigger command string, with optional remove(!) operator | |
| 949 | + * @trigger_data: The trigger_data for the trigger | |
| 950 | + * | |
| 951 | + * Unregister an event trigger. The @cmd_ops are used to call the | |
| 952 | + * cmd_ops->unreg() function which actually does the unregistration. | |
| 953 | + */ | |
| 954 | +void event_trigger_unregister(struct event_command *cmd_ops, | |
| 955 | + struct trace_event_file *file, | |
| 956 | + char *glob, | |
| 957 | + struct event_trigger_data *trigger_data) | |
| 958 | +{ | |
| 959 | + cmd_ops->unreg(glob, trigger_data, file); | |
| 972 | 960 | } |
| 973 | 961 | |
| 974 | 962 | /* |
| @@ -1027,7 +1015,7 @@ event_trigger_parse(struct event_command *cmd_ops, · 52 unmodified lines | ||
| 1027 | 1015 | INIT_LIST_HEAD(&trigger_data->named_list); |
| 1028 | 1016 | |
| 1029 | 1017 | if (glob[0] == '!') { |
| 1030 | − cmd_ops->unreg(glob+1, trigger_data, file); | |
| 1018 | + event_trigger_unregister(cmd_ops, file, glob+1, trigger_data); | |
| 1031 | 1019 | kfree(trigger_data); |
| 1032 | 1020 | ret = 0; |
| 1033 | 1021 | goto out; |
| @@ -1062,17 +1050,10 @@ event_trigger_parse(struct event_command *cmd_ops, · 28 unmodified lines | ||
| 1062 | 1050 | out_reg: |
| 1063 | 1051 | /* Up the trigger_data count to make sure reg doesn't free it on failure */ |
| 1064 | 1052 | event_trigger_init(trigger_ops, trigger_data); |
| 1065 | − ret = cmd_ops->reg(glob, trigger_data, file); | |
| 1066 | − /* | |
| 1067 | − * The above returns on success the # of functions enabled, | |
| 1068 | − * but if it didn't find any functions it returns zero. | |
| 1069 | − * Consider no functions a failure too. | |
| 1070 | − */ | |
| 1071 | − if (!ret) { | |
| 1072 | − cmd_ops->unreg(glob, trigger_data, file); | |
| 1073 | − ret = -ENOENT; | |
| 1074 | − } else if (ret > 0) | |
| 1075 | − ret = 0; | |
| 1053 | + | |
| 1054 | + ret = event_trigger_register(cmd_ops, file, glob, trigger_data); | |
| 1055 | + if (ret) | |
| 1056 | + goto out_free; | |
| 1076 | 1057 | |
| 1077 | 1058 | /* Down the counter of trigger_data or free it if not used anymore */ |
| 1078 | 1059 | event_trigger_free(trigger_ops, trigger_data); |
| @@ -1854,7 +1835,7 @@ int event_enable_trigger_parse(struct event_command *cmd_ops, · 775 unmodified lines | ||
| 1854 | 1835 | trigger_data->private_data = enable_data; |
| 1855 | 1836 | |
| 1856 | 1837 | if (glob[0] == '!') { |
| 1857 | − cmd_ops->unreg(glob+1, trigger_data, file); | |
| 1838 | + event_trigger_unregister(cmd_ops, file, glob+1, trigger_data); | |
| 1858 | 1839 | kfree(trigger_data); |
| 1859 | 1840 | kfree(enable_data); |
| 1860 | 1841 | ret = 0; |
| @@ -1901,19 +1882,11 @@ int event_enable_trigger_parse(struct event_command *cmd_ops, · 40 unmodified lines | ||
| 1901 | 1882 | ret = trace_event_enable_disable(event_enable_file, 1, 1); |
| 1902 | 1883 | if (ret < 0) |
| 1903 | 1884 | goto out_put; |
| 1904 | − ret = cmd_ops->reg(glob, trigger_data, file); | |
| 1905 | − /* | |
| 1906 | − * The above returns on success the # of functions enabled, | |
| 1907 | − * but if it didn't find any functions it returns zero. | |
| 1908 | − * Consider no functions a failure too. | |
| 1909 | − */ | |
| 1910 | − if (!ret) { | |
| 1911 | − ret = -ENOENT; | |
| 1912 | − goto out_disable; | |
| 1913 | − } else if (ret < 0) | |
| 1885 | + | |
| 1886 | + ret = event_trigger_register(cmd_ops, file, glob, trigger_data); | |
| 1887 | + if (ret) | |
| 1914 | 1888 | goto out_disable; |
| 1915 | − /* Just return zero, not the number of enabled functions */ | |
| 1916 | − ret = 0; | |
| 1889 | + | |
| 1917 | 1890 | event_trigger_free(trigger_ops, trigger_data); |
| 1918 | 1891 | out: |
| 1919 | 1892 | return ret; |
| @@ -1959,13 +1932,12 @@ int event_enable_register_trigger(char *glob, · 39 unmodified lines | ||
| 1959 | 1932 | } |
| 1960 | 1933 | |
| 1961 | 1934 | list_add_rcu(&data->list, &file->triggers); |
| 1962 | − ret++; | |
| 1963 | 1935 | |
| 1964 | 1936 | update_cond_flag(file); |
| 1965 | − if (trace_event_trigger_enable_disable(file, 1) < 0) { | |
| 1937 | + ret = trace_event_trigger_enable_disable(file, 1); | |
| 1938 | + if (ret < 0) { | |
| 1966 | 1939 | list_del_rcu(&data->list); |
| 1967 | 1940 | update_cond_flag(file); |
| 1968 | − ret--; | |
| 1969 | 1941 | } |
| 1970 | 1942 | out: |
| 1971 | 1943 | return ret; |