On Mon, Jul 11, 2022 at 12:14 PM Sven Schnelle <svens@xxxxxxxxxxxxx> wrote:
>
> Tzvetomir Stoyanov <tz.stoyanov@xxxxxxxxx> writes:
>
> > On Mon, Jul 11, 2022 at 10:44 AM Sven Schnelle <svens@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> >> ---
> >> 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
