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

[Paul Elder]

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