[libcamera-devel] utils: ipc: Fix deserialization of multiple fd parameters
diff mbox series

Message ID 20231211105638.1787362-1-paul.elder@ideasonboard.com
State Accepted
Commit 1d0d2cf67cb7100cfc7a368347271f88365b0a52
Headers show
Series
  • [libcamera-devel] utils: ipc: Fix deserialization of multiple fd parameters
Related show

Commit Message

Paul Elder Dec. 11, 2023, 10:56 a.m. UTC 
There was a bug where the code generated for deserialization of function
parameters would fail if there were multiple file descriptor parameters.
Fix this.

Bug: https://bugs.libcamera.org/show_bug.cgi?id=205
Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
---
 utils/ipc/generators/libcamera_templates/proxy_functions.tmpl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Kieran Bingham Dec. 11, 2023, 1:45 p.m. UTC | #1 
Quoting Paul Elder via libcamera-devel (2023-12-11 10:56:38)
> There was a bug where the code generated for deserialization of function
> parameters would fail if there were multiple file descriptor parameters.

Why? It's hard to decifer that code change. Probably especially as
there's very little context in the patch.

Was it previously not expecting FileDescriptors in the last entry?

> Fix this.

by ... processing the fds on each iteration?

> 
> Bug: https://bugs.libcamera.org/show_bug.cgi?id=205
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
>  utils/ipc/generators/libcamera_templates/proxy_functions.tmpl | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
> index 2be65d432..b5797b149 100644
> --- a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
> +++ b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
> @@ -186,7 +186,7 @@ IPADataSerializer<{{param|name}}>::deserialize(
>  {% for param in params|with_fds %}
>  {%- if loop.first %}
>         const size_t {{param.mojom_name}}FdStart = 0;
> -{%- elif not loop.last %}
> +{%- else %}
>         const size_t {{param.mojom_name}}FdStart = {{loop.previtem.mojom_name}}FdStart + {{loop.previtem.mojom_name}}FdsSize;
>  {%- endif %}
>  {%- endfor %}
> -- 
> 2.39.2
>
Paul Elder Dec. 12, 2023, 8:47 a.m. UTC | #2 
On Mon, Dec 11, 2023 at 01:45:20PM +0000, Kieran Bingham wrote:
> Quoting Paul Elder via libcamera-devel (2023-12-11 10:56:38)
> > There was a bug where the code generated for deserialization of function
> > parameters would fail if there were multiple file descriptor parameters.
> 
> Why? It's hard to decifer that code change. Probably especially as
> there's very little context in the patch.

I suppose more clearly it's "the deserialization code wouldn't be
generated for the last file descriptor parameter".

> 
> Was it previously not expecting FileDescriptors in the last entry?

It was an optimization to get rid of unused code iirc. Evidently I
didn't test it with multiple fd parameters.

> 
> > Fix this.
> 
> by ... processing the fds on each iteration?

Yes :) As opposed to skipping the last one.


Paul

> 
> > 
> > Bug: https://bugs.libcamera.org/show_bug.cgi?id=205
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > ---
> >  utils/ipc/generators/libcamera_templates/proxy_functions.tmpl | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
> > index 2be65d432..b5797b149 100644
> > --- a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
> > +++ b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
> > @@ -186,7 +186,7 @@ IPADataSerializer<{{param|name}}>::deserialize(
> >  {% for param in params|with_fds %}
> >  {%- if loop.first %}
> >         const size_t {{param.mojom_name}}FdStart = 0;
> > -{%- elif not loop.last %}
> > +{%- else %}
> >         const size_t {{param.mojom_name}}FdStart = {{loop.previtem.mojom_name}}FdStart + {{loop.previtem.mojom_name}}FdsSize;
> >  {%- endif %}
> >  {%- endfor %}
> > -- 
> > 2.39.2
> >
Andrey Konovalov Dec. 19, 2023, 9:13 p.m. UTC | #3 
Hi Paul,

On 11.12.2023 13:56, Paul Elder wrote:
> There was a bug where the code generated for deserialization of function
> parameters would fail if there were multiple file descriptor parameters.
> Fix this.
> 
> Bug: https://bugs.libcamera.org/show_bug.cgi?id=205
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>

Tested-by: Andrey Konovalov <andrey.konovalov@linaro.org>

> ---
>   utils/ipc/generators/libcamera_templates/proxy_functions.tmpl | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
> index 2be65d432..b5797b149 100644
> --- a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
> +++ b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
> @@ -186,7 +186,7 @@ IPADataSerializer<{{param|name}}>::deserialize(
>   {% for param in params|with_fds %}
>   {%- if loop.first %}
>   	const size_t {{param.mojom_name}}FdStart = 0;
> -{%- elif not loop.last %}
> +{%- else %}
>   	const size_t {{param.mojom_name}}FdStart = {{loop.previtem.mojom_name}}FdStart + {{loop.previtem.mojom_name}}FdsSize;
>   {%- endif %}
>   {%- endfor %}
Kieran Bingham Jan. 9, 2024, 12:53 p.m. UTC | #4 
Quoting Paul Elder (2023-12-12 08:47:45)
> On Mon, Dec 11, 2023 at 01:45:20PM +0000, Kieran Bingham wrote:
> > Quoting Paul Elder via libcamera-devel (2023-12-11 10:56:38)
> > > There was a bug where the code generated for deserialization of function
> > > parameters would fail if there were multiple file descriptor parameters.
> > 
> > Why? It's hard to decifer that code change. Probably especially as
> > there's very little context in the patch.
> 
> I suppose more clearly it's "the deserialization code wouldn't be
> generated for the last file descriptor parameter".
> 
> > 
> > Was it previously not expecting FileDescriptors in the last entry?
> 
> It was an optimization to get rid of unused code iirc. Evidently I
> didn't test it with multiple fd parameters.
> 
> > 
> > > Fix this.
> > 
> > by ... processing the fds on each iteration?
> 
> Yes :) As opposed to skipping the last one.

Ok, then:


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

I think an updated commit message could help here:

"""
The IPADataSerializer::deserialiser attempts to optimise code paths and
remove potentially unused code where multiple File Descriptors were not
expected to be utilised.

The addition of multiple SharedFD entries in the IPC highlights this as
a bug. 

Clean up the conditionals to ensure that all File Descriptors are
correctly deserialized.
"""

Any objections?

> 
> 
> Paul
> 
> > 
> > > 
> > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=205
> > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > ---
> > >  utils/ipc/generators/libcamera_templates/proxy_functions.tmpl | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
> > > index 2be65d432..b5797b149 100644
> > > --- a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
> > > +++ b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
> > > @@ -186,7 +186,7 @@ IPADataSerializer<{{param|name}}>::deserialize(
> > >  {% for param in params|with_fds %}
> > >  {%- if loop.first %}
> > >         const size_t {{param.mojom_name}}FdStart = 0;
> > > -{%- elif not loop.last %}
> > > +{%- else %}
> > >         const size_t {{param.mojom_name}}FdStart = {{loop.previtem.mojom_name}}FdStart + {{loop.previtem.mojom_name}}FdsSize;
> > >  {%- endif %}
> > >  {%- endfor %}
> > > -- 
> > > 2.39.2
> > >
Laurent Pinchart Jan. 9, 2024, 1 p.m. UTC | #5 
On Tue, Jan 09, 2024 at 12:53:20PM +0000, Kieran Bingham via libcamera-devel wrote:
> Quoting Paul Elder (2023-12-12 08:47:45)
> > On Mon, Dec 11, 2023 at 01:45:20PM +0000, Kieran Bingham wrote:
> > > Quoting Paul Elder via libcamera-devel (2023-12-11 10:56:38)
> > > > There was a bug where the code generated for deserialization of function
> > > > parameters would fail if there were multiple file descriptor parameters.
> > > 
> > > Why? It's hard to decifer that code change. Probably especially as
> > > there's very little context in the patch.
> > 
> > I suppose more clearly it's "the deserialization code wouldn't be
> > generated for the last file descriptor parameter".
> > 
> > > Was it previously not expecting FileDescriptors in the last entry?
> > 
> > It was an optimization to get rid of unused code iirc. Evidently I
> > didn't test it with multiple fd parameters.
> > 
> > > > Fix this.
> > > 
> > > by ... processing the fds on each iteration?
> > 
> > Yes :) As opposed to skipping the last one.
> 
> Ok, then:
> 
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> I think an updated commit message could help here:
> 
> """
> The IPADataSerializer::deserialiser attempts to optimise code paths and
> remove potentially unused code where multiple File Descriptors were not
> expected to be utilised.
> 
> The addition of multiple SharedFD entries in the IPC highlights this as
> a bug. 
> 
> Clean up the conditionals to ensure that all File Descriptors are
> correctly deserialized.
> """
> 
> Any objections?

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> > > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=205
> > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > > ---
> > > >  utils/ipc/generators/libcamera_templates/proxy_functions.tmpl | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
> > > > index 2be65d432..b5797b149 100644
> > > > --- a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
> > > > +++ b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
> > > > @@ -186,7 +186,7 @@ IPADataSerializer<{{param|name}}>::deserialize(
> > > >  {% for param in params|with_fds %}
> > > >  {%- if loop.first %}
> > > >         const size_t {{param.mojom_name}}FdStart = 0;
> > > > -{%- elif not loop.last %}
> > > > +{%- else %}
> > > >         const size_t {{param.mojom_name}}FdStart = {{loop.previtem.mojom_name}}FdStart + {{loop.previtem.mojom_name}}FdsSize;
> > > >  {%- endif %}
> > > >  {%- endfor %}
Paul Elder Jan. 9, 2024, 1:04 p.m. UTC | #6 
On Tue, Jan 09, 2024 at 12:53:20PM +0000, Kieran Bingham wrote:
> Quoting Paul Elder (2023-12-12 08:47:45)
> > On Mon, Dec 11, 2023 at 01:45:20PM +0000, Kieran Bingham wrote:
> > > Quoting Paul Elder via libcamera-devel (2023-12-11 10:56:38)
> > > > There was a bug where the code generated for deserialization of function
> > > > parameters would fail if there were multiple file descriptor parameters.
> > > 
> > > Why? It's hard to decifer that code change. Probably especially as
> > > there's very little context in the patch.
> > 
> > I suppose more clearly it's "the deserialization code wouldn't be
> > generated for the last file descriptor parameter".
> > 
> > > 
> > > Was it previously not expecting FileDescriptors in the last entry?
> > 
> > It was an optimization to get rid of unused code iirc. Evidently I
> > didn't test it with multiple fd parameters.
> > 
> > > 
> > > > Fix this.
> > > 
> > > by ... processing the fds on each iteration?
> > 
> > Yes :) As opposed to skipping the last one.
> 
> Ok, then:
> 
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Thanks.

> 
> I think an updated commit message could help here:
> 
> """
> The IPADataSerializer::deserialiser attempts to optimise code paths and

s/deserialiser/deserializer/

> remove potentially unused code where multiple File Descriptors were not
> expected to be utilised.

Not quite... it optimized the generated code to not deserialize the
*last* file descriptor when there were multiple fds directly in the
parameters of an IPC call.

> The addition of multiple SharedFD entries in the IPC highlights this as
> a bug. 
> 
> Clean up the conditionals to ensure that all File Descriptors are
> correctly deserialized.

Yes.

> """
> 
> Any objections?

Just that one up there.


Thanks,

Paul

> 
> > 
> > 
> > Paul
> > 
> > > 
> > > > 
> > > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=205
> > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > > ---
> > > >  utils/ipc/generators/libcamera_templates/proxy_functions.tmpl | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
> > > > index 2be65d432..b5797b149 100644
> > > > --- a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
> > > > +++ b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
> > > > @@ -186,7 +186,7 @@ IPADataSerializer<{{param|name}}>::deserialize(
> > > >  {% for param in params|with_fds %}
> > > >  {%- if loop.first %}
> > > >         const size_t {{param.mojom_name}}FdStart = 0;
> > > > -{%- elif not loop.last %}
> > > > +{%- else %}
> > > >         const size_t {{param.mojom_name}}FdStart = {{loop.previtem.mojom_name}}FdStart + {{loop.previtem.mojom_name}}FdsSize;
> > > >  {%- endif %}
> > > >  {%- endfor %}
> > > > -- 
> > > > 2.39.2
> > > >

Patch
diff mbox series 

diff --git a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
index 2be65d432..b5797b149 100644
--- a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
+++ b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
@@ -186,7 +186,7 @@  IPADataSerializer<{{param|name}}>::deserialize(
 {% for param in params|with_fds %}
 {%- if loop.first %}
 	const size_t {{param.mojom_name}}FdStart = 0;
-{%- elif not loop.last %}
+{%- else %}
 	const size_t {{param.mojom_name}}FdStart = {{loop.previtem.mojom_name}}FdStart + {{loop.previtem.mojom_name}}FdsSize;
 {%- endif %}
 {%- endfor %}