| Message ID | 20221212230820.3466-1-ddiss@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] common/attr: require xfs_spaceman for xfs acl_get_max | expand |
On Tue, Dec 13, 2022 at 12:08:18AM +0100, David Disseldorp wrote: > For xfs, _acl_get_max() calls xfs_info which in turn calls > "xfs_spaceman -c info ...". > > Without this change, xfs_spaceman absence results in: > --- tests/generic/026.out > +++ /home/david/xfstests/results//generic/026.out.bad > @@ -1,9 +1,11 @@ > QA output created by 026 > +/usr/sbin/xfs_info: line 44: xfs_spaceman: command not found Hmm.. That's weird, both xfs_info and xfs_spaceman are from xfsprogs package. If someone xfsprogs version doesn't hasve provide xfs_spaceman, why its xfs_info would like to call that? What xfsprogs do you use? Thanks, Zorro > > === Test out large ACLs === > +/usr/sbin/xfs_info: line 44: xfs_spaceman: command not found > 1 below acl max > acl max > ... > > Signed-off-by: David Disseldorp <ddiss@suse.de> > --- > common/attr | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/common/attr b/common/attr > index cce4d1b2..a94c5e4b 100644 > --- a/common/attr > +++ b/common/attr > @@ -57,6 +57,9 @@ _acl_get_max() > > _require_acl_get_max() > { > + # _acl_get_max -> xfs_info -> xfs_spaceman > + [ "$FSTYP" == "xfs" ] && _require_xfs_spaceman_command "info" > + > if [ $(_acl_get_max) -eq 0 ]; then > _notrun "$FSTYP does not define maximum ACL count" > fi > -- > 2.35.3 >
On Tue, 13 Dec 2022 22:41:52 +0800, Zorro Lang wrote: > Hmm.. That's weird, both xfs_info and xfs_spaceman are from xfsprogs > package. If someone xfsprogs version doesn't hasve provide xfs_spaceman, > why its xfs_info would like to call that? What xfsprogs do you use? I have a hacky bunch of scripts that use Dracut to generate an initramfs image with minimal dependencies for xfstests alongside the test kernel: https://github.com/rapido-linux/rapido The rapido dependency list is now fixed, but I figured that this change makes sense given the other scattered _require_X checks for xfsprogs binaries. Feel free to drop this change if you don't agree. Cheers, David
On Tue, Dec 13, 2022 at 04:32:34PM +0100, David Disseldorp wrote: > On Tue, 13 Dec 2022 22:41:52 +0800, Zorro Lang wrote: > > > Hmm.. That's weird, both xfs_info and xfs_spaceman are from xfsprogs > > package. If someone xfsprogs version doesn't hasve provide xfs_spaceman, > > why its xfs_info would like to call that? What xfsprogs do you use? > > I have a hacky bunch of scripts that use Dracut to generate an > initramfs image with minimal dependencies for xfstests alongside the > test kernel: https://github.com/rapido-linux/rapido > > The rapido dependency list is now fixed, but I figured that this change > makes sense given the other scattered _require_X checks for xfsprogs > binaries. Feel free to drop this change if you don't agree. We should let xfsprogs decide how xfs_info works. xfs_info doesn't always depends on xfs_spaceman, old xfs_info might use xfs_db or other things to get xfs info, and I don't know if it'll be changed to use other command in the future again. So this change will cause all cases which call _require_acl_get_max will notrun on old (maybe future) xfsprogs which doesn't has xfs_spaceman "info" command. I think that's not what we want, so I'd like to drop this patch. Thanks for your understanding. Thanks, Zorro > > Cheers, David >
On Wed, 14 Dec 2022 02:25:31 +0800, Zorro Lang wrote: > We should let xfsprogs decide how xfs_info works. xfs_info doesn't always > depends on xfs_spaceman, old xfs_info might use xfs_db or other things to > get xfs info, and I don't know if it'll be changed to use other command in > the future again. > > So this change will cause all cases which call _require_acl_get_max will notrun > on old (maybe future) xfsprogs which doesn't has xfs_spaceman "info" command. > I think that's not what we want, so I'd like to drop this patch. Thanks for > your understanding. I wasn't aware that the xfs_info->xfs_spaceman call path was a new change. With that in mind, I agree it makes sense to drop this patch. Cheers, David
On Tue, Dec 13, 2022 at 08:08:31PM +0100, David Disseldorp wrote: > On Wed, 14 Dec 2022 02:25:31 +0800, Zorro Lang wrote: > > > We should let xfsprogs decide how xfs_info works. xfs_info doesn't always > > depends on xfs_spaceman, old xfs_info might use xfs_db or other things to > > get xfs info, and I don't know if it'll be changed to use other command in > > the future again. > > > > So this change will cause all cases which call _require_acl_get_max will notrun > > on old (maybe future) xfsprogs which doesn't has xfs_spaceman "info" command. > > I think that's not what we want, so I'd like to drop this patch. Thanks for > > your understanding. > > I wasn't aware that the xfs_info->xfs_spaceman call path was a new > change. With that in mind, I agree it makes sense to drop this patch. Since 4.17, xfs_info requires xfs_spaceman if the fs is mounted, or xfs_db if unmounted. Before that, it required xfs_growfs and only worked on mounted filesystems. That said, perhaps _acl_get_max should be doing: $XFS_INFO_PROG $TEST_DIR | _filter_mkfs > /dev/null 2> $tmp.info test "${PIPESTATUS[0]}" -ne 0 && \ _fail "cannot get maximum acl count for xfs" Rather than spraying whatever nonsense it does if spaceman is missing? --D > Cheers, David
On Tue, Dec 13, 2022 at 11:27:25AM -0800, Darrick J. Wong wrote: > On Tue, Dec 13, 2022 at 08:08:31PM +0100, David Disseldorp wrote: > > On Wed, 14 Dec 2022 02:25:31 +0800, Zorro Lang wrote: > > > > > We should let xfsprogs decide how xfs_info works. xfs_info doesn't always > > > depends on xfs_spaceman, old xfs_info might use xfs_db or other things to > > > get xfs info, and I don't know if it'll be changed to use other command in > > > the future again. > > > > > > So this change will cause all cases which call _require_acl_get_max will notrun > > > on old (maybe future) xfsprogs which doesn't has xfs_spaceman "info" command. > > > I think that's not what we want, so I'd like to drop this patch. Thanks for > > > your understanding. > > > > I wasn't aware that the xfs_info->xfs_spaceman call path was a new > > change. With that in mind, I agree it makes sense to drop this patch. > > Since 4.17, xfs_info requires xfs_spaceman if the fs is mounted, or > xfs_db if unmounted. Before that, it required xfs_growfs and only > worked on mounted filesystems. > > That said, perhaps _acl_get_max should be doing: > > $XFS_INFO_PROG $TEST_DIR | _filter_mkfs > /dev/null 2> $tmp.info > test "${PIPESTATUS[0]}" -ne 0 && \ > _fail "cannot get maximum acl count for xfs" > > Rather than spraying whatever nonsense it does if spaceman is missing? Make more sense, but I don't think it's necessary to check return status of each command line. xfs_info generally works well, we can do this change when we have a real requirement in one day :) Thanks, Zorro > > --D > > > Cheers, David >
--- tests/generic/026.out +++ /home/david/xfstests/results//generic/026.out.bad @@ -1,9 +1,11 @@ QA output created by 026 +/usr/sbin/xfs_info: line 44: xfs_spaceman: command not found === Test out large ACLs === +/usr/sbin/xfs_info: line 44: xfs_spaceman: command not found 1 below acl max acl max ... Signed-off-by: David Disseldorp <ddiss@suse.de> --- common/attr | 3 +++ 1 file changed, 3 insertions(+) diff --git a/common/attr b/common/attr index cce4d1b2..a94c5e4b 100644 --- a/common/attr +++ b/common/attr @@ -57,6 +57,9 @@ _acl_get_max() _require_acl_get_max() { + # _acl_get_max -> xfs_info -> xfs_spaceman + [ "$FSTYP" == "xfs" ] && _require_xfs_spaceman_command "info" + if [ $(_acl_get_max) -eq 0 ]; then _notrun "$FSTYP does not define maximum ACL count" fi