From c3044a97dcd57f371c42199aa8a00e508e18dbbe Mon Sep 17 00:00:00 2001 From: Simon Marchi Date: Mon, 29 Apr 2019 12:12:36 -0400 Subject: [PATCH] bt2: update object model This patch updates the Babeltrace Python object model (what all Babeltrace Python objects inherit from) to match the current status of the API. We have both Shared and Unique objects. Both types have a _ptr member, a SWIG Python object wrapping a typed pointer (e.g. pointer to struct bt_value). A Shared object type is defined by inheriting from _SharedObject. It wraps and owns a reference to a Babeltrace object that is reference counted. The reference count is dropped when the Python object gets deleted. The _SharedObject base class defines two class methods, _get_ref and _put_ref, which must be implemented by subclasses. These implementations must respectively get and put a reference of the specialized pointer (_ptr) they wrap. A Unique object type is defined by inheriting from _UniqueObject. It is not refcounted. Rather, it is wholly owned by another object (the owner) that is itself refcounted. A Unique object, in the Babeltrace API, is destroyed when its parent is destroyed. There is therefore a risk that someone would keep a (Python) reference to a unique object that got destroyed, because its parent got destroyed, resulting in some use-after-free bug. To avoid this, we make it so that when a Python object representing a Babeltrace Unique object is created, that Python object obtains a reference on the owner Shared object. When the Unique Python object is destroyed, the reference on the Shared parent is dropped. The concepts of Freezable and Private objects don't exist anymore, so they are removed. I have also removed the _PrivateConnection class, since it won't be needed in the end and inherited from _PrivateObject. For other classes representing Babeltrace concepts, I have made them inherit the object type they'll need to inherit in the end (when all the Python bindings will be fixed), even though none of these classes is functional at the moment. Change-Id: I9ab743dd7a28407913945648c1d74c467550df14 Signed-off-by: Simon Marchi Reviewed-on: https://review.lttng.org/c/babeltrace/+/1021 Tested-by: jenkins Reviewed-by: Philippe Proulx --- bindings/python/bt2/bt2/__init__.py.in | 1 - bindings/python/bt2/bt2/clock_class.py | 2 +- bindings/python/bt2/bt2/clock_snapshot.py | 2 +- bindings/python/bt2/bt2/component.py | 8 +- bindings/python/bt2/bt2/connection.py | 24 +--- bindings/python/bt2/bt2/ctf_writer.py | 4 +- bindings/python/bt2/bt2/event.py | 2 +- bindings/python/bt2/bt2/event_class.py | 2 +- bindings/python/bt2/bt2/field.py | 2 +- bindings/python/bt2/bt2/field_class.py | 6 +- bindings/python/bt2/bt2/graph.py | 2 +- bindings/python/bt2/bt2/message.py | 2 +- bindings/python/bt2/bt2/message_iterator.py | 2 +- bindings/python/bt2/bt2/object.py | 116 ++++++++++++++------ bindings/python/bt2/bt2/packet.py | 2 +- bindings/python/bt2/bt2/plugin.py | 4 +- bindings/python/bt2/bt2/port.py | 4 +- bindings/python/bt2/bt2/query_executor.py | 2 +- bindings/python/bt2/bt2/stream.py | 2 +- bindings/python/bt2/bt2/stream_class.py | 2 +- bindings/python/bt2/bt2/trace.py | 2 +- bindings/python/bt2/bt2/value.py | 2 +- 22 files changed, 109 insertions(+), 86 deletions(-) diff --git a/bindings/python/bt2/bt2/__init__.py.in b/bindings/python/bt2/bt2/__init__.py.in index 6a4ead01..d064dc59 100644 --- a/bindings/python/bt2/bt2/__init__.py.in +++ b/bindings/python/bt2/bt2/__init__.py.in @@ -37,7 +37,6 @@ from bt2.component import _UserSinkComponent from bt2.component import _UserSourceComponent from bt2.connection import * from bt2.connection import _Connection -from bt2.connection import _PrivateConnection from bt2.ctf_writer import * from bt2.ctf_writer import _CtfWriterStream from bt2.event import _Event diff --git a/bindings/python/bt2/bt2/clock_class.py b/bindings/python/bt2/bt2/clock_class.py index 140d5293..41c9863f 100644 --- a/bindings/python/bt2/bt2/clock_class.py +++ b/bindings/python/bt2/bt2/clock_class.py @@ -53,7 +53,7 @@ class ClockClassOffset: return (self.seconds, self.cycles) == (other.seconds, other.cycles) -class ClockClass(object._Object): +class ClockClass(object._SharedObject): def __init__(self, name, frequency, description=None, precision=None, offset=None, is_absolute=None, uuid=None): utils._check_str(name) diff --git a/bindings/python/bt2/bt2/clock_snapshot.py b/bindings/python/bt2/bt2/clock_snapshot.py index d30c98e3..fdd66a89 100644 --- a/bindings/python/bt2/bt2/clock_snapshot.py +++ b/bindings/python/bt2/bt2/clock_snapshot.py @@ -31,7 +31,7 @@ def _create_clock_snapshot_from_ptr(ptr): return clock_snapshot -class _ClockSnapshot(object._Object): +class _ClockSnapshot(object._UniqueObject): def __init__(self, clock_class_ptr, cycles): utils._check_uint64(cycles) ptr = native_bt.clock_snapshot_create(clock_class_ptr, cycles) diff --git a/bindings/python/bt2/bt2/component.py b/bindings/python/bt2/bt2/component.py index a82f5267..82558ebd 100644 --- a/bindings/python/bt2/bt2/component.py +++ b/bindings/python/bt2/bt2/component.py @@ -39,7 +39,7 @@ _NO_PRINT_TRACEBACK = _env_var == '1' # have been created by Python code, but since we only have the pointer, # we can only wrap it in a generic way and lose the original Python # class. -class _GenericComponentClass(object._Object): +class _GenericComponentClass(object._SharedObject): @property def name(self): name = native_bt.component_class_get_name(self._ptr) @@ -200,7 +200,7 @@ class _SinkComponent(_Component): # This is analogous to _GenericSourceComponentClass, but for source # component objects. -class _GenericSourceComponent(object._Object, _SourceComponent): +class _GenericSourceComponent(object._SharedObject, _SourceComponent): @property def output_ports(self): return _ComponentPorts(False, self, @@ -211,7 +211,7 @@ class _GenericSourceComponent(object._Object, _SourceComponent): # This is analogous to _GenericFilterComponentClass, but for filter # component objects. -class _GenericFilterComponent(object._Object, _FilterComponent): +class _GenericFilterComponent(object._SharedObject, _FilterComponent): @property def output_ports(self): return _ComponentPorts(False, self, @@ -229,7 +229,7 @@ class _GenericFilterComponent(object._Object, _FilterComponent): # This is analogous to _GenericSinkComponentClass, but for sink # component objects. -class _GenericSinkComponent(object._Object, _SinkComponent): +class _GenericSinkComponent(object._SharedObject, _SinkComponent): @property def input_ports(self): return _ComponentPorts(False, self, diff --git a/bindings/python/bt2/bt2/connection.py b/bindings/python/bt2/bt2/connection.py index 90c63994..a15c6792 100644 --- a/bindings/python/bt2/bt2/connection.py +++ b/bindings/python/bt2/bt2/connection.py @@ -44,7 +44,7 @@ def _create_private_from_ptr(ptr): return obj -class _Connection(object._Object): +class _Connection(object._SharedObject): @staticmethod def _downstream_port(ptr): port_ptr = native_bt.connection_get_downstream_port(ptr) @@ -78,25 +78,3 @@ class _Connection(object._Object): return False return self.addr == other.addr - - -class _PrivateConnection(object._PrivateObject, _Connection): - def create_message_iterator(self, message_types=None): - msg_types = bt2.message._msg_types_from_msg_classes(message_types) - status, msg_iter_ptr = native_bt.py3_create_priv_conn_msg_iter(int(self._ptr), - msg_types) - _handle_status(status, 'cannot create message iterator object') - assert(msg_iter_ptr) - return bt2.message_iterator._PrivateConnectionMessageIterator._create_from_ptr(msg_iter_ptr) - - @property - def is_ended(self): - return self._is_ended(self._pub_ptr) - - @property - def downstream_port(self): - return self._downstream_port(self._pub_ptr) - - @property - def upstream_port(self): - return self._upstream_port(self._pub_ptr) diff --git a/bindings/python/bt2/bt2/ctf_writer.py b/bindings/python/bt2/bt2/ctf_writer.py index c021ebeb..af48f651 100644 --- a/bindings/python/bt2/bt2/ctf_writer.py +++ b/bindings/python/bt2/bt2/ctf_writer.py @@ -28,7 +28,7 @@ import abc import bt2 -class CtfWriterClock(object._Object): +class CtfWriterClock(bt2.object._SharedObject): def __init__(self, name, description=None, frequency=None, precision=None, offset=None, is_absolute=None, uuid=None): utils._check_str(name) @@ -284,7 +284,7 @@ class _CtfWriterStream(stream._StreamBase): return cpy -class CtfWriter(object._Object): +class CtfWriter(bt2.object._SharedObject): def __init__(self, path): utils._check_str(path) ptr = native_bt.ctf_writer_create(path) diff --git a/bindings/python/bt2/bt2/event.py b/bindings/python/bt2/bt2/event.py index 519c20d8..0a881182 100644 --- a/bindings/python/bt2/bt2/event.py +++ b/bindings/python/bt2/bt2/event.py @@ -89,7 +89,7 @@ class _EventClockSnapshots(collections.abc.Mapping): return _EventClockSnapshotsIterator(self) -class _Event(object._Object): +class _Event(object._UniqueObject): @property def event_class(self): return self._event_class diff --git a/bindings/python/bt2/bt2/event_class.py b/bindings/python/bt2/bt2/event_class.py index 5259f918..057dbea4 100644 --- a/bindings/python/bt2/bt2/event_class.py +++ b/bindings/python/bt2/bt2/event_class.py @@ -47,7 +47,7 @@ class EventClassLogLevel: DEBUG = native_bt.EVENT_CLASS_LOG_LEVEL_DEBUG -class EventClass(object._Object): +class EventClass(object._SharedObject): def __init__(self, name, id=None, log_level=None, emf_uri=None, context_field_class=None, payload_field_class=None): utils._check_str(name) diff --git a/bindings/python/bt2/bt2/field.py b/bindings/python/bt2/bt2/field.py index 77c5803d..f8acd021 100644 --- a/bindings/python/bt2/bt2/field.py +++ b/bindings/python/bt2/bt2/field.py @@ -50,7 +50,7 @@ def _create_from_ptr(ptr): return field -class _Field(object._Object, metaclass=abc.ABCMeta): +class _Field(object._UniqueObject, metaclass=abc.ABCMeta): def __copy__(self): ptr = native_bt.field_copy(self._ptr) utils._handle_ptr(ptr, 'cannot copy {} field object'.format(self._NAME.lower())) diff --git a/bindings/python/bt2/bt2/field_class.py b/bindings/python/bt2/bt2/field_class.py index cec7cafb..db36c373 100644 --- a/bindings/python/bt2/bt2/field_class.py +++ b/bindings/python/bt2/bt2/field_class.py @@ -32,7 +32,7 @@ def _create_from_ptr(ptr): return _TYPE_ID_TO_OBJ[typeid]._create_from_ptr(ptr) -class _FieldClass(object._Object, metaclass=abc.ABCMeta): +class _FieldClass(object._SharedObject, metaclass=abc.ABCMeta): def __init__(self, ptr): super().__init__(ptr) @@ -268,8 +268,8 @@ class _EnumerationFieldClassMapping: return (self.name, self.lower, self.upper) == (other.name, other.lower, other.upper) -class _EnumerationFieldClassMappingIterator(object._Object, - collections.abc.Iterator): +class _EnumerationFieldClassMappingIterator(object._SharedObject, + collections.abc.Iterator): def __init__(self, iter_ptr, is_signed): super().__init__(iter_ptr) self._is_signed = is_signed diff --git a/bindings/python/bt2/bt2/graph.py b/bindings/python/bt2/bt2/graph.py index ce861028..200f6021 100644 --- a/bindings/python/bt2/bt2/graph.py +++ b/bindings/python/bt2/bt2/graph.py @@ -86,7 +86,7 @@ def _graph_ports_disconnected_listener_from_native(user_listener, pass -class Graph(object._Object): +class Graph(object._SharedObject): def __init__(self): ptr = native_bt.graph_create() diff --git a/bindings/python/bt2/bt2/message.py b/bindings/python/bt2/bt2/message.py index 236ddd89..da2bf89d 100644 --- a/bindings/python/bt2/bt2/message.py +++ b/bindings/python/bt2/bt2/message.py @@ -53,7 +53,7 @@ def _msg_types_from_msg_classes(message_types): return msg_types -class _Message(object._Object): +class _Message(object._SharedObject): pass diff --git a/bindings/python/bt2/bt2/message_iterator.py b/bindings/python/bt2/bt2/message_iterator.py index 69b1c4f6..f2e4fad5 100644 --- a/bindings/python/bt2/bt2/message_iterator.py +++ b/bindings/python/bt2/bt2/message_iterator.py @@ -44,7 +44,7 @@ class _MessageIterator(collections.abc.Iterator): raise NotImplementedError -class _GenericMessageIterator(object._Object, _MessageIterator): +class _GenericMessageIterator(object._SharedObject, _MessageIterator): def _get_msg(self): msg_ptr = native_bt.message_iterator_get_message(self._ptr) utils._handle_ptr(msg_ptr, "cannot get message iterator object's current message object") diff --git a/bindings/python/bt2/bt2/object.py b/bindings/python/bt2/bt2/object.py index 774027cb..05c6deb7 100644 --- a/bindings/python/bt2/bt2/object.py +++ b/bindings/python/bt2/bt2/object.py @@ -20,11 +20,16 @@ # OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN # THE SOFTWARE. -from bt2 import native_bt -import abc +class _BaseObject: + # Ensure that the object always has _ptr set, even if it throws during + # construction. + + def __new__(cls, *args, **kwargs): + obj = super().__new__(cls) + obj._ptr = None + return obj -class _Object: def __init__(self, ptr): self._ptr = ptr @@ -32,50 +37,91 @@ class _Object: def addr(self): return int(self._ptr) + def __repr__(self): + return '<{}.{} object @ {}>'.format(self.__class__.__module__, + self.__class__.__name__, + hex(self.addr)) + + def __copy__(self): + raise NotImplementedError + + def __deepcopy__(self, memo): + raise NotImplementedError + + +# A Python object that is itself not refcounted, but is wholly owned by an +# object that is itself refcounted (a _SharedObject). A Babeltrace unique +# object gets destroyed once its owner gets destroyed (its refcount drops to +# 0). +# +# In the Python bindings, to avoid having to deal with issues with the lifetime +# of unique objects, we make it so acquiring a reference on a unique object +# acquires a reference on its owner. + +class _UniqueObject(_BaseObject): + + # Create a _UniqueObject. + # + # - ptr: SWIG Object, pointer to the unique object. + # - owner_ptr: SWIG Object, pointer to the owner of the unique + # object. A new reference is acquired. + # - owner_get_ref: Callback to get a reference on the owner + # - owner_put_ref: Callback to put a reference on the owner. + @classmethod - def _create_from_ptr(cls, ptr): + def _create_from_ptr_and_get_ref(cls, ptr, owner_ptr, + owner_get_ref, owner_put_ref): obj = cls.__new__(cls) obj._ptr = ptr + obj._owner_ptr = owner_ptr + obj._owner_put_ref = owner_put_ref + owner_get_ref(obj._owner_ptr) return obj - def _get(self): - native_bt.get(self._ptr) - def __del__(self): - ptr = getattr(self, '_ptr', None) - native_bt.put(ptr) - self._ptr = None + self._owner_put_ref(self._owner_ptr) - def __repr__(self): - return '<{}.{} object @ {}>'.format(self.__class__.__module__, - self.__class__.__name__, - hex(self.addr)) +# Python object that owns a reference to a Babeltrace object. +class _SharedObject(_BaseObject): -class _PrivateObject: - def __del__(self): - pub_ptr = getattr(self, '_pub_ptr', None) - native_bt.put(pub_ptr) - self._pub_ptr = None - super().__del__() + # Get a new reference on ptr. + # + # This must be implemented by subclasses to work correctly with a pointer + # of the native type they wrap. + @staticmethod + def _get_ref(ptr): + raise NotImplementedError -class _Freezable(metaclass=abc.ABCMeta): - @property - def is_frozen(self): - return self._is_frozen() + # Put a reference on ptr. + # + # This must be implemented by subclasses to work correctly with a pointer + # of the native type they wrap. - @property - def frozen(self): - return self.is_frozen + @staticmethod + def _put_ref(ptr): + raise NotImplementedError - def freeze(self): - self._freeze() + # Create a _SharedObject from an existing reference. + # + # This assumes that the caller owns a reference to the Babeltrace object + # and transfers this ownership to the newly created Python object. - @abc.abstractmethod - def _is_frozen(self): - pass + @classmethod + def _create_from_ptr(cls, ptr_owned): + obj = cls.__new__(cls) + obj._ptr = ptr_owned + return obj - @abc.abstractmethod - def _freeze(self): - pass + # Like _create_from_ptr, but acquire a new reference rather than + # stealing the caller's reference. + + @classmethod + def _create_from_ptr_and_get_ref(cls, ptr): + obj = cls._create_from_ptr(ptr) + cls._get_ref(obj._ptr) + return obj + + def __del__(self): + self._put_ref(self._ptr) diff --git a/bindings/python/bt2/bt2/packet.py b/bindings/python/bt2/bt2/packet.py index 6f85a988..19417726 100644 --- a/bindings/python/bt2/bt2/packet.py +++ b/bindings/python/bt2/bt2/packet.py @@ -28,7 +28,7 @@ import abc import bt2 -class _Packet(object._Object): +class _Packet(object._SharedObject): @property def stream(self): stream_ptr = native_bt.packet_get_stream(self._ptr) diff --git a/bindings/python/bt2/bt2/plugin.py b/bindings/python/bt2/bt2/plugin.py index a37e7631..a7ecf9b3 100644 --- a/bindings/python/bt2/bt2/plugin.py +++ b/bindings/python/bt2/bt2/plugin.py @@ -53,7 +53,7 @@ def find_plugin(name): return _Plugin._create_from_ptr(ptr) -class _PluginSet(object._Object, collections.abc.Sequence): +class _PluginSet(object._SharedObject, collections.abc.Sequence): def __len__(self): count = native_bt.plugin_set_get_plugin_count(self._ptr) assert(count >= 0) @@ -170,7 +170,7 @@ class _PluginComponentClasses(collections.abc.Mapping): return _PluginComponentClassesIterator(self) -class _Plugin(object._Object): +class _Plugin(object._SharedObject): @property def name(self): name = native_bt.plugin_get_name(self._ptr) diff --git a/bindings/python/bt2/bt2/port.py b/bindings/python/bt2/bt2/port.py index 68039a43..ebd27650 100644 --- a/bindings/python/bt2/bt2/port.py +++ b/bindings/python/bt2/bt2/port.py @@ -59,7 +59,7 @@ def _create_private_from_ptr(ptr): return obj -class _Port(object._Object): +class _Port(object._SharedObject): @staticmethod def _name(ptr): name = native_bt.port_get_name(ptr) @@ -131,7 +131,7 @@ class _OutputPort(_Port): return bt2.message_iterator._OutputPortMessageIterator._create_from_ptr(msg_iter_ptr) -class _PrivatePort(object._PrivateObject, _Port): +class _PrivatePort(_Port): @property def name(self): return self._name(self._pub_ptr) diff --git a/bindings/python/bt2/bt2/query_executor.py b/bindings/python/bt2/bt2/query_executor.py index 25d77c04..db7db610 100644 --- a/bindings/python/bt2/bt2/query_executor.py +++ b/bindings/python/bt2/bt2/query_executor.py @@ -25,7 +25,7 @@ import bt2.component import bt2 -class QueryExecutor(object._Object): +class QueryExecutor(object._SharedObject): def _handle_status(self, status, gen_error_msg): if status == native_bt.QUERY_STATUS_AGAIN: raise bt2.TryAgain diff --git a/bindings/python/bt2/bt2/stream.py b/bindings/python/bt2/bt2/stream.py index 3d313cd5..22ff0dfd 100644 --- a/bindings/python/bt2/bt2/stream.py +++ b/bindings/python/bt2/bt2/stream.py @@ -38,7 +38,7 @@ def _create_from_ptr(stream_ptr): return cls._create_from_ptr(stream_ptr) -class _StreamBase(object._Object): +class _StreamBase(object._SharedObject): @property def stream_class(self): stream_class_ptr = native_bt.stream_get_class(self._ptr) diff --git a/bindings/python/bt2/bt2/stream_class.py b/bindings/python/bt2/bt2/stream_class.py index 9c12a408..97cd593f 100644 --- a/bindings/python/bt2/bt2/stream_class.py +++ b/bindings/python/bt2/bt2/stream_class.py @@ -48,7 +48,7 @@ class _EventClassIterator(collections.abc.Iterator): return ev_id -class StreamClass(object._Object, collections.abc.Mapping): +class StreamClass(object._SharedObject, collections.abc.Mapping): def __init__(self, name=None, id=None, packet_context_field_class=None, event_header_field_class=None, event_context_field_class=None, event_classes=None): diff --git a/bindings/python/bt2/bt2/trace.py b/bindings/python/bt2/bt2/trace.py index 33fd8c08..2f3f24e7 100644 --- a/bindings/python/bt2/bt2/trace.py +++ b/bindings/python/bt2/bt2/trace.py @@ -160,7 +160,7 @@ class _TraceEnv(collections.abc.MutableMapping): return _TraceEnvIterator(self) -class Trace(object._Object, collections.abc.Mapping): +class Trace(object._SharedObject, collections.abc.Mapping): def __init__(self, name=None, native_byte_order=None, env=None, packet_header_field_class=None, clock_classes=None, stream_classes=None): diff --git a/bindings/python/bt2/bt2/value.py b/bindings/python/bt2/bt2/value.py index f36d2ea9..73366fdf 100644 --- a/bindings/python/bt2/bt2/value.py +++ b/bindings/python/bt2/bt2/value.py @@ -88,7 +88,7 @@ def create_value(value): raise TypeError("cannot create value object from '{}' object".format(value.__class__.__name__)) -class _Value(object._Object, object._Freezable, metaclass=abc.ABCMeta): +class _Value(object._SharedObject, metaclass=abc.ABCMeta): def __eq__(self, other): if other is None: # self is never the null value object -- 2.34.1