[PATCH 1/4] libcamera: Add signal disconnected for IPC
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sun Oct 20 18:40:00 CEST 2024
On Sun, Oct 20, 2024 at 11:30:45AM +0100, Kieran Bingham wrote:
> Quoting Kieran Bingham (2024-10-19 23:44:07)
> > Quoting Harvey Yang (2024-10-18 08:57:34)
> > > This CL also adds an API in Camera::Private to trigger
> > > Camera::disconnected signal.
> >
> > CL?
Change List, the gerrit term for a patch.
> > Please read: https://cbea.ms/git-commit/#why-not-how
+1
> >
> > But adding a signal on this sounds like a good idea to me.
> >
> > How does the disconnected signal work. Is it reported when the IPA is
> > closed? (or crashes?)
>
> Sorry - somehow I dropped a sentence here:
>
> > or is there some reason for the IP
>
> ... or is there some other reason for the IPC to be disconnected ?
>
> > > Signed-off-by: Harvey Yang <chenghaoyang at chromium.org>
> > > Co-developed-by: Han-Lin Chen <hanlinchen at chromium.org>
> > > Signed-off-by: Han-Lin Chen <hanlinchen at chromium.org>
> > > ---
> > > include/libcamera/internal/camera.h | 2 ++
> > > .../libcamera/internal/ipc_pipe_unixsocket.h | 2 ++
> > > include/libcamera/internal/ipc_unixsocket.h | 2 ++
> > > src/libcamera/camera.cpp | 10 ++++++++
> > > src/libcamera/ipc_pipe_unixsocket.cpp | 8 +++++++
> > > src/libcamera/ipc_unixsocket.cpp | 24 +++++++++++++++++--
> > > .../module_ipa_proxy.cpp.tmpl | 8 +++++++
> > > .../module_ipa_proxy.h.tmpl | 2 ++
> > > 8 files changed, 56 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h
> > > index 0add0428b..0bef0980e 100644
> > > --- a/include/libcamera/internal/camera.h
> > > +++ b/include/libcamera/internal/camera.h
> > > @@ -33,6 +33,8 @@ public:
> > >
> > > PipelineHandler *pipe() { return pipe_.get(); }
> > >
> > > + void notifyDisconnection();
> > > +
> > > std::list<Request *> queuedRequests_;
> > > ControlInfoMap controlInfo_;
> > > ControlList properties_;
> > > diff --git a/include/libcamera/internal/ipc_pipe_unixsocket.h b/include/libcamera/internal/ipc_pipe_unixsocket.h
> > > index 8c972613f..09f65b102 100644
> > > --- a/include/libcamera/internal/ipc_pipe_unixsocket.h
> > > +++ b/include/libcamera/internal/ipc_pipe_unixsocket.h
> > > @@ -28,6 +28,8 @@ public:
> > >
> > > int sendAsync(const IPCMessage &data) override;
> > >
> > > + Signal<> *disconnected();
> > > +
> > > private:
> > > struct CallData {
> > > IPCUnixSocket::Payload *response;
> > > diff --git a/include/libcamera/internal/ipc_unixsocket.h b/include/libcamera/internal/ipc_unixsocket.h
> > > index 48bb7a942..a1e326b6b 100644
> > > --- a/include/libcamera/internal/ipc_unixsocket.h
> > > +++ b/include/libcamera/internal/ipc_unixsocket.h
> > > @@ -39,6 +39,8 @@ public:
> > >
> > > Signal<> readyRead;
> > >
> > > + Signal<> disconnected;
> > > +
> > > private:
> > > struct Header {
> > > uint32_t data;
> > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > > index a86f552a4..ef5a6725f 100644
> > > --- a/src/libcamera/camera.cpp
> > > +++ b/src/libcamera/camera.cpp
> > > @@ -603,6 +603,16 @@ Camera::Private::~Private()
> > > * \return The pipeline handler that created this camera
> > > */
> > >
> > > +/**
> > > + * \brief Notify the application that camera is disconnected with signal
> > > + * Camera::disconnected
That seems to be a big shortcut. IPC disconnection doesn't mean the
camera has been disconnected. What will happen if the camera then really
becomes disconnected ? Won't the disconnect signal be emitted a second
time, confusing applications ? And how is the pipeline handler expected
to gracefully shut things down ? I think there's still some design
homework to be done here.
I would also like to understand under which circumstances you've seen
this happening.
> > > + */
> > > +void Camera::Private::notifyDisconnection()
> > > +{
> > > + Camera *o = LIBCAMERA_O_PTR();
> > > + o->disconnected.emit();
> > > +}
> > > +
> > > /**
> > > * \fn Camera::Private::validator()
> > > * \brief Retrieve the control validator related to this camera
> > > diff --git a/src/libcamera/ipc_pipe_unixsocket.cpp b/src/libcamera/ipc_pipe_unixsocket.cpp
> > > index 668ec73b9..51fd3b1fb 100644
> > > --- a/src/libcamera/ipc_pipe_unixsocket.cpp
> > > +++ b/src/libcamera/ipc_pipe_unixsocket.cpp
> > > @@ -84,6 +84,14 @@ int IPCPipeUnixSocket::sendAsync(const IPCMessage &data)
> > > return 0;
> > > }
> > >
> > > +Signal<> *IPCPipeUnixSocket::disconnected()
> > > +{
> > > + if (socket_)
> > > + return &socket_->disconnected;
> > > +
> > > + return nullptr;
> > > +}
> > > +
> > > void IPCPipeUnixSocket::readyRead()
> > > {
> > > IPCUnixSocket::Payload payload;
> > > diff --git a/src/libcamera/ipc_unixsocket.cpp b/src/libcamera/ipc_unixsocket.cpp
> > > index 002053e35..3d0248857 100644
> > > --- a/src/libcamera/ipc_unixsocket.cpp
> > > +++ b/src/libcamera/ipc_unixsocket.cpp
> > > @@ -186,11 +186,16 @@ int IPCUnixSocket::send(const Payload &payload)
> > > if (!hdr.data && !hdr.fds)
> > > return -EINVAL;
> > >
> > > - ret = ::send(fd_.get(), &hdr, sizeof(hdr), 0);
> > > + ret = ::send(fd_.get(), &hdr, sizeof(hdr), MSG_NOSIGNAL);
> >
> > What's this flag change for? Can you explain in the commit message or as
> > a comment please?
The MSG_NOSIGNAL documentation states
Don’t generate a SIGPIPE signal if the peer on a stream-oriented
socket has closed the connection. The EPIPE error is still
returned. This provides similar behavior to using sigaction(2) to
ignore SIGPIPE, but, whereas MSG_NOSIGNAL is a per-call feature,
ignoring SIGPIPE sets a process attribute that affects all threads
in the process.
but here you're checking ECONNRESET. The documentation of ECONNRESET and
EPIPE for this function respectively state
ECONNRESET
Connection reset by peer.
EPIPE
The local end has been shut down on a connection oriented
socket. In this case, the process will also receive a SIGPIPE
unless MSG_NOSIGNAL is set.
Do we need MSG_NOSIGNAL to properly handle ECONNRESET ? Do we need to
handle EPIPE ? In any case, all this needs to be explained in the commit
message.
> >
> > > if (ret < 0) {
> > > ret = -errno;
> > > LOG(IPCUnixSocket, Error)
> > > << "Failed to send: " << strerror(-ret);
> > > + if (errno == ECONNRESET) {
errno may have been modified by the LOG statement. You need to use ret
here. Same below.
> > > + disconnected.emit();
> > > + fd_.reset();
Why fd_.reset() here and below ? The commit message really misses lots
of information.
> >
> > So this is really about if the IPA connection gets severed indeed?
> >
> > > + }
> > > +
> > > return ret;
> > > }
> > >
> > > @@ -241,6 +246,11 @@ int IPCUnixSocket::receive(Payload *payload)
> > > * \brief A Signal emitted when a message is ready to be read
> > > */
> > >
> > > +/**
> > > + * \var IPCUnixSocket::disconnected
> > > + * \brief A Signal emitted when the Unix socket IPC is disconnected
> > > + */
> > > +
> > > int IPCUnixSocket::sendData(const void *buffer, size_t length,
> > > const int32_t *fds, unsigned int num)
> > > {
> > > @@ -266,10 +276,15 @@ int IPCUnixSocket::sendData(const void *buffer, size_t length,
> > > if (fds)
> > > memcpy(CMSG_DATA(cmsg), fds, num * sizeof(uint32_t));
> > >
> > > - if (sendmsg(fd_.get(), &msg, 0) < 0) {
> > > + if (sendmsg(fd_.get(), &msg, MSG_NOSIGNAL) < 0) {
> > > int ret = -errno;
> > > LOG(IPCUnixSocket, Error)
> > > << "Failed to sendmsg: " << strerror(-ret);
> > > + if (errno == ECONNRESET) {
> > > + disconnected.emit();
> > > + fd_.reset();
> > > + }
> > > +
> > > return ret;
> > > }
> > >
> > > @@ -324,6 +339,11 @@ void IPCUnixSocket::dataNotifier()
> > > ret = -errno;
> > > LOG(IPCUnixSocket, Error)
> > > << "Failed to receive header: " << strerror(-ret);
> > > + if (errno == ECONNRESET) {
> > > + disconnected.emit();
> > > + fd_.reset();
> > > + }
> > > +
> > > return;
> > > }
> > >
> > > 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
> > > index ce3cc5ab6..27f03417a 100644
> > > --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> > > +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> > > @@ -104,6 +104,14 @@ namespace {{ns}} {
> > > }
> > > }
> > >
> > > +Signal<> *{{proxy_name}}::disconnected()
> > > +{
> > > + if (ipc_)
> > > + return ipc_->disconnected();
> > > +
> > > + return nullptr;
> > > +}
> > > +
> > > {% if interface_event.methods|length > 0 %}
> > > void {{proxy_name}}::recvMessage(const IPCMessage &data)
> > > {
> > > 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
> > > index e213b18a0..2b7ba872e 100644
> > > --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl
> > > +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl
> > > @@ -53,6 +53,8 @@ public:
> > > > {{method.mojom_name}};
> > > {% endfor %}
> > >
> > > + Signal<> *disconnected();
> > > +
> > > private:
> > > void recvMessage(const IPCMessage &data);
> > >
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list