[{"id":21971,"web_url":"https://patchwork.libcamera.org/comment/21971/","msgid":"<20220107150132.usvammci5fqfyux5@uno.localdomain>","date":"2022-01-07T15:01:32","subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: base: signal: Prevent\n\texcessive slot usage","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Kieran,\n\nOn Fri, Jan 07, 2022 at 12:55:06PM +0000, Kieran Bingham wrote:\n> Slots are not expected to be connected to lots of signals. Our normal\n\nIsn't it the other way around ?\n\"Signals are not expected to be connected to a lots of slots\" :)\n\n> use case is a one to one mapping with a signal expected to call one\n> slot.\n>\n> While we support multiple slots, excessive use is likely the result of a\n> bug indicating a failure to disconnect slots or repeated connection of a\n> slot which can lead to memory leaks and poor performance when deleting\n> the Object.\n>\n> Assert that a maximum of 8 slots are connected which will catch any bug\n> and still allow some multiple slot usage. If this limit is ever met it\n> can be extended and considered at that time.\n\nI wish we had WARN_ONCE().\n\n>\n> Signed-off-by: Kieran Bingham \n> ---\n>\n> Note that this patch causes the CTS tests to fail as they hit this\n> assertion.\n>\n> It is not yet investigated if we really need more slots or if the\n> assertion has already caught a slot-leak, but posting for early review\n> anyway.\n>\n> src/libcamera/base/signal.cpp | 8 ++++++++\n> 1 file changed, 8 insertions(+)\n>\n> diff --git a/src/libcamera/base/signal.cpp b/src/libcamera/base/signal.cpp\n> index 9df45d079a42..7e6a23cc909d 100644\n> --- a/src/libcamera/base/signal.cpp\n> +++ b/src/libcamera/base/signal.cpp\n> @@ -7,6 +7,7 @@\n>\n> #include \n>\n> +#include \n> #include \n>\n> /**\n> @@ -35,6 +36,13 @@ void SignalBase::connect(BoundMethodBase *slot)\n> \tif (object)\n> \t\tobject->connect(this);\n> \tslots_.push_back(slot);\n> +\n> +\t/*\n> +\t * Slots are not expected to be used to connect many items. If we exceed\n> +\t * a reasonable amount, presume that there is a bug and fail fast on\n> +\t * debug builds.\n> +\t */\n> +\tASSERT(slots_.size() < 8);\n\nThat's indeed a useful safety check when developing. I think it's\nworth having it in.\n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> }\n>\n> void SignalBase::disconnect(Object *object)\n> --\n> 2.32.0\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 4E128BE080\n\tfor ;\n\tFri, 7 Jan 2022 15:00:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B916A608E6;\n\tFri, 7 Jan 2022 16:00:35 +0100 (CET)","from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::228])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 921CB60868\n\tfor ;\n\tFri, 7 Jan 2022 16:00:33 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 1088A1BF213;\n\tFri, 7 Jan 2022 15:00:32 +0000 (UTC)"],"Date":"Fri, 7 Jan 2022 16:01:32 +0100","From":"Jacopo Mondi ","To":"Kieran Bingham ","Message-ID":"<20220107150132.usvammci5fqfyux5@uno.localdomain>","References":"<20220107125506.1477795-1-kieran.bingham@ideasonboard.com>\n\t<20220107125506.1477795-3-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220107125506.1477795-3-kieran.bingham@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: base: signal: Prevent\n\texcessive slot usage","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21973,"web_url":"https://patchwork.libcamera.org/comment/21973/","msgid":"<164156828395.1224575.14685464986290943153@Monstersaurus>","date":"2022-01-07T15:11:23","subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: base: signal: Prevent\n\texcessive slot usage","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Jacopo Mondi (2022-01-07 15:01:32)\n> Hi Kieran,\n> \n> On Fri, Jan 07, 2022 at 12:55:06PM +0000, Kieran Bingham wrote:\n> > Slots are not expected to be connected to lots of signals. Our normal\n> \n> Isn't it the other way around ?\n> \"Signals are not expected to be connected to a lots of slots\" :)\n\nI ... er ... um ... probably? maybe ;-)\n\n> \n> > use case is a one to one mapping with a signal expected to call one\n> > slot.\n> >\n> > While we support multiple slots, excessive use is likely the result of a\n> > bug indicating a failure to disconnect slots or repeated connection of a\n> > slot which can lead to memory leaks and poor performance when deleting\n> > the Object.\n> >\n> > Assert that a maximum of 8 slots are connected which will catch any bug\n> > and still allow some multiple slot usage. If this limit is ever met it\n> > can be extended and considered at that time.\n> \n> I wish we had WARN_ONCE().\n\nWe could add that, but in this instance, I really want the assertion to\nbreak things, otherwise for instance in CTS it woudl just keep running -\nand woudl require someone to manually go through the logs.\n\nMaybe it would be noisy enough to get noticed ... but maybe not...\n\n\n> \n> >\n> > Signed-off-by: Kieran Bingham \n> > ---\n> >\n> > Note that this patch causes the CTS tests to fail as they hit this\n> > assertion.\n> >\n> > It is not yet investigated if we really need more slots or if the\n> > assertion has already caught a slot-leak, but posting for early review\n> > anyway.\n\nOf course the fact that it breaks CTS currently means this patch can't\ngo in until it's resolved anyway.\n\n\n\n> >\n> > src/libcamera/base/signal.cpp | 8 ++++++++\n> > 1 file changed, 8 insertions(+)\n> >\n> > diff --git a/src/libcamera/base/signal.cpp b/src/libcamera/base/signal.cpp\n> > index 9df45d079a42..7e6a23cc909d 100644\n> > --- a/src/libcamera/base/signal.cpp\n> > +++ b/src/libcamera/base/signal.cpp\n> > @@ -7,6 +7,7 @@\n> >\n> > #include \n> >\n> > +#include \n> > #include \n> >\n> > /**\n> > @@ -35,6 +36,13 @@ void SignalBase::connect(BoundMethodBase *slot)\n> > if (object)\n> > object->connect(this);\n> > slots_.push_back(slot);\n> > +\n> > + /*\n> > + * Slots are not expected to be used to connect many items. If we exceed\n> > + * a reasonable amount, presume that there is a bug and fail fast on\n> > + * debug builds.\n> > + */\n> > + ASSERT(slots_.size() < 8);\n> \n> That's indeed a useful safety check when developing. I think it's\n> worth having it in.\n\nThanks, I agree of course (once we find/fix the associated CTS bug)\n\n> \n> Reviewed-by: Jacopo Mondi \n> \n> Thanks\n> j\n> \n> > }\n> >\n> > void SignalBase::disconnect(Object *object)\n> > --\n> > 2.32.0\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 2DAE3BE080\n\tfor ;\n\tFri, 7 Jan 2022 15:11:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5490160921;\n\tFri, 7 Jan 2022 16:11:28 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9861B60868\n\tfor ;\n\tFri, 7 Jan 2022 16:11:26 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2824F8D7;\n\tFri, 7 Jan 2022 16:11:26 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"C6dBbazf\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1641568286;\n\tbh=y9bAYlAVBqEW9w1gg691RgdwyxH3At7N35i/vqpMstQ=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=C6dBbazfB8zlkhKd1+47H7ezempTntdQT2KmlWrLqkrVpKMjuRJ94unA5k4DiMubJ\n\teJGjVVE77nlVpj4mQ7S7eZJjGIecZ82s0CWUdYrRRnkAHpOc+vDzrGvaO0pfeAGW9h\n\tbCXX7omuUMOJKKMQjpdKcWg2byqj4BNe6KVRjz2s=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220107150132.usvammci5fqfyux5@uno.localdomain>","References":"<20220107125506.1477795-1-kieran.bingham@ideasonboard.com>\n\t<20220107125506.1477795-3-kieran.bingham@ideasonboard.com>\n\t<20220107150132.usvammci5fqfyux5@uno.localdomain>","From":"Kieran Bingham ","To":"Jacopo Mondi ","Date":"Fri, 07 Jan 2022 15:11:23 +0000","Message-ID":"<164156828395.1224575.14685464986290943153@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: base: signal: Prevent\n\texcessive slot usage","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21975,"web_url":"https://patchwork.libcamera.org/comment/21975/","msgid":"","date":"2022-01-07T15:31:13","subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: base: signal: Prevent\n\texcessive slot usage","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Kieran\n\nAlso for this one:\n\nTested-by: David Plowman \n\nThanks!\nDavid\n\nOn Fri, 7 Jan 2022 at 15:11, Kieran Bingham\n wrote:\n>\n> Quoting Jacopo Mondi (2022-01-07 15:01:32)\n> > Hi Kieran,\n> >\n> > On Fri, Jan 07, 2022 at 12:55:06PM +0000, Kieran Bingham wrote:\n> > > Slots are not expected to be connected to lots of signals. Our normal\n> >\n> > Isn't it the other way around ?\n> > \"Signals are not expected to be connected to a lots of slots\" :)\n>\n> I ... er ... um ... probably? maybe ;-)\n>\n> >\n> > > use case is a one to one mapping with a signal expected to call one\n> > > slot.\n> > >\n> > > While we support multiple slots, excessive use is likely the result of a\n> > > bug indicating a failure to disconnect slots or repeated connection of a\n> > > slot which can lead to memory leaks and poor performance when deleting\n> > > the Object.\n> > >\n> > > Assert that a maximum of 8 slots are connected which will catch any bug\n> > > and still allow some multiple slot usage. If this limit is ever met it\n> > > can be extended and considered at that time.\n> >\n> > I wish we had WARN_ONCE().\n>\n> We could add that, but in this instance, I really want the assertion to\n> break things, otherwise for instance in CTS it woudl just keep running -\n> and woudl require someone to manually go through the logs.\n>\n> Maybe it would be noisy enough to get noticed ... but maybe not...\n>\n>\n> >\n> > >\n> > > Signed-off-by: Kieran Bingham \n> > > ---\n> > >\n> > > Note that this patch causes the CTS tests to fail as they hit this\n> > > assertion.\n> > >\n> > > It is not yet investigated if we really need more slots or if the\n> > > assertion has already caught a slot-leak, but posting for early review\n> > > anyway.\n>\n> Of course the fact that it breaks CTS currently means this patch can't\n> go in until it's resolved anyway.\n>\n>\n>\n> > >\n> > > src/libcamera/base/signal.cpp | 8 ++++++++\n> > > 1 file changed, 8 insertions(+)\n> > >\n> > > diff --git a/src/libcamera/base/signal.cpp b/src/libcamera/base/signal.cpp\n> > > index 9df45d079a42..7e6a23cc909d 100644\n> > > --- a/src/libcamera/base/signal.cpp\n> > > +++ b/src/libcamera/base/signal.cpp\n> > > @@ -7,6 +7,7 @@\n> > >\n> > > #include \n> > >\n> > > +#include \n> > > #include \n> > >\n> > > /**\n> > > @@ -35,6 +36,13 @@ void SignalBase::connect(BoundMethodBase *slot)\n> > > if (object)\n> > > object->connect(this);\n> > > slots_.push_back(slot);\n> > > +\n> > > + /*\n> > > + * Slots are not expected to be used to connect many items. If we exceed\n> > > + * a reasonable amount, presume that there is a bug and fail fast on\n> > > + * debug builds.\n> > > + */\n> > > + ASSERT(slots_.size() < 8);\n> >\n> > That's indeed a useful safety check when developing. I think it's\n> > worth having it in.\n>\n> Thanks, I agree of course (once we find/fix the associated CTS bug)\n>\n> >\n> > Reviewed-by: Jacopo Mondi \n> >\n> > Thanks\n> > j\n> >\n> > > }\n> > >\n> > > void SignalBase::disconnect(Object *object)\n> > > --\n> > > 2.32.0\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 BA79FBF415\n\tfor ;\n\tFri, 7 Jan 2022 15:31:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 71AB860921;\n\tFri, 7 Jan 2022 16:31:26 +0100 (CET)","from mail-wr1-x432.google.com (mail-wr1-x432.google.com\n\t[IPv6:2a00:1450:4864:20::432])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EFA3460868\n\tfor ;\n\tFri, 7 Jan 2022 16:31:24 +0100 (CET)","by mail-wr1-x432.google.com with SMTP id r10so4066395wrc.3\n\tfor ;\n\tFri, 07 Jan 2022 07:31:24 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"TtNbhoE5\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=cr7TsNrsXVYnUxum2cTe/u8nlJMtPOGrul+Co4AWH44=;\n\tb=TtNbhoE5atdrH51btOdzcYPII/2arcdr+JvF5NflKF9SGz+xMNP5ZDq0PHCxUS7JbF\n\tyq01ReqQQLWgQR+ydejKcm1dtXolF7n7z458Rt1r7zSoEXek/+Cf8z1BCAmB7/fcMdtM\n\tvoHhdCxCWrvFEjAoKb7y4O6bPqsX9UiNueHDCoBNJO9VjXKw4kEvb5k7ZUCAXO8x0MWN\n\ti5LOJO/ep9MGUCPR5TEWzUSInBNuus5OKfah/E1An/YFq1wDDY97/VOKh9IR7/WR/zu9\n\tfteMFg3L2pHqlsPN5w5Lirg5HprIqEv3o+Otud7/KyTgXTgCqiP4jc7ARbtYHZcPxEAm\n\tmpOA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=cr7TsNrsXVYnUxum2cTe/u8nlJMtPOGrul+Co4AWH44=;\n\tb=M9iGk+tr65wo6t26O/nBj7yyCwa0qKjBpcqvQ9t4DTiDYz3fSOBN9+wzaA2bAcP4+R\n\tqRH4z1tuXzWVhJ57sLr/l+sjfTONxiz0d9xQJFhsGxuliODQZ0HrgZeawkBFRYzRHaCS\n\tst4YbWj6v++KiQkA6+UmzjvmSozbWEtbd+bRy59RrDETIaJ8IoeJ0iV8RHZifO6ztovn\n\tFl3NudEwwofA0sx1dlBoN0L4rK2PV0PSoCRwHQsT23Oo1cktjccznQymkgiyTvaB1Did\n\tRHu74faCVUSrTLWxDMeScaE4O1lVaFLe19d7YQ9bW5ms7+E6vkpe3FRBabWhlyWlf6iY\n\t68tQ==","X-Gm-Message-State":"AOAM532Lx9aJllrj3zBBNAKzyG0ExZeh0sW1WJgKVgzLKwa4STUUsLcK\n\tFzgZOy+eo0fmNORmfrEfUcn/iSfcCZL1CFwqPKbKLg==","X-Google-Smtp-Source":"ABdhPJyfmbM6qW6jJrcsskxUUXOJ/otOWdNpqkqRL7Lzy8w9hg7oQvhze0LHux+MML/crfdCw5a685G3d4KR614KNZ0=","X-Received":"by 2002:adf:ea0d:: with SMTP id\n\tq13mr53189127wrm.597.1641569484635; \n\tFri, 07 Jan 2022 07:31:24 -0800 (PST)","MIME-Version":"1.0","References":"<20220107125506.1477795-1-kieran.bingham@ideasonboard.com>\n\t<20220107125506.1477795-3-kieran.bingham@ideasonboard.com>\n\t<20220107150132.usvammci5fqfyux5@uno.localdomain>\n\t<164156828395.1224575.14685464986290943153@Monstersaurus>","In-Reply-To":"<164156828395.1224575.14685464986290943153@Monstersaurus>","From":"David Plowman ","Date":"Fri, 7 Jan 2022 15:31:13 +0000","Message-ID":"","To":"Kieran Bingham ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: base: signal: Prevent\n\texcessive slot usage","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21987,"web_url":"https://patchwork.libcamera.org/comment/21987/","msgid":"","date":"2022-01-09T02:13:42","subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: base: signal: Prevent\n\texcessive slot usage","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nThank you for the patch.\n\nOn Fri, Jan 07, 2022 at 03:11:23PM +0000, Kieran Bingham wrote:\n> Quoting Jacopo Mondi (2022-01-07 15:01:32)\n> > On Fri, Jan 07, 2022 at 12:55:06PM +0000, Kieran Bingham wrote:\n> > > Slots are not expected to be connected to lots of signals. Our normal\n> > \n> > Isn't it the other way around ?\n> > \"Signals are not expected to be connected to a lots of slots\" :)\n> \n> I ... er ... um ... probably? maybe ;-)\n> \n> > > use case is a one to one mapping with a signal expected to call one\n> > > slot.\n> > >\n> > > While we support multiple slots, excessive use is likely the result of a\n> > > bug indicating a failure to disconnect slots or repeated connection of a\n> > > slot which can lead to memory leaks and poor performance when deleting\n> > > the Object.\n> > >\n> > > Assert that a maximum of 8 slots are connected which will catch any bug\n> > > and still allow some multiple slot usage. If this limit is ever met it\n> > > can be extended and considered at that time.\n> > \n> > I wish we had WARN_ONCE().\n> \n> We could add that, but in this instance, I really want the assertion to\n> break things, otherwise for instance in CTS it woudl just keep running -\n> and woudl require someone to manually go through the logs.\n> \n> Maybe it would be noisy enough to get noticed ... but maybe not...\n> \n> > > Signed-off-by: Kieran Bingham \n> > > ---\n> > >\n> > > Note that this patch causes the CTS tests to fail as they hit this\n> > > assertion.\n> > >\n> > > It is not yet investigated if we really need more slots or if the\n> > > assertion has already caught a slot-leak, but posting for early review\n> > > anyway.\n> \n> Of course the fact that it breaks CTS currently means this patch can't\n> go in until it's resolved anyway.\n> \n> > > src/libcamera/base/signal.cpp | 8 ++++++++\n> > > 1 file changed, 8 insertions(+)\n> > >\n> > > diff --git a/src/libcamera/base/signal.cpp b/src/libcamera/base/signal.cpp\n> > > index 9df45d079a42..7e6a23cc909d 100644\n> > > --- a/src/libcamera/base/signal.cpp\n> > > +++ b/src/libcamera/base/signal.cpp\n> > > @@ -7,6 +7,7 @@\n> > >\n> > > #include \n> > >\n> > > +#include \n> > > #include \n> > >\n> > > /**\n> > > @@ -35,6 +36,13 @@ void SignalBase::connect(BoundMethodBase *slot)\n> > > if (object)\n> > > object->connect(this);\n> > > slots_.push_back(slot);\n> > > +\n> > > + /*\n> > > + * Slots are not expected to be used to connect many items. If we exceed\n> > > + * a reasonable amount, presume that there is a bug and fail fast on\n> > > + * debug builds.\n> > > + */\n> > > + ASSERT(slots_.size() < 8);\n> > \n> > That's indeed a useful safety check when developing. I think it's\n> > worth having it in.\n> \n> Thanks, I agree of course (once we find/fix the associated CTS bug)\n\nYou know how much I dislike such arbitrary limits :-)\n\nOne option would be to check if an identical connection already exists.\nQt has a Qt::UniqueConnection flag\n(https://doc.qt.io/qt-6/qt.html#ConnectionType-enum) for this purpose,\nwe could make it the default and add a non-unique connection flag (or\njust skip it for now until we have use cases for multiple connections of\nthe same signal to the same slot). The advantage is that we could then\ncatch the very first invalid connection attempt, possibly also catching\nmore bugs that would create extraneous connections but not go above the\nlimit.\n\nOne issue, however, is that comparing connections may not be trivial\nwhen the slot is a lambda function, as the closure class created by\nlambda function has no equality comparison operator (the problem would\nbe the same for any functor without an equality comparison operator). I\nthink erroneous connections to lambda slots is exactly what you were\ntrying to catch in this patch series.\n\nIt could be possible to hack around this, by passing __builtin_FILE()\nand __builtin_LINE() as value for default arguments to the connect()\nfunction (considering that two lambda slots are equal if they're defined\non the same source line), but that's not very pretty, nor very exact:\n\n\tfor (auto object : objects)\n\t\tmySignal.connect([object]() { object->slot(); });\n\nwould consider the slots to be identical. It could be argued that, in\nthis case, the caller should pass the non-unique connection flag though.\n\n> > Reviewed-by: Jacopo Mondi \n> > \n> > > }\n> > >\n> > > void SignalBase::disconnect(Object *object)","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 79D57BE080\n\tfor ;\n\tSun, 9 Jan 2022 02:13:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D0A3C60921;\n\tSun, 9 Jan 2022 03:13:52 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1B6226017E\n\tfor ;\n\tSun, 9 Jan 2022 03:13:51 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 70778A1B;\n\tSun, 9 Jan 2022 03:13:50 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"PVdmeB+R\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1641694430;\n\tbh=r3v8Tibyeh8chVlJyTHf0aEtDdlr0YShcOG/RpDO6yY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=PVdmeB+RN/HhW09mYB6lbsV+POIWKjjVdLOyLPNgWLL6FGsZ7CJF+gef7kYxuuwC1\n\tckvgwLIIp1dpE1gq4O1vMZhJuIBs+4rNECUczmMbckQaxreHHDUNY2bLQGq/HXaQLB\n\tquNuNkh+ZUSd6br60DTegmedoliUMQb56/1A5i+o=","Date":"Sun, 9 Jan 2022 04:13:42 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20220107125506.1477795-1-kieran.bingham@ideasonboard.com>\n\t<20220107125506.1477795-3-kieran.bingham@ideasonboard.com>\n\t<20220107150132.usvammci5fqfyux5@uno.localdomain>\n\t<164156828395.1224575.14685464986290943153@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<164156828395.1224575.14685464986290943153@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: base: signal: Prevent\n\texcessive slot usage","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]