bmahler commented on code in PR #487:
URL: https://github.com/apache/mesos/pull/487#discussion_r1465354330
##########
src/linux/routing/queueing/internal.hpp:
##########
@@ -118,7 +120,69 @@ Try<Netlink<struct rtnl_qdisc>> encodeDiscipline(
}
/////////////////////////////////////////////////
-// Helpers for internal APIs.
+// Helpers for {en}decoding queueing classes.
+/////////////////////////////////////////////////
+
+namespace cls {
+// Forward declaration. Each type of queueing class needs to implement
+// this function to encode its type specific configurations into the
+// libnl queueing class (rtnl_class).
+template <typename Config>
+Try<Nothing> encode(
+ const Netlink<struct rtnl_class>& cls,
+ const Config& config);
+
+
+// Forward declaration. Each type of queueing class needs to implement
+// this function to decode its type specific configurations from the
+// libnl queueing class (rtnl_class). Returns None if the libnl
+// queueing class does not match the specific queueing class type.
+template <typename Config>
+Result<Config> decode(const Netlink<struct rtnl_class>& cls);
+
+
+// Encodes a queueing class (in our representation) to a libnl
+// queueing class (rtnl_class). We use templating here so that it works
+// for any type of queueing class.
+template <typename Config>
+Try<Netlink<struct rtnl_class>> encode(
+ const Netlink<struct rtnl_link>& link,
+ const Class<Config>& config)
+{
+ struct rtnl_class* c = rtnl_class_alloc();
Review Comment:
here is that alloc I'm referring to that needs a corresponding put, but it
looks like we make the caller deal with when to call the cleanup function
##########
src/linux/routing/queueing/internal.hpp:
##########
@@ -118,7 +120,69 @@ Try<Netlink<struct rtnl_qdisc>> encodeDiscipline(
}
/////////////////////////////////////////////////
-// Helpers for internal APIs.
+// Helpers for {en}decoding queueing classes.
+/////////////////////////////////////////////////
+
+namespace cls {
+// Forward declaration. Each type of queueing class needs to implement
+// this function to encode its type specific configurations into the
+// libnl queueing class (rtnl_class).
+template <typename Config>
+Try<Nothing> encode(
+ const Netlink<struct rtnl_class>& cls,
+ const Config& config);
+
+
+// Forward declaration. Each type of queueing class needs to implement
+// this function to decode its type specific configurations from the
+// libnl queueing class (rtnl_class). Returns None if the libnl
+// queueing class does not match the specific queueing class type.
+template <typename Config>
+Result<Config> decode(const Netlink<struct rtnl_class>& cls);
+
+
+// Encodes a queueing class (in our representation) to a libnl
+// queueing class (rtnl_class). We use templating here so that it works
+// for any type of queueing class.
+template <typename Config>
+Try<Netlink<struct rtnl_class>> encode(
+ const Netlink<struct rtnl_link>& link,
+ const Class<Config>& config)
+{
+ struct rtnl_class* c = rtnl_class_alloc();
+ if (c == nullptr) {
+ return Error("Failed to allocate a libnl class");
+ }
+
+ Netlink<struct rtnl_class> cls(c);
+
+ rtnl_tc_set_link(TC_CAST(cls.get()), link.get());
+ rtnl_tc_set_parent(TC_CAST(cls.get()), config.parent.get());
+
+ if (config.handle.isSome()) {
+ rtnl_tc_set_handle(TC_CAST(cls.get()), config.handle.get().get());
+ }
+
+ int error = rtnl_tc_set_kind(TC_CAST(cls.get()), config.kind.c_str());
+ if (error != 0) {
+ return Error(
+ "Failed to set the kind of the queueing class: " +
+ std::string(nl_geterror(error)));
+ }
+
+ // Perform queueuing class specific encoding.
+ Try<Nothing> encoding = encode(cls, config.config);
+ if (encoding.isError()) {
+ return Error("Failed to encode the queueing class: " + encoding.error());
+ }
Review Comment:
along the same line of thinking, aren't these error cases leaking the
memory..? we've done the rtnl_class_alloc() but we're erroring out here and no
one is going to free it?
##########
src/linux/routing/queueing/class.hpp:
##########
@@ -0,0 +1,61 @@
+// 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.
+
+#ifndef __LINUX_ROUTING_QUEUEING_CLASS_HPP__
+#define __LINUX_ROUTING_QUEUEING_CLASS_HPP__
+
+#include <string>
+
+#include <netlink/route/class.h>
+
+#include <stout/option.hpp>
+
+#include "linux/routing/handle.hpp"
+#include "linux/routing/queueing/internal.hpp"
+
+namespace routing {
+
+template <>
+inline void cleanup(struct rtnl_class* cls)
+{
+ rtnl_class_put(cls);
+}
Review Comment:
just a note, this just seems to be the way to free the struct after an
rtnl_class_alloc, it's a bit weird to see this function here without a
corresponding alloc function, but maybe those come out from a different place
I would have expected maybe something like the Class struct below to alloc /
free rtnl_class with a shared pointer around it to allow copying the Class
struct. We don't need to change this in this patch just making some
observations around the interface here.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
