On Mon, May 13, 2019 at 2:19 AM Clemens Lang <c...@macports.org> wrote:
> Hi Mihir, > > In my experience, it will make it much simpler to review the entire code > if you expand your repository with a README file which gives a (textual) > rough overview of how the code works, possibly with a link to the paper > you implemented. I'd also advise extensively commenting your code to > explain why you do things the way to implemented them, but from a quick > glance, it seems you already did that to a certain extent. > https://github.com/MihirLuthra/sharedMemory I made a readme for the repository just as you suggested. Although if it lacks to be understandable, I will update it accordingly as you say. I have made the code extensively commented now and refactored it to a nice extent I guess. Also when reading code I suggest reading “sharedmemory.h” first and then “sharedmemory.c” will make the comments more understandable. Although this is the first time I coded multi-threaded program, and now I realise that it really takes long to remove a small bug. But now I belief the code is bug free and works completely correct. Two extra things which I plan to do is reference counting for munmap(2) after expanding and utilisation of wasted space in file(which will drop the memory usage to almost half of what it currently is). I have planned for them and probably will do these two in 2-3 days more. > > You can further abstract using a separate SharedMemoryManager object per > thread if you use the pthread API. See for example > __darwintrace_sock_set and sock_key in darwintrace.h and darwintrace.c. > > This should allow you to write a get_memory_manager() function that will > automatically give you a thread-specific SharedMemoryManager object or > create one if none exists for the current thread yet. > > I saw the code in darwintrace.c. Probably yes, I can call insert() and inside it I could call get_memory_manager() which checks thread specific storage for it and it makes the code pretty clean too. Is that the way you mean? Also, this reminded me of one more thing, if I implement search() in open.c, access.c etc. I need not call __darwintrace_setup() right? Like is there any other important thing darwintrace calls do which I need to do separately after search() or can I straight away skip to the end according to search() results? > > It's probably a good idea to move the path normalization to a separate > function, yes. Note that the current does not correctly deal with > directory symlinks. For example, trace mode does not recognize that > > /opt/local/libexec/qt5/include/QtCore/QByteArray > > is a file installed by the qt5-qtbase port, because > > /opt/local/libexec/qt5/include/QtCore > > is a symbolic link to > > /opt/local/libexec/qt5/lib/QtCore.framework/Versions/5/Headers > > Once path lookup is fast, we could actually modify the normalization > code to also check folder symbolic links inside the MacPorts prefix > against the database of ports. > > This would require intertwining the path lookup with path normalization. > Keep that in mind for later, but don't do it now. > > Got it. Will do once this shared memory functionality is complete. > > > > On Wed, May 08, 2019 at 04:24:28PM +0530, Mihir Luthra wrote: > > I figured out the right ways to use my files in code. Can you explain > > me why is it causing these errors.[1] > > > > Just before __darwintrace_is_in_sandbox_check returns path permission, > > I captured it in a bool variable and inserted the normal path in the > > shared file. Each time it prints insertion successfull, so probably I > > feel the extra code I inserted didn’t’ crash, but if you can give me > > some idea what possibly could have gone wrong I maybe able to debug > > it. > > > > [1] https://pastebin.com/1t7a0GJq > > :info:extract sip_copy_proc: > mkdir(/opt/macports-test/var/macports/sip-workaround): No such file or > directory > > We use this directory to create a copy of system binaries that we > wouldn't be able to inject into because they have the magic > "unmodifiable system file" bit introduced by Apple in 10.13. Normally, > typing 'sudo make install' in your macports development copy should have > created this directory (see doc/base.mtree.in), but that doesn't seem to > have happened for you. You should be able to create the directory > manually and chmod it to 1777. > Got it. That error is gone now. > > > > 2) When I install copies with multiple names, i mean I tried with > > prefixes macports-test2, macports-test3 but just as like I sent you > > the log file last time, it actually reads hardcoded “macports-test” > > somewhere which I am unable to understand. > > You'll have to re-run ./configure with a different value for --prefix > for each different installation. If you want to install into multiple > places, it's probably wise to have multiple copies of the source code as > well, otherwise you'll end up recompiling unchanged files very often > during development. > > > > 4) What location I should choose for my temporary shared files in your > > opinion? I currently specified the path to be /tmp/.sharedmemory > > > > Is there any better location within macports directories? > > A path similar to what we use for the trace mode socket might be a good > choice. The template for that socket is > > /tmp/macports-trace-XXXXXX > > see src/port1.0/porttrace.tcl:51 > Got it. Will do the same. Meanwhile till the time you review my code, I will see if I can get the code right into base and add other features to it. Thanks for the help, Mihir
