linux-test-project / ltp

Linux Test Project (mailing list: https://lists.linux.it/listinfo/ltp)
https://linux-test-project.readthedocs.io/
GNU General Public License v2.0
2.31k stars 1.01k forks source link

swapon01 doesn't take EPERM into account #1091

Closed richiejp closed 11 months ago

richiejp commented 1 year ago

Test fails when swapon returns EPERM. This is the correct behavior when we don't have privs.

Martchus commented 11 months ago

On my system the test fails with libswap.c:57: TCONF: Swapfile on tmpfs not implemented. That it tries to use /tmp which is using a tmpfs on many systems is likely not ideal as well. The same counts for the other swapon tests.

This could be easily fixed by adding:

    .needs_root = 1,
    .mntpoint = MNTPOINT,
    .mount_device = 1,
pevik commented 11 months ago

Well, EPERM is on other filesystems as well, tested on btrfs and ext4. I guess we would need root for the permission anyway.

.needs_root = 1,
.mntpoint = MNTPOINT,
.mount_device = 1,

How about use .all_filesystems = 1. tmpfs is already detected on root, but I'd maybe check add for EPERM in libswap.c:59 => TCONF, just for sure.

Martchus commented 11 months ago

Does it really make sense to use .all_filesystems = 1? This would run the test on multiple filesystems but it suppose we just need one that works.

Martchus commented 11 months ago

@richiejp I must also say that the ticket description is not very clear.

To me the test code looks like it would produce TFAIL if swapon01 returns EPERM. I say "looks like" because I'm not sure how to reproduce the problem. Are you asking to change the test code so it would produce TCONF in that case (as @pevik suggested)?

pevik commented 11 months ago

FYI working on this, more fixes are needed, eg. in tst_ioctl.c

Martchus commented 11 months ago

Ok, then I'll leave it to you. (By the way, is there some way to assign oneself to tickets. This would allow us to avoid having multiple people work on the same ticket in parallel.)

metan-ucw commented 11 months ago

Also I'm not sure that all_filesystems would work here, since unless user has passed a real block device, we would end up with a block device in a file on a tmpfs, that probably still does not work with swap. There is more problems with LTP tests with /tmp on tmpfs, the only proper solution is to pass TMPDIR env variable to LTP that points to a real filesystem.

metan-ucw commented 11 months ago

Also I guess that all that is actually needed is to add '.needs_root = 1,' to swapon01.c

pevik commented 11 months ago

@Martchus @richiejp Feel free to do it as you started on it. The cleanup I do is not relevant to this.

Martchus commented 11 months ago

Supposedly using .needs_root = 1 is the simplest approach, indeed. I created a patch: https://patchwork.ozlabs.org/project/ltp/patch/20231011152900.4274-1-mkittler@suse.de/