[ltt-dev] [UST PATCH] Introduce a new communication protocol for UST v3

[David Goulet]

Almost there :)

(I've cut a bit of code for you to stop searching my comments :P)

On 10-10-20 04:51 AM, Nils Carlson wrote:

> diff --git a/ustctl/ustctl.c b/ustctl/ustctl.c
> index bf149d1..07630dc 100644
> --- a/ustctl/ustctl.c
> +++ b/ustctl/ustctl.c
> @@ -169,6 +169,59 @@ int parse_opts_long(int argc, char **argv, struct ust_opts *opts)
>   	return 0;
>   }
>
> +static int scan_ch_marker(const char *channel_marker, char **channel,
> +			char **marker)
> +{
> +	int result;
> +
> +	*channel = NULL;
> +	*marker = NULL;
> +
> +	result = sscanf(channel_marker, "%a[^/]/%as", channel, marker);

The result handling here seems weird to me. I read the man page of 
sscanf and with EOF value *and* 0, it's an error. However EOF = -1 and 
not 0 like the first ERR suggest.

Should only check result != 2 instead, use PERROR (and not ERR) for the 
appropriate errno message and return an error (here -1 but soon a ust 
error code) ? (Other function across ustctl behave like that)

> +	if (result == 0) {
> +		ERR("Failed to parse marker and channel names, got EOF");
> +		return -1;
> +	} else if (result<  0) {
> +		PERROR("Failed to parse marker and channel names");
> +		return -1;
> +	} else if (result != 2) {
> +		ERR("Failed to parse marker and channel names");

Just saw that :

channel and marker should be *channel and *marker on free() because that 
way, they are valid pointer each time.

> +		if (channel) {
> +			free(channel);
> +		}
> +		if (marker) {
> +			free(marker);
> +		}
> +		return -1;

This final else if is a bit useless because sscanf can *only* return 
fewer match than provided... so we only need a return 0 if we do not 
match !=2. However, let do this on the next iteration fixing error 
handling across UST.

> +	} else if (result == 2) {
> +		return 0;
> +	}
> +}
> +

We should really update the man page for ERRORS because, as an example, 
if a marker gets enabled a second time, we have a EEXIST (File Exist) 
code output on the ustctl side... might be a bit confusing :)

Thanks!
-- 
David Goulet
LTTng project, DORSAL Lab.

PGP/GPG : 1024D/16BD8563
BE3C 672B 9331 9796 291A  14C6 4AF7 C14B 16BD 8563