You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@trafficserver.apache.org by Masakazu Kitajo <ma...@apache.org> on 2023/01/03 18:28:36 UTC
Re: [E] Re: Proposed change to TS API: TSHeapBuf
What's the benefit of forcing users to use the structure? A user might want
to store the returned pointer into a user-defined structure.
On Tue, Sep 6, 2022 at 10:24 AM Walt Karas <wk...@yahooinc.com.invalid>
wrote:
> Presumably we don't need to allow for use of antiquated C compilers that
> don't allow structures as return values. So this:
>
> typedef struct
> {
> char *data;
> int length;
> }
> TSVarLenData;
>
> TSVarLenData TSSomething(x, y, z);
>
> Is another alternative.
>
> On Thu, Sep 1, 2022 at 5:32 PM Masakazu Kitajo <ma...@apache.org> wrote:
>
> > Using encapsulation/data abstraction is fine, but it doesn't seem like
> > TSHeapBuf makes sense. No TS API receives it. If plugin code is the only
> > user and it has to deal with a non-const raw pointer and a data length
> > after all, why do we want to wrap them in the first place?
> >
> > As for smaller steps, like I suggested on the PR, you could introduce
> > TSHeapBuf separately from the fix, I think. And if there are similar TS
> > APIs that could return TSHeapBuf, supporting TSHeapBuf on only one of
> them
> > makes inconsistency in TS API. IMO, the two changes, the fix and the new
> > API, should be made like 1 + 1, but not 1.5 + 0.5 nor 1 + 0.5 + 0.5.
> >
> > Masakazu
> >
> > On Thu, Sep 1, 2022 at 10:08 AM Walt Karas <wk...@yahooinc.com.invalid>
> > wrote:
> >
> > > As a rule of thumb, I prefer using encapsulation/data abstraction. I
> > think
> > > perhaps that is one reason I've been a poor match to this project.
> There
> > > doesn't seem to be a consensus that we should follow this rule of
> thumb.
> > >
> > > KIt, that would be my preference. But I am part of the consensus I
> think
> > > we have, that we should favor a series of smaller steps, rather than
> > doing
> > > all of them in one big step.
> > >
> > > On Wed, Aug 31, 2022 at 12:13 PM Shu Kit Chan <ch...@gmail.com>
> > > wrote:
> > >
> > > > Also are we planning to eventually rewrite our existing APIs (where
> > > > applicable) to use this?
> > > >
> > > > On Wed, Aug 31, 2022 at 8:36 AM Masakazu Kitajo <ma...@apache.org>
> > > wrote:
> > > > >
> > > > > What's the advantage of using TSHeapBuf? What issue does it solve?
> > > > >
> > > > >
> > > > > On Wed, Aug 31, 2022 at 7:48 AM Walt Karas
> > <wkaras@yahooinc.com.invalid
> > > >
> > > > > wrote:
> > > > >
> > > > > > Described here:
> > > > > >
> > > > > >
> > > >
> > >
> >
> https://urldefense.com/v3/__https://github.com/apache/trafficserver/blob/os_pkey_cnf_reload/doc/developer-guide/api/functions/TSHeapBuf.en.rst*tsheapbufdata__;Iw!!Op6eflyXZCqGR5I!Du0fBfMhb4pdM2ECFijJ7aJ-jT70jEPeZwjhsvWt2Dr2cSZ5G7HWY20wZOmFHIR3MxnvPZpoRDMlII5dgow$
> > > >
> > > > > > ,
> > > > > >
> > > > > > In PR
> > > >
> > >
> >
> https://urldefense.com/v3/__https://github.com/apache/trafficserver/pull/8790__;!!Op6eflyXZCqGR5I!Du0fBfMhb4pdM2ECFijJ7aJ-jT70jEPeZwjhsvWt2Dr2cSZ5G7HWY20wZOmFHIR3MxnvPZpoRDMlYh9lIuc$
> > > > .
> > > > > >
> > > > > > This allows a dynamically allocated buffer, of any reasonable
> > length,
> > > > to be
> > > > > > returned by a TS API function, like this:
> > > > > >
> > > > > > TSHeapBuf hb = TSSomething(x, y, z);
> > > > > >
> > > > > > One alternative is an interface like this:
> > > > > >
> > > > > > int length;
> > > > > > char *data = TSSomething(x, y, z, &length);
> > > > > >
> > > > > > The data is dynamically allocated, and would be freed with
> > TSfree().
> > > > > >
> > > > > > Another alternative is:
> > > > > >
> > > > > > char *buf = TSalloc(BUF_SIZE);
> > > > > > int actual_size = TSSomething(x, y, z, buf, BUF_SIZE);
> > > > > > if (actual_size > BUF_SIZE) {
> > > > > > // buf was too small, unchanged.
> > > > > > TSfree(buf);
> > > > > > buf = TSalloc(actual_size);
> > > > > > TSSomething(x, y, z, buf, actual_size);
> > > > > > }
> > > > > >
> > > >
> > >
> >
>
Re: [E] Re: Proposed change to TS API: TSHeapBuf
Posted by Walt Karas <wk...@yahooinc.com.INVALID>.
I'll wait a couple more days for input, then I'll change it to return char
* and take an int * parameter to output the length. If we're switching to
C++, we can update this based on whatever consensus we reach for the
switchover strategy.
On Wed, Jan 4, 2023 at 7:03 PM Masakazu Kitajo <ma...@apache.org> wrote:
> > They could make the returned structure a nesting structure in
> their structure.
>
> What if the structure is defined by a library and a plugin author can't
> modify it? What I'm trying to say is that I don't see the benefit of
> limiting how a user receives the pointer and the length where nothing
> receives TSVarLenData. Do we require two copy ops for the "unusual" case
> just to avoid having two variables by providing TSVarLenData? Is it that
> convenient?
>
> I don't have a strong opinion about moving to C++ at the moment, but if we
> go that way and start using string_view, TSVarLenData wouldn't be used for
> new APIs. And we'd add a string_view version of an old API that uses
> TSVarLenData so that users can easily pass the returned value to other
> functions. Then we'd deprecate the old API and remove it in the future.
> That's a lot of work.
>
> I feel like introducing TSVarLenData has more downside than the upside.
>
> On Wed, Jan 4, 2023 at 9:13 AM Walt Karas <wk...@yahooinc.com.invalid>
> wrote:
>
> > They could make the returned structure a nesting structure in their
> > structure. Better to make things more convenient for the typical case,
> not
> > the unusual case.
> >
> > On Tue, Jan 3, 2023 at 12:28 PM Masakazu Kitajo <ma...@apache.org>
> wrote:
> >
> > > What's the benefit of forcing users to use the structure? A user might
> > want
> > > to store the returned pointer into a user-defined structure.
> > >
> > >
> > >
> > > On Tue, Sep 6, 2022 at 10:24 AM Walt Karas <wkaras@yahooinc.com.invalid
> >
> > > wrote:
> > >
> > > > Presumably we don't need to allow for use of antiquated C compilers
> > that
> > > > don't allow structures as return values. So this:
> > > >
> > > > typedef struct
> > > > {
> > > > char *data;
> > > > int length;
> > > > }
> > > > TSVarLenData;
> > > >
> > > > TSVarLenData TSSomething(x, y, z);
> > > >
> > > > Is another alternative.
> > > >
> > > > On Thu, Sep 1, 2022 at 5:32 PM Masakazu Kitajo <ma...@apache.org>
> > > wrote:
> > > >
> > > > > Using encapsulation/data abstraction is fine, but it doesn't seem
> > like
> > > > > TSHeapBuf makes sense. No TS API receives it. If plugin code is the
> > > only
> > > > > user and it has to deal with a non-const raw pointer and a data
> > length
> > > > > after all, why do we want to wrap them in the first place?
> > > > >
> > > > > As for smaller steps, like I suggested on the PR, you could
> introduce
> > > > > TSHeapBuf separately from the fix, I think. And if there are
> similar
> > TS
> > > > > APIs that could return TSHeapBuf, supporting TSHeapBuf on only one
> of
> > > > them
> > > > > makes inconsistency in TS API. IMO, the two changes, the fix and
> the
> > > new
> > > > > API, should be made like 1 + 1, but not 1.5 + 0.5 nor 1 + 0.5 +
> 0.5.
> > > > >
> > > > > Masakazu
> > > > >
> > > > > On Thu, Sep 1, 2022 at 10:08 AM Walt Karas
> > <wkaras@yahooinc.com.invalid
> > > >
> > > > > wrote:
> > > > >
> > > > > > As a rule of thumb, I prefer using encapsulation/data
> > abstraction. I
> > > > > think
> > > > > > perhaps that is one reason I've been a poor match to this
> project.
> > > > There
> > > > > > doesn't seem to be a consensus that we should follow this rule
> of
> > > > thumb.
> > > > > >
> > > > > > KIt, that would be my preference. But I am part of the
> consensus I
> > > > think
> > > > > > we have, that we should favor a series of smaller steps, rather
> > than
> > > > > doing
> > > > > > all of them in one big step.
> > > > > >
> > > > > > On Wed, Aug 31, 2022 at 12:13 PM Shu Kit Chan <
> > chanshukit@gmail.com>
> > > > > > wrote:
> > > > > >
> > > > > > > Also are we planning to eventually rewrite our existing APIs
> > (where
> > > > > > > applicable) to use this?
> > > > > > >
> > > > > > > On Wed, Aug 31, 2022 at 8:36 AM Masakazu Kitajo <
> > maskit@apache.org
> > > >
> > > > > > wrote:
> > > > > > > >
> > > > > > > > What's the advantage of using TSHeapBuf? What issue does it
> > > solve?
> > > > > > > >
> > > > > > > >
> > > > > > > > On Wed, Aug 31, 2022 at 7:48 AM Walt Karas
> > > > > <wkaras@yahooinc.com.invalid
> > > > > > >
> > > > > > > > wrote:
> > > > > > > >
> > > > > > > > > Described here:
> > > > > > > > >
> > > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
> https://urldefense.com/v3/__https://github.com/apache/trafficserver/blob/os_pkey_cnf_reload/doc/developer-guide/api/functions/TSHeapBuf.en.rst*tsheapbufdata__;Iw!!Op6eflyXZCqGR5I!Du0fBfMhb4pdM2ECFijJ7aJ-jT70jEPeZwjhsvWt2Dr2cSZ5G7HWY20wZOmFHIR3MxnvPZpoRDMlII5dgow$
> > > > > > >
> > > > > > > > > ,
> > > > > > > > >
> > > > > > > > > In PR
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
> https://urldefense.com/v3/__https://github.com/apache/trafficserver/pull/8790__;!!Op6eflyXZCqGR5I!Du0fBfMhb4pdM2ECFijJ7aJ-jT70jEPeZwjhsvWt2Dr2cSZ5G7HWY20wZOmFHIR3MxnvPZpoRDMlYh9lIuc$
> > > > > > > .
> > > > > > > > >
> > > > > > > > > This allows a dynamically allocated buffer, of any
> reasonable
> > > > > length,
> > > > > > > to be
> > > > > > > > > returned by a TS API function, like this:
> > > > > > > > >
> > > > > > > > > TSHeapBuf hb = TSSomething(x, y, z);
> > > > > > > > >
> > > > > > > > > One alternative is an interface like this:
> > > > > > > > >
> > > > > > > > > int length;
> > > > > > > > > char *data = TSSomething(x, y, z, &length);
> > > > > > > > >
> > > > > > > > > The data is dynamically allocated, and would be freed with
> > > > > TSfree().
> > > > > > > > >
> > > > > > > > > Another alternative is:
> > > > > > > > >
> > > > > > > > > char *buf = TSalloc(BUF_SIZE);
> > > > > > > > > int actual_size = TSSomething(x, y, z, buf, BUF_SIZE);
> > > > > > > > > if (actual_size > BUF_SIZE) {
> > > > > > > > > // buf was too small, unchanged.
> > > > > > > > > TSfree(buf);
> > > > > > > > > buf = TSalloc(actual_size);
> > > > > > > > > TSSomething(x, y, z, buf, actual_size);
> > > > > > > > > }
> > > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
Re: [E] Re: Proposed change to TS API: TSHeapBuf
Posted by Masakazu Kitajo <ma...@apache.org>.
> They could make the returned structure a nesting structure in
their structure.
What if the structure is defined by a library and a plugin author can't
modify it? What I'm trying to say is that I don't see the benefit of
limiting how a user receives the pointer and the length where nothing
receives TSVarLenData. Do we require two copy ops for the "unusual" case
just to avoid having two variables by providing TSVarLenData? Is it that
convenient?
I don't have a strong opinion about moving to C++ at the moment, but if we
go that way and start using string_view, TSVarLenData wouldn't be used for
new APIs. And we'd add a string_view version of an old API that uses
TSVarLenData so that users can easily pass the returned value to other
functions. Then we'd deprecate the old API and remove it in the future.
That's a lot of work.
I feel like introducing TSVarLenData has more downside than the upside.
On Wed, Jan 4, 2023 at 9:13 AM Walt Karas <wk...@yahooinc.com.invalid>
wrote:
> They could make the returned structure a nesting structure in their
> structure. Better to make things more convenient for the typical case, not
> the unusual case.
>
> On Tue, Jan 3, 2023 at 12:28 PM Masakazu Kitajo <ma...@apache.org> wrote:
>
> > What's the benefit of forcing users to use the structure? A user might
> want
> > to store the returned pointer into a user-defined structure.
> >
> >
> >
> > On Tue, Sep 6, 2022 at 10:24 AM Walt Karas <wk...@yahooinc.com.invalid>
> > wrote:
> >
> > > Presumably we don't need to allow for use of antiquated C compilers
> that
> > > don't allow structures as return values. So this:
> > >
> > > typedef struct
> > > {
> > > char *data;
> > > int length;
> > > }
> > > TSVarLenData;
> > >
> > > TSVarLenData TSSomething(x, y, z);
> > >
> > > Is another alternative.
> > >
> > > On Thu, Sep 1, 2022 at 5:32 PM Masakazu Kitajo <ma...@apache.org>
> > wrote:
> > >
> > > > Using encapsulation/data abstraction is fine, but it doesn't seem
> like
> > > > TSHeapBuf makes sense. No TS API receives it. If plugin code is the
> > only
> > > > user and it has to deal with a non-const raw pointer and a data
> length
> > > > after all, why do we want to wrap them in the first place?
> > > >
> > > > As for smaller steps, like I suggested on the PR, you could introduce
> > > > TSHeapBuf separately from the fix, I think. And if there are similar
> TS
> > > > APIs that could return TSHeapBuf, supporting TSHeapBuf on only one of
> > > them
> > > > makes inconsistency in TS API. IMO, the two changes, the fix and the
> > new
> > > > API, should be made like 1 + 1, but not 1.5 + 0.5 nor 1 + 0.5 + 0.5.
> > > >
> > > > Masakazu
> > > >
> > > > On Thu, Sep 1, 2022 at 10:08 AM Walt Karas
> <wkaras@yahooinc.com.invalid
> > >
> > > > wrote:
> > > >
> > > > > As a rule of thumb, I prefer using encapsulation/data
> abstraction. I
> > > > think
> > > > > perhaps that is one reason I've been a poor match to this project.
> > > There
> > > > > doesn't seem to be a consensus that we should follow this rule of
> > > thumb.
> > > > >
> > > > > KIt, that would be my preference. But I am part of the consensus I
> > > think
> > > > > we have, that we should favor a series of smaller steps, rather
> than
> > > > doing
> > > > > all of them in one big step.
> > > > >
> > > > > On Wed, Aug 31, 2022 at 12:13 PM Shu Kit Chan <
> chanshukit@gmail.com>
> > > > > wrote:
> > > > >
> > > > > > Also are we planning to eventually rewrite our existing APIs
> (where
> > > > > > applicable) to use this?
> > > > > >
> > > > > > On Wed, Aug 31, 2022 at 8:36 AM Masakazu Kitajo <
> maskit@apache.org
> > >
> > > > > wrote:
> > > > > > >
> > > > > > > What's the advantage of using TSHeapBuf? What issue does it
> > solve?
> > > > > > >
> > > > > > >
> > > > > > > On Wed, Aug 31, 2022 at 7:48 AM Walt Karas
> > > > <wkaras@yahooinc.com.invalid
> > > > > >
> > > > > > > wrote:
> > > > > > >
> > > > > > > > Described here:
> > > > > > > >
> > > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
> https://urldefense.com/v3/__https://github.com/apache/trafficserver/blob/os_pkey_cnf_reload/doc/developer-guide/api/functions/TSHeapBuf.en.rst*tsheapbufdata__;Iw!!Op6eflyXZCqGR5I!Du0fBfMhb4pdM2ECFijJ7aJ-jT70jEPeZwjhsvWt2Dr2cSZ5G7HWY20wZOmFHIR3MxnvPZpoRDMlII5dgow$
> > > > > >
> > > > > > > > ,
> > > > > > > >
> > > > > > > > In PR
> > > > > >
> > > > >
> > > >
> > >
> >
> https://urldefense.com/v3/__https://github.com/apache/trafficserver/pull/8790__;!!Op6eflyXZCqGR5I!Du0fBfMhb4pdM2ECFijJ7aJ-jT70jEPeZwjhsvWt2Dr2cSZ5G7HWY20wZOmFHIR3MxnvPZpoRDMlYh9lIuc$
> > > > > > .
> > > > > > > >
> > > > > > > > This allows a dynamically allocated buffer, of any reasonable
> > > > length,
> > > > > > to be
> > > > > > > > returned by a TS API function, like this:
> > > > > > > >
> > > > > > > > TSHeapBuf hb = TSSomething(x, y, z);
> > > > > > > >
> > > > > > > > One alternative is an interface like this:
> > > > > > > >
> > > > > > > > int length;
> > > > > > > > char *data = TSSomething(x, y, z, &length);
> > > > > > > >
> > > > > > > > The data is dynamically allocated, and would be freed with
> > > > TSfree().
> > > > > > > >
> > > > > > > > Another alternative is:
> > > > > > > >
> > > > > > > > char *buf = TSalloc(BUF_SIZE);
> > > > > > > > int actual_size = TSSomething(x, y, z, buf, BUF_SIZE);
> > > > > > > > if (actual_size > BUF_SIZE) {
> > > > > > > > // buf was too small, unchanged.
> > > > > > > > TSfree(buf);
> > > > > > > > buf = TSalloc(actual_size);
> > > > > > > > TSSomething(x, y, z, buf, actual_size);
> > > > > > > > }
> > > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
Re: [E] Re: Proposed change to TS API: TSHeapBuf
Posted by Walt Karas <wk...@yahooinc.com.INVALID>.
They could make the returned structure a nesting structure in their
structure. Better to make things more convenient for the typical case, not
the unusual case.
On Tue, Jan 3, 2023 at 12:28 PM Masakazu Kitajo <ma...@apache.org> wrote:
> What's the benefit of forcing users to use the structure? A user might want
> to store the returned pointer into a user-defined structure.
>
>
>
> On Tue, Sep 6, 2022 at 10:24 AM Walt Karas <wk...@yahooinc.com.invalid>
> wrote:
>
> > Presumably we don't need to allow for use of antiquated C compilers that
> > don't allow structures as return values. So this:
> >
> > typedef struct
> > {
> > char *data;
> > int length;
> > }
> > TSVarLenData;
> >
> > TSVarLenData TSSomething(x, y, z);
> >
> > Is another alternative.
> >
> > On Thu, Sep 1, 2022 at 5:32 PM Masakazu Kitajo <ma...@apache.org>
> wrote:
> >
> > > Using encapsulation/data abstraction is fine, but it doesn't seem like
> > > TSHeapBuf makes sense. No TS API receives it. If plugin code is the
> only
> > > user and it has to deal with a non-const raw pointer and a data length
> > > after all, why do we want to wrap them in the first place?
> > >
> > > As for smaller steps, like I suggested on the PR, you could introduce
> > > TSHeapBuf separately from the fix, I think. And if there are similar TS
> > > APIs that could return TSHeapBuf, supporting TSHeapBuf on only one of
> > them
> > > makes inconsistency in TS API. IMO, the two changes, the fix and the
> new
> > > API, should be made like 1 + 1, but not 1.5 + 0.5 nor 1 + 0.5 + 0.5.
> > >
> > > Masakazu
> > >
> > > On Thu, Sep 1, 2022 at 10:08 AM Walt Karas <wkaras@yahooinc.com.invalid
> >
> > > wrote:
> > >
> > > > As a rule of thumb, I prefer using encapsulation/data abstraction. I
> > > think
> > > > perhaps that is one reason I've been a poor match to this project.
> > There
> > > > doesn't seem to be a consensus that we should follow this rule of
> > thumb.
> > > >
> > > > KIt, that would be my preference. But I am part of the consensus I
> > think
> > > > we have, that we should favor a series of smaller steps, rather than
> > > doing
> > > > all of them in one big step.
> > > >
> > > > On Wed, Aug 31, 2022 at 12:13 PM Shu Kit Chan <ch...@gmail.com>
> > > > wrote:
> > > >
> > > > > Also are we planning to eventually rewrite our existing APIs (where
> > > > > applicable) to use this?
> > > > >
> > > > > On Wed, Aug 31, 2022 at 8:36 AM Masakazu Kitajo <maskit@apache.org
> >
> > > > wrote:
> > > > > >
> > > > > > What's the advantage of using TSHeapBuf? What issue does it
> solve?
> > > > > >
> > > > > >
> > > > > > On Wed, Aug 31, 2022 at 7:48 AM Walt Karas
> > > <wkaras@yahooinc.com.invalid
> > > > >
> > > > > > wrote:
> > > > > >
> > > > > > > Described here:
> > > > > > >
> > > > > > >
> > > > >
> > > >
> > >
> >
> https://urldefense.com/v3/__https://github.com/apache/trafficserver/blob/os_pkey_cnf_reload/doc/developer-guide/api/functions/TSHeapBuf.en.rst*tsheapbufdata__;Iw!!Op6eflyXZCqGR5I!Du0fBfMhb4pdM2ECFijJ7aJ-jT70jEPeZwjhsvWt2Dr2cSZ5G7HWY20wZOmFHIR3MxnvPZpoRDMlII5dgow$
> > > > >
> > > > > > > ,
> > > > > > >
> > > > > > > In PR
> > > > >
> > > >
> > >
> >
> https://urldefense.com/v3/__https://github.com/apache/trafficserver/pull/8790__;!!Op6eflyXZCqGR5I!Du0fBfMhb4pdM2ECFijJ7aJ-jT70jEPeZwjhsvWt2Dr2cSZ5G7HWY20wZOmFHIR3MxnvPZpoRDMlYh9lIuc$
> > > > > .
> > > > > > >
> > > > > > > This allows a dynamically allocated buffer, of any reasonable
> > > length,
> > > > > to be
> > > > > > > returned by a TS API function, like this:
> > > > > > >
> > > > > > > TSHeapBuf hb = TSSomething(x, y, z);
> > > > > > >
> > > > > > > One alternative is an interface like this:
> > > > > > >
> > > > > > > int length;
> > > > > > > char *data = TSSomething(x, y, z, &length);
> > > > > > >
> > > > > > > The data is dynamically allocated, and would be freed with
> > > TSfree().
> > > > > > >
> > > > > > > Another alternative is:
> > > > > > >
> > > > > > > char *buf = TSalloc(BUF_SIZE);
> > > > > > > int actual_size = TSSomething(x, y, z, buf, BUF_SIZE);
> > > > > > > if (actual_size > BUF_SIZE) {
> > > > > > > // buf was too small, unchanged.
> > > > > > > TSfree(buf);
> > > > > > > buf = TSalloc(actual_size);
> > > > > > > TSSomething(x, y, z, buf, actual_size);
> > > > > > > }
> > > > > > >
> > > > >
> > > >
> > >
> >
>