On Tue, Jan 25, 2022 at 5:08 PM Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
>
> On Tue, 25 Jan 2022 09:48:43 +0200
> Tzvetomir Stoyanov <tz.stoyanov@xxxxxxxxx> wrote:
>
> > > > >
> > > > > > + free(bytes);
> > > > > > + handle->pointer = 0;
> > > > > > + handle->capacity = s_uncompressed;
> > > > >
> > > > > If size is the greater of the two (compressed could be larger) and
> > > > > capacity is the allocated buffer, shouldn't we make it equal to size
> > > > > and not uncompressed?
> > > >
> > > > The name "capacity" in reading logic is a bit confusing, as it
> > > > actually is used to track the size of uncompressed data. The buffer
> > > > may be larger, but if the uncompressed is smaller - the rest is
> > > > garbage and must not be read.
> > >
> > > Then the extend function is broken:
> >
> > For this particular use case, the logic is not correct - the realloc()
> > could be redundant, if the real buffer size is bigger than the
> > capacity. But as usually a compression handle is used for reading or
> > for writing only, this is unlikely to happen. At least I cannot
> > imagine a use case where the same compression handle will be used for
> > reading a compress block and for writing compressed data. But even if
> > we hit that case - there is no data corruption or memory leak, just a
> > redundant realloc(). To solve that, reading and writing capacities
> > could be introduced ?
> > struct tracecmd_compression {
> > ...
> > unsigned int capacity_read;
> > unsigned int capacity_write;
> > ...
> > }
>
> A redundant realloc is not an issue, in fact, they are pretty fast (it
> detects that the requested size still fits what is allocated and just
> returns).
>
> >
> > But that change will be for a very rare case - when the compressed
> > data size is bigger than the uncompressed (usually happens only with a
> > very small data) + the same compression handle is used for compression
> > and decompression (not used in that way in trace-cmd).
> >
> > >
> > > static inline int buffer_extend(struct tracecmd_compression *handle, unsigned int size)
> > > {
> > > int extend;
> > > char *buf;
> > >
> > > if (size <= handle->capacity)
> > > return 0;
> > >
> > > extend = ((size - handle->capacity) / BUFSIZ + 1) * BUFSIZ;
> > > buf = realloc(handle->buffer, handle->capacity + extend);
> > > if (!buf)
> > > return -1;
> > > handle->buffer = buf;
> > > handle->capacity += extend;
> > >
> > > return 0;
> > > }
> > >
> > > As it sets the capacity to the allocated size, not what is stored.
> > >
> > > Perhaps you meant:
> > >
> > > handle->capacity += size;
>
> The above should still work, instead of doing the += extend.
>
> The point I was making is that capacity should either be the requested size
> allocated, or what is actually allocated. The above function sets it to
> what is allocated, and the other code sets it to what was requested. We
> should pick it to be one or the other.
I didn't know about that realloc optimization (check if requested size
fits into already allocated memory). In that case, we can use the
requested size in both places. But still allocate a page aligned
buffer in buffer_extend(), to avoid frequent reallocs (although
realloc will be called almost every time).
>
> -- Steve
>
>
> > >
> > > ?
> > >
> > > >
> > > > >
> > > > >
> > > > > > + return 0;
> > > > > > +error:
> > > > > > + tracecmd_compress_reset(handle);
> > > > > > + free(bytes);
> > > > > > + return -1;
> > > > > > +}
> > > > > > +
--
Tzvetomir (Ceco) Stoyanov
VMware Open Source Technology Center
|