Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multi ping #109

Open
wants to merge 24 commits into
base: master
Choose a base branch
from
Open

Multi ping #109

wants to merge 24 commits into from

Conversation

mrbesen
Copy link
Contributor

@mrbesen mrbesen commented Feb 3, 2024

I often require to ping multiple hosts. managing 1 or 2 cnping windows is okay, but after that it gets annoying. So i thought why not allow cnping to ping to more than one host at a given time?
So i implemented a simple "multiPing"

sudo ./cnping -p 0.1 -h http://google.com -h 8.8.8.8 -h 1.1.1.1 -h http://example.com

grafik

there are for sure some problems:

  1. i haven't tested windows at all. Would be pure luck if it still compiles for win
  2. there might be still some global variables left, bleeding information from one ping host to another mixing the resolved addresses and other stuff
  3. for a test i disconnected my network an cnping just crashed -> something is not right there.
  4. histogram mode is broken
  5. historical min/max/los is mixed between all views

I just wanted to show you what i have got so far and ask for your opinion on that.
You think its worth to continue with this branch?

What is up with the using_regular_ping variable?
Looks like is either uninitilized or 1. Seems wrong / shady to me.

@dreua
Copy link
Member

dreua commented Feb 3, 2024

Nice, I had the same use case a few times and thought about that. Screenshot looks very cool I don't currently have time to look closer at the code. If you think this variable is shady/wrong I'd guess you're right. 😉

@cnlohr
Copy link
Member

cnlohr commented Feb 5, 2024

This looks pretty cool. It's going to be some work to make this convenient across multiple platforms. But, not insurmountable. I will give up on trying to keep the cnping.exe at 32kB.

@dreua
Copy link
Member

dreua commented Feb 20, 2024

I think there is something off with the makros, this looks like you are including linux headers in a windows build. Haven't looked at the code yet... (I first thought it would be an error in the pipeline and #110 came out of that.)

i686-w64-mingw32-gcc -g -fno-ident -mwindows -m32 -DCNFGOGL -s -Os -I/opt/X11/include -Wall -DVERSION=\""0e51"\" -o cnping.exe cnping.c ping.c httping.c resolve.c resources.o  -lgdi32 -lws2_32 -s -D_WIN32_WINNT=0x0600 -DWIN32 -liphlpapi -lopengl32 -DMINGW_BUILD  -DWIN_USE_NO_ADMIN_PING
In file included from cnping.c:33:
ping.h:11:10: fatal error: netinet/in.h: No such file or directory
   11 | #include <netinet/in.h>
      |          ^~~~~~~~~~~~~~
compilation terminated.

Copy link
Member

@dreua dreua left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can try this locally with make cnping.exe but it needs some cross platform libraries installed.

ping.h Outdated Show resolved Hide resolved
Copy link
Member

@dreua dreua left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The import is resolved but there seems to be a missmatch in agruments and an undefined "psaddr" now. I can reproduce this comiling on Fedora.

@mrbesen
Copy link
Contributor Author

mrbesen commented Feb 21, 2024

undefined "psaddr"

psaddr was a global variable. But it can not be anymore, because it is different for each host. Thats why i moved it into the struct PreparedPing. It might be missing a pointer to the struct PreparedPing in some functions. That is probably what "the missmatch in arguments" is about.

@dreua
Copy link
Member

dreua commented Feb 21, 2024

What surprises me is that it would compile for you but not for me or the ci. Note that our makefile is not be that reliable. Try make clean or even a manual clean of the build files in order to conpile everything from scratch.

@mrbesen
Copy link
Contributor Author

mrbesen commented Feb 21, 2024

I just tried the executeable on Windows11.
And it did work - kinda.

It created an error message, but the ping was running.
I then changed the latency of one of my servers and it did change in the correct ping diagram. So that is an success.

This is the "No Admin privs" Version.

I belive the error is related to the variable static uint8_t listth; of the listener function.

image

i just re-launched the cnping.exe again, but now it has 100% packetloss. But if i launch 2 seperated cnping processes it does still work.

With the "Admin privs" version i get: "Ping send failed: No error (0)".
With multi host ping and single host ping.

@mrbesen
Copy link
Contributor Author

mrbesen commented Feb 21, 2024

What surprises me is that it would compile for you but not for me or the ci.

I belive i had some manual untracked changes in the Makefile for debugging purposes. I restored the original Makefile now and fixed all compile problems i found.

@mrbesen
Copy link
Contributor Author

mrbesen commented Feb 22, 2024

I belive the error is related to the variable static uint8_t listth; of the listener function.

I have now just removed that code block, that just blocked repeated execution of listener().
Why was that code block there? Why would listener be executed more than once? seems odd to me.

Now the "No Admins privs" version works on my win11 machine with multiple ping hosts. But i belive there is still some problems with int ping_failed_to_send; as it might get mixed up between different hosts.

@mrbesen
Copy link
Contributor Author

mrbesen commented Mar 17, 2024

As you can see i have run a test with 17 different hosts (even tho most of them where unreachable).
As you can see in the screenshot a blue bar appears at the bottom. I believe this to be a rounding problem, as its size changes depending on the size of the window.

image

So in this screenshot i have a window height of 593px.

593/17 = 34.882352941 → rounded to int: 34

So we have a height of 34px per host.

593 - (34*17) = 15
So because of the missing fractional part we have 15px "unallocated".

I have 3 plans on how this could be fixed:

  1. Depending on the fractional part choose every x-hosts to have a pixel extra.
    I believe it should be quite accurate with this formula: pingHostId % (1.0/fract(height)) == 0
    This will probably wok but depending on the exact values might still leave a few pixels unused.
  2. Just take the first or last host to take all the extra pixels
    This is probably easier to implement, but might look weird if you have to many hosts and the last one can be up as twice as big as any other. But this can only be a problem if you have many hosts at one (like me with 17 hosts the worst this can be is 16px)
  3. A mix of the both options: try to give every x-hosts some extra pixels, but also keep track of how many pixels are unused to always use the last remaining pixels by the last host. This is probably somewhat complicated to do, as "keeping track" is not that easy the way the sizes are currently calculated.

What do you guys think?

Anyways it might be best to not always calculate that during the draw. Is there an callback from rawdraw, when the window got resized, so we can recalculate those things then?

@dreua
Copy link
Member

dreua commented Mar 17, 2024

I think 2 is just fine. If someone really is unhappy with it and uses it a lot we or they can always iterate on that. Being not perfect at times can also be a feature to draw in new interested contributors. Especially if it is not breaking anything. (I came on board because the background used to be all blue on startup. I made it black.)

In general I'd lean strongly to the KISS principle and prefer stuff breaking in an obvious way or looking unpolished at times very much over making it complex and harder to understand, debug and maintain.

BUT as you are the one investing the time here: Do whatever gives you the most joy, I'm fine with all options you listed. 😉

@dreua
Copy link
Member

dreua commented Mar 17, 2024

Off topic: I heard that blurring is not safe because it can be reversed. Not sure if the blurred data is super sensitive but you may want to keep that in mind.

@mrbesen
Copy link
Contributor Author

mrbesen commented Mar 18, 2024

Off topic: I heard that blurring is not safe because it can be reversed. Not sure if the blurred data is super sensitive but you may want to keep that in mind.

Thanks :) I am aware of that, but i believe that only works on slightly blurred images. But anyway the host names are not super secret as they are not available to the internet anyways. Just not anybody's concern.
And i rather have such things blurred in the screenshot, as colored bars can be mistaken for some rendering artifacts.

@mrbesen
Copy link
Contributor Author

mrbesen commented Mar 19, 2024

I think 2 is just fine. If someone really is unhappy with it and uses it a lot we or they can always iterate on that. Being not perfect at times can also be a feature to draw in new interested contributors. Especially if it is not breaking anything.

I did implement 2 now. Quite simple and easy and works as intended. Even with many hosts you cant really tell, that the last one might have a different size.

I also inverted the order of the hosts, so the first host in the command line is displayed at the top.

(I came on board because the background used to be all blue on startup. I made it black.)

Hehe. true. My first contribution here was the interface -I flag. (Because i wanted to check the internet connectivity over a specific interface)

@dreua
Copy link
Member

dreua commented Mar 20, 2024

Just gave it a quick test and I think this is a great feature! Really makes sense for debugging questions like: "Is my wifi bad, is my router buggy, is my isp not performing, is the destination host just slow?" This answers it all in a glance! And without fiddling around with multiple windows, having to close them again etc etc. For better comparison between ping times I just used the -y parameter, wondering if it would be better if the y scale would automatically be synced for all hosts (i.e. following the host with the hightest ping time). But here we go again polishing stuff 😉

Be sure to remove the "draft" flag when you think its ready!

@mrbesen
Copy link
Contributor Author

mrbesen commented May 9, 2024

i have done some tests now:

  • On my linux machine:

    • everything works as i expect it to.
  • On my win11 machine:

    • the default build with no admin privs does work as expected
    • the build with admin privs does not work. It only shows lost pings but with wireshark i could not see any icmp packets beeing sent. So that does not seem to work. I added some debug prints and sendto returns an expected value of 20. My windows debugging skills are at my limit there.

I have attached my builds (amd64) if someone wants to try for themself.
Maybe some tests with weird combinations of http, icmp, ipv6 and ipv4 ping could be interesting, but i am confident that this will work.
cnping_builds_d961ca92.zip

Even tho the version with admin privileges is broken i believe this PR is "completed" as on the current master this doesn't even compile. So i consider that feature "not made worse" 😄

@mrbesen mrbesen marked this pull request as ready for review May 9, 2024 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants