On Fri, Mar 04, 2022 at 03:41:33PM +0200, Sakari Ailus wrote:
> On Fri, Mar 04, 2022 at 02:54:21PM +0200, Andy Shevchenko wrote:
> > On Thu, Mar 03, 2022 at 02:33:46PM -0800, Bjorn Andersson wrote:
...
> > > + if (count >= matches_len && matches) {
> > > + fwnode_handle_put(ep);
> > > + return count;
> > > + }
> >
> > Wouldn't be the same as
> >
> > if (count >= matches_len) {
> > fwnode_handle_put(ep);
> > break;
> > }
>
> Don't you need to check for non-NULL matches here?
Right, this should be kept as in original patch.
> I find return above easier to read.
Okay, original code may work, so I have no strong opinion about return vs
break, although I find slightly better to have a single point of return in
such case.
> > ?
...
> > > + count_graph = fwnode_graph_devcon_matches(fwnode, con_id, data, match,
> > > + matches, matches_len);
> >
> > > + matches += count_graph;
> > > + matches_len -= count_graph;
> >
> > No, won't work when matches == NULL.
> >
> > Also, matches_len is expected to be 0 in that case (or at least being ignored,
> > check with vsnprintf() behaviour in similar case).
> >
> > So, something like this, perhaps
> >
> > if (matches && matches_len) {
> > matches += count_graph;
> > matches_len -= count_graph;
> > }
>
> Good find!
I have checked vsnprintf() and indeed, it expects to have the size is 0 when
the resulting buffer pointer is NULL, and it doesn't do any additional checks.
> > > + count_ref = fwnode_devcon_matches(fwnode, con_id, data, match,
> > > + matches, matches_len);
--
With Best Regards,
Andy Shevchenko
