[{"id":28586,"web_url":"https://patchwork.libcamera.org/comment/28586/","msgid":"<871qa8b6qh.fsf@redhat.com>","date":"2024-01-23T11:59:34","subject":"Re: [PATCH v2 12/12] libcamera: object: Add and use thread-bound\n\tassertion","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Laurent Pinchart writes:\n\n> Several functions in libcamera classes are marked as thread-bound,\n> restricting the contexts in which those functions can be called. There\n> is no infrastructure to enforce these restrictions, causing difficult to\n> debug race conditions when they are not met by callers.\n>\n> As a first step to solve this, add an assertThreadBound() protected\n> function to the Object class to test if the calling thread context is\n> valid, and use it in member functions of Object subclasses marked as\n> thread-bound. This replaces manual tests in a few locations.\n>\n> The thread-bound member functions of classes that do not inherit from\n> Object are not checked, and neither are the functions of classes marked\n> as thread-bound at the class level. These issue should be addressed in\n> the future.\n>\n> Signed-off-by: Laurent Pinchart \n\nReviewed-by: Milan Zamazal \n\n> ---\n> Changes since v1:\n>\n> - Fix typo in commit message\n> ---\n> include/libcamera/base/object.h | 2 ++\n> src/libcamera/base/event_notifier.cpp | 6 +++++\n> src/libcamera/base/object.cpp | 32 ++++++++++++++++++++++++++-\n> src/libcamera/base/timer.cpp | 10 +++------\n> 4 files changed, 42 insertions(+), 8 deletions(-)\n>\n> diff --git a/include/libcamera/base/object.h b/include/libcamera/base/object.h\n> index 933336361155..cb7e0a132be2 100644\n> --- a/include/libcamera/base/object.h\n> +++ b/include/libcamera/base/object.h\n> @@ -49,6 +49,8 @@ public:\n> protected:\n> \tvirtual void message(Message *msg);\n> \n> +\tbool assertThreadBound(const char *message);\n> +\n> private:\n> \tfriend class SignalBase;\n> \tfriend class Thread;\n> diff --git a/src/libcamera/base/event_notifier.cpp b/src/libcamera/base/event_notifier.cpp\n> index fd93c0878c6f..a519aec38efb 100644\n> --- a/src/libcamera/base/event_notifier.cpp\n> +++ b/src/libcamera/base/event_notifier.cpp\n> @@ -8,6 +8,7 @@\n> #include \n> \n> #include \n> +#include \n> #include \n> #include \n> \n> @@ -20,6 +21,8 @@\n> \n> namespace libcamera {\n> \n> +LOG_DECLARE_CATEGORY(Event)\n> +\n> /**\n> * \\class EventNotifier\n> * \\brief Notify of activity on a file descriptor\n> @@ -104,6 +107,9 @@ EventNotifier::~EventNotifier()\n> */\n> void EventNotifier::setEnabled(bool enable)\n> {\n> +\tif (!assertThreadBound(\"EventNotifier can't be enabled from another thread\"))\n> +\t\treturn;\n> +\n> \tif (enabled_ == enable)\n> \t\treturn;\n> \n> diff --git a/src/libcamera/base/object.cpp b/src/libcamera/base/object.cpp\n> index c6040fc6a78b..81054b5833a3 100644\n> --- a/src/libcamera/base/object.cpp\n> +++ b/src/libcamera/base/object.cpp\n> @@ -225,6 +225,35 @@ void Object::message(Message *msg)\n> \t}\n> }\n> \n> +/**\n> + * \\fn Object::assertThreadBound()\n> + * \\brief Check if the caller complies with thread-bound constraints\n> + * \\param[in] message The message to be printed on error\n> + *\n> + * This function verifies the calling constraints required by the \\threadbound\n> + * definition. It shall be called at the beginning of member functions of an\n> + * Object subclass that are explicitly marked as thread-bound in their\n> + * documentation.\n> + *\n> + * If the thread-bound constraints are not met, the function prints \\a message\n> + * as an error message. For debug builds, it additionally causes an assertion\n> + * error.\n> + *\n> + * \\todo Verify the thread-bound requirements for functions marked as\n> + * thread-bound at the class level.\n> + *\n> + * \\return True if the call is thread-bound compliant, false otherwise\n> + */\n> +bool Object::assertThreadBound(const char *message)\n> +{\n> +\tif (Thread::current() == thread_)\n> +\t\treturn true;\n> +\n> +\tLOG(Object, Error) << message;\n> +\tASSERT(false);\n> +\treturn false;\n> +}\n> +\n> /**\n> * \\fn R Object::invokeMethod()\n> * \\brief Invoke a method asynchronously on an Object instance\n> @@ -276,7 +305,8 @@ void Object::message(Message *msg)\n> */\n> void Object::moveToThread(Thread *thread)\n> {\n> -\tASSERT(Thread::current() == thread_);\n> +\tif (!assertThreadBound(\"Object can't be moved from another thread\"))\n> +\t\treturn;\n> \n> \tif (thread_ == thread)\n> \t\treturn;\n> diff --git a/src/libcamera/base/timer.cpp b/src/libcamera/base/timer.cpp\n> index 74b060af32ff..24dbf1e892c3 100644\n> --- a/src/libcamera/base/timer.cpp\n> +++ b/src/libcamera/base/timer.cpp\n> @@ -85,10 +85,8 @@ void Timer::start(std::chrono::milliseconds duration)\n> */\n> void Timer::start(std::chrono::steady_clock::time_point deadline)\n> {\n> -\tif (Thread::current() != thread()) {\n> -\t\tLOG(Timer, Error) << \"Timer \" << this << \" << can't be started from another thread\";\n> +\tif (!assertThreadBound(\"Timer can't be started from another thread\"))\n> \t\treturn;\n> -\t}\n> \n> \tdeadline_ = deadline;\n> \n> @@ -114,13 +112,11 @@ void Timer::start(std::chrono::steady_clock::time_point deadline)\n> */\n> void Timer::stop()\n> {\n> -\tif (!isRunning())\n> +\tif (!assertThreadBound(\"Timer can't be stopped from another thread\"))\n> \t\treturn;\n> \n> -\tif (Thread::current() != thread()) {\n> -\t\tLOG(Timer, Error) << \"Timer \" << this << \" can't be stopped from another thread\";\n> +\tif (!isRunning())\n> \t\treturn;\n> -\t}\n> \n> \tunregisterTimer();\n> }","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 8CA58BDC71\n\tfor ;\n\tTue, 23 Jan 2024 11:59:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 397CF62916;\n\tTue, 23 Jan 2024 12:59:43 +0100 (CET)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C2CA0628E9\n\tfor ;\n\tTue, 23 Jan 2024 12:59:40 +0100 (CET)","from mail-wr1-f71.google.com (mail-wr1-f71.google.com\n\t[209.85.221.71]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-356-tUDRK7AyOTGaTqQFYqL3lg-1; Tue, 23 Jan 2024 06:59:38 -0500","by mail-wr1-f71.google.com with SMTP id\n\tffacd0b85a97d-337bf78ef28so1787795f8f.3\n\tfor ;\n\tTue, 23 Jan 2024 03:59:37 -0800 (PST)","from nuthatch (nat-pool-brq-t.redhat.com. [213.175.37.10])\n\tby smtp.gmail.com with ESMTPSA id\n\tl10-20020adfe58a000000b003394495566dsm1348903wrm.22.2024.01.23.03.59.34\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 23 Jan 2024 03:59:34 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"OpoHUOtS\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1706011179;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=tB4fef9mKlyK3zterh5JFrpiHs9X300hemBqQ3LTmT8=;\n\tb=OpoHUOtSp7P0ltbvtJd7S3+MDWbH5SIv+SF438XM/w4h4pIDSBZThpdKGaILMsv5CPWyWo\n\tMFr48ufb5Zop4MzHrHvFreg2IRzXbZK35p56XOrqeBFH9ata4HtJz7aR/NNTXO3fLT2IeK\n\tldR5r9Kk2QNm01KPNqJgEBaTreNLHNc=","X-MC-Unique":"tUDRK7AyOTGaTqQFYqL3lg-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1706011175; x=1706615975;\n\th=mime-version:user-agent:message-id:date:references:in-reply-to\n\t:subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=tB4fef9mKlyK3zterh5JFrpiHs9X300hemBqQ3LTmT8=;\n\tb=XY+Wwov3QJqb3fxKkSj/ET2u+ZxZVgIuie0+T1vWaTYdb4pih6hq6DWqCq6q+z/bSv\n\tiukE4zBw8C+/VY2Y/qyHSxfHbh2hrStNQiYsGNKxPtZEvDIyatOGYbCI0T+BY3C45VrJ\n\t7hTsF7XDsMKViBGz4JdojOJkMsYNm1C1SUjlMZiax+AJlW2RrZQH0CGQM1y6W0M4XiSq\n\tfi/S2AMrMcIrsv6aw8Htn7rwaAxT31Am4AIUrWaJz10zerewEqP0et0ED21Pj13Qd+od\n\tTNeoR2iFbxgmsA7Eo6kz0SkKRnmaRzTjEhGDKl5+3qiLlSn3YojrJ+2zgEeaNfwM653e\n\tzByg==","X-Gm-Message-State":"AOJu0YzswWOnHC75YXd42K0ENyOJJELlNf147JYBOYqXwar8HeFaXa1N\n\tzhNfaf13lQ5ZiyDbSmqa5YTY5ua1eaE95XNPrbKa4bQY6PJjxrhGdqAl5l96JJh4eKrghRaw1Gt\n\tld/qzJSNNv9Tra9QIYMzMVgpijo7EuXBT78NbUWaC2iHmlyaIrJIyAXmt2VGlLq6m7Xnm65aj5e\n\t/Xb9ljro0xpmZ+v/faRHDV/pzECAlRMA8geDfQ18qjub8TO/Fo//mFkQw=","X-Received":["by 2002:a5d:5086:0:b0:337:c977:29eb with SMTP id\n\ta6-20020a5d5086000000b00337c97729ebmr2906255wrt.84.1706011175566; \n\tTue, 23 Jan 2024 03:59:35 -0800 (PST)","by 2002:a5d:5086:0:b0:337:c977:29eb with SMTP id\n\ta6-20020a5d5086000000b00337c97729ebmr2906247wrt.84.1706011175161; \n\tTue, 23 Jan 2024 03:59:35 -0800 (PST)"],"X-Google-Smtp-Source":"AGHT+IFzmmiXOflSPZrzpru9byRiJWUKAYwN8Uym/V+Y8phEkArcAQm+iUbwhJstof9E9+solVqMBQ==","From":"Milan Zamazal ","To":"Laurent Pinchart ","Subject":"Re: [PATCH v2 12/12] libcamera: object: Add and use thread-bound\n\tassertion","In-Reply-To":"<20240123011249.22716-13-laurent.pinchart@ideasonboard.com>\n\t(Laurent Pinchart's message of \"Tue, 23 Jan 2024 03:12:49 +0200\")","References":"<20240123011249.22716-1-laurent.pinchart@ideasonboard.com>\n\t<20240123011249.22716-13-laurent.pinchart@ideasonboard.com>","Date":"Tue, 23 Jan 2024 12:59:34 +0100","Message-ID":"<871qa8b6qh.fsf@redhat.com>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]