skarnet / execline

The execline scripting language
https://skarnet.org/software/execline/
ISC License
151 stars 19 forks source link

`fdmove -c 1 1 cmd` fails #14

Open emanuele6 opened 1 year ago

emanuele6 commented 1 year ago

fdmove 1 1 is correctly a no-op, but fdmove -c will fail if you use the same file descriptor:

$ fdmove 1 1 ls -l /proc/self/fd
total 0
lrwx------ 1 emanuele6 wheel 64 Nov 16 09:23 0 -> /dev/pts/7
lrwx------ 1 emanuele6 wheel 64 Nov 16 09:23 1 -> /dev/pts/7
lrwx------ 1 emanuele6 wheel 64 Nov 16 09:23 2 -> /dev/pts/7
lr-x------ 1 emanuele6 wheel 64 Nov 16 09:23 3 -> /proc/3330198/fd
$ fdmove -c 1 1 ls -l /proc/self/fd
fdmove: fatal: unable to move fd 1 to fd 1: Invalid argument

skalibs's fd_copy(to, from) function hardcodes to == from as a failure instead of returning to for successful no-op if it's positive, or just calling dup2(to, from) and letting it do nothing successfully.

https://github.com/skarnet/skalibs/blob/2ea36ebf726bb3a29ff8c6b12a6ff3df9020859f/src/libstddjb/fd_copy.c#L10

When you are doing something like fdmove -c 1 "$fd", the fact that it fails if fd is 1 is unideal.


Slightly related: fdmove is documented as

On a fdmove a b prog... is roughly equivalent to sh -c 'exec prog... a>&b b<&-'. It means: if you write to fd a now, it will have the same effect as writing to fd b beforehand, and you cannot write to fd b anymore.

I think it could be useful to mention that fdmove a b is not simply equivalent to fdmove -c a b fdclose b, because it is a no-op and won't close b if a == b; more like a<&b- in bash/ksh93 rather than a<&b b<&-.

o/  emanuele6

skarnet commented 1 year ago

Arguably fdmove -c 1 1 should be a failure, because you're expecting a copy of a descriptor, and you're not getting it. But your expectation makes sense too.

I don't really have strong feelings towards either behaviour, but I do, at some point, need to make a pass on all the instances of fd_move and fd_copy in my software, because I think there are some inconsistencies. Sometimes I want to mimic the behaviour of dup2 exactly, and sometimes I don't; before changing anything, I need to determine what the most common/useful behaviour is.

Until then, changing things would be counterproductive, I think. Sorry I don't have a better answer than "I'll make things super clear at some point". :)

emanuele6 commented 1 year ago

Arguably fdmove -c 1 1 should be a failure, because you're expecting a copy of a descriptor, and you're not getting it. But your expectation makes sense too.

I need to determine what the most common/useful behaviour is.

Well, 1<&1 in shell, and dup2(1,1) are well defined as successful no-ops.

Also, if you got the fd from fdreserve, and stdout happened to be close so FD0 was set to 1, what are you supposed to do in this case?

fdreserve 1
importas -u fd FD0
creatememfd ${fd} mymemfd
foreground { somecmd ${fd} }
fdmove -c 1 ${fd}
someothercmd ${fd}

duplicate someothercmd just to workaround this problem?

fdreserve 1
importas -u fd FD0
creatememfd ${fd} mymemfd
foreground { somecmd ${fd} }
if { test 1 = ${fd} } { someothercmd ${fd} }
fdmove -c 1 ${fd}
someothercmd ${fd}

But what if I need to duplicate two fds, and I want to check them both?

I guess I could use a chainif-like command that only conditionally includes fdmove -c 1 ${fd} like

fdreserve 1
importas -u fd FD0
creatememfd ${fd} mymemfd
foreground { somecmd ${fd} }
chainif { test 1 != ${fd} } { fdmove -c 1 ${fd} }
someothercmd ${fd}

But I think this is just a unnecessary complication.

sh would not fail, the kernel would fail either, and I don't see how fdmove -c failing can ever be useful. :(

skarnet commented 1 year ago

By policy, you should always know whether stdout is open or not. POSIX mandates that applications always start with fds 0, 1 and 2 open; you don't have to always follow this rule, but if you don't, it's an internal API and your tools should be aware of it.

If there is a situation where you have a file descriptor and it may or may not be fd 1, and you genuinely don't know, then something is wrong in your design.

That said, I agree that erroring out is less than ideal; but again, I cannot revisit this before a deep consistency pass over all the uses of fd_move and fd_copy in the whole skarnet.org stack. It will happen at some point; please be patient.

emanuele6 commented 1 year ago

By policy, you should always know whether stdout is open or not. POSIX mandates that applications always start with fds 0, 1 and 2 open; you don't have to always follow this rule, but if you don't, it's an internal API and your tools should be aware of it.

What does this have to do with fdmove -c 10 10 failing though? I used stdout in that example which was a little silly, but I could have used any other hardcoded fd. Any way, if one knows that stdout is closed, they have to deal with it, yes. And they can try to do that with one of their scripts that uses fdmove -c maybe, but if they reasonably use fdreserve in combination with fdmove -c, it may just fail for no reason, so they will have to work around that too. :/

If there is a situation where you have a file descriptor and it may or may not be fd 1, and you genuinely don't know, then something is wrong in your design.

Do you think that fdreserve should only reserve file descriptors 3 or above (or 9 or above like in shells with {var}<&) then? Personally I think it is fine reserving the lowest file descriptors available including stdin, stdout, and stderr, but maybe it would be better if it did not reserve special file descriptors to make scripts that use it more robust. It could be achieved fairly easily using fcntl(F_DUPFD), or simply allocating extra file descriptors if one of stdin, stdout, or stderr is not already open.

That said, I agree that erroring out is less than ideal; but again, I cannot revisit this before a deep consistency pass over all the uses of fd_move and fd_copy in the whole skarnet.org stack. It will happen at some point; please be patient.

That is fine, I just wanted to report the problem. Do whatever you please whenever you please. =)

PS: I wrote a chainif command; it's quite cool. https://gist.github.com/emanuele6/3f7e6a09af612fada3874b96e0a70475