[PATCH] trace-cmd: fix writing of uncompressed size

[PATCH] trace-cmd: fix writing of uncompressed size

[Sven Schnelle]

Pass &size instead of &handle->pointer. Interestingly this doesn't hurt
on x86, but makes trace-cmd fail on s390.

Fixes: 3f8447b1 ("trace-cmd library: Add support for compression algorithms")
Signed-off-by: Sven Schnelle <svens@linux.ibm.com>
---
 lib/trace-cmd/trace-compress.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/trace-cmd/trace-compress.c b/lib/trace-cmd/trace-compress.c
index a63295e..ad9b7fc 100644
--- a/lib/trace-cmd/trace-compress.c
+++ b/lib/trace-cmd/trace-compress.c
@@ -331,7 +331,7 @@ int tracecmd_compress_block(struct tracecmd_compression *handle)
 		goto out;
 
 	/* Write uncompressed data size */
-	endian4 = tep_read_number(handle->tep, &handle->pointer, 4);
+	endian4 = tep_read_number(handle->tep, &size, 4);
 	ret = do_write(handle, &endian4, 4);
 	if (ret != 4) {
 		ret = -1;
-- 
2.36.1

Re: [PATCH] trace-cmd: fix writing of uncompressed size

[Tzvetomir Stoyanov]

On Mon, Jul 11, 2022 at 10:44 AM Sven Schnelle <svens@linux.ibm.com> wrote:
>
> Pass &size instead of &handle->pointer. Interestingly this doesn't hurt
> on x86, but makes trace-cmd fail on s390.

Hi Sven,
Thanks for testing this code on s390, I've tested it only on x86.
Please, can you give more information about the trace-cmd failure on
s390?

>
> Fixes: 3f8447b1 ("trace-cmd library: Add support for compression algorithms")
> Signed-off-by: Sven Schnelle <svens@linux.ibm.com>
> ---
>  lib/trace-cmd/trace-compress.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/trace-cmd/trace-compress.c b/lib/trace-cmd/trace-compress.c
> index a63295e..ad9b7fc 100644
> --- a/lib/trace-cmd/trace-compress.c
> +++ b/lib/trace-cmd/trace-compress.c
> @@ -331,7 +331,7 @@ int tracecmd_compress_block(struct tracecmd_compression *handle)
>                 goto out;
>
>         /* Write uncompressed data size */
> -       endian4 = tep_read_number(handle->tep, &handle->pointer, 4);
> +       endian4 = tep_read_number(handle->tep, &size, 4);

Here 'size' is the size of the buffer, used by the compression
algorithm to compress the data block. That size depends on the
algorithm, but usually it is less than the uncompressed data size and
bigger than the compressed data size. On this position in the file
must be written the size of the uncompressed data, that is
'handle->pointer'. I agree that the name is a bit misleading, as this
is not a pointer to a memory address.  The 'handle->pointer' is the
offset within the compression buffer, where the next uncompressed data
will be written. The logic uses a dynamic buffer with given capacity.
When the buffer is empty, 'handle->pointer' is 0. With each
uncompressed data chunk, written into that buffer, 'handle->pointer'
increases with its size - i.e. the first byte is written on position 0
(the initial 'handle->pointer') and the pointer increases to 1. That's
why it reflects the size of the uncompressed data. When the pointer
reaches the capacity of the buffer, the buffer is extended. At the
end, that buffer is passed to the compression algorithm, to compress
the block.

>         ret = do_write(handle, &endian4, 4);
>         if (ret != 4) {
>                 ret = -1;
> --
> 2.36.1
>


-- 
Tzvetomir (Ceco) Stoyanov
VMware Open Source Technology Center

Re: [PATCH] trace-cmd: fix writing of uncompressed size

[Sven Schnelle]

Hi,

Tzvetomir Stoyanov <tz.stoyanov@gmail.com> writes:

> On Mon, Jul 11, 2022 at 10:44 AM Sven Schnelle <svens@linux.ibm.com> wrote:
>>
>> Pass &size instead of &handle->pointer. Interestingly this doesn't hurt
>> on x86, but makes trace-cmd fail on s390.
>
> Hi Sven,
> Thanks for testing this code on s390, I've tested it only on x86.
> Please, can you give more information about the trace-cmd failure on
> s390?

I'm running:

# trace-cmd record -p function -F /bin/ls
  plugin 'function'
[ls output follows]
CPU22 data recorded at offset=0x154000
    115345 bytes in size (1269760 uncompressed)
[skipped other CPUs, they didn't record anything]

trace-cmd report doesn't show any trace:

# trace-cmd report
version = 7

trace-cmd dump says:

# trace-cmd dump
 Tracing meta data in file trace.dat:
        [Initial format]
                7       [Version]
                1       [Big endian]
                8       [Bytes in a long]
                4096    [Page size, bytes]
                zstd    [Compression algorithm]
                1.5.2   [Compression version]
        [buffer "", "local" clock, 4096 page size, 1 cpus, 115345 bytes flyrecord data]
        [142 options]
trace-cmd: Unknown error -22
  cannot uncompress section block

Thanks,
Sven

Re: [PATCH] trace-cmd: fix writing of uncompressed size

[Sven Schnelle]

Tzvetomir Stoyanov <tz.stoyanov@gmail.com> writes:

> On Mon, Jul 11, 2022 at 10:44 AM Sven Schnelle <svens@linux.ibm.com> wrote:
>>
>> Pass &size instead of &handle->pointer. Interestingly this doesn't hurt
>> on x86, but makes trace-cmd fail on s390.
>
> Hi Sven,
> Thanks for testing this code on s390, I've tested it only on x86.
> Please, can you give more information about the trace-cmd failure on
> s390?
>
>>
>> Fixes: 3f8447b1 ("trace-cmd library: Add support for compression algorithms")
>> Signed-off-by: Sven Schnelle <svens@linux.ibm.com>
>> ---
>>  lib/trace-cmd/trace-compress.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/lib/trace-cmd/trace-compress.c b/lib/trace-cmd/trace-compress.c
>> index a63295e..ad9b7fc 100644
>> --- a/lib/trace-cmd/trace-compress.c
>> +++ b/lib/trace-cmd/trace-compress.c
>> @@ -331,7 +331,7 @@ int tracecmd_compress_block(struct tracecmd_compression *handle)
>>                 goto out;
>>
>>         /* Write uncompressed data size */
>> -       endian4 = tep_read_number(handle->tep, &handle->pointer, 4);
>> +       endian4 = tep_read_number(handle->tep, &size, 4);
>
> Here 'size' is the size of the buffer, used by the compression
> algorithm to compress the data block. That size depends on the
> algorithm, but usually it is less than the uncompressed data size and
> bigger than the compressed data size. On this position in the file
> must be written the size of the uncompressed data, that is
> 'handle->pointer'. I agree that the name is a bit misleading, as this
> is not a pointer to a memory address.  The 'handle->pointer' is the
> offset within the compression buffer, where the next uncompressed data
> will be written. The logic uses a dynamic buffer with given capacity.
> When the buffer is empty, 'handle->pointer' is 0. With each
> uncompressed data chunk, written into that buffer, 'handle->pointer'
> increases with its size - i.e. the first byte is written on position 0
> (the initial 'handle->pointer') and the pointer increases to 1. That's
> why it reflects the size of the uncompressed data. When the pointer
> reaches the capacity of the buffer, the buffer is extended. At the
> end, that buffer is passed to the compression algorithm, to compress
> the block.

I see that 'handle->pointer' is unsigned long, which is 8 bytes on
s390. But it is converted as 4 byte int. That would work on LE
platforms, but not on BE.

Re: [PATCH] trace-cmd: fix writing of uncompressed size

[Tzvetomir Stoyanov]

On Mon, Jul 11, 2022 at 12:14 PM Sven Schnelle <svens@linux.ibm.com> wrote:
>
> Tzvetomir Stoyanov <tz.stoyanov@gmail.com> writes:
>
> > On Mon, Jul 11, 2022 at 10:44 AM Sven Schnelle <svens@linux.ibm.com> wrote:
> >>
> >> Pass &size instead of &handle->pointer. Interestingly this doesn't hurt
> >> on x86, but makes trace-cmd fail on s390.
> >
> > Hi Sven,
> > Thanks for testing this code on s390, I've tested it only on x86.
> > Please, can you give more information about the trace-cmd failure on
> > s390?
> >
> >>
> >> Fixes: 3f8447b1 ("trace-cmd library: Add support for compression algorithms")
> >> Signed-off-by: Sven Schnelle <svens@linux.ibm.com>
> >> ---
> >>  lib/trace-cmd/trace-compress.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/lib/trace-cmd/trace-compress.c b/lib/trace-cmd/trace-compress.c
> >> index a63295e..ad9b7fc 100644
> >> --- a/lib/trace-cmd/trace-compress.c
> >> +++ b/lib/trace-cmd/trace-compress.c
> >> @@ -331,7 +331,7 @@ int tracecmd_compress_block(struct tracecmd_compression *handle)
> >>                 goto out;
> >>
> >>         /* Write uncompressed data size */
> >> -       endian4 = tep_read_number(handle->tep, &handle->pointer, 4);
> >> +       endian4 = tep_read_number(handle->tep, &size, 4);
> >
> > Here 'size' is the size of the buffer, used by the compression
> > algorithm to compress the data block. That size depends on the
> > algorithm, but usually it is less than the uncompressed data size and
> > bigger than the compressed data size. On this position in the file
> > must be written the size of the uncompressed data, that is
> > 'handle->pointer'. I agree that the name is a bit misleading, as this
> > is not a pointer to a memory address.  The 'handle->pointer' is the
> > offset within the compression buffer, where the next uncompressed data
> > will be written. The logic uses a dynamic buffer with given capacity.
> > When the buffer is empty, 'handle->pointer' is 0. With each
> > uncompressed data chunk, written into that buffer, 'handle->pointer'
> > increases with its size - i.e. the first byte is written on position 0
> > (the initial 'handle->pointer') and the pointer increases to 1. That's
> > why it reflects the size of the uncompressed data. When the pointer
> > reaches the capacity of the buffer, the buffer is extended. At the
> > end, that buffer is passed to the compression algorithm, to compress
> > the block.
>
> I see that 'handle->pointer' is unsigned long, which is 8 bytes on
> s390. But it is converted as 4 byte int. That would work on LE
> platforms, but not on BE.

Yes, that looks like a problem. I think the best fix is to change
'handle->pointer' to unsigned int, can you test that on s390? There is
no need of 8 bytes for that size, unsigned int should be OK. This is
the size of one data chunk, it should not exceed 4G. Do you want to
submit such a fix, if it works on s390?
Thanks Sven!


-- 
Tzvetomir (Ceco) Stoyanov
VMware Open Source Technology Center

Re: [PATCH] trace-cmd: fix writing of uncompressed size

[Sven Schnelle]

Tzvetomir Stoyanov <tz.stoyanov@gmail.com> writes:

>> I see that 'handle->pointer' is unsigned long, which is 8 bytes on
>> s390. But it is converted as 4 byte int. That would work on LE
>> platforms, but not on BE.
>
> Yes, that looks like a problem. I think the best fix is to change
> 'handle->pointer' to unsigned int, can you test that on s390? There is
> no need of 8 bytes for that size, unsigned int should be OK. This is
> the size of one data chunk, it should not exceed 4G. Do you want to
> submit such a fix, if it works on s390?

Yes, converting it to unsigned int works. I'll submit a fix.

Thanks!
Sven

[PATCH] trace-cmd: Fix writing of uncompressed size

[Steven Rostedt]

From: Sven Schnelle <svens@linux.ibm.com>

pointer in struct tracecmd_compression is 'unsigned long', which is 8 byte
in size on most platforms, but the tep_read_number() call in the next line
treats it as a 4 byte value. As there's no need for unsigned long change
the type to unsigned int.

Link: https://lore.kernel.org/all/20220711074418.858843-1-svens@linux.ibm.com/
Link: https://lore.kernel.org/all/20220711094340.2829115-1-svens@linux.ibm.com/

Fixes: 3f8447b1 ("trace-cmd library: Add support for compression algorithms")
Signed-off-by: Sven Schnelle <svens@linux.ibm.com>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 lib/trace-cmd/trace-compress.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/trace-cmd/trace-compress.c b/lib/trace-cmd/trace-compress.c
index a63295e..461de8d 100644
--- a/lib/trace-cmd/trace-compress.c
+++ b/lib/trace-cmd/trace-compress.c
@@ -32,7 +32,7 @@ struct tracecmd_compression {
 	int				fd;
 	unsigned int			capacity;
 	unsigned int			capacity_read;
-	unsigned long			pointer;
+	unsigned int			pointer;
 	char				*buffer;
 	struct compress_proto		*proto;
 	struct tep_handle		*tep;
-- 
2.36.1