[PATCH 0/2] fstests: Fix checking of UDF filesystems

[PATCH 0/2] fstests: Fix checking of UDF filesystems

[Jan Kara]

Hello,

these two patches fix the usage of udf_test program in _check_udf_filesystem
so that it can reliably check udf filesystems. It provides it proper arguments
and also unmounts the filesystem prior to running the check.

								Honza

[PATCH 1/2] common: Provide blocksize and ecclength to udf fsck

[Jan Kara]

udf_test program used for verifying filesystem is not able to determine
filesystem blocksize. Provide it in the options together with disabling
ecclength as it is not used on harddrives.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 common/rc | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/common/rc b/common/rc
index 8060c03b7d18..de94e5a4cde4 100644
--- a/common/rc
+++ b/common/rc
@@ -3075,9 +3075,13 @@ _check_udf_filesystem()
     fi
 
     local device=$1
-    local opt_arg=""
+    local blksz=`echo $MKFS_OPTIONS | sed -rn 's/.*-b ?+([0-9]+).*/\1/p'`
+    if [ -z "$blksz" ]; then
+	blksz=512
+    fi
+    local opt_arg="-ecclength 1 -blocksize $blksz"
     if [ $# -eq 2 ]; then
-	opt_arg="-lastvalidblock $(( $2 - 1 ))"
+	opt_arg+=" -lastvalidblock $(( $2 - 1 ))"
     fi
 
     rm -f $seqres.checkfs
-- 
2.35.3

Re: [PATCH 1/2] common: Provide blocksize and ecclength to udf fsck

[David Disseldorp]

Hi Jan,

On Thu, 19 Jan 2023 11:54:02 +0100, Jan Kara wrote:

> udf_test program used for verifying filesystem is not able to determine
> filesystem blocksize. Provide it in the options together with disabling
> ecclength as it is not used on harddrives.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  common/rc | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/common/rc b/common/rc
> index 8060c03b7d18..de94e5a4cde4 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -3075,9 +3075,13 @@ _check_udf_filesystem()
>      fi
>  
>      local device=$1
> -    local opt_arg=""
> +    local blksz=`echo $MKFS_OPTIONS | sed -rn 's/.*-b ?+([0-9]+).*/\1/p'`

It looks as though mkudffs also accepts --blocksize= for this, so it
should also be handled here, IIUC.

Looks fine otherwise.

> +    if [ -z "$blksz" ]; then
> +	blksz=512
> +    fi
> +    local opt_arg="-ecclength 1 -blocksize $blksz"
>      if [ $# -eq 2 ]; then
> -	opt_arg="-lastvalidblock $(( $2 - 1 ))"
> +	opt_arg+=" -lastvalidblock $(( $2 - 1 ))"
>      fi
>  
>      rm -f $seqres.checkfs

Re: [PATCH 1/2] common: Provide blocksize and ecclength to udf fsck

[Jan Kara]

Hello!

On Fri 20-01-23 15:41:23, David Disseldorp wrote:
> On Thu, 19 Jan 2023 11:54:02 +0100, Jan Kara wrote:
> > udf_test program used for verifying filesystem is not able to determine
> > filesystem blocksize. Provide it in the options together with disabling
> > ecclength as it is not used on harddrives.
> > 
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> >  common/rc | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/common/rc b/common/rc
> > index 8060c03b7d18..de94e5a4cde4 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -3075,9 +3075,13 @@ _check_udf_filesystem()
> >      fi
> >  
> >      local device=$1
> > -    local opt_arg=""
> > +    local blksz=`echo $MKFS_OPTIONS | sed -rn 's/.*-b ?+([0-9]+).*/\1/p'`
> 
> It looks as though mkudffs also accepts --blocksize= for this, so it
> should also be handled here, IIUC.

Thanks for review! I've fixed this.

									Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 1/2] common: Provide blocksize and ecclength to udf fsck

[Zorro Lang]

On Fri, Jan 20, 2023 at 04:45:10PM +0100, Jan Kara wrote:
> Hello!
> 
> On Fri 20-01-23 15:41:23, David Disseldorp wrote:
> > On Thu, 19 Jan 2023 11:54:02 +0100, Jan Kara wrote:
> > > udf_test program used for verifying filesystem is not able to determine
> > > filesystem blocksize. Provide it in the options together with disabling
> > > ecclength as it is not used on harddrives.
> > > 
> > > Signed-off-by: Jan Kara <jack@suse.cz>
> > > ---
> > >  common/rc | 8 ++++++--
> > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/common/rc b/common/rc
> > > index 8060c03b7d18..de94e5a4cde4 100644
> > > --- a/common/rc
> > > +++ b/common/rc
> > > @@ -3075,9 +3075,13 @@ _check_udf_filesystem()
> > >      fi
> > >  
> > >      local device=$1
> > > -    local opt_arg=""
> > > +    local blksz=`echo $MKFS_OPTIONS | sed -rn 's/.*-b ?+([0-9]+).*/\1/p'`
> > 
> > It looks as though mkudffs also accepts --blocksize= for this, so it
> > should also be handled here, IIUC.

And _scratch_mkfs_sized need blocksize too. If udf support blocksize parameter,
you might like to take a look at that helper too, due to it always use 4096 for udf
blocksize.

Similar helpers are _scratch_mkfs_geom (maybe not suit for udf) and
_scratch_mkfs_blocksized.

Thanks,
Zorro

> 
> Thanks for review! I've fixed this.
> 
> 									Honza
> -- 
> Jan Kara <jack@suse.com>
> SUSE Labs, CR
> 

Re: [PATCH 1/2] common: Provide blocksize and ecclength to udf fsck

[Jan Kara]

On Sat 21-01-23 03:08:15, Zorro Lang wrote:
> On Fri, Jan 20, 2023 at 04:45:10PM +0100, Jan Kara wrote:
> > Hello!
> > 
> > On Fri 20-01-23 15:41:23, David Disseldorp wrote:
> > > On Thu, 19 Jan 2023 11:54:02 +0100, Jan Kara wrote:
> > > > udf_test program used for verifying filesystem is not able to determine
> > > > filesystem blocksize. Provide it in the options together with disabling
> > > > ecclength as it is not used on harddrives.
> > > > 
> > > > Signed-off-by: Jan Kara <jack@suse.cz>
> > > > ---
> > > >  common/rc | 8 ++++++--
> > > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/common/rc b/common/rc
> > > > index 8060c03b7d18..de94e5a4cde4 100644
> > > > --- a/common/rc
> > > > +++ b/common/rc
> > > > @@ -3075,9 +3075,13 @@ _check_udf_filesystem()
> > > >      fi
> > > >  
> > > >      local device=$1
> > > > -    local opt_arg=""
> > > > +    local blksz=`echo $MKFS_OPTIONS | sed -rn 's/.*-b ?+([0-9]+).*/\1/p'`
> > > 
> > > It looks as though mkudffs also accepts --blocksize= for this, so it
> > > should also be handled here, IIUC.
> 
> And _scratch_mkfs_sized need blocksize too. If udf support blocksize parameter,
> you might like to take a look at that helper too, due to it always use 4096 for udf
> blocksize.
> 
> Similar helpers are _scratch_mkfs_geom (maybe not suit for udf) and
> _scratch_mkfs_blocksized.

Thanks for the tips, I'll have a look into those next.

									Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 2/2] common: Unmount udf filesystem prior checking

[Jan Kara]

_check_udf_filesystem forgot to unmount the filesystem prior to checking
it. That was leading to check failures.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 common/rc | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/common/rc b/common/rc
index de94e5a4cde4..df6f237cabaa 100644
--- a/common/rc
+++ b/common/rc
@@ -3074,6 +3074,12 @@ _check_udf_filesystem()
 	return
     fi
 
+    # Is the filesystem mounted?
+    local type=`_fs_type $device`
+    if [ "$type" = "$FSTYP" ]; then
+	local mountpoint=`_umount_or_remount_ro $device`
+    fi
+
     local device=$1
     local blksz=`echo $MKFS_OPTIONS | sed -rn 's/.*-b ?+([0-9]+).*/\1/p'`
     if [ -z "$blksz" ]; then
@@ -3090,6 +3096,10 @@ _check_udf_filesystem()
 	_udf_test_known_error_filter | \
 	grep -E -iv "Error count:.*[0-9]+.*total occurrences:.*[0-9]+|Warning count:.*[0-9]+.*total occurrences:.*[0-9]+" && \
         echo "Warning UDF Verifier reported errors see $seqres.checkfs." && return 1
+    # Remount the filesystem
+    if [ "$type" = "$FSTYP" ]; then
+	_mount_or_remount_rw "$MOUNT_OPTIONS" $device $mountpoint
+    fi
     return 0
 }
 
-- 
2.35.3

Re: [PATCH 2/2] common: Unmount udf filesystem prior checking

[David Disseldorp]

On Thu, 19 Jan 2023 11:54:03 +0100, Jan Kara wrote:

> _check_udf_filesystem forgot to unmount the filesystem prior to checking
> it. That was leading to check failures.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  common/rc | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/common/rc b/common/rc
> index de94e5a4cde4..df6f237cabaa 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -3074,6 +3074,12 @@ _check_udf_filesystem()
>  	return
>      fi
>  
> +    # Is the filesystem mounted?
> +    local type=`_fs_type $device`
> +    if [ "$type" = "$FSTYP" ]; then
> +	local mountpoint=`_umount_or_remount_ro $device`
> +    fi
> +
>      local device=$1
>      local blksz=`echo $MKFS_OPTIONS | sed -rn 's/.*-b ?+([0-9]+).*/\1/p'`
>      if [ -z "$blksz" ]; then
> @@ -3090,6 +3096,10 @@ _check_udf_filesystem()
>  	_udf_test_known_error_filter | \
>  	grep -E -iv "Error count:.*[0-9]+.*total occurrences:.*[0-9]+|Warning count:.*[0-9]+.*total occurrences:.*[0-9]+" && \
>          echo "Warning UDF Verifier reported errors see $seqres.checkfs." && return 1
> +    # Remount the filesystem
> +    if [ "$type" = "$FSTYP" ]; then
> +	_mount_or_remount_rw "$MOUNT_OPTIONS" $device $mountpoint
> +    fi
>      return 0
>  }
>  

Looks fine.
Reviewed-by: David Disseldorp <ddiss@suse.de>