Re: [RFC] Fix eir parsing function.

[Szymon Janc <szymon.janc@tieto.com>]

Hi Johan,

> Hi Szymon,
> 
> On Fri, Nov 29, 2013, Szymon Janc wrote:
> > > Hi Andrei,
> > > 
> > > On Fri, Nov 29, 2013, Andrei Emeltchenko wrote:
> > > > Currently eir_parse always return 0 but it is checked throughout the
> > > > code (in android/bluetooth code as well in src/adapteri, etc) for
> > > > return value (err < 0) which never happens. Return -1 if there is
> > > > nothing to parse. Send as RFC as I do not know how current code supposed
> > > > to be working.
> > > > ---
> > > >  src/eir.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/src/eir.c b/src/eir.c
> > > > index 084884e..f7450c9 100644
> > > > --- a/src/eir.c
> > > > +++ b/src/eir.c
> > > > @@ -138,7 +138,7 @@ int eir_parse(struct eir_data *eir, const uint8_t *eir_data, uint8_t eir_len)
> > > >  
> > > >  	/* No EIR data to parse */
> > > >  	if (eir_data == NULL)
> > > > -		return 0;
> > > > +		return -1;
> > > >  
> > > >  	while (len < eir_len - 1) {
> > > >  		uint8_t field_len = eir_data[0];
> > > 
> > > I think the only concern here is "when is it safe to call
> > > eir_data_free()?". If it would not be safe to call that in case the
> > > eir_parse function "fails" then we'd need to be able to clearly
> > > distinguish failure though a -1 return value. However, as it now stands
> > > it is always safe to call eir_data_free() on a struct that was passed to
> > > eir_parse(), so I'd go for simply changing this function to return void
> > > instead of int.
> > 
> > I think it should be possible to check if parsing failed due to invalid EIR
> > but I think parsing empty EIR should not result in error.
> > 
> > Most callers do verify eir_len or buffer validity before calling eir_parse()
> > anyway so I would change this  check to:
> > if (!eir_data || !eir_len) return 0; 
> > 
> > and simplify callers code.
> 
> I'm fine with adding a check for !eir_len, but if you check the actual
> implementation of eir_parse you'll see that it has no places where it'd
> return an error in the case of invalid EIR. In such a case the function
> just stops parsing at that point and returns. I think from a
> interoperability/usability perspective this makes sense: we take the
> best effort to parse what we can and use the data that we were able to
> parse until the parsing error happened. If you'd return an error when
> parsing fails it'd make the calling code disregard any of the valid data
> that was parsed until the point where the invalid data began.

Right, then making this return void makes sense as currently it is confusing
that it might return an error (eg. as used in eir_parse_oob()).

-- 
BR
Szymon Janc