[{"id":16536,"web_url":"https://patchwork.libcamera.org/comment/16536/","msgid":"","date":"2021-04-25T23:35:06","subject":"Re: [libcamera-devel] [PATCH v3 2/3] utils: ipc: Use the proper\n\tnamespace for mojom structs","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Fri, Apr 23, 2021 at 07:47:10PM +0900, Paul Elder wrote:\n> Structs defined in mojom previously used the namespace of the mojom file\n> that was being used as the source. This is obviously not the correct\n> namespace for structs that are defined in core.mojom. Fix the jinja\n> function for getting the element type including namespace, and use it.\n> \n> Signed-off-by: Paul Elder \n\nNice cleanup.\n\nReviewed-by: Laurent Pinchart \n\n> ---\n> New in v3\n> ---\n> .../module_ipa_serializer.h.tmpl | 2 +-\n> .../libcamera_templates/serializer.tmpl | 34 +++++++++----------\n> .../generators/mojom_libcamera_generator.py | 16 ++++++---\n> 3 files changed, 30 insertions(+), 22 deletions(-)\n> \n> diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_serializer.h.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_serializer.h.tmpl\n> index 64ae99dc..779d2114 100644\n> --- a/utils/ipc/generators/libcamera_templates/module_ipa_serializer.h.tmpl\n> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_serializer.h.tmpl\n> @@ -30,7 +30,7 @@ namespace libcamera {\n> LOG_DECLARE_CATEGORY(IPADataSerializer)\n> {% for struct in structs_nonempty %}\n> template<>\n> -class IPADataSerializer<{{struct|name_full(namespace_str)}}>\n> +class IPADataSerializer<{{struct|name_full}}>\n> {\n> public:\n> {{- serializer.serializer(struct, namespace_str)}}\n> diff --git a/utils/ipc/generators/libcamera_templates/serializer.tmpl b/utils/ipc/generators/libcamera_templates/serializer.tmpl\n> index af35b9e3..d8d55807 100644\n> --- a/utils/ipc/generators/libcamera_templates/serializer.tmpl\n> +++ b/utils/ipc/generators/libcamera_templates/serializer.tmpl\n> @@ -68,7 +68,7 @@\n> \t{%- elif field|is_str %}\n> \t\t\tIPADataSerializer<{{field|name}}>::serialize(data.{{field.mojom_name}});\n> \t{%- else %}\n> -\t\t\tIPADataSerializer<{{field|name_full(namespace)}}>::serialize(data.{{field.mojom_name}}, cs);\n> +\t\t\tIPADataSerializer<{{field|name_full}}>::serialize(data.{{field.mojom_name}}, cs);\n> \t{%- endif %}\n> \t\tappendPOD(retData, {{field.mojom_name}}.size());\n> \t{%- if field|has_fd %}\n> @@ -97,7 +97,7 @@\n> \t\t{%- if field|is_pod %}\n> \t\tret.{{field.mojom_name}} = IPADataSerializer<{{field|name}}>::deserialize(m, m + {{field_size}});\n> \t\t{%- else %}\n> -\t\tret.{{field.mojom_name}} = static_cast<{{field|name_full(namespace)}}>(IPADataSerializer::deserialize(m, m + {{field_size}}));\n> +\t\tret.{{field.mojom_name}} = static_cast<{{field|name_full}}>(IPADataSerializer::deserialize(m, m + {{field_size}}));\n> \t\t{%- endif %}\n> \t{%- if not loop.last %}\n> \t\tm += {{field_size}};\n> @@ -150,11 +150,11 @@\n> \t{%- elif field|has_fd and (field|is_array or field|is_map) %}\n> \t\t\tIPADataSerializer<{{field|name}}>::deserialize(m, m + {{field.mojom_name}}Size, n, n + {{field.mojom_name}}FdsSize, cs);\n> \t{%- elif field|has_fd and (not (field|is_array or field|is_map)) %}\n> -\t\t\tIPADataSerializer<{{field|name_full(namespace)}}>::deserialize(m, m + {{field.mojom_name}}Size, n, n + {{field.mojom_name}}FdsSize, cs);\n> +\t\t\tIPADataSerializer<{{field|name_full}}>::deserialize(m, m + {{field.mojom_name}}Size, n, n + {{field.mojom_name}}FdsSize, cs);\n> \t{%- elif (not field|has_fd) and (field|is_array or field|is_map) %}\n> \t\t\tIPADataSerializer<{{field|name}}>::deserialize(m, m + {{field.mojom_name}}Size, cs);\n> \t{%- else %}\n> -\t\t\tIPADataSerializer<{{field|name_full(namespace)}}>::deserialize(m, m + {{field.mojom_name}}Size, cs);\n> +\t\t\tIPADataSerializer<{{field|name_full}}>::deserialize(m, m + {{field.mojom_name}}Size, cs);\n> \t{%- endif %}\n> \t{%- if not loop.last %}\n> \t\tm += {{field_size}};\n> @@ -178,7 +178,7 @@\n> #}\n> {%- macro serializer(struct, namespace) %}\n> \tstatic std::tuple, std::vector>\n> -\tserialize(const {{struct|name_full(namespace)}} &data,\n> +\tserialize(const {{struct|name_full}} &data,\n> {%- if struct|needs_control_serializer %}\n> \t\t ControlSerializer *cs)\n> {%- else %}\n> @@ -208,7 +208,7 @@\n> # \\a struct, in the case that \\a struct has file descriptors.\n> #}\n> {%- macro deserializer_fd(struct, namespace) %}\n> -\tstatic {{struct|name_full(namespace)}}\n> +\tstatic {{struct|name_full}}\n> \tdeserialize(std::vector &data,\n> \t\t std::vector &fds,\n> {%- if struct|needs_control_serializer %}\n> @@ -217,11 +217,11 @@\n> \t\t ControlSerializer *cs = nullptr)\n> {%- endif %}\n> \t{\n> -\t\treturn IPADataSerializer<{{struct|name_full(namespace)}}>::deserialize(data.cbegin(), data.cend(), fds.cbegin(), fds.cend(), cs);\n> +\t\treturn IPADataSerializer<{{struct|name_full}}>::deserialize(data.cbegin(), data.cend(), fds.cbegin(), fds.cend(), cs);\n> \t}\n> \n> {# \\todo Don't inline this function #}\n> -\tstatic {{struct|name_full(namespace)}}\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> @@ -232,7 +232,7 @@\n> \t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> {%- endif %}\n> \t{\n> -\t\t{{struct|name_full(namespace)}} ret;\n> +\t\t{{struct|name_full}} ret;\n> \t\tstd::vector::const_iterator m = dataBegin;\n> \t\tstd::vector::const_iterator n = fdsBegin;\n> \n> @@ -253,22 +253,22 @@\n> # deserializers with file descriptors.\n> #}\n> {%- macro deserializer_fd_simple(struct, namespace) %}\n> -\tstatic {{struct|name_full(namespace)}}\n> +\tstatic {{struct|name_full}}\n> \tdeserialize(std::vector &data,\n> \t\t [[maybe_unused]] std::vector &fds,\n> \t\t ControlSerializer *cs = nullptr)\n> \t{\n> -\t\treturn IPADataSerializer<{{struct|name_full(namespace)}}>::deserialize(data.cbegin(), data.cend(), cs);\n> +\t\treturn IPADataSerializer<{{struct|name_full}}>::deserialize(data.cbegin(), data.cend(), cs);\n> \t}\n> \n> -\tstatic {{struct|name_full(namespace)}}\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 ControlSerializer *cs = nullptr)\n> \t{\n> -\t\treturn IPADataSerializer<{{struct|name_full(namespace)}}>::deserialize(dataBegin, dataEnd, cs);\n> +\t\treturn IPADataSerializer<{{struct|name_full}}>::deserialize(dataBegin, dataEnd, cs);\n> \t}\n> {%- endmacro %}\n> \n> @@ -280,7 +280,7 @@\n> # \\a struct, in the case that \\a struct does not have file descriptors.\n> #}\n> {%- macro deserializer_no_fd(struct, namespace) %}\n> -\tstatic {{struct|name_full(namespace)}}\n> +\tstatic {{struct|name_full}}\n> \tdeserialize(std::vector &data,\n> {%- if struct|needs_control_serializer %}\n> \t\t ControlSerializer *cs)\n> @@ -288,11 +288,11 @@\n> \t\t ControlSerializer *cs = nullptr)\n> {%- endif %}\n> \t{\n> -\t\treturn IPADataSerializer<{{struct|name_full(namespace)}}>::deserialize(data.cbegin(), data.cend(), cs);\n> +\t\treturn IPADataSerializer<{{struct|name_full}}>::deserialize(data.cbegin(), data.cend(), cs);\n> \t}\n> \n> {# \\todo Don't inline this function #}\n> -\tstatic {{struct|name_full(namespace)}}\n> +\tstatic {{struct|name_full}}\n> \tdeserialize(std::vector::const_iterator dataBegin,\n> \t\t std::vector::const_iterator dataEnd,\n> {%- if struct|needs_control_serializer %}\n> @@ -301,7 +301,7 @@\n> \t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> {%- endif %}\n> \t{\n> -\t\t{{struct|name_full(namespace)}} ret;\n> +\t\t{{struct|name_full}} ret;\n> \t\tstd::vector::const_iterator m = dataBegin;\n> \n> \t\tsize_t dataSize = std::distance(dataBegin, dataEnd);\n> diff --git a/utils/ipc/generators/mojom_libcamera_generator.py b/utils/ipc/generators/mojom_libcamera_generator.py\n> index db9e28a6..effdfed6 100644\n> --- a/utils/ipc/generators/mojom_libcamera_generator.py\n> +++ b/utils/ipc/generators/mojom_libcamera_generator.py\n> @@ -258,12 +258,12 @@ def GetNameForElement(element):\n> return element.mojom_name\n> # vectors\n> if (mojom.IsArrayKind(element)):\n> - elem_name = GetNameForElement(element.kind)\n> + elem_name = GetFullNameForElement(element.kind)\n> return f'std::vector<{elem_name}>'\n> # maps\n> if (mojom.IsMapKind(element)):\n> - key_name = GetNameForElement(element.key_kind)\n> - value_name = GetNameForElement(element.value_kind)\n> + key_name = GetFullNameForElement(element.key_kind)\n> + value_name = GetFullNameForElement(element.value_kind)\n> return f'std::map<{key_name}, {value_name}>'\n> # struct fields and function parameters\n> if isinstance(element, (mojom.Field, mojom.Method, mojom.Parameter)):\n> @@ -296,8 +296,16 @@ def GetNameForElement(element):\n> raise Exception('Unsupported element: %s' % element)\n> raise Exception('Unexpected element: %s' % element)\n> \n> -def GetFullNameForElement(element, namespace_str):\n> +def GetFullNameForElement(element):\n> name = GetNameForElement(element)\n> + namespace_str = ''\n> + if mojom.IsStructKind(element):\n> + namespace_str = element.module.mojom_namespace.replace('.', '::')\n> + elif (hasattr(element, 'kind') and\n> + (mojom.IsStructKind(element.kind) or\n> + mojom.IsEnumKind(element.kind))):\n> + namespace_str = element.kind.module.mojom_namespace.replace('.', '::')\n> +\n> if namespace_str == '':\n> return name\n> return f'{namespace_str}::{name}'","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 C76EABDC91\n\tfor ;\n\tSun, 25 Apr 2021 23:35:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 296536887E;\n\tMon, 26 Apr 2021 01:35:14 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9F45A602D1\n\tfor ;\n\tMon, 26 Apr 2021 01:35:12 +0200 (CEST)","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 0BD1E4FB;\n\tMon, 26 Apr 2021 01:35:11 +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=\"F4MWRJ92\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1619393712;\n\tbh=2DJ77JebJKjAWHs7A2t2lWmmXw8gGCBMgTR/JCCs+lc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=F4MWRJ92TcNxi6S31SXyZTibZg1/TOEzyp+HTAxXpYcnGMnYIyTDICzRyZ9jl08/Y\n\t7xEkLfHvFlsaJR9McC6lnzZcn2hL2lnG6nX4n+3tcF4T3FhbMr8nnowQyzq//D9TBx\n\tTQewi+3NhSnw78SJA1Ovtz+UuxSTsfAGoVWRFsFs=","Date":"Mon, 26 Apr 2021 02:35:06 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Message-ID":"","References":"<20210423104711.401547-1-paul.elder@ideasonboard.com>\n\t<20210423104711.401547-3-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210423104711.401547-3-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 2/3] utils: ipc: Use the proper\n\tnamespace for mojom structs","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]