Enable yanking in one instance and pasting in another

[m7a]

Hello,

I would like to suggest an additional option be added for VIFM to be
able to synchronize register contents with other instances. Here is
why I think this would be useful:

I have been using console based file managers for a while (I started
with VIFM, then switched to ranger on Linux and kept VIFM on Windows
for a while and now I am back to VIFM on all platforms) and found it
to be difficult to adapt to a tab-based or dual-pane scheme of
managing files. What I would like to do is
have just one directory open per file manager instance and be
able to move and copy files across those multiple instances
(similar to what can be achieved
in GUI file managers using the system-wide clipboard).

As a prototype, I remapped the yy, dd, p and P commands in
my vifmrc to store the argument of the yy and dd commands in
a file which is then read whenever a p or P is pressed to find
out which file is to be pasted. While this works as long as one
restricts copying to yy, already simple derivations like y4j for
copying the current file and four files down do not invoke the
overriden mapping for yy.

Instead I thought it would be very nice if there was a way for
VIFM to automatically synchronize register contents with other
instances by writing and reading the vifminfo whenever yank,
delete or paste commands are invoked in any way.

I have implemented an option called syncregs in the pull
request/patch linked to this message and would like to know if
there are any obvious errors in my thoughts (like one can already
do that with VIFM or my implementation is flawed) and if there
are no obvious errors, if this has a chance of being added to
VIFM?

Thank you very much for the good work on VIFM. Compared to the
time before I started using ranger, it has improved very much
and I see the project is developed very actively -- very much
appreciated at my side! Also, I enjoy reading the source code
for its simplicity and clean style! :)

[xaizek]

Hello,

welcome back :-)

I enjoy reading the source code for its simplicity and clean style!

I wish it were true in all parts of the code base, but I think overall code quality increases over time.

have just one directory open per file manager instance and be able to move and copy files across those multiple instances

There is a TODO entry for something similar, but I thought introducing tabs will solve that, not so sure anymore.

if there are any obvious errors in my thoughts (like one can already do that with VIFM or my implementation is flawed)

It's not possible at the moment. Proposed implementation isn't really complete, because registers are also accessed at least for :registers menu and macros. Also, reading and writing vifminfo updates everything (histories, ui state, etc.).

if this has a chance of being added to VIFM?

I think registers is the main thing that needs to be synchronized, but I don't know a really good way of doing it.

Synchronizing state via file system might have quite an impact on performance (updating vifminfo file is probably one of the heaviest procedures in vifm). There is an IPC over FIFO, but there is a limit on how many bytes you can pass atomically and it's really inconvenient to use. Another option is to use shared memory, but we don't know how much memory we will need and pointers can't be used there, yet this is probably the best option.

I like the approach you took of writing results of sequence of updates instead of synchronizing each update separately. Keeping that I think I'd use large chunk of shared memory (OS shouldn't actually allocate unused space) with a lock in it to implement something like regs_sync(int for_write). This requires special (de)serialization, still this might be the most lightweight way of implementing it.

These are just some thoughts I had and it's worth spending some more time on this subject, so feel free to provide your comments. So far it seems that implementation can be not intrusive (like this patch) and without extra performance penalties.

[m7a]

have just one directory open per file manager instance and be able to move and copy files across those multiple instances

There is a TODO entry for something similar, but I thought introducing tabs will solve that, not so sure anymore.

Well, I just ``come from'' the tab-world and what I miss about tabs is the enhanced visibility of seeing multiple directories at once. Something like a quad-view would probably be OK for my needs, too, but I thought the most generic form of copying between instances probably best models how I use programs (I tend to quickly open and close most programs except E-Mail and Webbrowser very often).

Synchronizing state via file system might have quite an impact on performance (updating vifminfo file is probably one of the heaviest procedures in vifm).

Well, this is what I thought, too but I could not think of better alternatives at the time (I considered networking and similar system-wide IPC which always lead me to worry about how this behaves in a multi-user scenario. A file in the local home directory seemed to be very reliable and portable form a multi-user-security point of view...)

Another option is to use shared memory, but we don't know how much memory we will need and pointers can't be used there, yet this is probably the best option.

Are there any shared memory technologies already in use in VIFM or do you recommend one specifically? I am asking because I this seems like a clean(er) solution than what I have thought of and come up with (hopefully one does not have to do special provisions for preventing multiple users' instances from interfering :) ) and I would love to try to implement it this way.

I like the approach you took of writing results of sequence of updates instead of synchronizing each update separately.

I am sorry, but I must confess I do not understand this sentence: What is the sequence of updates which I have not synchronized separately?

These are just some thoughts I had and it's worth spending some more time on this subject, so feel free to provide your comments. So far it seems that implementation can be not intrusive (like this patch) and without extra performance penalties.

Here they are, thank you very much for your feedback :)

I am also interested in implementing this cleanly. If you point me into a direction for shared memory I will try to come up with a more sophisticated patch :)

[xaizek]

What is the sequence of updates which I have not synchronized separately?

It was about granularity, I just meant that you synchronized snapshots of registers' state instead of every single update to registers (e.g., appending of a file).

Are there any shared memory technologies already in use in VIFM or do you recommend one specifically?

None at the moment. There is POSIX shared memory (shm_mem()) and semaphores (sem_open()). Such primitives account for permissions, but object name should probably include user id. Windows has similar API that respects permissions (there is Local\ namespace), so porting it there after it works on Unix-like systems shouldn't be a problem.

Other two kinds are SysV shared memory and file mappings, but first one is legacy and second one provides persistence, which we don't really need (it also involves disk IO).

[m7a]

What is the sequence of updates which I have not synchronized separately?

It was about granularity, I just meant that you synchronized snapshots of registers' state instead of every single update to registers

OK, now I got it :)

Are there any shared memory technologies already in use in VIFM or do you recommend one specifically?

None at the moment. There is POSIX shared memory (shm_mem()) and semaphores (sem_open()). Such primitives account for permissions, but object name should probably include user id [...]

I will try to come up with a better solution using POSIX shared memory and see how far I get :) As soon as I have anything like an improved patch, I will write again :) Thank you very much for your attention.

[m7a]

Hello,

after a long time I finally found time to create an implementation based on POSIX shared memory. It allows the option syncregs to be used to point to a shared memory name like e.g /vifm-shmem which is then used by all instances with that configuration.

It is not finally complete yet: I would like to make the test use different values for constants SHARED_MMAP_BYTES and SHARED_INITIAL compared to the regular build, as to easily test what happens upon reaching the limits. Unfortunately, I could not find out how to do this. Is this possible with constants or should I add a function by which the test can manipulate the values?

Also, being new to Github Pull Requests, I do not know how to change my pull request without completely deleting my fork which might cause this pull request to become defective? As a workaround, I have attached my current idea (works for the most recend master-branch as of this writing) as a regular patch called
shared_memory_synchronization_2.txt

I am looking forward to your feedback, thanks in advance.

shared_memory_synchronization_2.txt

[xaizek]

Hello,

after a long time I finally found time to create an implementation based on POSIX shared memory.

That's quite significant amount of code, thanks for your efforts!

It allows the option syncregs to be used to point to a shared memory name like e.g /vifm-shmem which is then used by all instances with that configuration.

So you can define group of vifm instances which share state between each other, I like this. Do you plan /vifm- prefix to be added internally eventually? Users probably don't need to know naming scheme of a shared object.

Mind that syncregs_handler() will be called on every change of the option, so if you set it to a and then to b, regs_sync_enable() will be called twice without regs_sync_disable() in between, so you might want to handle new value of the option in registers.c where current state is known.

It is not finally complete yet: I would like to make the test use different values for constants SHARED_MMAP_BYTES and SHARED_INITIAL compared to the regular build, as to easily test what happens upon reaching the limits. Unfortunately, I could not find out how to do this. Is this possible with constants or should I add a function by which the test can manipulate the values?

Yes, you need to add functions. Sources used to be compiled for tests separately, but they don't any more to make sure they tests are run against the code which will actually be used by the application.

See this header for a helper macro, which will make functions visible only in tests (still exported, but can't be used by accident in the application itself).

Also, being new to Github Pull Requests, I do not know how to change my pull request without completely deleting my fork which might cause this pull request to become defective?

Do git push --force to your fork and the pull request will be updated.

I am looking forward to your feedback, thanks in advance.

I didn't check the whole set of changes in detail, but they look good to me in general.

[m7a]

Thank you very much for your prompt reply :)

It allows the option syncregs to be used to point to a shared memory name like e.g /vifm-shmem which is then used by all instances with that configuration.

So you can define group of vifm instances which share state between each other, I like this. Do you plan /vifm- prefix to be added internally eventually? Users probably don't need to know naming scheme of a shared object.

I have added the prefix to the code. I am very much with you that the details of the shared memory need not be exposed to users and have just used the /vifm- prefix suggested :)

Mind that syncregs_handler() will be called on every change of the option, so if you set it to a and then to b, regs_sync_enable() will be called twice without regs_sync_disable() in between, so you might want to handle new value of the option in registers.c where current state is known.

OK. For now I always call regs_sync_disable() before performing the enable although I am not quite sure if it is the best option. An alternative would involve storing the current shared memory name and then check if the new value of the setting proposes another name and if they mismatch call disable+enable otherwise do nothing? (I am not sure which is better...)

See this header for a helper macro, which will make functions visible only in tests (still exported, but can't be used by accident in the application itself).

This works well, thank you :)

Do git push --force to your fork and the pull request will be updated.

This also works well and the CI pointed me to some non-GNU-C89-things (mainly for(size_t i = ... constructions for which I have now realized a separate declaration of variables like i and j).

Currently, the CI is still failing due to the need to link with -lrt. For my local development I have always called make LDFLAGS=-lrt for testing and building -- I have little idea how GNU autotools work, so I am quite unsure where I would have to put that flag to trigger a corrected CI run? Adding it to configure.ac does not seem to work.

Additionally, I remembered two questions when I revised the code for the /vifm--prefix:

In my tests (registers_shared_memory_tests), I am using SETUP_ONCE() followed by some TEST(...) and finally, I would like to dispose the created processes. Doing this in a TEARDOWN_ONCE() block produced errors which did not happen when I just declared another TEST(teardown_once) instead. What are the technical/lifcycle differences between a TEST and a TEARDOWN_ONCE?

The other question is concerned with the shared memory implementation: Currently, the implementation is very careful about attaching to the shared memory in the most race-condition-free manner. A similar problem arises when one considers disconnecting from shared memory. Currently, I have simplified the process by simply not deleting the shared memory file which might be left over. Is this acceptable? If not, does it make sense to have some sort of reference counter which is incremented for each instance attached to the shared memory and decremented for each instsance detached and once it reaches 0, the current instance is responsible for deleting the file (which immediately raises the race condition that some instance might just be in the process of attaching at that time causing further instances not to share memory with that instance because it refers to a deleted file...)

[xaizek]

For now I always call regs_sync_disable() before performing the enable although I am not quite sure if it is the best option. An alternative would involve storing the current shared memory name and then check if the new value of the setting proposes another name and if they mismatch call disable+enable otherwise do nothing?

This might actually be enough, because option handler is called only when value of an option actually changes, so you know that it was either cleared or changed to another non-empty value.

I have little idea how GNU autotools work, so I am quite unsure where I would have to put that flag to trigger a corrected CI run

Adding something like this should make it pass:

AC_CHECK_LIB(rt, shm_open, [LIBS="$LIBS -lrt"])

This will cause configure and Makefile.in to be regenerated when you do make, then they need to be added to the commit.

What are the technical/lifcycle differences between a TEST and a TEARDOWN_ONCE?

TEARDOWN_ONCE is called once after all tests are run. However, your tests are the only one which are completely in a single file, maybe it causes some difference. Leave it as is if it doesn't work, I can correct it later.

Currently, I have simplified the process by simply not deleting the shared memory file which might be left over. Is this acceptable?

I don't think that size of memory mapping will exceed its initial size in most cases and keeping that amount of RAM busy sounds acceptable. This is something that can be addressed later if a need arises (it will be allocated only for people who enable the option anyways).

[m7a]

This might actually be enough, because option handler is called only when value of an option actually changes, so you know that it was either cleared or changed to another non-empty value.

OK.

AC_CHECK_LIB(rt, shm_open, [LIBS="$LIBS -lrt"])

Thank you very much, it compiles and links sucessfully*** :)

TEARDOWN_ONCE is called once after all tests are run. However, your tests are the only one which are completely in a single file, maybe it causes some difference. Leave it as is if it doesn't work, I can correct it later.

OK.

I don't think that size of memory mapping will exceed its initial size in most cases and keeping that amount of RAM busy sounds acceptable. This is something that can be addressed later if a need arises (it will be allocated only for people who enable the option anyways).

Yes.

***) Having received some test results from the CI which indicated that the OSX-build was failing because of a Linux-only function used by the test, I removed that function from non-Linux builds.

Still, the CI fails in two distinct ways on Linux (GCC) and OSX (GCC and Clang behave the same from the test point of view there).

On Linux, GCC outputs an error about a ``buffer overflow''. Can I do something to reproduce that issue locally as to dig into why this is happening? Obviously, on my local Debian system, this error does not occur.

On OSX, the test fails indicating that sharing did not work (i.e. the output suggests that the interaction with the test _application worked correct but produced outputs as if there was no shared memory). I am thinking that this might mean that attaching to shared memory fails somehow. Still, I am not quite sure how to go further about this? Unfortunately, I do not have a local OSX system to test with, but I am probably going to add a test to check that attaching to shared memory appears to have succeeded and see if that also fails on OSX?

During some additional local testing I noticed that (1) the DEBUG=... variable did not work with my test yet (fixed) and that (2) when using an invocation like make -j8 in the test directory, it seemed that running a test does not necessarily mean that all tests are compiled yet? I am wondering if that is the case, because I got intermittent File not found'' and Permission denied'' errors from my register_shared_memory_tests which could (or so it currently appears) not find the required reqgister_shared_memory_application.
Is it possible that tests run before compilation has completed? If yes: Can I somehow establish that registers_shared_memory_application is always compiled before registers_shared_memory_tests runs?

Thanks again for the constructive feedback, it is very much appreciated!

[xaizek]

OSX (GCC and Clang behave the same from the test point of view there).

On OS X Clang pretends to be GCC, so failure should be the same.

On Linux, GCC outputs an error about a ``buffer overflow''. Can I do something to reproduce that issue locally as to dig into why this is happening? Obviously, on my local Debian system, this error does not occur.

It must be hardened build, which is compiled with something like:

CFLAGS="-O2 -fstack-protector --param=ssp-buffer-size=4 -Wformat -Werror=format-security -D_FORTIFY_SOURCE=2"

In general you might try using valgrind (make DEBUG=valgrind) or configure vifm with sanitizers enabled (--with-sanitize=basic). But in this case, you've triggered a bug in stic :) Your code is fine. Patch:

diff --git a/tests/stic/stic.c b/tests/stic/stic.c
index d9a423895..a5cff30f4 100644
--- a/tests/stic/stic.c
+++ b/tests/stic/stic.c
@@ -537,7 +537,7 @@ int run_tests(stic_void_void tests)
 	}
 
 	if(stic_is_display_only() || stic_machine_readable) return 1;
-	sprintf(version, "stic v%s%s%s", STIC_VERSION,
+	snprintf(version, sizeof(version), "stic v%s%s%s", STIC_VERSION,
 			(stic_suite_name[0] == '\0' ? "" : " :: "), stic_suite_name);
 	printf("\n");
 	stic_header_printer(version, stic_screen_width, '=');

On OSX, the test fails indicating that sharing did not work (i.e. the output suggests that the interaction with the test _application worked correct but produced outputs as if there was no shared memory). I am thinking that this might mean that attaching to shared memory fails somehow. Still, I am not quite sure how to go further about this?

There might be some difference in flags or an error which doesn't show up on Linux. It's always a problem to debug it on CI, usually with print statements or something like that. I'd make it work on Linux and dealt with OS X later. It should work since it's POSIX.

Is it possible that tests run before compilation has completed?

I think so. This seems to change that:

 tests/Makefile | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/Makefile b/tests/Makefile
index ecd1a4786..80b37fd6f 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -295,11 +295,11 @@ else
 	$(AT)mkdir -p $$@
 endif
 
-$1: $$($1.bin)
+$1: $$($1.bin) build
 ifeq ($B,)
-	@$(TEST_RUN_PREFIX) $$^ -s $(TEST_RUN_POST)
+	@$(TEST_RUN_PREFIX) $$< -s $(TEST_RUN_POST)
 else
-	@cd $B && $(TEST_RUN_PREFIX) $$^ -s $(TEST_RUN_POST)
+	@cd $B && $(TEST_RUN_PREFIX) $$< -s $(TEST_RUN_POST)
 endif
 
 # this runs separate fixtures, targets are of the form dir.name

[m7a]

Thank you very much. Having applied the changes suggested, it succeeds on Linux GCC and Clang :)

It is only sad to see that the coverage report shows all of the functionality as untested due to my use of two separate processes which cover (at least parts) of the code but do not run as part of the test framework. Can their runs somehow be included in the coverage statistics?

[xaizek]

Hm, I think coverage from forks should actually be collected, unless program is not terminated properly (see), but I tried removing prctl() and adding explicit call to __gcov_flush() and it didn't work.

Another thing I've found is that you can't do -std=c99 or activate any strict mode in compiler for handling of forks to work properly (see). I tried putting a break point on __gcov_fork and it didn't fire, which is weird, so it might be this issue. Although I don't think such flags are being passed.

[xaizek]

diff --git a/tests/Makefile b/tests/Makefile
index ecd1a4786..0f05355f3 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -132,6 +132,7 @@ override CFLAGS := $(CFLAGS) -MMD -pipe -Wall -Wno-char-subscripts \
 override LDFLAGS := $(LDFLAGS)
 ifeq (,$(findstring clang,$(CC)))
     # clang is inconvenient with regard to this flag, don't do coverage with it
+    override CFLAGS += --coverage
     override LDFLAGS += --coverage
 endif
 ifdef unix_env

With this __gcov_fork is invoked, but I coverage seems to stay the same.

[xaizek]

diff --git a/tests/registers_shared_memory_tests/registers_shared_memory.c b/tests/registers_shared_memory_tests/registers_shared_memory.c
index 33d8f1d00..ee11cab47 100644
--- a/tests/registers_shared_memory_tests/registers_shared_memory.c
+++ b/tests/registers_shared_memory_tests/registers_shared_memory.c
@@ -103,6 +103,10 @@ static void spawn_regcmd(size_t number)
 		dup2(outpipefd[0], STDIN_FILENO);
 		dup2(inpipefd[1], STDOUT_FILENO);
 		dup2(inpipefd[1], STDERR_FILENO);
+		close(outpipefd[0]);
+		close(outpipefd[1]);
+		close(inpipefd[0]);
+		close(inpipefd[1]);
 #ifdef __linux__
 		prctl(PR_SET_PDEATHSIG, SIGTERM);
 #endif

You didn't close descriptors in the child, this is why it wouldn't finish. I think this stuff with prctl() is unnecessary now.

[m7a]

Thank you very much for the assistance, it is again very much appreciated!

With the suggested changes, it displays coverage correctly now as far as I can tell.

[m7a]

Hello again :)

I have just added another commit which enhances error display in the tests by printing error messages to the standard output (instead of trying to call a GUI function which is not available in the minimal test program). Using that, I got an error message for the failing OSX test which boils down EINVAL upon calling mmap.

Looking for this on the Internet, I found a promising thread at https://lists.apple.com/archives/darwin-kernel/2005/Feb/msg00077.html, but as far as I can tell, the proposed solution (which is basically that one has to ftruncate once before doing the mmap which I already did) is obviously already implemented. I wonder whether the EINVAL is due to Mac OS not supporting the use of mmap with a larger area than allocated by the ftruncate before? I think I will submit another commit which makes these sizes equal and then revert it once I got the results as to see whether it makes a difference...

[m7a]

Yes, that was it https://travis-ci.org/vifm/vifm/builds/366274721 :)

I am still going to revert the change. Would you think that it is a good idea to implement an equal size for shared_mmap_bytes and shared_initial and by means of an #ifdef detecting OSX? I am asking because while the automated test will succeed (and most of its test cases will be irrelevant for being about the resizing :) ) it will still be true that that version has never been tested manually, then...

[xaizek]

Hi, nice that it works on OS X too now :)

It might actually be more than just a quirk of OS X, but something that other systems do too. We can probably assume that it's just OS X for now.

it will still be true that that version has never been tested manually, then...

This is fine, most other changes aren't manually tested on Windows, *BSD, OS X, Haiku, Debian GNU/Hurd, etc. Luckily, things usually just work and those which don't can always be reported, CIs are there just to find out about issues earlier if possible.

[m7a]

It might actually be more than just a quirk of OS X, but something that other systems do too. We can probably assume that it's just OS X for now.

I have added a commit which enables the resizing on Linux only (which I think is the safer approach given I do not know exactly what all the other OSes do :) ).

Also, I have documented my intentions extensively in the comments (not sure if it might not be a bit too verbose, but at least all the related links and design thoughts should now be there....)

Do you think it could be ready for inclusion into VIFM already? Anything else I could do to further improve the pull request?

[xaizek]

Do you think it could be ready for inclusion into VIFM already? Anything else I could do to further improve the pull request?

I think it's pretty much ready. I'll go over the changes and do some stylistic updates and use more of existing functions for the sake of consistency (it will probably take less time to just do these mostly mechanical things than to discuss what needs to be done, so I didn't mention them). And will at least make it compile on Windows before merging changes into master, but probably make it work on Windows too since CreateFileMapping + MapViewOfFile are very much alike shm_open + mmap and I don't expect issues with that.

Thanks for your work on this feature!

[xaizek]

Merged in 7625304, GitHub didn't recognize this as merging because I dropped test commit and commit that reverted it.