Ticket #4590 (closed defect: fixed)

Opened 3 months ago

Last modified 2 months ago

Extended attributes - Cannot get attributes of source directory - Invalid argument (22)

Reported by: andyrozman Owned by: andrew_b
Priority: major Milestone: 4.8.33
Component: mc-core Version: master
Keywords: Cc:
Blocked By: Blocking:
Branch state: merged Votes for changeset: committed-master

Description

Version: 4.8.32-12-g9e06a50eb
OS: Ubuntu 24.04.1 LTS

Sometimes when I am copying files I get red screen "Cannot get attributes of source directory - Invalid argument (22)".

In most cases I can use "Skip/Skip? All" function and files are still moved or copied...

This is failing when I am trying to copy files on veracrypt encrypted volume (I have full permission on drive, even started with umask=000, which gives full permissions). Copying of folders in this case fails... If I enter the directory and copy files, I still get upper message, but I can still copy the files.

I started receiving this errors since I did my own build of mc few weeks ago (before that I used version supplied with Ubuntu 22.04 LTS). After I upgraded Ubuntu to 24.04 last weekend, this error started preventing copying folders.

It would be nice if this could be fixed. I rely on mc for most of operations.

I tried to copy files with different software and copying there works without problems...

Attachments

Screenshot 2024-09-24 at 07.32.26.png (164.9 KB) - added by zaytsev 3 months ago.
0001-extract_line-remove-extra-const-qualifier-erroneousl.patch (1.5 KB) - added by zaytsev 2 months ago.

Change History

comment:1 Changed 3 months ago by zaytsev

I think this is the result of #4532 - after this change mc tries to preserve extended ext2 attributes, but in your case the fgetflags call fails, causing this error.

What filesystem is on the volume? Does this always happen for every directory, or only sometimes?

If attributes are not supported, the file system should return ENOTSUP, but in your case it returns EINVAL. Sounds to me like a bug somewhere between Veracrypt and the kernel (or a "design" change). I think we already had something like this for CIFS (#3987). The fix there was to retry if EINTR was returned.

Andrew, what do you think?

comment:2 Changed 3 months ago by andyrozman

So on veracrypt container there is ext2 FS (it might be ext3 or 4). I think that problem is that volume in in veracrypt container/volume and that that underlaying volume doesn't support your calls.

So I can try to do chmod on directory, and command goes through, but in the end attribute is the same as it was before...

Would it be possible to add whitelist into mc, so that when making operations on whitelisted volume, it wouldn't try preserving attributes?

It happens always, and copying files is possible (while using Skip option), but copying folders is not possible. Using retry option doesn't make anything, just prevents copying... This is if I am doing operation on drive (so copy from and to same drive)

But it works if I copy from different drive (normal ext4), it shows error message, but with "Skip all" it copies files and folders.

comment:3 Changed 3 months ago by zaytsev

I think that problem is that volume in in veracrypt container/volume and that that underlaying volume doesn't support your calls.

As I explained, if this is the case, then it should return ENOTSUP, but it doesn't.

So I can try to do chmod on directory, and command goes through, but in the end attribute is the same as it was before...

The chmod command is useless here, you can set flags with chattr / lsattr. For example, you can try F as a directory flag. If lsattr fails on directories with EINVAL, which is what I suspect, you should report the bug to Veracrypt.

Would it be possible to add whitelist into mc

So how do we know that it's a broken volume that doesn't return ENOTSUP on unsupported operations, instead of some other problem causing EINVAL?

It happens always ...

I don't understand your explanation:

1) Does only the "Cannot get attributes of source directory" message appear, or not? (For files?)
2) Does copying directories between non-Veracrypt extX work? (Yes?)
3) Does copying directories from Veracrypt work? (No?)
4) Does copying directories to Veracrypt work? (No, different message?)

comment:4 Changed 3 months ago by andrew_b

Can we handle EINVAL and ENOSYS in addition to ENOTSUP: not to preserve ext2 attributes without error messages?

comment:5 Changed 3 months ago by zaytsev

Looking at the VeraCrypt source code, it never returns ENOTSUP / EOPNOTSUPP:

https://veracrypt.fr/code/VeraCrypt/tree/src/Driver/Fuse/FuseService.cpp#n369

In fact, it doesn't have any support for extended attributes (no custom ioctl implementation), and the error comes from libfuse default ioctl implementation. My understanding is that if no handler is implemented, then ENOSYS is returned by the FS-level handler:

https://github.com/libfuse/libfuse/blob/master/lib/fuse.c#L2267

It should then be converted to ENOTTY by the kernel:

https://github.com/torvalds/linux/blob/master/fs/fuse/ioctl.c#L12

This is used by fgetflags, which returns EOPNOTSUPP in many cases, but when it gets to ioctl, it returns its result (so if the kernel got EINVAL, it will return it).

https://github.com/tytso/e2fsprogs/blob/master/lib/e2p/fgetflags.c

This works for me on Fedora 40:

zaytsev@fedora:~/src/libfuse/example/test$ lsattr
lsattr: Operation not supported While reading flags on ./hello
lsattr: Operation not supported While reading flags on ./testdir

zaytsev@fedora:~/src/libfuse/example/test$ uname -a
Linux fedora 6.10.10-200.fc40.aarch64 #1 SMP PREEMPT_DYNAMIC Thu Sep 12 18:52:07 UTC 2024 aarch64 GNU/Linux

I don't know why it doesn't work for the reporter. There was a bugfix in libfuse3 error propagation, but I also tried 2.9.9 with hello-fs and it works.

https://github.com/libfuse/libfuse/issues/640
https://github.com/libfuse/libfuse/commit/5128cee2dd0e54b74e9ea75dfc8cf70a866ee120

comment:6 Changed 3 months ago by zaytsev

Can we handle EINVAL and ENOSYS in addition to ENOTSUP: not to preserve ext2 attributes without error messages?

It should not be possible to get ENOSYS from FS - and the reporter gets EINVAL, so it would not help. What I think we should definitely do is use EOPNOTSUPP or check both.

(ENOTSUP and EOPNOTSUPP have the same value on Linux, but according to POSIX.1 these error values should be different).

Regarding EINVAL, it would be good to first understand why this happens. Do you have any idea?

Maybe the reporter can put the lsattr output in his test VeraCrypt directory. Then we can see what it says for files and directories. I can also install Ubuntu 24.04 in a VM to see if I can reproduce it.

comment:8 Changed 3 months ago by zaytsev

Right, this is the same code I referenced. As you can see they use EOPNOTSUPP everywhere and there is no EINVAL anywhere. So it can only come from the ioctl call. But I don't understand how this can happen.

comment:9 Changed 3 months ago by zaytsev

zaytsev@ubuntu-24-04:~/src/libfuse/example/test$ lsattr
lsattr: Operation not supported While reading flags on ./hello
lsattr: Operation not supported While reading flags on ./testdir

zaytsev@ubuntu-24-04:~/src/fuse-2.9.9/example/test$ lsattr
lsattr: Operation not supported While reading flags on ./hello
lsattr: Operation not supported While reading flags on ./testdir

Can there be some other reason why EINVAL is returned, some race condition between function calls? VeraCrypt only has AMD64 builds for Ubuntu :( I can't install it on ARM64.

comment:10 Changed 3 months ago by andyrozman

So I tried lsattr in one of folders:

lsattr: Invalid argument While reading flags on ./chord4.iml
lsattr: Invalid argument While reading flags on ./chord4.log
lsattr: Invalid argument While reading flags on ./chord4.log.1
lsattr: Invalid argument While reading flags on ./chord4.log.2
lsattr: Invalid argument While reading flags on ./chord4.log.3
lsattr: Invalid argument While reading flags on ./chord4.log.4
lsattr: Invalid argument While reading flags on ./chord4.log.5
lsattr: Invalid argument While reading flags on ./chordProperties
lsattr: Invalid argument While reading flags on ./data
lsattr: Invalid argument While reading flags on ./doc
lsattr: Invalid argument While reading flags on ./etc
lsattr: Invalid argument While reading flags on ./issues
lsattr: Invalid argument While reading flags on ./LICENSE
lsattr: Invalid argument While reading flags on ./pom.xml
lsattr: Invalid argument While reading flags on ./README.md
lsattr: Invalid argument While reading flags on ./src
lsattr: Invalid argument While reading flags on ./target

It is same for all the others.

Like I said machine is Ubuntu 24.04.1 LTS, which was upgraded from 22.04.1 (previous week). This worked on old machine with old mc (don't know what version it was). Problem started happening when I built my own mc version (newest from master), I think that was few weeks ago. I didn't use my encrypted drive then a lot, I mean I did copy some files from it to different drive (which brought red banner,but action suceded with use of Skip), but I wasn't copying files and folders on the same drive (which is operation that is no longer possible).

Operations:
copy file ENC -> ENC works - (shows warning - can be skipped)
copy dirs ENC -> ENC DOESN'T work - (shows warning - can't be skipped)
copy file ENC -> O:EXT4 works - (warning - can be skipped)
copy dirs ENC -> O:EXT4 DOESN'T work - (warning - can't be skipped)
copy file O:EXT4 -> O:EXT4 works
copy dirs O:EXT4 -> O:EXT4 works

So it seems I can't copy any directories from or to encrypted drive (with mc).

So I searched for libfuse (locate libfuse) and this is what it found.

/usr/lib/x86_64-linux-gnu/libfuse.so.2
/usr/lib/x86_64-linux-gnu/libfuse.so.2.9.9
/usr/lib/x86_64-linux-gnu/libfuse3.so.3
/usr/lib/x86_64-linux-gnu/libfuse3.so.3.14.0

Last edited 3 months ago by zaytsev (previous) (diff)

comment:11 Changed 3 months ago by zaytsev

Thanks, that helps!

So the problem is indeed that the encrypted file system returns EINVAL instead of EOPNOTSUPP. I'm still a bit wary of just blacklisting EINVAL and would like to understand why this happens and who is at fault.

Can you show mount flags?

It would also be good to know if lsattr works on other fuse systems on your machine, such as dav2fs or sshfs. If they work, then it's really pointing to !Veracrypt... and maybe we should involve VeraCrypt? developer.

Last edited 3 months ago by zaytsev (previous) (diff)

comment:12 Changed 3 months ago by andyrozman

I have only NTFS with fuse fs, and its showing similar problem.

➜  windows lsattr
lsattr: Invalid argument While reading flags on ./DumpStack.log.tmp
lsattr: Invalid argument While reading flags on ./$GetCurrent
lsattr: Invalid argument While reading flags on ./$Recycle.Bin
lsattr: Invalid argument While reading flags on ./$Windows.~WS
lsattr: Invalid argument While reading flags on ./$WinREAgent
lsattr: Invalid argument While reading flags on ./cygwin64
lsattr: Operation not supported While reading flags on ./Documents and Settings
lsattr: Invalid argument While reading flags on ./hiberfil.sys
lsattr: Invalid argument While reading flags on ./Intel
lsattr: Invalid argument While reading flags on ./pagefile.sys
lsattr: Invalid argument While reading flags on ./PerfLogs
lsattr: Invalid argument While reading flags on ./Program Files
lsattr: Invalid argument While reading flags on ./Program Files (x86)
lsattr: Invalid argument While reading flags on ./ProgramData
lsattr: Invalid argument While reading flags on ./Recovery
lsattr: Invalid argument While reading flags on ./RootPhone
lsattr: Invalid argument While reading flags on ./swapfile.sys
lsattr: Invalid argument While reading flags on ./SWSetup
lsattr: Invalid argument While reading flags on ./System Volume Information
lsattr: Invalid argument While reading flags on ./system.sav
lsattr: Invalid argument While reading flags on ./Temp
lsattr: Invalid argument While reading flags on ./Users
lsattr: Invalid argument While reading flags on ./Windows

And there is same problem... I can copy files and directories to NTFS-fuse, but I can copy only files from NTFS-fuse.

But I noticed that my external drives (that are NTFS) are connecting with NTFS-3g and everything works ok there (no warnings).

So my assumption would be that it has something to do with fuseblk not with veracrypt itself.

Or maybe my fuse is missconfigured or something. I do have two libfuses there libfuse.so.2.9.9 and libfuse3.so.3.14.0.

As for mount flags, till now I haven't used any... Before I reported this problem I changed my flags, just in case that could be the problem (but it seems it wasn't), but I use just standard ones (--fs-options="uid=1000,gid=1000,umask=000"), I tried first with umask=022, but same result...

Last edited 3 months ago by zaytsev (previous) (diff)

comment:13 Changed 3 months ago by zaytsev

  • Cc veracrypt@… added
  • Summary changed from Cannot get attributes of source directory - Invalid argument (22) to VeraCrypt - Cannot get attributes of source directory - Invalid argument (22)

Dear VeraCrypt developers,

Our user reports that he has problems copying files from VeraCrypt volumes. We identified the problem as fgetflags on files & directories return EINVAL instead of EOPNOTSUPP.

The user confirms that the NTFS-3g driver is working correctly on his system. I also checked hello-filesystems with fuse3 and fuse2.9.9, and when using the high-level interface and no ioctl implementation is provided, the kernel returns ENOTTY, which is then correctly converted to EOPNOTSUPP.

I suspected that EINVAL was incorrectly returned by VeraCrypt to the fuse driver instead of ENOSYS, but I couldn't find any evidence of this in the code.

The problem really seems to be with VeraCrypt. We would appreciate your help.

comment:14 Changed 3 months ago by ossi

i wonder whether this isn't due to some security stuff like selinux or apparmor being misconfigured and not letting the syscalls through properly. we've repeatedly seen this in qt.

comment:15 Changed 3 months ago by andyrozman

Don't use selinux, but I do have apparmor installed.

I checked in log, and there are no apparmor messages there (usually I see something there) that would be tied to this.

So I checked and it seems that my version of veracrypt is still using the old libfuse: libfuse.so.2 (libfuse.so.2.9.9).

Maybe new libfuse works better, but that will be only helpful if new version of veracrypt uses it (my version of veracrypt is 2 years old, because that was the last version that supported true-crypt volume (most of my volumes are vera-crypt ones, but I still have one that is old).

I will let you guys know if this works, but it will take me few days to do this.

comment:16 Changed 3 months ago by andyrozman

I tried to install new version of veracrypt (my old one is 1.24.4, which is about 4y old), but it seems that my volumes are not compatible (no idea why, waiting for response from their support team), everything looks like it should work but it doesn't. I did check that new version of veracrypt is still using the old libfuse library...

Is there a way that I could change my version of mc that this checks wouldn't be made? I mean in shell I can copy any directory to my veracrypt drive without the problem... Only mc is preventing copy and shows this errors? Since this worked in my previous version of mc there should be a way that it could work now.

Couldn't we add handling of "Invalid argument While reading flags on" to be the same as "Operation not supported While reading flags on".

Last edited 3 months ago by andyrozman (previous) (diff)

comment:17 Changed 3 months ago by zaytsev

Couldn't we add handling of "Invalid argument While reading flags on" to be the same as "Operation not supported While reading flags on".

I'm leaning towards this solution because it seems there are more broken fuse users out there - or maybe, as ossi suggests, something is changing the syscall return value.

What I didn't like about this idea is that there could be other reasons why EINVAL is returned and the attributes are not transferred without the user noticing. But apparently there is very little risk of this happening.

Is there a way that I could change my version of mc that this checks wouldn't be made?

You can uncheck "preserve attributes", but afaik you'll also lose timestamps that way. Otherwise, you can edit the source or wait for the patch. Or use the previous version.

comment:18 Changed 3 months ago by andyrozman

Then I will wait for the patch... Thanks for fixing this.

Using old version stopped working because of some other issues with zsh and mc, which is why I built new version of mc in the first place (before I upgraded to Ubuntu 24.04 which had newer version by default).

comment:19 follow-up: ↓ 29 Changed 3 months ago by zaytsev

Just to support my point that ENOTSUP != EOPNOTSUP - I can now reproduce the same error on Solaris, where EOPNOTSUP is 122.

It also seems that our skip logic is broken:

  1. I copy 1 file
  2. I get an error message
  3. When I press "Skip", the file is copied.

Now when I repeat the same with a directory, the directory is not copied.

Changed 3 months ago by zaytsev

comment:20 follow-up: ↓ 21 Changed 3 months ago by zaytsev

  • Cc veracrypt@… removed
  • Milestone changed from Future Releases to 4.8.33

I may have an idea where EINVAL comes from.

Unfortunately, my understanding of how VeraCrypt, fuse and native filesystems interact is still incomplete. I only fully understand how the basic fuse filesystems (sshfs) work. But apparently VeraCrypt exposes a block device via fuse, which is then mounted using the "normal" filesystem drivers. So the error codes we get are not necessarily coming from VeraCrypt or fuse, but may be returned by the filesystem drivers.

I looked in the kernel to see what drivers return, and EOPNOTSUPP and ENOTTY (which we don't need to deal with explicitly because it's converted to EOPNOTSUPP by fgetflags), but they also seem to return EINVAL with a similar meaning (btrfs). I still can't reproduce it on my machine. andyrozman, can you show the mount options as I asked (type mount)? I tried noacl, but that option has been removed in recent kernels.

Andrew, my suggestion is to change ENOTSUP to EOPNOTSUPP to fix Solaris & Co. and also blacklist EINVAL. Not sure about the skip logic. Could you please have a look? I'm trying to wrap up my Illumos stuff.

comment:21 in reply to: ↑ 20 Changed 3 months ago by andrew_b

Replying to zaytsev:

Andrew, my suggestion is to change ENOTSUP to EOPNOTSUPP to fix Solaris & Co.

There is not a unanimity of ENOTSUP, EOPNOTSUPP and ENOSYS usage in the world. I'd propose to check them all.

and also blacklist EINVAL.

Where?

comment:22 follow-up: ↓ 32 Changed 3 months ago by zaytsev

I agree to ignore ENOTSUP, EOPNOTSUPP, ENOSYS and EINVAL. Not nice, but apparently that's life.

comment:23 Changed 3 months ago by zaytsev

  • Summary changed from VeraCrypt - Cannot get attributes of source directory - Invalid argument (22) to Extended attributes - Cannot get attributes of source directory - Invalid argument (22)

comment:24 Changed 3 months ago by andrew_b

  • Owner set to andrew_b
  • Status changed from new to accepted
  • Branch state changed from no branch to on review

Branch: 4590_ext2attr_errno
Initial changeset:d7f2e2515db7bfa6bb78e5f4c89f1801faacc0a9

comment:25 follow-ups: ↓ 27 ↓ 31 Changed 3 months ago by zaytsev

I pushed a fixup to rename ext2 to extended, what do you think? What bothers me is that these extended attributes started as ext2, but now they are supported in the whole ext* family, and also by other file systems that don't have anything to do with ext, like btrfs, xfs, etc. I think the reference to ext2 in this case is more confusing than helpful.

comment:26 follow-up: ↓ 28 Changed 3 months ago by zaytsev

Also, have you checked the skip logic, is it by design?

comment:27 in reply to: ↑ 25 Changed 3 months ago by andrew_b

Replying to zaytsev:

I pushed a fixup to rename ext2 to extended, what do you think?

Ok.

What bothers me is that these extended attributes started as ext2, but now they are supported in the whole ext* family, and also by other file systems that don't have anything to do with ext, like btrfs, xfs, etc. I think the reference to ext2 in this case is more confusing than helpful.

man lsattr still says

lsattr - list file attributes on a Linux second extended file system

and man chattr

chattr - change file attributes on a Linux file system

comment:28 in reply to: ↑ 26 ; follow-up: ↓ 35 Changed 3 months ago by andrew_b

Replying to zaytsev:

Also, have you checked the skip logic, is it by design?

Yes, I tested with ntfs and tmpfs. There was no error messages.

comment:29 in reply to: ↑ 19 ; follow-up: ↓ 30 Changed 3 months ago by zaytsev

Yes, I tested with ntfs and tmpfs. There was no error messages.

That is not what I meant. This is what I meant:

Replying to zaytsev:

It also seems that our skip logic is broken:

  1. I copy 1 file
  2. I get an error message
  3. When I press "Skip", the file is copied.

Now when I repeat the same with a directory, the directory is not copied.

comment:30 in reply to: ↑ 29 Changed 3 months ago by andrew_b

Replying to zaytsev:

Now when I repeat the same with a directory, the directory is not copied.

Tried with ntfs. Directory is copied w/o messages related to extended attributes. There are messages "Cannot chmod target file XXXX. Operation not permitted (1)" but it's ok.

And (unexpectedly!) button "Abort" does not stop the directory copy. I'll check the in the cleanup branch.

comment:31 in reply to: ↑ 25 Changed 3 months ago by andrew_b

Replying to zaytsev:

I pushed a fixup to rename ext2 to extended, what do you think?

I guess the some misunderstanding is possible. Extended attributes are attr(5) and attr(1). They are not supported in mc yet: #2468.

comment:32 in reply to: ↑ 22 Changed 3 months ago by andrew_b

Replying to zaytsev:

I agree to ignore ENOTSUP, EOPNOTSUPP, ENOSYS and EINVAL. Not nice, but apparently that's life.

On some not very old Linux kernels (4.9, 5.4) there is yet another error on tmpfs:

 Cannot set attributes for target file "/tmp/.bash_history"
       Inappropriate ioctl for device (25)

This is

ENOTTY 25 Inappropriate ioctl for device
Last edited 3 months ago by andrew_b (previous) (diff)

comment:33 follow-up: ↓ 37 Changed 3 months ago by zaytsev

I guess the some misunderstanding is possible. Extended attributes are attr(5) and attr(1). They are not supported in mc yet: #2468.

You are right! How about "inode flags" - that seems to be the official and unambiguous name:

https://man7.org/linux/man-pages/man2/ioctl_iflags.2.html

I made another fixup.

comment:34 Changed 3 months ago by zaytsev

On some no very old Linux kernels (4.9, 5.4) there is yet another error on tmpfs

Maybe it has rather to do with an old version of e2fsprogs? The conversion from ENOTTY to EOPNOTSUPP was introduced only 3 years ago:

https://github.com/tytso/e2fsprogs/commit/40ea4628ba1b55f8eba311f12399d039698dbeeb

But we can also filter it, I have nothing against that. And ELOOP and ENXIO while we are at it, for the same reason ;-)

comment:35 in reply to: ↑ 28 ; follow-ups: ↓ 36 ↓ 46 Changed 3 months ago by zaytsev

Replying to andrew_b:

Replying to zaytsev:

Also, have you checked the skip logic, is it by design?

Yes, I tested with ntfs and tmpfs. There was no error messages.

Maybe I don't understand the intention of the Skip button. I always thought that "Skip" means do NOT copy (пропустить) file or directory if some problem occurs. I am now testing on Solaris and the following happens:

  1. I copy 2 files, error message comes up
    • Abort works (no files are copied at all)
    • Skip - copies one file at a time, with an error message each time
    • Skip all - all files are copied, no error message is displayed
  2. I am copying a directory with 1 file, error message appears
    • Skip means that the directory and the file in it will not be copied at all!
    • Skip all is the same - nothing is copied.

Maybe it's a translation problem, and by "Skip" (file, and continue with next file) you actually meant "Ignore" (error getting flags, but process file)? Then it makes sense somehow, but why is the directory not copied at all?

comment:36 in reply to: ↑ 35 Changed 3 months ago by andrew_b

Replying to zaytsev:

  1. I am copying a directory with 1 file, error message appears

What error message exactly?

  • Skip means that the directory and the file in it will not be copied at all!
  • Skip all is the same - nothing is copied.

Maybe it's a translation problem, and by "Skip" (file, and continue with next file) you actually meant "Ignore" (error getting flags, but process file)? Then it makes sense somehow, but why is the directory not copied at all?

Generally, "Skip" means "Skip this error" and "Skip all" means "Skip all errors". If you get an error at file open, "Skip" works as "Skip file".
Probably, "Ignore" and "Ignore all" are better names.

comment:37 in reply to: ↑ 33 Changed 3 months ago by andrew_b

Replying to zaytsev:

I guess the some misunderstanding is possible. Extended attributes are attr(5) and attr(1). They are not supported in mc yet: #2468.

You are right! How about "inode flags" - that seems to be the official and unambiguous name:

https://man7.org/linux/man-pages/man2/ioctl_iflags.2.html

This is for programmers not for users. Users will be confused seeing such message.

comment:38 Changed 3 months ago by zaytsev

This is for programmers not for users. Users will be confused seeing such message.

I would be confused by the "ext2 attributes" message, because I don't have an ext2 filesystem :( but ok, if you are convinced that "ext2 attributes" is better, just discard both fixups.

comment:39 Changed 3 months ago by zaytsev

What error message exactly?

In all cases by "error" I mean " Cannot get attributes of source file "/export/home/va~cale/testfile1": Operation not supported on transport endpoint (122)".

Generally, "Skip" means "Skip this error" and "Skip all" means "Skip all errors". If you get an error at file open, "Skip" works as "Skip file". Probably, "Ignore" and "Ignore all" are better names.

OK, I understand. In English, an error cannot be skipped. It can only be ignored. This was causing confusion for me and Andy. It would be great if we could change it to "ignore".

But I still don't understand why "skip" on directory causes the directory and the files in it not to be copied, but skip on file causes selected files to be copied.

comment:40 follow-up: ↓ 41 Changed 3 months ago by zaytsev

I'll be away until next week. Are you still planning to look into the above? Also interested in your opinion on #4584, before we go on with the formatter. Rebasing it will be a lot of work.

comment:41 in reply to: ↑ 40 Changed 3 months ago by andrew_b

Replying to zaytsev:

Are you still planning to look into the above? Also interested in your opinion on #4584

I'll look into that at weekend.

comment:42 Changed 3 months ago by andrew_b

Rebased to recent master.

Branch: 4590_ext2attr_errno
Initial changeset:94cf9b81e993c84f2396053d36be817bddaa9fb7

comment:43 follow-up: ↓ 47 Changed 2 months ago by zaytsev

I'm trying to test this branch, but even when rebased to master, it doesn't compile for me on macOS:

../../../../src/vfs/tar/tar-internal.c:469:47: error: use of undeclared identifier 'UINTMAX_WIDTH'
  469 |         topbits = ((uintmax_t) - signbit) << (UINTMAX_WIDTH - LG_256 - (LG_256 - 2));
      |                                               ^

It seems there is some more breakage caused by gnulib updates.

comment:44 Changed 2 months ago by zaytsev

With the following patch the branch builds, but I get an extra warning, patch attached:

diff --git a/src/vfs/tar/tar-internal.c b/src/vfs/tar/tar-internal.c
index d0691edc6..e2bb817de 100644
--- a/src/vfs/tar/tar-internal.c
+++ b/src/vfs/tar/tar-internal.c
@@ -466,7 +466,7 @@ tar_from_header (const char *where0, size_t digs, char const *type, intmax_t min
         uintmax_t topbits;
 
         signbit = *where & (1 << (LG_256 - 2));
-        topbits = ((uintmax_t) - signbit) << (UINTMAX_WIDTH - LG_256 - (LG_256 - 2));
+        topbits = ((uintmax_t) - signbit) << (__UINTMAX_WIDTH__ - LG_256 - (LG_256 - 2));
 
         value = (*where++ & ((1 << (LG_256 - 2)) - 1)) - signbit;

comment:45 Changed 2 months ago by zaytsev

Build on Solaris is also broken for the same reason, but it doesn't have __UINTMAX_WIDTH__. I have copy & pasted the fallback from gnulib and it seems to work on both systems (macOS and Solaris).

diff --git a/src/vfs/tar/tar-internal.c b/src/vfs/tar/tar-internal.c
index d0691edc6..d694d4539 100644
--- a/src/vfs/tar/tar-internal.c
+++ b/src/vfs/tar/tar-internal.c
@@ -466,7 +466,16 @@ tar_from_header (const char *where0, size_t digs, char const *type, intmax_t min
         uintmax_t topbits;
 
         signbit = *where & (1 << (LG_256 - 2));
-        topbits = ((uintmax_t) - signbit) << (UINTMAX_WIDTH - LG_256 - (LG_256 - 2));
+
+#define _GL_INTEGER_WIDTH(min, max) (((min) < 0) + _GL_COB128 (max))
+#define _GL_COB128(n) (_GL_COB64 ((n) >> 31 >> 31 >> 2) + _GL_COB64 (n))
+#define _GL_COB64(n) (_GL_COB32 ((n) >> 31 >> 1) + _GL_COB32 (n))
+#define _GL_COB32(n) (_GL_COB16 ((n) >> 16) + _GL_COB16 (n))
+#define _GL_COB16(n) (_GL_COB8 ((n) >> 8) + _GL_COB8 (n))
+#define _GL_COB8(n) (_GL_COB4 ((n) >> 4) + _GL_COB4 (n))
+#define _GL_COB4(n) (!!((n) & 8) + !!((n) & 4) + !!((n) & 2) + !!((n) & 1))
+
+        topbits = ((uintmax_t) - signbit) << (_GL_INTEGER_WIDTH (0, UINTMAX_MAX) - LG_256 - (LG_256 - 2));
 
         value = (*where++ & ((1 << (LG_256 - 2)) - 1)) - signbit;
 

comment:46 in reply to: ↑ 35 ; follow-up: ↓ 48 Changed 2 months ago by zaytsev

Replying to zaytsev:

  1. I copy 2 files, error message comes up
    • Abort works (no files are copied at all)
    • Skip - copies one file at a time, with an error message each time
    • Skip all - all files are copied, no error message is displayed
  2. I am copying a directory with 1 file, error message appears
    • Skip means that the directory and the file in it will not be copied at all!
    • Skip all is the same - nothing is copied.

I have tested the branch again on Solaris, but specifically to check this part: after renaming "Skip" to "Ignore" the behavior is more logical, but still incorrect.

  • (2) behaves as I expect
    • If I press "Ignore" on file, the error is ignored and files are copied without ext2 attributes.
  • (1) is still broken
    • If I press "Ignore" on directory, the directories (and files inside) are not copied at all.

comment:47 in reply to: ↑ 43 Changed 2 months ago by andrew_b

Replying to zaytsev:

I'm trying to test this branch, but even when rebased to master, it doesn't compile for me on macOS:

../../../../src/vfs/tar/tar-internal.c:469:47: error: use of undeclared identifier 'UINTMAX_WIDTH'
  469 |         topbits = ((uintmax_t) - signbit) << (UINTMAX_WIDTH - LG_256 - (LG_256 - 2));
      |                                               ^

It seems there is some more breakage caused by gnulib updates.

UINTMAX_WIDTH is defined in stdint.h (but for C2x). A workaround:

#ifndef UINTMAX_WIDTH
#define UINTMAX_WIDTH (sizeof (uintmax_t) * CHAR_BIT)
#endif

comment:48 in reply to: ↑ 46 Changed 2 months ago by andrew_b

Replying to zaytsev:

Replying to zaytsev:

  1. I copy 2 files, error message comes up
    • Abort works (no files are copied at all)
    • Skip - copies one file at a time, with an error message each time
    • Skip all - all files are copied, no error message is displayed
  2. I am copying a directory with 1 file, error message appears
    • Skip means that the directory and the file in it will not be copied at all!
    • Skip all is the same - nothing is copied.

I have tested the branch again on Solaris, but specifically to check this part: after renaming "Skip" to "Ignore" the behavior is more logical, but still incorrect.

  • (2) behaves as I expect
    • If I press "Ignore" on file, the error is ignored and files are copied without ext2 attributes.
  • (1) is still broken
    • If I press "Ignore" on directory, the directories (and files inside) are not copied at all.

Are (1) and (2) swapped? There are no directories in (1).
What is error (errno and text) on directory?

comment:49 follow-up: ↓ 51 Changed 2 months ago by zaytsev

Are (1) and (2) swapped? There are no directories in (1).

Yes, sorry.

What is error (errno and text) on directory?

EOPNOTSUPP, I've just made your function to return FALSE, so that I can test it. Your suppression is working, I'm just trying to explain that ignore logic seems to be broken for directories and show how to reproduce it.

comment:50 Changed 2 months ago by zaytsev

UINTMAX_WIDTH is defined in stdint.h (but for C2x). A workaround:

Confirm that this works on macOS, can test on Solaris later if needed.

comment:51 in reply to: ↑ 49 Changed 2 months ago by andrew_b

Replying to zaytsev:

I'm just trying to explain that ignore logic seems to be broken for directories and show how to reproduce it.

There is a strange (wrong?) logic in copy_dir_dir(): Ignore and Ignore all work as Abort.

comment:52 Changed 2 months ago by andrew_b

I've added a commit, please test.

comment:53 Changed 2 months ago by zaytsev

  • Votes for changeset set to zaytsev
  • Branch state changed from on review to approved

I can confirm that UINTMAX_WIDTH replacement also works on Solaris, and after your fix, the copy dir files behavior is the one I expect. It seems everything is working correctly now.

Could you please apply the warning fix, if you don't want it in this branch, then direct to master.

comment:54 Changed 2 months ago by andrew_b

  • Status changed from accepted to testing
  • Votes for changeset changed from zaytsev to committed-master
  • Resolution set to fixed
  • Branch state changed from approved to merged

Merged to master: [8d0bb40f169f329e0c8ece66b7d65c2f6ef3556d].

git log --oneline de7d72cab..8d0bb40f1

comment:55 Changed 2 months ago by andrew_b

  • Status changed from testing to closed

comment:56 Changed 2 months ago by zaytsev

Ok, I took the warning fix to master.

Note: See TracTickets for help on using tickets.