Open ctriv opened 9 years ago
Hi Chris,
thank you for this pull request. I like idea about rootless pings.
I have doubts about searching request by data. It might be very long operation. Also it might be good to make subclasses for different kinds of pings as all this ifs looks doubtful.
Regarding searching by data, if you use the dgram socket for icmp the linux kernel rewrites the identifier. I don't see any other way to match the response than to use data. While it is a longer string match than the id, both approaches end up being a linear search of the packet array. Perhaps a hash could speed this up if performance becomes a problem?
As for subclassing, that would be my preference. However, doing this as a subclass would require a large refactoring of the current AnyEvent::Ping class or a large amount of copy-pasted code with minor modifications. After attempting the former, I felt that a few conditionals was a better approach.
Hi, please take a look at https://github.com/und3f/anyevent-ping/tree/decapsulation branch. I think it is more suitable for subclassing now.
I'm on a buisness trip this week so I won't have time to hack on this, but that looks like a good approach. I'll put together another branch using that next week.
chris.
On Mon, Apr 13, 2015 at 7:12 AM, Sergii Zasenko notifications@github.com wrote:
Hi, please take a look at https://github.com/und3f/anyevent-ping/tree/decapsulation branch. I think it is more suitable for subclassing now.
— Reply to this email directly or view it on GitHub https://github.com/und3f/anyevent-ping/pull/11#issuecomment-92369608.
Chris Reinhardt - chris.reinhardt@gmail.com
Great, I'll merge decapsulated changes to master. It might be a good idea to make separated package like AE-Ping-Dgram.
Linux and OS X now support an extension that allows non-root users to ping via dgram icmp sockets. This changeset adds a socket_type option which lets AnyEvent::Ping use dgram sockets.