[PATCH lttng-tools] Better error reporting when a snapshot record size is too small

[PATCH lttng-tools] Better error reporting when a snapshot record size is too small

[Jérémie Galarneau]

Print an error message when a snapshot record is made with a
max size smaller than the subbuffers. This limitation is now
documented in the man page.

Signed-off-by: Jérémie Galarneau <jeremie.galarneau@efficios.com>
---
 doc/man/lttng.1                   |  4 ++-
 src/bin/lttng/commands/snapshot.c | 59 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 62 insertions(+), 1 deletion(-)

diff --git a/doc/man/lttng.1 b/doc/man/lttng.1
index 972f71c..bf1b67e 100644
--- a/doc/man/lttng.1
+++ b/doc/man/lttng.1
@@ -787,7 +787,9 @@ List the output of a session. Attributes of the output are printed.
 Snapshot a session's buffer(s) for all domains. If an URL is specified, it is
 used instead of a previously added output. Specifying only a name or/and a max
 size will override the current output values. For instance, you can record a
-snapshot with a custom maximum size or with a different name.
+snapshot with a custom maximum size or with a different name. However, the
+max size must be high enough to contain a complete subbuffer. See the
+--subbuf-size switch for default subbuffer sizes.
 
 .nf
 $ lttng snapshot add-output -n mysnapshot file:///data/snapshot
diff --git a/src/bin/lttng/commands/snapshot.c b/src/bin/lttng/commands/snapshot.c
index c704eee..7fda949 100644
--- a/src/bin/lttng/commands/snapshot.c
+++ b/src/bin/lttng/commands/snapshot.c
@@ -125,6 +125,56 @@ static int count_arguments(const char **argv)
 	return i;
 }
 
+static uint64_t get_largest_subbuf()
+{
+	int domain_count;
+	int channel_count;
+	int domain_idx;
+	int channel_idx;
+	struct lttng_domain *domains;
+	uint64_t largest_subbuf = 0;
+
+	domain_count = lttng_list_domains(current_session_name, &domains);
+	if (domain_count < 0) {
+		ERR("Unable to list session %s's domains",
+			current_session_name);
+		goto end;
+	}
+
+	for (domain_idx = 0; domain_idx < domain_count; domain_idx++) {
+		struct lttng_channel *channels;
+		struct lttng_handle *handle = lttng_create_handle(
+			current_session_name, &domains[domain_idx]);
+
+		if (!handle) {
+			ERR("Unable to create handle for session %s",
+				current_session_name);
+			goto end;
+		}
+
+		channel_count = lttng_list_channels(handle, &channels);
+		if (channel_count < 0) {
+			ERR("Unable to list channels for session %s",
+				current_session_name);
+			goto end;
+		}
+
+		for (channel_idx = 0; channel_idx < channel_count;
+			channel_idx++) {
+			if (channels[channel_idx].attr.subbuf_size >
+				largest_subbuf) {
+				largest_subbuf =
+					channels[channel_idx].attr.subbuf_size;
+			}
+		}
+		free(channels);
+		lttng_destroy_handle(handle);
+	}
+end:
+	free(domains);
+	return largest_subbuf;
+}
+
 /*
  * Create a snapshot output object from arguments using the given URL.
  *
@@ -160,6 +210,15 @@ static struct lttng_snapshot_output *create_output_from_args(const char *url)
 	}
 
 	if (opt_max_size) {
+		/* Validate that the max size can contain one subbuffer. */
+		uint64_t largest_subbuf = get_largest_subbuf();
+		if (largest_subbuf == 0) {
+			goto error;
+		} else if (largest_subbuf > opt_max_size) {
+			ERR("Snapshot size must be greater or equal to the largest subbuffer's size (%zu).",
+				largest_subbuf);
+			goto error;
+		}
 		ret = lttng_snapshot_output_set_size(opt_max_size, output);
 		if (ret < 0) {
 			goto error;
-- 
1.9.1


_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

Re: [PATCH lttng-tools] Better error reporting when a snapshot record size is too small

[David Goulet]

[-- Attachment #1.1: Type: text/plain, Size: 6460 bytes --]

On 15 Apr (11:14:18), Jérémie Galarneau wrote:
> On Tue, Apr 15, 2014 at 10:44 AM, David Goulet <dgoulet@efficios.com> wrote:
> > On 15 Apr (10:32:09), Jérémie Galarneau wrote:
> >> Print an error message when a snapshot record is made with a
> >> max size smaller than the subbuffers. This limitation is now
> >> documented in the man page.
> >
> > Why is this not done on the session daemon side? Like "Oh you provided
> > me with a wrong max size" type of error message.
> >
> > Because here this fixes the issue only on the command line side but a
> > user using the API would still fail somehow silently or ... ?
> >
> 
> The API call could return something like
> -LTTNG_ERR_SNAPSHOT_TOO_SMALL... It sure would be better than what we
> have now (a DBG3(...) message on the sessiond side).
> 
> > Not that I don't like the approach here but I just need to know if there
> > is a specific reason it's done on the lttng client side only. This patch
> > adds two calls that can exchange quite a bit of data with the sessiond
> > (list domains and channels) so unless there is no other way, I would go
> > for a sessiond side validation instead.
> >
> 
> Well, I think clients will end up implementing the same logic
> anyway... If the record call fails because the max size is too small,
> API users will query the sessiond to know what size would fit.
> Ideally, the lttng client will also report the smallest suitable size
> in the error message (like it does in my patch).
> 
> I understand your concern about data exchanges delaying the snapshot,
> though. What I'd propose is to report the new -LTTNG_ERR_SNAPSHOT_...
> code from the API, and have the client report the smallest size
> possible size to the user using get_largest_subbuf() when such an
> error occurs. What do you think?

Perfect solution for me!

David

> 
> Thanks,
> Jérémie
> 
> > Cheers!
> > David
> >
> >>
> >> Signed-off-by: Jérémie Galarneau <jeremie.galarneau@efficios.com>
> >> ---
> >>  doc/man/lttng.1                   |  4 ++-
> >>  src/bin/lttng/commands/snapshot.c | 59 +++++++++++++++++++++++++++++++++++++++
> >>  2 files changed, 62 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/doc/man/lttng.1 b/doc/man/lttng.1
> >> index 972f71c..bf1b67e 100644
> >> --- a/doc/man/lttng.1
> >> +++ b/doc/man/lttng.1
> >> @@ -787,7 +787,9 @@ List the output of a session. Attributes of the output are printed.
> >>  Snapshot a session's buffer(s) for all domains. If an URL is specified, it is
> >>  used instead of a previously added output. Specifying only a name or/and a max
> >>  size will override the current output values. For instance, you can record a
> >> -snapshot with a custom maximum size or with a different name.
> >> +snapshot with a custom maximum size or with a different name. However, the
> >> +max size must be high enough to contain a complete subbuffer. See the
> >> +--subbuf-size switch for default subbuffer sizes.
> >>
> >>  .nf
> >>  $ lttng snapshot add-output -n mysnapshot file:///data/snapshot
> >> diff --git a/src/bin/lttng/commands/snapshot.c b/src/bin/lttng/commands/snapshot.c
> >> index c704eee..7fda949 100644
> >> --- a/src/bin/lttng/commands/snapshot.c
> >> +++ b/src/bin/lttng/commands/snapshot.c
> >> @@ -125,6 +125,56 @@ static int count_arguments(const char **argv)
> >>       return i;
> >>  }
> >>
> >> +static uint64_t get_largest_subbuf()
> >> +{
> >> +     int domain_count;
> >> +     int channel_count;
> >> +     int domain_idx;
> >> +     int channel_idx;
> >> +     struct lttng_domain *domains;
> >> +     uint64_t largest_subbuf = 0;
> >> +
> >> +     domain_count = lttng_list_domains(current_session_name, &domains);
> >> +     if (domain_count < 0) {
> >> +             ERR("Unable to list session %s's domains",
> >> +                     current_session_name);
> >> +             goto end;
> >> +     }
> >> +
> >> +     for (domain_idx = 0; domain_idx < domain_count; domain_idx++) {
> >> +             struct lttng_channel *channels;
> >> +             struct lttng_handle *handle = lttng_create_handle(
> >> +                     current_session_name, &domains[domain_idx]);
> >> +
> >> +             if (!handle) {
> >> +                     ERR("Unable to create handle for session %s",
> >> +                             current_session_name);
> >> +                     goto end;
> >> +             }
> >> +
> >> +             channel_count = lttng_list_channels(handle, &channels);
> >> +             if (channel_count < 0) {
> >> +                     ERR("Unable to list channels for session %s",
> >> +                             current_session_name);
> >> +                     goto end;
> >> +             }
> >> +
> >> +             for (channel_idx = 0; channel_idx < channel_count;
> >> +                     channel_idx++) {
> >> +                     if (channels[channel_idx].attr.subbuf_size >
> >> +                             largest_subbuf) {
> >> +                             largest_subbuf =
> >> +                                     channels[channel_idx].attr.subbuf_size;
> >> +                     }
> >> +             }
> >> +             free(channels);
> >> +             lttng_destroy_handle(handle);
> >> +     }
> >> +end:
> >> +     free(domains);
> >> +     return largest_subbuf;
> >> +}
> >> +
> >>  /*
> >>   * Create a snapshot output object from arguments using the given URL.
> >>   *
> >> @@ -160,6 +210,15 @@ static struct lttng_snapshot_output *create_output_from_args(const char *url)
> >>       }
> >>
> >>       if (opt_max_size) {
> >> +             /* Validate that the max size can contain one subbuffer. */
> >> +             uint64_t largest_subbuf = get_largest_subbuf();
> >> +             if (largest_subbuf == 0) {
> >> +                     goto error;
> >> +             } else if (largest_subbuf > opt_max_size) {
> >> +                     ERR("Snapshot size must be greater or equal to the largest subbuffer's size (%zu).",
> >> +                             largest_subbuf);
> >> +                     goto error;
> >> +             }
> >>               ret = lttng_snapshot_output_set_size(opt_max_size, output);
> >>               if (ret < 0) {
> >>                       goto error;
> >> --
> >> 1.9.1
> >>
> 
> 
> 
> -- 
> Jérémie Galarneau
> EfficiOS Inc.
> http://www.efficios.com

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 603 bytes --]

[-- Attachment #2: Type: text/plain, Size: 155 bytes --]

_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

Re: [PATCH lttng-tools] Better error reporting when a snapshot record size is too small

[Jérémie Galarneau]

On Tue, Apr 15, 2014 at 10:44 AM, David Goulet <dgoulet@efficios.com> wrote:
> On 15 Apr (10:32:09), Jérémie Galarneau wrote:
>> Print an error message when a snapshot record is made with a
>> max size smaller than the subbuffers. This limitation is now
>> documented in the man page.
>
> Why is this not done on the session daemon side? Like "Oh you provided
> me with a wrong max size" type of error message.
>
> Because here this fixes the issue only on the command line side but a
> user using the API would still fail somehow silently or ... ?
>

The API call could return something like
-LTTNG_ERR_SNAPSHOT_TOO_SMALL... It sure would be better than what we
have now (a DBG3(...) message on the sessiond side).

> Not that I don't like the approach here but I just need to know if there
> is a specific reason it's done on the lttng client side only. This patch
> adds two calls that can exchange quite a bit of data with the sessiond
> (list domains and channels) so unless there is no other way, I would go
> for a sessiond side validation instead.
>

Well, I think clients will end up implementing the same logic
anyway... If the record call fails because the max size is too small,
API users will query the sessiond to know what size would fit.
Ideally, the lttng client will also report the smallest suitable size
in the error message (like it does in my patch).

I understand your concern about data exchanges delaying the snapshot,
though. What I'd propose is to report the new -LTTNG_ERR_SNAPSHOT_...
code from the API, and have the client report the smallest size
possible size to the user using get_largest_subbuf() when such an
error occurs. What do you think?

Thanks,
Jérémie

> Cheers!
> David
>
>>
>> Signed-off-by: Jérémie Galarneau <jeremie.galarneau@efficios.com>
>> ---
>>  doc/man/lttng.1                   |  4 ++-
>>  src/bin/lttng/commands/snapshot.c | 59 +++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 62 insertions(+), 1 deletion(-)
>>
>> diff --git a/doc/man/lttng.1 b/doc/man/lttng.1
>> index 972f71c..bf1b67e 100644
>> --- a/doc/man/lttng.1
>> +++ b/doc/man/lttng.1
>> @@ -787,7 +787,9 @@ List the output of a session. Attributes of the output are printed.
>>  Snapshot a session's buffer(s) for all domains. If an URL is specified, it is
>>  used instead of a previously added output. Specifying only a name or/and a max
>>  size will override the current output values. For instance, you can record a
>> -snapshot with a custom maximum size or with a different name.
>> +snapshot with a custom maximum size or with a different name. However, the
>> +max size must be high enough to contain a complete subbuffer. See the
>> +--subbuf-size switch for default subbuffer sizes.
>>
>>  .nf
>>  $ lttng snapshot add-output -n mysnapshot file:///data/snapshot
>> diff --git a/src/bin/lttng/commands/snapshot.c b/src/bin/lttng/commands/snapshot.c
>> index c704eee..7fda949 100644
>> --- a/src/bin/lttng/commands/snapshot.c
>> +++ b/src/bin/lttng/commands/snapshot.c
>> @@ -125,6 +125,56 @@ static int count_arguments(const char **argv)
>>       return i;
>>  }
>>
>> +static uint64_t get_largest_subbuf()
>> +{
>> +     int domain_count;
>> +     int channel_count;
>> +     int domain_idx;
>> +     int channel_idx;
>> +     struct lttng_domain *domains;
>> +     uint64_t largest_subbuf = 0;
>> +
>> +     domain_count = lttng_list_domains(current_session_name, &domains);
>> +     if (domain_count < 0) {
>> +             ERR("Unable to list session %s's domains",
>> +                     current_session_name);
>> +             goto end;
>> +     }
>> +
>> +     for (domain_idx = 0; domain_idx < domain_count; domain_idx++) {
>> +             struct lttng_channel *channels;
>> +             struct lttng_handle *handle = lttng_create_handle(
>> +                     current_session_name, &domains[domain_idx]);
>> +
>> +             if (!handle) {
>> +                     ERR("Unable to create handle for session %s",
>> +                             current_session_name);
>> +                     goto end;
>> +             }
>> +
>> +             channel_count = lttng_list_channels(handle, &channels);
>> +             if (channel_count < 0) {
>> +                     ERR("Unable to list channels for session %s",
>> +                             current_session_name);
>> +                     goto end;
>> +             }
>> +
>> +             for (channel_idx = 0; channel_idx < channel_count;
>> +                     channel_idx++) {
>> +                     if (channels[channel_idx].attr.subbuf_size >
>> +                             largest_subbuf) {
>> +                             largest_subbuf =
>> +                                     channels[channel_idx].attr.subbuf_size;
>> +                     }
>> +             }
>> +             free(channels);
>> +             lttng_destroy_handle(handle);
>> +     }
>> +end:
>> +     free(domains);
>> +     return largest_subbuf;
>> +}
>> +
>>  /*
>>   * Create a snapshot output object from arguments using the given URL.
>>   *
>> @@ -160,6 +210,15 @@ static struct lttng_snapshot_output *create_output_from_args(const char *url)
>>       }
>>
>>       if (opt_max_size) {
>> +             /* Validate that the max size can contain one subbuffer. */
>> +             uint64_t largest_subbuf = get_largest_subbuf();
>> +             if (largest_subbuf == 0) {
>> +                     goto error;
>> +             } else if (largest_subbuf > opt_max_size) {
>> +                     ERR("Snapshot size must be greater or equal to the largest subbuffer's size (%zu).",
>> +                             largest_subbuf);
>> +                     goto error;
>> +             }
>>               ret = lttng_snapshot_output_set_size(opt_max_size, output);
>>               if (ret < 0) {
>>                       goto error;
>> --
>> 1.9.1
>>



-- 
Jérémie Galarneau
EfficiOS Inc.
http://www.efficios.com

_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

Re: [PATCH lttng-tools] Better error reporting when a snapshot record size is too small

[David Goulet]

[-- Attachment #1.1: Type: text/plain, Size: 4300 bytes --]

On 15 Apr (10:32:09), Jérémie Galarneau wrote:
> Print an error message when a snapshot record is made with a
> max size smaller than the subbuffers. This limitation is now
> documented in the man page.

Why is this not done on the session daemon side? Like "Oh you provided
me with a wrong max size" type of error message.

Because here this fixes the issue only on the command line side but a
user using the API would still fail somehow silently or ... ?

Not that I don't like the approach here but I just need to know if there
is a specific reason it's done on the lttng client side only. This patch
adds two calls that can exchange quite a bit of data with the sessiond
(list domains and channels) so unless there is no other way, I would go
for a sessiond side validation instead.

Cheers!
David

> 
> Signed-off-by: Jérémie Galarneau <jeremie.galarneau@efficios.com>
> ---
>  doc/man/lttng.1                   |  4 ++-
>  src/bin/lttng/commands/snapshot.c | 59 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 62 insertions(+), 1 deletion(-)
> 
> diff --git a/doc/man/lttng.1 b/doc/man/lttng.1
> index 972f71c..bf1b67e 100644
> --- a/doc/man/lttng.1
> +++ b/doc/man/lttng.1
> @@ -787,7 +787,9 @@ List the output of a session. Attributes of the output are printed.
>  Snapshot a session's buffer(s) for all domains. If an URL is specified, it is
>  used instead of a previously added output. Specifying only a name or/and a max
>  size will override the current output values. For instance, you can record a
> -snapshot with a custom maximum size or with a different name.
> +snapshot with a custom maximum size or with a different name. However, the
> +max size must be high enough to contain a complete subbuffer. See the
> +--subbuf-size switch for default subbuffer sizes.
>  
>  .nf
>  $ lttng snapshot add-output -n mysnapshot file:///data/snapshot
> diff --git a/src/bin/lttng/commands/snapshot.c b/src/bin/lttng/commands/snapshot.c
> index c704eee..7fda949 100644
> --- a/src/bin/lttng/commands/snapshot.c
> +++ b/src/bin/lttng/commands/snapshot.c
> @@ -125,6 +125,56 @@ static int count_arguments(const char **argv)
>  	return i;
>  }
>  
> +static uint64_t get_largest_subbuf()
> +{
> +	int domain_count;
> +	int channel_count;
> +	int domain_idx;
> +	int channel_idx;
> +	struct lttng_domain *domains;
> +	uint64_t largest_subbuf = 0;
> +
> +	domain_count = lttng_list_domains(current_session_name, &domains);
> +	if (domain_count < 0) {
> +		ERR("Unable to list session %s's domains",
> +			current_session_name);
> +		goto end;
> +	}
> +
> +	for (domain_idx = 0; domain_idx < domain_count; domain_idx++) {
> +		struct lttng_channel *channels;
> +		struct lttng_handle *handle = lttng_create_handle(
> +			current_session_name, &domains[domain_idx]);
> +
> +		if (!handle) {
> +			ERR("Unable to create handle for session %s",
> +				current_session_name);
> +			goto end;
> +		}
> +
> +		channel_count = lttng_list_channels(handle, &channels);
> +		if (channel_count < 0) {
> +			ERR("Unable to list channels for session %s",
> +				current_session_name);
> +			goto end;
> +		}
> +
> +		for (channel_idx = 0; channel_idx < channel_count;
> +			channel_idx++) {
> +			if (channels[channel_idx].attr.subbuf_size >
> +				largest_subbuf) {
> +				largest_subbuf =
> +					channels[channel_idx].attr.subbuf_size;
> +			}
> +		}
> +		free(channels);
> +		lttng_destroy_handle(handle);
> +	}
> +end:
> +	free(domains);
> +	return largest_subbuf;
> +}
> +
>  /*
>   * Create a snapshot output object from arguments using the given URL.
>   *
> @@ -160,6 +210,15 @@ static struct lttng_snapshot_output *create_output_from_args(const char *url)
>  	}
>  
>  	if (opt_max_size) {
> +		/* Validate that the max size can contain one subbuffer. */
> +		uint64_t largest_subbuf = get_largest_subbuf();
> +		if (largest_subbuf == 0) {
> +			goto error;
> +		} else if (largest_subbuf > opt_max_size) {
> +			ERR("Snapshot size must be greater or equal to the largest subbuffer's size (%zu).",
> +				largest_subbuf);
> +			goto error;
> +		}
>  		ret = lttng_snapshot_output_set_size(opt_max_size, output);
>  		if (ret < 0) {
>  			goto error;
> -- 
> 1.9.1
> 

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 603 bytes --]

[-- Attachment #2: Type: text/plain, Size: 155 bytes --]

_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev