Re: [PATCH 1/2] lttng: Fix /proc/ltt parser

* Jan Kiszka ([email protected]) wrote:
> The parser for /proc/ltt (*) is borken, at least the describing comment
> and the actual implementation diverge and the latter is unhandy. Find a
> rework of ltt-marker-control.c below. It implements the following
> interface:
> 
> connect <marker name> [<probe name> [compact|dynamic [<channel>]]]
> disconnect <marker name> [<probe name>]
> set_id <marker name> [<probe name>] compact|dynamic
> set_channel <marker name> [<probe name>] <channel>
> 
> If <probe name> is not given, the default probe is assumed.
> 
> The patch also removes a few pointless "inline" attributes. The compiler
> should be able to decide about the right way.
> 
> Signed-off-by: Jan Kiszka <[email protected]>
> 

That a very good cleanup, thanks a lot! Applying.

> 
> (*) Is /proc the right place for this thing at all? Shouldn't this be
> debugfs as well?
> 

Hrm, that's a good question. I thought listing the markers present in
the kernel was a bit like listing any other internal kernel data
structure (like the kernel modules, or connected interrupts). And
eventually we will have markers in userspace processes, so
/proc/PID/markers will become helpful.

Also, it would make the whole LTTng depend on debugfs. Currently, only
ltt-relay.ko depends on it. If we create a different transport, we can
hook it differently.

So I would wait until people on LKML object before starting the "let's
change filesystem just for fun" game. It's already been done many time
in the original LTT. Considering this past experience, I tend to wait
until we get full feedback before changing the FS used for exported
kernel API.

Mathieu

> ---
>  ltt/ltt-marker-control.c |  128 ++++++++++++++++++++---------------------------
>  1 file changed, 57 insertions(+), 71 deletions(-)
> 
> Index: b/ltt/ltt-marker-control.c
> ===================================================================
> --- a/ltt/ltt-marker-control.c
> +++ b/ltt/ltt-marker-control.c
> @@ -87,11 +87,13 @@ static struct id_name_info {
>  
>  static struct file_operations ltt_fops;
>  
> -static inline struct ltt_available_probe *get_probe_from_name(const char *pname)
> +static struct ltt_available_probe *get_probe_from_name(const char *pname)
>  {
>  	struct ltt_available_probe *iter;
>  	int comparison, found = 0;
>  
> +	if (!pname)
> +		pname = DEFAULT_PROBE;
>  	list_for_each_entry(iter, &probes_registered_list, node) {
>  		comparison = strcmp(pname, iter->name);
>  		if (!comparison)
> @@ -105,7 +107,7 @@ static inline struct ltt_available_probe
>  		return NULL;
>  }
>  
> -static inline int get_channel_index_from_name(const char *name)
> +static int get_channel_index_from_name(const char *name)
>  {
>  	struct chan_name_info *info;
>  
> @@ -119,7 +121,7 @@ static inline int get_channel_index_from
>  	return -ENOENT;
>  }
>  
> -static inline enum marker_id get_id_from_name(const char *name)
> +static enum marker_id get_id_from_name(const char *name)
>  {
>  	struct id_name_info *info;
>  
> @@ -132,14 +134,14 @@ static inline enum marker_id get_id_from
>  	return -ENOENT;
>  }
>  
> -static inline char *skip_spaces(char *buf)
> +static char *skip_spaces(char *buf)
>  {
>  	while (*buf != '\0' && isspace(*buf))
>  		buf++;
>  	return buf;
>  }
>  
> -static inline char *skip_nonspaces(char *buf)
> +static char *skip_nonspaces(char *buf)
>  {
>  	while (*buf != '\0' && !isspace(*buf))
>  		buf++;
> @@ -609,12 +611,12 @@ EXPORT_SYMBOL_GPL(ltt_dump_marker_state)
>  /*
>   * function handling proc entry write.
>   *
> - * connect marker_name [probe_name] [id type (compact/dynamic)] [channel]
> - * disconnect marker_name
> + * connect <marker name> [<probe name> [compact|dynamic [<channel>]]]
> + * disconnect <marker name> [<probe name>]
>   *
>   * Actions can be done on a connected marker:
> - * set_id marker_name compact/dynamic
> - * set_channel marker_name channel
> + * set_id <marker name> [<probe name>] compact|dynamic
> + * set_channel <marker name> [<probe name>] <channel>
>   *
>   * note : when a core probe registers, it directly establishes the connexion.
>   * Cannot be disconnected by users.
> @@ -623,11 +625,11 @@ static ssize_t ltt_write(struct file *fi
>  			   size_t count, loff_t *offset)
>  {
>  	char *kbuf;
> -	char *iter, *marker_action, *marker_name = NULL, *probe_name = NULL,
> -			*id = NULL, *channel = NULL;
> +	char *iter, *marker_action, *arg[4];
>  	ssize_t ret;
>  	int channel_index;
>  	enum marker_id n_id;
> +	int i;
>  
>  	if (!count)
>  		return -EINVAL;
> @@ -644,88 +646,72 @@ static ssize_t ltt_write(struct file *fi
>  		ret = -EINVAL;
>  		goto end;
>  	}
> -	if (iter < kbuf + count) {
> -		iter++;			/* skip the added '\0' */
> -		get_marker_string(iter, &marker_name, &iter);
> -		if (marker_name == iter)
> -			marker_name = NULL;
> -	}
> -	if (iter < kbuf + count) {
> -		iter++;			/* skip the added '\0' */
> -		get_marker_string(iter, &probe_name, &iter);
> -		if (probe_name == iter)
> -			probe_name = DEFAULT_PROBE;
> -	}
> -	if (iter < kbuf + count) {
> -		iter++;			/* skip the added '\0' */
> -		get_marker_string(iter, &id, &iter);
> -		if (id == iter)
> -			id = NULL;
> -	}
> -	if (iter < kbuf + count) {
> -		iter++;			/* skip the added '\0' */
> -		get_marker_string(iter, &channel, &iter);
> -		if (channel == iter)
> -			channel = NULL;
> +	for (i = 0; i < 4; i++) {
> +		arg[i] = NULL;
> +		if (iter < kbuf + count) {
> +			iter++;			/* skip the added '\0' */
> +			get_marker_string(iter, &arg[i], &iter);
> +			if (arg[i] == iter)
> +				arg[i] = NULL;
> +		}
>  	}
>  
> -	printk(KERN_DEBUG "Marker control : '%s' '%s' id: %s channel: %s\n",
> -			marker_action, marker_name, id, channel);
> -
> -	if (!marker_name) {
> +	if (!arg[0]) {
>  		ret = -EINVAL;
>  		goto end;
>  	}
>  
>  	if (!strcmp(marker_action, "connect")) {
> -		channel_index = get_channel_index_from_name(channel);
> -		if (channel_index < 0) {
> -			ret = channel_index;
> -			goto end;
> -		}
> -		n_id = get_id_from_name(id);
> +		n_id = get_id_from_name(arg[2]);
>  		if (n_id < 0) {
>  			ret = n_id;
>  			goto end;
>  		}
> -		ret = ltt_marker_connect(marker_name, probe_name, n_id,
> -					(uint16_t)channel_index, 1, 1);
> +		channel_index = get_channel_index_from_name(arg[3]);
> +		if (channel_index < 0) {
> +			ret = channel_index;
> +			goto end;
> +		}
> +		ret = ltt_marker_connect(arg[0], arg[1], n_id,
> +					 (uint16_t)channel_index, 1, 1);
>  		if (ret)
>  			goto end;
>  	} else if (!strcmp(marker_action, "disconnect")) {
> -		ret = ltt_marker_disconnect(marker_name, probe_name, 1);
> +		ret = ltt_marker_disconnect(arg[0], arg[1], 1);
>  		if (ret)
>  			goto end;
> -	} else if (!strcmp(marker_action, "set_id")) {
> -		if (!id) {
> -			ret = -EINVAL;
> -			goto end;
> -		}
> -		n_id = get_id_from_name(id);
> -		if (n_id < 0) {
> -			ret = n_id;
> -			goto end;
> +	} else {
> +		if (!arg[2]) {
> +			arg[2] = arg[1];
> +			arg[1] = DEFAULT_PROBE;
>  		}
> -		ret = do_set_id(marker_name, probe_name, n_id, 1);
> -		if (ret)
> -			goto end;
> -	} else if (!strcmp(marker_action, "set_channel")) {
> -		if (!channel) {
> +		if (!arg[2]) {
>  			ret = -EINVAL;
>  			goto end;
>  		}
> -		channel_index = get_channel_index_from_name(channel);
> -		if (channel_index < 0) {
> -			ret = channel_index;
> +		if (!strcmp(marker_action, "set_id")) {
> +			n_id = get_id_from_name(arg[2]);
> +			if (n_id < 0) {
> +				ret = n_id;
> +				goto end;
> +			}
> +			ret = do_set_id(arg[0], arg[1], n_id, 1);
> +			if (ret)
> +				goto end;
> +		} else if (!strcmp(marker_action, "set_channel")) {
> +			channel_index = get_channel_index_from_name(arg[2]);
> +			if (channel_index < 0) {
> +				ret = channel_index;
> +				goto end;
> +			}
> +			ret = do_set_channel(arg[0], arg[1],
> +					     (uint16_t)channel_index, 1);
> +			if (ret)
> +				goto end;
> +		} else {
> +			ret = -EINVAL;
>  			goto end;
>  		}
> -		ret = do_set_channel(marker_name, probe_name,
> -			(uint16_t)channel_index, 1);
> -		if (ret)
> -			goto end;
> -	} else {
> -		ret = -EINVAL;
> -		goto end;
>  	}
>  	ret = count;
>  end:
> _______________________________________________
> Ltt-dev mailing list
> [email protected]
> http://listserv.shafik.org/mailman/listinfo/ltt-dev
> 

-- 
Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68