[libcamera-devel] [PATCH] utils: ipc: Fix deserialization of multiple fd parameters

[Kieran Bingham]

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 at 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 at 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
> > >