Re: [E] Modified and New TS APIs Proposal

[Alan Carroll <so...@verizonmedia.com.INVALID>]

There is already a problem where the set of "connect" methods and the
options grow without bounds. This first came up with transparency (outbound
transparent) and now we have it with IOBuffer control. What I'd like is to
create a new connect function that takes an option structure. This would
contain

1. Version/size
2. Plugin ID
3. Transparency
4. IOBuffer controls

The advantage is this could be more easily expanded later without
restructuring the API.

Re: [E] Modified and New TS APIs Proposal

[Jeff Elsloo <el...@apache.org>]

I just pushed a commit that implements option 2 above because I felt
that it was a more straightforward approach that can be easily
documented (and is). The alternate approach of `memcpy`ing a static
instance, while simple on the surface, seems a little too in the weeds
for an API caller that may not even realize the static instance exists
unless they happen to find an example. The function will be obvious
enough to keep folks "on the rails" so to speak.
--
Thanks,
Jeff

On Thu, Jul 29, 2021 at 9:31 AM Alan Carroll
<so...@verizonmedia.com.invalid> wrote:
>
> >
> > Yes, we don't want to mess around with the actual indexing in this PR.
> >
>
> A couple of thoughts -
>
> 1. We could have a static instance of the options which could be `memcpy`
> into the struct to initialize it to default values.
>
> 2. It's kind of ugly, but we could have a function to initialize an option
> struct to default values.

Re: [E] Modified and New TS APIs Proposal

[Alan Carroll <so...@verizonmedia.com.INVALID>]

>
> Yes, we don't want to mess around with the actual indexing in this PR.
>

A couple of thoughts -

1. We could have a static instance of the options which could be `memcpy`
into the struct to initialize it to default values.

2. It's kind of ugly, but we could have a function to initialize an option
struct to default values.

Re: [E] Modified and New TS APIs Proposal

[Jeff Elsloo <el...@apache.org>]

I pushed a commit to address items one and three, but the issue with
item two is that the range of possible values is bound by an enum that
maps to `#defines` in core, both of which are zero indexed (0-14). The
item at index zero is the 128 byte buffer size, so if someone in the
future uses this struct and does not initialize that field, they may
find their implementation lacking in performance because it will
default to zero and not 8, which is the actual default.

One solution could be to change the `TSIOBufferSizeIndex` enum and
insert a new "undefined" value at zero, and correspondingly bump up
all of the `#defined` `BUFFER_SIZE_INDEX_XXX` values by one. We could
then rely on the zeroth value to be invalid, and index 1 and forward
would be valid. If we do that, we then need to find all the
dependencies, including parameters and their input validators in
`records.config` related code. Adding a bitmask, constructor, or
anything else seems like more work for all callers in the long run
when we can just change that enum and the `#defines` to make a value
of zero implicitly invalid if a buffer index is not specified when the
struct is initialized.

Given that the only usage of this struct is currently limited to 1)
the wrapper function `TSHttpConnectWithPluginId` that does specify a
value in PR #8088, and 2) my changes to slice in PR #8089 that rely on
`records.config` parameters, addressing the buffer indexing issue
seems like it should be a separate PR. If at some point we can rely on
zero being invalid, we can update the code to assert the
`buffer_index` member to be non-zero when the field is important to a
function using this struct. The way both PRs work currently, it is not
possible to provide a invalid/uninitialized value with this struct
unless a developer uses it in that manner after this change is
committed to the codebase.
--
Thanks,
Jeff

On Wed, Jul 28, 2021 at 8:44 AM Alan Carroll
<so...@verizonmedia.com.invalid> wrote:
>
> Yes, that seems in the right direction. A few points:
>
> 1. If the struct isn't opaque, then it should be passed by pointer, not by
> value.
> 2. There fields must be optional, e.g. I shouldn't be forced to pick some
> value for the buffering. I'm not sure of the best way to do this - a bit
> mask? An "invalid" value for each member?
> 3. The struct must start with either a version value, or a size value, or
> both, so that backwards compatibility can be maintained.
>
> On Tue, Jul 27, 2021 at 3:59 PM Jeff Elsloo <el...@apache.org> wrote:
>
> > Hi Alan,
> >
> > I modified the function I introduced to take an options struct as its
> > single argument to reduce the clutter. I added the fields necessary to
> > support my work in PR #8088 and will update #8089 if the changes in
> > the former are accepted. I did not add a field for transparency as you
> > mentioned above, nor am I incorporating any changes to
> > `TSHttpConnectTransparent()`, even if it calls
> > `PluginVCCore::alloc()`. That call will continue to use the defaults
> > until someone has time to refactor and test that function.
> >
> > Please take a look and let me know if this is along the lines of what
> > you're suggesting. With this foundation, we might be able to merge
> > `TSHttpConnectTransparent()` into `TSHttpConnectPlugin()` as time
> > permits.
> > --
> > Thanks,
> > Jeff
> >
> > On Mon, Jul 26, 2021 at 1:07 PM Alan Carroll
> > <so...@verizonmedia.com.invalid> wrote:
> > >
> > > There is already a problem where the set of "connect" methods and the
> > > options grow without bounds. This first came up with transparency
> > (outbound
> > > transparent) and now we have it with IOBuffer control. What I'd like is
> > to
> > > create a new connect function that takes an option structure. This would
> > > contain
> > >
> > > 1. Version/size
> > > 2. Plugin ID
> > > 3. Transparency
> > > 4. IOBuffer controls
> > >
> > > The advantage is this could be more easily expanded later without
> > > restructuring the API.
> >

Re: [E] Modified and New TS APIs Proposal

[Alan Carroll <so...@verizonmedia.com.INVALID>]

Yes, that seems in the right direction. A few points:

1. If the struct isn't opaque, then it should be passed by pointer, not by
value.
2. There fields must be optional, e.g. I shouldn't be forced to pick some
value for the buffering. I'm not sure of the best way to do this - a bit
mask? An "invalid" value for each member?
3. The struct must start with either a version value, or a size value, or
both, so that backwards compatibility can be maintained.

On Tue, Jul 27, 2021 at 3:59 PM Jeff Elsloo <el...@apache.org> wrote:

> Hi Alan,
>
> I modified the function I introduced to take an options struct as its
> single argument to reduce the clutter. I added the fields necessary to
> support my work in PR #8088 and will update #8089 if the changes in
> the former are accepted. I did not add a field for transparency as you
> mentioned above, nor am I incorporating any changes to
> `TSHttpConnectTransparent()`, even if it calls
> `PluginVCCore::alloc()`. That call will continue to use the defaults
> until someone has time to refactor and test that function.
>
> Please take a look and let me know if this is along the lines of what
> you're suggesting. With this foundation, we might be able to merge
> `TSHttpConnectTransparent()` into `TSHttpConnectPlugin()` as time
> permits.
> --
> Thanks,
> Jeff
>
> On Mon, Jul 26, 2021 at 1:07 PM Alan Carroll
> <so...@verizonmedia.com.invalid> wrote:
> >
> > There is already a problem where the set of "connect" methods and the
> > options grow without bounds. This first came up with transparency
> (outbound
> > transparent) and now we have it with IOBuffer control. What I'd like is
> to
> > create a new connect function that takes an option structure. This would
> > contain
> >
> > 1. Version/size
> > 2. Plugin ID
> > 3. Transparency
> > 4. IOBuffer controls
> >
> > The advantage is this could be more easily expanded later without
> > restructuring the API.
>

Re: [E] Modified and New TS APIs Proposal

[Jeff Elsloo <el...@apache.org>]

Hi Alan,

I modified the function I introduced to take an options struct as its
single argument to reduce the clutter. I added the fields necessary to
support my work in PR #8088 and will update #8089 if the changes in
the former are accepted. I did not add a field for transparency as you
mentioned above, nor am I incorporating any changes to
`TSHttpConnectTransparent()`, even if it calls
`PluginVCCore::alloc()`. That call will continue to use the defaults
until someone has time to refactor and test that function.

Please take a look and let me know if this is along the lines of what
you're suggesting. With this foundation, we might be able to merge
`TSHttpConnectTransparent()` into `TSHttpConnectPlugin()` as time
permits.
--
Thanks,
Jeff

On Mon, Jul 26, 2021 at 1:07 PM Alan Carroll
<so...@verizonmedia.com.invalid> wrote:
>
> There is already a problem where the set of "connect" methods and the
> options grow without bounds. This first came up with transparency (outbound
> transparent) and now we have it with IOBuffer control. What I'd like is to
> create a new connect function that takes an option structure. This would
> contain
>
> 1. Version/size
> 2. Plugin ID
> 3. Transparency
> 4. IOBuffer controls
>
> The advantage is this could be more easily expanded later without
> restructuring the API.