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

From: Jan Kara
Date: Mon Mar 16 2015 - 03:46:30 EST


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/