[{"id":31820,"web_url":"https://patchwork.libcamera.org/comment/31820/","msgid":"<172937784724.2485972.2782330253351017591@ping.linuxembedded.co.uk>","date":"2024-10-19T22:44:07","subject":"Re: [PATCH 1/4] libcamera: Add signal disconnected for IPC","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Harvey Yang (2024-10-18 08:57:34)\n> This CL also adds an API in Camera::Private to trigger\n> Camera::disconnected signal.\n\nCL?\n\n\nPlease read: https://cbea.ms/git-commit/#why-not-how\n\nBut adding a signal on this sounds like a good idea to me.\n\nHow does the disconnected signal work. Is it reported when the IPA is\nclosed? (or crashes?) or is there some reason for the IP\n\n> \n> Signed-off-by: Harvey Yang \n> Co-developed-by: Han-Lin Chen \n> Signed-off-by: Han-Lin Chen \n> ---\n> include/libcamera/internal/camera.h | 2 ++\n> .../libcamera/internal/ipc_pipe_unixsocket.h | 2 ++\n> include/libcamera/internal/ipc_unixsocket.h | 2 ++\n> src/libcamera/camera.cpp | 10 ++++++++\n> src/libcamera/ipc_pipe_unixsocket.cpp | 8 +++++++\n> src/libcamera/ipc_unixsocket.cpp | 24 +++++++++++++++++--\n> .../module_ipa_proxy.cpp.tmpl | 8 +++++++\n> .../module_ipa_proxy.h.tmpl | 2 ++\n> 8 files changed, 56 insertions(+), 2 deletions(-)\n> \n> diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h\n> index 0add0428b..0bef0980e 100644\n> --- a/include/libcamera/internal/camera.h\n> +++ b/include/libcamera/internal/camera.h\n> @@ -33,6 +33,8 @@ public:\n> \n> PipelineHandler *pipe() { return pipe_.get(); }\n> \n> + void notifyDisconnection();\n> +\n> std::list queuedRequests_;\n> ControlInfoMap controlInfo_;\n> ControlList properties_;\n> diff --git a/include/libcamera/internal/ipc_pipe_unixsocket.h b/include/libcamera/internal/ipc_pipe_unixsocket.h\n> index 8c972613f..09f65b102 100644\n> --- a/include/libcamera/internal/ipc_pipe_unixsocket.h\n> +++ b/include/libcamera/internal/ipc_pipe_unixsocket.h\n> @@ -28,6 +28,8 @@ public:\n> \n> int sendAsync(const IPCMessage &data) override;\n> \n> + Signal<> *disconnected();\n> +\n> private:\n> struct CallData {\n> IPCUnixSocket::Payload *response;\n> diff --git a/include/libcamera/internal/ipc_unixsocket.h b/include/libcamera/internal/ipc_unixsocket.h\n> index 48bb7a942..a1e326b6b 100644\n> --- a/include/libcamera/internal/ipc_unixsocket.h\n> +++ b/include/libcamera/internal/ipc_unixsocket.h\n> @@ -39,6 +39,8 @@ public:\n> \n> Signal<> readyRead;\n> \n> + Signal<> disconnected;\n> +\n> private:\n> struct Header {\n> uint32_t data;\n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index a86f552a4..ef5a6725f 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -603,6 +603,16 @@ Camera::Private::~Private()\n> * \\return The pipeline handler that created this camera\n> */\n> \n> +/**\n> + * \\brief Notify the application that camera is disconnected with signal\n> + * Camera::disconnected\n> + */\n> +void Camera::Private::notifyDisconnection()\n> +{\n> + Camera *o = LIBCAMERA_O_PTR();\n> + o->disconnected.emit();\n> +}\n> +\n> /**\n> * \\fn Camera::Private::validator()\n> * \\brief Retrieve the control validator related to this camera\n> diff --git a/src/libcamera/ipc_pipe_unixsocket.cpp b/src/libcamera/ipc_pipe_unixsocket.cpp\n> index 668ec73b9..51fd3b1fb 100644\n> --- a/src/libcamera/ipc_pipe_unixsocket.cpp\n> +++ b/src/libcamera/ipc_pipe_unixsocket.cpp\n> @@ -84,6 +84,14 @@ int IPCPipeUnixSocket::sendAsync(const IPCMessage &data)\n> return 0;\n> }\n> \n> +Signal<> *IPCPipeUnixSocket::disconnected()\n> +{\n> + if (socket_)\n> + return &socket_->disconnected;\n> +\n> + return nullptr;\n> +}\n> +\n> void IPCPipeUnixSocket::readyRead()\n> {\n> IPCUnixSocket::Payload payload;\n> diff --git a/src/libcamera/ipc_unixsocket.cpp b/src/libcamera/ipc_unixsocket.cpp\n> index 002053e35..3d0248857 100644\n> --- a/src/libcamera/ipc_unixsocket.cpp\n> +++ b/src/libcamera/ipc_unixsocket.cpp\n> @@ -186,11 +186,16 @@ int IPCUnixSocket::send(const Payload &payload)\n> if (!hdr.data && !hdr.fds)\n> return -EINVAL;\n> \n> - ret = ::send(fd_.get(), &hdr, sizeof(hdr), 0);\n> + ret = ::send(fd_.get(), &hdr, sizeof(hdr), MSG_NOSIGNAL);\n\nWhat's this flag change for? Can you explain in the commit message or as\na comment please?\n\n> if (ret < 0) {\n> ret = -errno;\n> LOG(IPCUnixSocket, Error)\n> << \"Failed to send: \" << strerror(-ret);\n> + if (errno == ECONNRESET) {\n> + disconnected.emit();\n> + fd_.reset();\n\nSo this is really about if the IPA connection gets severed indeed?\n\n> + }\n> +\n> return ret;\n> }\n> \n> @@ -241,6 +246,11 @@ int IPCUnixSocket::receive(Payload *payload)\n> * \\brief A Signal emitted when a message is ready to be read\n> */\n> \n> +/**\n> + * \\var IPCUnixSocket::disconnected\n> + * \\brief A Signal emitted when the Unix socket IPC is disconnected\n> + */\n> +\n> int IPCUnixSocket::sendData(const void *buffer, size_t length,\n> const int32_t *fds, unsigned int num)\n> {\n> @@ -266,10 +276,15 @@ int IPCUnixSocket::sendData(const void *buffer, size_t length,\n> if (fds)\n> memcpy(CMSG_DATA(cmsg), fds, num * sizeof(uint32_t));\n> \n> - if (sendmsg(fd_.get(), &msg, 0) < 0) {\n> + if (sendmsg(fd_.get(), &msg, MSG_NOSIGNAL) < 0) {\n> int ret = -errno;\n> LOG(IPCUnixSocket, Error)\n> << \"Failed to sendmsg: \" << strerror(-ret);\n> + if (errno == ECONNRESET) {\n> + disconnected.emit();\n> + fd_.reset();\n> + }\n> +\n> return ret;\n> }\n> \n> @@ -324,6 +339,11 @@ void IPCUnixSocket::dataNotifier()\n> ret = -errno;\n> LOG(IPCUnixSocket, Error)\n> << \"Failed to receive header: \" << strerror(-ret);\n> + if (errno == ECONNRESET) {\n> + disconnected.emit();\n> + fd_.reset();\n> + }\n> +\n> return;\n> }\n> \n> diff --git a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> index ce3cc5ab6..27f03417a 100644\n> --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> @@ -104,6 +104,14 @@ namespace {{ns}} {\n> }\n> }\n> \n> +Signal<> *{{proxy_name}}::disconnected()\n> +{\n> + if (ipc_)\n> + return ipc_->disconnected();\n> +\n> + return nullptr;\n> +}\n> +\n> {% if interface_event.methods|length > 0 %}\n> void {{proxy_name}}::recvMessage(const IPCMessage &data)\n> {\n> diff --git a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> index e213b18a0..2b7ba872e 100644\n> --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> @@ -53,6 +53,8 @@ public:\n> > {{method.mojom_name}};\n> {% endfor %}\n> \n> + Signal<> *disconnected();\n> +\n> private:\n> void recvMessage(const IPCMessage &data);\n> \n> -- \n> 2.47.0.rc1.288.g06298d1525-goog\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 BA955C3304\n\tfor ;\n\tSat, 19 Oct 2024 22:44:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 905206538C;\n\tSun, 20 Oct 2024 00:44:12 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C2634633C6\n\tfor ;\n\tSun, 20 Oct 2024 00:44:09 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C55DF502;\n\tSun, 20 Oct 2024 00:42:24 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"JtHEKOb5\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1729377744;\n\tbh=4AlNNVn+5IArQqtXYobr8iH9+kT/USCNBV2nAiWHWP8=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=JtHEKOb5x4s29ySCJ5dItefFH9juFQnKw8RCbhtGGQ57KuxqlOyO+HEyJwpvhnzE5\n\tyYsSJT+8iQ8z/k+8w2hoVu3ReRQCb4UrvN2W9i09WARpTZgLBvcx/Yj3Z45Bho1xXw\n\toUp09yxMJqk5KCEjGqmfYbGw184fmLrS+IJl+Kc4=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20241018075942.1150378-2-chenghaoyang@chromium.org>","References":"<20241018075942.1150378-1-chenghaoyang@chromium.org>\n\t<20241018075942.1150378-2-chenghaoyang@chromium.org>","Subject":"Re: [PATCH 1/4] libcamera: Add signal disconnected for IPC","From":"Kieran Bingham ","Cc":"Harvey Yang ,\n\tHan-Lin Chen ","To":"Harvey Yang ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Sat, 19 Oct 2024 23:44:07 +0100","Message-ID":"<172937784724.2485972.2782330253351017591@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31825,"web_url":"https://patchwork.libcamera.org/comment/31825/","msgid":"<172942024566.31090.11990963289361470@ping.linuxembedded.co.uk>","date":"2024-10-20T10:30:45","subject":"Re: [PATCH 1/4] libcamera: Add signal disconnected for IPC","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Kieran Bingham (2024-10-19 23:44:07)\n> Quoting Harvey Yang (2024-10-18 08:57:34)\n> > This CL also adds an API in Camera::Private to trigger\n> > Camera::disconnected signal.\n> \n> CL?\n> \n> \n> Please read: https://cbea.ms/git-commit/#why-not-how\n> \n> But adding a signal on this sounds like a good idea to me.\n> \n> How does the disconnected signal work. Is it reported when the IPA is\n> closed? (or crashes?) \n\n\nSorry - somehow I dropped a sentence here:\n\n> or is there some reason for the IP\n\n... or is there some other reason for the IPC to be disconnected ?\n\n\n> \n> > \n> > Signed-off-by: Harvey Yang \n> > Co-developed-by: Han-Lin Chen \n> > Signed-off-by: Han-Lin Chen \n> > ---\n> > include/libcamera/internal/camera.h | 2 ++\n> > .../libcamera/internal/ipc_pipe_unixsocket.h | 2 ++\n> > include/libcamera/internal/ipc_unixsocket.h | 2 ++\n> > src/libcamera/camera.cpp | 10 ++++++++\n> > src/libcamera/ipc_pipe_unixsocket.cpp | 8 +++++++\n> > src/libcamera/ipc_unixsocket.cpp | 24 +++++++++++++++++--\n> > .../module_ipa_proxy.cpp.tmpl | 8 +++++++\n> > .../module_ipa_proxy.h.tmpl | 2 ++\n> > 8 files changed, 56 insertions(+), 2 deletions(-)\n> > \n> > diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h\n> > index 0add0428b..0bef0980e 100644\n> > --- a/include/libcamera/internal/camera.h\n> > +++ b/include/libcamera/internal/camera.h\n> > @@ -33,6 +33,8 @@ public:\n> > \n> > PipelineHandler *pipe() { return pipe_.get(); }\n> > \n> > + void notifyDisconnection();\n> > +\n> > std::list queuedRequests_;\n> > ControlInfoMap controlInfo_;\n> > ControlList properties_;\n> > diff --git a/include/libcamera/internal/ipc_pipe_unixsocket.h b/include/libcamera/internal/ipc_pipe_unixsocket.h\n> > index 8c972613f..09f65b102 100644\n> > --- a/include/libcamera/internal/ipc_pipe_unixsocket.h\n> > +++ b/include/libcamera/internal/ipc_pipe_unixsocket.h\n> > @@ -28,6 +28,8 @@ public:\n> > \n> > int sendAsync(const IPCMessage &data) override;\n> > \n> > + Signal<> *disconnected();\n> > +\n> > private:\n> > struct CallData {\n> > IPCUnixSocket::Payload *response;\n> > diff --git a/include/libcamera/internal/ipc_unixsocket.h b/include/libcamera/internal/ipc_unixsocket.h\n> > index 48bb7a942..a1e326b6b 100644\n> > --- a/include/libcamera/internal/ipc_unixsocket.h\n> > +++ b/include/libcamera/internal/ipc_unixsocket.h\n> > @@ -39,6 +39,8 @@ public:\n> > \n> > Signal<> readyRead;\n> > \n> > + Signal<> disconnected;\n> > +\n> > private:\n> > struct Header {\n> > uint32_t data;\n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index a86f552a4..ef5a6725f 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -603,6 +603,16 @@ Camera::Private::~Private()\n> > * \\return The pipeline handler that created this camera\n> > */\n> > \n> > +/**\n> > + * \\brief Notify the application that camera is disconnected with signal\n> > + * Camera::disconnected\n> > + */\n> > +void Camera::Private::notifyDisconnection()\n> > +{\n> > + Camera *o = LIBCAMERA_O_PTR();\n> > + o->disconnected.emit();\n> > +}\n> > +\n> > /**\n> > * \\fn Camera::Private::validator()\n> > * \\brief Retrieve the control validator related to this camera\n> > diff --git a/src/libcamera/ipc_pipe_unixsocket.cpp b/src/libcamera/ipc_pipe_unixsocket.cpp\n> > index 668ec73b9..51fd3b1fb 100644\n> > --- a/src/libcamera/ipc_pipe_unixsocket.cpp\n> > +++ b/src/libcamera/ipc_pipe_unixsocket.cpp\n> > @@ -84,6 +84,14 @@ int IPCPipeUnixSocket::sendAsync(const IPCMessage &data)\n> > return 0;\n> > }\n> > \n> > +Signal<> *IPCPipeUnixSocket::disconnected()\n> > +{\n> > + if (socket_)\n> > + return &socket_->disconnected;\n> > +\n> > + return nullptr;\n> > +}\n> > +\n> > void IPCPipeUnixSocket::readyRead()\n> > {\n> > IPCUnixSocket::Payload payload;\n> > diff --git a/src/libcamera/ipc_unixsocket.cpp b/src/libcamera/ipc_unixsocket.cpp\n> > index 002053e35..3d0248857 100644\n> > --- a/src/libcamera/ipc_unixsocket.cpp\n> > +++ b/src/libcamera/ipc_unixsocket.cpp\n> > @@ -186,11 +186,16 @@ int IPCUnixSocket::send(const Payload &payload)\n> > if (!hdr.data && !hdr.fds)\n> > return -EINVAL;\n> > \n> > - ret = ::send(fd_.get(), &hdr, sizeof(hdr), 0);\n> > + ret = ::send(fd_.get(), &hdr, sizeof(hdr), MSG_NOSIGNAL);\n> \n> What's this flag change for? Can you explain in the commit message or as\n> a comment please?\n> \n> > if (ret < 0) {\n> > ret = -errno;\n> > LOG(IPCUnixSocket, Error)\n> > << \"Failed to send: \" << strerror(-ret);\n> > + if (errno == ECONNRESET) {\n> > + disconnected.emit();\n> > + fd_.reset();\n> \n> So this is really about if the IPA connection gets severed indeed?\n> \n> > + }\n> > +\n> > return ret;\n> > }\n> > \n> > @@ -241,6 +246,11 @@ int IPCUnixSocket::receive(Payload *payload)\n> > * \\brief A Signal emitted when a message is ready to be read\n> > */\n> > \n> > +/**\n> > + * \\var IPCUnixSocket::disconnected\n> > + * \\brief A Signal emitted when the Unix socket IPC is disconnected\n> > + */\n> > +\n> > int IPCUnixSocket::sendData(const void *buffer, size_t length,\n> > const int32_t *fds, unsigned int num)\n> > {\n> > @@ -266,10 +276,15 @@ int IPCUnixSocket::sendData(const void *buffer, size_t length,\n> > if (fds)\n> > memcpy(CMSG_DATA(cmsg), fds, num * sizeof(uint32_t));\n> > \n> > - if (sendmsg(fd_.get(), &msg, 0) < 0) {\n> > + if (sendmsg(fd_.get(), &msg, MSG_NOSIGNAL) < 0) {\n> > int ret = -errno;\n> > LOG(IPCUnixSocket, Error)\n> > << \"Failed to sendmsg: \" << strerror(-ret);\n> > + if (errno == ECONNRESET) {\n> > + disconnected.emit();\n> > + fd_.reset();\n> > + }\n> > +\n> > return ret;\n> > }\n> > \n> > @@ -324,6 +339,11 @@ void IPCUnixSocket::dataNotifier()\n> > ret = -errno;\n> > LOG(IPCUnixSocket, Error)\n> > << \"Failed to receive header: \" << strerror(-ret);\n> > + if (errno == ECONNRESET) {\n> > + disconnected.emit();\n> > + fd_.reset();\n> > + }\n> > +\n> > return;\n> > }\n> > \n> > diff --git a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> > index ce3cc5ab6..27f03417a 100644\n> > --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> > +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> > @@ -104,6 +104,14 @@ namespace {{ns}} {\n> > }\n> > }\n> > \n> > +Signal<> *{{proxy_name}}::disconnected()\n> > +{\n> > + if (ipc_)\n> > + return ipc_->disconnected();\n> > +\n> > + return nullptr;\n> > +}\n> > +\n> > {% if interface_event.methods|length > 0 %}\n> > void {{proxy_name}}::recvMessage(const IPCMessage &data)\n> > {\n> > diff --git a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> > index e213b18a0..2b7ba872e 100644\n> > --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> > +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> > @@ -53,6 +53,8 @@ public:\n> > > {{method.mojom_name}};\n> > {% endfor %}\n> > \n> > + Signal<> *disconnected();\n> > +\n> > private:\n> > void recvMessage(const IPCMessage &data);\n> > \n> > -- \n> > 2.47.0.rc1.288.g06298d1525-goog\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 66C28BD1F1\n\tfor ;\n\tSun, 20 Oct 2024 10:30:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 465006538C;\n\tSun, 20 Oct 2024 12:30:51 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D7DBB65379\n\tfor ;\n\tSun, 20 Oct 2024 12:30:48 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B5FCC352;\n\tSun, 20 Oct 2024 12:29:03 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"vZ56AZ01\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1729420143;\n\tbh=g/JTFZ9HFjGHzMrakIKR/pi3YWr0ngGDbSDwueylDyE=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=vZ56AZ01DnDM077DESuYY/pVvFl/xhM6KMjhTZu03gJm5AlZ0NZSxzwubodgTgsby\n\tqHX0fyhhK8g3qWeRKDZmqK63HJJgtirC2Wki1H9KG1DfHthamlVKTtb7JT2H8pgW+V\n\tCVxXpuzq+SrTYkW5Iw1h6x5xJ/Em1jNnz+tiuNpw=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<172937784724.2485972.2782330253351017591@ping.linuxembedded.co.uk>","References":"<20241018075942.1150378-1-chenghaoyang@chromium.org>\n\t<20241018075942.1150378-2-chenghaoyang@chromium.org>\n\t<172937784724.2485972.2782330253351017591@ping.linuxembedded.co.uk>","Subject":"Re: [PATCH 1/4] libcamera: Add signal disconnected for IPC","From":"Kieran Bingham ","Cc":"Harvey Yang ,\n\tHan-Lin Chen ","To":"Harvey Yang ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Sun, 20 Oct 2024 11:30:45 +0100","Message-ID":"<172942024566.31090.11990963289361470@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31834,"web_url":"https://patchwork.libcamera.org/comment/31834/","msgid":"<20241020164000.GF7770@pendragon.ideasonboard.com>","date":"2024-10-20T16:40:00","subject":"Re: [PATCH 1/4] libcamera: Add signal disconnected for IPC","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Sun, Oct 20, 2024 at 11:30:45AM +0100, Kieran Bingham wrote:\n> Quoting Kieran Bingham (2024-10-19 23:44:07)\n> > Quoting Harvey Yang (2024-10-18 08:57:34)\n> > > This CL also adds an API in Camera::Private to trigger\n> > > Camera::disconnected signal.\n> > \n> > CL?\n\nChange List, the gerrit term for a patch.\n\n> > Please read: https://cbea.ms/git-commit/#why-not-how\n\n+1\n\n> > \n> > But adding a signal on this sounds like a good idea to me.\n> > \n> > How does the disconnected signal work. Is it reported when the IPA is\n> > closed? (or crashes?) \n> \n> Sorry - somehow I dropped a sentence here:\n> \n> > or is there some reason for the IP\n> \n> ... or is there some other reason for the IPC to be disconnected ?\n> \n> > > Signed-off-by: Harvey Yang \n> > > Co-developed-by: Han-Lin Chen \n> > > Signed-off-by: Han-Lin Chen \n> > > ---\n> > > include/libcamera/internal/camera.h | 2 ++\n> > > .../libcamera/internal/ipc_pipe_unixsocket.h | 2 ++\n> > > include/libcamera/internal/ipc_unixsocket.h | 2 ++\n> > > src/libcamera/camera.cpp | 10 ++++++++\n> > > src/libcamera/ipc_pipe_unixsocket.cpp | 8 +++++++\n> > > src/libcamera/ipc_unixsocket.cpp | 24 +++++++++++++++++--\n> > > .../module_ipa_proxy.cpp.tmpl | 8 +++++++\n> > > .../module_ipa_proxy.h.tmpl | 2 ++\n> > > 8 files changed, 56 insertions(+), 2 deletions(-)\n> > > \n> > > diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h\n> > > index 0add0428b..0bef0980e 100644\n> > > --- a/include/libcamera/internal/camera.h\n> > > +++ b/include/libcamera/internal/camera.h\n> > > @@ -33,6 +33,8 @@ public:\n> > > \n> > > PipelineHandler *pipe() { return pipe_.get(); }\n> > > \n> > > + void notifyDisconnection();\n> > > +\n> > > std::list queuedRequests_;\n> > > ControlInfoMap controlInfo_;\n> > > ControlList properties_;\n> > > diff --git a/include/libcamera/internal/ipc_pipe_unixsocket.h b/include/libcamera/internal/ipc_pipe_unixsocket.h\n> > > index 8c972613f..09f65b102 100644\n> > > --- a/include/libcamera/internal/ipc_pipe_unixsocket.h\n> > > +++ b/include/libcamera/internal/ipc_pipe_unixsocket.h\n> > > @@ -28,6 +28,8 @@ public:\n> > > \n> > > int sendAsync(const IPCMessage &data) override;\n> > > \n> > > + Signal<> *disconnected();\n> > > +\n> > > private:\n> > > struct CallData {\n> > > IPCUnixSocket::Payload *response;\n> > > diff --git a/include/libcamera/internal/ipc_unixsocket.h b/include/libcamera/internal/ipc_unixsocket.h\n> > > index 48bb7a942..a1e326b6b 100644\n> > > --- a/include/libcamera/internal/ipc_unixsocket.h\n> > > +++ b/include/libcamera/internal/ipc_unixsocket.h\n> > > @@ -39,6 +39,8 @@ public:\n> > > \n> > > Signal<> readyRead;\n> > > \n> > > + Signal<> disconnected;\n> > > +\n> > > private:\n> > > struct Header {\n> > > uint32_t data;\n> > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > index a86f552a4..ef5a6725f 100644\n> > > --- a/src/libcamera/camera.cpp\n> > > +++ b/src/libcamera/camera.cpp\n> > > @@ -603,6 +603,16 @@ Camera::Private::~Private()\n> > > * \\return The pipeline handler that created this camera\n> > > */\n> > > \n> > > +/**\n> > > + * \\brief Notify the application that camera is disconnected with signal\n> > > + * Camera::disconnected\n\nThat seems to be a big shortcut. IPC disconnection doesn't mean the\ncamera has been disconnected. What will happen if the camera then really\nbecomes disconnected ? Won't the disconnect signal be emitted a second\ntime, confusing applications ? And how is the pipeline handler expected\nto gracefully shut things down ? I think there's still some design\nhomework to be done here.\n\nI would also like to understand under which circumstances you've seen\nthis happening.\n\n> > > + */\n> > > +void Camera::Private::notifyDisconnection()\n> > > +{\n> > > + Camera *o = LIBCAMERA_O_PTR();\n> > > + o->disconnected.emit();\n> > > +}\n> > > +\n> > > /**\n> > > * \\fn Camera::Private::validator()\n> > > * \\brief Retrieve the control validator related to this camera\n> > > diff --git a/src/libcamera/ipc_pipe_unixsocket.cpp b/src/libcamera/ipc_pipe_unixsocket.cpp\n> > > index 668ec73b9..51fd3b1fb 100644\n> > > --- a/src/libcamera/ipc_pipe_unixsocket.cpp\n> > > +++ b/src/libcamera/ipc_pipe_unixsocket.cpp\n> > > @@ -84,6 +84,14 @@ int IPCPipeUnixSocket::sendAsync(const IPCMessage &data)\n> > > return 0;\n> > > }\n> > > \n> > > +Signal<> *IPCPipeUnixSocket::disconnected()\n> > > +{\n> > > + if (socket_)\n> > > + return &socket_->disconnected;\n> > > +\n> > > + return nullptr;\n> > > +}\n> > > +\n> > > void IPCPipeUnixSocket::readyRead()\n> > > {\n> > > IPCUnixSocket::Payload payload;\n> > > diff --git a/src/libcamera/ipc_unixsocket.cpp b/src/libcamera/ipc_unixsocket.cpp\n> > > index 002053e35..3d0248857 100644\n> > > --- a/src/libcamera/ipc_unixsocket.cpp\n> > > +++ b/src/libcamera/ipc_unixsocket.cpp\n> > > @@ -186,11 +186,16 @@ int IPCUnixSocket::send(const Payload &payload)\n> > > if (!hdr.data && !hdr.fds)\n> > > return -EINVAL;\n> > > \n> > > - ret = ::send(fd_.get(), &hdr, sizeof(hdr), 0);\n> > > + ret = ::send(fd_.get(), &hdr, sizeof(hdr), MSG_NOSIGNAL);\n> > \n> > What's this flag change for? Can you explain in the commit message or as\n> > a comment please?\n\nThe MSG_NOSIGNAL documentation states\n\n Don’t generate a SIGPIPE signal if the peer on a stream-oriented\n socket has closed the connection. The EPIPE error is still\n returned. This provides similar behavior to using sigaction(2) to\n ignore SIGPIPE, but, whereas MSG_NOSIGNAL is a per-call feature,\n ignoring SIGPIPE sets a process attribute that affects all threads\n in the process.\n\nbut here you're checking ECONNRESET. The documentation of ECONNRESET and\nEPIPE for this function respectively state\n\n ECONNRESET\n Connection reset by peer.\n\n EPIPE\n\tThe local end has been shut down on a connection oriented\n\tsocket. In this case, the process will also receive a SIGPIPE\n\tunless MSG_NOSIGNAL is set.\n\nDo we need MSG_NOSIGNAL to properly handle ECONNRESET ? Do we need to\nhandle EPIPE ? In any case, all this needs to be explained in the commit\nmessage.\n\n> > \n> > > if (ret < 0) {\n> > > ret = -errno;\n> > > LOG(IPCUnixSocket, Error)\n> > > << \"Failed to send: \" << strerror(-ret);\n> > > + if (errno == ECONNRESET) {\n\nerrno may have been modified by the LOG statement. You need to use ret\nhere. Same below.\n\n> > > + disconnected.emit();\n> > > + fd_.reset();\n\nWhy fd_.reset() here and below ? The commit message really misses lots\nof information.\n\n> > \n> > So this is really about if the IPA connection gets severed indeed?\n> > \n> > > + }\n> > > +\n> > > return ret;\n> > > }\n> > > \n> > > @@ -241,6 +246,11 @@ int IPCUnixSocket::receive(Payload *payload)\n> > > * \\brief A Signal emitted when a message is ready to be read\n> > > */\n> > > \n> > > +/**\n> > > + * \\var IPCUnixSocket::disconnected\n> > > + * \\brief A Signal emitted when the Unix socket IPC is disconnected\n> > > + */\n> > > +\n> > > int IPCUnixSocket::sendData(const void *buffer, size_t length,\n> > > const int32_t *fds, unsigned int num)\n> > > {\n> > > @@ -266,10 +276,15 @@ int IPCUnixSocket::sendData(const void *buffer, size_t length,\n> > > if (fds)\n> > > memcpy(CMSG_DATA(cmsg), fds, num * sizeof(uint32_t));\n> > > \n> > > - if (sendmsg(fd_.get(), &msg, 0) < 0) {\n> > > + if (sendmsg(fd_.get(), &msg, MSG_NOSIGNAL) < 0) {\n> > > int ret = -errno;\n> > > LOG(IPCUnixSocket, Error)\n> > > << \"Failed to sendmsg: \" << strerror(-ret);\n> > > + if (errno == ECONNRESET) {\n> > > + disconnected.emit();\n> > > + fd_.reset();\n> > > + }\n> > > +\n> > > return ret;\n> > > }\n> > > \n> > > @@ -324,6 +339,11 @@ void IPCUnixSocket::dataNotifier()\n> > > ret = -errno;\n> > > LOG(IPCUnixSocket, Error)\n> > > << \"Failed to receive header: \" << strerror(-ret);\n> > > + if (errno == ECONNRESET) {\n> > > + disconnected.emit();\n> > > + fd_.reset();\n> > > + }\n> > > +\n> > > return;\n> > > }\n> > > \n> > > diff --git a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> > > index ce3cc5ab6..27f03417a 100644\n> > > --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> > > +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> > > @@ -104,6 +104,14 @@ namespace {{ns}} {\n> > > }\n> > > }\n> > > \n> > > +Signal<> *{{proxy_name}}::disconnected()\n> > > +{\n> > > + if (ipc_)\n> > > + return ipc_->disconnected();\n> > > +\n> > > + return nullptr;\n> > > +}\n> > > +\n> > > {% if interface_event.methods|length > 0 %}\n> > > void {{proxy_name}}::recvMessage(const IPCMessage &data)\n> > > {\n> > > diff --git a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> > > index e213b18a0..2b7ba872e 100644\n> > > --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> > > +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> > > @@ -53,6 +53,8 @@ public:\n> > > > {{method.mojom_name}};\n> > > {% endfor %}\n> > > \n> > > + Signal<> *disconnected();\n> > > +\n> > > private:\n> > > void recvMessage(const IPCMessage &data);\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 0D269C3304\n\tfor ;\n\tSun, 20 Oct 2024 16:40:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8BE856538F;\n\tSun, 20 Oct 2024 18:40:08 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 12F7365379\n\tfor ;\n\tSun, 20 Oct 2024 18:40:07 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5269978E;\n\tSun, 20 Oct 2024 18:38:21 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Z85XeQzg\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1729442301;\n\tbh=t/I+Lyjgls2orZ5oJgVjKMc9czq/I8J52/cbF/pKOGw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Z85XeQzgHPHjxQ3mdisCArAHbgCTgwjVUF+eC8nCHQ+FDmFU81EHFge81HcQzU+VC\n\tCsTBEdyMmcE/r0+fqk5b2zCUkOW1ZUn3jv5jL8wwjoQNmL3wyetoYUcBhF8y2K4DW7\n\tnNDUxg9XCGOw7iQGzk2KMMNZ9hne6bLrg9fZMXZc=","Date":"Sun, 20 Oct 2024 19:40:00 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Harvey Yang ,\n\tlibcamera-devel@lists.libcamera.org,\n\tHan-Lin Chen ","Subject":"Re: [PATCH 1/4] libcamera: Add signal disconnected for IPC","Message-ID":"<20241020164000.GF7770@pendragon.ideasonboard.com>","References":"<20241018075942.1150378-1-chenghaoyang@chromium.org>\n\t<20241018075942.1150378-2-chenghaoyang@chromium.org>\n\t<172937784724.2485972.2782330253351017591@ping.linuxembedded.co.uk>\n\t<172942024566.31090.11990963289361470@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<172942024566.31090.11990963289361470@ping.linuxembedded.co.uk>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31850,"web_url":"https://patchwork.libcamera.org/comment/31850/","msgid":"","date":"2024-10-21T12:11:16","subject":"Re: [PATCH 1/4] libcamera: Add signal disconnected for IPC","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Hi Kieran and Laurent,\n\nI'll upload a new version when we have a consensus on the signal.\n\nOn Mon, Oct 21, 2024 at 12:40 AM Laurent Pinchart\n wrote:\n>\n> On Sun, Oct 20, 2024 at 11:30:45AM +0100, Kieran Bingham wrote:\n> > Quoting Kieran Bingham (2024-10-19 23:44:07)\n> > > Quoting Harvey Yang (2024-10-18 08:57:34)\n> > > > This CL also adds an API in Camera::Private to trigger\n> > > > Camera::disconnected signal.\n> > >\n> > > CL?\n>\n> Change List, the gerrit term for a patch.\n\nRight, sorry. Updated.\n\n>\n> > > Please read: https://cbea.ms/git-commit/#why-not-how\n>\n> +1\n>\n> > >\n> > > But adding a signal on this sounds like a good idea to me.\n> > >\n> > > How does the disconnected signal work. Is it reported when the IPA is\n> > > closed? (or crashes?)\n\nYes, the signal is added exactly for this reason.\n\n> >\n> > Sorry - somehow I dropped a sentence here:\n> >\n> > > or is there some reason for the IP\n> >\n> > ... or is there some other reason for the IPC to be disconnected ?\n> >\n> > > > Signed-off-by: Harvey Yang \n> > > > Co-developed-by: Han-Lin Chen \n> > > > Signed-off-by: Han-Lin Chen \n> > > > ---\n> > > > include/libcamera/internal/camera.h | 2 ++\n> > > > .../libcamera/internal/ipc_pipe_unixsocket.h | 2 ++\n> > > > include/libcamera/internal/ipc_unixsocket.h | 2 ++\n> > > > src/libcamera/camera.cpp | 10 ++++++++\n> > > > src/libcamera/ipc_pipe_unixsocket.cpp | 8 +++++++\n> > > > src/libcamera/ipc_unixsocket.cpp | 24 +++++++++++++++++--\n> > > > .../module_ipa_proxy.cpp.tmpl | 8 +++++++\n> > > > .../module_ipa_proxy.h.tmpl | 2 ++\n> > > > 8 files changed, 56 insertions(+), 2 deletions(-)\n> > > >\n> > > > diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h\n> > > > index 0add0428b..0bef0980e 100644\n> > > > --- a/include/libcamera/internal/camera.h\n> > > > +++ b/include/libcamera/internal/camera.h\n> > > > @@ -33,6 +33,8 @@ public:\n> > > >\n> > > > PipelineHandler *pipe() { return pipe_.get(); }\n> > > >\n> > > > + void notifyDisconnection();\n> > > > +\n> > > > std::list queuedRequests_;\n> > > > ControlInfoMap controlInfo_;\n> > > > ControlList properties_;\n> > > > diff --git a/include/libcamera/internal/ipc_pipe_unixsocket.h b/include/libcamera/internal/ipc_pipe_unixsocket.h\n> > > > index 8c972613f..09f65b102 100644\n> > > > --- a/include/libcamera/internal/ipc_pipe_unixsocket.h\n> > > > +++ b/include/libcamera/internal/ipc_pipe_unixsocket.h\n> > > > @@ -28,6 +28,8 @@ public:\n> > > >\n> > > > int sendAsync(const IPCMessage &data) override;\n> > > >\n> > > > + Signal<> *disconnected();\n> > > > +\n> > > > private:\n> > > > struct CallData {\n> > > > IPCUnixSocket::Payload *response;\n> > > > diff --git a/include/libcamera/internal/ipc_unixsocket.h b/include/libcamera/internal/ipc_unixsocket.h\n> > > > index 48bb7a942..a1e326b6b 100644\n> > > > --- a/include/libcamera/internal/ipc_unixsocket.h\n> > > > +++ b/include/libcamera/internal/ipc_unixsocket.h\n> > > > @@ -39,6 +39,8 @@ public:\n> > > >\n> > > > Signal<> readyRead;\n> > > >\n> > > > + Signal<> disconnected;\n> > > > +\n> > > > private:\n> > > > struct Header {\n> > > > uint32_t data;\n> > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > > index a86f552a4..ef5a6725f 100644\n> > > > --- a/src/libcamera/camera.cpp\n> > > > +++ b/src/libcamera/camera.cpp\n> > > > @@ -603,6 +603,16 @@ Camera::Private::~Private()\n> > > > * \\return The pipeline handler that created this camera\n> > > > */\n> > > >\n> > > > +/**\n> > > > + * \\brief Notify the application that camera is disconnected with signal\n> > > > + * Camera::disconnected\n>\n> That seems to be a big shortcut. IPC disconnection doesn't mean the\n> camera has been disconnected. What will happen if the camera then really\n> becomes disconnected ? Won't the disconnect signal be emitted a second\n> time, confusing applications ? And how is the pipeline handler expected\n> to gracefully shut things down ? I think there's still some design\n> homework to be done here.\n\nYou're right. I think this is how CrOS camera service thinks when the IPA\nprocess is terminated. There's a test that expects to receive\nCAMERA3_MSG_ERROR_REQUEST when the IPA process crashes [1].\n\nI understand that this is not how libcamera takes an IPA disconnection.\nDo you think we need another signal for this case?\nWe might want to discuss how an IPA proxy is bound to a camera as well,\nas some pipeline handlers might use a single IPA proxy to process all\ncameras.\n\n[1]: https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5467266\n\n>\n> I would also like to understand under which circumstances you've seen\n> this happening.\n>\n> > > > + */\n> > > > +void Camera::Private::notifyDisconnection()\n> > > > +{\n> > > > + Camera *o = LIBCAMERA_O_PTR();\n> > > > + o->disconnected.emit();\n> > > > +}\n> > > > +\n> > > > /**\n> > > > * \\fn Camera::Private::validator()\n> > > > * \\brief Retrieve the control validator related to this camera\n> > > > diff --git a/src/libcamera/ipc_pipe_unixsocket.cpp b/src/libcamera/ipc_pipe_unixsocket.cpp\n> > > > index 668ec73b9..51fd3b1fb 100644\n> > > > --- a/src/libcamera/ipc_pipe_unixsocket.cpp\n> > > > +++ b/src/libcamera/ipc_pipe_unixsocket.cpp\n> > > > @@ -84,6 +84,14 @@ int IPCPipeUnixSocket::sendAsync(const IPCMessage &data)\n> > > > return 0;\n> > > > }\n> > > >\n> > > > +Signal<> *IPCPipeUnixSocket::disconnected()\n> > > > +{\n> > > > + if (socket_)\n> > > > + return &socket_->disconnected;\n> > > > +\n> > > > + return nullptr;\n> > > > +}\n> > > > +\n> > > > void IPCPipeUnixSocket::readyRead()\n> > > > {\n> > > > IPCUnixSocket::Payload payload;\n> > > > diff --git a/src/libcamera/ipc_unixsocket.cpp b/src/libcamera/ipc_unixsocket.cpp\n> > > > index 002053e35..3d0248857 100644\n> > > > --- a/src/libcamera/ipc_unixsocket.cpp\n> > > > +++ b/src/libcamera/ipc_unixsocket.cpp\n> > > > @@ -186,11 +186,16 @@ int IPCUnixSocket::send(const Payload &payload)\n> > > > if (!hdr.data && !hdr.fds)\n> > > > return -EINVAL;\n> > > >\n> > > > - ret = ::send(fd_.get(), &hdr, sizeof(hdr), 0);\n> > > > + ret = ::send(fd_.get(), &hdr, sizeof(hdr), MSG_NOSIGNAL);\n> > >\n> > > What's this flag change for? Can you explain in the commit message or as\n> > > a comment please?\n>\n> The MSG_NOSIGNAL documentation states\n>\n> Don’t generate a SIGPIPE signal if the peer on a stream-oriented\n> socket has closed the connection. The EPIPE error is still\n> returned. This provides similar behavior to using sigaction(2) to\n> ignore SIGPIPE, but, whereas MSG_NOSIGNAL is a per-call feature,\n> ignoring SIGPIPE sets a process attribute that affects all threads\n> in the process.\n>\n> but here you're checking ECONNRESET. The documentation of ECONNRESET and\n> EPIPE for this function respectively state\n>\n> ECONNRESET\n> Connection reset by peer.\n>\n> EPIPE\n> The local end has been shut down on a connection oriented\n> socket. In this case, the process will also receive a SIGPIPE\n> unless MSG_NOSIGNAL is set.\n>\n> Do we need MSG_NOSIGNAL to properly handle ECONNRESET ? Do we need to\n> handle EPIPE ? In any case, all this needs to be explained in the commit\n> message.\n\nRight, we probably don't need MSG_NOSIGNAL. Removed.\n\nI'll also detect both ECONNRESET and EPIPE as a disconnection in the\nnext version. Updated the commit message.\nIn my trials though, it's always ECONNRESET. I'm not sure how the current\ncode can trigger an EPIPE.\n\n>\n> > >\n> > > > if (ret < 0) {\n> > > > ret = -errno;\n> > > > LOG(IPCUnixSocket, Error)\n> > > > << \"Failed to send: \" << strerror(-ret);\n> > > > + if (errno == ECONNRESET) {\n>\n> errno may have been modified by the LOG statement. You need to use ret\n> here. Same below.\n\nGot it. Updated.\n\n>\n> > > > + disconnected.emit();\n> > > > + fd_.reset();\n>\n> Why fd_.reset() here and below ? The commit message really misses lots\n> of information.\n\nYeah we probably don't need to prevent the following messages.\nTested on mtkisp7 and it works.\n\n>\n> > >\n> > > So this is really about if the IPA connection gets severed indeed?\n\nYes, it is.\n\nBR,\nHarvey\n\n> > >\n> > > > + }\n> > > > +\n> > > > return ret;\n> > > > }\n> > > >\n> > > > @@ -241,6 +246,11 @@ int IPCUnixSocket::receive(Payload *payload)\n> > > > * \\brief A Signal emitted when a message is ready to be read\n> > > > */\n> > > >\n> > > > +/**\n> > > > + * \\var IPCUnixSocket::disconnected\n> > > > + * \\brief A Signal emitted when the Unix socket IPC is disconnected\n> > > > + */\n> > > > +\n> > > > int IPCUnixSocket::sendData(const void *buffer, size_t length,\n> > > > const int32_t *fds, unsigned int num)\n> > > > {\n> > > > @@ -266,10 +276,15 @@ int IPCUnixSocket::sendData(const void *buffer, size_t length,\n> > > > if (fds)\n> > > > memcpy(CMSG_DATA(cmsg), fds, num * sizeof(uint32_t));\n> > > >\n> > > > - if (sendmsg(fd_.get(), &msg, 0) < 0) {\n> > > > + if (sendmsg(fd_.get(), &msg, MSG_NOSIGNAL) < 0) {\n> > > > int ret = -errno;\n> > > > LOG(IPCUnixSocket, Error)\n> > > > << \"Failed to sendmsg: \" << strerror(-ret);\n> > > > + if (errno == ECONNRESET) {\n> > > > + disconnected.emit();\n> > > > + fd_.reset();\n> > > > + }\n> > > > +\n> > > > return ret;\n> > > > }\n> > > >\n> > > > @@ -324,6 +339,11 @@ void IPCUnixSocket::dataNotifier()\n> > > > ret = -errno;\n> > > > LOG(IPCUnixSocket, Error)\n> > > > << \"Failed to receive header: \" << strerror(-ret);\n> > > > + if (errno == ECONNRESET) {\n> > > > + disconnected.emit();\n> > > > + fd_.reset();\n> > > > + }\n> > > > +\n> > > > return;\n> > > > }\n> > > >\n> > > > diff --git a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> > > > index ce3cc5ab6..27f03417a 100644\n> > > > --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> > > > +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> > > > @@ -104,6 +104,14 @@ namespace {{ns}} {\n> > > > }\n> > > > }\n> > > >\n> > > > +Signal<> *{{proxy_name}}::disconnected()\n> > > > +{\n> > > > + if (ipc_)\n> > > > + return ipc_->disconnected();\n> > > > +\n> > > > + return nullptr;\n> > > > +}\n> > > > +\n> > > > {% if interface_event.methods|length > 0 %}\n> > > > void {{proxy_name}}::recvMessage(const IPCMessage &data)\n> > > > {\n> > > > diff --git a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> > > > index e213b18a0..2b7ba872e 100644\n> > > > --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> > > > +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> > > > @@ -53,6 +53,8 @@ public:\n> > > > > {{method.mojom_name}};\n> > > > {% endfor %}\n> > > >\n> > > > + Signal<> *disconnected();\n> > > > +\n> > > > private:\n> > > > void recvMessage(const IPCMessage &data);\n> > > >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 0E630C32A3\n\tfor ;\n\tMon, 21 Oct 2024 12:11:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D933B65392;\n\tMon, 21 Oct 2024 14:11:30 +0200 (CEST)","from mail-lj1-x235.google.com (mail-lj1-x235.google.com\n\t[IPv6:2a00:1450:4864:20::235])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E4FA86538A\n\tfor ;\n\tMon, 21 Oct 2024 14:11:28 +0200 (CEST)","by mail-lj1-x235.google.com with SMTP id\n\t38308e7fff4ca-2fb5fa911aaso64379081fa.2\n\tfor ;\n\tMon, 21 Oct 2024 05:11:28 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"Nl++eIkX\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1729512688; x=1730117488;\n\tdarn=lists.libcamera.org; \n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=AnlnDUGte6FSaAuMaw9QTztTZ540qIiKc6s4MZsaJwI=;\n\tb=Nl++eIkXljKp3ScPCbXMAPrd3W3XOdU8l8LqlsN2xUMT34bOkbjyDZgdguuWcsenjq\n\tWVJK20A4aCE5Ji7qjF7fTkCUzcCpTC+JKQy3efV+QHCSAOblJKWPqkgrlR/SNK30tJR0\n\tn4KOf3CkRG1QKGL3wMuUw0GTv0SMNZM7ObBls=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1729512688; x=1730117488;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=AnlnDUGte6FSaAuMaw9QTztTZ540qIiKc6s4MZsaJwI=;\n\tb=Py1FR/RasVaJEYuIwDFstFaeTPFxwJwp06xcSHFOkxyTAHLQc9Sku7hc2yA4sHHT80\n\t5ioJPuvgVIbO8ZN3nxHlVmO9NCg8F69GjEYVJVahVyFmbMlj8VTOMJDMBhBkX+P+ltjB\n\tDX6oQ/ZWRvg0Aem26KE0QeaTK5JtkyCjNYwdgVZRaQ8DEQC8fo9bvCjIBnjH01PLL21E\n\tCDPUpaYmPxLcPtndjtR5Pd1RI/ub0Gt5lqi/Mh8FZt86uSUvlJRJfUh9OA7b3b11LziE\n\tmPyqjnw+/Ur3cwbrS5//qTwGiXMuLkUTnDujNK/fXpRtbG3uojn9EV5u9QoZcV8T0ydX\n\t4bPA==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCUZ3Ceby+rOsH6PKCXayHeCkmvyXaYaXJO6+wtWQ6t7xS+R0au3kbuHZCWHfTW8HaZVPm5Chzq1DMXcjyJlyww=@lists.libcamera.org","X-Gm-Message-State":"AOJu0YzRRbB+1ojYU3l4Q8GRFwWCoMiTZlNT7BmIFil7/QdVXOR0azN/\n\tgvmXMoSA5p/cVUgHHzOc+F5+RbBfyQo6Hxf65eDfTqjXKRHXcM4zGHc+bhpmxUvo84EgPUWgpho\n\tO45uWT30d6x7GLZao2/sw2f3QDqrw5eXOcVLZ","X-Google-Smtp-Source":"AGHT+IE4tZPQmBkAA9v1k2NbVljz3gyTWMBKaWZMe6e2M0NSqsu90bUdS38F7T0qv67dKRwLjI9Vn1W+GCJVxFrliYw=","X-Received":"by 2002:a05:651c:2211:b0:2fb:5206:1675 with SMTP id\n\t38308e7fff4ca-2fb82fb099cmr79395161fa.27.1729512687883;\n\tMon, 21 Oct 2024 05:11:27 -0700 (PDT)","MIME-Version":"1.0","References":"<20241018075942.1150378-1-chenghaoyang@chromium.org>\n\t<20241018075942.1150378-2-chenghaoyang@chromium.org>\n\t<172937784724.2485972.2782330253351017591@ping.linuxembedded.co.uk>\n\t<172942024566.31090.11990963289361470@ping.linuxembedded.co.uk>\n\t<20241020164000.GF7770@pendragon.ideasonboard.com>","In-Reply-To":"<20241020164000.GF7770@pendragon.ideasonboard.com>","From":"Cheng-Hao Yang ","Date":"Mon, 21 Oct 2024 20:11:16 +0800","Message-ID":"","Subject":"Re: [PATCH 1/4] libcamera: Add signal disconnected for IPC","To":"Laurent Pinchart ","Cc":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org, \n\tHan-Lin Chen ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]