1. Patchew
  2. linux
  3. [PATCH v2 0/3] udf: refactor udf_current_aext()/udf_next_aext()/inode_bmap() to handle error
  4. View patch

[PATCH v2 1/3] udf: refactor udf_current_aext() to handle error

Zhao Mengmeng posted 3 patches 2 months ago
There is a newer version of this series
[PATCH v2 1/3] udf: refactor udf_current_aext() to handle error
Posted by Zhao Mengmeng 2 months ago 
From: Zhao Mengmeng <zhaomengmeng@kylinos.cn>

As Jan suggested in links below, refactor udf_current_aext() to
differentiate between error and "hit EOF", it now takes pointer to etype
to store the extent type, return 0 when get etype success; return -ENODATA
when hit EOF; return -EINVAL when i_alloc_type invalid. Add two macroes to
test return value.

Link: https://lore.kernel.org/all/20240912111235.6nr3wuqvktecy3vh@quack3/
Signed-off-by: Zhao Mengmeng <zhaomengmeng@kylinos.cn>
Suggested-by: Jan Kara <jack@suse.cz>
---
 fs/udf/inode.c    | 37 +++++++++++++++++++++++--------------
 fs/udf/truncate.c | 10 ++++++++--
 fs/udf/udfdecl.h  |  8 ++++++--
 3 files changed, 37 insertions(+), 18 deletions(-)

diff --git a/fs/udf/inode.c b/fs/udf/inode.c
index 4726a4d014b6..3be59aa8d4fd 100644
--- a/fs/udf/inode.c
+++ b/fs/udf/inode.c
@@ -1955,6 +1955,7 @@ int udf_setup_indirect_aext(struct inode *inode, udf_pblk_t block,
 	struct extent_position nepos;
 	struct kernel_lb_addr neloc;
 	int ver, adsize;
+	int err = 0;
 
 	if (UDF_I(inode)->i_alloc_type == ICBTAG_FLAG_AD_SHORT)
 		adsize = sizeof(struct short_ad);
@@ -1999,10 +2000,12 @@ int udf_setup_indirect_aext(struct inode *inode, udf_pblk_t block,
 	if (epos->offset + adsize > sb->s_blocksize) {
 		struct kernel_lb_addr cp_loc;
 		uint32_t cp_len;
-		int cp_type;
+		int8_t cp_type;
 
 		epos->offset -= adsize;
-		cp_type = udf_current_aext(inode, epos, &cp_loc, &cp_len, 0);
+		err = udf_current_aext(inode, epos, &cp_loc, &cp_len, &cp_type, 0);
+		if (err < 0)
+			goto err_out;
 		cp_len |= ((uint32_t)cp_type) << 30;
 
 		__udf_add_aext(inode, &nepos, &cp_loc, cp_len, 1);
@@ -2017,6 +2020,9 @@ int udf_setup_indirect_aext(struct inode *inode, udf_pblk_t block,
 	*epos = nepos;
 
 	return 0;
+err_out:
+	brelse(bh);
+	return err;
 }
 
 /*
@@ -2167,9 +2173,12 @@ int8_t udf_next_aext(struct inode *inode, struct extent_position *epos,
 {
 	int8_t etype;
 	unsigned int indirections = 0;
+	int err = 0;
+
+	while ((err = udf_current_aext(inode, epos, eloc, elen, &etype, inc))) {
+		if (err || etype != (EXT_NEXT_EXTENT_ALLOCDESCS >> 30))
+			break;
 
-	while ((etype = udf_current_aext(inode, epos, eloc, elen, inc)) ==
-	       (EXT_NEXT_EXTENT_ALLOCDESCS >> 30)) {
 		udf_pblk_t block;
 
 		if (++indirections > UDF_MAX_INDIR_EXTS) {
@@ -2190,14 +2199,14 @@ int8_t udf_next_aext(struct inode *inode, struct extent_position *epos,
 		}
 	}
 
-	return etype;
+	return !err ? etype : -1;
 }
 
-int8_t udf_current_aext(struct inode *inode, struct extent_position *epos,
-			struct kernel_lb_addr *eloc, uint32_t *elen, int inc)
+int udf_current_aext(struct inode *inode, struct extent_position *epos,
+		     struct kernel_lb_addr *eloc, uint32_t *elen, int8_t *etype,
+		     int inc)
 {
 	int alen;
-	int8_t etype;
 	uint8_t *ptr;
 	struct short_ad *sad;
 	struct long_ad *lad;
@@ -2224,8 +2233,8 @@ int8_t udf_current_aext(struct inode *inode, struct extent_position *epos,
 	case ICBTAG_FLAG_AD_SHORT:
 		sad = udf_get_fileshortad(ptr, alen, &epos->offset, inc);
 		if (!sad)
-			return -1;
-		etype = le32_to_cpu(sad->extLength) >> 30;
+			return -ENODATA;
+		*etype = le32_to_cpu(sad->extLength) >> 30;
 		eloc->logicalBlockNum = le32_to_cpu(sad->extPosition);
 		eloc->partitionReferenceNum =
 				iinfo->i_location.partitionReferenceNum;
@@ -2234,17 +2243,17 @@ int8_t udf_current_aext(struct inode *inode, struct extent_position *epos,
 	case ICBTAG_FLAG_AD_LONG:
 		lad = udf_get_filelongad(ptr, alen, &epos->offset, inc);
 		if (!lad)
-			return -1;
-		etype = le32_to_cpu(lad->extLength) >> 30;
+			return -ENODATA;
+		*etype = le32_to_cpu(lad->extLength) >> 30;
 		*eloc = lelb_to_cpu(lad->extLocation);
 		*elen = le32_to_cpu(lad->extLength) & UDF_EXTENT_LENGTH_MASK;
 		break;
 	default:
 		udf_debug("alloc_type = %u unsupported\n", iinfo->i_alloc_type);
-		return -1;
+		return -EINVAL;
 	}
 
-	return etype;
+	return 0;
 }
 
 static int udf_insert_aext(struct inode *inode, struct extent_position epos,
diff --git a/fs/udf/truncate.c b/fs/udf/truncate.c
index a686c10fd709..16db2613401d 100644
--- a/fs/udf/truncate.c
+++ b/fs/udf/truncate.c
@@ -188,6 +188,7 @@ int udf_truncate_extents(struct inode *inode)
 	loff_t byte_offset;
 	int adsize;
 	struct udf_inode_info *iinfo = UDF_I(inode);
+	int err = 0;
 
 	if (iinfo->i_alloc_type == ICBTAG_FLAG_AD_SHORT)
 		adsize = sizeof(struct short_ad);
@@ -217,8 +218,8 @@ int udf_truncate_extents(struct inode *inode)
 	else
 		lenalloc -= sizeof(struct allocExtDesc);
 
-	while ((etype = udf_current_aext(inode, &epos, &eloc,
-					 &elen, 0)) != -1) {
+	while (!(err = udf_current_aext(inode, &epos, &eloc, &elen, &etype,
+					0))) {
 		if (etype == (EXT_NEXT_EXTENT_ALLOCDESCS >> 30)) {
 			udf_write_aext(inode, &epos, &neloc, nelen, 0);
 			if (indirect_ext_len) {
@@ -253,6 +254,11 @@ int udf_truncate_extents(struct inode *inode)
 		}
 	}
 
+	if (UDF_EXT_ERR(err)) {
+		brelse(epos.bh);
+		return err;
+	}
+
 	if (indirect_ext_len) {
 		BUG_ON(!epos.bh);
 		udf_free_blocks(sb, NULL, &epos.block, 0, indirect_ext_len);
diff --git a/fs/udf/udfdecl.h b/fs/udf/udfdecl.h
index 88692512a466..a902652450dd 100644
--- a/fs/udf/udfdecl.h
+++ b/fs/udf/udfdecl.h
@@ -43,6 +43,9 @@ extern __printf(3, 4) void _udf_warn(struct super_block *sb,
 #define UDF_NAME_LEN		254
 #define UDF_NAME_LEN_CS0	255
 
+#define UDF_EXT_EOF(err)        ((err) == -ENODATA)
+#define UDF_EXT_ERR(err)        (((err) < 0) && (!UDF_EXT_EOF(err)))
+
 static inline size_t udf_file_entry_alloc_offset(struct inode *inode)
 {
 	struct udf_inode_info *iinfo = UDF_I(inode);
@@ -171,8 +174,9 @@ extern void udf_write_aext(struct inode *, struct extent_position *,
 extern int8_t udf_delete_aext(struct inode *, struct extent_position);
 extern int8_t udf_next_aext(struct inode *, struct extent_position *,
 			    struct kernel_lb_addr *, uint32_t *, int);
-extern int8_t udf_current_aext(struct inode *, struct extent_position *,
-			       struct kernel_lb_addr *, uint32_t *, int);
+extern int udf_current_aext(struct inode *inode, struct extent_position *epos,
+			    struct kernel_lb_addr *eloc, uint32_t *elen,
+			    int8_t *etype, int inc);
 extern void udf_update_extra_perms(struct inode *inode, umode_t mode);
 
 /* misc.c */
-- 
2.43.0
Re: [PATCH v2 1/3] udf: refactor udf_current_aext() to handle error
Posted by Jan Kara 2 months ago 
On Thu 26-09-24 20:07:51, Zhao Mengmeng wrote:
> From: Zhao Mengmeng <zhaomengmeng@kylinos.cn>
> 
> As Jan suggested in links below, refactor udf_current_aext() to
> differentiate between error and "hit EOF", it now takes pointer to etype
> to store the extent type, return 0 when get etype success; return -ENODATA
> when hit EOF; return -EINVAL when i_alloc_type invalid. Add two macroes to
> test return value.
> 
> Link: https://lore.kernel.org/all/20240912111235.6nr3wuqvktecy3vh@quack3/
> Signed-off-by: Zhao Mengmeng <zhaomengmeng@kylinos.cn>
> Suggested-by: Jan Kara <jack@suse.cz>
...
> @@ -2167,9 +2173,12 @@ int8_t udf_next_aext(struct inode *inode, struct extent_position *epos,
>  {
>  	int8_t etype;
>  	unsigned int indirections = 0;
> +	int err = 0;
> +
> +	while ((err = udf_current_aext(inode, epos, eloc, elen, &etype, inc))) {
> +		if (err || etype != (EXT_NEXT_EXTENT_ALLOCDESCS >> 30))
> +			break;
>  
> -	while ((etype = udf_current_aext(inode, epos, eloc, elen, inc)) ==
> -	       (EXT_NEXT_EXTENT_ALLOCDESCS >> 30)) {

This looks wrong. If udf_current_aext() succeeds, you'll immediately abort
the loop now. I'd rather code this as:

	while (1) {
		err = udf_current_aext(inode, epos, eloc, elen, &etype, inc);
		if (err || etype != (EXT_NEXT_EXTENT_ALLOCDESCS >> 30))
			break;
		...
	}

> diff --git a/fs/udf/udfdecl.h b/fs/udf/udfdecl.h
> index 88692512a466..a902652450dd 100644
> --- a/fs/udf/udfdecl.h
> +++ b/fs/udf/udfdecl.h
> @@ -43,6 +43,9 @@ extern __printf(3, 4) void _udf_warn(struct super_block *sb,
>  #define UDF_NAME_LEN		254
>  #define UDF_NAME_LEN_CS0	255
>  
> +#define UDF_EXT_EOF(err)        ((err) == -ENODATA)
> +#define UDF_EXT_ERR(err)        (((err) < 0) && (!UDF_EXT_EOF(err)))
> +

So I agree the explicit ENODATA checks are a bit ugly but these macros
aren't really much better. How about the following calling convention:

On error, ret < 0, on EOF ret == 0, on success ret == 1. This is a similar
convention as e.g. for read(2) so it is well understood and easy test for
various combinations.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR
Re: [PATCH v2 1/3] udf: refactor udf_current_aext() to handle error
Posted by Zhao Mengmeng 2 months ago 
On 2024/9/27 19:55, Jan Kara wrote:
> On Thu 26-09-24 20:07:51, Zhao Mengmeng wrote:
>> From: Zhao Mengmeng <zhaomengmeng@kylinos.cn>
>>
>> As Jan suggested in links below, refactor udf_current_aext() to
>> differentiate between error and "hit EOF", it now takes pointer to etype
>> to store the extent type, return 0 when get etype success; return -ENODATA
>> when hit EOF; return -EINVAL when i_alloc_type invalid. Add two macroes to
>> test return value.
>>
>> Link: https://lore.kernel.org/all/20240912111235.6nr3wuqvktecy3vh@quack3/
>> Signed-off-by: Zhao Mengmeng <zhaomengmeng@kylinos.cn>
>> Suggested-by: Jan Kara <jack@suse.cz>
> ...
>> @@ -2167,9 +2173,12 @@ int8_t udf_next_aext(struct inode *inode, struct extent_position *epos,
>>  {
>>  	int8_t etype;
>>  	unsigned int indirections = 0;
>> +	int err = 0;
>> +
>> +	while ((err = udf_current_aext(inode, epos, eloc, elen, &etype, inc))) {
>> +		if (err || etype != (EXT_NEXT_EXTENT_ALLOCDESCS >> 30))
>> +			break;
>>  
>> -	while ((etype = udf_current_aext(inode, epos, eloc, elen, inc)) ==
>> -	       (EXT_NEXT_EXTENT_ALLOCDESCS >> 30)) {
> 
> This looks wrong. If udf_current_aext() succeeds, you'll immediately abort
> the loop now. I'd rather code this as:
> 
> 	while (1) {
> 		err = udf_current_aext(inode, epos, eloc, elen, &etype, inc);
> 		if (err || etype != (EXT_NEXT_EXTENT_ALLOCDESCS >> 30))
> 			break;
> 		...
> 	}

Yes, you are right. I forget the return check, should be:
	while (!(err = udf_current_aext(inode, epos, eloc, elen, &etype, inc))) {
		if (err || etype != (EXT_NEXT_EXTENT_ALLOCDESCS >> 30))
			break;
>> diff --git a/fs/udf/udfdecl.h b/fs/udf/udfdecl.h
>> index 88692512a466..a902652450dd 100644
>> --- a/fs/udf/udfdecl.h
>> +++ b/fs/udf/udfdecl.h
>> @@ -43,6 +43,9 @@ extern __printf(3, 4) void _udf_warn(struct super_block *sb,
>>  #define UDF_NAME_LEN		254
>>  #define UDF_NAME_LEN_CS0	255
>>  
>> +#define UDF_EXT_EOF(err)        ((err) == -ENODATA)
>> +#define UDF_EXT_ERR(err)        (((err) < 0) && (!UDF_EXT_EOF(err)))
>> +
> 
> So I agree the explicit ENODATA checks are a bit ugly but these macros
> aren't really much better. How about the following calling convention:
> 
> On error, ret < 0, on EOF ret == 0, on success ret == 1. This is a similar
> convention as e.g. for read(2) so it is well understood and easy test for
> various combinations.

No problem, I will add some comments on these three functions do it in V3.

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.