Re: [hackers] [sbase][PATCH] flock: call exec without forking
On Sun, Mar 06, 2022 at 03:14:00PM -0800, Michael Forney wrote: > There are a couple of cases I can think of where not forking might not > be what you want: > - If the process forks again and then exits, the new child ends up > holding on to the lock longer than the lifetime of the original > process. There seems to be a -o flag to close the lock before execing > the child to prevent this, but this doesn't work if we exec directly. > The first point made me realize that -o with your patch makes flock(1) > a no-op. It would take the lock, then release it, and then exec. So if > we were to apply the patch, I think we'd need to remove the -o option. Why thank you for answering why '-o' exists. I was originally fine with getting rid of '-o' too, but the usecase you present seems relevant enough. > - It's debatable whether this is a valid thing to do, but if the > process tries to close all open file descriptors (using something like > closefrom on OpenBSD), it might end up releasing the lock > accidentally. Sounds plausible. I can see why people may want to not let the command implicitly release its lock. > Did you check what other implementations of flock(1) do in terms of > forking, and what options they support? It looks like in addition to > busybox, NetBSD and FreeBSD both have it. I have now. Seems like everyone forks and like half (util-linux and FreeBSD) provide the -F option. OK you've convinced me that there is reason to keep forking and that the current behavior of flock is fine. Thanks, Arthur
Re: [hackers] [sbase][PATCH] flock: call exec without forking
On 2022-03-06, Arthur Williams wrote: > Yeah I noticed that it wasn't defined in POSIX (which makes me wounder > why it is here in sbase and not ubase). To clarify what I meant, there > are popular implementations that don't have the '-F' flag (like busybox) > and adding it would just encourage people to write code that didn't work > in many environments. And this may be fine if there was some benefit, > but I just don't see the any positives of forking. > > But if we agree it isn't incorrect to not fork, any objection to the > patch? There are a couple of cases I can think of where not forking might not be what you want: - If the process forks again and then exits, the new child ends up holding on to the lock longer than the lifetime of the original process. There seems to be a -o flag to close the lock before execing the child to prevent this, but this doesn't work if we exec directly. - It's debatable whether this is a valid thing to do, but if the process tries to close all open file descriptors (using something like closefrom on OpenBSD), it might end up releasing the lock accidentally. The first point made me realize that -o with your patch makes flock(1) a no-op. It would take the lock, then release it, and then exec. So if we were to apply the patch, I think we'd need to remove the -o option. Did you check what other implementations of flock(1) do in terms of forking, and what options they support? It looks like in addition to busybox, NetBSD and FreeBSD both have it.
Re: [hackers] [sbase][PATCH] flock: call exec without forking
On Sun, Mar 06, 2022 at 09:00:25PM +0100, Quentin Rameau wrote: > > Don't believe the '-F' flag is portable so avoided trying to add > > it. > > flock(1) itself isn't portable, so none of its flag are. > Yeah I noticed that it wasn't defined in POSIX (which makes me wounder why it is here in sbase and not ubase). To clarify what I meant, there are popular implementations that don't have the '-F' flag (like busybox) and adding it would just encourage people to write code that didn't work in many environments. And this may be fine if there was some benefit, but I just don't see the any positives of forking. But if we agree it isn't incorrect to not fork, any objection to the patch?
Re: [hackers] [sbase][PATCH] flock: call exec without forking
> Don't believe the '-F' flag is portable so avoided trying to add > it. flock(1) itself isn't portable, so none of its flag are.
Re: [hackers] [sbase][PATCH] flock: call exec without forking
On Sun, Mar 06, 2022 at 10:40:51PM +0600, NRK wrote: > On Fri, Mar 04, 2022 at 01:11:00AM -0600, Arthur Williams wrote: > > Previously the code forked, waited for the child to exit and then > > returned the child's exit code. It is simpler to just skip fork and call > > exec directly. > > Hi Arthur, > > I didn't comment on this the last time, as I'm not entirely sure if this > is correct or not myself. However the flock(1) shipped with linux-utils > has a specific flag to avoid forking, the manpage states: > >-F, --no-fork >Do not fork before executing command. Upon execution the flock >process is replaced by command which continues to hold the lock. >This option is incompatible with --close as there would otherwise >be nothing left to hold the lock. > > NOTE: `--close` is the `-o` flag. > > - NRK > So I didn't think it mattered in terms of correctness. I was just using flock in conjunction with my service manager and didn't see the point to having an extra process waiting per service that wasn't really providing any benefit. It also cluttered the output of pstree. Yeah not the most critical of reasons to support a change, but it took less code/was simpler to just not fork so that seemed to be the more "suckless" thing to do. If there is seem deep reason for forking, I'm unaware of it. Don't believe the '-F' flag is portable so avoided trying to add it. - Arthur
Re: [hackers] [sbase][PATCH] flock: call exec without forking
On Fri, Mar 04, 2022 at 01:11:00AM -0600, Arthur Williams wrote: > Previously the code forked, waited for the child to exit and then > returned the child's exit code. It is simpler to just skip fork and call > exec directly. Hi Arthur, I didn't comment on this the last time, as I'm not entirely sure if this is correct or not myself. However the flock(1) shipped with linux-utils has a specific flag to avoid forking, the manpage states: -F, --no-fork Do not fork before executing command. Upon execution the flock process is replaced by command which continues to hold the lock. This option is incompatible with --close as there would otherwise be nothing left to hold the lock. NOTE: `--close` is the `-o` flag. - NRK
