Re: [PATCH v2 1/3] udf: refactor udf_current_aext() to handle error
From: Jan Kara
Date: Fri Sep 27 2024 - 07:55:34 EST
On Thu 26-09-24 20:07:51, Zhao Mengmeng wrote:
> From: Zhao Mengmeng <zhaomengmeng@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> Suggested-by: Jan Kara <jack@xxxxxxx>
...
> @@ -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@xxxxxxxx>
SUSE Labs, CR