You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "JosiahWI (via GitHub)" <gi...@apache.org> on 2023/06/16 20:35:37 UTC
[GitHub] [trafficserver] JosiahWI opened a new pull request, #9869: Extract HttpUserAgent class from HttpSM
JosiahWI opened a new pull request, #9869:
URL: https://github.com/apache/trafficserver/pull/9869
This extracts a new class with 3 fields (entry, raw_buffer_reader, and txn). It doesn't have much responsibility yet. But it does provide a starting point for moving some behavior out of the state machine.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on pull request #9869: Extract HttpUserAgent class from HttpSM
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on PR #9869:
URL: https://github.com/apache/trafficserver/pull/9869#issuecomment-1595515611
[approve ci]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] JosiahWI commented on a diff in pull request #9869: Extract HttpUserAgent class from HttpSM
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on code in PR #9869:
URL: https://github.com/apache/trafficserver/pull/9869#discussion_r1232918349
##########
proxy/http/HttpSM.h:
##########
@@ -569,9 +545,7 @@ class HttpSM : public Continuation, public PluginUserArgs<TS_USER_ARGS_TXN>
HttpVCTable vc_table;
- IOBufferReader *ua_raw_buffer_reader = nullptr;
-
- HttpVCTableEntry *ua_entry = nullptr;
+ HttpUserAgent _ua{};
Review Comment:
`HttpServer` seems like a good name. It might be a good idea to try this in a separate PR, unless you are ok with having a pretty large diff? This one is already a little bigger than I thought would be nice to review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] JosiahWI commented on pull request #9869: Extract HttpUserAgent class from HttpSM
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on PR #9869:
URL: https://github.com/apache/trafficserver/pull/9869#issuecomment-1608438271
The autest failure on this one indicates a real problem. I've been working on it but debugging the state machine without unit tests is a relatively large time sink.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9869: Extract HttpUserAgent class from HttpSM
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9869:
URL: https://github.com/apache/trafficserver/pull/9869#discussion_r1232893363
##########
proxy/http/HttpSM.h:
##########
@@ -101,20 +92,6 @@ enum BackgroundFill_t {
extern ink_mutex debug_sm_list_mutex;
-struct HttpVCTableEntry {
- VConnection *vc;
- MIOBuffer *read_buffer;
- MIOBuffer *write_buffer;
- VIO *read_vio;
- VIO *write_vio;
- HttpSMHandler vc_read_handler;
- HttpSMHandler vc_write_handler;
- HttpVC_t vc_type;
- HttpSM *sm;
- bool eos;
- bool in_tunnel;
-};
-
struct HttpVCTable {
Review Comment:
Having `HttpVCTableEntry` and `HttpVCTable` at different places doesn't look like a right thing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] JosiahWI commented on pull request #9869: Extract HttpUserAgent class from HttpSM
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on PR #9869:
URL: https://github.com/apache/trafficserver/pull/9869#issuecomment-1599603626
The goal is to encapsulate these fields such that the state machine doesn't need to access this data at all. If it has less data it has to manage directly it will be a lot easier to follow the logic. I want to move operations on client and server data out of the state machine, so that the state machine focuses on changing between states correctly and delegates the details of changing the state to those new classes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] JosiahWI commented on pull request #9869: Extract HttpUserAgent class from HttpSM
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on PR #9869:
URL: https://github.com/apache/trafficserver/pull/9869#issuecomment-1599582563
I wanted to split up the class a bit based on the single responsibility principle to make it more manageable. I did not have a long-term vision of how the refactoring would continue, but I'd like to try moving the other client fields over (in this PR) like you suggested, and splitting out `HttpServer` in a separate one.
I'm pretty new to the codebase and it's not very clear to me yet which fields belong together. I thought client and user agent were somehow different concepts in the codebase, because they are named differently.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9869: Extract HttpUserAgent class from HttpSM
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9869:
URL: https://github.com/apache/trafficserver/pull/9869#discussion_r1232896275
##########
proxy/http/HttpSM.h:
##########
@@ -569,9 +545,7 @@ class HttpSM : public Continuation, public PluginUserArgs<TS_USER_ARGS_TXN>
HttpVCTable vc_table;
- IOBufferReader *ua_raw_buffer_reader = nullptr;
-
- HttpVCTableEntry *ua_entry = nullptr;
+ HttpUserAgent _ua{};
Review Comment:
It looks like we could do a similar thing for `server_entry` and `server_txn`. What would it be called? `HttpServer`? It would be nice if the two had a same prefix or suffix in their names to indicate they are basically the same thing but one for client side and one for server side. We might want to have a base class for them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] JosiahWI commented on pull request #9869: Extract HttpUserAgent class from HttpSM
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on PR #9869:
URL: https://github.com/apache/trafficserver/pull/9869#issuecomment-1602684357
Rebased on master.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bryancall commented on pull request #9869: Extract HttpUserAgent class from HttpSM
Posted by "bryancall (via GitHub)" <gi...@apache.org>.
bryancall commented on PR #9869:
URL: https://github.com/apache/trafficserver/pull/9869#issuecomment-1608415685
[approve ci autest]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] JosiahWI commented on pull request #9869: Extract HttpUserAgent class from HttpSM
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on PR #9869:
URL: https://github.com/apache/trafficserver/pull/9869#issuecomment-1607984326
Rebased on master.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #9869: Extract HttpUserAgent class from HttpSM
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on PR #9869:
URL: https://github.com/apache/trafficserver/pull/9869#issuecomment-1599694880
Thanks for the clarification.
> I wanted to split up the class a bit based on the single responsibility principle to make it more manageable.
Sounds reasonable. You've already answered my question on the other comment but I was not sure what the responsibility of the new class where you have only one for user agent side.
> I'm pretty new to the codebase and it's not very clear to me yet which fields belong together. I thought client and user agent were somehow different concepts in the codebase, because they are named differently.
I'd say nobody fully understand the concepts 🙂 The two terms might have been used for a client on HTTP layer and a client on TCP layer, but this is just my guess.
> The goal is to encapsulate these fields such that the state machine doesn't need to access this data at all. If it has less data it has to manage directly it will be a lot easier to follow the logic.
Sounds great. Let's make it happen.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit merged pull request #9869: Extract HttpUserAgent class from HttpSM
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit merged PR #9869:
URL: https://github.com/apache/trafficserver/pull/9869
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] JosiahWI commented on pull request #9869: Extract HttpUserAgent class from HttpSM
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on PR #9869:
URL: https://github.com/apache/trafficserver/pull/9869#issuecomment-1609823289
I found the bug. I missed a single !, and I think I spent as much time finding that single missing ! as I spent making this entire PR. I'm going to add a unit test that covers that case, but I'm encountering a lot of segfaults in the test setup.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] JosiahWI commented on pull request #9869: Extract HttpUserAgent class from HttpSM
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on PR #9869:
URL: https://github.com/apache/trafficserver/pull/9869#issuecomment-1595567274
My objective is to move some of the logic for response headers into this new class. For example, `HttpSM::setup_push_read_response_header()` can be moved entirely or in part into this new class.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9869: Extract HttpUserAgent class from HttpSM
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9869:
URL: https://github.com/apache/trafficserver/pull/9869#discussion_r1232896275
##########
proxy/http/HttpSM.h:
##########
@@ -569,9 +545,7 @@ class HttpSM : public Continuation, public PluginUserArgs<TS_USER_ARGS_TXN>
HttpVCTable vc_table;
- IOBufferReader *ua_raw_buffer_reader = nullptr;
-
- HttpVCTableEntry *ua_entry = nullptr;
+ HttpUserAgent _ua{};
Review Comment:
It looks like we could do a similar thing for `server_entry` and `server_txn`. What would it be called? `HttpServer`? It would be nice if the two had a same prefix or suffix in their names to indicate they are basically the same thing but one for client side and one for server side.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] JosiahWI commented on a diff in pull request #9869: Extract HttpUserAgent class from HttpSM
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on code in PR #9869:
URL: https://github.com/apache/trafficserver/pull/9869#discussion_r1232918018
##########
proxy/http/HttpSM.h:
##########
@@ -101,20 +92,6 @@ enum BackgroundFill_t {
extern ink_mutex debug_sm_list_mutex;
-struct HttpVCTableEntry {
- VConnection *vc;
- MIOBuffer *read_buffer;
- MIOBuffer *write_buffer;
- VIO *read_vio;
- VIO *write_vio;
- HttpSMHandler vc_read_handler;
- HttpSMHandler vc_write_handler;
- HttpVC_t vc_type;
- HttpSM *sm;
- bool eos;
- bool in_tunnel;
-};
-
struct HttpVCTable {
Review Comment:
Good catch. They should probably go in the same header.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9869: Extract HttpUserAgent class from HttpSM
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9869:
URL: https://github.com/apache/trafficserver/pull/9869#discussion_r1261451197
##########
proxy/http/HttpSM.h:
##########
@@ -679,10 +615,16 @@ HttpTransact::State::state_machine_id() const
return state_machine->sm_id;
}
+inline HttpUserAgent const &
+HttpSM::get_user_agent() const
+{
+ return _ua;
+}
+
inline ProxyTransaction *
HttpSM::get_ua_txn()
Review Comment:
I wondered if we should keep this because now we can do `sm->get_user_agent()->get_txn()`. Maybe we should keep it, so we can get a user-agent transaction and a server transaction in a consistent way.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] JosiahWI commented on pull request #9869: Extract HttpUserAgent class from HttpSM
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on PR #9869:
URL: https://github.com/apache/trafficserver/pull/9869#issuecomment-1633035872
There isn't supposed to be any logic change. The AuTests passing gives me a little confidence that the behavior is preserved, and I am planning to add some more unit tests in a followup PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] JosiahWI commented on pull request #9869: Extract HttpUserAgent class from HttpSM
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on PR #9869:
URL: https://github.com/apache/trafficserver/pull/9869#issuecomment-1632847475
I'm wondering whether it's worth it to build the new test in the automake build. In the CMake build it was convenient to build a single test executable, but the automake build has things split into multiple test executables, and building the one new test would require a large amount of boilerplate (linking 10+ libs or whatnot), and I'm hoping the automake build will eventually be removed anyway so we don't have to maintain two build systems.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #9869: Extract HttpUserAgent class from HttpSM
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on PR #9869:
URL: https://github.com/apache/trafficserver/pull/9869#issuecomment-1599413737
Hmm, the goal is still unclear to me. If it's going to be like we can get anything about a user agent from `HttpUserAgent`, and anything about a server from `HttpServer` that kinda make sense. But we already have `client_info` and `server_info` in `HttpTransact::State`. What would be the difference? And why don't you move other variables that are related to client (e.g. `client_request_hdr_bytes`, `client_tcp_reused`, client_protocol`, etc) into the class?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] JosiahWI commented on pull request #9869: Extract HttpUserAgent class from HttpSM
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on PR #9869:
URL: https://github.com/apache/trafficserver/pull/9869#issuecomment-1628831067
Rebased on master. Some unfinished changes also got pushed by mistake.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] JosiahWI commented on a diff in pull request #9869: Extract HttpUserAgent class from HttpSM
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on code in PR #9869:
URL: https://github.com/apache/trafficserver/pull/9869#discussion_r1232917880
##########
proxy/http/HttpUserAgent.h:
##########
@@ -0,0 +1,110 @@
+/** @file
+
+ A brief file description
+
+ @section license License
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+ */
+
+#pragma once
+
+#include "I_IOBuffer.h"
+#include "I_VConnection.h"
+#include "I_VIO.h"
+#include "ProxyTransaction.h"
+
+class HttpSM;
+using HttpSMHandler = int (HttpSM::*)(int, void *);
+
+enum HttpVC_t {
+ HTTP_UNKNOWN = 0,
+ HTTP_UA_VC,
+ HTTP_SERVER_VC,
+ HTTP_TRANSFORM_VC,
+ HTTP_CACHE_READ_VC,
+ HTTP_CACHE_WRITE_VC,
+ HTTP_RAW_SERVER_VC
+};
+
+struct HttpVCTableEntry {
+ VConnection *vc;
+ MIOBuffer *read_buffer;
+ MIOBuffer *write_buffer;
+ VIO *read_vio;
+ VIO *write_vio;
+ HttpSMHandler vc_read_handler;
+ HttpSMHandler vc_write_handler;
+ HttpVC_t vc_type;
+ HttpSM *sm;
+ bool eos;
+ bool in_tunnel;
+};
Review Comment:
Do you have a name in mind for this header file?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9869: Extract HttpUserAgent class from HttpSM
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9869:
URL: https://github.com/apache/trafficserver/pull/9869#discussion_r1235781795
##########
proxy/http/HttpSM.h:
##########
@@ -569,9 +545,7 @@ class HttpSM : public Continuation, public PluginUserArgs<TS_USER_ARGS_TXN>
HttpVCTable vc_table;
- IOBufferReader *ua_raw_buffer_reader = nullptr;
-
- HttpVCTableEntry *ua_entry = nullptr;
+ HttpUserAgent _ua{};
Review Comment:
I'm not requesting the change for server on this PR, but I want to know what you are going to do.
If we're going to have `HttpServer` and it has `m_entry`, `m_raw_buffer_reader` and `m_txn`, I'd make a base class and probably name it `HttpPeer` or something, and I don't think the base class is too much addition. But that could be different from what you have on your mind. I couldn't read your plan from this change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9869: Extract HttpUserAgent class from HttpSM
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9869:
URL: https://github.com/apache/trafficserver/pull/9869#discussion_r1242441821
##########
proxy/http/HttpSM.h:
##########
@@ -679,10 +622,58 @@ HttpTransact::State::state_machine_id() const
return state_machine->sm_id;
}
+inline bool
+HttpSM::get_client_tcp_reused() const
+{
+ return _ua.get_client_tcp_reused();
+}
+
+inline bool
+HttpSM::get_client_ssl_reused() const
+{
+ return _ua.get_client_ssl_reused();
+}
+
+inline bool
+HttpSM::get_client_connection_is_ssl() const
+{
+ return _ua.get_client_connection_is_ssl();
+}
+
+inline char const *
+HttpSM::get_client_protocol() const
+{
+ return _ua.get_client_protocol();
+}
+
+inline char const *
+HttpSM::get_client_sec_protocol() const
+{
+ return _ua.get_client_sec_protocol();
+}
+
+inline char const *
+HttpSM::get_client_cipher_suite() const
+{
+ return _ua.get_client_cipher_suite();
+}
+
+inline char const *
+HttpSM::get_client_curve() const
+{
+ return _ua.get_client_curve();
+}
+
+inline int
+HttpSM::get_client_alpn_id() const
+{
+ return _ua.get_client_alpn_id();
+}
+
Review Comment:
So, we have to add a function to `HttpSM` when we add a function to `HttpUserAgent`? Is it a bad idea to have `HttpSM::get_client` or `HttpSM::get_useragent` so we don't have to have these?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] JosiahWI commented on a diff in pull request #9869: Extract HttpUserAgent class from HttpSM
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on code in PR #9869:
URL: https://github.com/apache/trafficserver/pull/9869#discussion_r1235908556
##########
proxy/http/HttpSM.h:
##########
@@ -569,9 +545,7 @@ class HttpSM : public Continuation, public PluginUserArgs<TS_USER_ARGS_TXN>
HttpVCTable vc_table;
- IOBufferReader *ua_raw_buffer_reader = nullptr;
-
- HttpVCTableEntry *ua_entry = nullptr;
+ HttpUserAgent _ua{};
Review Comment:
I hadn't thought as far ahead as extracting `HttpServer` and making a base class, but I think that's a good direction to take this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] JosiahWI commented on a diff in pull request #9869: Extract HttpUserAgent class from HttpSM
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on code in PR #9869:
URL: https://github.com/apache/trafficserver/pull/9869#discussion_r1235392189
##########
proxy/http/HttpUserAgent.h:
##########
@@ -0,0 +1,110 @@
+/** @file
+
+ A brief file description
+
+ @section license License
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+ */
+
+#pragma once
+
+#include "I_IOBuffer.h"
+#include "I_VConnection.h"
+#include "I_VIO.h"
+#include "ProxyTransaction.h"
+
+class HttpSM;
+using HttpSMHandler = int (HttpSM::*)(int, void *);
+
+enum HttpVC_t {
+ HTTP_UNKNOWN = 0,
+ HTTP_UA_VC,
+ HTTP_SERVER_VC,
+ HTTP_TRANSFORM_VC,
+ HTTP_CACHE_READ_VC,
+ HTTP_CACHE_WRITE_VC,
+ HTTP_RAW_SERVER_VC
+};
+
+struct HttpVCTableEntry {
+ VConnection *vc;
+ MIOBuffer *read_buffer;
+ MIOBuffer *write_buffer;
+ VIO *read_vio;
+ VIO *write_vio;
+ HttpSMHandler vc_read_handler;
+ HttpSMHandler vc_write_handler;
+ HttpVC_t vc_type;
+ HttpSM *sm;
+ bool eos;
+ bool in_tunnel;
+};
Review Comment:
I've selected HttpVCTable.h.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9869: Extract HttpUserAgent class from HttpSM
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9869:
URL: https://github.com/apache/trafficserver/pull/9869#discussion_r1232892544
##########
proxy/http/HttpUserAgent.h:
##########
@@ -0,0 +1,110 @@
+/** @file
+
+ A brief file description
+
+ @section license License
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+ */
+
+#pragma once
+
+#include "I_IOBuffer.h"
+#include "I_VConnection.h"
+#include "I_VIO.h"
+#include "ProxyTransaction.h"
+
+class HttpSM;
+using HttpSMHandler = int (HttpSM::*)(int, void *);
+
+enum HttpVC_t {
+ HTTP_UNKNOWN = 0,
+ HTTP_UA_VC,
+ HTTP_SERVER_VC,
+ HTTP_TRANSFORM_VC,
+ HTTP_CACHE_READ_VC,
+ HTTP_CACHE_WRITE_VC,
+ HTTP_RAW_SERVER_VC
+};
+
+struct HttpVCTableEntry {
+ VConnection *vc;
+ MIOBuffer *read_buffer;
+ MIOBuffer *write_buffer;
+ VIO *read_vio;
+ VIO *write_vio;
+ HttpSMHandler vc_read_handler;
+ HttpSMHandler vc_write_handler;
+ HttpVC_t vc_type;
+ HttpSM *sm;
+ bool eos;
+ bool in_tunnel;
+};
Review Comment:
These are obviously not just for "UserAgent". Probably need another header file if they can't stay in HttpSM.h.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] JosiahWI commented on pull request #9869: Extract HttpUserAgent class from HttpSM
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on PR #9869:
URL: https://github.com/apache/trafficserver/pull/9869#issuecomment-1632798321
[approve ci autest]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] JosiahWI commented on pull request #9869: Extract HttpUserAgent class from HttpSM
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on PR #9869:
URL: https://github.com/apache/trafficserver/pull/9869#issuecomment-1632411298
Rebased on master to resolve merge conflict.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] JosiahWI commented on a diff in pull request #9869: Extract HttpUserAgent class from HttpSM
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on code in PR #9869:
URL: https://github.com/apache/trafficserver/pull/9869#discussion_r1242468725
##########
proxy/http/HttpSM.h:
##########
@@ -679,10 +622,58 @@ HttpTransact::State::state_machine_id() const
return state_machine->sm_id;
}
+inline bool
+HttpSM::get_client_tcp_reused() const
+{
+ return _ua.get_client_tcp_reused();
+}
+
+inline bool
+HttpSM::get_client_ssl_reused() const
+{
+ return _ua.get_client_ssl_reused();
+}
+
+inline bool
+HttpSM::get_client_connection_is_ssl() const
+{
+ return _ua.get_client_connection_is_ssl();
+}
+
+inline char const *
+HttpSM::get_client_protocol() const
+{
+ return _ua.get_client_protocol();
+}
+
+inline char const *
+HttpSM::get_client_sec_protocol() const
+{
+ return _ua.get_client_sec_protocol();
+}
+
+inline char const *
+HttpSM::get_client_cipher_suite() const
+{
+ return _ua.get_client_cipher_suite();
+}
+
+inline char const *
+HttpSM::get_client_curve() const
+{
+ return _ua.get_client_curve();
+}
+
+inline int
+HttpSM::get_client_alpn_id() const
+{
+ return _ua.get_client_alpn_id();
+}
+
Review Comment:
I was asking myself this question as well. Since the number of getters here is slightly getting out of hand, I think we should go with your suggestion. The goal is to be able to get rid of many of these getters, but that change will also be easier to handle if we don't have to remove getters in 2 places every time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org