[{"id":18941,"web_url":"https://patchwork.libcamera.org/comment/18941/","msgid":"<8ba43326-4c49-cbfe-35bf-3b438dd31a32@ideasonboard.com>","date":"2021-08-19T07:28:50","subject":"Re: [libcamera-devel] [PATCH v4] ipa: Use FileDescriptor instead of\n\tint in layers above IPC payload","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Paul,\n\nOn 8/19/21 12:49 PM, Paul Elder wrote:\n> Regarding (de)serialization in isolated IPA calls, we have four layers:\n> - struct\n> - byte vector + fd vector\n> - IPCMessage\n> - IPC payload\n>\n> The proxy handles the upper three layers (with help from the\n> IPADataSerializer), and passes an IPCMessage to the IPC mechanism\n> (implemented as an IPCPipe), which sends an IPC payload to its worker\n> counterpart.\n>\n> When a FileDescriptor is involved, previously it was only a\n> FileDescriptor in the first layer; in the lower three it was an int. To\n> reduce the risk of potential fd leaks in the future, keep the\n> FileDescriptor as-is throughout the upper three layers. Only the IPC\n> mechanism will deal with ints, if it so wishes, when it does the actual\n> IPC. IPCPipeUnixSocket does deal with ints for sending fds, so the\n> conversion between IPCMessage and IPCUnixSocket::Payload converts\n> between FileDescriptor and int.\n>\n> Additionally, change the data portion of the serialized form of\n> FileDescriptor to a 32-bit unsigned integer, for alightnment purposes\n> and in preparation for conversion to an index into the fd array.\n>\n> Also update the deserializer of FrameBuffer::Plane accordingly.\n>\n> Signed-off-by: Paul Elder \n> Tested-by: Umang Jain \n> Reviewed-by: Laurent Pinchart \n> Reviewed-by: Umang Jain \n>\n> ---\n> Changes in v4:\n> - update deserializer for FrameBuffer::Plane\n> ---\n\n\nv4 tested again with fd-leak test. No issues observed.\n\n\n> .../libcamera/internal/ipa_data_serializer.h | 40 ++++----\n> include/libcamera/internal/ipc_pipe.h | 10 +-\n> src/libcamera/ipa_data_serializer.cpp | 95 ++++++++++---------\n> src/libcamera/ipc_pipe.cpp | 12 ++-\n> .../ipa_data_serializer_test.cpp | 12 +--\n> .../module_ipa_proxy.cpp.tmpl | 2 +-\n> .../module_ipa_proxy.h.tmpl | 2 +-\n> .../libcamera_templates/proxy_functions.tmpl | 2 +-\n> .../libcamera_templates/serializer.tmpl | 26 ++---\n> 9 files changed, 108 insertions(+), 93 deletions(-)\n>\n> diff --git a/include/libcamera/internal/ipa_data_serializer.h b/include/libcamera/internal/ipa_data_serializer.h\n> index 519093bd..4353e07b 100644\n> --- a/include/libcamera/internal/ipa_data_serializer.h\n> +++ b/include/libcamera/internal/ipa_data_serializer.h\n> @@ -66,7 +66,7 @@ template\n> class IPADataSerializer\n> {\n> public:\n> -\tstatic std::tuple, std::vector>\n> +\tstatic std::tuple, std::vector>\n> \tserialize(const T &data, ControlSerializer *cs = nullptr);\n> \n> \tstatic T deserialize(const std::vector &data,\n> @@ -76,12 +76,12 @@ public:\n> \t\t\t ControlSerializer *cs = nullptr);\n> \n> \tstatic T deserialize(const std::vector &data,\n> -\t\t\t const std::vector &fds,\n> +\t\t\t const std::vector &fds,\n> \t\t\t ControlSerializer *cs = nullptr);\n> \tstatic T deserialize(std::vector::const_iterator dataBegin,\n> \t\t\t std::vector::const_iterator dataEnd,\n> -\t\t\t std::vector::const_iterator fdsBegin,\n> -\t\t\t std::vector::const_iterator fdsEnd,\n> +\t\t\t std::vector::const_iterator fdsBegin,\n> +\t\t\t std::vector::const_iterator fdsEnd,\n> \t\t\t ControlSerializer *cs = nullptr);\n> };\n> \n> @@ -104,11 +104,11 @@ template\n> class IPADataSerializer>\n> {\n> public:\n> -\tstatic std::tuple, std::vector>\n> +\tstatic std::tuple, std::vector>\n> \tserialize(const std::vector &data, ControlSerializer *cs = nullptr)\n> \t{\n> \t\tstd::vector dataVec;\n> -\t\tstd::vector fdsVec;\n> +\t\tstd::vector fdsVec;\n> \n> \t\t/* Serialize the length. */\n> \t\tuint32_t vecLen = data.size();\n> @@ -117,7 +117,7 @@ public:\n> \t\t/* Serialize the members. */\n> \t\tfor (auto const &it : data) {\n> \t\t\tstd::vector dvec;\n> -\t\t\tstd::vector fvec;\n> +\t\t\tstd::vector fvec;\n> \n> \t\t\tstd::tie(dvec, fvec) =\n> \t\t\t\tIPADataSerializer::serialize(it, cs);\n> @@ -141,11 +141,11 @@ public:\n> \t\t\t\t\t std::vector::const_iterator dataEnd,\n> \t\t\t\t\t ControlSerializer *cs = nullptr)\n> \t{\n> -\t\tstd::vector fds;\n> +\t\tstd::vector fds;\n> \t\treturn deserialize(dataBegin, dataEnd, fds.cbegin(), fds.end(), cs);\n> \t}\n> \n> -\tstatic std::vector deserialize(std::vector &data, std::vector &fds,\n> +\tstatic std::vector deserialize(std::vector &data, std::vector &fds,\n> \t\t\t\t\t ControlSerializer *cs = nullptr)\n> \t{\n> \t\treturn deserialize(data.cbegin(), data.end(), fds.cbegin(), fds.end(), cs);\n> @@ -153,15 +153,15 @@ public:\n> \n> \tstatic std::vector deserialize(std::vector::const_iterator dataBegin,\n> \t\t\t\t\t std::vector::const_iterator dataEnd,\n> -\t\t\t\t\t std::vector::const_iterator fdsBegin,\n> -\t\t\t\t\t [[maybe_unused]] std::vector::const_iterator fdsEnd,\n> +\t\t\t\t\t std::vector::const_iterator fdsBegin,\n> +\t\t\t\t\t [[maybe_unused]] std::vector::const_iterator fdsEnd,\n> \t\t\t\t\t ControlSerializer *cs = nullptr)\n> \t{\n> \t\tuint32_t vecLen = readPOD(dataBegin, 0, dataEnd);\n> \t\tstd::vector ret(vecLen);\n> \n> \t\tstd::vector::const_iterator dataIter = dataBegin + 4;\n> -\t\tstd::vector::const_iterator fdIter = fdsBegin;\n> +\t\tstd::vector::const_iterator fdIter = fdsBegin;\n> \t\tfor (uint32_t i = 0; i < vecLen; i++) {\n> \t\t\tuint32_t sizeofData = readPOD(dataIter, 0, dataEnd);\n> \t\t\tuint32_t sizeofFds = readPOD(dataIter, 4, dataEnd);\n> @@ -201,11 +201,11 @@ template\n> class IPADataSerializer>\n> {\n> public:\n> -\tstatic std::tuple, std::vector>\n> +\tstatic std::tuple, std::vector>\n> \tserialize(const std::map &data, ControlSerializer *cs = nullptr)\n> \t{\n> \t\tstd::vector dataVec;\n> -\t\tstd::vector fdsVec;\n> +\t\tstd::vector fdsVec;\n> \n> \t\t/* Serialize the length. */\n> \t\tuint32_t mapLen = data.size();\n> @@ -214,7 +214,7 @@ public:\n> \t\t/* Serialize the members. */\n> \t\tfor (auto const &it : data) {\n> \t\t\tstd::vector dvec;\n> -\t\t\tstd::vector fvec;\n> +\t\t\tstd::vector fvec;\n> \n> \t\t\tstd::tie(dvec, fvec) =\n> \t\t\t\tIPADataSerializer::serialize(it.first, cs);\n> @@ -247,11 +247,11 @@ public:\n> \t\t\t\t\t std::vector::const_iterator dataEnd,\n> \t\t\t\t\t ControlSerializer *cs = nullptr)\n> \t{\n> -\t\tstd::vector fds;\n> +\t\tstd::vector fds;\n> \t\treturn deserialize(dataBegin, dataEnd, fds.cbegin(), fds.end(), cs);\n> \t}\n> \n> -\tstatic std::map deserialize(std::vector &data, std::vector &fds,\n> +\tstatic std::map deserialize(std::vector &data, std::vector &fds,\n> \t\t\t\t\t ControlSerializer *cs = nullptr)\n> \t{\n> \t\treturn deserialize(data.cbegin(), data.end(), fds.cbegin(), fds.end(), cs);\n> @@ -259,8 +259,8 @@ public:\n> \n> \tstatic std::map deserialize(std::vector::const_iterator dataBegin,\n> \t\t\t\t\t std::vector::const_iterator dataEnd,\n> -\t\t\t\t\t std::vector::const_iterator fdsBegin,\n> -\t\t\t\t\t [[maybe_unused]] std::vector::const_iterator fdsEnd,\n> +\t\t\t\t\t std::vector::const_iterator fdsBegin,\n> +\t\t\t\t\t [[maybe_unused]] std::vector::const_iterator fdsEnd,\n> \t\t\t\t\t ControlSerializer *cs = nullptr)\n> \t{\n> \t\tstd::map ret;\n> @@ -268,7 +268,7 @@ public:\n> \t\tuint32_t mapLen = readPOD(dataBegin, 0, dataEnd);\n> \n> \t\tstd::vector::const_iterator dataIter = dataBegin + 4;\n> -\t\tstd::vector::const_iterator fdIter = fdsBegin;\n> +\t\tstd::vector::const_iterator fdIter = fdsBegin;\n> \t\tfor (uint32_t i = 0; i < mapLen; i++) {\n> \t\t\tuint32_t sizeofData = readPOD(dataIter, 0, dataEnd);\n> \t\t\tuint32_t sizeofFds = readPOD(dataIter, 4, dataEnd);\n> diff --git a/include/libcamera/internal/ipc_pipe.h b/include/libcamera/internal/ipc_pipe.h\n> index e58de340..52cc8fa3 100644\n> --- a/include/libcamera/internal/ipc_pipe.h\n> +++ b/include/libcamera/internal/ipc_pipe.h\n> @@ -11,6 +11,8 @@\n> \n> #include \n> \n> +#include \n> +\n> #include \"libcamera/internal/ipc_unixsocket.h\"\n> \n> namespace libcamera {\n> @@ -26,23 +28,23 @@ public:\n> \tIPCMessage();\n> \tIPCMessage(uint32_t cmd);\n> \tIPCMessage(const Header &header);\n> -\tIPCMessage(const IPCUnixSocket::Payload &payload);\n> +\tIPCMessage(IPCUnixSocket::Payload &payload);\n> \n> \tIPCUnixSocket::Payload payload() const;\n> \n> \tHeader &header() { return header_; }\n> \tstd::vector &data() { return data_; }\n> -\tstd::vector &fds() { return fds_; }\n> +\tstd::vector &fds() { return fds_; }\n> \n> \tconst Header &header() const { return header_; }\n> \tconst std::vector &data() const { return data_; }\n> -\tconst std::vector &fds() const { return fds_; }\n> +\tconst std::vector &fds() const { return fds_; }\n> \n> private:\n> \tHeader header_;\n> \n> \tstd::vector data_;\n> -\tstd::vector fds_;\n> +\tstd::vector fds_;\n> };\n> \n> class IPCPipe\n> diff --git a/src/libcamera/ipa_data_serializer.cpp b/src/libcamera/ipa_data_serializer.cpp\n> index fb941e6b..5b183c70 100644\n> --- a/src/libcamera/ipa_data_serializer.cpp\n> +++ b/src/libcamera/ipa_data_serializer.cpp\n> @@ -7,6 +7,8 @@\n> \n> #include \"libcamera/internal/ipa_data_serializer.h\"\n> \n> +#include \n> +\n> #include \n> \n> /**\n> @@ -141,7 +143,7 @@ namespace {\n> /**\n> * \\fn template IPADataSerializer::deserialize(\n> * \tconst std::vector &data,\n> - * \tconst std::vector &fds,\n> + * \tconst std::vector &fds,\n> * \tControlSerializer *cs = nullptr)\n> * \\brief Deserialize byte vector and fd vector into an object\n> * \\tparam T Type of object to deserialize to\n> @@ -162,8 +164,8 @@ namespace {\n> * \\fn template IPADataSerializer::deserialize(\n> * \tstd::vector::const_iterator dataBegin,\n> * \tstd::vector::const_iterator dataEnd,\n> - * \tstd::vector::const_iterator fdsBegin,\n> - * \tstd::vector::const_iterator fdsEnd,\n> + * \tstd::vector::const_iterator fdsBegin,\n> + * \tstd::vector::const_iterator fdsEnd,\n> * \tControlSerializer *cs = nullptr)\n> * \\brief Deserialize byte vector and fd vector into an object\n> * \\tparam T Type of object to deserialize to\n> @@ -187,7 +189,7 @@ namespace {\n> #define DEFINE_POD_SERIALIZER(type)\t\t\t\t\t\\\n> \t\t\t\t\t\t\t\t\t\\\n> template<>\t\t\t\t\t\t\t\t\\\n> -std::tuple, std::vector>\t\t\t\\\n> +std::tuple, std::vector>\t\t\\\n> IPADataSerializer::serialize(const type &data,\t\t\t\\\n> \t\t\t\t [[maybe_unused]] ControlSerializer *cs) \\\n> {\t\t\t\t\t\t\t\t\t\\\n> @@ -215,7 +217,7 @@ type IPADataSerializer::deserialize(const std::vector &data, \\\n> \t\t\t\t\t\t\t\t\t\\\n> template<>\t\t\t\t\t\t\t\t\\\n> type IPADataSerializer::deserialize(const std::vector &data, \\\n> -\t\t\t\t\t [[maybe_unused]] const std::vector &fds, \\\n> +\t\t\t\t\t [[maybe_unused]] const std::vector &fds, \\\n> \t\t\t\t\t ControlSerializer *cs)\t\\\n> {\t\t\t\t\t\t\t\t\t\\\n> \treturn deserialize(data.cbegin(), data.end(), cs);\t\t\\\n> @@ -224,8 +226,8 @@ type IPADataSerializer::deserialize(const std::vector &data, \\\n> template<>\t\t\t\t\t\t\t\t\\\n> type IPADataSerializer::deserialize(std::vector::const_iterator dataBegin, \\\n> \t\t\t\t\t std::vector::const_iterator dataEnd, \\\n> -\t\t\t\t\t [[maybe_unused]] std::vector::const_iterator fdsBegin, \\\n> -\t\t\t\t\t [[maybe_unused]] std::vector::const_iterator fdsEnd, \\\n> +\t\t\t\t\t [[maybe_unused]] std::vector::const_iterator fdsBegin, \\\n> +\t\t\t\t\t [[maybe_unused]] std::vector::const_iterator fdsEnd, \\\n> \t\t\t\t\t ControlSerializer *cs)\t\\\n> {\t\t\t\t\t\t\t\t\t\\\n> \treturn deserialize(dataBegin, dataEnd, cs);\t\t\t\\\n> @@ -249,7 +251,7 @@ DEFINE_POD_SERIALIZER(double)\n> * function parameter serdes).\n> */\n> template<>\n> -std::tuple, std::vector>\n> +std::tuple, std::vector>\n> IPADataSerializer::serialize(const std::string &data,\n> \t\t\t\t\t [[maybe_unused]] ControlSerializer *cs)\n> {\n> @@ -276,7 +278,7 @@ IPADataSerializer::deserialize(std::vector::const_iterator\n> template<>\n> std::string\n> IPADataSerializer::deserialize(const std::vector &data,\n> -\t\t\t\t\t [[maybe_unused]] const std::vector &fds,\n> +\t\t\t\t\t [[maybe_unused]] const std::vector &fds,\n> \t\t\t\t\t [[maybe_unused]] ControlSerializer *cs)\n> {\n> \treturn { data.cbegin(), data.cend() };\n> @@ -286,8 +288,8 @@ template<>\n> std::string\n> IPADataSerializer::deserialize(std::vector::const_iterator dataBegin,\n> \t\t\t\t\t std::vector::const_iterator dataEnd,\n> -\t\t\t\t\t [[maybe_unused]] std::vector::const_iterator fdsBegin,\n> -\t\t\t\t\t [[maybe_unused]] std::vector::const_iterator fdsEnd,\n> +\t\t\t\t\t [[maybe_unused]] std::vector::const_iterator fdsBegin,\n> +\t\t\t\t\t [[maybe_unused]] std::vector::const_iterator fdsEnd,\n> \t\t\t\t\t [[maybe_unused]] ControlSerializer *cs)\n> {\n> \treturn { dataBegin, dataEnd };\n> @@ -305,7 +307,7 @@ IPADataSerializer::deserialize(std::vector::const_iterator\n> * be used. The serialized ControlInfoMap will have zero length.\n> */\n> template<>\n> -std::tuple, std::vector>\n> +std::tuple, std::vector>\n> IPADataSerializer::serialize(const ControlList &data, ControlSerializer *cs)\n> {\n> \tif (!cs)\n> @@ -405,7 +407,7 @@ IPADataSerializer::deserialize(const std::vector &data,\n> template<>\n> ControlList\n> IPADataSerializer::deserialize(const std::vector &data,\n> -\t\t\t\t\t [[maybe_unused]] const std::vector &fds,\n> +\t\t\t\t\t [[maybe_unused]] const std::vector &fds,\n> \t\t\t\t\t ControlSerializer *cs)\n> {\n> \treturn deserialize(data.cbegin(), data.end(), cs);\n> @@ -415,8 +417,8 @@ template<>\n> ControlList\n> IPADataSerializer::deserialize(std::vector::const_iterator dataBegin,\n> \t\t\t\t\t std::vector::const_iterator dataEnd,\n> -\t\t\t\t\t [[maybe_unused]] std::vector::const_iterator fdsBegin,\n> -\t\t\t\t\t [[maybe_unused]] std::vector::const_iterator fdsEnd,\n> +\t\t\t\t\t [[maybe_unused]] std::vector::const_iterator fdsBegin,\n> +\t\t\t\t\t [[maybe_unused]] std::vector::const_iterator fdsEnd,\n> \t\t\t\t\t ControlSerializer *cs)\n> {\n> \treturn deserialize(dataBegin, dataEnd, cs);\n> @@ -429,7 +431,7 @@ IPADataSerializer::deserialize(std::vector::const_iterator\n> * X bytes - Serialized ControlInfoMap (using ControlSerializer)\n> */\n> template<>\n> -std::tuple, std::vector>\n> +std::tuple, std::vector>\n> IPADataSerializer::serialize(const ControlInfoMap &map,\n> \t\t\t\t\t ControlSerializer *cs)\n> {\n> @@ -491,7 +493,7 @@ IPADataSerializer::deserialize(const std::vector &data,\n> template<>\n> ControlInfoMap\n> IPADataSerializer::deserialize(const std::vector &data,\n> -\t\t\t\t\t [[maybe_unused]] const std::vector &fds,\n> +\t\t\t\t\t [[maybe_unused]] const std::vector &fds,\n> \t\t\t\t\t ControlSerializer *cs)\n> {\n> \treturn deserialize(data.cbegin(), data.end(), cs);\n> @@ -501,15 +503,15 @@ template<>\n> ControlInfoMap\n> IPADataSerializer::deserialize(std::vector::const_iterator dataBegin,\n> \t\t\t\t\t std::vector::const_iterator dataEnd,\n> -\t\t\t\t\t [[maybe_unused]] std::vector::const_iterator fdsBegin,\n> -\t\t\t\t\t [[maybe_unused]] std::vector::const_iterator fdsEnd,\n> +\t\t\t\t\t [[maybe_unused]] std::vector::const_iterator fdsBegin,\n> +\t\t\t\t\t [[maybe_unused]] std::vector::const_iterator fdsEnd,\n> \t\t\t\t\t ControlSerializer *cs)\n> {\n> \treturn deserialize(dataBegin, dataEnd, cs);\n> }\n> \n> /*\n> - * FileDescriptors are serialized into a single byte that tells if the\n> + * FileDescriptors are serialized into four bytes that tells if the\n> * FileDescriptor is valid or not. If it is valid, then for serialization\n> * the fd will be written to the fd vector, or for deserialization the\n> * fd vector const_iterator will be valid.\n> @@ -517,42 +519,47 @@ IPADataSerializer::deserialize(std::vector::const_itera\n> * This validity is necessary so that we don't send -1 fd over sendmsg(). It\n> * also allows us to simply send the entire fd vector into the deserializer\n> * and it will be recursively consumed as necessary.\n> - *\n> - * \\todo Consider serializing the FileDescriptor in 4 bytes to ensure\n> - * 32-bit alignment of all serialized data\n> */\n> template<>\n> -std::tuple, std::vector>\n> +std::tuple, std::vector>\n> IPADataSerializer::serialize(const FileDescriptor &data,\n> \t\t\t\t\t [[maybe_unused]] ControlSerializer *cs)\n> {\n> -\tstd::vector dataVec = { data.isValid() };\n> -\tstd::vector fdVec;\n> +\tstd::vector dataVec;\n> +\tstd::vector fdVec;\n> +\n> +\t/*\n> +\t * Store as uint32_t to prepare for conversion from validity flag\n> +\t * to index, and for alignment.\n> +\t */\n> +\tappendPOD(dataVec, data.isValid());\n> +\n> \tif (data.isValid())\n> -\t\tfdVec.push_back(data.fd());\n> +\t\tfdVec.push_back(data);\n> +\n> \n> \treturn { dataVec, fdVec };\n> }\n> \n> template<>\n> -FileDescriptor IPADataSerializer::deserialize(std::vector::const_iterator dataBegin,\n> -\t\t\t\t\t\t\t std::vector::const_iterator dataEnd,\n> -\t\t\t\t\t\t\t std::vector::const_iterator fdsBegin,\n> -\t\t\t\t\t\t\t std::vector::const_iterator fdsEnd,\n> +FileDescriptor IPADataSerializer::deserialize([[maybe_unused]] std::vector::const_iterator dataBegin,\n> +\t\t\t\t\t\t\t [[maybe_unused]] std::vector::const_iterator dataEnd,\n> +\t\t\t\t\t\t\t std::vector::const_iterator fdsBegin,\n> +\t\t\t\t\t\t\t std::vector::const_iterator fdsEnd,\n> \t\t\t\t\t\t\t [[maybe_unused]] ControlSerializer *cs)\n> {\n> -\tASSERT(std::distance(dataBegin, dataEnd) >= 1);\n> +\tASSERT(std::distance(dataBegin, dataEnd) >= 4);\n> \n> -\tbool valid = !!(*dataBegin);\n> +\tuint32_t valid = readPOD(dataBegin, 0, dataEnd);\n> \n> \tASSERT(!(valid && std::distance(fdsBegin, fdsEnd) < 1));\n> \n> -\treturn valid ? FileDescriptor(*fdsBegin) : FileDescriptor();\n> +\treturn valid ? *fdsBegin : FileDescriptor();\n> }\n> \n> template<>\n> FileDescriptor IPADataSerializer::deserialize(const std::vector &data,\n> -\t\t\t\t\t\t\t const std::vector &fds,\n> +\t\t\t\t\t\t\t const std::vector &fds,\n> \t\t\t\t\t\t\t [[maybe_unused]] ControlSerializer *cs)\n> {\n> \treturn deserialize(data.cbegin(), data.end(), fds.cbegin(), fds.end());\n> @@ -561,19 +568,19 @@ FileDescriptor IPADataSerializer::deserialize(const std::vector<\n> /*\n> * FrameBuffer::Plane is serialized as:\n> *\n> - * 1 byte - FileDescriptor\n> + * 4 byte - FileDescriptor\n> * 4 bytes - uint32_t Length\n> */\n> template<>\n> -std::tuple, std::vector>\n> +std::tuple, std::vector>\n> IPADataSerializer::serialize(const FrameBuffer::Plane &data,\n> \t\t\t\t\t\t [[maybe_unused]] ControlSerializer *cs)\n> {\n> \tstd::vector dataVec;\n> -\tstd::vector fdsVec;\n> +\tstd::vector fdsVec;\n> \n> \tstd::vector fdBuf;\n> -\tstd::vector fdFds;\n> +\tstd::vector fdFds;\n> \tstd::tie(fdBuf, fdFds) =\n> \t\tIPADataSerializer::serialize(data.fd);\n> \tdataVec.insert(dataVec.end(), fdBuf.begin(), fdBuf.end());\n> @@ -588,15 +595,15 @@ template<>\n> FrameBuffer::Plane\n> IPADataSerializer::deserialize(std::vector::const_iterator dataBegin,\n> \t\t\t\t\t\t std::vector::const_iterator dataEnd,\n> -\t\t\t\t\t\t std::vector::const_iterator fdsBegin,\n> -\t\t\t\t\t\t [[maybe_unused]] std::vector::const_iterator fdsEnd,\n> +\t\t\t\t\t\t std::vector::const_iterator fdsBegin,\n> +\t\t\t\t\t\t [[maybe_unused]] std::vector::const_iterator fdsEnd,\n> \t\t\t\t\t\t [[maybe_unused]] ControlSerializer *cs)\n> {\n> \tFrameBuffer::Plane ret;\n> \n> -\tret.fd = IPADataSerializer::deserialize(dataBegin, dataBegin + 1,\n> +\tret.fd = IPADataSerializer::deserialize(dataBegin, dataBegin + 4,\n> \t\t\t\t\t\t\t\tfdsBegin, fdsBegin + 1);\n> -\tret.length = readPOD(dataBegin, 1, dataEnd);\n> +\tret.length = readPOD(dataBegin, 4, dataEnd);\n> \n> \treturn ret;\n> }\n> @@ -604,7 +611,7 @@ IPADataSerializer::deserialize(std::vector::const_i\n> template<>\n> FrameBuffer::Plane\n> IPADataSerializer::deserialize(const std::vector &data,\n> -\t\t\t\t\t\t const std::vector &fds,\n> +\t\t\t\t\t\t const std::vector &fds,\n> \t\t\t\t\t\t ControlSerializer *cs)\n> {\n> \treturn deserialize(data.cbegin(), data.end(), fds.cbegin(), fds.end(), cs);\n> diff --git a/src/libcamera/ipc_pipe.cpp b/src/libcamera/ipc_pipe.cpp\n> index 28e20e03..84136a82 100644\n> --- a/src/libcamera/ipc_pipe.cpp\n> +++ b/src/libcamera/ipc_pipe.cpp\n> @@ -77,13 +77,17 @@ IPCMessage::IPCMessage(const Header &header)\n> *\n> * This essentially converts an IPCUnixSocket payload into an IPCMessage.\n> * The header is extracted from the payload into the IPCMessage's header field.\n> + *\n> + * If the IPCUnixSocket payload had any valid file descriptors, then they will\n> + * all be invalidated.\n> */\n> -IPCMessage::IPCMessage(const IPCUnixSocket::Payload &payload)\n> +IPCMessage::IPCMessage(IPCUnixSocket::Payload &payload)\n> {\n> \tmemcpy(&header_, payload.data.data(), sizeof(header_));\n> \tdata_ = std::vector(payload.data.begin() + sizeof(header_),\n> \t\t\t\t payload.data.end());\n> -\tfds_ = payload.fds;\n> +\tfor (int32_t &fd : payload.fds)\n> +\t\tfds_.push_back(FileDescriptor(std::move(fd)));\n> }\n> \n> /**\n> @@ -104,7 +108,9 @@ IPCUnixSocket::Payload IPCMessage::payload() const\n> \n> \t/* \\todo Make this work without copy */\n> \tmemcpy(payload.data.data() + sizeof(Header), data_.data(), data_.size());\n> -\tpayload.fds = fds_;\n> +\n> +\tfor (const FileDescriptor &fd : fds_)\n> +\t\tpayload.fds.push_back(fd.fd());\n> \n> \treturn payload;\n> }\n> diff --git a/test/serialization/ipa_data_serializer_test.cpp b/test/serialization/ipa_data_serializer_test.cpp\n> index bf7e34e2..eca19a66 100644\n> --- a/test/serialization/ipa_data_serializer_test.cpp\n> +++ b/test/serialization/ipa_data_serializer_test.cpp\n> @@ -53,7 +53,7 @@ template\n> int testPodSerdes(T in)\n> {\n> \tstd::vector buf;\n> -\tstd::vector fds;\n> +\tstd::vector fds;\n> \n> \tstd::tie(buf, fds) = IPADataSerializer::serialize(in);\n> \tT out = IPADataSerializer::deserialize(buf, fds);\n> @@ -72,7 +72,7 @@ int testVectorSerdes(const std::vector &in,\n> \t\t ControlSerializer *cs = nullptr)\n> {\n> \tstd::vector buf;\n> -\tstd::vector fds;\n> +\tstd::vector fds;\n> \n> \tstd::tie(buf, fds) = IPADataSerializer>::serialize(in, cs);\n> \tstd::vector out = IPADataSerializer>::deserialize(buf, fds, cs);\n> @@ -92,7 +92,7 @@ int testMapSerdes(const std::map &in,\n> \t\t ControlSerializer *cs = nullptr)\n> {\n> \tstd::vector buf;\n> -\tstd::vector fds;\n> +\tstd::vector fds;\n> \n> \tstd::tie(buf, fds) = IPADataSerializer>::serialize(in, cs);\n> \tstd::map out = IPADataSerializer>::deserialize(buf, fds, cs);\n> @@ -219,7 +219,7 @@ private:\n> \t\t};\n> \n> \t\tstd::vector buf;\n> -\t\tstd::vector fds;\n> +\t\tstd::vector fds;\n> \n> \t\tif (testVectorSerdes(vecUint8) != TestPass)\n> \t\t\treturn TestFail;\n> @@ -291,7 +291,7 @@ private:\n> \t\t\t{ { \"a\", { 1, 2, 3 } }, { \"b\", { 4, 5, 6 } }, { \"c\", { 7, 8, 9 } } };\n> \n> \t\tstd::vector buf;\n> -\t\tstd::vector fds;\n> +\t\tstd::vector fds;\n> \n> \t\tif (testMapSerdes(mapUintStr) != TestPass)\n> \t\t\treturn TestFail;\n> @@ -359,7 +359,7 @@ private:\n> \t\tstd::string strEmpty = \"\";\n> \n> \t\tstd::vector buf;\n> -\t\tstd::vector fds;\n> +\t\tstd::vector fds;\n> \n> \t\tif (testPodSerdes(u32min) != TestPass)\n> \t\t\treturn TestFail;\n> diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> index a4e008c7..aab1fffb 100644\n> --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> @@ -236,7 +236,7 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)\n> void {{proxy_name}}::{{method.mojom_name}}IPC(\n> \tstd::vector::const_iterator data,\n> \tsize_t dataSize,\n> -\t[[maybe_unused]] const std::vector &fds)\n> +\t[[maybe_unused]] const std::vector &fds)\n> {\n> {%- for param in method.parameters %}\n> \t{{param|name}} {{param.mojom_name}};\n> diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> index c222f5f2..1979e68f 100644\n> --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> @@ -65,7 +65,7 @@ private:\n> \tvoid {{method.mojom_name}}IPC(\n> \t\tstd::vector::const_iterator data,\n> \t\tsize_t dataSize,\n> -\t\tconst std::vector &fds);\n> +\t\tconst std::vector &fds);\n> {% endfor %}\n> \n> \t/* Helper class to invoke async functions in another thread. */\n> diff --git a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl\n> index ea9cc86b..ebcd2aaa 100644\n> --- a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl\n> +++ b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl\n> @@ -54,7 +54,7 @@\n> {%- for param in params %}\n> \tstd::vector {{param.mojom_name}}Buf;\n> {%- if param|has_fd %}\n> -\tstd::vector {{param.mojom_name}}Fds;\n> +\tstd::vector {{param.mojom_name}}Fds;\n> \tstd::tie({{param.mojom_name}}Buf, {{param.mojom_name}}Fds) =\n> {%- else %}\n> \tstd::tie({{param.mojom_name}}Buf, std::ignore) =\n> diff --git a/utils/ipc/generators/libcamera_templates/serializer.tmpl b/utils/ipc/generators/libcamera_templates/serializer.tmpl\n> index d8d55807..b8ef8e7b 100644\n> --- a/utils/ipc/generators/libcamera_templates/serializer.tmpl\n> +++ b/utils/ipc/generators/libcamera_templates/serializer.tmpl\n> @@ -40,7 +40,7 @@\n> \t\tretData.insert(retData.end(), {{field.mojom_name}}.begin(), {{field.mojom_name}}.end());\n> {%- elif field|is_fd %}\n> \t\tstd::vector {{field.mojom_name}};\n> -\t\tstd::vector {{field.mojom_name}}Fds;\n> +\t\tstd::vector {{field.mojom_name}}Fds;\n> \t\tstd::tie({{field.mojom_name}}, {{field.mojom_name}}Fds) =\n> \t\t\tIPADataSerializer<{{field|name}}>::serialize(data.{{field.mojom_name}});\n> \t\tretData.insert(retData.end(), {{field.mojom_name}}.begin(), {{field.mojom_name}}.end());\n> @@ -58,7 +58,7 @@\n> {%- elif field|is_plain_struct or field|is_array or field|is_map or field|is_str %}\n> \t\tstd::vector {{field.mojom_name}};\n> \t{%- if field|has_fd %}\n> -\t\tstd::vector {{field.mojom_name}}Fds;\n> +\t\tstd::vector {{field.mojom_name}}Fds;\n> \t\tstd::tie({{field.mojom_name}}, {{field.mojom_name}}Fds) =\n> \t{%- else %}\n> \t\tstd::tie({{field.mojom_name}}, std::ignore) =\n> @@ -104,9 +104,9 @@\n> \t\tdataSize -= {{field_size}};\n> \t{%- endif %}\n> {% elif field|is_fd %}\n> -\t{%- set field_size = 1 %}\n> +\t{%- set field_size = 4 %}\n> \t\t{{- check_data_size(field_size, 'dataSize', field.mojom_name, 'data')}}\n> -\t\tret.{{field.mojom_name}} = IPADataSerializer<{{field|name}}>::deserialize(m, m + 1, n, n + 1, cs);\n> +\t\tret.{{field.mojom_name}} = IPADataSerializer<{{field|name}}>::deserialize(m, m + {{field_size}}, n, n + 1, cs);\n> \t{%- if not loop.last %}\n> \t\tm += {{field_size}};\n> \t\tdataSize -= {{field_size}};\n> @@ -177,7 +177,7 @@\n> # \\a struct.\n> #}\n> {%- macro serializer(struct, namespace) %}\n> -\tstatic std::tuple, std::vector>\n> +\tstatic std::tuple, std::vector>\n> \tserialize(const {{struct|name_full}} &data,\n> {%- if struct|needs_control_serializer %}\n> \t\t ControlSerializer *cs)\n> @@ -187,7 +187,7 @@\n> \t{\n> \t\tstd::vector retData;\n> {%- if struct|has_fd %}\n> -\t\tstd::vector retFds;\n> +\t\tstd::vector retFds;\n> {%- endif %}\n> {%- for field in struct.fields %}\n> {{serializer_field(field, namespace, loop)}}\n> @@ -210,7 +210,7 @@\n> {%- macro deserializer_fd(struct, namespace) %}\n> \tstatic {{struct|name_full}}\n> \tdeserialize(std::vector &data,\n> -\t\t std::vector &fds,\n> +\t\t std::vector &fds,\n> {%- if struct|needs_control_serializer %}\n> \t\t ControlSerializer *cs)\n> {%- else %}\n> @@ -224,8 +224,8 @@\n> \tstatic {{struct|name_full}}\n> \tdeserialize(std::vector::const_iterator dataBegin,\n> \t\t std::vector::const_iterator dataEnd,\n> -\t\t std::vector::const_iterator fdsBegin,\n> -\t\t std::vector::const_iterator fdsEnd,\n> +\t\t std::vector::const_iterator fdsBegin,\n> +\t\t std::vector::const_iterator fdsEnd,\n> {%- if struct|needs_control_serializer %}\n> \t\t ControlSerializer *cs)\n> {%- else %}\n> @@ -234,7 +234,7 @@\n> \t{\n> \t\t{{struct|name_full}} ret;\n> \t\tstd::vector::const_iterator m = dataBegin;\n> -\t\tstd::vector::const_iterator n = fdsBegin;\n> +\t\tstd::vector::const_iterator n = fdsBegin;\n> \n> \t\tsize_t dataSize = std::distance(dataBegin, dataEnd);\n> \t\t[[maybe_unused]] size_t fdsSize = std::distance(fdsBegin, fdsEnd);\n> @@ -255,7 +255,7 @@\n> {%- macro deserializer_fd_simple(struct, namespace) %}\n> \tstatic {{struct|name_full}}\n> \tdeserialize(std::vector &data,\n> -\t\t [[maybe_unused]] std::vector &fds,\n> +\t\t [[maybe_unused]] std::vector &fds,\n> \t\t ControlSerializer *cs = nullptr)\n> \t{\n> \t\treturn IPADataSerializer<{{struct|name_full}}>::deserialize(data.cbegin(), data.cend(), cs);\n> @@ -264,8 +264,8 @@\n> \tstatic {{struct|name_full}}\n> \tdeserialize(std::vector::const_iterator dataBegin,\n> \t\t std::vector::const_iterator dataEnd,\n> -\t\t [[maybe_unused]] std::vector::const_iterator fdsBegin,\n> -\t\t [[maybe_unused]] std::vector::const_iterator fdsEnd,\n> +\t\t [[maybe_unused]] std::vector::const_iterator fdsBegin,\n> +\t\t [[maybe_unused]] std::vector::const_iterator fdsEnd,\n> \t\t ControlSerializer *cs = nullptr)\n> \t{\n> \t\treturn IPADataSerializer<{{struct|name_full}}>::deserialize(dataBegin, dataEnd, cs);","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 CBC40BD87C\n\tfor ;\n\tThu, 19 Aug 2021 07:28:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3B50E688A3;\n\tThu, 19 Aug 2021 09:28:57 +0200 (CEST)","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 2EA986888E\n\tfor ;\n\tThu, 19 Aug 2021 09:28:56 +0200 (CEST)","from [192.168.1.104] (unknown [103.238.109.15])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B1D292A8;\n\tThu, 19 Aug 2021 09:28:54 +0200 (CEST)"],"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=\"nZ2nOk86\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629358135;\n\tbh=zUGLSB6DqW4oeuPE2FuHu7K4wc2N9WOwPw8wUZVY9q4=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=nZ2nOk86oZY5C9F5LCiZqWgY2Cml2xUQdvHnS1RnR6EixZPpXnqj/1VQlM6LNFfo+\n\tz3DyXisPcS/agFn0JrMCwH9X675wQb3+6GD+ZOJw2VlZs5CZbOqWfyYfe7W5seso8Z\n\t19Gsff7a7U5npHHRxIMKtKzDf2/9+7qcW8kHPVuU=","To":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210819071956.3431114-1-paul.elder@ideasonboard.com>","From":"Umang Jain ","Message-ID":"<8ba43326-4c49-cbfe-35bf-3b438dd31a32@ideasonboard.com>","Date":"Thu, 19 Aug 2021 12:58:50 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<20210819071956.3431114-1-paul.elder@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v4] ipa: Use FileDescriptor instead of\n\tint in layers above IPC payload","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\" "}}]