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.

Sign up for GitHub

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

Jump to bottom

New network_connections() function #2495

Closed
wants to merge 2 commits into from
Closed

New network_connections() function #2495

wants to merge 2 commits into from

Conversation

tzz
Copy link
Contributor

@tzz tzz commented Feb 13, 2016

Split off from #2245, requires it before this is merged since it incorporates it.

  • add network_connections() function
  • export some function definitions from unix_iface.c so evalfunction.c can use them
  • acceptance tests!

Implements https://dev.cfengine.com/issues/4520

@tzz tzz mentioned this pull request Feb 13, 2016
Closed
@cfengine-review-bot
Copy link

Can one of the admins verify this patch?

tzz added a commit to tzz/core that referenced this pull request Feb 22, 2016
@tzz
libenv: move leftover code that belongs to cfengine#2495
4e98d3f
@tzz tzz force-pushed the tzz/proc/net/network_connections branch from e77e9df to 1dcfd34 Compare February 22, 2016 10:04
@tzz
Copy link
Contributor Author

tzz commented Feb 22, 2016

Rebased on top of #2245 again.

@tzz tzz force-pushed the tzz/proc/net/network_connections branch 2 times, most recently from c915c71 to 316cd6b Compare February 29, 2016 20:49
@tzz
Copy link
Contributor Author

tzz commented Feb 29, 2016

Rebased on top of #2245 again. Also broke up the old format of IP:PORT for every entry into { address: "IP", port: "PORT" }. Acceptance test updated.

@tzz
Copy link
Contributor Author

tzz commented Feb 29, 2016

@bahamat @kacf the change makes the network connections info easier to use with IPv6, where the : separator can mean port or octet.

@tzz tzz force-pushed the tzz/proc/net/network_connections branch from 316cd6b to f3c122f Compare February 29, 2016 21:43
@tzz
Copy link
Contributor Author

tzz commented Mar 16, 2016

I remembered that this implements https://dev.cfengine.com/issues/4520 so I've mentioned it in the ticket.

tzz added a commit to tzz/core that referenced this pull request Mar 29, 2016
@tzz
libenv: move leftover code that belongs to cfengine#2495
cd746c4
@tzz tzz force-pushed the tzz/proc/net/network_connections branch from f3c122f to 8b851af Compare March 31, 2016 13:24
@tzz
Copy link
Contributor Author

tzz commented Mar 31, 2016

This PR has been rebased after the merge of #2245 and is ready for review.

kacf
kacf reviewed Apr 4, 2016
View reviewed changes
libenv/unix_iface.c
}

BufferDestroy(buf);
JsonObjectRemoveKey(element, raw_key);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could raw_key and new_key theoretically be the same? If so I assume that JsonObjectAppendElement would in fact not append the element, but overwrite it, with it being a map and all (I haven't actually verified this though). Then you should not remove the element afterwards.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, nice catch!

@kacf kacf self-assigned this Apr 5, 2016
@tzz tzz force-pushed the tzz/proc/net/network_connections branch 2 times, most recently from 368a9d1 to e80f77c Compare April 5, 2016 09:55
@tzz
Copy link
Contributor Author

tzz commented Apr 5, 2016

This is fixed, rebased, and ready. The only question I had is if we should put the network connections info under inet -> tcp, inet6 -> tcp, inet -> udp, inet6 -> udp instead of tcp, tcp6, udp, udp6 to be consistent with the sys.inet and sys.inet6 data containers. I'd leave this as is, but maybe @kacf or @bahamat have feedback?

Leaving them as simple top-level keys (the current situation) makes them easier to use with mergedata() so that's an advantage for the users.

@kacf
Copy link
Contributor

kacf commented Apr 6, 2016

I think it's a good idea what you say about putting inet -> tcp, etc. Does it make it more difficult to use with mergedata? How so?

@tzz
Copy link
Contributor Author

tzz commented Apr 6, 2016

mergedata(foo, network_connections()) will put inet -> tcp etc. into foo. If foo has a pre-existing inet key, it will be overwritten since the merge happens only at the top level (this is not optional). So I think we're better off with the current behavior.

@tzz tzz force-pushed the tzz/proc/net/network_connections branch from e80f77c to 299b03f Compare April 6, 2016 09:28
@tzz
Copy link
Contributor Author

tzz commented Apr 6, 2016

@kacf I added test_debug to the tests as you suggested, and this is rebased and ready.

@tzz tzz mentioned this pull request Apr 6, 2016
Merged
@tzz
Copy link
Contributor Author

tzz commented Apr 6, 2016

Documentation is in cfengine/documentation#1443

@kacf
Copy link
Contributor

kacf commented Apr 7, 2016

So I think we're better off with the current behavior.

Ok, either is fine with me.

@kacf
Copy link
Contributor

kacf commented Apr 7, 2016

trigger build

@kacf
Copy link
Contributor

kacf commented Apr 11, 2016

proc-net-functions.cf test fails on all non-Linux test machines. No messages other than the regular FAIL message.

I'll try to look at this, but after #2319.

@tzz
Copy link
Contributor Author

tzz commented Apr 11, 2016

Sorry, I forgot to add the meta tag to skip on non-Linux. Done, this should pass now.

@kacf
Copy link
Contributor

kacf commented Apr 11, 2016

Ah ok, rerunning.

@kacf
Copy link
Contributor

kacf commented Apr 11, 2016

trigger build

@kacf
Copy link
Contributor

kacf commented Apr 15, 2016

Alright, together with https://github.com/cfengine/enterprise/pull/307, this is green.

kacf added a commit that referenced this pull request Apr 15, 2016
@kacf
Merge branch 'networking_connections' (pull request #2495)
5f065b8 
* networking_connections:
  Add network_connections() function that parses /proc/net
@kacf
Copy link
Contributor

kacf commented Apr 15, 2016

Squashed and merged manually. Thanks!

@kacf kacf closed this Apr 15, 2016
@tzz
Copy link
Contributor Author

tzz commented Apr 15, 2016

Woohoo! Epic!

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@kacf kacf

Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tzz @cfengine-review-bot @kacf