[lttng-dev] [PATCH lttng-ust 4/8] Add probe provider unregister function

[Mathieu Desnoyers]

----- On Feb 2, 2018, at 2:48 PM, Francis Deslauriers francis.deslauriers at efficios.com wrote:

> Signed-off-by: Francis Deslauriers <francis.deslauriers at efficios.com>
> ---
> include/lttng/ust-events.h  |  1 +
> liblttng-ust/lttng-events.c | 89 +++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 90 insertions(+)
> 
> diff --git a/include/lttng/ust-events.h b/include/lttng/ust-events.h
> index caf7e63..019b0eb 100644
> --- a/include/lttng/ust-events.h
> +++ b/include/lttng/ust-events.h
> @@ -656,6 +656,7 @@ void synchronize_trace(void);
> 
> int lttng_probe_register(struct lttng_probe_desc *desc);
> void lttng_probe_unregister(struct lttng_probe_desc *desc);
> +void lttng_probe_provider_unregister_events(struct lttng_probe_desc *desc);
> int lttng_fix_pending_events(void);
> int lttng_probes_init(void);
> void lttng_probes_exit(void);
> diff --git a/liblttng-ust/lttng-events.c b/liblttng-ust/lttng-events.c
> index 7419f78..e8d4857 100644
> --- a/liblttng-ust/lttng-events.c
> +++ b/liblttng-ust/lttng-events.c
> @@ -789,6 +789,95 @@ void lttng_create_event_if_missing(struct lttng_enabler
> *enabler)
> }
> 
> /*
> + * Iterate over all the UST sessions to unregister and destroy all probes from
> + * the probe provider descriptor passed in arguments. Must me called with the

passed in arguments -> received as argument

> + * ust_lock held.
> + */
> +void lttng_probe_provider_unregister_events(struct lttng_probe_desc
> *provider_desc)
> +{
> +	int i;

move int i; to the last variable (shortest line).

> +	struct cds_list_head *sessionsp;
> +	struct lttng_session *session;
> +	struct cds_hlist_head *head;
> +	struct cds_hlist_node *node;
> +	struct lttng_event *event;
> +
> +	/* Get handle on list of sessions. */
> +	sessionsp = _lttng_get_sessions();
> +
> +	/*
> +	 * Iterate over all events in the probe provider descriptions and sessions
> +	 * to queue the unregistration of the events.
> +	 */
> +	for (i = 0; i < provider_desc->nr_events; i++) {
> +		const char *event_name;
> +		uint32_t hash;
> +		size_t name_len;
> +		const struct lttng_event_desc *event_desc;

same here about var definition layout.

> +
> +		event_desc = provider_desc->event_desc[i];
> +		event_name = event_desc->name;
> +		name_len = strlen(event_name);
> +		hash = jhash(event_name, name_len, 0);
> +
> +		/* Iterate over all session to find the current event description. */
> +		cds_list_for_each_entry(session, sessionsp, node) {

remove whiteline.

> +
> +			/*
> +			 * Get the list of events in the hashtable bucket and iterate to
> +			 * find the event matching this descriptor.
> +			 */
> +			head = &session->events_ht.table[hash & (LTTNG_UST_EVENT_HT_SIZE - 1)];
> +			cds_hlist_for_each_entry(event, node, head, hlist) {
> +				if (event_desc == event->desc) {
> +
> +					/* Queue the unregistration of this event. */
> +					_lttng_event_unregister(event);
> +					break;
> +				}
> +			}
> +		}
> +	}
> +
> +	/* Wait for grace period. */
> +	synchronize_trace();
> +	/* Prune the unregistration queue. */
> +	__tracepoint_probe_prune_release_queue();
> +
> +	/*
> +	 * It is now safe to destroy the events and remove them from the event list
> +	 * and hashtables.
> +	 */
> +	for (i = 0; i < provider_desc->nr_events; i++) {
> +		const char *event_name;
> +		uint32_t hash;
> +		size_t name_len;
> +		const struct lttng_event_desc *event_desc;

same.

> +
> +		event_desc = provider_desc->event_desc[i];
> +		event_name = event_desc->name;
> +		name_len = strlen(event_name);
> +		hash = jhash(event_name, name_len, 0);
> +



> +		/* Iterate over all sessions to find the current event description. */
> +		cds_list_for_each_entry(session, sessionsp, node) {

whiteline.

> +
> +			/*
> +			 * Get the list of events in the hashtable bucket and iterate to
> +			 * find the event matching this descriptor.
> +			 */

beware for the iteration below: you may need to use a cds_list_for_each_entry_safe()
if you can remove the list entry while you iterate.

> +			head = &session->events_ht.table[hash & (LTTNG_UST_EVENT_HT_SIZE - 1)];
> +			cds_hlist_for_each_entry(event, node, head, hlist) {
> +				if (event_desc == event->desc) {
> +					_lttng_event_destroy(event);

you have a "break" here. Is there a possibility that you can find more than one match ?

Thanks,

Mathieu

> +					break;
> +				}
> +			}
> +		}
> +	}
> +}
> +
> +/*
>  * Create events associated with an enabler (if not already present),
>  * and add backward reference from the event to the enabler.
>  */
> --
> 2.7.4

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com