Commit 6d9030ac authored by Thomas Haller's avatar Thomas Haller Committed by Lubomir Rintel

device/trivial: add comment about lifetime of "kind" in tc_commit()

In general, all fields of public NMPlatform* structs must be
plain/simple. Meaning: copying the struct must be possible without
caring about cloning/duplicating memory.
In other words, if there are fields which lifetime is limited,
then these fields cannot be inside the public part NMPlatform*.

That is why

  - "NMPlatformLink.kind", "NMPlatformQdisc.kind", "NMPlatformTfilter.kind"
    are set by platform code to an interned string (g_intern_string())
    that has a static lifetime.

  - the "ingress_qos_map" field is inside the ref-counted struct NMPObjectLnkVlan
    and not NMPlatformLnkVlan. This field requires managing the lifetime
    of the array and NMPlatformLnkVlan cannot provide that.

See also for example NMPClass.cmd_obj_copy() which can deep-copy an object.
But this is only suitable for fields in NMPObject*. The purpose of this
rule is that you always can safely copy a NMPlatform* struct without
worrying about the ownership and lifetime of the fields (the field's
lifetime is unlimited).

This rule and managing of resource lifetime is the main reason for the
NMPlatform*/NMPObject* split. NMPlatform* structs simply have no mechanism
for copying/releasing fields, that is why the NMPObject* counterpart exists
(which is ref-counted and has a copy and destructor function).

This is violated in tc_commit() for the "kind" strings. The lifetime
of these strings is tied to the setting instance.

We cannot intern the strings (because these are arbitrary strings
and interned strings are leaked indefinitely). We also cannot g_strdup()
the strings, because NMPlatform* is not supposed to own strings.

So, just add comments that warn about this ugliness.

The more correct solution would be to move the "kind" fields inside
NMPObjectQdisc and NMPObjectTfilter, but that is a lot of extra effort.

(cherry picked from commit f2ae994b)
parent ea7de52d
Pipeline #36969 passed with stage
in 29 minutes and 24 seconds
...@@ -6516,7 +6516,12 @@ tc_commit (NMDevice *self) ...@@ -6516,7 +6516,12 @@ tc_commit (NMDevice *self)
NMPlatformQdisc *qdisc = NMP_OBJECT_CAST_QDISC (q); NMPlatformQdisc *qdisc = NMP_OBJECT_CAST_QDISC (q);
qdisc->ifindex = ip_ifindex; qdisc->ifindex = ip_ifindex;
/* Note: kind string is still owned by NMTCTfilter.
* This qdisc instance must not be kept alive beyond this function.
* nm_platform_qdisc_sync() promises to do that. */
qdisc->kind = nm_tc_qdisc_get_kind (s_qdisc); qdisc->kind = nm_tc_qdisc_get_kind (s_qdisc);
qdisc->addr_family = AF_UNSPEC; qdisc->addr_family = AF_UNSPEC;
qdisc->handle = nm_tc_qdisc_get_handle (s_qdisc); qdisc->handle = nm_tc_qdisc_get_handle (s_qdisc);
qdisc->parent = nm_tc_qdisc_get_parent (s_qdisc); qdisc->parent = nm_tc_qdisc_get_parent (s_qdisc);
...@@ -6558,7 +6563,12 @@ tc_commit (NMDevice *self) ...@@ -6558,7 +6563,12 @@ tc_commit (NMDevice *self)
NMPlatformTfilter *tfilter = NMP_OBJECT_CAST_TFILTER (q); NMPlatformTfilter *tfilter = NMP_OBJECT_CAST_TFILTER (q);
tfilter->ifindex = ip_ifindex; tfilter->ifindex = ip_ifindex;
/* Note: kind string is still owned by NMTCTfilter.
* This tfilter instance must not be kept alive beyond this function.
* nm_platform_tfilter_sync() promises to do that. */
tfilter->kind = nm_tc_tfilter_get_kind (s_tfilter); tfilter->kind = nm_tc_tfilter_get_kind (s_tfilter);
tfilter->addr_family = AF_UNSPEC; tfilter->addr_family = AF_UNSPEC;
tfilter->handle = nm_tc_tfilter_get_handle (s_tfilter); tfilter->handle = nm_tc_tfilter_get_handle (s_tfilter);
tfilter->parent = nm_tc_tfilter_get_parent (s_tfilter); tfilter->parent = nm_tc_tfilter_get_parent (s_tfilter);
...@@ -6568,7 +6578,11 @@ tc_commit (NMDevice *self) ...@@ -6568,7 +6578,11 @@ tc_commit (NMDevice *self)
if (action) { if (action) {
GVariant *var; GVariant *var;
/* Note: kind string is still owned by NMTCAction.
* This tfilter instance must not be kept alive beyond this function.
* nm_platform_tfilter_sync() promises to do that. */
tfilter->action.kind = nm_tc_action_get_kind (action); tfilter->action.kind = nm_tc_action_get_kind (action);
if (strcmp (tfilter->action.kind, "simple") == 0) { if (strcmp (tfilter->action.kind, "simple") == 0) {
var = nm_tc_action_get_attribute (action, "sdata"); var = nm_tc_action_get_attribute (action, "sdata");
if (var && g_variant_is_of_type (var, G_VARIANT_TYPE_BYTESTRING)) { if (var && g_variant_is_of_type (var, G_VARIANT_TYPE_BYTESTRING)) {
......
...@@ -8244,6 +8244,9 @@ qdisc_add (NMPlatform *platform, ...@@ -8244,6 +8244,9 @@ qdisc_add (NMPlatform *platform,
char s_buf[256]; char s_buf[256];
nm_auto_nlmsg struct nl_msg *msg = NULL; nm_auto_nlmsg struct nl_msg *msg = NULL;
/* Note: @qdisc must not be copied or kept alive because the lifetime of qdisc.kind
* is undefined. */
msg = _nl_msg_new_qdisc (RTM_NEWQDISC, flags, qdisc); msg = _nl_msg_new_qdisc (RTM_NEWQDISC, flags, qdisc);
event_handler_read_netlink (platform, FALSE); event_handler_read_netlink (platform, FALSE);
...@@ -8285,6 +8288,9 @@ tfilter_add (NMPlatform *platform, ...@@ -8285,6 +8288,9 @@ tfilter_add (NMPlatform *platform,
char s_buf[256]; char s_buf[256];
nm_auto_nlmsg struct nl_msg *msg = NULL; nm_auto_nlmsg struct nl_msg *msg = NULL;
/* Note: @tfilter must not be copied or kept alive because the lifetime of tfilter.kind
* and tfilter.action.kind is undefined. */
msg = _nl_msg_new_tfilter (RTM_NEWTFILTER, flags, tfilter); msg = _nl_msg_new_tfilter (RTM_NEWTFILTER, flags, tfilter);
event_handler_read_netlink (platform, FALSE); event_handler_read_netlink (platform, FALSE);
......
...@@ -5077,10 +5077,27 @@ nm_platform_qdisc_add (NMPlatform *self, ...@@ -5077,10 +5077,27 @@ nm_platform_qdisc_add (NMPlatform *self,
int ifindex = qdisc->ifindex; int ifindex = qdisc->ifindex;
_CHECK_SELF (self, klass, -NME_BUG); _CHECK_SELF (self, klass, -NME_BUG);
/* Note: @qdisc must not be copied or kept alive because the lifetime of qdisc.kind
* is undefined. */
_LOG3D ("adding or updating a qdisc: %s", nm_platform_qdisc_to_string (qdisc, NULL, 0)); _LOG3D ("adding or updating a qdisc: %s", nm_platform_qdisc_to_string (qdisc, NULL, 0));
return klass->qdisc_add (self, flags, qdisc); return klass->qdisc_add (self, flags, qdisc);
} }
/**
* nm_platform_qdisc_sync:
* @self: the #NMPlatform instance
* @ifindex: the ifindex where to configure the qdiscs.
* @known_qdiscs: the list of qdiscs (#NMPObject).
*
* The function promises not to take any reference to the qdisc
* instances from @known_qdiscs, nor to keep them around after
* the function returns. This is important, because it allows the
* caller to pass NMPlatformQdisc instances which "kind" string
* have a limited lifetime.
*
* Returns: %TRUE on success.
*/
gboolean gboolean
nm_platform_qdisc_sync (NMPlatform *self, nm_platform_qdisc_sync (NMPlatform *self,
int ifindex, int ifindex,
...@@ -5143,10 +5160,27 @@ nm_platform_tfilter_add (NMPlatform *self, ...@@ -5143,10 +5160,27 @@ nm_platform_tfilter_add (NMPlatform *self,
int ifindex = tfilter->ifindex; int ifindex = tfilter->ifindex;
_CHECK_SELF (self, klass, -NME_BUG); _CHECK_SELF (self, klass, -NME_BUG);
/* Note: @tfilter must not be copied or kept alive because the lifetime of tfilter.kind
* and tfilter.action.kind is undefined. */
_LOG3D ("adding or updating a tfilter: %s", nm_platform_tfilter_to_string (tfilter, NULL, 0)); _LOG3D ("adding or updating a tfilter: %s", nm_platform_tfilter_to_string (tfilter, NULL, 0));
return klass->tfilter_add (self, flags, tfilter); return klass->tfilter_add (self, flags, tfilter);
} }
/**
* nm_platform_qdisc_sync:
* @self: the #NMPlatform instance
* @ifindex: the ifindex where to configure the qdiscs.
* @known_tfilters: the list of tfilters (#NMPObject).
*
* The function promises not to take any reference to the tfilter
* instances from @known_tfilters, nor to keep them around after
* the function returns. This is important, because it allows the
* caller to pass NMPlatformTfilter instances which "kind" string
* have a limited lifetime.
*
* Returns: %TRUE on success.
*/
gboolean gboolean
nm_platform_tfilter_sync (NMPlatform *self, nm_platform_tfilter_sync (NMPlatform *self,
int ifindex, int ifindex,
......
...@@ -626,7 +626,11 @@ typedef struct { ...@@ -626,7 +626,11 @@ typedef struct {
typedef struct { typedef struct {
__NMPlatformObjWithIfindex_COMMON; __NMPlatformObjWithIfindex_COMMON;
/* beware, kind is embedded in an NMPObject, hence you must
* take care of the lifetime of the string. */
const char *kind; const char *kind;
int addr_family; int addr_family;
guint32 handle; guint32 handle;
guint32 parent; guint32 parent;
...@@ -649,7 +653,11 @@ typedef struct { ...@@ -649,7 +653,11 @@ typedef struct {
} NMPlatformActionMirred; } NMPlatformActionMirred;
typedef struct { typedef struct {
/* beware, kind is embedded in an NMPObject, hence you must
* take care of the lifetime of the string. */
const char *kind; const char *kind;
union { union {
NMPlatformActionSimple simple; NMPlatformActionSimple simple;
NMPlatformActionMirred mirred; NMPlatformActionMirred mirred;
...@@ -661,7 +669,11 @@ typedef struct { ...@@ -661,7 +669,11 @@ typedef struct {
typedef struct { typedef struct {
__NMPlatformObjWithIfindex_COMMON; __NMPlatformObjWithIfindex_COMMON;
/* beware, kind is embedded in an NMPObject, hence you must
* take care of the lifetime of the string. */
const char *kind; const char *kind;
int addr_family; int addr_family;
guint32 handle; guint32 handle;
guint32 parent; guint32 parent;
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment