Re: [PATCH 4/5 linux-next] udf: rename udf_get_filename()

[Jan Kara]

  Hi Fabian,

On Sun 15-03-15 09:34:35, Fabian Frederick wrote:
> > On 14 March 2015 at 07:52 Jan Kara <jack@xxxxxxx> wrote:
> >
> >
> > On Tue 10-03-15 21:44:34, Fabian Frederick wrote:
> > > udf_readdir(), udf_find_entry() and udf_pc_to_char() use
> > > udf_get_filename to obtain name length. Give that function
> > > an appropriate name.
> >   Hum, have you read what that function does? It actually converts the name
> > to a different format and returns converted length. So your name is IMHO
> > more confusing - it's as if sprintf() was called sprintf_length()... Not
> > applied.
> 
> Ok for the name but AFAICS there's still a problem with error management in
> udf_get_filename().
> We return 0 when not able to allocate filename and callsites don't seem to
> relate the real problem.
  Umm, we have three callsites:
udf_readdir() - that skips the name if returned length is 0. Arguably we
  should return error to userspace if we didn't emit any name yet. But then
  all other error handling in that function should behave like that.
udf_find_entry() - again we skip name if returned length is 0. Again we
  could do better in error handling but that would require rewriting
  udf_find_entry() to propagate errors up the stack instead of just
  returning NULL.
udf_pc_to_char() - This forgets to check and can return error. I'm happy to
  take a fix for that (or I will write it unless you do).

So returning error value < 0 for error from udf_get_filename() would look
OK to me. But given how udf_readdir() and udf_find_entry() behave it won't
help too much. But it's a step in the right direction. Thanks for spotting
this.

								Honza

-- 
Jan Kara <jack@xxxxxxx>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/