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 <solidwallofc...@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 <els...@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 > > <solidwallofc...@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. > >
