LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
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

  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 \
    --subject='Re: [PATCH 4/5 linux-next] udf: rename udf_get_filename()' \
    /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

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).