From: Jan Kara <jack@suse.cz>
To: Fabian Frederick <fabf@skynet.be>
Cc: linux-kernel@vger.kernel.org, Jan Kara <jack@suse.cz>
Subject: Re: [PATCH V2 8/9 linux-next] udf: propagate udf_get_filename() errors
Date: Thu, 9 Apr 2015 11:27:46 +0200 [thread overview]
Message-ID: <20150409092746.GI18044@quack.suse.cz> (raw)
In-Reply-To: <1428521039-18491-9-git-send-email-fabf@skynet.be>
[-- Attachment #1: Type: text/plain, Size: 5467 bytes --]
On Wed 08-04-15 21:23:58, Fabian Frederick wrote:
> -Return udf_get_filename() error from udf_readdir()
> -Return -ENOMEM from udf_find_entry() when unable to allocate
> fname and udf_get_filename() error
> -udf_find_entry() callsites are also updated:
> udf_lookup(), udf_rmdir(), udf_unlink() and udf_rename()
>
> Suggested-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Fabian Frederick <fabf@skynet.be>
Thanks. I've merged this patch with slight modification -
udf_find_entry() should return -EIO instead of -EINVAL in all the cases.
I've also folded the documentation of udf_find_entry() into this patch.
Attached is the result.
Honza
> ---
> V2:
> -Don't set error in udf_readdir()
> -Improve code flow
> -Merge if(nfi) if (!inode) in udf_rename()
>
> fs/udf/namei.c | 70 ++++++++++++++++++++++++++++++++++++++++++----------------
> 1 file changed, 51 insertions(+), 19 deletions(-)
>
> diff --git a/fs/udf/namei.c b/fs/udf/namei.c
> index dd648b7..891c067 100644
> --- a/fs/udf/namei.c
> +++ b/fs/udf/namei.c
> @@ -167,8 +167,11 @@ static struct fileIdentDesc *udf_find_entry(struct inode *dir,
> fibh->soffset = fibh->eoffset = f_pos & (sb->s_blocksize - 1);
> if (dinfo->i_alloc_type != ICBTAG_FLAG_AD_IN_ICB) {
> if (inode_bmap(dir, f_pos >> sb->s_blocksize_bits, &epos,
> - &eloc, &elen, &offset) != (EXT_RECORDED_ALLOCATED >> 30))
> + &eloc, &elen, &offset) != (EXT_RECORDED_ALLOCATED >> 30)) {
> + fi = ERR_PTR(-EINVAL);
> goto out_err;
> + }
> +
> block = udf_get_lb_pblock(sb, &eloc, offset);
> if ((++offset << sb->s_blocksize_bits) < elen) {
> if (dinfo->i_alloc_type == ICBTAG_FLAG_AD_SHORT)
> @@ -179,19 +182,25 @@ static struct fileIdentDesc *udf_find_entry(struct inode *dir,
> offset = 0;
>
> fibh->sbh = fibh->ebh = udf_tread(sb, block);
> - if (!fibh->sbh)
> + if (!fibh->sbh) {
> + fi = ERR_PTR(-EINVAL);
> goto out_err;
> + }
> }
>
> fname = kmalloc(UDF_NAME_LEN, GFP_NOFS);
> - if (!fname)
> + if (!fname) {
> + fi = ERR_PTR(-ENOMEM);
> goto out_err;
> + }
>
> while (f_pos < size) {
> fi = udf_fileident_read(dir, &f_pos, fibh, cfi, &epos, &eloc,
> &elen, &offset);
> - if (!fi)
> + if (!fi) {
> + fi = ERR_PTR(-EINVAL);
> goto out_err;
> + }
>
> liu = le16_to_cpu(cfi->lengthOfImpUse);
> lfi = cfi->lengthFileIdent;
> @@ -234,13 +243,18 @@ static struct fileIdentDesc *udf_find_entry(struct inode *dir,
> continue;
>
> flen = udf_get_filename(sb, nameptr, lfi, fname, UDF_NAME_LEN);
> + if (flen < 0) {
> + fi = ERR_PTR(flen);
> + goto out_err;
> + }
> +
> if ((flen >= 0) && udf_match(flen, fname, child->len,
> child->name))
> goto out_ok;
> }
>
> -out_err:
> fi = NULL;
> +out_err:
> if (fibh->sbh != fibh->ebh)
> brelse(fibh->ebh);
> brelse(fibh->sbh);
> @@ -257,6 +271,7 @@ static struct dentry *udf_lookup(struct inode *dir, struct dentry *dentry,
> struct inode *inode = NULL;
> struct fileIdentDesc cfi;
> struct udf_fileident_bh fibh;
> + struct fileIdentDesc *fi;
>
> if (dentry->d_name.len > UDF_NAME_LEN - 2)
> return ERR_PTR(-ENAMETOOLONG);
> @@ -276,7 +291,11 @@ static struct dentry *udf_lookup(struct inode *dir, struct dentry *dentry,
> } else
> #endif /* UDF_RECOVERY */
>
> - if (udf_find_entry(dir, &dentry->d_name, &fibh, &cfi)) {
> + fi = udf_find_entry(dir, &dentry->d_name, &fibh, &cfi);
> + if (IS_ERR(fi))
> + return ERR_CAST(fi);
> +
> + if (fi) {
> struct kernel_lb_addr loc;
>
> if (fibh.sbh != fibh.ebh)
> @@ -774,8 +793,11 @@ static int udf_rmdir(struct inode *dir, struct dentry *dentry)
>
> retval = -ENOENT;
> fi = udf_find_entry(dir, &dentry->d_name, &fibh, &cfi);
> - if (!fi)
> + if (IS_ERR_OR_NULL(fi)) {
> + if (fi)
> + retval = PTR_ERR(fi);
> goto out;
> + }
>
> retval = -EIO;
> tloc = lelb_to_cpu(cfi.icb.extLocation);
> @@ -817,8 +839,12 @@ static int udf_unlink(struct inode *dir, struct dentry *dentry)
>
> retval = -ENOENT;
> fi = udf_find_entry(dir, &dentry->d_name, &fibh, &cfi);
> - if (!fi)
> +
> + if (IS_ERR_OR_NULL(fi)) {
> + if (fi)
> + retval = PTR_ERR(fi);
> goto out;
> + }
>
> retval = -EIO;
> tloc = lelb_to_cpu(cfi.icb.extLocation);
> @@ -1047,24 +1073,30 @@ static int udf_rename(struct inode *old_dir, struct dentry *old_dentry,
> struct udf_inode_info *old_iinfo = UDF_I(old_inode);
>
> ofi = udf_find_entry(old_dir, &old_dentry->d_name, &ofibh, &ocfi);
> - if (ofi) {
> - if (ofibh.sbh != ofibh.ebh)
> - brelse(ofibh.ebh);
> - brelse(ofibh.sbh);
> + if (IS_ERR(ofi)) {
> + retval = PTR_ERR(ofi);
> + goto end_rename;
> }
> +
> + if (ofibh.sbh != ofibh.ebh)
> + brelse(ofibh.ebh);
> +
> + brelse(ofibh.sbh);
> tloc = lelb_to_cpu(ocfi.icb.extLocation);
> if (!ofi || udf_get_lb_pblock(old_dir->i_sb, &tloc, 0)
> != old_inode->i_ino)
> goto end_rename;
>
> nfi = udf_find_entry(new_dir, &new_dentry->d_name, &nfibh, &ncfi);
> - if (nfi) {
> - if (!new_inode) {
> - if (nfibh.sbh != nfibh.ebh)
> - brelse(nfibh.ebh);
> - brelse(nfibh.sbh);
> - nfi = NULL;
> - }
> + if (IS_ERR(nfi)) {
> + retval = PTR_ERR(nfi);
> + goto end_rename;
> + }
> + if (nfi && !new_inode) {
> + if (nfibh.sbh != nfibh.ebh)
> + brelse(nfibh.ebh);
> + brelse(nfibh.sbh);
> + nfi = NULL;
> }
> if (S_ISDIR(old_inode->i_mode)) {
> int offset = udf_ext0_offset(old_inode);
> --
> 1.9.1
>
--
Jan Kara <jack@suse.cz>
SUSE Labs, CR
[-- Attachment #2: 0001-udf-Return-error-from-udf_find_entry.patch --]
[-- Type: text/x-patch, Size: 6229 bytes --]
>From d0cf0af6334553955107a9136f9da85c2e5dfd73 Mon Sep 17 00:00:00 2001
From: Fabian Frederick <fabf@skynet.be>
Date: Wed, 8 Apr 2015 21:23:58 +0200
Subject: [PATCH] udf: Return error from udf_find_entry()
Return appropriate error from udf_find_entry() instead of just NULL.
That way we can distinguish the fact that some error happened when
looking up filename (and return error to userspace) from the fact that
we just didn't find the filename. Also update callers of
udf_find_entry() accordingly.
[JK: Improved udf_find_entry() documentation]
Suggested-by: Jan Kara <jack@suse.cz>
Signed-off-by: Fabian Frederick <fabf@skynet.be>
Signed-off-by: Jan Kara <jack@suse.cz>
---
fs/udf/namei.c | 91 +++++++++++++++++++++++++++++++++++++++++++++-------------
1 file changed, 71 insertions(+), 20 deletions(-)
diff --git a/fs/udf/namei.c b/fs/udf/namei.c
index 5e833bd1c4e9..5767c36d5652 100644
--- a/fs/udf/namei.c
+++ b/fs/udf/namei.c
@@ -138,6 +138,25 @@ int udf_write_fi(struct inode *inode, struct fileIdentDesc *cfi,
return 0;
}
+/**
+ * udf_find_entry - find entry in given directory.
+ *
+ * @dir: directory inode to search in
+ * @child: qstr of the name
+ * @fibh: buffer head / inode with file identifier descriptor we found
+ * @cfi: found file identifier descriptor with given name
+ *
+ * This function searches in the directory @dir for a file name @child. When
+ * found, @fibh points to the buffer head(s) (bh is NULL for in ICB
+ * directories) containing the file identifier descriptor (FID). In that case
+ * the function returns pointer to the FID in the buffer or inode - but note
+ * that FID may be split among two buffers (blocks) so accessing it via that
+ * pointer isn't easily possible. This pointer can be used only as an iterator
+ * for other directory manipulation functions. For inspection of the FID @cfi
+ * can be used - the found FID is copied there.
+ *
+ * Returns pointer to FID, NULL when nothing found, or error code.
+ */
static struct fileIdentDesc *udf_find_entry(struct inode *dir,
const struct qstr *child,
struct udf_fileident_bh *fibh,
@@ -167,8 +186,11 @@ static struct fileIdentDesc *udf_find_entry(struct inode *dir,
fibh->soffset = fibh->eoffset = f_pos & (sb->s_blocksize - 1);
if (dinfo->i_alloc_type != ICBTAG_FLAG_AD_IN_ICB) {
if (inode_bmap(dir, f_pos >> sb->s_blocksize_bits, &epos,
- &eloc, &elen, &offset) != (EXT_RECORDED_ALLOCATED >> 30))
+ &eloc, &elen, &offset) != (EXT_RECORDED_ALLOCATED >> 30)) {
+ fi = ERR_PTR(-EIO);
goto out_err;
+ }
+
block = udf_get_lb_pblock(sb, &eloc, offset);
if ((++offset << sb->s_blocksize_bits) < elen) {
if (dinfo->i_alloc_type == ICBTAG_FLAG_AD_SHORT)
@@ -179,19 +201,25 @@ static struct fileIdentDesc *udf_find_entry(struct inode *dir,
offset = 0;
fibh->sbh = fibh->ebh = udf_tread(sb, block);
- if (!fibh->sbh)
+ if (!fibh->sbh) {
+ fi = ERR_PTR(-EIO);
goto out_err;
+ }
}
fname = kmalloc(UDF_NAME_LEN, GFP_NOFS);
- if (!fname)
+ if (!fname) {
+ fi = ERR_PTR(-ENOMEM);
goto out_err;
+ }
while (f_pos < size) {
fi = udf_fileident_read(dir, &f_pos, fibh, cfi, &epos, &eloc,
&elen, &offset);
- if (!fi)
+ if (!fi) {
+ fi = ERR_PTR(-EIO);
goto out_err;
+ }
liu = le16_to_cpu(cfi->lengthOfImpUse);
lfi = cfi->lengthFileIdent;
@@ -234,12 +262,17 @@ static struct fileIdentDesc *udf_find_entry(struct inode *dir,
continue;
flen = udf_get_filename(sb, nameptr, lfi, fname, UDF_NAME_LEN);
- if (flen > 0 && udf_match(flen, fname, child->len, child->name))
+ if (flen < 0) {
+ fi = ERR_PTR(flen);
+ goto out_err;
+ }
+
+ if (udf_match(flen, fname, child->len, child->name))
goto out_ok;
}
-out_err:
fi = NULL;
+out_err:
if (fibh->sbh != fibh->ebh)
brelse(fibh->ebh);
brelse(fibh->sbh);
@@ -256,6 +289,7 @@ static struct dentry *udf_lookup(struct inode *dir, struct dentry *dentry,
struct inode *inode = NULL;
struct fileIdentDesc cfi;
struct udf_fileident_bh fibh;
+ struct fileIdentDesc *fi;
if (dentry->d_name.len > UDF_NAME_LEN - 2)
return ERR_PTR(-ENAMETOOLONG);
@@ -275,7 +309,11 @@ static struct dentry *udf_lookup(struct inode *dir, struct dentry *dentry,
} else
#endif /* UDF_RECOVERY */
- if (udf_find_entry(dir, &dentry->d_name, &fibh, &cfi)) {
+ fi = udf_find_entry(dir, &dentry->d_name, &fibh, &cfi);
+ if (IS_ERR(fi))
+ return ERR_CAST(fi);
+
+ if (fi) {
struct kernel_lb_addr loc;
if (fibh.sbh != fibh.ebh)
@@ -774,8 +812,11 @@ static int udf_rmdir(struct inode *dir, struct dentry *dentry)
retval = -ENOENT;
fi = udf_find_entry(dir, &dentry->d_name, &fibh, &cfi);
- if (!fi)
+ if (IS_ERR_OR_NULL(fi)) {
+ if (fi)
+ retval = PTR_ERR(fi);
goto out;
+ }
retval = -EIO;
tloc = lelb_to_cpu(cfi.icb.extLocation);
@@ -817,8 +858,12 @@ static int udf_unlink(struct inode *dir, struct dentry *dentry)
retval = -ENOENT;
fi = udf_find_entry(dir, &dentry->d_name, &fibh, &cfi);
- if (!fi)
+
+ if (IS_ERR_OR_NULL(fi)) {
+ if (fi)
+ retval = PTR_ERR(fi);
goto out;
+ }
retval = -EIO;
tloc = lelb_to_cpu(cfi.icb.extLocation);
@@ -1049,24 +1094,30 @@ static int udf_rename(struct inode *old_dir, struct dentry *old_dentry,
struct udf_inode_info *old_iinfo = UDF_I(old_inode);
ofi = udf_find_entry(old_dir, &old_dentry->d_name, &ofibh, &ocfi);
- if (ofi) {
- if (ofibh.sbh != ofibh.ebh)
- brelse(ofibh.ebh);
- brelse(ofibh.sbh);
+ if (IS_ERR(ofi)) {
+ retval = PTR_ERR(ofi);
+ goto end_rename;
}
+
+ if (ofibh.sbh != ofibh.ebh)
+ brelse(ofibh.ebh);
+
+ brelse(ofibh.sbh);
tloc = lelb_to_cpu(ocfi.icb.extLocation);
if (!ofi || udf_get_lb_pblock(old_dir->i_sb, &tloc, 0)
!= old_inode->i_ino)
goto end_rename;
nfi = udf_find_entry(new_dir, &new_dentry->d_name, &nfibh, &ncfi);
- if (nfi) {
- if (!new_inode) {
- if (nfibh.sbh != nfibh.ebh)
- brelse(nfibh.ebh);
- brelse(nfibh.sbh);
- nfi = NULL;
- }
+ if (IS_ERR(nfi)) {
+ retval = PTR_ERR(nfi);
+ goto end_rename;
+ }
+ if (nfi && !new_inode) {
+ if (nfibh.sbh != nfibh.ebh)
+ brelse(nfibh.ebh);
+ brelse(nfibh.sbh);
+ nfi = NULL;
}
if (S_ISDIR(old_inode->i_mode)) {
int offset = udf_ext0_offset(old_inode);
--
2.1.4
next prev parent reply other threads:[~2015-04-09 9:28 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-04-08 19:23 [PATCH V2 0/9 linux-next] udf: improve error management Fabian Frederick
2015-04-08 19:23 ` [PATCH V2 1/9 linux-next] udf: udf_get_filename(): return -ENOMEM when allocation fails Fabian Frederick
2015-04-09 8:21 ` Jan Kara
2015-04-08 19:23 ` [PATCH V2 2/9 linux-next] udf: remove unnecessary test in udf_build_ustr_exact() Fabian Frederick
2015-04-09 8:25 ` Jan Kara
2015-04-08 19:23 ` [PATCH V2 3/9 linux-next] udf: unicode: update function name in comments Fabian Frederick
2015-04-09 8:26 ` Jan Kara
2015-04-08 19:23 ` [PATCH V2 4/9 linux-next] udf: improve error management in udf_CS0toUTF8() Fabian Frederick
2015-04-09 8:36 ` Jan Kara
2015-04-09 11:53 ` Jan Kara
2015-04-08 19:23 ` [PATCH V2 5/9 linux-next] udf: improve error management in udf_CS0toNLS() Fabian Frederick
2015-04-09 8:37 ` Jan Kara
2015-04-09 11:54 ` Jan Kara
2015-04-08 19:23 ` [PATCH V2 6/9 linux-next] udf: bug on exotic flag in udf_get_filename() Fabian Frederick
2015-04-09 8:39 ` Jan Kara
2015-04-08 19:23 ` [PATCH V2 7/9 linux-next] udf: return error when newIndex is 0 in udf_translate_to_linux() Fabian Frederick
2015-04-09 8:51 ` Jan Kara
2015-04-09 11:55 ` Jan Kara
2015-04-08 19:23 ` [PATCH V2 8/9 linux-next] udf: propagate udf_get_filename() errors Fabian Frederick
2015-04-09 9:27 ` Jan Kara [this message]
2015-04-08 19:23 ` [PATCH V2 9/9 linux-next] udf: add function definition for udf_find_entry() Fabian Frederick
2015-04-09 9:26 ` 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=20150409092746.GI18044@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
Powered by JetHome