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

Allow opening counters multiple times #893

Merged
merged 1 commit into from
Apr 26, 2016
Merged

Allow opening counters multiple times #893

merged 1 commit into from
Apr 26, 2016

Conversation

teknico
Copy link
Contributor

@teknico teknico commented Apr 26, 2016

Sometimes, for instance when running tests, it is useful being able to open the same private counter multiple times in the same program.

The underlying memory is not mapped multiple times: the already existing counter is returned.

@wingo wingo self-assigned this Apr 26, 2016
@wingo
Copy link
Contributor

wingo commented Apr 26, 2016

LGTM, though feedback from @eugeneia / @lukego is welcome of course

@wingo
Copy link
Contributor

wingo commented Apr 26, 2016

lgtm!

@wingo wingo merged commit 0b58c44 into snabbco:wingo-next Apr 26, 2016
@teknico teknico deleted the allow-multiple-counter-open branch April 26, 2016 13:15
@lukego
Copy link
Member

lukego commented Apr 27, 2016

Just an observation that now you can open the counters any number of times but the first close will invalidate all of them. Could be error-prone? Would be helpful to see how this is used.

@wingo
Copy link
Contributor

wingo commented Apr 27, 2016

@lukego the use case is in snabb lwaftr check -- Nicola needs to check that on runs of the lwaftr that use pcap files as inputs, that the counters change in the ways that we expect. Since the counters are the public interface, I didn't want to grovel in the app_table.lwaftr state to get them; better to open by name, but they were already opened by the lwaftr...

In practice I don't think anyone is closing counters except via link.free on reconfigure, so it is a moot point practically speaking i think :)

@lukego
Copy link
Member

lukego commented Apr 27, 2016

Thanks for explaining!

takikawa pushed a commit to takikawa/snabb that referenced this pull request Aug 5, 2017
…rsion

Snabb lwAFTR version is Snabb version
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.

3 participants