| Message ID | 20240111142407.2163578-2-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] xfs: check that the mountpoint is actually mounted in _supports_xfs_scrub | expand |
On Thu, Jan 11, 2024 at 03:24:05PM +0100, Christoph Hellwig wrote: > Add a sanity check that the passed in mount point is actually mounted > to guard against actually calling _supports_xfs_scrub before > $SCRATCH_MNT is mounted. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > common/xfs | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/common/xfs b/common/xfs > index f53b33fc5..9db998837 100644 > --- a/common/xfs > +++ b/common/xfs > @@ -649,6 +649,8 @@ _supports_xfs_scrub() > test "$FSTYP" = "xfs" || return 1 > test -x "$XFS_SCRUB_PROG" || return 1 > > + mountpoint $mountpoint >/dev/null || echo "$mountpoint is not mounted" The helper needs to return nonzero on failure, e.g. if ! mountpoint -q $mountpoint; then echo "$mountpoint is not mounted" return 1 fi > + > # Probe for kernel support... > $XFS_IO_PROG -c 'help scrub' 2>&1 | grep -q 'types are:.*probe' || return 1 Like it already does down here. --D > $XFS_IO_PROG -c "scrub probe" "$mountpoint" 2>&1 | grep -q "Inappropriate ioctl" && return 1 > -- > 2.39.2 >
On Thu, Jan 11, 2024 at 09:20:22AM -0800, Darrick J. Wong wrote: > > + mountpoint $mountpoint >/dev/null || echo "$mountpoint is not mounted" > > The helper needs to return nonzero on failure, e.g. > > if ! mountpoint -q $mountpoint; then > echo "$mountpoint is not mounted" > return 1 > fi No, it doesn't.. I actually did exactly that first, but that causes the test to be _notrun instead of reporting the error and thus telling the author that they usage of this helper is wrong.
On Thu, Jan 11, 2024 at 06:25:56PM +0100, Christoph Hellwig wrote: > On Thu, Jan 11, 2024 at 09:20:22AM -0800, Darrick J. Wong wrote: > > > + mountpoint $mountpoint >/dev/null || echo "$mountpoint is not mounted" > > > > The helper needs to return nonzero on failure, e.g. > > > > if ! mountpoint -q $mountpoint; then > > echo "$mountpoint is not mounted" > > return 1 > > fi > > No, it doesn't.. I actually did exactly that first, but that causes the > test to be _notrun instead of reporting the error and thus telling the > author that they usage of this helper is wrong. So below "usage" message won't be gotten either, if a _notrun be called after this helper return 1 . if [ -z "$device" ] || [ -z "$mountpoint" ]; then echo "Usage: _supports_xfs_scrub mountpoint device" return 1 fi If there's not _notrun after that, the message will be gotten I think. So I think the "return 1" makes sense. What do both of you think ? Thanks, Zorro >
On Fri, Jan 12, 2024 at 05:17:02AM +0800, Zorro Lang wrote: > On Thu, Jan 11, 2024 at 06:25:56PM +0100, Christoph Hellwig wrote: > > On Thu, Jan 11, 2024 at 09:20:22AM -0800, Darrick J. Wong wrote: > > > > + mountpoint $mountpoint >/dev/null || echo "$mountpoint is not mounted" > > > > > > The helper needs to return nonzero on failure, e.g. > > > > > > if ! mountpoint -q $mountpoint; then > > > echo "$mountpoint is not mounted" > > > return 1 > > > fi > > > > No, it doesn't.. I actually did exactly that first, but that causes the > > test to be _notrun instead of reporting the error and thus telling the > > author that they usage of this helper is wrong. > > So below "usage" message won't be gotten either, if a _notrun be called > after this helper return 1 . > > if [ -z "$device" ] || [ -z "$mountpoint" ]; then > echo "Usage: _supports_xfs_scrub mountpoint device" > return 1 > fi > > If there's not _notrun after that, the message will be gotten I think. > So I think the "return 1" makes sense. > > What do both of you think ? _fail "\$mountpoint must be mounted to use _require_scratch_xfs_scrub" ? --D > Thanks, > Zorro > > > > >
On Fri, Jan 12, 2024 at 05:17:02AM +0800, Zorro Lang wrote: > > No, it doesn't.. I actually did exactly that first, but that causes the > > test to be _notrun instead of reporting the error and thus telling the > > author that they usage of this helper is wrong. > > So below "usage" message won't be gotten either, if a _notrun be called > after this helper return 1 . True. But for the case reproducing my original error where I just misplaced it it does get shown. > If there's not _notrun after that, the message will be gotten I think. > So I think the "return 1" makes sense. As this point we might as well skip this patch, as it won't be useful.
On Thu, Jan 11, 2024 at 06:17:49PM -0800, Darrick J. Wong wrote: > > If there's not _notrun after that, the message will be gotten I think. > > So I think the "return 1" makes sense. > > > > What do both of you think ? > > _fail "\$mountpoint must be mounted to use _require_scratch_xfs_scrub" ? Yes, that's better. I didn't even remember _fail exists..
diff --git a/common/xfs b/common/xfs index f53b33fc5..9db998837 100644 --- a/common/xfs +++ b/common/xfs @@ -649,6 +649,8 @@ _supports_xfs_scrub() test "$FSTYP" = "xfs" || return 1 test -x "$XFS_SCRUB_PROG" || return 1 + mountpoint $mountpoint >/dev/null || echo "$mountpoint is not mounted" + # Probe for kernel support... $XFS_IO_PROG -c 'help scrub' 2>&1 | grep -q 'types are:.*probe' || return 1 $XFS_IO_PROG -c "scrub probe" "$mountpoint" 2>&1 | grep -q "Inappropriate ioctl" && return 1
Add a sanity check that the passed in mount point is actually mounted to guard against actually calling _supports_xfs_scrub before $SCRATCH_MNT is mounted. Signed-off-by: Christoph Hellwig <hch@lst.de> --- common/xfs | 2 ++ 1 file changed, 2 insertions(+)