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

[Zhao Mengmeng <zhaomzhao@126.com>]

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.