From: Jan Kara <jack@suse.cz>
To: Fabian Frederick <fabf@skynet.be>
Cc: Jan Kara <jack@suse.cz>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 4/5 linux-next] udf: rename udf_get_filename()
Date: Mon, 16 Mar 2015 08:46:20 +0100 [thread overview]
Message-ID: <20150316074619.GB4934@quack.suse.cz> (raw)
In-Reply-To: <886970217.268830.1426408475073.open-xchange@webmail.nmp.proximus.be>
Hi Fabian,
On Sun 15-03-15 09:34:35, Fabian Frederick wrote:
> > On 14 March 2015 at 07:52 Jan Kara <jack@suse.cz> 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@suse.cz>
SUSE Labs, CR
next prev parent reply other threads:[~2015-03-16 7:46 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-03-10 20:44 [PATCH 1/5 linux-next] udf: remove unused variable in udf_table_free_blocks() Fabian Frederick
2015-03-10 20:44 ` [PATCH 2/5 linux-next] udf: use sector_t for udf_bitmap_prealloc_blocks() return value Fabian Frederick
2015-03-14 6:39 ` Jan Kara
2015-03-10 20:44 ` [PATCH 3/5 linux-next] udf: remove else after return in __load_block_bitmap() Fabian Frederick
2015-03-14 6:39 ` Jan Kara
2015-03-10 20:44 ` [PATCH 4/5 linux-next] udf: rename udf_get_filename() Fabian Frederick
2015-03-14 6:52 ` Jan Kara
2015-03-15 8:34 ` Fabian Frederick
2015-03-16 7:46 ` Jan Kara [this message]
2015-03-16 20:24 ` Fabian Frederick
2015-03-10 20:44 ` [PATCH 5/5 linux-next] udf: remove redundant buffer_head.h includes Fabian Frederick
2015-03-14 6:54 ` Jan Kara
2015-03-14 6:34 ` [PATCH 1/5 linux-next] udf: remove unused variable in udf_table_free_blocks() Jan Kara
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20150316074619.GB4934@quack.suse.cz \
--to=jack@suse.cz \
--cc=fabf@skynet.be \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).