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/11/06 13:32:55 UTC
[PR] Validation of YAML root structure for WIP SplitDNS YAML config [trafficserver]
JosiahWI opened a new pull request, #10732:
URL: https://github.com/apache/trafficserver/pull/10732
This contains just the file loading and root validation part of a YAML loader for SplitDNS. I foresee that the complete feature will be a large change, and I would like feedback now while the changes are small to help guide my design and to increase the effectiveness of the code review.
I am marking this as a draft PR because all these changes are 'dead code' and are not useful yet by themselves.
--
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
Re: [PR] Validation of YAML root structure for WIP SplitDNS YAML config [trafficserver]
Posted by "SolidWallOfCode (via GitHub)" <gi...@apache.org>.
SolidWallOfCode commented on code in PR #10732:
URL: https://github.com/apache/trafficserver/pull/10732#discussion_r1396532786
##########
src/iocore/dns/SplitDNSYAMLLoader.cc:
##########
@@ -0,0 +1,58 @@
+/** @file
+
+ Catch based unit tests for libinknet
+
+ @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.
+ */
+
+#include "iocore/dns/SplitDNSYAMLLoader.h"
+
+#include "tscpp/util/ts_errata.h"
+
+#include <swoc/Errata.h>
+#include <yaml-cpp/yaml.h>
+
+#include <sstream>
+#include <string>
+
+static std::string
+mark_to_error_message(YAML::Mark mark)
Review Comment:
No - use `Errata` and make a formatter for this. See [here](https://github.com/apache/trafficserver/blob/master/src/proxy/IPAllow.cc#L55). In fact, we might want to promote that to 'YamlCfg.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
Re: [PR] Validation of YAML root structure for WIP SplitDNS YAML config [trafficserver]
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on PR #10732:
URL: https://github.com/apache/trafficserver/pull/10732#issuecomment-1806134455
[approve ci centos]
--
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
Re: [PR] Validation of YAML root structure for WIP SplitDNS YAML config [trafficserver]
Posted by "SolidWallOfCode (via GitHub)" <gi...@apache.org>.
SolidWallOfCode commented on code in PR #10732:
URL: https://github.com/apache/trafficserver/pull/10732#discussion_r1396525644
##########
include/iocore/dns/SplitDNSYAMLLoader.h:
##########
@@ -0,0 +1,80 @@
+/** @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 "P_SplitDNSProcessor.h"
+
+#include <swoc/Errata.h>
+#include <swoc/swoc_file.h>
+#include <yaml-cpp/yaml.h>
+
+#include <cstdio>
+#include <ostream>
+#include <string_view>
+#include <system_error>
+#include <utility>
+
+namespace splitdns
+{
+namespace yaml
+{
+ class SplitDNSYAMLLoader
+ {
+ public:
+ static swoc::Errata
Review Comment:
This might be better as `swoc::Rv<self_type>`. Otherwise, what happens to the local loader instance? Doesn't it go out of scope?
--
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
Re: [PR] Validation of YAML root structure for WIP SplitDNS YAML config [trafficserver]
Posted by "SolidWallOfCode (via GitHub)" <gi...@apache.org>.
SolidWallOfCode commented on code in PR #10732:
URL: https://github.com/apache/trafficserver/pull/10732#discussion_r1396527052
##########
include/iocore/dns/SplitDNSYAMLLoader.h:
##########
@@ -0,0 +1,80 @@
+/** @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 "P_SplitDNSProcessor.h"
+
+#include <swoc/Errata.h>
+#include <swoc/swoc_file.h>
+#include <yaml-cpp/yaml.h>
+
+#include <cstdio>
+#include <ostream>
+#include <string_view>
+#include <system_error>
+#include <utility>
+
+namespace splitdns
+{
+namespace yaml
+{
+ class SplitDNSYAMLLoader
+ {
+ public:
+ static swoc::Errata
+ load(std::string const &content, SplitDNS &out)
+ {
+ self_type loader{content, out};
+ loader.setUpSplitDNSFromYAMLTree();
+ // A swoc::Errata may not be copied so we have to explicitly move it.
+ return std::move(loader.err);
+ }
+
+ private:
+ using self_type = SplitDNSYAMLLoader;
+
+ YAML::Node current_node;
+ swoc::Errata err;
+
+ SplitDNSYAMLLoader(std::string const &content, SplitDNS &out) { this->current_node = YAML::Load(content); }
+
+ void setUpSplitDNSFromYAMLTree();
+ };
+
+ inline void
+ load(std::string_view config_filename, SplitDNS &out, std::ostream &errorstream)
+ {
+ std::error_code ec;
+ auto content{swoc::file::load(config_filename, ec)};
+ if (ec.value() == 0) {
+ if (auto err{SplitDNSYAMLLoader::load(content, out)}; !err.is_ok()) {
+ errorstream << err << "While loading " << config_filename << ".\n";
Review Comment:
No - use `Errata`. E.g.
```
inline swoc::Errata ...
if (! ec) {
// ...
return err;
} else { // the else is redundant, since the previous returns.
return swoc::Errata(ERRATA_ERROR, ec, "Failed to load {} : {}", config_filename, ec);
}
--
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
Re: [PR] Validation of YAML root structure for WIP SplitDNS YAML config [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on PR #10732:
URL: https://github.com/apache/trafficserver/pull/10732#issuecomment-1821162502
[approve ci rat]
--
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
Re: [PR] Validation of YAML root structure for WIP SplitDNS YAML config [trafficserver]
Posted by "SolidWallOfCode (via GitHub)" <gi...@apache.org>.
SolidWallOfCode commented on code in PR #10732:
URL: https://github.com/apache/trafficserver/pull/10732#discussion_r1397570571
##########
src/iocore/dns/SplitDNSYAMLLoader.cc:
##########
@@ -0,0 +1,58 @@
+/** @file
+
+ Catch based unit tests for libinknet
+
+ @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.
+ */
+
+#include "iocore/dns/SplitDNSYAMLLoader.h"
+
+#include "tscpp/util/ts_errata.h"
+
+#include <swoc/Errata.h>
+#include <yaml-cpp/yaml.h>
+
+#include <sstream>
+#include <string>
+
+static std::string
+mark_to_error_message(YAML::Mark mark)
Review Comment:
Actually, see #10809
--
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
Re: [PR] Validation of YAML root structure for WIP SplitDNS YAML config [trafficserver]
Posted by "SolidWallOfCode (via GitHub)" <gi...@apache.org>.
SolidWallOfCode commented on code in PR #10732:
URL: https://github.com/apache/trafficserver/pull/10732#discussion_r1396554127
##########
src/iocore/dns/SplitDNSYAMLLoader.cc:
##########
@@ -0,0 +1,58 @@
+/** @file
+
+ Catch based unit tests for libinknet
+
+ @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.
+ */
+
+#include "iocore/dns/SplitDNSYAMLLoader.h"
+
+#include "tscpp/util/ts_errata.h"
+
+#include <swoc/Errata.h>
+#include <yaml-cpp/yaml.h>
+
+#include <sstream>
+#include <string>
+
+static std::string
+mark_to_error_message(YAML::Mark mark)
+{
+ std::stringstream ss;
+ ss << "At line " << mark.line << " column " << mark.column << '.';
+ return ss.str();
+}
+
+namespace splitdns
+{
+namespace yaml
+{
+ void
+ SplitDNSYAMLLoader::setUpSplitDNSFromYAMLTree()
+ {
+ if (!this->current_node["dns"]) {
+ this->err = swoc::Errata(ERRATA_ERROR, "Root tag 'dns' not found.");
+ this->err.note(mark_to_error_message(this->current_node.Mark()));
+ } else if (!this->current_node["dns"]["split"]) {
Review Comment:
Don't look up nodes twice. Cache the previous lookup result.
--
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
Re: [PR] Validation of YAML root structure for WIP SplitDNS YAML config [trafficserver]
Posted by "SolidWallOfCode (via GitHub)" <gi...@apache.org>.
SolidWallOfCode commented on code in PR #10732:
URL: https://github.com/apache/trafficserver/pull/10732#discussion_r1396529184
##########
include/iocore/dns/SplitDNSYAMLLoader.h:
##########
@@ -0,0 +1,80 @@
+/** @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 "P_SplitDNSProcessor.h"
+
+#include <swoc/Errata.h>
+#include <swoc/swoc_file.h>
+#include <yaml-cpp/yaml.h>
+
+#include <cstdio>
+#include <ostream>
+#include <string_view>
+#include <system_error>
+#include <utility>
+
+namespace splitdns
+{
+namespace yaml
+{
+ class SplitDNSYAMLLoader
+ {
+ public:
+ static swoc::Errata
+ load(std::string const &content, SplitDNS &out)
+ {
+ self_type loader{content, out};
+ loader.setUpSplitDNSFromYAMLTree();
+ // A swoc::Errata may not be copied so we have to explicitly move it.
+ return std::move(loader.err);
+ }
+
+ private:
+ using self_type = SplitDNSYAMLLoader;
+
+ YAML::Node current_node;
+ swoc::Errata err;
+
+ SplitDNSYAMLLoader(std::string const &content, SplitDNS &out) { this->current_node = YAML::Load(content); }
+
+ void setUpSplitDNSFromYAMLTree();
+ };
+
+ inline void
+ load(std::string_view config_filename, SplitDNS &out, std::ostream &errorstream)
+ {
+ std::error_code ec;
+ auto content{swoc::file::load(config_filename, ec)};
+ if (ec.value() == 0) {
+ if (auto err{SplitDNSYAMLLoader::load(content, out)}; !err.is_ok()) {
+ errorstream << err << "While loading " << config_filename << ".\n";
Review Comment:
On a higher level, for code like this, to avoid excessive nesting, it is generally best to check the error first, _if that will result in an early return_. E.g.
```
if (ec) {
return swoc::Errata(...);
}
// file load was successful, check the contents.
if (auto err = load(...) ; !err.is_ok()) {
err.note("While loading config file {}", config_filename);
return err;
}
--
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
Re: [PR] Validation of YAML root structure for WIP SplitDNS YAML config [trafficserver]
Posted by "SolidWallOfCode (via GitHub)" <gi...@apache.org>.
SolidWallOfCode commented on code in PR #10732:
URL: https://github.com/apache/trafficserver/pull/10732#discussion_r1396527052
##########
include/iocore/dns/SplitDNSYAMLLoader.h:
##########
@@ -0,0 +1,80 @@
+/** @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 "P_SplitDNSProcessor.h"
+
+#include <swoc/Errata.h>
+#include <swoc/swoc_file.h>
+#include <yaml-cpp/yaml.h>
+
+#include <cstdio>
+#include <ostream>
+#include <string_view>
+#include <system_error>
+#include <utility>
+
+namespace splitdns
+{
+namespace yaml
+{
+ class SplitDNSYAMLLoader
+ {
+ public:
+ static swoc::Errata
+ load(std::string const &content, SplitDNS &out)
+ {
+ self_type loader{content, out};
+ loader.setUpSplitDNSFromYAMLTree();
+ // A swoc::Errata may not be copied so we have to explicitly move it.
+ return std::move(loader.err);
+ }
+
+ private:
+ using self_type = SplitDNSYAMLLoader;
+
+ YAML::Node current_node;
+ swoc::Errata err;
+
+ SplitDNSYAMLLoader(std::string const &content, SplitDNS &out) { this->current_node = YAML::Load(content); }
+
+ void setUpSplitDNSFromYAMLTree();
+ };
+
+ inline void
+ load(std::string_view config_filename, SplitDNS &out, std::ostream &errorstream)
+ {
+ std::error_code ec;
+ auto content{swoc::file::load(config_filename, ec)};
+ if (ec.value() == 0) {
+ if (auto err{SplitDNSYAMLLoader::load(content, out)}; !err.is_ok()) {
+ errorstream << err << "While loading " << config_filename << ".\n";
Review Comment:
No - use `Errata`. E.g.
```
inline swoc::Errata ...
if (! ec) {
// ...
return err;
} else {
return swoc::Errata(ERRATA_ERROR, ec, "Failed to load {} : {}", config_filename, ec);
}
--
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
Re: [PR] Validation of YAML root structure for WIP SplitDNS YAML config [trafficserver]
Posted by "SolidWallOfCode (via GitHub)" <gi...@apache.org>.
SolidWallOfCode commented on code in PR #10732:
URL: https://github.com/apache/trafficserver/pull/10732#discussion_r1396523985
##########
include/iocore/dns/SplitDNSYAMLLoader.h:
##########
@@ -0,0 +1,80 @@
+/** @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 "P_SplitDNSProcessor.h"
+
+#include <swoc/Errata.h>
+#include <swoc/swoc_file.h>
+#include <yaml-cpp/yaml.h>
+
+#include <cstdio>
+#include <ostream>
+#include <string_view>
+#include <system_error>
+#include <utility>
+
+namespace splitdns
+{
+namespace yaml
+{
+ class SplitDNSYAMLLoader
+ {
+ public:
+ static swoc::Errata
Review Comment:
Bad idea, don't do it.
--
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
Re: [PR] Validation of YAML root structure for WIP SplitDNS YAML config [trafficserver]
Posted by "SolidWallOfCode (via GitHub)" <gi...@apache.org>.
SolidWallOfCode commented on code in PR #10732:
URL: https://github.com/apache/trafficserver/pull/10732#discussion_r1396523702
##########
include/iocore/dns/SplitDNSYAMLLoader.h:
##########
@@ -0,0 +1,80 @@
+/** @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 "P_SplitDNSProcessor.h"
+
+#include <swoc/Errata.h>
Review Comment:
This should be "tscpp/util/ts_errata.h". That include "swoc/Errata.h" plus some other utilities.
--
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
Re: [PR] Validation of YAML root structure for WIP SplitDNS YAML config [trafficserver]
Posted by "SolidWallOfCode (via GitHub)" <gi...@apache.org>.
SolidWallOfCode commented on code in PR #10732:
URL: https://github.com/apache/trafficserver/pull/10732#discussion_r1396554127
##########
src/iocore/dns/SplitDNSYAMLLoader.cc:
##########
@@ -0,0 +1,58 @@
+/** @file
+
+ Catch based unit tests for libinknet
+
+ @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.
+ */
+
+#include "iocore/dns/SplitDNSYAMLLoader.h"
+
+#include "tscpp/util/ts_errata.h"
+
+#include <swoc/Errata.h>
+#include <yaml-cpp/yaml.h>
+
+#include <sstream>
+#include <string>
+
+static std::string
+mark_to_error_message(YAML::Mark mark)
+{
+ std::stringstream ss;
+ ss << "At line " << mark.line << " column " << mark.column << '.';
+ return ss.str();
+}
+
+namespace splitdns
+{
+namespace yaml
+{
+ void
+ SplitDNSYAMLLoader::setUpSplitDNSFromYAMLTree()
+ {
+ if (!this->current_node["dns"]) {
+ this->err = swoc::Errata(ERRATA_ERROR, "Root tag 'dns' not found.");
+ this->err.note(mark_to_error_message(this->current_node.Mark()));
+ } else if (!this->current_node["dns"]["split"]) {
Review Comment:
Don't look up nodes twice. Cache the previous lookup result. See examples [here](https://github.com/apache/trafficserver/blob/master/src/proxy/IPAllow.cc#L341)
--
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
Re: [PR] Validation of YAML root structure for WIP SplitDNS YAML config [trafficserver]
Posted by "SolidWallOfCode (via GitHub)" <gi...@apache.org>.
SolidWallOfCode commented on code in PR #10732:
URL: https://github.com/apache/trafficserver/pull/10732#discussion_r1396529184
##########
include/iocore/dns/SplitDNSYAMLLoader.h:
##########
@@ -0,0 +1,80 @@
+/** @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 "P_SplitDNSProcessor.h"
+
+#include <swoc/Errata.h>
+#include <swoc/swoc_file.h>
+#include <yaml-cpp/yaml.h>
+
+#include <cstdio>
+#include <ostream>
+#include <string_view>
+#include <system_error>
+#include <utility>
+
+namespace splitdns
+{
+namespace yaml
+{
+ class SplitDNSYAMLLoader
+ {
+ public:
+ static swoc::Errata
+ load(std::string const &content, SplitDNS &out)
+ {
+ self_type loader{content, out};
+ loader.setUpSplitDNSFromYAMLTree();
+ // A swoc::Errata may not be copied so we have to explicitly move it.
+ return std::move(loader.err);
+ }
+
+ private:
+ using self_type = SplitDNSYAMLLoader;
+
+ YAML::Node current_node;
+ swoc::Errata err;
+
+ SplitDNSYAMLLoader(std::string const &content, SplitDNS &out) { this->current_node = YAML::Load(content); }
+
+ void setUpSplitDNSFromYAMLTree();
+ };
+
+ inline void
+ load(std::string_view config_filename, SplitDNS &out, std::ostream &errorstream)
+ {
+ std::error_code ec;
+ auto content{swoc::file::load(config_filename, ec)};
+ if (ec.value() == 0) {
+ if (auto err{SplitDNSYAMLLoader::load(content, out)}; !err.is_ok()) {
+ errorstream << err << "While loading " << config_filename << ".\n";
Review Comment:
On a higher level, for code like this, to avoid excessive nesting, it is generally best to check the error first, _if that will result in an early return_. E.g.
```
if (ec) {
return swoc::Errata(...);
}
// file load was successful, check the contents.
if (auto err = load(...) ; !err.is_ok()) {
err.note("While loading config file {}", config_filename);
return err;
}
// Parse succeeded. We don't care about err since we know it was success.
--
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
Re: [PR] Validation of YAML root structure for WIP SplitDNS YAML config [trafficserver]
Posted by "SolidWallOfCode (via GitHub)" <gi...@apache.org>.
SolidWallOfCode commented on code in PR #10732:
URL: https://github.com/apache/trafficserver/pull/10732#discussion_r1397572585
##########
src/iocore/dns/unit_tests/test_SplitDNSYAMLLoader.cc:
##########
@@ -0,0 +1,82 @@
+/** @file
+
+ Catch based unit tests for SplitDNS
+
+ @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.
+ */
+
+#include "iocore/dns/SplitDNSYAMLLoader.h"
+#include "P_SplitDNSProcessor.h"
+
+#include <catch.hpp>
+
+#include <sstream>
+#include <string_view>
+
+static bool
+error_contains(std::string_view error, std::string_view substr)
+{
+ return error.find(substr) != std::string_view::npos;
Review Comment:
Oops! You're correct. I wonder if I should put it in `TextView`.
--
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
Re: [PR] Validation of YAML root structure for WIP SplitDNS YAML config [trafficserver]
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on PR #10732:
URL: https://github.com/apache/trafficserver/pull/10732#issuecomment-1806156360
I have to add FetchSM to the new stub now.
--
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
Re: [PR] Validation of YAML root structure for WIP SplitDNS YAML config [trafficserver]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #10732: Validation of YAML root structure for WIP SplitDNS YAML config
URL: https://github.com/apache/trafficserver/pull/10732
--
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
Re: [PR] Validation of YAML root structure for WIP SplitDNS YAML config [trafficserver]
Posted by "SolidWallOfCode (via GitHub)" <gi...@apache.org>.
SolidWallOfCode commented on code in PR #10732:
URL: https://github.com/apache/trafficserver/pull/10732#discussion_r1396523985
##########
include/iocore/dns/SplitDNSYAMLLoader.h:
##########
@@ -0,0 +1,80 @@
+/** @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 "P_SplitDNSProcessor.h"
+
+#include <swoc/Errata.h>
+#include <swoc/swoc_file.h>
+#include <yaml-cpp/yaml.h>
+
+#include <cstdio>
+#include <ostream>
+#include <string_view>
+#include <system_error>
+#include <utility>
+
+namespace splitdns
+{
+namespace yaml
+{
+ class SplitDNSYAMLLoader
+ {
+ public:
+ static swoc::Errata
Review Comment:
Bad idea, don't do it.
--
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
Re: [PR] Validation of YAML root structure for WIP SplitDNS YAML config [trafficserver]
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on code in PR #10732:
URL: https://github.com/apache/trafficserver/pull/10732#discussion_r1396568057
##########
src/iocore/dns/unit_tests/test_SplitDNSYAMLLoader.cc:
##########
@@ -0,0 +1,82 @@
+/** @file
+
+ Catch based unit tests for SplitDNS
+
+ @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.
+ */
+
+#include "iocore/dns/SplitDNSYAMLLoader.h"
+#include "P_SplitDNSProcessor.h"
+
+#include <catch.hpp>
+
+#include <sstream>
+#include <string_view>
+
+static bool
+error_contains(std::string_view error, std::string_view substr)
+{
+ return error.find(substr) != std::string_view::npos;
Review Comment:
Isn't that a C++23 feature?
--
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
Re: [PR] Validation of YAML root structure for WIP SplitDNS YAML config [trafficserver]
Posted by "SolidWallOfCode (via GitHub)" <gi...@apache.org>.
SolidWallOfCode commented on code in PR #10732:
URL: https://github.com/apache/trafficserver/pull/10732#discussion_r1396532786
##########
src/iocore/dns/SplitDNSYAMLLoader.cc:
##########
@@ -0,0 +1,58 @@
+/** @file
+
+ Catch based unit tests for libinknet
+
+ @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.
+ */
+
+#include "iocore/dns/SplitDNSYAMLLoader.h"
+
+#include "tscpp/util/ts_errata.h"
+
+#include <swoc/Errata.h>
+#include <yaml-cpp/yaml.h>
+
+#include <sstream>
+#include <string>
+
+static std::string
+mark_to_error_message(YAML::Mark mark)
Review Comment:
No - use `Errata` and make a formatter for this. See [here](https://github.com/apache/trafficserver/blob/master/src/proxy/IPAllow.cc#L55). In fact, we might want to promote that to 'YamlCfg.h". With that, you can use "{}" and pass `YAML::Mark` as the argument.
--
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
Re: [PR] Validation of YAML root structure for WIP SplitDNS YAML config [trafficserver]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #10732:
URL: https://github.com/apache/trafficserver/pull/10732#issuecomment-2038610065
This pull request has been automatically marked as stale because it has not had recent activity. Marking it stale to flag it for further consideration by the community.
--
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
Re: [PR] Validation of YAML root structure for WIP SplitDNS YAML config [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on PR #10732:
URL: https://github.com/apache/trafficserver/pull/10732#issuecomment-1800292602
[approve ci clang-format]
--
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
Re: [PR] Validation of YAML root structure for WIP SplitDNS YAML config [trafficserver]
Posted by "SolidWallOfCode (via GitHub)" <gi...@apache.org>.
SolidWallOfCode commented on code in PR #10732:
URL: https://github.com/apache/trafficserver/pull/10732#discussion_r1396523285
##########
include/iocore/dns/SplitDNSYAMLLoader.h:
##########
@@ -0,0 +1,80 @@
+/** @file
+
+ A brief file description
Review Comment:
Let's do just a bit better on this comment.
--
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
Re: [PR] Validation of YAML root structure for WIP SplitDNS YAML config [trafficserver]
Posted by "SolidWallOfCode (via GitHub)" <gi...@apache.org>.
SolidWallOfCode commented on code in PR #10732:
URL: https://github.com/apache/trafficserver/pull/10732#discussion_r1396525092
##########
include/iocore/dns/SplitDNSYAMLLoader.h:
##########
@@ -0,0 +1,80 @@
+/** @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 "P_SplitDNSProcessor.h"
+
+#include <swoc/Errata.h>
+#include <swoc/swoc_file.h>
+#include <yaml-cpp/yaml.h>
+
+#include <cstdio>
+#include <ostream>
+#include <string_view>
+#include <system_error>
+#include <utility>
+
+namespace splitdns
+{
+namespace yaml
+{
+ class SplitDNSYAMLLoader
+ {
+ public:
+ static swoc::Errata
+ load(std::string const &content, SplitDNS &out)
+ {
+ self_type loader{content, out};
+ loader.setUpSplitDNSFromYAMLTree();
+ // A swoc::Errata may not be copied so we have to explicitly move it.
+ return std::move(loader.err);
+ }
+
+ private:
+ using self_type = SplitDNSYAMLLoader;
+
+ YAML::Node current_node;
+ swoc::Errata err;
Review Comment:
Bad idea - don't do 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
Re: [PR] Validation of YAML root structure for WIP SplitDNS YAML config [trafficserver]
Posted by "SolidWallOfCode (via GitHub)" <gi...@apache.org>.
SolidWallOfCode commented on code in PR #10732:
URL: https://github.com/apache/trafficserver/pull/10732#discussion_r1396557150
##########
src/iocore/dns/unit_tests/test_SplitDNSYAMLLoader.cc:
##########
@@ -0,0 +1,82 @@
+/** @file
+
+ Catch based unit tests for SplitDNS
+
+ @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.
+ */
+
+#include "iocore/dns/SplitDNSYAMLLoader.h"
+#include "P_SplitDNSProcessor.h"
+
+#include <catch.hpp>
+
+#include <sstream>
+#include <string_view>
+
+static bool
+error_contains(std::string_view error, std::string_view substr)
+{
+ return error.find(substr) != std::string_view::npos;
Review Comment:
Why not just use [`std::string_view::contains`](https://en.cppreference.com/w/cpp/string/basic_string_view/contains)?
##########
src/iocore/dns/unit_tests/test_SplitDNSYAMLLoader.cc:
##########
@@ -0,0 +1,82 @@
+/** @file
+
+ Catch based unit tests for SplitDNS
+
+ @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.
+ */
+
+#include "iocore/dns/SplitDNSYAMLLoader.h"
+#include "P_SplitDNSProcessor.h"
+
+#include <catch.hpp>
+
+#include <sstream>
+#include <string_view>
+
+static bool
+error_contains(std::string_view error, std::string_view substr)
+{
+ return error.find(substr) != std::string_view::npos;
Review Comment:
Why not use [`std::string_view::contains`](https://en.cppreference.com/w/cpp/string/basic_string_view/contains)?
--
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