Pcapblocking - Githubissues

dmuhamedagic commented 6 years ago

The call to pcap_setnonblock(3) should apparently come only after pcap_activate(3). The man page says:

   When  first activated with pcap_activate() or opened with
   pcap_open_live() , a capture handle is not in  ``non-blocking
   mode'';  a  call to  pcap_setnonblock()  is required in order to
   put it into ``non-blocking'' mode.

Alan-R commented 6 years ago

This seems good, but I'm not certain I agree with using macros instead of local variables. That will require more thought...

On Fri, Apr 27, 2018, at 5:19 AM, Dejan Muhamedagic wrote:

The call to pcap_setnonblock(3) should apparently come only after pcap_activate(3). The man page says:

When first activated with pcap_activate() or opened with pcap_open_live() , a capture handle is not in non-blocking mode''; a call to pcap_setnonblock() is required in order to put it intonon-blocking'' mode.

You can view, comment on, or merge this pull request online at:

https://github.com/assimilation/assimilation-official/pull/60

Commit Summary

Low: clientlib: add more logging

dev: clientlib: add macros for clarification

Medium: clientlib: optimize setting next transmit time

High: clientlib: correct setting pcap non-blocking mode> File Changes

M clientlib/fsprotocol.c[1] (73)

M clientlib/fsqueue.c[2] (6)

M clientlib/packetdecoder.c[3] (6)

M clientlib/pcap_min.c[4] (15)> Patch Links:

https://github.com/assimilation/assimilation-official/pull/60.patch

https://github.com/assimilation/assimilation-official/pull/60.diff> — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub[5], or mute the thread[6].>

-- Alan Robertson alanr@unix.sh

Links:

dmuhamedagic commented 6 years ago

On Fri, Apr 27, 2018 at 05:36:32AM -0700, Alan Robertson wrote:

This seems good, but I'm not certain I agree with using macros instead of local variables. That will require more thought...

The motivation was actually to make the code more readable and, perhaps, easier to understand. Local variables removal was a side effect.

dmuhamedagic commented 6 years ago

I would like to unterse the previous comment and make my intentions clearer by presenting one example:

`fspe->parent->unacked = g_list_prepend(fspe->parent->unacked, fspe);`

The keywords above are g_list_prepend and unacked, but there are no less than eight words which actually mean something that isn't even said. The intention is obviously more directly expressed like this:

`set_pending(fspe);`

Such stuff makes for an unnecessary additional mental activity and sometimes one cannot see the forest for the trees. Reading code is tough. Reading code written by somebody else more so. Let's try to make that task easier.

Alan-R commented 6 years ago

OK. Let's take this example. You use "set_pending".

The word "set" like set_pending typically means to assign to a variable, or set a bit. In the context, would assume that you're setting the pending bit - since pending is a binary state. That's not what's happening at all. With typical functions like set_foo - nothing bad happens if you call it twice. In this case, it would be disaster. So the semantics are totally unlike "set". And, pending - what does that mean? It's a vague word with lots of connotations and lots of meanings - and none specific to the job, terminology, or common language of a protocol. In this case the code is putting the connection into a list of connections with unacknowledged packets. In the context of the protocol, what's important is that it is not yet acknowledged. That's what's important, and the most important thing is lost by using the word pending instead of "unacked". So, the first part "set" is misleading, and the second part "pending" hides what's most important. So, no I don't agree with this - in several respects.

To understand this code it's vital that you know what the glib functions do and the idioms for using them. If you don't, you're going to be screwed coming and going. This is an idiom for using g_list. I'm sorry that using g_list and other glib functions are verbose. But I don't want to write and debug my own. Queues and lists and so on are what this code is about. If you hide that, you lose understanding of what the code is doing. There are likely clearer ways to express what the code is saying, but I don't believe your changes do that. Some initial comments about saying "if you don't know how to use the g_list and g_list idioms you're going to be confused" wouldn't be a bad idea. Maybe even give a few examples in the leading comments. And then a comment here and there to make the intent clearer would be better than obfuscating what's happening - which is what, in my judgment, those changes do. I really appreciate the trouble you've gone to, and very much appreciate the fix you've provided to the real underlying problem, and would gladly merge this branch, if this set of changes was eliminated. I'd love to bring in this branch if this one change weren't in the middle of it.

On Sat, Apr 28, 2018, at 4:48 AM, Dejan Muhamedagic wrote:

I would like to unterse the previous comment and make my intentions clearer by presenting one example:

fspe->parent->unacked = g_list_prepend(fspe->parent->unacked, fspe);

The keywords above are g_list_prepend and unacked, but there are no less than eight words which actually mean something that isn't even said. The intention is obviously more directly expressed like this:

set_pending(fspe);

Such stuff makes for an unnecessary additional mental activity and sometimes one cannot see the forest for the trees. Reading code is tough. Reading code written by somebody else more so. Let's try to make that task easier.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub[1], or mute the thread[2].>

-- Alan Robertson alanr@unix.sh

Links:

dmuhamedagic commented 6 years ago

The changes we're discussing are actually not related to the PR; they got in here by accident. I rectified that now, so this can be merged. I'll make (perhaps) a separate PR for the refactoring part and then we can continue the discussion there.

Alan-R commented 6 years ago

Great job!

assimilation / assimilation-official

Pcapblocking #60